From fec5d4c89bd8d897bcb9e92b5afd4dc5c8dac747 Mon Sep 17 00:00:00 2001 From: Abdul Wahab Date: Thu, 27 Aug 2026 10:42:58 +0500 Subject: [PATCH] Fix: safe temp staging and downloadFile error handling (#479) AI assistance: implemented with Cursor Grok 4.6. Co-authored-by: Cursor --- cli/bin/commands/skills.mjs | 86 ++++++++++-------- tests/skills-cli.test.js | 174 +++++++++++++++++++++++++++++++++++- 2 files changed, 221 insertions(+), 39 deletions(-) diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index edbf56790..8ad893d3c 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -9,11 +9,10 @@ */ import { execSync } from 'node:child_process'; -import { existsSync, readFileSync, readdirSync, statSync, lstatSync, unlinkSync, mkdirSync, writeFileSync, rmSync, rmdirSync, renameSync, createWriteStream, realpathSync, symlinkSync, readlinkSync, cpSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync, statSync, lstatSync, unlinkSync, mkdirSync, mkdtempSync, writeFileSync, rmSync, rmdirSync, renameSync, realpathSync, symlinkSync, readlinkSync, cpSync } from 'node:fs'; import { join, resolve, dirname, relative, isAbsolute, sep } from 'node:path'; import { createInterface, emitKeypressEvents } from 'node:readline'; import { fileURLToPath } from 'node:url'; -import { get } from 'node:https'; import { createHash } from 'node:crypto'; import { tmpdir, homedir } from 'node:os'; import { unzipSync } from 'fflate'; @@ -622,13 +621,17 @@ async function downloadAndExtractBundle() { const localBundle = process.env.IMPECCABLE_BUNDLE_PATH; if (localBundle) return copyOrExtractLocalBundle(localBundle); - const tmpZip = join(tmpdir(), `impeccable-update-${Date.now()}.zip`); - const tmpDir = join(tmpdir(), `impeccable-update-${Date.now()}`); - await downloadFile(`${API_BASE}/api/download/bundle/universal`, tmpZip); - mkdirSync(tmpDir, { recursive: true }); - await extractZip(tmpZip, tmpDir); - rmSync(tmpZip, { force: true }); - return tmpDir; + const staging = mkdtempSync(join(tmpdir(), 'impeccable-update-')); + const tmpZip = join(staging, 'bundle.zip'); + try { + await downloadFile(`${API_BASE}/api/download/bundle/universal`, tmpZip); + await extractZip(tmpZip, staging); + rmSync(tmpZip, { force: true }); + return staging; + } catch (e) { + rmSync(staging, { recursive: true, force: true }); + throw e; + } } async function copyOrExtractLocalBundle(sourceValue) { @@ -637,16 +640,18 @@ async function copyOrExtractLocalBundle(sourceValue) { throw new Error(`Local bundle not found: ${source}`); } - const tmpDir = join(tmpdir(), `impeccable-local-bundle-${process.pid}-${Date.now()}`); - mkdirSync(tmpDir, { recursive: true }); - - if (statSync(source).isDirectory()) { - cpSync(source, tmpDir, { recursive: true }); - return tmpDir; + const staging = mkdtempSync(join(tmpdir(), 'impeccable-local-bundle-')); + try { + if (statSync(source).isDirectory()) { + cpSync(source, staging, { recursive: true }); + } else { + await extractZip(source, staging); + } + return staging; + } catch (e) { + rmSync(staging, { recursive: true, force: true }); + throw e; } - - await extractZip(source, tmpDir); - return tmpDir; } /** @@ -2163,26 +2168,29 @@ function getModifiedSkillFiles(root, providerDirs) { return modified; } -function downloadFile(url, dest) { - return new Promise((resolve, reject) => { - const file = createWriteStream(dest); - get(url, (res) => { - if (res.statusCode >= 300 && res.statusCode < 400 && res.headers.location) { - // Follow redirect - get(res.headers.location, (res2) => { - res2.pipe(file); - file.on('finish', () => { file.close(); resolve(); }); - }).on('error', reject); - return; - } - if (res.statusCode !== 200) { - reject(new Error(`HTTP ${res.statusCode}`)); - return; - } - res.pipe(file); - file.on('finish', () => { file.close(); resolve(); }); - }).on('error', reject); - }); +async function downloadFile(url, dest, { fetchImpl = globalThis.fetch } = {}) { + let current = url; + let hopsLeft = 5; + while (true) { + const parsed = new URL(current); + if (parsed.protocol !== 'https:') { + throw new Error('Refusing non-HTTPS URL'); + } + const res = await fetchImpl(current, { redirect: 'manual' }); + if (res.status >= 300 && res.status < 400) { + const location = res.headers.get('location'); + if (!location) throw new Error(`HTTP ${res.status}`); + if (hopsLeft <= 0) throw new Error('Too many redirects'); + hopsLeft -= 1; + current = new URL(location, current).href; + continue; + } + if (res.status !== 200) { + throw new Error(`HTTP ${res.status}`); + } + writeFileSync(dest, Buffer.from(await res.arrayBuffer()), { flag: 'wx' }); + return; + } } async function update(flags = []) { @@ -2332,6 +2340,8 @@ export { copyProviderHooks, copyProviderSkills, decideHookInstall, + downloadAndExtractBundle, + downloadFile, expectedHookDests, extractZip, formatInstallDetectionLines, diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index 1156d3dc2..5aacf8e2e 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -11,7 +11,7 @@ */ import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; import { execSync, execFileSync } from 'child_process'; -import { mkdtempSync, existsSync, readdirSync, readFileSync, mkdirSync, writeFileSync, rmSync, lstatSync, realpathSync, readlinkSync, symlinkSync } from 'fs'; +import { mkdtempSync, existsSync, readdirSync, readFileSync, mkdirSync, writeFileSync, rmSync, lstatSync, realpathSync, readlinkSync, symlinkSync, statSync } from 'fs'; import { join } from 'path'; import { tmpdir } from 'os'; import { @@ -19,6 +19,8 @@ import { copyProviderHooks, copyProviderSkills, decideHookInstall, + downloadAndExtractBundle, + downloadFile, expectedHookDests, formatInstallDetectionLines, mergeHookManifests, @@ -2214,3 +2216,173 @@ describe('hermesGlobalHome resolver (PR #521)', () => { rmSync(home, { recursive: true, force: true }); }, 20000); }); + +describe('downloadAndExtractBundle: safe staging dir (#479)', () => { + test('local bundle uses mkdtemp under tmpdir with 0700 perms', async () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-staging-')); + const bundleRoot = createFakeUniversalBundle(tmp, ['.claude']); + const prev = process.env.IMPECCABLE_BUNDLE_PATH; + let stagingDir; + try { + process.env.IMPECCABLE_BUNDLE_PATH = bundleRoot; + stagingDir = await downloadAndExtractBundle(); + + expect(stagingDir.startsWith(tmpdir())).toBe(true); + const basename = stagingDir.split(/[/\\]/).pop(); + expect(basename.startsWith('impeccable-local-bundle-')).toBe(true); + expect(basename).not.toMatch(/^impeccable-local-bundle-\d+-\d+$/); + + if (process.platform !== 'win32') { + expect(statSync(stagingDir).mode & 0o777).toBe(0o700); + } + + expect(existsSync(join(stagingDir, '.claude', 'skills', 'impeccable', 'SKILL.md'))).toBe(true); + } finally { + if (prev === undefined) delete process.env.IMPECCABLE_BUNDLE_PATH; + else process.env.IMPECCABLE_BUNDLE_PATH = prev; + if (stagingDir) rmSync(stagingDir, { recursive: true, force: true }); + rmSync(tmp, { recursive: true, force: true }); + } + }); +}); + +describe('downloadFile (#479)', () => { + test('200 writes body to dest with wx flag', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async () => new Response('hello', { status: 200 }); + await downloadFile('https://example.com/file', dest, { fetchImpl }); + expect(readFileSync(dest, 'utf8')).toBe('hello'); + + await expect(downloadFile('https://example.com/file', dest, { fetchImpl })) + .rejects.toThrow(); + expect(readFileSync(dest, 'utf8')).toBe('hello'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('404 throws and dest does not exist', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async () => new Response('not found', { status: 404 }); + await expect(downloadFile('https://example.com/missing', dest, { fetchImpl })) + .rejects.toThrow(/HTTP 404/); + expect(existsSync(dest)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('redirect 302 to 200 follows location and writes second body', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + let callCount = 0; + const fetchImpl = async (url) => { + callCount++; + if (url === 'https://example.com/start') { + return new Response('', { status: 302, headers: { location: 'https://example.com/final' } }); + } + return new Response('final body', { status: 200 }); + }; + await downloadFile('https://example.com/start', dest, { fetchImpl }); + expect(callCount).toBe(2); + expect(readFileSync(dest, 'utf8')).toBe('final body'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('redirect 302 to 404 throws and dest does not exist', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async (url) => { + if (url.includes('/start')) { + return new Response('', { status: 302, headers: { location: 'https://example.com/bad' } }); + } + return new Response('error', { status: 404 }); + }; + await expect(downloadFile('https://example.com/start', dest, { fetchImpl })) + .rejects.toThrow(/HTTP 404/); + expect(existsSync(dest)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('redirect to http throws non-HTTPS and dest does not exist', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async () => new Response('', { status: 302, headers: { location: 'http://example.com/insecure' } }); + await expect(downloadFile('https://example.com/start', dest, { fetchImpl })) + .rejects.toThrow(/non-HTTPS/i); + expect(existsSync(dest)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('relative redirect location resolved against current URL', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async (url) => { + if (url === 'https://example.com/api/start') { + return new Response('', { status: 302, headers: { location: '/final' } }); + } + expect(url).toBe('https://example.com/final'); + return new Response('ok', { status: 200 }); + }; + await downloadFile('https://example.com/api/start', dest, { fetchImpl }); + expect(readFileSync(dest, 'utf8')).toBe('ok'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('more than maxRedirects hops throws and dest does not exist', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async () => new Response('', { status: 302, headers: { location: 'https://example.com/loop' } }); + await expect(downloadFile('https://example.com/loop', dest, { fetchImpl })) + .rejects.toThrow(/Too many redirects/); + expect(existsSync(dest)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('fetchImpl rejection leaves dest absent', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + const fetchImpl = async () => { throw new Error('network down'); }; + await expect(downloadFile('https://example.com/file', dest, { fetchImpl })) + .rejects.toThrow(/network down/); + expect(existsSync(dest)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test('http initial URL throws without calling fetch', async () => { + const dir = mkdtempSync(join(tmpdir(), 'imp-dl-')); + const dest = join(dir, 'out.bin'); + try { + let called = false; + const fetchImpl = async () => { called = true; return new Response('x', { status: 200 }); }; + await expect(downloadFile('http://example.com/file', dest, { fetchImpl })) + .rejects.toThrow(/non-HTTPS/i); + expect(called).toBe(false); + expect(existsSync(dest)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +});