[Security] Harden downloadFile with response validation

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

@@ -292,6 +292,24 @@ describe('downloadFile', () => {
       await expect(fileExists(to)).resolves.toBe(false)
     })
   })
+
+  test('Fails and cleans up if server returns 500', 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 file from 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

@@ -226,38 +226,52 @@ export function downloadFile(url: string, to: string): Promise<string> {
 
   return runWithTimer('cmd_all_timing_network_ms')(() => {
     return new Promise<string>((resolve, reject) => {
-      if (!fileExistsSync(dirname(to))) {
-        mkdirSync(dirname(to))
-      }
+      nodeFetch(url, {redirect: 'follow'})
+        .then((res) => {
+          if (!res.ok) {
+            return reject(new Error(`Failed to download file from ${sanitizedUrl}: ${res.status} ${res.statusText}`))
+          }
+          if (!res.body) {
+            return reject(new Error(`Failed to download file from ${sanitizedUrl}: Empty response body`))
+          }
 
-      const file = createFileWriteStream(to)
+          if (!fileExistsSync(dirname(to))) {
+            mkdirSync(dirname(to))
+          }
 
-      // if we can't remove the file for some reason (seen on windows), that's ok -- it's in a temporary directory
-      const tryToRemoveFile = () => {
-        try {
-          unlinkFileSync(to)
-          // eslint-disable-next-line no-catch-all/no-catch-all
-        } catch (err: unknown) {
-          outputDebug(outputContent`Failed to remove file ${outputToken.path(to)}: ${outputToken.raw(String(err))}`)
-        }
-      }
+          const file = createFileWriteStream(to)
 
-      file.on('finish', () => {
-        file.close()
-        resolve(to)
-      })
+          // if we can't remove the file for some reason (seen on windows), that's ok -- it's in a temporary directory
+          const tryToRemoveFile = () => {
+            try {
+              if (!file.destroyed) {
+                file.destroy()
+              }
+              unlinkFileSync(to)
+              // eslint-disable-next-line no-catch-all/no-catch-all
+            } catch (err: unknown) {
+              outputDebug(outputContent`Failed to remove file ${outputToken.path(to)}: ${outputToken.raw(String(err))}`)
+            }
+          }
 
-      file.on('error', (err) => {
-        tryToRemoveFile()
-        reject(err)
-      })
+          file.on('finish', () => {
+            file.close()
+            resolve(to)
+          })
 
-      nodeFetch(url, {redirect: 'follow'})
-        .then((res) => {
-          res.body?.pipe(file)
+          file.on('error', (err) => {
+            tryToRemoveFile()
+            reject(err instanceof Error ? err : new Error(String(err)))
+          })
+
+          res.body.on('error', (err) => {
+            tryToRemoveFile()
+            reject(err instanceof Error ? err : new Error(String(err)))
+          })
+
+          res.body.pipe(file)
         })
         .catch((err) => {
-          tryToRemoveFile()
           reject(err instanceof Error ? err : new Error(String(err)))
         })
     })