From 944e6b78db75b17247c1e9d1d46f48a3db5237e7 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 6 Mar 2025 14:25:32 -0800 Subject: [PATCH 01/12] Add secret and signature masking for cache and artifact packages --- .../__tests__/artifactTwirpClient.test.ts | 84 ++++++++++++++++ .../internal/shared/artifact-twirp-client.ts | 20 +++- .../cache/__tests__/cacheTwirpClient.test.ts | 98 +++++++++++++++++++ packages/cache/__tests__/saveCacheV2.test.ts | 6 ++ .../src/internal/shared/cacheTwirpClient.ts | 20 +++- packages/core/src/core.ts | 16 +++ 6 files changed, 240 insertions(+), 4 deletions(-) create mode 100644 packages/artifact/__tests__/artifactTwirpClient.test.ts create mode 100644 packages/cache/__tests__/cacheTwirpClient.test.ts diff --git a/packages/artifact/__tests__/artifactTwirpClient.test.ts b/packages/artifact/__tests__/artifactTwirpClient.test.ts new file mode 100644 index 00000000..035031e1 --- /dev/null +++ b/packages/artifact/__tests__/artifactTwirpClient.test.ts @@ -0,0 +1,84 @@ +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' + +jest.mock('@actions/core', () => ({ + setSecret: jest.fn(), + info: jest.fn(), + debug: jest.fn() +})) + +describe('ArtifactHttpClient', () => { + let client: ArtifactHttpClient + + beforeEach(() => { + jest.clearAllMocks() + process.env['ACTIONS_RUNTIME_TOKEN'] = 'test-token' + process.env['ACTIONS_RESULTS_URL'] = 'https://example.com' + client = new ArtifactHttpClient('test-agent') + }) + + afterEach(() => { + delete process.env['ACTIONS_RUNTIME_TOKEN'] + delete process.env['ACTIONS_RESULTS_URL'] + }) + + describe('maskSecretUrls', () => { + it('should mask signed_upload_url', () => { + const response: CreateArtifactResponse = { + ok: true, + signedUploadUrl: '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=***') + }) + + it('should mask 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_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***') + }) + + it('should not call setSecret if URLs are missing', () => { + const response = {} as CreateArtifactResponse + + client.maskSecretUrls(response) + + expect(setSecret).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' + } + + 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=***') + }) + + 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_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***') + }) + }) +}) diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index 00c65bc7..1d3e8c69 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/packages/artifact/src/internal/shared/artifact-twirp-client.ts @@ -1,10 +1,14 @@ import {HttpClient, HttpClientResponse, HttpCodes} from '@actions/http-client' import {BearerCredentialHandler} from '@actions/http-client/lib/auth' -import {info, debug} from '@actions/core' +import {setSecret, info, debug, maskSigUrl} from '@actions/core' 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 { @@ -16,7 +20,7 @@ interface Rpc { ): Promise } -class ArtifactHttpClient implements Rpc { +export class ArtifactHttpClient implements Rpc { private httpClient: HttpClient private baseUrl: string private maxAttempts = 5 @@ -70,6 +74,17 @@ class ArtifactHttpClient implements Rpc { } } + maskSecretUrls( + body: CreateArtifactResponse | GetSignedArtifactURLResponse + ): void { + if ('signedUploadUrl' in body && body.signedUploadUrl) { + maskSigUrl(body.signedUploadUrl, 'signed_upload_url') + } + if ('signedUrl' in body && body.signedUrl) { + maskSigUrl(body.signedUrl, 'signed_url') + } + } + async retryableRequest( operation: () => Promise ): Promise<{response: HttpClientResponse; body: object}> { @@ -86,6 +101,7 @@ class ArtifactHttpClient implements Rpc { debug(`[Response] - ${response.message.statusCode}`) debug(`Headers: ${JSON.stringify(response.message.headers, null, 2)}`) const body = JSON.parse(rawBody) + this.maskSecretUrls(body) debug(`Body: ${JSON.stringify(body, null, 2)}`) if (this.isSuccessStatusCode(statusCode)) { return {response, body} diff --git a/packages/cache/__tests__/cacheTwirpClient.test.ts b/packages/cache/__tests__/cacheTwirpClient.test.ts new file mode 100644 index 00000000..dd38e8fe --- /dev/null +++ b/packages/cache/__tests__/cacheTwirpClient.test.ts @@ -0,0 +1,98 @@ +import { + CreateCacheEntryResponse, + GetCacheEntryDownloadURLResponse +} from '../src/generated/results/api/v1/cache' +import {CacheServiceClient} from '../src/internal/shared/cacheTwirpClient' +import {setSecret, debug} from '@actions/core' + +jest.mock('@actions/core', () => ({ + setSecret: jest.fn(), + info: jest.fn(), + debug: jest.fn() +})) + +describe('CacheServiceClient', () => { + let client: CacheServiceClient + + beforeEach(() => { + jest.clearAllMocks() + process.env['ACTIONS_RUNTIME_TOKEN'] = 'test-token' // <-- set the required env variable + client = new CacheServiceClient('test-agent') + }) + + afterEach(() => { + delete process.env['ACTIONS_RUNTIME_TOKEN'] // <-- clean up after tests + }) + + 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 + + client.maskSecretUrls(response) + + expect(setSecret).toHaveBeenCalledWith('secret-token') + expect(debug).toHaveBeenCalledWith( + 'Masked signedUploadUrl: 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 + + client.maskSecretUrls(response) + + expect(setSecret).toHaveBeenCalledWith('secret-token') + expect(debug).toHaveBeenCalledWith( + 'Masked signedDownloadUrl: 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) + + expect(setSecret).not.toHaveBeenCalled() + }) + + 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 + + client.maskSecretUrls(response) + + expect(setSecret).toHaveBeenCalledWith('secret-token') + expect(debug).toHaveBeenCalledWith( + 'Masked signedUploadUrl: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) + }) + + 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 + + client.maskSecretUrls(response) + + expect(setSecret).toHaveBeenCalledWith('secret-token') + expect(debug).toHaveBeenCalledWith( + 'Masked signedDownloadUrl: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) + }) + }) +}) diff --git a/packages/cache/__tests__/saveCacheV2.test.ts b/packages/cache/__tests__/saveCacheV2.test.ts index e96c2ac9..fed2c2a4 100644 --- a/packages/cache/__tests__/saveCacheV2.test.ts +++ b/packages/cache/__tests__/saveCacheV2.test.ts @@ -8,10 +8,16 @@ import * as tar from '../src/internal/tar' import {CacheServiceClientJSON} from '../src/generated/results/api/v1/cache.twirp-client' import * as cacheHttpClient from '../src/internal/cacheHttpClient' import {UploadOptions} from '../src/options' +import { + CreateCacheEntryResponse, + GetCacheEntryDownloadURLResponse +} from '../src/generated/results/api/v1/cache' +import {CacheServiceClient} from '../src/internal/shared/cacheTwirpClient' let logDebugMock: jest.SpyInstance jest.mock('../src/internal/tar') +jest.mock('@actions/core') const uploadFileMock = jest.fn() const blockBlobClientMock = jest.fn().mockImplementation(() => ({ diff --git a/packages/cache/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index 69d9a8fc..9c121121 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/packages/cache/src/internal/shared/cacheTwirpClient.ts @@ -1,4 +1,4 @@ -import {info, debug} from '@actions/core' +import {info, debug, maskSigUrl} from '@actions/core' import {getUserAgentString} from './user-agent' import {NetworkError, UsageError} from './errors' import {getCacheServiceURL} from '../config' @@ -6,6 +6,10 @@ 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 { @@ -24,7 +28,7 @@ interface Rpc { * * This class is used to interact with cache service v2. */ -class CacheServiceClient implements Rpc { +export class CacheServiceClient implements Rpc { private httpClient: HttpClient private baseUrl: string private maxAttempts = 5 @@ -94,6 +98,7 @@ class CacheServiceClient implements Rpc { debug(`[Response] - ${response.message.statusCode}`) debug(`Headers: ${JSON.stringify(response.message.headers, null, 2)}`) const body = JSON.parse(rawBody) + this.maskSecretUrls(body) debug(`Body: ${JSON.stringify(body, null, 2)}`) if (this.isSuccessStatusCode(statusCode)) { return {response, body} @@ -148,6 +153,17 @@ class CacheServiceClient implements Rpc { throw new Error(`Request failed`) } + maskSecretUrls( + body: CreateCacheEntryResponse | GetCacheEntryDownloadURLResponse + ): void { + if ('signedUploadUrl' in body && body.signedUploadUrl) { + maskSigUrl(body.signedUploadUrl, 'signedUploadUrl') + } + if ('signedDownloadUrl' in body && body.signedDownloadUrl) { + maskSigUrl(body.signedDownloadUrl, 'signedDownloadUrl') + } + } + isSuccessStatusCode(statusCode?: number): boolean { if (!statusCode) return false return statusCode >= 200 && statusCode < 300 diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index 0a141693..12359d0a 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -391,3 +391,19 @@ export {toPosixPath, toWin32Path, toPlatformPath} from './path-utils' * Platform utilities exports */ export * as platform from './platform' + +/** + * 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 The URL with the `sig` parameter masked. + */ +export function maskSigUrl(url: string, urlType: string): string { + const sigMatch = url.match(/[?&]sig=([^&]+)/) + if (sigMatch) { + setSecret(sigMatch[1]) + debug(`Masked ${urlType}: ${url.replace(sigMatch[1], '***')}`) + return url.replace(sigMatch[1], '***') + } + return url +} From 884aa17886e000ca29bd76ddd27ce2d00f96bafd Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 6 Mar 2025 14:31:21 -0800 Subject: [PATCH 02/12] remove these changes --- packages/cache/__tests__/saveCacheV2.test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/cache/__tests__/saveCacheV2.test.ts b/packages/cache/__tests__/saveCacheV2.test.ts index fed2c2a4..e96c2ac9 100644 --- a/packages/cache/__tests__/saveCacheV2.test.ts +++ b/packages/cache/__tests__/saveCacheV2.test.ts @@ -8,16 +8,10 @@ import * as tar from '../src/internal/tar' import {CacheServiceClientJSON} from '../src/generated/results/api/v1/cache.twirp-client' import * as cacheHttpClient from '../src/internal/cacheHttpClient' import {UploadOptions} from '../src/options' -import { - CreateCacheEntryResponse, - GetCacheEntryDownloadURLResponse -} from '../src/generated/results/api/v1/cache' -import {CacheServiceClient} from '../src/internal/shared/cacheTwirpClient' let logDebugMock: jest.SpyInstance jest.mock('../src/internal/tar') -jest.mock('@actions/core') const uploadFileMock = jest.fn() const blockBlobClientMock = jest.fn().mockImplementation(() => ({ From 1cd2f8a53893399b03203fe50f440e66309b591e Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 7 Mar 2025 06:01:25 -0800 Subject: [PATCH 03/12] Instead of using utility method in core lib, use method in both twirp clients --- .../__tests__/artifactTwirpClient.test.ts | 28 +++++++++++++------ .../internal/shared/artifact-twirp-client.ts | 19 +++++++++++-- .../cache/__tests__/cacheTwirpClient.test.ts | 12 ++++---- packages/cache/package.json | 4 ++- .../src/internal/shared/cacheTwirpClient.ts | 19 +++++++++++-- packages/core/src/core.ts | 16 ----------- 6 files changed, 61 insertions(+), 37 deletions(-) diff --git a/packages/artifact/__tests__/artifactTwirpClient.test.ts b/packages/artifact/__tests__/artifactTwirpClient.test.ts index 035031e1..882c7ef4 100644 --- a/packages/artifact/__tests__/artifactTwirpClient.test.ts +++ b/packages/artifact/__tests__/artifactTwirpClient.test.ts @@ -30,24 +30,30 @@ describe('ArtifactHttpClient', () => { it('should mask signed_upload_url', () => { const response: CreateArtifactResponse = { ok: true, - signedUploadUrl: 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' + signedUploadUrl: + '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(debug).toHaveBeenCalledWith( + 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) }) it('should mask signed_download_url', () => { const response: GetSignedArtifactURLResponse = { - signedUrl: 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' + 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_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***') + expect(debug).toHaveBeenCalledWith( + 'Masked signed_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) }) it('should not call setSecret if URLs are missing', () => { @@ -61,24 +67,30 @@ describe('ArtifactHttpClient', () => { 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' + signedUploadUrl: + '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(debug).toHaveBeenCalledWith( + 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) }) 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' + 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_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***') + expect(debug).toHaveBeenCalledWith( + 'Masked signed_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + ) }) }) }) diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index 1d3e8c69..2d389114 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/packages/artifact/src/internal/shared/artifact-twirp-client.ts @@ -1,6 +1,6 @@ import {HttpClient, HttpClientResponse, HttpCodes} from '@actions/http-client' import {BearerCredentialHandler} from '@actions/http-client/lib/auth' -import {setSecret, info, debug, maskSigUrl} from '@actions/core' +import {setSecret, info, debug} from '@actions/core' import {ArtifactServiceClientJSON} from '../../generated' import {getResultsServiceUrl, getRuntimeToken} from './config' import {getUserAgentString} from './user-agent' @@ -74,14 +74,27 @@ 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'). + */ + maskSigUrl(url: string, urlType: string): void { + const sigMatch = url.match(/[?&]sig=([^&]+)/) + if (sigMatch) { + setSecret(sigMatch[1]) + debug(`Masked ${urlType}: ${url.replace(sigMatch[1], '***')}`) + } + } + maskSecretUrls( body: CreateArtifactResponse | GetSignedArtifactURLResponse ): void { if ('signedUploadUrl' in body && body.signedUploadUrl) { - maskSigUrl(body.signedUploadUrl, 'signed_upload_url') + this.maskSigUrl(body.signedUploadUrl, 'signed_upload_url') } if ('signedUrl' in body && body.signedUrl) { - maskSigUrl(body.signedUrl, 'signed_url') + this.maskSigUrl(body.signedUrl, 'signed_url') } } diff --git a/packages/cache/__tests__/cacheTwirpClient.test.ts b/packages/cache/__tests__/cacheTwirpClient.test.ts index dd38e8fe..e981ce1a 100644 --- a/packages/cache/__tests__/cacheTwirpClient.test.ts +++ b/packages/cache/__tests__/cacheTwirpClient.test.ts @@ -16,12 +16,12 @@ describe('CacheServiceClient', () => { beforeEach(() => { jest.clearAllMocks() - process.env['ACTIONS_RUNTIME_TOKEN'] = 'test-token' // <-- set the required env variable + process.env['ACTIONS_RUNTIME_TOKEN'] = 'test-token' client = new CacheServiceClient('test-agent') }) afterEach(() => { - delete process.env['ACTIONS_RUNTIME_TOKEN'] // <-- clean up after tests + delete process.env['ACTIONS_RUNTIME_TOKEN'] }) describe('maskSecretUrls', () => { @@ -36,7 +36,7 @@ describe('CacheServiceClient', () => { expect(setSecret).toHaveBeenCalledWith('secret-token') expect(debug).toHaveBeenCalledWith( - 'Masked signedUploadUrl: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' ) }) @@ -52,7 +52,7 @@ describe('CacheServiceClient', () => { expect(setSecret).toHaveBeenCalledWith('secret-token') expect(debug).toHaveBeenCalledWith( - 'Masked signedDownloadUrl: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + 'Masked signed_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' ) }) @@ -75,7 +75,7 @@ describe('CacheServiceClient', () => { expect(setSecret).toHaveBeenCalledWith('secret-token') expect(debug).toHaveBeenCalledWith( - 'Masked signedUploadUrl: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' + 'Masked signed_upload_url: https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=***' ) }) @@ -91,7 +91,7 @@ describe('CacheServiceClient', () => { expect(setSecret).toHaveBeenCalledWith('secret-token') expect(debug).toHaveBeenCalledWith( - 'Masked signedDownloadUrl: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' + 'Masked signed_download_url: https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=***' ) }) }) diff --git a/packages/cache/package.json b/packages/cache/package.json index 3a200f89..4667849d 100644 --- a/packages/cache/package.json +++ b/packages/cache/package.json @@ -31,7 +31,8 @@ "scripts": { "audit-moderate": "npm install && npm audit --json --audit-level=moderate > audit.json", "test": "echo \"Error: run tests from root\" && exit 1", - "tsc": "tsc" + "tsc": "tsc", + "clean": "rm -rf node_modules lib" }, "bugs": { "url": "https://github.com/actions/toolkit/issues" @@ -46,6 +47,7 @@ "@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": { diff --git a/packages/cache/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index 9c121121..2c8acbe2 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/packages/cache/src/internal/shared/cacheTwirpClient.ts @@ -1,4 +1,4 @@ -import {info, debug, maskSigUrl} from '@actions/core' +import {info, debug, setSecret} from '@actions/core' import {getUserAgentString} from './user-agent' import {NetworkError, UsageError} from './errors' import {getCacheServiceURL} from '../config' @@ -153,14 +153,27 @@ export class CacheServiceClient implements Rpc { throw new Error(`Request failed`) } + /** + * 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'). + */ + maskSigUrl(url: string, urlType: string): void { + const sigMatch = url.match(/[?&]sig=([^&]+)/) + if (sigMatch) { + setSecret(sigMatch[1]) + debug(`Masked ${urlType}: ${url.replace(sigMatch[1], '***')}`) + } + } + maskSecretUrls( body: CreateCacheEntryResponse | GetCacheEntryDownloadURLResponse ): void { if ('signedUploadUrl' in body && body.signedUploadUrl) { - maskSigUrl(body.signedUploadUrl, 'signedUploadUrl') + this.maskSigUrl(body.signedUploadUrl, 'signed_upload_url') } if ('signedDownloadUrl' in body && body.signedDownloadUrl) { - maskSigUrl(body.signedDownloadUrl, 'signedDownloadUrl') + this.maskSigUrl(body.signedDownloadUrl, 'signed_download_url') } } diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index 12359d0a..0a141693 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -391,19 +391,3 @@ export {toPosixPath, toWin32Path, toPlatformPath} from './path-utils' * Platform utilities exports */ export * as platform from './platform' - -/** - * 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 The URL with the `sig` parameter masked. - */ -export function maskSigUrl(url: string, urlType: string): string { - const sigMatch = url.match(/[?&]sig=([^&]+)/) - if (sigMatch) { - setSecret(sigMatch[1]) - debug(`Masked ${urlType}: ${url.replace(sigMatch[1], '***')}`) - return url.replace(sigMatch[1], '***') - } - return url -} From 47c4fa85dfa4a27d40c3dcb6988526b0da801511 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Mon, 10 Mar 2025 06:47:52 -0700 Subject: [PATCH 04/12] 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') } } From 5007821c77036db7ca9919a95dae207313ada173 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Mon, 10 Mar 2025 06:51:30 -0700 Subject: [PATCH 05/12] Remove clean script --- packages/cache/package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/cache/package.json b/packages/cache/package.json index fcdd7343..9b8b0ac6 100644 --- a/packages/cache/package.json +++ b/packages/cache/package.json @@ -31,8 +31,7 @@ "scripts": { "audit-moderate": "npm install && npm audit --json --audit-level=moderate > audit.json", "test": "echo \"Error: run tests from root\" && exit 1", - "tsc": "tsc", - "clean": "rm -rf node_modules lib" + "tsc": "tsc" }, "bugs": { "url": "https://github.com/actions/toolkit/issues" From 3ac34ffcb7cbe45e9ca22ce26e0210ca0b6e711e Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Wed, 12 Mar 2025 03:17:35 -0700 Subject: [PATCH 06/12] Mask different situations, malformed URL, encoded, decoded, raw signatures, nested parameters, and moved to a utility file --- .../__tests__/artifactTwirpClient.test.ts | 124 -------- packages/artifact/__tests__/util.test.ts | 289 ++++++++++++++++++ .../internal/shared/artifact-twirp-client.ts | 37 +-- packages/artifact/src/internal/shared/util.ts | 168 ++++++++++ .../cache/__tests__/cacheTwirpClient.test.ts | 125 -------- packages/cache/__tests__/util.test.ts | 289 ++++++++++++++++++ .../src/internal/shared/cacheTwirpClient.ts | 40 +-- packages/cache/src/internal/shared/util.ts | 171 +++++++++++ 8 files changed, 923 insertions(+), 320 deletions(-) delete mode 100644 packages/artifact/__tests__/artifactTwirpClient.test.ts delete mode 100644 packages/cache/__tests__/cacheTwirpClient.test.ts create mode 100644 packages/cache/__tests__/util.test.ts create mode 100644 packages/cache/src/internal/shared/util.ts diff --git a/packages/artifact/__tests__/artifactTwirpClient.test.ts b/packages/artifact/__tests__/artifactTwirpClient.test.ts deleted file mode 100644 index 18c7218e..00000000 --- a/packages/artifact/__tests__/artifactTwirpClient.test.ts +++ /dev/null @@ -1,124 +0,0 @@ -import {ArtifactHttpClient} from '../src/internal/shared/artifact-twirp-client' -import {setSecret} from '@actions/core' -import {CreateArtifactResponse} from '../src/generated/results/api/v1/artifact' - -jest.mock('@actions/core', () => ({ - setSecret: jest.fn(), - info: jest.fn(), - debug: jest.fn() -})) - -describe('ArtifactHttpClient', () => { - let client: ArtifactHttpClient - - beforeEach(() => { - jest.clearAllMocks() - process.env['ACTIONS_RUNTIME_TOKEN'] = 'test-token' - process.env['ACTIONS_RESULTS_URL'] = 'https://example.com' - client = new ArtifactHttpClient('test-agent') - }) - - afterEach(() => { - delete process.env['ACTIONS_RUNTIME_TOKEN'] - 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 spy = jest.spyOn(client, 'maskSigUrl') - const response = { - ok: true, - signed_upload_url: - 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - } - - client.maskSecretUrls(response) - - expect(spy).toHaveBeenCalledWith( - 'https://example.com/upload?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - ) - }) - - it('should mask signed_download_url', () => { - 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(spy).toHaveBeenCalledWith( - 'https://example.com/download?se=2025-03-05T16%3A47%3A23Z&sig=secret-token' - ) - }) - - it('should not call maskSigUrl if URLs are missing', () => { - const spy = jest.spyOn(client, 'maskSigUrl') - const response = {} as CreateArtifactResponse - - client.maskSecretUrls(response) - - expect(spy).not.toHaveBeenCalled() - }) - - 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(spy).toHaveBeenCalledTimes(2) - expect(spy).toHaveBeenCalledWith( - 'https://example.com/upload?sig=secret-token1' - ) - expect(spy).toHaveBeenCalledWith( - 'https://example.com/download?sig=secret-token2' - ) - }) - }) -}) diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 76fe4e18..34535db2 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -1,5 +1,7 @@ import * as config from '../src/internal/shared/config' import * as util from '../src/internal/shared/util' +import {maskSigUrl, maskSecretUrls} from '../src/internal/shared/util' +import {setSecret, debug} from '@actions/core' export const testRuntimeToken = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwic2NwIjoiQWN0aW9ucy5FeGFtcGxlIEFjdGlvbnMuQW5vdGhlckV4YW1wbGU6dGVzdCBBY3Rpb25zLlJlc3VsdHM6Y2U3ZjU0YzctNjFjNy00YWFlLTg4N2YtMzBkYTQ3NWY1ZjFhOmNhMzk1MDg1LTA0MGEtNTI2Yi0yY2U4LWJkYzg1ZjY5Mjc3NCIsImlhdCI6MTUxNjIzOTAyMn0.XYnI_wHPBlUi1mqYveJnnkJhp4dlFjqxzRmISPsqfw8' @@ -59,3 +61,290 @@ describe('get-backend-ids-from-token', () => { ) }) }) + +jest.mock('@actions/core') + +describe('maskSigUrl', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('masks the sig parameter in the URL and sets it as a secret', () => { + const url = 'https://example.com?sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('returns the original URL if no sig parameter is present', () => { + const url = 'https://example.com' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe(url) + expect(setSecret).not.toHaveBeenCalled() + }) + + it('masks the sig parameter in the middle of the URL and sets it as a secret', () => { + const url = 'https://example.com?param1=value1&sig=12345¶m2=value2' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe( + 'https://example.com/?param1=value1&sig=***¶m2=value2' + ) + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('returns the original URL if it is empty', () => { + const url = '' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('') + expect(setSecret).not.toHaveBeenCalled() + }) + + it('handles URLs with special characters in signature', () => { + const url = 'https://example.com?sig=abc/+=%3D' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***') + + expect(setSecret).toHaveBeenCalledWith('abc/+') + expect(setSecret).toHaveBeenCalledWith('abc/ ==') + expect(setSecret).toHaveBeenCalledWith('abc%2F%20%3D%3D') + }) + + it('handles relative URLs', () => { + const url = '/path?param=value&sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/path?param=value&sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with uppercase SIG parameter', () => { + const url = 'https://example.com?SIG=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?SIG=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles malformed URLs using regex fallback', () => { + const url = 'not:a:valid:url:but:has:sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('not:a:valid:url:but:has:sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + }) + + it('handles URLs with fragments', () => { + const url = 'https://example.com?sig=12345#fragment' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***#fragment') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with Sig parameter (first letter uppercase)', () => { + const url = 'https://example.com?Sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?Sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with sIg parameter (middle letter uppercase)', () => { + const url = 'https://example.com?sIg=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sIg=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with siG parameter (last letter uppercase)', () => { + const url = 'https://example.com?siG=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?siG=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with mixed case sig parameters in the same URL', () => { + const url = 'https://example.com?sig=123&SIG=456&Sig=789' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***&SIG=***&Sig=***') + expect(setSecret).toHaveBeenCalledWith('123') + expect(setSecret).toHaveBeenCalledWith('456') + expect(setSecret).toHaveBeenCalledWith('789') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('123')) + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('456')) + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('789')) + }) + + it('handles malformed URLs with different sig case variations', () => { + const url = 'not:a:valid:url:but:has:Sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('not:a:valid:url:but:has:Sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + }) + + it('handles malformed URLs with uppercase SIG in irregular formats', () => { + const url = 'something&SIG=12345&other:stuff' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('something&SIG=***&other:stuff') + expect(setSecret).toHaveBeenCalledWith('12345') + }) + + it('handles sig parameter at the start of the query string', () => { + const url = 'https://example.com?sig=12345¶m=value' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***¶m=value') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles sig parameter at the end of the query string', () => { + const url = 'https://example.com?param=value&sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?param=value&sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) +}) + +describe('maskSecretUrls', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('masks sig parameters in signed_upload_url and signed_url', () => { + const body = { + signed_upload_url: 'https://upload.com?sig=upload123', + signed_url: 'https://download.com?sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) + expect(setSecret).toHaveBeenCalledWith('download123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) + }) + + it('handles case where only upload_url is present', () => { + const body = { + signed_upload_url: 'https://upload.com?sig=upload123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) + }) + + it('handles case where only download_url is present', () => { + const body = { + signed_url: 'https://download.com?sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('download123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) + }) + + it('handles case where URLs do not contain sig parameters', () => { + const body = { + signed_upload_url: 'https://upload.com?token=abc', + signed_url: 'https://download.com?token=xyz' + } + maskSecretUrls(body) + expect(setSecret).not.toHaveBeenCalled() + }) + + it('handles empty string URLs', () => { + const body = { + signed_upload_url: '', + signed_url: '' + } + maskSecretUrls(body) + expect(setSecret).not.toHaveBeenCalled() + }) + + it('handles malformed URLs in the body', () => { + const body = { + signed_upload_url: 'not:a:valid:url:but:has:sig=upload123', + signed_url: 'also:not:valid:with:sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith('download123') + }) + + it('handles URLs with different case variations of sig parameter', () => { + const body = { + signed_upload_url: 'https://upload.com?SIG=upload123', + signed_url: 'https://download.com?Sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) + expect(setSecret).toHaveBeenCalledWith('download123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) + }) + + it('handles URLs with special characters in signature', () => { + const specialSig = 'xyz!@#$' + const encodedSpecialSig = encodeURIComponent(specialSig) + + const body = { + signed_upload_url: 'https://upload.com?sig=abc/+=%3D', + signed_url: `https://download.com?sig=${encodedSpecialSig}` + } + maskSecretUrls(body) + + const allCalls = (setSecret as jest.Mock).mock.calls.flat() + + expect(allCalls).toContain('abc/+') + expect(allCalls).toContain('abc/ ==') + expect(allCalls).toContain('abc%2F%20%3D%3D') + + expect(allCalls).toContain(specialSig) + }) + + it('handles deeply nested URLs in the body', () => { + const body = { + data: { + urls: { + signed_upload_url: 'https://upload.com?sig=nested123', + signed_url: 'https://download.com?sig=nested456' + } + } + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('nested123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested123')) + expect(setSecret).toHaveBeenCalledWith('nested456') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested456')) + }) + + it('handles arrays of URLs in the body', () => { + const body = { + signed_urls: [ + 'https://first.com?sig=first123', + 'https://second.com?sig=second456' + ] + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('first123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('first123')) + expect(setSecret).toHaveBeenCalledWith('second456') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('second456')) + }) + + it('does nothing if body is not an object or is null', () => { + maskSecretUrls(null) + expect(debug).toHaveBeenCalledWith('body is not an object or is null') + expect(setSecret).not.toHaveBeenCalled() + }) + + it('does nothing if signed_upload_url and signed_url are not strings', () => { + const body = { + signed_upload_url: 123, + signed_url: 456 + } + maskSecretUrls(body) + expect(setSecret).not.toHaveBeenCalled() + }) +}) diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index 57a73ad8..84461518 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/packages/artifact/src/internal/shared/artifact-twirp-client.ts @@ -1,10 +1,11 @@ import {HttpClient, HttpClientResponse, HttpCodes} from '@actions/http-client' import {BearerCredentialHandler} from '@actions/http-client/lib/auth' -import {setSecret, info, debug} from '@actions/core' +import {info, debug} from '@actions/core' import {ArtifactServiceClientJSON} from '../../generated' import {getResultsServiceUrl, getRuntimeToken} from './config' import {getUserAgentString} from './user-agent' import {NetworkError, UsageError} from './errors' +import {maskSecretUrls} from './util' // The twirp http client must implement this interface interface Rpc { @@ -70,38 +71,6 @@ 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. - * @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): 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): 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') - } - } - async retryableRequest( operation: () => Promise ): Promise<{response: HttpClientResponse; body: object}> { @@ -118,7 +87,7 @@ export class ArtifactHttpClient implements Rpc { debug(`[Response] - ${response.message.statusCode}`) debug(`Headers: ${JSON.stringify(response.message.headers, null, 2)}`) const body = JSON.parse(rawBody) - this.maskSecretUrls(body) + maskSecretUrls(body) debug(`Body: ${JSON.stringify(body, null, 2)}`) if (this.isSuccessStatusCode(statusCode)) { return {response, body} diff --git a/packages/artifact/src/internal/shared/util.ts b/packages/artifact/src/internal/shared/util.ts index 07392b36..754c6b76 100644 --- a/packages/artifact/src/internal/shared/util.ts +++ b/packages/artifact/src/internal/shared/util.ts @@ -1,6 +1,7 @@ import * as core from '@actions/core' import {getRuntimeToken} from './config' import jwt_decode from 'jwt-decode' +import {debug, setSecret} from '@actions/core' export interface BackendIds { workflowRunBackendId: string @@ -69,3 +70,170 @@ export function getBackendIdsFromToken(): BackendIds { throw InvalidJwtError } + +/** + * Masks the `sig` parameter in a URL and sets it as a secret. + * @param url The URL containing the `sig` parameter. + * @returns A masked URL where the sig parameter value is replaced with '***' if found, + * or the original URL if no sig parameter is present. + */ +export function maskSigUrl(url: string): string { + if (!url) return url + + try { + const rawSigRegex = /[?&](sig)=([^&=#]+)/gi + let match + + while ((match = rawSigRegex.exec(url)) !== null) { + const rawSignature = match[2] + if (rawSignature) { + setSecret(rawSignature) + } + } + + let parsedUrl: URL + try { + parsedUrl = new URL(url) + } catch { + try { + parsedUrl = new URL(url, 'https://example.com') + } catch (error) { + debug(`Failed to parse URL: ${url}`) + return maskSigWithRegex(url) + } + } + + let masked = false + const paramNames = Array.from(parsedUrl.searchParams.keys()) + + for (const paramName of paramNames) { + if (paramName.toLowerCase() === 'sig') { + const signature = parsedUrl.searchParams.get(paramName) + if (signature) { + setSecret(signature) + setSecret(encodeURIComponent(signature)) + parsedUrl.searchParams.set(paramName, '***') + masked = true + } + } + } + if (masked) { + return parsedUrl.toString() + } + + if (/([:?&]|^)(sig)=([^&=#]+)/i.test(url)) { + return maskSigWithRegex(url) + } + } catch (error) { + debug( + `Error masking URL: ${ + error instanceof Error ? error.message : String(error) + }` + ) + return maskSigWithRegex(url) + } + + return url +} + +/** + * Fallback method to mask signatures using regex when URL parsing fails + */ +function maskSigWithRegex(url: string): string { + try { + const regex = /([:?&]|^)(sig)=([^&=#]+)/gi + + return url.replace(regex, (fullMatch, prefix, paramName, value) => { + if (value) { + setSecret(value) + try { + setSecret(decodeURIComponent(value)) + } catch { + // Ignore decoding errors + } + return `${prefix}${paramName}=***` + } + return fullMatch + }) + } catch (error) { + debug( + `Error in maskSigWithRegex: ${ + error instanceof Error ? error.message : String(error) + }` + ) + return url + } +} + +/** + * Masks any URLs containing signature parameters in the provided object + * Recursively searches through nested objects and arrays + */ +export function maskSecretUrls( + body: Record | unknown[] | null +): void { + if (typeof body !== 'object' || body === null) { + debug('body is not an object or is null') + return + } + + type NestedValue = + | string + | number + | boolean + | null + | undefined + | NestedObject + | NestedArray + interface NestedObject { + [key: string]: NestedValue + signed_upload_url?: string + signed_url?: string + } + type NestedArray = NestedValue[] + + const processUrl = (url: string): void => { + maskSigUrl(url) + } + + const processObject = ( + obj: Record | NestedValue[] + ): void => { + if (typeof obj !== 'object' || obj === null) { + return + } + + if (Array.isArray(obj)) { + for (const item of obj) { + if (typeof item === 'string') { + processUrl(item) + } else if (typeof item === 'object' && item !== null) { + processObject(item as Record | NestedValue[]) + } + } + return + } + + if ( + 'signed_upload_url' in obj && + typeof obj.signed_upload_url === 'string' + ) { + maskSigUrl(obj.signed_upload_url) + } + if ('signed_url' in obj && typeof obj.signed_url === 'string') { + maskSigUrl(obj.signed_url) + } + + for (const key in obj) { + const value = obj[key] + if (typeof value === 'string') { + if (/([:?&]|^)(sig)=/i.test(value)) { + maskSigUrl(value) + } + } else if (typeof value === 'object' && value !== null) { + processObject(value as Record | NestedValue[]) + } + } + } + processObject(body as Record | NestedValue[]) +} diff --git a/packages/cache/__tests__/cacheTwirpClient.test.ts b/packages/cache/__tests__/cacheTwirpClient.test.ts deleted file mode 100644 index f0cc59f3..00000000 --- a/packages/cache/__tests__/cacheTwirpClient.test.ts +++ /dev/null @@ -1,125 +0,0 @@ -import {CacheServiceClient} from '../src/internal/shared/cacheTwirpClient' -import {setSecret} from '@actions/core' - -jest.mock('@actions/core', () => ({ - setSecret: jest.fn(), - info: jest.fn(), - debug: jest.fn() -})) - -describe('CacheServiceClient', () => { - let client: CacheServiceClient - - beforeEach(() => { - jest.clearAllMocks() - process.env['ACTIONS_RUNTIME_TOKEN'] = 'test-token' - client = new CacheServiceClient('test-agent') - }) - - afterEach(() => { - delete process.env['ACTIONS_RUNTIME_TOKEN'] - }) - - 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 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 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(body) - - 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/__tests__/util.test.ts b/packages/cache/__tests__/util.test.ts new file mode 100644 index 00000000..41c29f4b --- /dev/null +++ b/packages/cache/__tests__/util.test.ts @@ -0,0 +1,289 @@ +import {maskSigUrl, maskSecretUrls} from '../src/internal/shared/util' +import {setSecret, debug} from '@actions/core' + +jest.mock('@actions/core') + +describe('maskSigUrl', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('masks the sig parameter in the URL and sets it as a secret', () => { + const url = 'https://example.com?sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('returns the original URL if no sig parameter is present', () => { + const url = 'https://example.com' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe(url) + expect(setSecret).not.toHaveBeenCalled() + }) + + it('masks the sig parameter in the middle of the URL and sets it as a secret', () => { + const url = 'https://example.com?param1=value1&sig=12345¶m2=value2' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe( + 'https://example.com/?param1=value1&sig=***¶m2=value2' + ) + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('returns the original URL if it is empty', () => { + const url = '' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('') + expect(setSecret).not.toHaveBeenCalled() + }) + + it('handles URLs with special characters in signature', () => { + const url = 'https://example.com?sig=abc/+=%3D' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***') + + expect(setSecret).toHaveBeenCalledWith('abc/+') + expect(setSecret).toHaveBeenCalledWith('abc/ ==') + expect(setSecret).toHaveBeenCalledWith('abc%2F%20%3D%3D') + }) + + it('handles relative URLs', () => { + const url = '/path?param=value&sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/path?param=value&sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with uppercase SIG parameter', () => { + const url = 'https://example.com?SIG=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?SIG=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles malformed URLs using regex fallback', () => { + const url = 'not:a:valid:url:but:has:sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('not:a:valid:url:but:has:sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + }) + + it('handles URLs with fragments', () => { + const url = 'https://example.com?sig=12345#fragment' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***#fragment') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with Sig parameter (first letter uppercase)', () => { + const url = 'https://example.com?Sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?Sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with sIg parameter (middle letter uppercase)', () => { + const url = 'https://example.com?sIg=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sIg=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with siG parameter (last letter uppercase)', () => { + const url = 'https://example.com?siG=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?siG=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles URLs with mixed case sig parameters in the same URL', () => { + const url = 'https://example.com?sig=123&SIG=456&Sig=789' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***&SIG=***&Sig=***') + expect(setSecret).toHaveBeenCalledWith('123') + expect(setSecret).toHaveBeenCalledWith('456') + expect(setSecret).toHaveBeenCalledWith('789') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('123')) + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('456')) + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('789')) + }) + + it('handles malformed URLs with different sig case variations', () => { + const url = 'not:a:valid:url:but:has:Sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('not:a:valid:url:but:has:Sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + }) + + it('handles malformed URLs with uppercase SIG in irregular formats', () => { + const url = 'something&SIG=12345&other:stuff' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('something&SIG=***&other:stuff') + expect(setSecret).toHaveBeenCalledWith('12345') + }) + + it('handles sig parameter at the start of the query string', () => { + const url = 'https://example.com?sig=12345¶m=value' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?sig=***¶m=value') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) + + it('handles sig parameter at the end of the query string', () => { + const url = 'https://example.com?param=value&sig=12345' + const maskedUrl = maskSigUrl(url) + expect(maskedUrl).toBe('https://example.com/?param=value&sig=***') + expect(setSecret).toHaveBeenCalledWith('12345') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) + }) +}) + +describe('maskSecretUrls', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('masks sig parameters in signed_upload_url and signed_download_url', () => { + const body = { + signed_upload_url: 'https://upload.com?sig=upload123', + signed_download_url: 'https://download.com?sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) + expect(setSecret).toHaveBeenCalledWith('download123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) + }) + + it('handles case where only upload_url is present', () => { + const body = { + signed_upload_url: 'https://upload.com?sig=upload123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) + }) + + it('handles case where only download_url is present', () => { + const body = { + signed_download_url: 'https://download.com?sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('download123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) + }) + + it('handles case where URLs do not contain sig parameters', () => { + const body = { + signed_upload_url: 'https://upload.com?token=abc', + signed_download_url: 'https://download.com?token=xyz' + } + maskSecretUrls(body) + expect(setSecret).not.toHaveBeenCalled() + }) + + it('handles empty string URLs', () => { + const body = { + signed_upload_url: '', + signed_download_url: '' + } + maskSecretUrls(body) + expect(setSecret).not.toHaveBeenCalled() + }) + + it('handles malformed URLs in the body', () => { + const body = { + signed_upload_url: 'not:a:valid:url:but:has:sig=upload123', + signed_download_url: 'also:not:valid:with:sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith('download123') + }) + + it('handles URLs with different case variations of sig parameter', () => { + const body = { + signed_upload_url: 'https://upload.com?SIG=upload123', + signed_download_url: 'https://download.com?Sig=download123' + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('upload123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) + expect(setSecret).toHaveBeenCalledWith('download123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) + }) + + it('handles URLs with special characters in signature', () => { + const specialSig = 'xyz!@#$' + const encodedSpecialSig = encodeURIComponent(specialSig) + + const body = { + signed_upload_url: 'https://upload.com?sig=abc/+=%3D', + signed_download_url: `https://download.com?sig=${encodedSpecialSig}` + } + maskSecretUrls(body) + + const allCalls = (setSecret as jest.Mock).mock.calls.flat() + + expect(allCalls).toContain('abc/+') + expect(allCalls).toContain('abc/ ==') + expect(allCalls).toContain('abc%2F%20%3D%3D') + + expect(allCalls).toContain(specialSig) + }) + + it('handles deeply nested URLs in the body', () => { + const body = { + data: { + urls: { + signed_upload_url: 'https://upload.com?sig=nested123', + signed_download_url: 'https://download.com?sig=nested456' + } + } + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('nested123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested123')) + expect(setSecret).toHaveBeenCalledWith('nested456') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested456')) + }) + + it('handles arrays of URLs in the body', () => { + const body = { + signed_urls: [ + 'https://first.com?sig=first123', + 'https://second.com?sig=second456' + ] + } + maskSecretUrls(body) + expect(setSecret).toHaveBeenCalledWith('first123') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('first123')) + expect(setSecret).toHaveBeenCalledWith('second456') + expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('second456')) + }) + + it('does nothing if body is not an object or is null', () => { + maskSecretUrls(null) + expect(debug).toHaveBeenCalledWith('body is not an object or is null') + expect(setSecret).not.toHaveBeenCalled() + }) + + it('does nothing if signed_upload_url and signed_download_url are not strings', () => { + const body = { + signed_upload_url: 123, + signed_download_url: 456 + } + maskSecretUrls(body) + expect(setSecret).not.toHaveBeenCalled() + }) +}) diff --git a/packages/cache/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index fb293b20..cfd01513 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/packages/cache/src/internal/shared/cacheTwirpClient.ts @@ -1,4 +1,4 @@ -import {info, debug, setSecret} from '@actions/core' +import {info, debug} from '@actions/core' import {getUserAgentString} from './user-agent' import {NetworkError, UsageError} from './errors' import {getCacheServiceURL} from '../config' @@ -6,6 +6,7 @@ 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 {maskSecretUrls} from './util' // The twirp http client must implement this interface interface Rpc { @@ -94,7 +95,7 @@ export class CacheServiceClient implements Rpc { debug(`[Response] - ${response.message.statusCode}`) debug(`Headers: ${JSON.stringify(response.message.headers, null, 2)}`) const body = JSON.parse(rawBody) - this.maskSecretUrls(body) + maskSecretUrls(body) debug(`Body: ${JSON.stringify(body, null, 2)}`) if (this.isSuccessStatusCode(statusCode)) { return {response, body} @@ -149,41 +150,6 @@ export class CacheServiceClient implements Rpc { throw new Error(`Request failed`) } - /** - * Masks the `sig` parameter in a URL and sets it as a secret. - * @param url The URL containing the `sig` parameter. - * @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): 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): 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') - } - } - isSuccessStatusCode(statusCode?: number): boolean { if (!statusCode) return false return statusCode >= 200 && statusCode < 300 diff --git a/packages/cache/src/internal/shared/util.ts b/packages/cache/src/internal/shared/util.ts new file mode 100644 index 00000000..aecced9a --- /dev/null +++ b/packages/cache/src/internal/shared/util.ts @@ -0,0 +1,171 @@ +import {debug, setSecret} from '@actions/core' + +/** + * Masks the `sig` parameter in a URL and sets it as a secret. + * @param url The URL containing the `sig` parameter. + * @returns A masked URL where the sig parameter value is replaced with '***' if found, + * or the original URL if no sig parameter is present. + */ +export function maskSigUrl(url: string): string { + if (!url) return url + + try { + const rawSigRegex = /[?&](sig)=([^&=#]+)/gi + let match + + while ((match = rawSigRegex.exec(url)) !== null) { + const rawSignature = match[2] + if (rawSignature) { + setSecret(rawSignature) + } + } + + let parsedUrl: URL + try { + parsedUrl = new URL(url) + } catch { + try { + parsedUrl = new URL(url, 'https://example.com') + } catch (error) { + debug(`Failed to parse URL: ${url}`) + return maskSigWithRegex(url) + } + } + + let masked = false + const paramNames = Array.from(parsedUrl.searchParams.keys()) + + for (const paramName of paramNames) { + if (paramName.toLowerCase() === 'sig') { + const signature = parsedUrl.searchParams.get(paramName) + if (signature) { + setSecret(signature) + setSecret(encodeURIComponent(signature)) + parsedUrl.searchParams.set(paramName, '***') + masked = true + } + } + } + if (masked) { + return parsedUrl.toString() + } + + if (/([:?&]|^)(sig)=([^&=#]+)/i.test(url)) { + return maskSigWithRegex(url) + } + } catch (error) { + debug( + `Error masking URL: ${ + error instanceof Error ? error.message : String(error) + }` + ) + return maskSigWithRegex(url) + } + + return url +} + +/** + * Fallback method to mask signatures using regex when URL parsing fails + */ +function maskSigWithRegex(url: string): string { + try { + const regex = /([:?&]|^)(sig)=([^&=#]+)/gi + + return url.replace(regex, (fullMatch, prefix, paramName, value) => { + if (value) { + setSecret(value) + try { + setSecret(decodeURIComponent(value)) + } catch { + // Ignore decoding errors + } + return `${prefix}${paramName}=***` + } + return fullMatch + }) + } catch (error) { + debug( + `Error in maskSigWithRegex: ${ + error instanceof Error ? error.message : String(error) + }` + ) + return url + } +} + +/** + * Masks any URLs containing signature parameters in the provided object + * Recursively searches through nested objects and arrays + */ +export function maskSecretUrls( + body: Record | unknown[] | null +): void { + if (typeof body !== 'object' || body === null) { + debug('body is not an object or is null') + return + } + + type NestedValue = + | string + | number + | boolean + | null + | undefined + | NestedObject + | NestedArray + interface NestedObject { + [key: string]: NestedValue + signed_upload_url?: string + signed_download_url?: string + } + type NestedArray = NestedValue[] + + const processUrl = (url: string): void => { + maskSigUrl(url) + } + + const processObject = ( + obj: Record | NestedValue[] + ): void => { + if (typeof obj !== 'object' || obj === null) { + return + } + + if (Array.isArray(obj)) { + for (const item of obj) { + if (typeof item === 'string') { + processUrl(item) + } else if (typeof item === 'object' && item !== null) { + processObject(item as Record | NestedValue[]) + } + } + return + } + + if ( + 'signed_upload_url' in obj && + typeof obj.signed_upload_url === 'string' + ) { + maskSigUrl(obj.signed_upload_url) + } + if ( + 'signed_download_url' in obj && + typeof obj.signed_download_url === 'string' + ) { + maskSigUrl(obj.signed_download_url) + } + + for (const key in obj) { + const value = obj[key] + if (typeof value === 'string') { + if (/([:?&]|^)(sig)=/i.test(value)) { + maskSigUrl(value) + } + } else if (typeof value === 'object' && value !== null) { + processObject(value as Record | NestedValue[]) + } + } + } + processObject(body as Record | NestedValue[]) +} From abd9054c61212efc1c8e5de983494d99548dcf17 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Wed, 12 Mar 2025 08:14:01 -0700 Subject: [PATCH 07/12] Log debug error when failing to decode --- packages/cache/src/internal/shared/util.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/cache/src/internal/shared/util.ts b/packages/cache/src/internal/shared/util.ts index aecced9a..b9212d8d 100644 --- a/packages/cache/src/internal/shared/util.ts +++ b/packages/cache/src/internal/shared/util.ts @@ -77,8 +77,12 @@ function maskSigWithRegex(url: string): string { setSecret(value) try { setSecret(decodeURIComponent(value)) - } catch { - // Ignore decoding errors + } catch (error) { + debug( + `Failed to decode URL parameter: ${ + error instanceof Error ? error.message : String(error) + }` + ) } return `${prefix}${paramName}=***` } From fc482662af1c4982242f044418f68bf6275bbfb2 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 13 Mar 2025 04:23:45 -0700 Subject: [PATCH 08/12] PR feedback, back to simplified approach, no export on client as well --- packages/artifact/__tests__/util.test.ts | 180 +----------------- .../internal/shared/artifact-twirp-client.ts | 2 +- packages/artifact/src/internal/shared/util.ts | 148 ++------------ packages/cache/__tests__/util.test.ts | 180 +----------------- .../src/internal/shared/cacheTwirpClient.ts | 2 +- packages/cache/src/internal/shared/util.ts | 158 ++------------- 6 files changed, 37 insertions(+), 633 deletions(-) diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 34535db2..018bfc45 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -69,14 +69,6 @@ describe('maskSigUrl', () => { jest.clearAllMocks() }) - it('masks the sig parameter in the URL and sets it as a secret', () => { - const url = 'https://example.com?sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - it('returns the original URL if no sig parameter is present', () => { const url = 'https://example.com' const maskedUrl = maskSigUrl(url) @@ -85,7 +77,7 @@ describe('maskSigUrl', () => { }) it('masks the sig parameter in the middle of the URL and sets it as a secret', () => { - const url = 'https://example.com?param1=value1&sig=12345¶m2=value2' + const url = 'https://example.com/?param1=value1&sig=12345¶m2=value2' const maskedUrl = maskSigUrl(url) expect(maskedUrl).toBe( 'https://example.com/?param1=value1&sig=***¶m2=value2' @@ -101,39 +93,6 @@ describe('maskSigUrl', () => { expect(setSecret).not.toHaveBeenCalled() }) - it('handles URLs with special characters in signature', () => { - const url = 'https://example.com?sig=abc/+=%3D' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***') - - expect(setSecret).toHaveBeenCalledWith('abc/+') - expect(setSecret).toHaveBeenCalledWith('abc/ ==') - expect(setSecret).toHaveBeenCalledWith('abc%2F%20%3D%3D') - }) - - it('handles relative URLs', () => { - const url = '/path?param=value&sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/path?param=value&sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with uppercase SIG parameter', () => { - const url = 'https://example.com?SIG=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?SIG=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles malformed URLs using regex fallback', () => { - const url = 'not:a:valid:url:but:has:sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('not:a:valid:url:but:has:sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - }) - it('handles URLs with fragments', () => { const url = 'https://example.com?sig=12345#fragment' const maskedUrl = maskSigUrl(url) @@ -141,72 +100,6 @@ describe('maskSigUrl', () => { expect(setSecret).toHaveBeenCalledWith('12345') expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) }) - - it('handles URLs with Sig parameter (first letter uppercase)', () => { - const url = 'https://example.com?Sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?Sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with sIg parameter (middle letter uppercase)', () => { - const url = 'https://example.com?sIg=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sIg=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with siG parameter (last letter uppercase)', () => { - const url = 'https://example.com?siG=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?siG=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with mixed case sig parameters in the same URL', () => { - const url = 'https://example.com?sig=123&SIG=456&Sig=789' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***&SIG=***&Sig=***') - expect(setSecret).toHaveBeenCalledWith('123') - expect(setSecret).toHaveBeenCalledWith('456') - expect(setSecret).toHaveBeenCalledWith('789') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('123')) - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('456')) - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('789')) - }) - - it('handles malformed URLs with different sig case variations', () => { - const url = 'not:a:valid:url:but:has:Sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('not:a:valid:url:but:has:Sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - }) - - it('handles malformed URLs with uppercase SIG in irregular formats', () => { - const url = 'something&SIG=12345&other:stuff' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('something&SIG=***&other:stuff') - expect(setSecret).toHaveBeenCalledWith('12345') - }) - - it('handles sig parameter at the start of the query string', () => { - const url = 'https://example.com?sig=12345¶m=value' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***¶m=value') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles sig parameter at the end of the query string', () => { - const url = 'https://example.com?param=value&sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?param=value&sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) }) describe('maskSecretUrls', () => { @@ -262,77 +155,6 @@ describe('maskSecretUrls', () => { expect(setSecret).not.toHaveBeenCalled() }) - it('handles malformed URLs in the body', () => { - const body = { - signed_upload_url: 'not:a:valid:url:but:has:sig=upload123', - signed_url: 'also:not:valid:with:sig=download123' - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('upload123') - expect(setSecret).toHaveBeenCalledWith('download123') - }) - - it('handles URLs with different case variations of sig parameter', () => { - const body = { - signed_upload_url: 'https://upload.com?SIG=upload123', - signed_url: 'https://download.com?Sig=download123' - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('upload123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) - expect(setSecret).toHaveBeenCalledWith('download123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) - }) - - it('handles URLs with special characters in signature', () => { - const specialSig = 'xyz!@#$' - const encodedSpecialSig = encodeURIComponent(specialSig) - - const body = { - signed_upload_url: 'https://upload.com?sig=abc/+=%3D', - signed_url: `https://download.com?sig=${encodedSpecialSig}` - } - maskSecretUrls(body) - - const allCalls = (setSecret as jest.Mock).mock.calls.flat() - - expect(allCalls).toContain('abc/+') - expect(allCalls).toContain('abc/ ==') - expect(allCalls).toContain('abc%2F%20%3D%3D') - - expect(allCalls).toContain(specialSig) - }) - - it('handles deeply nested URLs in the body', () => { - const body = { - data: { - urls: { - signed_upload_url: 'https://upload.com?sig=nested123', - signed_url: 'https://download.com?sig=nested456' - } - } - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('nested123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested123')) - expect(setSecret).toHaveBeenCalledWith('nested456') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested456')) - }) - - it('handles arrays of URLs in the body', () => { - const body = { - signed_urls: [ - 'https://first.com?sig=first123', - 'https://second.com?sig=second456' - ] - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('first123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('first123')) - expect(setSecret).toHaveBeenCalledWith('second456') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('second456')) - }) - it('does nothing if body is not an object or is null', () => { maskSecretUrls(null) expect(debug).toHaveBeenCalledWith('body is not an object or is null') diff --git a/packages/artifact/src/internal/shared/artifact-twirp-client.ts b/packages/artifact/src/internal/shared/artifact-twirp-client.ts index 84461518..57499125 100644 --- a/packages/artifact/src/internal/shared/artifact-twirp-client.ts +++ b/packages/artifact/src/internal/shared/artifact-twirp-client.ts @@ -17,7 +17,7 @@ interface Rpc { ): Promise } -export class ArtifactHttpClient implements Rpc { +class ArtifactHttpClient implements Rpc { private httpClient: HttpClient private baseUrl: string private maxAttempts = 5 diff --git a/packages/artifact/src/internal/shared/util.ts b/packages/artifact/src/internal/shared/util.ts index 754c6b76..61f2ab6a 100644 --- a/packages/artifact/src/internal/shared/util.ts +++ b/packages/artifact/src/internal/shared/util.ts @@ -81,159 +81,41 @@ export function maskSigUrl(url: string): string { if (!url) return url try { - const rawSigRegex = /[?&](sig)=([^&=#]+)/gi - let match + const parsedUrl = new URL(url) + const signature = parsedUrl.searchParams.get('sig') - while ((match = rawSigRegex.exec(url)) !== null) { - const rawSignature = match[2] - if (rawSignature) { - setSecret(rawSignature) - } - } - - let parsedUrl: URL - try { - parsedUrl = new URL(url) - } catch { - try { - parsedUrl = new URL(url, 'https://example.com') - } catch (error) { - debug(`Failed to parse URL: ${url}`) - return maskSigWithRegex(url) - } - } - - let masked = false - const paramNames = Array.from(parsedUrl.searchParams.keys()) - - for (const paramName of paramNames) { - if (paramName.toLowerCase() === 'sig') { - const signature = parsedUrl.searchParams.get(paramName) - if (signature) { - setSecret(signature) - setSecret(encodeURIComponent(signature)) - parsedUrl.searchParams.set(paramName, '***') - masked = true - } - } - } - if (masked) { + if (signature) { + setSecret(signature) + setSecret(encodeURIComponent(signature)) + parsedUrl.searchParams.set('sig', '***') return parsedUrl.toString() } - - if (/([:?&]|^)(sig)=([^&=#]+)/i.test(url)) { - return maskSigWithRegex(url) - } } catch (error) { debug( - `Error masking URL: ${ + `Failed to parse URL: ${url} ${ error instanceof Error ? error.message : String(error) }` ) - return maskSigWithRegex(url) } - return url } -/** - * Fallback method to mask signatures using regex when URL parsing fails - */ -function maskSigWithRegex(url: string): string { - try { - const regex = /([:?&]|^)(sig)=([^&=#]+)/gi - - return url.replace(regex, (fullMatch, prefix, paramName, value) => { - if (value) { - setSecret(value) - try { - setSecret(decodeURIComponent(value)) - } catch { - // Ignore decoding errors - } - return `${prefix}${paramName}=***` - } - return fullMatch - }) - } catch (error) { - debug( - `Error in maskSigWithRegex: ${ - error instanceof Error ? error.message : String(error) - }` - ) - return url - } -} - /** * Masks any URLs containing signature parameters in the provided object - * Recursively searches through nested objects and arrays */ -export function maskSecretUrls( - body: Record | unknown[] | null -): void { +export function maskSecretUrls(body: Record | null): void { if (typeof body !== 'object' || body === null) { debug('body is not an object or is null') return } - type NestedValue = - | string - | number - | boolean - | null - | undefined - | NestedObject - | NestedArray - interface NestedObject { - [key: string]: NestedValue - signed_upload_url?: string - signed_url?: string + if ( + 'signed_upload_url' in body && + typeof body.signed_upload_url === 'string' + ) { + maskSigUrl(body.signed_upload_url) } - type NestedArray = NestedValue[] - - const processUrl = (url: string): void => { - maskSigUrl(url) + if ('signed_url' in body && typeof body.signed_url === 'string') { + maskSigUrl(body.signed_url) } - - const processObject = ( - obj: Record | NestedValue[] - ): void => { - if (typeof obj !== 'object' || obj === null) { - return - } - - if (Array.isArray(obj)) { - for (const item of obj) { - if (typeof item === 'string') { - processUrl(item) - } else if (typeof item === 'object' && item !== null) { - processObject(item as Record | NestedValue[]) - } - } - return - } - - if ( - 'signed_upload_url' in obj && - typeof obj.signed_upload_url === 'string' - ) { - maskSigUrl(obj.signed_upload_url) - } - if ('signed_url' in obj && typeof obj.signed_url === 'string') { - maskSigUrl(obj.signed_url) - } - - for (const key in obj) { - const value = obj[key] - if (typeof value === 'string') { - if (/([:?&]|^)(sig)=/i.test(value)) { - maskSigUrl(value) - } - } else if (typeof value === 'object' && value !== null) { - processObject(value as Record | NestedValue[]) - } - } - } - processObject(body as Record | NestedValue[]) } diff --git a/packages/cache/__tests__/util.test.ts b/packages/cache/__tests__/util.test.ts index 41c29f4b..12cec07d 100644 --- a/packages/cache/__tests__/util.test.ts +++ b/packages/cache/__tests__/util.test.ts @@ -8,14 +8,6 @@ describe('maskSigUrl', () => { jest.clearAllMocks() }) - it('masks the sig parameter in the URL and sets it as a secret', () => { - const url = 'https://example.com?sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - it('returns the original URL if no sig parameter is present', () => { const url = 'https://example.com' const maskedUrl = maskSigUrl(url) @@ -24,7 +16,7 @@ describe('maskSigUrl', () => { }) it('masks the sig parameter in the middle of the URL and sets it as a secret', () => { - const url = 'https://example.com?param1=value1&sig=12345¶m2=value2' + const url = 'https://example.com/?param1=value1&sig=12345¶m2=value2' const maskedUrl = maskSigUrl(url) expect(maskedUrl).toBe( 'https://example.com/?param1=value1&sig=***¶m2=value2' @@ -40,39 +32,6 @@ describe('maskSigUrl', () => { expect(setSecret).not.toHaveBeenCalled() }) - it('handles URLs with special characters in signature', () => { - const url = 'https://example.com?sig=abc/+=%3D' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***') - - expect(setSecret).toHaveBeenCalledWith('abc/+') - expect(setSecret).toHaveBeenCalledWith('abc/ ==') - expect(setSecret).toHaveBeenCalledWith('abc%2F%20%3D%3D') - }) - - it('handles relative URLs', () => { - const url = '/path?param=value&sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/path?param=value&sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with uppercase SIG parameter', () => { - const url = 'https://example.com?SIG=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?SIG=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles malformed URLs using regex fallback', () => { - const url = 'not:a:valid:url:but:has:sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('not:a:valid:url:but:has:sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - }) - it('handles URLs with fragments', () => { const url = 'https://example.com?sig=12345#fragment' const maskedUrl = maskSigUrl(url) @@ -80,72 +39,6 @@ describe('maskSigUrl', () => { expect(setSecret).toHaveBeenCalledWith('12345') expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) }) - - it('handles URLs with Sig parameter (first letter uppercase)', () => { - const url = 'https://example.com?Sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?Sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with sIg parameter (middle letter uppercase)', () => { - const url = 'https://example.com?sIg=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sIg=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with siG parameter (last letter uppercase)', () => { - const url = 'https://example.com?siG=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?siG=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles URLs with mixed case sig parameters in the same URL', () => { - const url = 'https://example.com?sig=123&SIG=456&Sig=789' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***&SIG=***&Sig=***') - expect(setSecret).toHaveBeenCalledWith('123') - expect(setSecret).toHaveBeenCalledWith('456') - expect(setSecret).toHaveBeenCalledWith('789') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('123')) - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('456')) - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('789')) - }) - - it('handles malformed URLs with different sig case variations', () => { - const url = 'not:a:valid:url:but:has:Sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('not:a:valid:url:but:has:Sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - }) - - it('handles malformed URLs with uppercase SIG in irregular formats', () => { - const url = 'something&SIG=12345&other:stuff' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('something&SIG=***&other:stuff') - expect(setSecret).toHaveBeenCalledWith('12345') - }) - - it('handles sig parameter at the start of the query string', () => { - const url = 'https://example.com?sig=12345¶m=value' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***¶m=value') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) - - it('handles sig parameter at the end of the query string', () => { - const url = 'https://example.com?param=value&sig=12345' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?param=value&sig=***') - expect(setSecret).toHaveBeenCalledWith('12345') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) - }) }) describe('maskSecretUrls', () => { @@ -201,77 +94,6 @@ describe('maskSecretUrls', () => { expect(setSecret).not.toHaveBeenCalled() }) - it('handles malformed URLs in the body', () => { - const body = { - signed_upload_url: 'not:a:valid:url:but:has:sig=upload123', - signed_download_url: 'also:not:valid:with:sig=download123' - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('upload123') - expect(setSecret).toHaveBeenCalledWith('download123') - }) - - it('handles URLs with different case variations of sig parameter', () => { - const body = { - signed_upload_url: 'https://upload.com?SIG=upload123', - signed_download_url: 'https://download.com?Sig=download123' - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('upload123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('upload123')) - expect(setSecret).toHaveBeenCalledWith('download123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('download123')) - }) - - it('handles URLs with special characters in signature', () => { - const specialSig = 'xyz!@#$' - const encodedSpecialSig = encodeURIComponent(specialSig) - - const body = { - signed_upload_url: 'https://upload.com?sig=abc/+=%3D', - signed_download_url: `https://download.com?sig=${encodedSpecialSig}` - } - maskSecretUrls(body) - - const allCalls = (setSecret as jest.Mock).mock.calls.flat() - - expect(allCalls).toContain('abc/+') - expect(allCalls).toContain('abc/ ==') - expect(allCalls).toContain('abc%2F%20%3D%3D') - - expect(allCalls).toContain(specialSig) - }) - - it('handles deeply nested URLs in the body', () => { - const body = { - data: { - urls: { - signed_upload_url: 'https://upload.com?sig=nested123', - signed_download_url: 'https://download.com?sig=nested456' - } - } - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('nested123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested123')) - expect(setSecret).toHaveBeenCalledWith('nested456') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('nested456')) - }) - - it('handles arrays of URLs in the body', () => { - const body = { - signed_urls: [ - 'https://first.com?sig=first123', - 'https://second.com?sig=second456' - ] - } - maskSecretUrls(body) - expect(setSecret).toHaveBeenCalledWith('first123') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('first123')) - expect(setSecret).toHaveBeenCalledWith('second456') - expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('second456')) - }) - it('does nothing if body is not an object or is null', () => { maskSecretUrls(null) expect(debug).toHaveBeenCalledWith('body is not an object or is null') diff --git a/packages/cache/src/internal/shared/cacheTwirpClient.ts b/packages/cache/src/internal/shared/cacheTwirpClient.ts index cfd01513..f6c2af61 100644 --- a/packages/cache/src/internal/shared/cacheTwirpClient.ts +++ b/packages/cache/src/internal/shared/cacheTwirpClient.ts @@ -25,7 +25,7 @@ interface Rpc { * * This class is used to interact with cache service v2. */ -export class CacheServiceClient implements Rpc { +class CacheServiceClient implements Rpc { private httpClient: HttpClient private baseUrl: string private maxAttempts = 5 diff --git a/packages/cache/src/internal/shared/util.ts b/packages/cache/src/internal/shared/util.ts index b9212d8d..b69bb18c 100644 --- a/packages/cache/src/internal/shared/util.ts +++ b/packages/cache/src/internal/shared/util.ts @@ -10,166 +10,44 @@ export function maskSigUrl(url: string): string { if (!url) return url try { - const rawSigRegex = /[?&](sig)=([^&=#]+)/gi - let match + const parsedUrl = new URL(url) + const signature = parsedUrl.searchParams.get('sig') - while ((match = rawSigRegex.exec(url)) !== null) { - const rawSignature = match[2] - if (rawSignature) { - setSecret(rawSignature) - } - } - - let parsedUrl: URL - try { - parsedUrl = new URL(url) - } catch { - try { - parsedUrl = new URL(url, 'https://example.com') - } catch (error) { - debug(`Failed to parse URL: ${url}`) - return maskSigWithRegex(url) - } - } - - let masked = false - const paramNames = Array.from(parsedUrl.searchParams.keys()) - - for (const paramName of paramNames) { - if (paramName.toLowerCase() === 'sig') { - const signature = parsedUrl.searchParams.get(paramName) - if (signature) { - setSecret(signature) - setSecret(encodeURIComponent(signature)) - parsedUrl.searchParams.set(paramName, '***') - masked = true - } - } - } - if (masked) { + if (signature) { + setSecret(signature) + setSecret(encodeURIComponent(signature)) + parsedUrl.searchParams.set('sig', '***') return parsedUrl.toString() } - - if (/([:?&]|^)(sig)=([^&=#]+)/i.test(url)) { - return maskSigWithRegex(url) - } } catch (error) { debug( - `Error masking URL: ${ + `Failed to parse URL: ${url} ${ error instanceof Error ? error.message : String(error) }` ) - return maskSigWithRegex(url) } - return url } -/** - * Fallback method to mask signatures using regex when URL parsing fails - */ -function maskSigWithRegex(url: string): string { - try { - const regex = /([:?&]|^)(sig)=([^&=#]+)/gi - - return url.replace(regex, (fullMatch, prefix, paramName, value) => { - if (value) { - setSecret(value) - try { - setSecret(decodeURIComponent(value)) - } catch (error) { - debug( - `Failed to decode URL parameter: ${ - error instanceof Error ? error.message : String(error) - }` - ) - } - return `${prefix}${paramName}=***` - } - return fullMatch - }) - } catch (error) { - debug( - `Error in maskSigWithRegex: ${ - error instanceof Error ? error.message : String(error) - }` - ) - return url - } -} - /** * Masks any URLs containing signature parameters in the provided object - * Recursively searches through nested objects and arrays */ -export function maskSecretUrls( - body: Record | unknown[] | null -): void { +export function maskSecretUrls(body: Record | null): void { if (typeof body !== 'object' || body === null) { debug('body is not an object or is null') return } - type NestedValue = - | string - | number - | boolean - | null - | undefined - | NestedObject - | NestedArray - interface NestedObject { - [key: string]: NestedValue - signed_upload_url?: string - signed_download_url?: string + if ( + 'signed_upload_url' in body && + typeof body.signed_upload_url === 'string' + ) { + maskSigUrl(body.signed_upload_url) } - type NestedArray = NestedValue[] - - const processUrl = (url: string): void => { - maskSigUrl(url) + if ( + 'signed_download_url' in body && + typeof body.signed_download_url === 'string' + ) { + maskSigUrl(body.signed_download_url) } - - const processObject = ( - obj: Record | NestedValue[] - ): void => { - if (typeof obj !== 'object' || obj === null) { - return - } - - if (Array.isArray(obj)) { - for (const item of obj) { - if (typeof item === 'string') { - processUrl(item) - } else if (typeof item === 'object' && item !== null) { - processObject(item as Record | NestedValue[]) - } - } - return - } - - if ( - 'signed_upload_url' in obj && - typeof obj.signed_upload_url === 'string' - ) { - maskSigUrl(obj.signed_upload_url) - } - if ( - 'signed_download_url' in obj && - typeof obj.signed_download_url === 'string' - ) { - maskSigUrl(obj.signed_download_url) - } - - for (const key in obj) { - const value = obj[key] - if (typeof value === 'string') { - if (/([:?&]|^)(sig)=/i.test(value)) { - maskSigUrl(value) - } - } else if (typeof value === 'object' && value !== null) { - processObject(value as Record | NestedValue[]) - } - } - } - processObject(body as Record | NestedValue[]) } From 6876e2a664ec02908178087905b9155e9892a437 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 13 Mar 2025 04:47:49 -0700 Subject: [PATCH 09/12] update ts docs --- packages/artifact/src/internal/shared/util.ts | 44 ++++++++++++++---- packages/cache/__tests__/util.test.ts | 18 +++----- packages/cache/src/internal/shared/util.ts | 45 ++++++++++++++----- packages/core/src/command.ts | 31 +++++++++++-- packages/core/src/core.ts | 27 ++++++++++- 5 files changed, 129 insertions(+), 36 deletions(-) diff --git a/packages/artifact/src/internal/shared/util.ts b/packages/artifact/src/internal/shared/util.ts index 61f2ab6a..e346e639 100644 --- a/packages/artifact/src/internal/shared/util.ts +++ b/packages/artifact/src/internal/shared/util.ts @@ -73,13 +73,23 @@ export function getBackendIdsFromToken(): BackendIds { /** * Masks the `sig` parameter in a URL and sets it as a secret. - * @param url The URL containing the `sig` parameter. - * @returns A masked URL where the sig parameter value is replaced with '***' if found, - * or the original URL if no sig parameter is present. + * + * @param url - The URL containing the signature parameter to mask + * @remarks + * This function attempts to parse the provided URL and identify the 'sig' query parameter. + * If found, it registers both the raw and URL-encoded signature values as secrets using + * the Actions `setSecret` API, which prevents them from being displayed in logs. + * + * The function handles errors gracefully if URL parsing fails, logging them as debug messages. + * + * @example + * ```typescript + * // Mask a signature in an Azure SAS token URL + * maskSigUrl('https://example.blob.core.windows.net/container/file.txt?sig=abc123&se=2023-01-01'); + * ``` */ -export function maskSigUrl(url: string): string { - if (!url) return url - +export function maskSigUrl(url: string): void { + if (!url) return try { const parsedUrl = new URL(url) const signature = parsedUrl.searchParams.get('sig') @@ -88,7 +98,6 @@ export function maskSigUrl(url: string): string { setSecret(signature) setSecret(encodeURIComponent(signature)) parsedUrl.searchParams.set('sig', '***') - return parsedUrl.toString() } } catch (error) { debug( @@ -97,11 +106,28 @@ export function maskSigUrl(url: string): string { }` ) } - return url } /** - * Masks any URLs containing signature parameters in the provided object + * Masks sensitive information in URLs containing signature parameters. + * Currently supports masking 'sig' parameters in the 'signed_upload_url' + * and 'signed_download_url' properties of the provided object. + * + * @param body - The object should contain a signature + * @remarks + * This function extracts URLs from the object properties and calls maskSigUrl + * on each one to redact sensitive signature information. The function doesn't + * modify the original object; it only marks the signatures as secrets for + * logging purposes. + * + * @example + * ```typescript + * const responseBody = { + * signed_upload_url: 'https://example.com?sig=abc123', + * signed_download_url: 'https://example.com?sig=def456' + * }; + * maskSecretUrls(responseBody); + * ``` */ export function maskSecretUrls(body: Record | null): void { if (typeof body !== 'object' || body === null) { diff --git a/packages/cache/__tests__/util.test.ts b/packages/cache/__tests__/util.test.ts index 12cec07d..7cf071dd 100644 --- a/packages/cache/__tests__/util.test.ts +++ b/packages/cache/__tests__/util.test.ts @@ -8,34 +8,28 @@ describe('maskSigUrl', () => { jest.clearAllMocks() }) - it('returns the original URL if no sig parameter is present', () => { + it('does nothing if no sig parameter is present', () => { const url = 'https://example.com' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe(url) + maskSigUrl(url) expect(setSecret).not.toHaveBeenCalled() }) it('masks the sig parameter in the middle of the URL and sets it as a secret', () => { const url = 'https://example.com/?param1=value1&sig=12345¶m2=value2' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe( - 'https://example.com/?param1=value1&sig=***¶m2=value2' - ) + maskSigUrl(url) expect(setSecret).toHaveBeenCalledWith('12345') expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) }) - it('returns the original URL if it is empty', () => { + it('does nothing if the URL is empty', () => { const url = '' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('') + maskSigUrl(url) expect(setSecret).not.toHaveBeenCalled() }) it('handles URLs with fragments', () => { const url = 'https://example.com?sig=12345#fragment' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***#fragment') + maskSigUrl(url) expect(setSecret).toHaveBeenCalledWith('12345') expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) }) diff --git a/packages/cache/src/internal/shared/util.ts b/packages/cache/src/internal/shared/util.ts index b69bb18c..bbce73fa 100644 --- a/packages/cache/src/internal/shared/util.ts +++ b/packages/cache/src/internal/shared/util.ts @@ -2,13 +2,23 @@ import {debug, setSecret} from '@actions/core' /** * Masks the `sig` parameter in a URL and sets it as a secret. - * @param url The URL containing the `sig` parameter. - * @returns A masked URL where the sig parameter value is replaced with '***' if found, - * or the original URL if no sig parameter is present. + * + * @param url - The URL containing the signature parameter to mask + * @remarks + * This function attempts to parse the provided URL and identify the 'sig' query parameter. + * If found, it registers both the raw and URL-encoded signature values as secrets using + * the Actions `setSecret` API, which prevents them from being displayed in logs. + * + * The function handles errors gracefully if URL parsing fails, logging them as debug messages. + * + * @example + * ```typescript + * // Mask a signature in an Azure SAS token URL + * maskSigUrl('https://example.blob.core.windows.net/container/file.txt?sig=abc123&se=2023-01-01'); + * ``` */ -export function maskSigUrl(url: string): string { - if (!url) return url - +export function maskSigUrl(url: string): void { + if (!url) return try { const parsedUrl = new URL(url) const signature = parsedUrl.searchParams.get('sig') @@ -17,7 +27,6 @@ export function maskSigUrl(url: string): string { setSecret(signature) setSecret(encodeURIComponent(signature)) parsedUrl.searchParams.set('sig', '***') - return parsedUrl.toString() } } catch (error) { debug( @@ -26,18 +35,34 @@ export function maskSigUrl(url: string): string { }` ) } - return url } /** - * Masks any URLs containing signature parameters in the provided object + * Masks sensitive information in URLs containing signature parameters. + * Currently supports masking 'sig' parameters in the 'signed_upload_url' + * and 'signed_download_url' properties of the provided object. + * + * @param body - The object should contain a signature + * @remarks + * This function extracts URLs from the object properties and calls maskSigUrl + * on each one to redact sensitive signature information. The function doesn't + * modify the original object; it only marks the signatures as secrets for + * logging purposes. + * + * @example + * ```typescript + * const responseBody = { + * signed_upload_url: 'https://blob.core.windows.net/?sig=abc123', + * signed_download_url: 'https://blob.core/windows.net/?sig=def456' + * }; + * maskSecretUrls(responseBody); + * ``` */ export function maskSecretUrls(body: Record | null): void { if (typeof body !== 'object' || body === null) { debug('body is not an object or is null') return } - if ( 'signed_upload_url' in body && typeof body.signed_upload_url === 'string' diff --git a/packages/core/src/command.ts b/packages/core/src/command.ts index 2796fce9..8b3dda00 100644 --- a/packages/core/src/command.ts +++ b/packages/core/src/command.ts @@ -11,14 +11,37 @@ export interface CommandProperties { } /** - * Commands + * Issues a command to the GitHub Actions runner + * + * @param command - The command name to issue + * @param properties - Additional properties for the command (key-value pairs) + * @param message - The message to include with the command + * @remarks + * This function outputs a specially formatted string to stdout that the Actions + * runner interprets as a command. These commands can control workflow behavior, + * set outputs, create annotations, mask values, and more. * * Command Format: * ::name key=value,key=value::message * - * Examples: - * ::warning::This is the message - * ::set-env name=MY_VAR::some value + * @example + * ```typescript + * // Issue a warning annotation + * issueCommand('warning', {}, 'This is a warning message'); + * // Output: ::warning::This is a warning message + * + * // Set an environment variable + * issueCommand('set-env', { name: 'MY_VAR' }, 'some value'); + * // Output: ::set-env name=MY_VAR::some value + * + * // Add a secret mask + * issueCommand('add-mask', {}, 'secretValue123'); + * // Output: ::add-mask::secretValue123 + * ``` + * + * @internal + * This is an internal utility function that powers the public API functions + * such as setSecret, warning, error, and exportVariable. */ export function issueCommand( command: string, diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts index 0a141693..e9091ba2 100644 --- a/packages/core/src/core.ts +++ b/packages/core/src/core.ts @@ -94,7 +94,32 @@ export function exportVariable(name: string, val: any): void { /** * Registers a secret which will get masked from logs - * @param secret value of the secret + * + * @param secret - Value of the secret to be masked + * @remarks + * This function instructs the Actions runner to mask the specified value in any + * logs produced during the workflow run. Once registered, the secret value will + * be replaced with asterisks (***) whenever it appears in console output, logs, + * or error messages. + * + * This is useful for protecting sensitive information such as: + * - API keys + * - Access tokens + * - Authentication credentials + * - URL parameters containing signatures (SAS tokens) + * + * Note that masking only affects future logs; any previous appearances of the + * secret in logs before calling this function will remain unmasked. + * + * @example + * ```typescript + * // Register an API token as a secret + * const apiToken = "abc123xyz456"; + * setSecret(apiToken); + * + * // Now any logs containing this value will show *** instead + * console.log(`Using token: ${apiToken}`); // Outputs: "Using token: ***" + * ``` */ export function setSecret(secret: string): void { issueCommand('add-mask', {}, secret) From d13e6311f107af8f86b3686ee3f45631122202e8 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 14 Mar 2025 04:28:22 -0700 Subject: [PATCH 10/12] fix tests --- packages/artifact/__tests__/util.test.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 018bfc45..dd987d26 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -69,34 +69,28 @@ describe('maskSigUrl', () => { jest.clearAllMocks() }) - it('returns the original URL if no sig parameter is present', () => { + it('does nothing if no sig parameter is present', () => { const url = 'https://example.com' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe(url) + maskSigUrl(url) expect(setSecret).not.toHaveBeenCalled() }) it('masks the sig parameter in the middle of the URL and sets it as a secret', () => { const url = 'https://example.com/?param1=value1&sig=12345¶m2=value2' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe( - 'https://example.com/?param1=value1&sig=***¶m2=value2' - ) + maskSigUrl(url) expect(setSecret).toHaveBeenCalledWith('12345') expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) }) - it('returns the original URL if it is empty', () => { + it('does nothing if the URL is empty', () => { const url = '' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('') + maskSigUrl(url) expect(setSecret).not.toHaveBeenCalled() }) it('handles URLs with fragments', () => { const url = 'https://example.com?sig=12345#fragment' - const maskedUrl = maskSigUrl(url) - expect(maskedUrl).toBe('https://example.com/?sig=***#fragment') + maskSigUrl(url) expect(setSecret).toHaveBeenCalledWith('12345') expect(setSecret).toHaveBeenCalledWith(encodeURIComponent('12345')) }) From 39419dd8c312aec69d25e8e40a8e155c0a6fb583 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 14 Mar 2025 06:21:41 -0700 Subject: [PATCH 11/12] don't need to url encode or set var --- packages/artifact/src/internal/shared/util.ts | 3 --- packages/cache/src/internal/shared/util.ts | 3 --- 2 files changed, 6 deletions(-) diff --git a/packages/artifact/src/internal/shared/util.ts b/packages/artifact/src/internal/shared/util.ts index e346e639..d6c62794 100644 --- a/packages/artifact/src/internal/shared/util.ts +++ b/packages/artifact/src/internal/shared/util.ts @@ -93,11 +93,8 @@ export function maskSigUrl(url: string): void { try { const parsedUrl = new URL(url) const signature = parsedUrl.searchParams.get('sig') - if (signature) { setSecret(signature) - setSecret(encodeURIComponent(signature)) - parsedUrl.searchParams.set('sig', '***') } } catch (error) { debug( diff --git a/packages/cache/src/internal/shared/util.ts b/packages/cache/src/internal/shared/util.ts index bbce73fa..2e2d6434 100644 --- a/packages/cache/src/internal/shared/util.ts +++ b/packages/cache/src/internal/shared/util.ts @@ -22,11 +22,8 @@ export function maskSigUrl(url: string): void { try { const parsedUrl = new URL(url) const signature = parsedUrl.searchParams.get('sig') - if (signature) { setSecret(signature) - setSecret(encodeURIComponent(signature)) - parsedUrl.searchParams.set('sig', '***') } } catch (error) { debug( From 957d42e6c503430f00929e33a24c63be04206062 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 14 Mar 2025 06:38:57 -0700 Subject: [PATCH 12/12] add encoding back with extra tests --- packages/artifact/__tests__/util.test.ts | 53 +++++++++++++++++++ packages/artifact/src/internal/shared/util.ts | 1 + packages/cache/__tests__/util.test.ts | 53 +++++++++++++++++++ packages/cache/src/internal/shared/util.ts | 1 + 4 files changed, 108 insertions(+) diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index dd987d26..2649662e 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -96,6 +96,59 @@ describe('maskSigUrl', () => { }) }) +describe('maskSigUrl handles special characters in signatures', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('handles signatures with slashes', () => { + const url = 'https://example.com/?sig=abc/123' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc/123') + expect(setSecret).toHaveBeenCalledWith('abc%2F123') + }) + + it('handles signatures with plus signs', () => { + const url = 'https://example.com/?sig=abc+123' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc 123') + expect(setSecret).toHaveBeenCalledWith('abc%20123') + }) + + it('handles signatures with equals signs', () => { + const url = 'https://example.com/?sig=abc=123' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc=123') + expect(setSecret).toHaveBeenCalledWith('abc%3D123') + }) + + it('handles already percent-encoded signatures', () => { + const url = 'https://example.com/?sig=abc%2F123%3D' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc/123=') + expect(setSecret).toHaveBeenCalledWith('abc%2F123%3D') + }) + + it('handles complex Azure SAS signatures', () => { + const url = + 'https://example.com/container/file.txt?sig=nXyQIUj%2F%2F06Cxt80pBRYiiJlYqtPYg5sz%2FvEh5iHAhw%3D&se=2023-12-31' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith( + 'nXyQIUj//06Cxt80pBRYiiJlYqtPYg5sz/vEh5iHAhw=' + ) + expect(setSecret).toHaveBeenCalledWith( + 'nXyQIUj%2F%2F06Cxt80pBRYiiJlYqtPYg5sz%2FvEh5iHAhw%3D' + ) + }) + + it('handles signatures with multiple special characters', () => { + const url = 'https://example.com/?sig=a/b+c=d&e=f' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('a/b c=d') + expect(setSecret).toHaveBeenCalledWith('a%2Fb%20c%3Dd') + }) +}) + describe('maskSecretUrls', () => { beforeEach(() => { jest.clearAllMocks() diff --git a/packages/artifact/src/internal/shared/util.ts b/packages/artifact/src/internal/shared/util.ts index d6c62794..67120e27 100644 --- a/packages/artifact/src/internal/shared/util.ts +++ b/packages/artifact/src/internal/shared/util.ts @@ -95,6 +95,7 @@ export function maskSigUrl(url: string): void { const signature = parsedUrl.searchParams.get('sig') if (signature) { setSecret(signature) + setSecret(encodeURIComponent(signature)) } } catch (error) { debug( diff --git a/packages/cache/__tests__/util.test.ts b/packages/cache/__tests__/util.test.ts index 7cf071dd..3ba3bba7 100644 --- a/packages/cache/__tests__/util.test.ts +++ b/packages/cache/__tests__/util.test.ts @@ -35,6 +35,59 @@ describe('maskSigUrl', () => { }) }) +describe('maskSigUrl handles special characters in signatures', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('handles signatures with slashes', () => { + const url = 'https://example.com/?sig=abc/123' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc/123') + expect(setSecret).toHaveBeenCalledWith('abc%2F123') + }) + + it('handles signatures with plus signs', () => { + const url = 'https://example.com/?sig=abc+123' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc 123') + expect(setSecret).toHaveBeenCalledWith('abc%20123') + }) + + it('handles signatures with equals signs', () => { + const url = 'https://example.com/?sig=abc=123' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc=123') + expect(setSecret).toHaveBeenCalledWith('abc%3D123') + }) + + it('handles already percent-encoded signatures', () => { + const url = 'https://example.com/?sig=abc%2F123%3D' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('abc/123=') + expect(setSecret).toHaveBeenCalledWith('abc%2F123%3D') + }) + + it('handles complex Azure SAS signatures', () => { + const url = + 'https://example.com/container/file.txt?sig=nXyQIUj%2F%2F06Cxt80pBRYiiJlYqtPYg5sz%2FvEh5iHAhw%3D&se=2023-12-31' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith( + 'nXyQIUj//06Cxt80pBRYiiJlYqtPYg5sz/vEh5iHAhw=' + ) + expect(setSecret).toHaveBeenCalledWith( + 'nXyQIUj%2F%2F06Cxt80pBRYiiJlYqtPYg5sz%2FvEh5iHAhw%3D' + ) + }) + + it('handles signatures with multiple special characters', () => { + const url = 'https://example.com/?sig=a/b+c=d&e=f' + maskSigUrl(url) + expect(setSecret).toHaveBeenCalledWith('a/b c=d') + expect(setSecret).toHaveBeenCalledWith('a%2Fb%20c%3Dd') + }) +}) + describe('maskSecretUrls', () => { beforeEach(() => { jest.clearAllMocks() diff --git a/packages/cache/src/internal/shared/util.ts b/packages/cache/src/internal/shared/util.ts index 2e2d6434..36d2ebfd 100644 --- a/packages/cache/src/internal/shared/util.ts +++ b/packages/cache/src/internal/shared/util.ts @@ -24,6 +24,7 @@ export function maskSigUrl(url: string): void { const signature = parsedUrl.searchParams.get('sig') if (signature) { setSecret(signature) + setSecret(encodeURIComponent(signature)) } } catch (error) { debug(