[Security] Harden downloadFile and downloadGitHubRelease

packages/cli-kit/src/public/node/github.test.ts

@@ -9,7 +9,7 @@ import {fetch, downloadFile} from './http.js'
 import {AbortError} from './error.js'
 import {testWithTempDir} from './testing/test-with-temp-dir.js'
 import {joinPath} from './path.js'
-import {readFile} from './fs.js'
+import {readFile, writeFile} from './fs.js'
 import {isExecutable} from 'is-executable'
 import {describe, expect, test, vi} from 'vitest'
 import {Response} from 'node-fetch'
@@ -180,12 +180,10 @@ describe('downloadGitHubRelease', () => {
   testWithTempDir('successfully downloads the release asset', async ({tempDir}) => {
     // GIVEN
     const downloadContent = 'hello'
-    const content = Buffer.from(downloadContent)
-    const mockResponse = {
-      ok: true,
-      arrayBuffer: vi.fn().mockResolvedValue(content),
-    }
-    vi.mocked(fetch).mockResolvedValue(mockResponse as any)
+    vi.mocked(downloadFile).mockImplementation(async (_url, to) => {
+      await writeFile(to, downloadContent)
+      return to
+    })
 
     const binary = process.platform === 'win32' ? 'test-asset.exe' : 'test-asset'
     const targetPath = joinPath(tempDir, 'downloads', binary)
@@ -194,10 +192,9 @@ describe('downloadGitHubRelease', () => {
     await downloadGitHubRelease(repo, version, asset, targetPath)
 
     // THEN
-    expect(fetch).toHaveBeenCalledWith(
+    expect(downloadFile).toHaveBeenCalledWith(
       `https://github.com/${repo}/releases/download/${version}/${asset}`,
-      undefined,
-      'slow-request',
+      expect.stringContaining(asset),
     )
 
     const downloadedContent = await readFile(targetPath)

packages/cli-kit/src/public/node/github.ts

@@ -1,7 +1,7 @@
 import {outputContent, outputDebug, outputToken} from './output.js'
 import {err, ok, Result} from './result.js'
-import {fetch, Response} from './http.js'
-import {writeFile, mkdir, inTemporaryDirectory, moveFile, chmod} from './fs.js'
+import {fetch, downloadFile} from './http.js'
+import {mkdir, inTemporaryDirectory, moveFile, chmod} from './fs.js'
 import {dirname, joinPath} from './path.js'
 import {runWithTimer} from './metadata.js'
 import {AbortError} from './error.js'
@@ -150,21 +150,14 @@ export async function downloadGitHubRelease(
     outputDebug(outputContent`Downloading ${outputToken.link(assetName, url)}`)
     await inTemporaryDirectory(async (tmpDir) => {
       const tempPath = joinPath(tmpDir, assetName)
-      let response: Response
       try {
-        response = await fetch(url, undefined, 'slow-request')
-        if (!response.ok) {
-          throw new AbortError(`Failed to download ${assetName}: ${response.statusText}`)
-        }
+        await downloadFile(url, tempPath)
       } catch (error) {
         throw new AbortError(
           `Failed to download ${assetName}: ${error instanceof Error ? error.message : 'unknown error'}`,
         )
       }
 
-      const buffer = await response.arrayBuffer()
-      await writeFile(tempPath, Buffer.from(buffer))
-
       await chmod(tempPath, 0o755)
       await mkdir(dirname(targetPath))
       await moveFile(tempPath, targetPath)

packages/cli-kit/src/public/node/http.test.ts

@@ -292,6 +292,24 @@ describe('downloadFile', () => {
       await expect(fileExists(to)).resolves.toBe(false)
     })
   })
+
+  test('Throws an error if the response is not ok', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const url = 'https://shopify.example/500.txt'
+      const filename = '/bin/500.txt'
+      const to = joinPath(tmpDir, filename)
+
+      // When
+      const result = downloadFile(url, to)
+
+      // Then
+      await expect(result).rejects.toThrow(
+        'Failed to download https://shopify.example/500.txt: 500 Internal Server Error',
+      )
+      await expect(fileExists(to)).resolves.toBe(false)
+    })
+  })
 })
 
 describe('requestMode', () => {

packages/cli-kit/src/public/node/http.ts

@@ -244,6 +244,9 @@ export function downloadFile(url: string, to: string): Promise<string> {
 
     try {
       const res = await nodeFetch(url, {redirect: 'follow'})
+      if (!res.ok) {
+        throw new Error(`Failed to download ${sanitizedUrl}: ${res.status} ${res.statusText}`)
+      }
       if (!res.body) {
         throw new Error(`No response body received when downloading ${sanitizedUrl}`)
       }