Fix: safe temp staging and downloadFile error handling (#479)

AI assistance: implemented with Cursor Grok 4.6. Co-authored-by: Cursor <cursoragent@cursor.com>
2026-09-19 01:26:29 +03:00 · 2026-08-27 10:42:58 +05:00
parent 63b04e2530
commit fec5d4c89b
2 changed files with 221 additions and 39 deletions
@@ -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 });
+    }
+  });
+});