Merge pull request #1666 from actions/bethanyj28/download-path

Use `unzip.Parse` over `unzip.Extract`

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": {

packages/artifact/RELEASES.md

@@ -114,3 +114,7 @@
 ### 2.1.1
  
 - 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

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'))) // 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')
 

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",

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": [

packages/artifact/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<void> {
       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<string>()
+    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()
       })