fix(llm-security): F-2/F-3 — contain path traversal + kill npm-view shell injection

Session B of the security-fix track. Both sinks trusted untrusted strings at a
filesystem/subprocess boundary; same fix family as F-1.

F-2 (HIGH, arbitrary file write) — scanners/auto-cleaner.mjs: applyFixes() did
resolve(targetPath, f.file) with no containment, then wrote the cleaned content
back. f.file is untrusted (scanned-repo filenames, or a fully attacker-chosen
--findings file), so file: "../../.claude/settings.json" let the cleaner modify
files OUTSIDE the scanned tree. Add a prefix-containment check before grouping:
absPath must equal targetPath or start with targetPath + sep, else the finding
is refused and reported as skipped. (Documented residual gap: prefix containment
does not stop a symlink inside the tree pointing out — noted inline.)

F-3 (HIGH, command injection, pre-confirmation) — hooks/scripts/
pre-install-supply-chain.mjs: inspectNpmPackage ran execSafe(`npm view ${spec}
--json`), a shell string. spec derives from package tokens parsed out of the
scanned Bash command, so a metachar-bearing token reached the shell on
PreToolUse(Bash) — BEFORE the install, so it ran even if the user then denied
the command. Switch to spawnSync('npm', ['view', spec, '--json']) (no shell);
spec is passed as one argv element. The static `npm audit --json` execSafe call
is left as-is (no interpolation).

TDD (repro -> red -> green):
- tests/scanners/auto-cleaner-traversal.test.mjs: a "../secret.txt" finding must
  not rewrite the outside file; contained files still get cleaned.
- tests/hooks/supply-chain-injection.test.mjs: `npm install $(>/abs/PWNED)`
  (redirect-only $(...) survives normalizeBashExpansion + the whitespace split)
  must not create the sentinel.

