diff --git a/RELEASES.md b/RELEASES.md index 9e14a6b..7c30fe7 100644 --- a/RELEASES.md +++ b/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/__tests__/download-artifact.test.ts b/__tests__/download-artifact.test.ts index 1c0c9b0..e961532 100644 --- a/__tests__/download-artifact.test.ts +++ b/__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'))) // evil.zip contains files that are formatted x/../../etc/hosts + message.push(null) + return { + message + } +}) + describe('download-artifact', () => { describe('public', () => { beforeEach(setup) @@ -170,6 +180,51 @@ 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 + } + } + ) + + await expect( + downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) + ).rejects.toBeInstanceOf(Error) + + 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 + ) + }) + it('should successfully download an artifact to user defined path', async () => { const customPath = path.join(testDir, 'custom') diff --git a/__tests__/fixtures/evil.zip b/__tests__/fixtures/evil.zip new file mode 100644 index 0000000..d345590 Binary files /dev/null and b/__tests__/fixtures/evil.zip differ diff --git a/package-lock.json b/package-lock.json index 550cbb0..d25b152 100644 --- a/package-lock.json +++ b/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/package.json b/package.json index a5f6ad1..35f088e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "2.1.1", + "version": "2.1.2", "preview": true, "description": "Actions artifact lib", "keywords": [ diff --git a/src/internal/download/download-artifact.ts b/src/internal/download/download-artifact.ts index dc54f6f..09890c2 100644 --- a/src/internal/download/download-artifact.ts +++ b/src/internal/download/download-artifact.ts @@ -1,4 +1,7 @@ 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' @@ -44,6 +47,11 @@ 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...` @@ -78,6 +86,8 @@ export async function streamExtractExternal( } const timer = setTimeout(timerFn, timeout) + const createdDirectories = new Set() + createdDirectories.add(directory) response.message .on('data', () => { timer.refresh() @@ -89,8 +99,51 @@ export async function streamExtractExternal( clearTimeout(timer) reject(error) }) - .pipe(unzip.Extract({path: directory})) - .on('close', () => { + .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}`)) + } + + 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('finish', async () => { clearTimeout(timer) resolve() })