From 47c4fa85dfa4a27d40c3dcb6988526b0da801511 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Mon, 10 Mar 2025 06:47:52 -0700 Subject: [PATCH] masks the whole URL, update tests --- .../__tests__/artifactTwirpClient.test.ts | 104 +++++++++----- .../internal/shared/artifact-twirp-client.ts | 40 +++--- .../cache/__tests__/cacheTwirpClient.test.ts | 135 +++++++++++------- packages/cache/package-lock.json | 31 +++- packages/cache/package.json | 2 +- .../src/internal/shared/cacheTwirpClient.ts | 43 +++--- 6 files changed, 220 insertions(+), 135 deletions(-) diff --git a/packages/artifact/__tests__/artifactTwirpClient.test.ts b/packages/artifact/__tests__/artifactTwirpClient.test.ts index 882c7ef4..18c7218e 100644 --- a/packages/artifact/__tests__/artifactTwirpClient.test.ts +++ b/packages/artifact/__tests__/artifactTwirpClient.test.ts @@ -1,9 +1,6 @@ import {ArtifactHttpClient} from '../src/internal/shared/artifact-twirp-client' -import {setSecret, debug} from '@actions/core' -import { - CreateArtifactResponse, - GetSignedArtifactURLResponse -} from '../src/generated/results/api/v1/artifact' +import {setSecret} from '@actions/core' +import {CreateArtifactResponse} from '../src/generated/results/api/v1/artifact' jest.mock('@actions/core', () => ({ setSecret: jest.fn(), @@ -26,70 +23,101 @@ describe('ArtifactHttpClient', () => { delete process.env['ACTIONS_RESULTS_URL'] }) + describe('maskSigUrl', () => { + it('should mask the sig parameter and set it as a secret', () => { + const url = + 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' + + const maskedUrl = client.maskSigUrl(url) + + expect(setSecret).toHaveBeenCalledWith('secret-token') + expect(maskedUrl).toBe( + 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) + }) + + it('should return the original URL if no sig parameter is found', () => { + const url = 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z' + + const maskedUrl = client.maskSigUrl(url) + + expect(setSecret).not.toHaveBeenCalled() + expect(maskedUrl).toBe(url) + }) + + it('should handle sig parameter at the end of the URL', () => { + const url = 'https://example.com/upload?param1=value&sig=secret-token' + + const maskedUrl = client.maskSigUrl(url) + + expect(setSecret).toHaveBeenCalledWith('secret-token') + expect(maskedUrl).toBe('https://example.com/upload?param1=value&sig=***') + }) + + it('should handle sig parameter in the middle of the URL', () => { + const url = 'https://example.com/upload?sig=secret-token¶m2=value' + + const maskedUrl = client.maskSigUrl(url) + + expect(setSecret).toHaveBeenCalledWith('secret-token¶m2=value') + expect(maskedUrl).toBe('https://example.com/upload?sig=***') + }) + }) + describe('maskSecretUrls', () => { it('should mask signed_upload_url', () => { - const response: CreateArtifactResponse = { + const spy = jest.spyOn(client, 'maskSigUrl') + const response = { ok: true, - signedUploadUrl: + signed_upload_url: 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' } client.maskSecretUrls(response) - expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(spy).toHaveBeenCalledWith( + 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' ) }) it('should mask signed_download_url', () => { - const response: GetSignedArtifactURLResponse = { - signedUrl: + const spy = jest.spyOn(client, 'maskSigUrl') + const response = { + signed_url: 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' } client.maskSecretUrls(response) - expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(spy).toHaveBeenCalledWith( + 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' ) }) - it('should not call setSecret if URLs are missing', () => { + it('should not call maskSigUrl if URLs are missing', () => { + const spy = jest.spyOn(client, 'maskSigUrl') const response = {} as CreateArtifactResponse client.maskSecretUrls(response) - expect(setSecret).not.toHaveBeenCalled() + expect(spy).not.toHaveBeenCalled() }) - it('should mask only the sensitive token part of signed_upload_url', () => { - const response: CreateArtifactResponse = { - ok: true, - signedUploadUrl: - 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' + it('should handle both URL types when present', () => { + const spy = jest.spyOn(client, 'maskSigUrl') + const response = { + signed_upload_url: 'https://example.com/upload?sig=secret-token1', + signed_url: 'https://example.com/download?sig=secret-token2' } client.maskSecretUrls(response) - expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(spy).toHaveBeenCalledTimes(2) + expect(spy).toHaveBeenCalledWith( + 'https://example.com/upload?sig=secret-token1' ) - }) - - it('should mask only the sensitive token part of signed_download_url', () => { - const response: GetSignedArtifactURLResponse = { - signedUrl: - 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - } - - client.maskSecretUrls(response) - - expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(spy).toHaveBeenCalledWith( + 'https://example.com/download?sig=secret-token2' ) }) }) diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index 2d389114..57a73ad8 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/packages/artifact/src/internal/shared/artifact-twirp-client.ts @@ -5,10 +5,6 @@ import {ArtifactServiceClientJSON} from '../../generated' import {getResultsServiceUrl, getRuntimeToken} from './config' import {getUserAgentString} from './user-agent' import {NetworkError, UsageError} from './errors' -import { - CreateArtifactResponse, - GetSignedArtifactURLResponse -} from '../../generated/results/api/v1/artifact' // The twirp http client must implement this interface interface Rpc { @@ -77,24 +73,32 @@ export class ArtifactHttpClient implements Rpc { /** * Masks the `sig` parameter in a URL and sets it as a secret. * @param url The URL containing the `sig` parameter. - * @param urlType The type of the URL (e.g., 'signed_upload_url', 'signed_download_url'). + * @returns A masked URL where the sig parameter value is replaced with '***' if found, + * or the original URL if no sig parameter is present. */ - maskSigUrl(url: string, urlType: string): void { - const sigMatch = url.match(/[?&]sig=([^&]+)/) - if (sigMatch) { - setSecret(sigMatch[1]) - debug(`Masked ${urlType}: ${url.replace(sigMatch[1], '***')}`) + maskSigUrl(url: string): string { + const sigIndex = url.indexOf('sig=') + if (sigIndex !== -1) { + const sigValue = url.substring(sigIndex + 4) + setSecret(sigValue) + return `${url.substring(0, sigIndex + 4)}***` } + return url } - maskSecretUrls( - body: CreateArtifactResponse | GetSignedArtifactURLResponse - ): void { - if ('signedUploadUrl' in body && body.signedUploadUrl) { - this.maskSigUrl(body.signedUploadUrl, 'signed_upload_url') - } - if ('signedUrl' in body && body.signedUrl) { - this.maskSigUrl(body.signedUrl, 'signed_url') + maskSecretUrls(body): void { + if (typeof body === 'object' && body !== null) { + if ( + 'signed_upload_url' in body && + typeof body.signed_upload_url === 'string' + ) { + this.maskSigUrl(body.signed_upload_url) + } + if ('signed_url' in body && typeof body.signed_url === 'string') { + this.maskSigUrl(body.signed_url) + } + } else { + debug('body is not an object or is null') } } diff --git a/packages/cache/__tests__/cacheTwirpClient.test.ts b/packages/cache/__tests__/cacheTwirpClient.test.ts index e981ce1a..f0cc59f3 100644 --- a/packages/cache/__tests__/cacheTwirpClient.test.ts +++ b/packages/cache/__tests__/cacheTwirpClient.test.ts @@ -1,9 +1,5 @@ -import { - CreateCacheEntryResponse, - GetCacheEntryDownloadURLResponse -} from '../src/generated/results/api/v1/cache' import {CacheServiceClient} from '../src/internal/shared/cacheTwirpClient' -import {setSecret, debug} from '@actions/core' +import {setSecret} from '@actions/core' jest.mock('@actions/core', () => ({ setSecret: jest.fn(), @@ -24,75 +20,106 @@ describe('CacheServiceClient', () => { delete process.env['ACTIONS_RUNTIME_TOKEN'] }) - describe('maskSecretUrls', () => { - it('should mask signedUploadUrl', () => { - const response = { - ok: true, - signedUploadUrl: - 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - } as CreateCacheEntryResponse + describe('maskSigUrl', () => { + it('should mask the sig parameter and set it as a secret', () => { + const url = + 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - client.maskSecretUrls(response) + const maskedUrl = client.maskSigUrl(url) expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(maskedUrl).toBe( + 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' ) }) - it('should mask signedDownloadUrl', () => { - const response = { - ok: true, - signedDownloadUrl: - 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token', - matchedKey: 'cache-key' - } as GetCacheEntryDownloadURLResponse + it('should return the original URL if no sig parameter is found', () => { + const url = 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z' - client.maskSecretUrls(response) - - expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' - ) - }) - - it('should not call setSecret if URLs are missing', () => { - const response = {ok: true} as CreateCacheEntryResponse - - client.maskSecretUrls(response) + const maskedUrl = client.maskSigUrl(url) expect(setSecret).not.toHaveBeenCalled() + expect(maskedUrl).toBe(url) }) - it('should mask only the sensitive token part of signedUploadUrl', () => { - const response = { - ok: true, - signedUploadUrl: - 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - } as CreateCacheEntryResponse + it('should handle sig parameter at the end of the URL', () => { + const url = 'https://example.com/upload?param1=value&sig=secret-token' - client.maskSecretUrls(response) + const maskedUrl = client.maskSigUrl(url) expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(maskedUrl).toBe('https://example.com/upload?param1=value&sig=***') + }) + + it('should handle sig parameter in the middle of the URL', () => { + const url = 'https://example.com/upload?sig=secret-token¶m2=value' + + const maskedUrl = client.maskSigUrl(url) + + expect(setSecret).toHaveBeenCalledWith('secret-token¶m2=value') + expect(maskedUrl).toBe('https://example.com/upload?sig=***') + }) + }) + + describe('maskSecretUrls', () => { + it('should mask signed_upload_url', () => { + const spy = jest.spyOn(client, 'maskSigUrl') + const body = { + signed_upload_url: 'https://example.com/upload?sig=secret-token', + key: 'test-key', + version: 'test-version' + } + + client.maskSecretUrls(body) + + expect(spy).toHaveBeenCalledWith( + 'https://example.com/upload?sig=secret-token' ) }) - it('should mask only the sensitive token part of signedDownloadUrl', () => { - const response = { - ok: true, - signedDownloadUrl: - 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token', - matchedKey: 'cache-key' - } as GetCacheEntryDownloadURLResponse + it('should mask signed_download_url', () => { + const spy = jest.spyOn(client, 'maskSigUrl') + const body = { + signed_download_url: 'https://example.com/download?sig=secret-token', + key: 'test-key', + version: 'test-version' + } - client.maskSecretUrls(response) + client.maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('secret-token') - expect(debug).toHaveBeenCalledWith( - 'Masked signed_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + expect(spy).toHaveBeenCalledWith( + 'https://example.com/download?sig=secret-token' ) }) + + it('should mask both URLs when both are present', () => { + const spy = jest.spyOn(client, 'maskSigUrl') + const body = { + signed_upload_url: 'https://example.com/upload?sig=secret-token1', + signed_download_url: 'https://example.com/download?sig=secret-token2' + } + + client.maskSecretUrls(body) + + expect(spy).toHaveBeenCalledTimes(2) + expect(spy).toHaveBeenCalledWith( + 'https://example.com/upload?sig=secret-token1' + ) + expect(spy).toHaveBeenCalledWith( + 'https://example.com/download?sig=secret-token2' + ) + }) + + it('should not call maskSigUrl when URLs are missing', () => { + const spy = jest.spyOn(client, 'maskSigUrl') + const body = { + key: 'test-key', + version: 'test-version' + } + + client.maskSecretUrls(body) + + expect(spy).not.toHaveBeenCalled() + }) }) }) diff --git a/packages/cache/package-lock.json b/packages/cache/package-lock.json index 3dcc20d9..8d075bbd 100644 --- a/packages/cache/package-lock.json +++ b/packages/cache/package-lock.json @@ -21,6 +21,7 @@ "semver": "^6.3.1" }, "devDependencies": { + "@types/node": "^22.13.9", "@types/semver": "^6.0.0", "typescript": "^5.2.2" } @@ -324,9 +325,13 @@ } }, "node_modules/@types/node": { - "version": "20.4.6", - "resolved": "https://registry.npmjs.org/@types/node/-/node-20.4.6.tgz", - "integrity": "sha512-q0RkvNgMweWWIvSMDiXhflGUKMdIxBo2M2tYM/0kEGDueQByFzK4KZAgu5YHGFNxziTlppNpTIBcqHQAxlfHdA==" + "version": "22.13.9", + "resolved": "https://registry.npmjs.org/@types/node/-/node-22.13.9.tgz", + "integrity": "sha512-acBjXdRJ3A6Pb3tqnw9HZmyR3Fiol3aGxRCK1x3d+6CDAMjl7I649wpSd+yNURCjbOUGu9tqtLKnTGxmK6CyGw==", + "license": "MIT", + "dependencies": { + "undici-types": "~6.20.0" + } }, "node_modules/@types/node-fetch": { "version": "2.6.4", @@ -548,6 +553,12 @@ "node": ">=14.17" } }, + "node_modules/undici-types": { + "version": "6.20.0", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.20.0.tgz", + "integrity": "sha512-Ny6QZ2Nju20vw1SRHe3d9jVu6gJ+4e3+MMpqu7pqE5HT6WsTSlce++GQmK5UXS8mzV8DSYHrQH+Xrf2jVcuKNg==", + "license": "MIT" + }, "node_modules/webidl-conversions": { "version": "3.0.1", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-3.0.1.tgz", @@ -824,9 +835,12 @@ } }, "@types/node": { - "version": "20.4.6", - "resolved": "https://registry.npmjs.org/@types/node/-/node-20.4.6.tgz", - "integrity": "sha512-q0RkvNgMweWWIvSMDiXhflGUKMdIxBo2M2tYM/0kEGDueQByFzK4KZAgu5YHGFNxziTlppNpTIBcqHQAxlfHdA==" + "version": "22.13.9", + "resolved": "https://registry.npmjs.org/@types/node/-/node-22.13.9.tgz", + "integrity": "sha512-acBjXdRJ3A6Pb3tqnw9HZmyR3Fiol3aGxRCK1x3d+6CDAMjl7I649wpSd+yNURCjbOUGu9tqtLKnTGxmK6CyGw==", + "requires": { + "undici-types": "~6.20.0" + } }, "@types/node-fetch": { "version": "2.6.4", @@ -993,6 +1007,11 @@ "integrity": "sha512-mI4WrpHsbCIcwT9cF4FZvr80QUeKvsUsUvKDoR+X/7XHQH98xYD8YHZg7ANtz2GtZt/CBq2QJ0thkGJMHfqc1w==", "dev": true }, + "undici-types": { + "version": "6.20.0", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.20.0.tgz", + "integrity": "sha512-Ny6QZ2Nju20vw1SRHe3d9jVu6gJ+4e3+MMpqu7pqE5HT6WsTSlce++GQmK5UXS8mzV8DSYHrQH+Xrf2jVcuKNg==" + }, "webidl-conversions": { "version": "3.0.1", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-3.0.1.tgz", diff --git a/packages/cache/package.json b/packages/cache/package.json index 4667849d..fcdd7343 100644 --- a/packages/cache/package.json +++ b/packages/cache/package.json @@ -47,10 +47,10 @@ "@azure/ms-rest-js": "^2.6.0", "@azure/storage-blob": "^12.13.0", "@protobuf-ts/plugin": "^2.9.4", - "@types/node": "^22.13.9", "semver": "^6.3.1" }, "devDependencies": { + "@types/node": "^22.13.9", "@types/semver": "^6.0.0", "typescript": "^5.2.2" } diff --git a/packages/cache/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index 2c8acbe2..fb293b20 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/packages/cache/src/internal/shared/cacheTwirpClient.ts @@ -6,10 +6,6 @@ import {getRuntimeToken} from '../cacheUtils' import {BearerCredentialHandler} from '@actions/http-client/lib/auth' import {HttpClient, HttpClientResponse, HttpCodes} from '@actions/http-client' import {CacheServiceClientJSON} from '../../generated/results/api/v1/cache.twirp-client' -import { - CreateCacheEntryResponse, - GetCacheEntryDownloadURLResponse -} from '../../generated/results/api/v1/cache' // The twirp http client must implement this interface interface Rpc { @@ -156,24 +152,35 @@ export class CacheServiceClient implements Rpc { /** * Masks the `sig` parameter in a URL and sets it as a secret. * @param url The URL containing the `sig` parameter. - * @param urlType The type of the URL (e.g., 'signed_upload_url', 'signed_download_url'). + * @returns A masked URL where the sig parameter value is replaced with '***' if found, + * or the original URL if no sig parameter is present. */ - maskSigUrl(url: string, urlType: string): void { - const sigMatch = url.match(/[?&]sig=([^&]+)/) - if (sigMatch) { - setSecret(sigMatch[1]) - debug(`Masked ${urlType}: ${url.replace(sigMatch[1], '***')}`) + maskSigUrl(url: string): string { + const sigIndex = url.indexOf('sig=') + if (sigIndex !== -1) { + const sigValue = url.substring(sigIndex + 4) + setSecret(sigValue) + return `${url.substring(0, sigIndex + 4)}***` } + return url } - maskSecretUrls( - body: CreateCacheEntryResponse | GetCacheEntryDownloadURLResponse - ): void { - if ('signedUploadUrl' in body && body.signedUploadUrl) { - this.maskSigUrl(body.signedUploadUrl, 'signed_upload_url') - } - if ('signedDownloadUrl' in body && body.signedDownloadUrl) { - this.maskSigUrl(body.signedDownloadUrl, 'signed_download_url') + maskSecretUrls(body): void { + if (typeof body === 'object' && body !== null) { + if ( + 'signed_upload_url' in body && + typeof body.signed_upload_url === 'string' + ) { + this.maskSigUrl(body.signed_upload_url) + } + if ( + 'signed_download_url' in body && + typeof body.signed_download_url === 'string' + ) { + this.maskSigUrl(body.signed_download_url) + } + } else { + debug('body is not an object or is null') } }