Closing gates: full node --test suite 1863/0 (was 1860; +3); gitleaks clean;
F-1 regression re-run GREEN (sink still closed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V3s6WnubSSrFjAQTLQdVbG

hooks/scripts/pre-install-supply-chain.mjs

@@ -20,6 +20,7 @@
 //   - Allow (exit 0): everything else
 
 import { readFileSync, existsSync } from 'node:fs';
+import { spawnSync } from 'node:child_process';
 import {
   AGE_THRESHOLD_HOURS,
   NPM_COMPROMISED, PIP_COMPROMISED, CARGO_COMPROMISED, GEM_COMPROMISED,
@@ -251,7 +252,15 @@ function checkNpmProvenance(meta) {
 
 function inspectNpmPackage(name, version) {
   const spec = version ? `${name}@${version}` : name;
-  const raw = execSafe(`npm view ${spec} --json`);
+  // F-3: `spec` derives from attacker-controlled package tokens parsed out of the
+  // scanned Bash command. Pass it as a discrete argv element via spawnSync (no
+  // shell), so metacharacters like ';', '$(...)', backticks are never interpreted.
+  // npm still receives the full spec and reports the metadata (or an error JSON on
+  // stdout for an invalid name), matching the previous execSafe behaviour.
+  const res = spawnSync('npm', ['view', spec, '--json'], {
+    timeout: 10000, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'],
+  });
+  const raw = res.stdout || null;
   if (!raw) return null;
   try { return JSON.parse(raw); } catch { return null; }
 }

scanners/auto-cleaner.mjs

@@ -10,7 +10,7 @@
 
 import { readFile, writeFile, rename, unlink, stat } from 'node:fs/promises';
 import { writeFileSync, unlinkSync } from 'node:fs';
-import { resolve, extname, join, dirname } from 'node:path';
+import { resolve, extname, join, dirname, sep } from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { execSync } from 'node:child_process';
 import { fixResult, cleanEnvelope } from './lib/output.mjs';
@@ -774,6 +774,25 @@ async function applyFixes(targetPath, findings, dryRun) {
     }
 
     const absPath = resolve(targetPath, f.file);
+
+    // F-2: path-traversal containment. `f.file` is untrusted (scanned-repo
+    // filenames, or a fully attacker-chosen --findings file). A value like
+    // "../../.claude/settings.json" resolves OUTSIDE the scanned tree. Refuse
+    // anything that is not the target itself or contained within it, so the
+    // cleaner never writes outside the directory it was pointed at.
+    // NOTE: prefix containment does NOT defend against a symlink inside the
+    // tree pointing out of it — a known residual gap (see security-fix brief).
+    if (absPath !== targetPath && !absPath.startsWith(targetPath + sep)) {
+      fixes.push(fixResult({
+        finding_id: f.id,
+        file: f.file,
+        operation: 'skip',
+        status: 'skipped',
+        description: 'Path escapes target directory — refused (path traversal)',
+      }));
+      continue;
+    }
+
     if (!fileGroups.has(f.file)) {
       fileGroups.set(f.file, { findings: [], absPath });
     }

tests/hooks/supply-chain-injection.test.mjs

@@ -0,0 +1,54 @@
+// supply-chain-injection.test.mjs — Security regression for F-3 (HIGH, command injection).
+//
+// The pre-install-supply-chain hook inspects unknown npm packages via
+// inspectNpmPackage(), which historically ran `execSafe(`npm view ${spec} --json`)`
+// — a shell string. The `spec` derives from package tokens parsed out of the scanned
+// Bash command (extractNpmPackages -> parseSpec), so a token carrying shell
+// metacharacters reached the shell.
+//
+// Critically this fires on PreToolUse(Bash) BEFORE the user's install runs — so it
+// executes pre-confirmation, even if the user then DENIES the npm command.
+//
+// Payload note: `normalizeBashExpansion` rewrites ${IFS}, <(...), `...`, ${...} etc.,
+// but leaves `$(...)` command substitution intact. A redirect-only substitution
+// `$(>PATH)` contains no whitespace, so it survives extractNpmPackages' whitespace
+// split as a single "package" token, and `>PATH` creates PATH when the shell evaluates
+// the substitution. The test asserts that sentinel file is NEVER created — it must FAIL
+// against the execSync sink and PASS once inspectNpmPackage uses spawnSync (no shell).
+
+import { describe, it } from 'node:test';
+import assert from 'node:assert/strict';
+import { mkdtempSync, existsSync, rmSync } from 'node:fs';
+import { join, resolve } from 'node:path';
+import { tmpdir } from 'node:os';
+import { runHook } from './hook-helper.mjs';
+
+const SCRIPT = resolve(import.meta.dirname, '../../hooks/scripts/pre-install-supply-chain.mjs');
+
+function bashPayload(command) {
+  return { tool_name: 'Bash', tool_input: { command } };
+}
+
+describe('pre-install-supply-chain command-injection regression (F-3)', () => {
+  it('does not execute shell metacharacters embedded in an npm package spec', async () => {
+    const dir = mkdtempSync(join(tmpdir(), 'supply-chain-injection-'));
+    const sentinel = join(dir, 'PWNED');
+    try {
+      assert.ok(!existsSync(sentinel), 'precondition: sentinel must not exist before the hook runs');
+
+      // `$(>/abs/PWNED)` — redirect-only command substitution, no whitespace.
+      // Vulnerable: the shell creates PWNED while evaluating `npm view $(>...) --json`.
+      // Fixed: the literal string is passed as one argv element to `npm view`, no shell.
+      const result = await runHook(SCRIPT, bashPayload(`npm install $(>${sentinel})`));
+
+      assert.ok(
+        !existsSync(sentinel),
+        'COMMAND INJECTION: a shell-metachar npm spec executed during pre-install inspection',
+      );
+      // The hook must still terminate (block/warn/allow), not crash.
+      assert.ok([0, 2].includes(result.code), `hook should exit cleanly, got code ${result.code}`);
+    } finally {
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+});

tests/scanners/auto-cleaner-traversal.test.mjs

@@ -0,0 +1,96 @@
+// auto-cleaner-traversal.test.mjs — Security regression for F-2 (HIGH, arbitrary file write).
+//
+// auto-cleaner's applyFixes() historically did `resolve(targetPath, f.file)` with no
+// containment check, then wrote the modified content back to that path. `f.file` comes
+// from findings JSON — untrusted scanned-repo filenames, or a fully attacker-chosen
+// `--findings` file. A finding with `file: "../secret.txt"` therefore let the cleaner
+// modify (overwrite) a file OUTSIDE the scanned tree.
+//
+// This test places an auto-fixable file outside the target dir and points an auto-tier
+// finding at it via "../secret.txt". It must FAIL while the sink is uncontained (the
+// outside file gets rewritten) and PASS once applyFixes refuses paths that escape the
+// target (prefix containment), reporting them as skipped instead of writing.
+
+import { describe, it } from 'node:test';
+import assert from 'node:assert/strict';
+import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs';
+import { join } from 'node:path';
+import { tmpdir } from 'node:os';
+import { resetCounter } from '../../scanners/lib/output.mjs';
+import { applyFixes } from '../../scanners/auto-cleaner.mjs';
+
+const ZW = '​'; // zero-width space — strip_zero_width will remove it
+
+describe('auto-cleaner path-traversal regression (F-2)', () => {
+  it('refuses to write a finding whose file path escapes the target directory', async () => {
+    // root/                     <- mkdtemp parent
+    //   secret.txt              <- the escape target, OUTSIDE the scanned tree
+    //   scan-target/            <- the directory actually passed to the cleaner
+    const root = mkdtempSync(join(tmpdir(), 'auto-cleaner-traversal-'));
+    const target = join(root, 'scan-target');
+    const escape = join(root, 'secret.txt');
+    try {
+      mkdirSync(target, { recursive: true });
+
+      // The escape file carries a zero-width char so the (auto-tier) strip_zero_width
+      // op WOULD change it — proving a write actually reaches outside the tree if
+      // containment is missing.
+      const original = `secret${ZW}value\n`;
+      writeFileSync(escape, original, 'utf-8');
+
+      // Untrusted finding: classified 'auto' (UNI zero-width), file traverses up and out.
+      const findings = [{
+        id: 'TRAVERSAL-1',
+        scanner: 'UNI',
+        title: 'Zero-width characters detected',
+        severity: 'high',
+        file: '../secret.txt',
+      }];
+
+      resetCounter();
+      const { fixes } = await applyFixes(target, findings, /* dryRun */ false);
+
+      // The file outside the target must be byte-for-byte untouched.
+      assert.equal(
+        readFileSync(escape, 'utf-8'),
+        original,
+        'PATH TRAVERSAL: auto-cleaner rewrote a file outside the scanned target directory',
+      );
+
+      // And the escaping finding must be surfaced as refused/skipped, never applied.
+      const escaped = fixes.find(f => f.file === '../secret.txt');
+      assert.ok(escaped, 'the escaping finding should be reported');
+      assert.notEqual(escaped.status, 'applied', 'escaping finding must not be applied');
+    } finally {
+      rmSync(root, { recursive: true, force: true });
+    }
+  });
+
+  it('still applies fixes to files contained within the target directory', async () => {
+    const target = mkdtempSync(join(tmpdir(), 'auto-cleaner-contained-'));
+    try {
+      const inside = join(target, 'inside.md');
+      writeFileSync(inside, `---\ntitle: x${ZW}y\n---\nbody\n`, 'utf-8');
+
+      const findings = [{
+        id: 'CONTAINED-1',
+        scanner: 'UNI',
+        title: 'Zero-width characters detected',
+        severity: 'high',
+        file: 'inside.md',
+      }];
+
+      resetCounter();
+      const { fixes } = await applyFixes(target, findings, /* dryRun */ false);
+
+      assert.ok(
+        !readFileSync(inside, 'utf-8').includes(ZW),
+        'contained file should have been cleaned',
+      );
+      const applied = fixes.find(f => f.file === 'inside.md' && f.status === 'applied');
+      assert.ok(applied, 'a contained auto-fix should be applied');
+    } finally {
+      rmSync(target, { recursive: true, force: true });
+    }
+  });
+});