From 476276bf98a0b95a88713272e32de69ae2085833 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Tue, 23 Apr 2024 15:54:54 -0400 Subject: [PATCH 1/4] use latest unzip-stream --- .../__tests__/download-artifact.test.ts | 20 ++++--- packages/artifact/package-lock.json | 10 ++-- .../internal/download/download-artifact.ts | 52 +------------------ 3 files changed, 19 insertions(+), 63 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index e961532a..d886a55c 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -200,14 +200,12 @@ describe('download-artifact', () => { } ) - await expect( - downloadArtifactPublic( - fixtures.artifactID, - fixtures.repositoryOwner, - fixtures.repositoryName, - fixtures.token - ) - ).rejects.toBeInstanceOf(Error) + const response = await downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) expect(downloadArtifactMock).toHaveBeenCalledWith({ owner: fixtures.repositoryOwner, @@ -223,6 +221,12 @@ describe('download-artifact', () => { expect(mockGetArtifactMalicious).toHaveBeenCalledWith( fixtures.blobStorageUrl ) + + // ensure path traversal was not possible + expect(fs.existsSync(path.join(fixtures.workspaceDir, 'x/etc/hosts'))).toBe(true); + expect(fs.existsSync(path.join(fixtures.workspaceDir, 'y/etc/hosts'))).toBe(true); + + expect(response.downloadPath).toBe(fixtures.workspaceDir) }) it('should successfully download an artifact to user defined path', async () => { diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index 643dcb86..878360b6 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -1,12 +1,12 @@ { "name": "@actions/artifact", - "version": "2.1.5", + "version": "2.1.6", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@actions/artifact", - "version": "2.1.5", + "version": "2.1.6", "license": "MIT", "dependencies": { "@actions/core": "^1.10.0", @@ -1738,9 +1738,9 @@ "integrity": "sha512-isyNax3wXoKaulPDZWHQqbmIx1k2tb9fb3GGDBRxCscfYV2Ch7WxPArBsFEG8s/safwXTT7H4QGhaIkTp9447w==" }, "node_modules/unzip-stream": { - "version": "0.3.1", - "resolved": "https://registry.npmjs.org/unzip-stream/-/unzip-stream-0.3.1.tgz", - "integrity": "sha512-RzaGXLNt+CW+T41h1zl6pGz3EaeVhYlK+rdAap+7DxW5kqsqePO8kRtWPaCiVqdhZc86EctSPVYNix30YOMzmw==", + "version": "0.3.4", + "resolved": "https://registry.npmjs.org/unzip-stream/-/unzip-stream-0.3.4.tgz", + "integrity": "sha512-PyofABPVv+d7fL7GOpusx7eRT9YETY2X04PhwbSipdj6bMxVCFJrr+nm0Mxqbf9hUiTin/UsnuFWBXlDZFy0Cw==", "dependencies": { "binary": "^0.3.0", "mkdirp": "^0.5.1" diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index fb7fe0a3..31965c45 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -1,7 +1,4 @@ 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' import * as core from '@actions/core' import * as httpClient from '@actions/http-client' @@ -47,11 +44,6 @@ 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...` @@ -86,8 +78,6 @@ export async function streamExtractExternal( } const timer = setTimeout(timerFn, timeout) - const createdDirectories = new Set() - createdDirectories.add(directory) response.message .on('data', () => { timer.refresh() @@ -99,46 +89,8 @@ export async function streamExtractExternal( clearTimeout(timer) reject(error) }) - .pipe(unzip.Parse()) - .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}`)) - } - - if (entry.type === 'Directory') { - if (!createdDirectories.has(fullPath)) { - createdDirectories.add(fullPath) - await resolveOrCreateDirectory(fullPath).then(() => { - entry.autodrain() - callback() - }) - } else { - entry.autodrain() - callback() - } - } else { - core.info(`Extracting artifact entry: ${fullPath}`) - if (!createdDirectories.has(path.dirname(fullPath))) { - createdDirectories.add(path.dirname(fullPath)) - await resolveOrCreateDirectory(path.dirname(fullPath)) - } - - const writeStream = createWriteStream(fullPath) - writeStream.on('finish', callback) - writeStream.on('error', reject) - entry.pipe(writeStream) - } - } - }) - ) - .on('finish', async () => { + .pipe(unzip.Extract({path: directory})) + .on('close', () => { clearTimeout(timer) resolve() }) From 0159bbe7f2469315c88f3fe687a7ab68837fa71b Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Tue, 23 Apr 2024 16:03:52 -0400 Subject: [PATCH 2/4] bump version --- packages/artifact/RELEASES.md | 4 ++++ packages/artifact/package-lock.json | 4 ++-- packages/artifact/package.json | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/artifact/RELEASES.md b/packages/artifact/RELEASES.md index 75d76f3e..afc7f6a7 100644 --- a/packages/artifact/RELEASES.md +++ b/packages/artifact/RELEASES.md @@ -1,5 +1,9 @@ # @actions/artifact Releases +### 2.1.7 + +- Update unzip-stream dependency and reverted to using `unzip.Extract()` + ### 2.1.6 - Will retry on invalid request responses. diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index 878360b6..dce18974 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -1,12 +1,12 @@ { "name": "@actions/artifact", - "version": "2.1.6", + "version": "2.1.7", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@actions/artifact", - "version": "2.1.6", + "version": "2.1.7", "license": "MIT", "dependencies": { "@actions/core": "^1.10.0", diff --git a/packages/artifact/package.json b/packages/artifact/package.json index 83d56a83..ab84d0f2 100644 --- a/packages/artifact/package.json +++ b/packages/artifact/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "2.1.6", + "version": "2.1.7", "preview": true, "description": "Actions artifact lib", "keywords": [ From 6e642f628f1636a50841102529403838cde4b6cb Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Tue, 23 Apr 2024 16:06:02 -0400 Subject: [PATCH 3/4] lint --- 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 d886a55c..7c6849c8 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -223,8 +223,8 @@ describe('download-artifact', () => { ) // ensure path traversal was not possible - expect(fs.existsSync(path.join(fixtures.workspaceDir, 'x/etc/hosts'))).toBe(true); - expect(fs.existsSync(path.join(fixtures.workspaceDir, 'y/etc/hosts'))).toBe(true); + expect(fs.existsSync(path.join(fixtures.workspaceDir, 'x/etc/hosts'))).toBe(true) + expect(fs.existsSync(path.join(fixtures.workspaceDir, 'y/etc/hosts'))).toBe(true) expect(response.downloadPath).toBe(fixtures.workspaceDir) }) diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index 31965c45..dc54f6fe 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -90,7 +90,7 @@ export async function streamExtractExternal( reject(error) }) .pipe(unzip.Extract({path: directory})) - .on('close', () => { + .on('close', () => { clearTimeout(timer) resolve() }) From 9eb3d3a673314b5f1b563801594f919b28562c03 Mon Sep 17 00:00:00 2001 From: bethanyj28 Date: Tue, 23 Apr 2024 16:10:57 -0400 Subject: [PATCH 4/4] lint --- packages/artifact/__tests__/download-artifact.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 7c6849c8..f73c9fc7 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -223,8 +223,12 @@ describe('download-artifact', () => { ) // ensure path traversal was not possible - expect(fs.existsSync(path.join(fixtures.workspaceDir, 'x/etc/hosts'))).toBe(true) - expect(fs.existsSync(path.join(fixtures.workspaceDir, 'y/etc/hosts'))).toBe(true) + expect( + fs.existsSync(path.join(fixtures.workspaceDir, 'x/etc/hosts')) + ).toBe(true) + expect( + fs.existsSync(path.join(fixtures.workspaceDir, 'y/etc/hosts')) + ).toBe(true) expect(response.downloadPath).toBe(fixtures.workspaceDir) })