From d4179a3792c2ad43075965bea847aa725c8bdc85 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 6 Mar 2025 14:25:32 -0800 Subject: [PATCH 1/9] Add secret and signature masking for cache and artifact packages --- __tests__/artifactTwirpClient.test.ts | 84 ++++++++++++++++++++ src/internal/shared/artifact-twirp-client.ts | 20 ++++- 2 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 __tests__/artifactTwirpClient.test.ts diff --git a/__tests__/artifactTwirpClient.test.ts b/__tests__/artifactTwirpClient.test.ts new file mode 100644 index 0000000..035031e --- /dev/null +++ b/__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/src/internal/shared/artifact-twirp-client.ts b/src/internal/shared/artifact-twirp-client.ts index 00c65bc..1d3e8c6 100644 --- a/src/internal/shared/artifact-twirp-client.ts +++ b/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} From 5040e42c6cda4296153cfd5f9178cded27535b8f Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 7 Mar 2025 06:01:25 -0800 Subject: [PATCH 2/9] Instead of using utility method in core lib, use method in both twirp clients --- __tests__/artifactTwirpClient.test.ts | 28 ++++++++++++++------ src/internal/shared/artifact-twirp-client.ts | 19 ++++++++++--- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/__tests__/artifactTwirpClient.test.ts b/__tests__/artifactTwirpClient.test.ts index 035031e..882c7ef 100644 --- a/__tests__/artifactTwirpClient.test.ts +++ b/__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/src/internal/shared/artifact-twirp-client.ts b/src/internal/shared/artifact-twirp-client.ts index 1d3e8c6..2d38911 100644 --- a/src/internal/shared/artifact-twirp-client.ts +++ b/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') } } From 2a461120c50b906363cc3c87285a0e2f285d5a24 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Mon, 10 Mar 2025 06:47:52 -0700 Subject: [PATCH 3/9] masks the whole URL, update tests --- __tests__/artifactTwirpClient.test.ts | 104 ++++++++++++------- src/internal/shared/artifact-twirp-client.ts | 40 +++---- 2 files changed, 88 insertions(+), 56 deletions(-) diff --git a/__tests__/artifactTwirpClient.test.ts b/__tests__/artifactTwirpClient.test.ts index 882c7ef..18c7218 100644 --- a/__tests__/artifactTwirpClient.test.ts +++ b/__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/src/internal/shared/artifact-twirp-client.ts b/src/internal/shared/artifact-twirp-client.ts index 2d38911..57a73ad 100644 --- a/src/internal/shared/artifact-twirp-client.ts +++ b/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') } } From c01a63223dca586ffedb04825a494408c17d456b Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Wed, 12 Mar 2025 03:17:35 -0700 Subject: [PATCH 4/9] Mask different situations, malformed URL, encoded, decoded, raw signatures, nested parameters, and moved to a utility file --- __tests__/artifactTwirpClient.test.ts | 124 -------- __tests__/util.test.ts | 289 +++++++++++++++++++ src/internal/shared/artifact-twirp-client.ts | 37 +-- src/internal/shared/util.ts | 168 +++++++++++ 4 files changed, 460 insertions(+), 158 deletions(-) delete mode 100644 __tests__/artifactTwirpClient.test.ts diff --git a/__tests__/artifactTwirpClient.test.ts b/__tests__/artifactTwirpClient.test.ts deleted file mode 100644 index 18c7218..0000000 --- a/__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/__tests__/util.test.ts b/__tests__/util.test.ts index 76fe4e1..34535db 100644 --- a/__tests__/util.test.ts +++ b/__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/src/internal/shared/artifact-twirp-client.ts b/src/internal/shared/artifact-twirp-client.ts index 57a73ad..8446151 100644 --- a/src/internal/shared/artifact-twirp-client.ts +++ b/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/src/internal/shared/util.ts b/src/internal/shared/util.ts index 07392b3..754c6b7 100644 --- a/src/internal/shared/util.ts +++ b/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[]) +} From a16f2308922f751197990e7f9e6defa07ad468d9 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 13 Mar 2025 04:23:45 -0700 Subject: [PATCH 5/9] PR feedback, back to simplified approach, no export on client as well --- __tests__/util.test.ts | 180 +------------------ src/internal/shared/artifact-twirp-client.ts | 2 +- src/internal/shared/util.ts | 148 ++------------- 3 files changed, 17 insertions(+), 313 deletions(-) diff --git a/__tests__/util.test.ts b/__tests__/util.test.ts index 34535db..018bfc4 100644 --- a/__tests__/util.test.ts +++ b/__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/src/internal/shared/artifact-twirp-client.ts b/src/internal/shared/artifact-twirp-client.ts index 8446151..5749912 100644 --- a/src/internal/shared/artifact-twirp-client.ts +++ b/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/src/internal/shared/util.ts b/src/internal/shared/util.ts index 754c6b7..61f2ab6 100644 --- a/src/internal/shared/util.ts +++ b/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[]) } From 26ff5dd756779eaa4d575df75053d6d1d3084bc1 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Thu, 13 Mar 2025 04:47:49 -0700 Subject: [PATCH 6/9] update ts docs --- src/internal/shared/util.ts | 44 +++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/internal/shared/util.ts b/src/internal/shared/util.ts index 61f2ab6..e346e63 100644 --- a/src/internal/shared/util.ts +++ b/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) { From 645586ea7c3a7031bb78cafee5fd70b94dd4c686 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 14 Mar 2025 04:28:22 -0700 Subject: [PATCH 7/9] fix tests --- __tests__/util.test.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/__tests__/util.test.ts b/__tests__/util.test.ts index 018bfc4..dd987d2 100644 --- a/__tests__/util.test.ts +++ b/__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 4899a08e7c340e1ae63dda2b88ff1e4c429f7dd3 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 14 Mar 2025 06:21:41 -0700 Subject: [PATCH 8/9] don't need to url encode or set var --- src/internal/shared/util.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/internal/shared/util.ts b/src/internal/shared/util.ts index e346e63..d6c6279 100644 --- a/src/internal/shared/util.ts +++ b/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( From 1ad7546f62d6cb6ca0effb1896f0b606cf540395 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Fri, 14 Mar 2025 06:38:57 -0700 Subject: [PATCH 9/9] add encoding back with extra tests --- __tests__/util.test.ts | 53 +++++++++++++++++++++++++++++++++++++ src/internal/shared/util.ts | 1 + 2 files changed, 54 insertions(+) diff --git a/__tests__/util.test.ts b/__tests__/util.test.ts index dd987d2..2649662 100644 --- a/__tests__/util.test.ts +++ b/__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/src/internal/shared/util.ts b/src/internal/shared/util.ts index d6c6279..67120e2 100644 --- a/src/internal/shared/util.ts +++ b/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(