From 3ac34ffcb7cbe45e9ca22ce26e0210ca0b6e711e Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Wed, 12 Mar 2025 03:17:35 -0700 Subject: [PATCH] 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[]) +}