From 4ec46bb6738a79e15e8ecd01bbaaddd112cba3db Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Thu, 14 Nov 2024 12:19:21 +0100 Subject: [PATCH] artifact(download): skip non-zip files --- .../__tests__/download-artifact.test.ts | 51 +++++++++++++++++++ .../internal/download/download-artifact.ts | 38 +++++++++----- .../src/internal/shared/interfaces.ts | 5 ++ 3 files changed, 80 insertions(+), 14 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index f73c9fc767..84eca6de84 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -104,6 +104,18 @@ const cleanup = async (): Promise => { const mockGetArtifactSuccess = jest.fn(() => { const message = new http.IncomingMessage(new net.Socket()) message.statusCode = 200 + message.headers['content-type'] = 'zip' + message.push(fs.readFileSync(fixtures.exampleArtifact.path)) + message.push(null) + return { + message + } +}) + +const mockGetArtifactGzip = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.headers['content-type'] = 'application/gzip' message.push(fs.readFileSync(fixtures.exampleArtifact.path)) message.push(null) return { @@ -124,6 +136,7 @@ const mockGetArtifactFailure = jest.fn(() => { const mockGetArtifactMalicious = jest.fn(() => { const message = new http.IncomingMessage(new net.Socket()) message.statusCode = 200 + message.headers['content-type'] = 'zip' message.push(fs.readFileSync(path.join(__dirname, 'fixtures', 'evil.zip'))) // evil.zip contains files that are formatted x/../../etc/hosts message.push(null) return { @@ -178,6 +191,7 @@ describe('download-artifact', () => { ) expectExtractedArchive(fixtures.workspaceDir) expect(response.downloadPath).toBe(fixtures.workspaceDir) + expect(response.skipped).toBe(false) }) it('should not allow path traversal from malicious artifacts', async () => { @@ -231,6 +245,7 @@ describe('download-artifact', () => { ).toBe(true) expect(response.downloadPath).toBe(fixtures.workspaceDir) + expect(response.skipped).toBe(false) }) it('should successfully download an artifact to user defined path', async () => { @@ -280,6 +295,7 @@ describe('download-artifact', () => { ) expectExtractedArchive(customPath) expect(response.downloadPath).toBe(customPath) + expect(response.skipped).toBe(false) }) it('should fail if download artifact API does not respond with location', async () => { @@ -316,6 +332,7 @@ describe('download-artifact', () => { // mock http client to delay response data by 30s const msg = new http.IncomingMessage(new net.Socket()) msg.statusCode = 200 + msg.headers['content-type'] = 'zip' const mockGet = jest.fn(async () => { return new Promise((resolve, reject) => { @@ -444,7 +461,39 @@ describe('download-artifact', () => { ) expect(mockGetArtifactSuccess).toHaveBeenCalledTimes(1) expect(response.downloadPath).toBe(fixtures.workspaceDir) + expect(response.skipped).toBe(false) }, 28000) + + it('should skip if artifact does not have the right content type', async () => { + const downloadArtifactMock = github.getOctokit(fixtures.token).rest + .actions.downloadArtifact as MockedDownloadArtifact + downloadArtifactMock.mockResolvedValueOnce({ + headers: { + location: fixtures.blobStorageUrl + }, + status: 302, + url: '', + data: Buffer.from('') + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetArtifactGzip + } + } + ) + + const response = await downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + expect(response.skipped).toBe(true) + }) }) describe('internal', () => { @@ -499,6 +548,7 @@ describe('download-artifact', () => { expectExtractedArchive(fixtures.workspaceDir) expect(response.downloadPath).toBe(fixtures.workspaceDir) + expect(response.skipped).toBe(false) expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) expect(mockListArtifacts).toHaveBeenCalledWith({ idFilter: { @@ -550,6 +600,7 @@ describe('download-artifact', () => { expectExtractedArchive(customPath) expect(response.downloadPath).toBe(customPath) + expect(response.skipped).toBe(false) expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) expect(mockListArtifacts).toHaveBeenCalledWith({ idFilter: { diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index dc54f6fe81..c0011499f0 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -37,12 +37,11 @@ async function exists(path: string): Promise { } } -async function streamExtract(url: string, directory: string): Promise { +async function streamExtract(url: string, directory: string): Promise { let retryCount = 0 while (retryCount < 5) { try { - await streamExtractExternal(url, directory) - return + return await streamExtractExternal(url, directory) } catch (error) { retryCount++ core.debug( @@ -59,18 +58,23 @@ async function streamExtract(url: string, directory: string): Promise { export async function streamExtractExternal( url: string, directory: string -): Promise { +): Promise { const client = new httpClient.HttpClient(getUserAgentString()) const response = await client.get(url) if (response.message.statusCode !== 200) { throw new Error( `Unexpected HTTP response from blob storage: ${response.message.statusCode} ${response.message.statusMessage}` ) + } else if (response.message.headers['content-type'] !== 'zip') { + core.debug( + `Invalid content-type: ${response.message.headers['content-type']}, skipping download` + ) + return false } const timeout = 30 * 1000 // 30 seconds - return new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { const timerFn = (): void => { response.message.destroy( new Error(`Blob storage chunk did not respond in ${timeout}ms`) @@ -92,7 +96,7 @@ export async function streamExtractExternal( .pipe(unzip.Extract({path: directory})) .on('close', () => { clearTimeout(timer) - resolve() + resolve(true) }) .on('error', (error: Error) => { reject(error) @@ -140,13 +144,16 @@ export async function downloadArtifactPublic( try { core.info(`Starting download of artifact to: ${downloadPath}`) - await streamExtract(location, downloadPath) - core.info(`Artifact download completed successfully.`) + if (await streamExtract(location, downloadPath)) { + core.info(`Artifact download completed successfully.`) + return {downloadPath, skipped: false} + } else { + core.info(`Artifact download skipped.`) + return {downloadPath, skipped: true} + } } catch (error) { throw new Error(`Unable to download and extract artifact: ${error.message}`) } - - return {downloadPath} } export async function downloadArtifactInternal( @@ -192,13 +199,16 @@ export async function downloadArtifactInternal( try { core.info(`Starting download of artifact to: ${downloadPath}`) - await streamExtract(signedUrl, downloadPath) - core.info(`Artifact download completed successfully.`) + if (await streamExtract(signedUrl, downloadPath)) { + core.info(`Artifact download completed successfully.`) + return {downloadPath, skipped: false} + } else { + core.info(`Artifact download skipped.`) + return {downloadPath, skipped: true} + } } catch (error) { throw new Error(`Unable to download and extract artifact: ${error.message}`) } - - return {downloadPath} } async function resolveOrCreateDirectory( diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index eb55ae8beb..fb7c74eba4 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/packages/artifact/src/internal/shared/interfaces.ts @@ -86,6 +86,11 @@ export interface DownloadArtifactResponse { * The path where the artifact was downloaded to */ downloadPath?: string + + /** + * If the artifact download was skipped + */ + skipped?: boolean } /**