From 2a356168e60847c0fbbcfe0436feed6452478793 Mon Sep 17 00:00:00 2001 From: Daniel Kennedy Date: Mon, 26 Jan 2026 13:48:31 -0500 Subject: [PATCH] Download artifact: don't extract the downloaded file if the content-type isn't a zip --- .vscode/launch.json | 36 ++++ .../__tests__/download-artifact.test.ts | 169 +++++++++++++++++- .../internal/download/download-artifact.ts | 83 ++++++--- 3 files changed, 261 insertions(+), 27 deletions(-) create mode 100644 .vscode/launch.json diff --git a/.vscode/launch.json b/.vscode/launch.json new file mode 100644 index 00000000..94965ca7 --- /dev/null +++ b/.vscode/launch.json @@ -0,0 +1,36 @@ +{ + "version": "0.2.0", + "configurations": [ + { + "type": "node", + "request": "launch", + "name": "Debug Jest Tests", + "program": "${workspaceFolder}/node_modules/jest/bin/jest.js", + "args": [ + "--runInBand", + "--testTimeout", + "10000" + ], + "cwd": "${workspaceFolder}", + "console": "integratedTerminal", + "internalConsoleOptions": "neverOpen", + "disableOptimisticBPs": true + }, + { + "type": "node", + "request": "launch", + "name": "Debug Current Test File", + "program": "${workspaceFolder}/node_modules/jest/bin/jest.js", + "args": [ + "--runInBand", + "--testTimeout", + "10000", + "${relativeFile}" + ], + "cwd": "${workspaceFolder}", + "console": "integratedTerminal", + "internalConsoleOptions": "neverOpen", + "disableOptimisticBPs": true + } + ] +} \ No newline at end of file diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 02390d6f..465beccd 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -104,6 +104,7 @@ const cleanup = async (): Promise => { const mockGetArtifactSuccess = jest.fn(() => { const message = new http.IncomingMessage(new net.Socket()) message.statusCode = 200 + message.headers['content-type'] = 'application/zip' message.push(fs.readFileSync(fixtures.exampleArtifact.path)) message.push(null) return { @@ -114,6 +115,7 @@ const mockGetArtifactSuccess = jest.fn(() => { const mockGetArtifactHung = jest.fn(() => { const message = new http.IncomingMessage(new net.Socket()) message.statusCode = 200 + message.headers['content-type'] = 'application/zip' // Don't push any data or call push(null) to end the stream // This creates a stream that hangs and never completes return { @@ -134,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'] = 'application/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 { @@ -623,6 +626,13 @@ describe('download-artifact', () => { }) describe('streamExtractExternal', () => { + beforeEach(async () => { + await setup() + // Create workspace directory for streamExtractExternal tests + await fs.promises.mkdir(fixtures.workspaceDir, {recursive: true}) + }) + afterEach(cleanup) + it('should fail if the timeout is exceeded', async () => { const mockSlowGetArtifact = jest.fn(mockGetArtifactHung) @@ -641,12 +651,169 @@ describe('download-artifact', () => { {timeout: 2} ) expect(true).toBe(false) // should not be called - } catch (e) { + } catch (e : any) { expect(e).toBeInstanceOf(Error) expect(e.message).toContain('did not respond in 2ms') expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) expect(mockSlowGetArtifact).toHaveBeenCalledTimes(1) } }) + + it('should extract zip file when content-type is application/zip', async () => { + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetArtifactSuccess + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify files were extracted (not saved as a single file) + await expectExtractedArchive(fixtures.workspaceDir) + }) + + it('should save raw file without extracting when content-type is not a zip', async () => { + const rawFileContent = 'This is a raw text file, not a zip' + const rawFileName = 'my-artifact.txt' + + const mockGetRawFile = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.headers['content-type'] = 'text/plain' + message.headers['content-disposition'] = `attachment; filename="${rawFileName}"` + message.push(Buffer.from(rawFileContent)) + message.push(null) + return { + message + } + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetRawFile + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify file was saved as-is, not extracted + const savedFilePath = path.join(fixtures.workspaceDir, rawFileName) + expect(fs.existsSync(savedFilePath)).toBe(true) + expect(fs.readFileSync(savedFilePath, 'utf8')).toBe(rawFileContent) + }) + + it('should save raw file with default name when content-disposition is missing', async () => { + const rawFileContent = 'Binary content here' + + const mockGetRawFileNoDisposition = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.headers['content-type'] = 'application/octet-stream' + // No content-disposition header + message.push(Buffer.from(rawFileContent)) + message.push(null) + return { + message + } + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetRawFileNoDisposition + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify file was saved with default name 'artifact' + const savedFilePath = path.join(fixtures.workspaceDir, 'artifact') + expect(fs.existsSync(savedFilePath)).toBe(true) + expect(fs.readFileSync(savedFilePath, 'utf8')).toBe(rawFileContent) + }) + + it('should not attempt to unzip when content-type is image/png', async () => { + const pngFileName = 'screenshot.png' + // Simple PNG header bytes for testing + const pngContent = Buffer.from([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]) + + const mockGetPngFile = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.headers['content-type'] = 'image/png' + message.headers['content-disposition'] = `attachment; filename="${pngFileName}"` + message.push(pngContent) + message.push(null) + return { + message + } + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetPngFile + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify PNG was saved as-is + const savedFilePath = path.join(fixtures.workspaceDir, pngFileName) + expect(fs.existsSync(savedFilePath)).toBe(true) + expect(fs.readFileSync(savedFilePath)).toEqual(pngContent) + }) + + it('should extract when content-type is application/x-zip-compressed', async () => { + const mockGetZipCompressed = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.headers['content-type'] = 'application/x-zip-compressed' + message.push(fs.readFileSync(fixtures.exampleArtifact.path)) + message.push(null) + return { + message + } + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetZipCompressed + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify files were extracted + await expectExtractedArchive(fixtures.workspaceDir) + }) }) }) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index a7404fe6..f8324791 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -1,6 +1,8 @@ import fs from 'fs/promises' +import * as fsSync from 'fs' import * as crypto from 'crypto' import * as stream from 'stream' +import * as path from 'path' import * as github from '@actions/github' import * as core from '@actions/core' @@ -21,6 +23,7 @@ import { } from '../../generated/index.js' import {getBackendIdsFromToken} from '../shared/util.js' import {ArtifactNotFoundError} from '../shared/errors.js' +import { on } from 'events' const scrubQueryParameters = (url: string): string => { const parsed = new URL(url) @@ -75,6 +78,26 @@ export async function streamExtractExternal( ) } + const contentType = response.message.headers['content-type'] || '' + const isZip = + contentType === 'application/zip' || + contentType === 'application/x-zip-compressed' || + contentType === 'zip' + + // Extract filename from Content-Disposition header + const contentDisposition = + response.message.headers['content-disposition'] || '' + let fileName = 'artifact' + const filenameMatch = contentDisposition.match( + /filename\*?=['"]?(?:UTF-\d['"]*)?([^;\r\n"']*)['"]?/i + ) + if (filenameMatch && filenameMatch[1]) { + fileName = decodeURIComponent(filenameMatch[1].trim()) + } + + core.debug(`Content-Type: ${contentType}, isZip: ${isZip}`) + core.debug(`Content-Disposition: ${contentDisposition}, fileName: ${fileName}`) + let sha256Digest: string | undefined = undefined return new Promise((resolve, reject) => { @@ -87,37 +110,45 @@ export async function streamExtractExternal( } const timer = setTimeout(timerFn, opts.timeout) + const onError = (error: Error): void => { + core.debug( + `response.message: Artifact download failed: ${error.message}` + ) + clearTimeout(timer) + reject(error) + } + const hashStream = crypto.createHash('sha256').setEncoding('hex') - const passThrough = new stream.PassThrough() + const passThrough = new stream.PassThrough() + .on('data', () => { + timer.refresh() + }) + .on('error', onError) response.message.pipe(passThrough) passThrough.pipe(hashStream) - const extractStream = passThrough - extractStream - .on('data', () => { - timer.refresh() - }) - .on('error', (error: Error) => { - core.debug( - `response.message: Artifact download failed: ${error.message}` - ) - clearTimeout(timer) - reject(error) - }) - .pipe(unzip.Extract({path: directory})) - .on('close', () => { - clearTimeout(timer) - if (hashStream) { - hashStream.end() - sha256Digest = hashStream.read() as string - core.info(`SHA256 digest of downloaded artifact is ${sha256Digest}`) - } - resolve({sha256Digest: `sha256:${sha256Digest}`}) - }) - .on('error', (error: Error) => { - reject(error) - }) + const onClose = (): void => { + clearTimeout(timer) + if (hashStream) { + hashStream.end() + sha256Digest = hashStream.read() as string + core.info(`SHA256 digest of downloaded artifact is ${sha256Digest}`) + } + resolve({sha256Digest: `sha256:${sha256Digest}`}) + } + + if (isZip) { + // Extract zip file + passThrough.pipe(unzip.Extract({path: directory})).on('close', onClose).on('error', onError) + } else { + // Save raw file without extracting + const filePath = path.join(directory, fileName) + const writeStream = fsSync.createWriteStream(filePath) + + core.info(`Downloading raw file (non-zip) to: ${filePath}`) + passThrough.pipe(writeStream).on('close', onClose).on('error', onError) + } }) }