diff --git a/.vscode/launch.json b/.vscode/launch.json new file mode 100644 index 0000000000..94965ca7b2 --- /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 ec9302721a..602c1d82c0 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,297 @@ describe('download-artifact', () => { {timeout: 2} ) expect(true).toBe(false) // should not be called - } catch (e) { + } catch (error: unknown) { + const e = error as Error 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) + }) + + it('should skip decompression when skipDecompress option is true even for zip content-type', async () => { + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetArtifactSuccess + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir, + {skipDecompress: true} + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify zip was saved as-is, not extracted + // When skipDecompress is true, the file should be saved with default name 'artifact' + const savedFilePath = path.join(fixtures.workspaceDir, 'artifact') + expect(fs.existsSync(savedFilePath)).toBe(true) + // The saved file should be the raw zip content + const savedContent = fs.readFileSync(savedFilePath) + const originalZipContent = fs.readFileSync(fixtures.exampleArtifact.path) + expect(savedContent).toEqual(originalZipContent) + }) + + it('should sanitize path traversal attempts in Content-Disposition filename', async () => { + const rawFileContent = 'malicious content' + const maliciousFileName = '../../../etc/passwd' + + const mockGetMaliciousFile = 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="${maliciousFileName}"` + message.push(Buffer.from(rawFileContent)) + message.push(null) + return { + message + } + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetMaliciousFile + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // Verify file was saved with sanitized name (just 'passwd', not the full path) + const sanitizedFileName = 'passwd' + const savedFilePath = path.join(fixtures.workspaceDir, sanitizedFileName) + expect(fs.existsSync(savedFilePath)).toBe(true) + expect(fs.readFileSync(savedFilePath, 'utf8')).toBe(rawFileContent) + + // Verify the file was NOT written outside the workspace directory + const maliciousPath = path.resolve( + fixtures.workspaceDir, + maliciousFileName + ) + expect(fs.existsSync(maliciousPath)).toBe(false) + }) + + it('should handle encoded path traversal attempts in Content-Disposition filename', async () => { + const rawFileContent = 'encoded malicious content' + // URL encoded version of ../../../etc/passwd + const encodedMaliciousFileName = '..%2F..%2F..%2Fetc%2Fpasswd' + + const mockGetEncodedMaliciousFile = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.headers['content-type'] = 'application/octet-stream' + message.headers['content-disposition'] = + `attachment; filename="${encodedMaliciousFileName}"` + message.push(Buffer.from(rawFileContent)) + message.push(null) + return { + message + } + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetEncodedMaliciousFile + } + } + ) + + await streamExtractExternal( + fixtures.blobStorageUrl, + fixtures.workspaceDir + ) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + // After decoding and sanitizing, should just be 'passwd' + const sanitizedFileName = 'passwd' + const savedFilePath = path.join(fixtures.workspaceDir, sanitizedFileName) + expect(fs.existsSync(savedFilePath)).toBe(true) + expect(fs.readFileSync(savedFilePath, 'utf8')).toBe(rawFileContent) + + // Verify the file was NOT written outside the workspace directory + const maliciousPathEncoded = path.resolve( + fixtures.workspaceDir, + encodedMaliciousFileName + ) + expect(fs.existsSync(maliciousPathEncoded)).toBe(false) + + const maliciousPath = path.resolve( + fixtures.workspaceDir, + "../../../etc/passwd" + ) + expect(fs.existsSync(maliciousPath)).toBe(false) + }) }) }) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index b760e77ab8..1ed22aeca3 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' @@ -43,12 +45,13 @@ async function exists(path: string): Promise { async function streamExtract( url: string, - directory: string + directory: string, + skipDecompress?: boolean ): Promise { let retryCount = 0 while (retryCount < 5) { try { - return await streamExtractExternal(url, directory) + return await streamExtractExternal(url, directory, {skipDecompress}) } catch (error) { retryCount++ core.debug( @@ -65,8 +68,9 @@ async function streamExtract( export async function streamExtractExternal( url: string, directory: string, - opts: {timeout: number} = {timeout: 30 * 1000} + opts: {timeout?: number; skipDecompress?: boolean} = {} ): Promise { + const {timeout = 30 * 1000, skipDecompress = false} = opts const client = new httpClient.HttpClient(getUserAgentString()) const response = await client.get(url) if (response.message.statusCode !== 200) { @@ -75,49 +79,85 @@ export async function streamExtractExternal( ) } + const contentType = response.message.headers['content-type'] || '' + const mimeType = contentType.split(';', 1)[0].trim().toLowerCase() + const isZip = + mimeType === 'application/zip' || + mimeType === 'application/x-zip-compressed' || + mimeType === 'application/zip-compressed' + + // 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]) { + // Sanitize fileName to prevent path traversal attacks + // Use path.basename to extract only the filename component + fileName = path.basename(decodeURIComponent(filenameMatch[1].trim())) + } + + core.debug( + `Content-Type: ${contentType}, isZip: ${isZip}, skipDecompress: ${skipDecompress}` + ) + core.debug( + `Content-Disposition: ${contentDisposition}, fileName: ${fileName}` + ) + let sha256Digest: string | undefined = undefined return new Promise((resolve, reject) => { const timerFn = (): void => { const timeoutError = new Error( - `Blob storage chunk did not respond in ${opts.timeout}ms` + `Blob storage chunk did not respond in ${timeout}ms` ) response.message.destroy(timeoutError) reject(timeoutError) } - const timer = setTimeout(timerFn, opts.timeout) + const timer = setTimeout(timerFn, 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() + .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 && !skipDecompress) { + // 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) + } }) } @@ -163,7 +203,11 @@ export async function downloadArtifactPublic( try { core.info(`Starting download of artifact to: ${downloadPath}`) - const extractResponse = await streamExtract(location, downloadPath) + const extractResponse = await streamExtract( + location, + downloadPath, + options?.skipDecompress + ) core.info(`Artifact download completed successfully.`) if (options?.expectedHash) { if (options?.expectedHash !== extractResponse.sha256Digest) { @@ -224,7 +268,11 @@ export async function downloadArtifactInternal( try { core.info(`Starting download of artifact to: ${downloadPath}`) - const extractResponse = await streamExtract(signedUrl, downloadPath) + const extractResponse = await streamExtract( + signedUrl, + downloadPath, + options?.skipDecompress + ) core.info(`Artifact download completed successfully.`) if (options?.expectedHash) { if (options?.expectedHash !== extractResponse.sha256Digest) { diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index 9675d39a3e..784ff98d1b 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/packages/artifact/src/internal/shared/interfaces.ts @@ -113,6 +113,12 @@ export interface DownloadArtifactOptions { * matches the expected hash. */ expectedHash?: string + + /** + * If true, the downloaded artifact will not be automatically extracted/decompressed. + * The artifact will be saved as-is to the destination path. + */ + skipDecompress?: boolean } export interface StreamExtractResponse {