From bc5b3a85ae98c7ef52f354703f87e9b94c85ea61 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 17:16:32 -0500 Subject: [PATCH 01/20] use on entry --- packages/artifact/__tests__/download-artifact.test.ts | 5 +++-- .../artifact/src/internal/download/download-artifact.ts | 9 ++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 1c0c9b0d..7d8fead5 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -13,7 +13,7 @@ import { streamExtractExternal } from '../src/internal/download/download-artifact' import {getUserAgentString} from '../src/internal/shared/user-agent' -import {noopLogs} from './common' +//import {noopLogs} from './common' import * as config from '../src/internal/shared/config' import {ArtifactServiceClientJSON} from '../src/generated' import * as util from '../src/internal/shared/util' @@ -83,12 +83,13 @@ const createTestArchive = async (): Promise => { const expectExtractedArchive = async (dir: string): Promise => { for (const file of fixtures.exampleArtifact.files) { const filePath = path.join(dir, file.path) + console.log('Checking file:', filePath) expect(fs.readFileSync(filePath, 'utf8')).toEqual(file.content) } } const setup = async (): Promise => { - noopLogs() + //noopLogs() await fs.promises.mkdir(testDir, {recursive: true}) await createTestArchive() diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index dc54f6fe..995dbafb 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -1,4 +1,6 @@ import fs from 'fs/promises' +import {createWriteStream} from 'fs' +import * as path from 'path' import * as github from '@actions/github' import * as core from '@actions/core' import * as httpClient from '@actions/http-client' @@ -89,7 +91,12 @@ export async function streamExtractExternal( clearTimeout(timer) reject(error) }) - .pipe(unzip.Extract({path: directory})) + .pipe(unzip.Parse()) + .on('entry', (entry: unzip.Entry) => { + const fullPath = path.normalize(path.join(directory, entry.path)) + core.debug(`Extracting artifact entry: ${fullPath}`) + entry.pipe(createWriteStream(fullPath)) + }) .on('close', () => { clearTimeout(timer) resolve() From 81d5e48db0a475d61abc3e79f8640b26f515ebcb Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 17:51:15 -0500 Subject: [PATCH 02/20] update tests --- packages/artifact/__tests__/download-artifact.test.ts | 4 ++-- packages/artifact/src/internal/download/download-artifact.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 7d8fead5..574f6b15 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -13,7 +13,7 @@ import { streamExtractExternal } from '../src/internal/download/download-artifact' import {getUserAgentString} from '../src/internal/shared/user-agent' -//import {noopLogs} from './common' +import {noopLogs} from './common' import * as config from '../src/internal/shared/config' import {ArtifactServiceClientJSON} from '../src/generated' import * as util from '../src/internal/shared/util' @@ -89,7 +89,7 @@ const expectExtractedArchive = async (dir: string): Promise => { } const setup = async (): Promise => { - //noopLogs() + noopLogs() await fs.promises.mkdir(testDir, {recursive: true}) await createTestArchive() diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 995dbafb..a06dcf28 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -97,7 +97,7 @@ export async function streamExtractExternal( core.debug(`Extracting artifact entry: ${fullPath}`) entry.pipe(createWriteStream(fullPath)) }) - .on('close', () => { + .on('end', () => { clearTimeout(timer) resolve() }) From b956d8a4dd9857695d632371161695cc5aa1b4ef Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 17:55:53 -0500 Subject: [PATCH 03/20] audit, lint, format --- package-lock.json | 6 +++--- packages/artifact/__tests__/download-artifact.test.ts | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1d43fbf3..7575e429 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6664,9 +6664,9 @@ } }, "node_modules/ip": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.0.tgz", - "integrity": "sha512-WKa+XuLG1A1R0UWhl2+1XQSi+fZWMsYKffMZTTYsiZaUD8k2yDAj5atimTUD2TZkyCkNEeYE5NhFZmupOGtjYQ==", + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.1.tgz", + "integrity": "sha512-lJUL9imLTNi1ZfXT+DU6rBBdbiKGBuay9B6xGSPVjUeQwaH1RIGqef8RZkUtHioLmSNpPR5M4HVKJGm1j8FWVQ==", "dev": true }, "node_modules/is-array-buffer": { diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 574f6b15..1c0c9b0d 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -83,7 +83,6 @@ const createTestArchive = async (): Promise => { const expectExtractedArchive = async (dir: string): Promise => { for (const file of fixtures.exampleArtifact.files) { const filePath = path.join(dir, file.path) - console.log('Checking file:', filePath) expect(fs.readFileSync(filePath, 'utf8')).toEqual(file.content) } } From 9dea373bba6cf394ffc1a74b7129ffdd68034e5c Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 20:29:42 -0500 Subject: [PATCH 04/20] wait for upload to finish --- .../src/internal/download/download-artifact.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index a06dcf28..9d337043 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -80,6 +80,7 @@ export async function streamExtractExternal( } const timer = setTimeout(timerFn, timeout) + const promises: Promise[] = [] response.message .on('data', () => { timer.refresh() @@ -95,11 +96,18 @@ export async function streamExtractExternal( .on('entry', (entry: unzip.Entry) => { const fullPath = path.normalize(path.join(directory, entry.path)) core.debug(`Extracting artifact entry: ${fullPath}`) - entry.pipe(createWriteStream(fullPath)) + const writeStream = createWriteStream(fullPath) + promises.push(new Promise((resolve, reject) => { + writeStream.on('finish', () => resolve()) + writeStream.on('error', reject) + })) + entry.pipe(writeStream) }) .on('end', () => { clearTimeout(timer) - resolve() + Promise.all(promises) + .then(() => resolve()) + .catch(error => reject(error)) }) .on('error', (error: Error) => { reject(error) From 31c555afdae9db6cc687cc4913b371f6dd1be64e Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 20:31:49 -0500 Subject: [PATCH 05/20] prettier --- .../src/internal/download/download-artifact.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 9d337043..ef4d3fca 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -97,10 +97,12 @@ export async function streamExtractExternal( const fullPath = path.normalize(path.join(directory, entry.path)) core.debug(`Extracting artifact entry: ${fullPath}`) const writeStream = createWriteStream(fullPath) - promises.push(new Promise((resolve, reject) => { - writeStream.on('finish', () => resolve()) - writeStream.on('error', reject) - })) + promises.push( + new Promise((resolve, reject) => { + writeStream.on('finish', () => resolve()) + writeStream.on('error', reject) + }) + ) entry.pipe(writeStream) }) .on('end', () => { From a24b9c018472b37d4672b1e9e956f5d6b59d6e76 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 21:54:54 -0500 Subject: [PATCH 06/20] handle directories --- .../internal/download/download-artifact.ts | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index ef4d3fca..5e893b4c 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -72,7 +72,7 @@ export async function streamExtractExternal( const timeout = 30 * 1000 // 30 seconds - return new Promise((resolve, reject) => { + return new Promise(async (resolve, reject) => { const timerFn = (): void => { response.message.destroy( new Error(`Blob storage chunk did not respond in ${timeout}ms`) @@ -93,23 +93,42 @@ export async function streamExtractExternal( reject(error) }) .pipe(unzip.Parse()) - .on('entry', (entry: unzip.Entry) => { + .on('entry', async (entry: unzip.Entry) => { const fullPath = path.normalize(path.join(directory, entry.path)) core.debug(`Extracting artifact entry: ${fullPath}`) - const writeStream = createWriteStream(fullPath) - promises.push( - new Promise((resolve, reject) => { - writeStream.on('finish', () => resolve()) - writeStream.on('error', reject) - }) - ) - entry.pipe(writeStream) + if (entry.type === 'Directory') { + if (!(await exists(fullPath))) { + await fs.mkdir(fullPath, {recursive: true}) + } + entry.autodrain() + } else { + if (!(await exists(path.dirname(fullPath)))) { + await fs.mkdir(path.dirname(fullPath), {recursive: true}) + } + const writeStream = createWriteStream(fullPath) + promises.push( + new Promise((resolve, reject) => { + writeStream.on('finish', () => { + console.log(`Finished writing ${fullPath}`) + resolve() + }) + writeStream.on('error', reject) + }) + ) + entry.pipe(writeStream) + } }) - .on('end', () => { + .on('end', async () => { + console.log('All entries have been extracted') clearTimeout(timer) - Promise.all(promises) - .then(() => resolve()) - .catch(error => reject(error)) + try { + console.log('Waiting for all write streams to finish') + await Promise.all(promises) + console.log('All write streams have finished') + resolve() + } catch (error) { + reject(error) + } }) .on('error', (error: Error) => { reject(error) From 83731e6528966f6dccc8284d5eea8c528a635f02 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Thu, 22 Feb 2024 22:06:32 -0500 Subject: [PATCH 07/20] remove awaits from on entry --- .../src/internal/download/download-artifact.ts | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 5e893b4c..cd2f8490 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -72,7 +72,7 @@ export async function streamExtractExternal( const timeout = 30 * 1000 // 30 seconds - return new Promise(async (resolve, reject) => { + return new Promise((resolve, reject) => { const timerFn = (): void => { response.message.destroy( new Error(`Blob storage chunk did not respond in ${timeout}ms`) @@ -93,23 +93,17 @@ export async function streamExtractExternal( reject(error) }) .pipe(unzip.Parse()) - .on('entry', async (entry: unzip.Entry) => { + .on('entry', (entry: unzip.Entry) => { const fullPath = path.normalize(path.join(directory, entry.path)) core.debug(`Extracting artifact entry: ${fullPath}`) if (entry.type === 'Directory') { - if (!(await exists(fullPath))) { - await fs.mkdir(fullPath, {recursive: true}) - } + promises.push(fs.mkdir(fullPath, {recursive: true}).then(() => {})) entry.autodrain() } else { - if (!(await exists(path.dirname(fullPath)))) { - await fs.mkdir(path.dirname(fullPath), {recursive: true}) - } const writeStream = createWriteStream(fullPath) promises.push( new Promise((resolve, reject) => { writeStream.on('finish', () => { - console.log(`Finished writing ${fullPath}`) resolve() }) writeStream.on('error', reject) @@ -119,12 +113,9 @@ export async function streamExtractExternal( } }) .on('end', async () => { - console.log('All entries have been extracted') clearTimeout(timer) try { - console.log('Waiting for all write streams to finish') await Promise.all(promises) - console.log('All write streams have finished') resolve() } catch (error) { reject(error) From 1e326de4744cd44f766a6a1ca03f6fcb532a9fcc Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 08:28:37 -0500 Subject: [PATCH 08/20] use existing function --- packages/artifact/src/internal/download/download-artifact.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index cd2f8490..f040d2f5 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -97,9 +97,12 @@ export async function streamExtractExternal( const fullPath = path.normalize(path.join(directory, entry.path)) core.debug(`Extracting artifact entry: ${fullPath}`) if (entry.type === 'Directory') { - promises.push(fs.mkdir(fullPath, {recursive: true}).then(() => {})) + promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) entry.autodrain() } else { + promises.push( + resolveOrCreateDirectory(path.dirname(fullPath)).then(() => {}) + ) const writeStream = createWriteStream(fullPath) promises.push( new Promise((resolve, reject) => { From d3301c9bc26a357dff44590e92f2918ef7e48b31 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 08:42:23 -0500 Subject: [PATCH 09/20] update path parsing --- packages/artifact/src/internal/download/download-artifact.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index f040d2f5..62e8577e 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -94,7 +94,8 @@ export async function streamExtractExternal( }) .pipe(unzip.Parse()) .on('entry', (entry: unzip.Entry) => { - const fullPath = path.normalize(path.join(directory, entry.path)) + const entryPath = path.normalize(entry.path).replace(/^(\.\.(\/|\\|$))+/, '') + const fullPath = path.join(directory, entryPath) core.debug(`Extracting artifact entry: ${fullPath}`) if (entry.type === 'Directory') { promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) From 8d03fb4787403d2c678f74684d842bcb24540b60 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 08:46:56 -0500 Subject: [PATCH 10/20] prettier --- packages/artifact/src/internal/download/download-artifact.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 62e8577e..358044f1 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -94,7 +94,9 @@ export async function streamExtractExternal( }) .pipe(unzip.Parse()) .on('entry', (entry: unzip.Entry) => { - const entryPath = path.normalize(entry.path).replace(/^(\.\.(\/|\\|$))+/, '') + const entryPath = path + .normalize(entry.path) + .replace(/^(\.\.(\/|\\|$))+/, '') const fullPath = path.join(directory, entryPath) core.debug(`Extracting artifact entry: ${fullPath}`) if (entry.type === 'Directory') { From e9005f772745209f88df5406a2491964465596af Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 10:54:12 -0500 Subject: [PATCH 11/20] ensure no path traversal --- .../__tests__/download-artifact.test.ts | 57 ++++++++++++++++++ packages/artifact/__tests__/fixtures/evil.zip | Bin 0 -> 238 bytes 2 files changed, 57 insertions(+) create mode 100644 packages/artifact/__tests__/fixtures/evil.zip diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 1c0c9b0d..a593c709 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -121,6 +121,16 @@ const mockGetArtifactFailure = jest.fn(() => { } }) +const mockGetArtifactMalicious = jest.fn(() => { + const message = new http.IncomingMessage(new net.Socket()) + message.statusCode = 200 + message.push(fs.readFileSync(path.join(__dirname, 'fixtures', 'evil.zip'))) + message.push(null) + return { + message + } +}) + describe('download-artifact', () => { describe('public', () => { beforeEach(setup) @@ -170,6 +180,53 @@ describe('download-artifact', () => { expect(response.downloadPath).toBe(fixtures.workspaceDir) }) + it('should not allow path traversal from malicious artifacts', 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: mockGetArtifactMalicious + } + } + ) + + const response = await downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) + + expect(downloadArtifactMock).toHaveBeenCalledWith({ + owner: fixtures.repositoryOwner, + repo: fixtures.repositoryName, + artifact_id: fixtures.artifactID, + archive_format: 'zip', + request: { + redirect: 'manual' + } + }) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + expect(mockGetArtifactMalicious).toHaveBeenCalledWith( + fixtures.blobStorageUrl + ) + expect( + fs.readFileSync(path.join(fixtures.workspaceDir, 'etc/hosts'), 'utf8') + ).toEqual('foo') + expect(response.downloadPath).toBe(fixtures.workspaceDir) + }) + it('should successfully download an artifact to user defined path', async () => { const customPath = path.join(testDir, 'custom') diff --git a/packages/artifact/__tests__/fixtures/evil.zip b/packages/artifact/__tests__/fixtures/evil.zip new file mode 100644 index 0000000000000000000000000000000000000000..71d842b8205bf1718df485886ef30edb26e858fc GIT binary patch literal 238 zcmWIWW@Zs#U|`^22nh<0P)sfEVFmJ-ftUw~_4M>pOOo|7@{3D~y-%NKX9&QloFAeb ziw;I65oX+00JVWZ10#q+)*j%ETPMgi7-(SB2Qq;=Q8lx&ffO(SVL6aa1aTMuwt6QT literal 0 HcmV?d00001 From 76489f433bfce35e3ff6732c04455d80f73e32e1 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 11:59:36 -0500 Subject: [PATCH 12/20] attempt with comparing index --- packages/artifact/__tests__/download-artifact.test.ts | 6 +++--- .../src/internal/download/download-artifact.ts | 10 ++++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index a593c709..11707a64 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -13,7 +13,7 @@ import { streamExtractExternal } from '../src/internal/download/download-artifact' import {getUserAgentString} from '../src/internal/shared/user-agent' -import {noopLogs} from './common' +//import {noopLogs} from './common' import * as config from '../src/internal/shared/config' import {ArtifactServiceClientJSON} from '../src/generated' import * as util from '../src/internal/shared/util' @@ -88,7 +88,7 @@ const expectExtractedArchive = async (dir: string): Promise => { } const setup = async (): Promise => { - noopLogs() + //noopLogs() await fs.promises.mkdir(testDir, {recursive: true}) await createTestArchive() @@ -180,7 +180,7 @@ describe('download-artifact', () => { expect(response.downloadPath).toBe(fixtures.workspaceDir) }) - it('should not allow path traversal from malicious artifacts', async () => { + it.only('should not allow path traversal from malicious artifacts', async () => { const downloadArtifactMock = github.getOctokit(fixtures.token).rest .actions.downloadArtifact as MockedDownloadArtifact downloadArtifactMock.mockResolvedValueOnce({ diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 358044f1..9781ce90 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -94,10 +94,12 @@ export async function streamExtractExternal( }) .pipe(unzip.Parse()) .on('entry', (entry: unzip.Entry) => { - const entryPath = path - .normalize(entry.path) - .replace(/^(\.\.(\/|\\|$))+/, '') - const fullPath = path.join(directory, entryPath) + console.log(`entryPath: ${entry.path}`) + const fullPath = path.normalize(path.join(directory, entry.path)) + console.log(`fullPath: ${fullPath}`) + if (fullPath.indexOf(directory) != 0) { + reject(new Error(`Invalid file path: ${fullPath}`)) + } core.debug(`Extracting artifact entry: ${fullPath}`) if (entry.type === 'Directory') { promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) From 4256ea99c5003c99de2118b78a39e80945434672 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 13:41:40 -0500 Subject: [PATCH 13/20] update test case and handling --- .../__tests__/download-artifact.test.ts | 10 +++---- packages/artifact/__tests__/fixtures/evil.zip | Bin 238 -> 246 bytes .../internal/download/download-artifact.ts | 25 ++++++++++++------ 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 11707a64..3f0efc9d 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -180,7 +180,7 @@ describe('download-artifact', () => { expect(response.downloadPath).toBe(fixtures.workspaceDir) }) - it.only('should not allow path traversal from malicious artifacts', async () => { + it('should not allow path traversal from malicious artifacts', async () => { const downloadArtifactMock = github.getOctokit(fixtures.token).rest .actions.downloadArtifact as MockedDownloadArtifact downloadArtifactMock.mockResolvedValueOnce({ @@ -200,12 +200,12 @@ describe('download-artifact', () => { } ) - const response = await downloadArtifactPublic( + await expect(downloadArtifactPublic( fixtures.artifactID, fixtures.repositoryOwner, fixtures.repositoryName, fixtures.token - ) + )).rejects.toBeInstanceOf(Error) expect(downloadArtifactMock).toHaveBeenCalledWith({ owner: fixtures.repositoryOwner, @@ -221,10 +221,6 @@ describe('download-artifact', () => { expect(mockGetArtifactMalicious).toHaveBeenCalledWith( fixtures.blobStorageUrl ) - expect( - fs.readFileSync(path.join(fixtures.workspaceDir, 'etc/hosts'), 'utf8') - ).toEqual('foo') - expect(response.downloadPath).toBe(fixtures.workspaceDir) }) it('should successfully download an artifact to user defined path', async () => { diff --git a/packages/artifact/__tests__/fixtures/evil.zip b/packages/artifact/__tests__/fixtures/evil.zip index 71d842b8205bf1718df485886ef30edb26e858fc..d345590fd7f3c863837c3344b26714b631f5ce3e 100644 GIT binary patch literal 246 zcmWIWW@Zs#U|`^2V9XAWP)sfEVFmJ-ftU}7EA;jB^ixZc^)vE|ONzZupJ!(Xz^OqH zsG$<10gE0+CJ|=b)&MnvK?5U*LbWu&8@FDNjWE!_Xb5Ct*dO4{$_7%x1ccQpOOo|7@{3D~y-%NKX9&QloFAeb ziw;I65oX+00JVWZ10#q+)*j%ETPMgi7-(SB2Qq;=Q8lx&ffO(SVL6aa1aTMuwt6QT diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 9781ce90..1c6c8ec2 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -46,6 +46,9 @@ async function streamExtract(url: string, directory: string): Promise { await streamExtractExternal(url, directory) return } catch (error) { + if (error.message.includes('Malformed extraction path')) { + throw new Error(`Artifact download failed with unretryable error: ${error.message}`) + } retryCount++ core.debug( `Failed to download artifact after ${retryCount} retries due to ${error.message}. Retrying in 5 seconds...` @@ -81,6 +84,7 @@ export async function streamExtractExternal( const timer = setTimeout(timerFn, timeout) const promises: Promise[] = [] + const createdDirectories = new Set() response.message .on('data', () => { timer.refresh() @@ -94,20 +98,25 @@ export async function streamExtractExternal( }) .pipe(unzip.Parse()) .on('entry', (entry: unzip.Entry) => { - console.log(`entryPath: ${entry.path}`) const fullPath = path.normalize(path.join(directory, entry.path)) - console.log(`fullPath: ${fullPath}`) - if (fullPath.indexOf(directory) != 0) { - reject(new Error(`Invalid file path: ${fullPath}`)) + if (fullPath.indexOf(directory) !== 0) { + reject(new Error(`Malformed extraction path: ${fullPath}`)) } + core.debug(`Extracting artifact entry: ${fullPath}`) if (entry.type === 'Directory') { - promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) + if (!createdDirectories.has(fullPath)) { + promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) + createdDirectories.add(fullPath) + } entry.autodrain() } else { - promises.push( - resolveOrCreateDirectory(path.dirname(fullPath)).then(() => {}) - ) + if (!createdDirectories.has(path.dirname(fullPath))) { + promises.push( + resolveOrCreateDirectory(path.dirname(fullPath)).then(() => {}) + ) + createdDirectories.add(path.dirname(fullPath)) + } const writeStream = createWriteStream(fullPath) promises.push( new Promise((resolve, reject) => { From ac84a9bee36dff3220e402afe05a2eb525eb46f2 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 13:46:22 -0500 Subject: [PATCH 14/20] re-add noop logs and format + lint --- .../__tests__/download-artifact.test.ts | 18 ++++++++++-------- .../src/internal/download/download-artifact.ts | 6 ++++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 3f0efc9d..7e93d1b4 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -13,7 +13,7 @@ import { streamExtractExternal } from '../src/internal/download/download-artifact' import {getUserAgentString} from '../src/internal/shared/user-agent' -//import {noopLogs} from './common' +import {noopLogs} from './common' import * as config from '../src/internal/shared/config' import {ArtifactServiceClientJSON} from '../src/generated' import * as util from '../src/internal/shared/util' @@ -88,7 +88,7 @@ const expectExtractedArchive = async (dir: string): Promise => { } const setup = async (): Promise => { - //noopLogs() + noopLogs() await fs.promises.mkdir(testDir, {recursive: true}) await createTestArchive() @@ -200,12 +200,14 @@ describe('download-artifact', () => { } ) - await expect(downloadArtifactPublic( - fixtures.artifactID, - fixtures.repositoryOwner, - fixtures.repositoryName, - fixtures.token - )).rejects.toBeInstanceOf(Error) + await expect( + downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) + ).rejects.toBeInstanceOf(Error) expect(downloadArtifactMock).toHaveBeenCalledWith({ owner: fixtures.repositoryOwner, diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 1c6c8ec2..3c2c26eb 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -47,7 +47,9 @@ async function streamExtract(url: string, directory: string): Promise { return } catch (error) { if (error.message.includes('Malformed extraction path')) { - throw new Error(`Artifact download failed with unretryable error: ${error.message}`) + throw new Error( + `Artifact download failed with unretryable error: ${error.message}` + ) } retryCount++ core.debug( @@ -99,7 +101,7 @@ export async function streamExtractExternal( .pipe(unzip.Parse()) .on('entry', (entry: unzip.Entry) => { const fullPath = path.normalize(path.join(directory, entry.path)) - if (fullPath.indexOf(directory) !== 0) { + if (!fullPath.startsWith(directory)) { reject(new Error(`Malformed extraction path: ${fullPath}`)) } From 614f27a4fbcad3f360082ffd9711c2f3da022ae1 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 14:34:39 -0500 Subject: [PATCH 15/20] use stream transform --- .../internal/download/download-artifact.ts | 83 ++++++++++--------- 1 file changed, 45 insertions(+), 38 deletions(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 3c2c26eb..09890c29 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -1,4 +1,5 @@ import fs from 'fs/promises' +import * as stream from 'stream' import {createWriteStream} from 'fs' import * as path from 'path' import * as github from '@actions/github' @@ -85,8 +86,8 @@ export async function streamExtractExternal( } const timer = setTimeout(timerFn, timeout) - const promises: Promise[] = [] const createdDirectories = new Set() + createdDirectories.add(directory) response.message .on('data', () => { timer.refresh() @@ -99,46 +100,52 @@ export async function streamExtractExternal( reject(error) }) .pipe(unzip.Parse()) - .on('entry', (entry: unzip.Entry) => { - const fullPath = path.normalize(path.join(directory, entry.path)) - if (!fullPath.startsWith(directory)) { - reject(new Error(`Malformed extraction path: ${fullPath}`)) - } + .pipe( + new stream.Transform({ + objectMode: true, + transform: async (entry, _, callback) => { + const fullPath = path.normalize(path.join(directory, entry.path)) + if (!directory.endsWith(path.sep)) { + directory += path.sep + } + if (!fullPath.startsWith(directory)) { + reject(new Error(`Malformed extraction path: ${fullPath}`)) + } - core.debug(`Extracting artifact entry: ${fullPath}`) - if (entry.type === 'Directory') { - if (!createdDirectories.has(fullPath)) { - promises.push(resolveOrCreateDirectory(fullPath).then(() => {})) - createdDirectories.add(fullPath) - } - entry.autodrain() - } else { - if (!createdDirectories.has(path.dirname(fullPath))) { - promises.push( - resolveOrCreateDirectory(path.dirname(fullPath)).then(() => {}) - ) - createdDirectories.add(path.dirname(fullPath)) - } - const writeStream = createWriteStream(fullPath) - promises.push( - new Promise((resolve, reject) => { - writeStream.on('finish', () => { - resolve() - }) + core.debug(`Extracting artifact entry: ${fullPath}`) + if (entry.type === 'Directory') { + if (!createdDirectories.has(fullPath)) { + createdDirectories.add(fullPath) + await resolveOrCreateDirectory(fullPath).then(() => { + entry.autodrain() + callback() + }) + } else { + entry.autodrain() + callback() + } + } else { + if (!createdDirectories.has(path.dirname(fullPath))) { + createdDirectories.add(path.dirname(fullPath)) + await resolveOrCreateDirectory(path.dirname(fullPath)).then( + () => { + entry.autodrain() + callback() + } + ) + } + + const writeStream = createWriteStream(fullPath) + writeStream.on('finish', callback) writeStream.on('error', reject) - }) - ) - entry.pipe(writeStream) - } - }) - .on('end', async () => { + entry.pipe(writeStream) + } + } + }) + ) + .on('finish', async () => { clearTimeout(timer) - try { - await Promise.all(promises) - resolve() - } catch (error) { - reject(error) - } + resolve() }) .on('error', (error: Error) => { reject(error) From 90894a8853d1f5c325da17886831dda24a12d797 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 15:03:09 -0500 Subject: [PATCH 16/20] bump version --- packages/artifact/RELEASES.md | 6 +++++- packages/artifact/package-lock.json | 4 ++-- packages/artifact/package.json | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/artifact/RELEASES.md b/packages/artifact/RELEASES.md index 9e14a6b6..7c30fe79 100644 --- a/packages/artifact/RELEASES.md +++ b/packages/artifact/RELEASES.md @@ -113,4 +113,8 @@ ### 2.1.1 -- Updated `isGhes` check to include `.ghe.com` and `.ghe.localhost` as accepted hosts \ No newline at end of file +- Updated `isGhes` check to include `.ghe.com` and `.ghe.localhost` as accepted hosts + +### 2.1.2 + +- Updated the stream extract functionality to use `unzip.Parse()` instead of `unzip.Extract()` for greater control of unzipping artifacts diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index 550cbb01..d25b1524 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -1,12 +1,12 @@ { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "license": "MIT", "dependencies": { "@actions/core": "^1.10.0", diff --git a/packages/artifact/package.json b/packages/artifact/package.json index a5f6ad10..35f088e6 100644 --- a/packages/artifact/package.json +++ b/packages/artifact/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "preview": true, "description": "Actions artifact lib", "keywords": [ From 8a1800c5da292e0e5a2fa346f4d48724f1c12167 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 15:15:17 -0500 Subject: [PATCH 17/20] use resolve instead of normalize --- packages/artifact/src/internal/download/download-artifact.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 09890c29..a5574e82 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -104,7 +104,7 @@ export async function streamExtractExternal( new stream.Transform({ objectMode: true, transform: async (entry, _, callback) => { - const fullPath = path.normalize(path.join(directory, entry.path)) + const fullPath = path.resolve(path.join(directory, entry.path)) if (!directory.endsWith(path.sep)) { directory += path.sep } From f77cbc9ef712200a1572866c6f41567e9dc3c2e9 Mon Sep 17 00:00:00 2001 From: Bethany Date: Fri, 23 Feb 2024 15:20:01 -0500 Subject: [PATCH 18/20] Update packages/artifact/src/internal/download/download-artifact.ts Co-authored-by: Tingluo Huang --- packages/artifact/src/internal/download/download-artifact.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index a5574e82..ef78faa4 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -104,7 +104,7 @@ export async function streamExtractExternal( new stream.Transform({ objectMode: true, transform: async (entry, _, callback) => { - const fullPath = path.resolve(path.join(directory, entry.path)) + const fullPath = path.resolve(directory, entry.path) if (!directory.endsWith(path.sep)) { directory += path.sep } From 7fa864a4f41863b88b3c42ea4fcb6eb4ece99b8c Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 15:28:25 -0500 Subject: [PATCH 19/20] go back to normalize) --- packages/artifact/src/internal/download/download-artifact.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index ef78faa4..09890c29 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -104,7 +104,7 @@ export async function streamExtractExternal( new stream.Transform({ objectMode: true, transform: async (entry, _, callback) => { - const fullPath = path.resolve(directory, entry.path) + const fullPath = path.normalize(path.join(directory, entry.path)) if (!directory.endsWith(path.sep)) { directory += path.sep } From 6cf4fbcef866f531163664bc15c5b738f4af9b04 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Fri, 23 Feb 2024 15:33:24 -0500 Subject: [PATCH 20/20] add a comment --- packages/artifact/__tests__/download-artifact.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 7e93d1b4..e961532a 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -124,7 +124,7 @@ const mockGetArtifactFailure = jest.fn(() => { const mockGetArtifactMalicious = jest.fn(() => { const message = new http.IncomingMessage(new net.Socket()) message.statusCode = 200 - message.push(fs.readFileSync(path.join(__dirname, 'fixtures', 'evil.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 { message