From 49c9dca488099a22bfc3abcf00c5bf18202490ab Mon Sep 17 00:00:00 2001
From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com>
Date: Sun, 14 Jun 2026 00:23:20 +0000
Subject: [PATCH] [Security] Harden downloadFile and downloadGitHubRelease

Hardened the `downloadFile` utility in `packages/cli-kit` to validate the HTTP response status code before processing the body. This prevents failed downloads from saving error pages as valid files.

Refactored `downloadGitHubRelease` to use `downloadFile`, enabling streaming downloads for GitHub release assets. This mitigates memory exhaustion (DoS) risks associated with downloading large releases.
---
 .../cli-kit/src/public/node/github.test.ts     | 17 +++++++----------
 packages/cli-kit/src/public/node/github.ts     | 13 +++----------
 packages/cli-kit/src/public/node/http.test.ts  | 18 ++++++++++++++++++
 packages/cli-kit/src/public/node/http.ts       |  3 +++
 4 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/packages/cli-kit/src/public/node/github.test.ts b/packages/cli-kit/src/public/node/github.test.ts
index d02f427c6e4..efed5db6255 100644
--- a/packages/cli-kit/src/public/node/github.test.ts
+++ b/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)
diff --git a/packages/cli-kit/src/public/node/github.ts b/packages/cli-kit/src/public/node/github.ts
index 0018b744dad..d70e62f15b7 100644
--- a/packages/cli-kit/src/public/node/github.ts
+++ b/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)
diff --git a/packages/cli-kit/src/public/node/http.test.ts b/packages/cli-kit/src/public/node/http.test.ts
index f848c4e76a6..8e87c01b2b7 100644
--- a/packages/cli-kit/src/public/node/http.test.ts
+++ b/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', () => {
diff --git a/packages/cli-kit/src/public/node/http.ts b/packages/cli-kit/src/public/node/http.ts
index 86bda5a9f9b..9a7d267ea35 100644
--- a/packages/cli-kit/src/public/node/http.ts
+++ b/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}`)
       }