Fix: stream bundle downloads to disk instead of buffering

AI assistance: implemented with Cursor Grok 4.6.

Co-authored-by: Cursor <cursoragent@cursor.com>

cli/bin/commands/skills.mjs

@@ -9,11 +9,12 @@
  */
 
 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, createWriteStream, realpathSync, symlinkSync, readlinkSync, cpSync } from 'node:fs';
 import { join, resolve, dirname, relative, isAbsolute, sep } from 'node:path';
 import { createInterface, emitKeypressEvents } from 'node:readline';
+import { Readable } from 'node:stream';
+import { pipeline } from 'node:stream/promises';
 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 +623,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 +642,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 +2170,35 @@ 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}`);
+    }
+    if (!res.body) throw new Error('Empty response body');
+    try {
+      await pipeline(Readable.fromWeb(res.body), createWriteStream(dest, { flags: 'wx' }));
+    } catch (e) {
+      if (e.code !== 'EEXIST') rmSync(dest, { force: true });
+      throw e;
+    }
+    return;
+  }
 }
 
 async function update(flags = []) {
@@ -2332,6 +2348,8 @@ export {
   copyProviderHooks,
   copyProviderSkills,
   decideHookInstall,
+  downloadAndExtractBundle,
+  downloadFile,
   expectedHookDests,
   extractZip,
   formatInstallDetectionLines,

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 });
+    }
+  });
+});