artifact(download): skip non-zip files

Author: crazy-max

packages/artifact/__tests__/download-artifact.test.ts

@@ -104,6 +104,18 @@ const cleanup = async (): Promise<void> => {
 const mockGetArtifactSuccess = jest.fn(() => {
   const message = new http.IncomingMessage(new net.Socket())
   message.statusCode = 200
+  message.headers['content-type'] = 'zip'
+  message.push(fs.readFileSync(fixtures.exampleArtifact.path))
+  message.push(null)
+  return {
+    message
+  }
+})
+
+const mockGetArtifactGzip = jest.fn(() => {
+  const message = new http.IncomingMessage(new net.Socket())
+  message.statusCode = 200
+  message.headers['content-type'] = 'application/gzip'
   message.push(fs.readFileSync(fixtures.exampleArtifact.path))
   message.push(null)
   return {
@@ -124,6 +136,7 @@ const mockGetArtifactFailure = jest.fn(() => {
 const mockGetArtifactMalicious = jest.fn(() => {
   const message = new http.IncomingMessage(new net.Socket())
   message.statusCode = 200
+  message.headers['content-type'] = '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 {
@@ -178,6 +191,7 @@ describe('download-artifact', () => {
       )
       expectExtractedArchive(fixtures.workspaceDir)
       expect(response.downloadPath).toBe(fixtures.workspaceDir)
+      expect(response.skipped).toBe(false)
     })
 
     it('should not allow path traversal from malicious artifacts', async () => {
@@ -231,6 +245,7 @@ describe('download-artifact', () => {
       ).toBe(true)
 
       expect(response.downloadPath).toBe(fixtures.workspaceDir)
+      expect(response.skipped).toBe(false)
     })
 
     it('should successfully download an artifact to user defined path', async () => {
@@ -280,6 +295,7 @@ describe('download-artifact', () => {
       )
       expectExtractedArchive(customPath)
       expect(response.downloadPath).toBe(customPath)
+      expect(response.skipped).toBe(false)
     })
 
     it('should fail if download artifact API does not respond with location', async () => {
@@ -316,6 +332,7 @@ describe('download-artifact', () => {
       // mock http client to delay response data by 30s
       const msg = new http.IncomingMessage(new net.Socket())
       msg.statusCode = 200
+      msg.headers['content-type'] = 'zip'
 
       const mockGet = jest.fn(async () => {
         return new Promise((resolve, reject) => {
@@ -444,7 +461,39 @@ describe('download-artifact', () => {
       )
       expect(mockGetArtifactSuccess).toHaveBeenCalledTimes(1)
       expect(response.downloadPath).toBe(fixtures.workspaceDir)
+      expect(response.skipped).toBe(false)
     }, 28000)
+
+    it('should skip if artifact does not have the right content type', 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: mockGetArtifactGzip
+          }
+        }
+      )
+
+      const response = await downloadArtifactPublic(
+        fixtures.artifactID,
+        fixtures.repositoryOwner,
+        fixtures.repositoryName,
+        fixtures.token
+      )
+
+      expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString())
+      expect(response.skipped).toBe(true)
+    })
   })
 
   describe('internal', () => {
@@ -499,6 +548,7 @@ describe('download-artifact', () => {
 
       expectExtractedArchive(fixtures.workspaceDir)
       expect(response.downloadPath).toBe(fixtures.workspaceDir)
+      expect(response.skipped).toBe(false)
       expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString())
       expect(mockListArtifacts).toHaveBeenCalledWith({
         idFilter: {
@@ -550,6 +600,7 @@ describe('download-artifact', () => {
 
       expectExtractedArchive(customPath)
       expect(response.downloadPath).toBe(customPath)
+      expect(response.skipped).toBe(false)
       expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString())
       expect(mockListArtifacts).toHaveBeenCalledWith({
         idFilter: {

packages/artifact/src/internal/download/download-artifact.ts

@@ -37,12 +37,11 @@ async function exists(path: string): Promise<boolean> {
   }
 }
 
-async function streamExtract(url: string, directory: string): Promise<void> {
+async function streamExtract(url: string, directory: string): Promise<boolean> {
   let retryCount = 0
   while (retryCount < 5) {
     try {
-      await streamExtractExternal(url, directory)
-      return
+      return await streamExtractExternal(url, directory)
     } catch (error) {
       retryCount++
       core.debug(
@@ -59,18 +58,23 @@ async function streamExtract(url: string, directory: string): Promise<void> {
 export async function streamExtractExternal(
   url: string,
   directory: string
-): Promise<void> {
+): Promise<boolean> {
   const client = new httpClient.HttpClient(getUserAgentString())
   const response = await client.get(url)
   if (response.message.statusCode !== 200) {
     throw new Error(
       `Unexpected HTTP response from blob storage: ${response.message.statusCode} ${response.message.statusMessage}`
     )
+  } else if (response.message.headers['content-type'] !== 'zip') {
+    core.debug(
+      `Invalid content-type: ${response.message.headers['content-type']}, skipping download`
+    )
+    return false
   }
 
   const timeout = 30 * 1000 // 30 seconds
 
-  return new Promise((resolve, reject) => {
+  return new Promise<boolean>((resolve, reject) => {
     const timerFn = (): void => {
       response.message.destroy(
         new Error(`Blob storage chunk did not respond in ${timeout}ms`)
@@ -92,7 +96,7 @@ export async function streamExtractExternal(
       .pipe(unzip.Extract({path: directory}))
       .on('close', () => {
         clearTimeout(timer)
-        resolve()
+        resolve(true)
       })
       .on('error', (error: Error) => {
         reject(error)
@@ -140,13 +144,16 @@ export async function downloadArtifactPublic(
 
   try {
     core.info(`Starting download of artifact to: ${downloadPath}`)
-    await streamExtract(location, downloadPath)
-    core.info(`Artifact download completed successfully.`)
+    if (await streamExtract(location, downloadPath)) {
+      core.info(`Artifact download completed successfully.`)
+      return {downloadPath, skipped: false}
+    } else {
+      core.info(`Artifact download skipped.`)
+      return {downloadPath, skipped: true}
+    }
   } catch (error) {
     throw new Error(`Unable to download and extract artifact: ${error.message}`)
   }
-
-  return {downloadPath}
 }
 
 export async function downloadArtifactInternal(
@@ -192,13 +199,16 @@ export async function downloadArtifactInternal(
 
   try {
     core.info(`Starting download of artifact to: ${downloadPath}`)
-    await streamExtract(signedUrl, downloadPath)
-    core.info(`Artifact download completed successfully.`)
+    if (await streamExtract(signedUrl, downloadPath)) {
+      core.info(`Artifact download completed successfully.`)
+      return {downloadPath, skipped: false}
+    } else {
+      core.info(`Artifact download skipped.`)
+      return {downloadPath, skipped: true}
+    }
   } catch (error) {
     throw new Error(`Unable to download and extract artifact: ${error.message}`)
   }
-
-  return {downloadPath}
 }
 
 async function resolveOrCreateDirectory(

packages/artifact/src/internal/shared/interfaces.ts

@@ -86,6 +86,11 @@ export interface DownloadArtifactResponse {
    * The path where the artifact was downloaded to
    */
   downloadPath?: string
+
+  /**
+   * If the artifact download was skipped
+   */
+  skipped?: boolean
 }
 
 /**