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 71d842b8..d345590f 100644 Binary files a/packages/artifact/__tests__/fixtures/evil.zip and b/packages/artifact/__tests__/fixtures/evil.zip differ 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) => {