From cafcc7be64828d035f5176789bf5f09d049a6715 Mon Sep 17 00:00:00 2001 From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com> Date: Thu, 4 Jun 2026 00:50:57 +0000 Subject: [PATCH 1/2] [Security] Harden downloadFile with response validation --- packages/cli-kit/src/public/node/http.test.ts | 18 ++++++++++++++++++ packages/cli-kit/src/public/node/http.ts | 10 +++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/cli-kit/src/public/node/http.test.ts b/packages/cli-kit/src/public/node/http.test.ts index f848c4e76a6..24402339879 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('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', () => { diff --git a/packages/cli-kit/src/public/node/http.ts b/packages/cli-kit/src/public/node/http.ts index 8cbc8556a57..5af5b7a7e26 100644 --- a/packages/cli-kit/src/public/node/http.ts +++ b/packages/cli-kit/src/public/node/http.ts @@ -254,7 +254,15 @@ export function downloadFile(url: string, to: string): Promise { nodeFetch(url, {redirect: 'follow'}) .then((res) => { - res.body?.pipe(file) + if (!res.ok) { + tryToRemoveFile() + return reject(new Error(`Failed to download file from ${sanitizedUrl}: ${res.status} ${res.statusText}`)) + } + if (!res.body) { + tryToRemoveFile() + return reject(new Error(`Failed to download file from ${sanitizedUrl}: Empty response body`)) + } + res.body.pipe(file) }) .catch((err) => { tryToRemoveFile() From 41f69abdf628f27700f6216d6052cdec64529435 Mon Sep 17 00:00:00 2001 From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com> Date: Thu, 4 Jun 2026 01:21:32 +0000 Subject: [PATCH 2/2] [Security] Harden downloadFile with response validation and improved cleanup Harden downloadFile in packages/cli-kit/src/public/node/http.ts by adding checks for res.ok and res.body. If these checks fail, the promise is rejected with a descriptive error message. Improved the cleanup logic to ensure the file stream is destroyed before unlinking, which releases file handles and improves stability. Added a regression test in packages/cli-kit/src/public/node/http.test.ts to verify that 500 errors trigger correct failure and cleanup. --- packages/cli-kit/src/public/node/http.ts | 64 +++++++++++++----------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/packages/cli-kit/src/public/node/http.ts b/packages/cli-kit/src/public/node/http.ts index 5af5b7a7e26..8121f31a2f1 100644 --- a/packages/cli-kit/src/public/node/http.ts +++ b/packages/cli-kit/src/public/node/http.ts @@ -226,46 +226,52 @@ export function downloadFile(url: string, to: string): Promise { return runWithTimer('cmd_all_timing_network_ms')(() => { return new Promise((resolve, reject) => { - if (!fileExistsSync(dirname(to))) { - mkdirSync(dirname(to)) - } - - const file = createFileWriteStream(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))}`) - } - } - - file.on('finish', () => { - file.close() - resolve(to) - }) - - file.on('error', (err) => { - tryToRemoveFile() - reject(err) - }) - nodeFetch(url, {redirect: 'follow'}) .then((res) => { if (!res.ok) { - tryToRemoveFile() return reject(new Error(`Failed to download file from ${sanitizedUrl}: ${res.status} ${res.statusText}`)) } if (!res.body) { - tryToRemoveFile() return reject(new Error(`Failed to download file from ${sanitizedUrl}: Empty response body`)) } + + if (!fileExistsSync(dirname(to))) { + mkdirSync(dirname(to)) + } + + const file = createFileWriteStream(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('finish', () => { + file.close() + resolve(to) + }) + + 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))) }) })