ci: excuse the unfixable extract-zip advisory by id instead of by hand

.github/workflows/ci.yml

@@ -227,10 +227,13 @@ jobs:
         run: npm ci
 
       - name: Reject vulnerable npm dependencies
-        # Clean only because package.json pins three overrides into Forge's own tree — see the
-        # "//overrides" note there. If this goes red after a Forge bump, re-check whether an
-        # override became redundant (drop it) or a new advisory appeared (add one).
-        run: npm audit --audit-level=moderate
+        # Not a bare `npm audit`: this tree carries one advisory with no fixed release
+        # anywhere upstream (extract-zip, via Electron Forge), which no override can clear.
+        # The gate keeps the full audit and excuses single advisories by id, each with a
+        # recorded reason — see scripts/audit-gate.mjs and the "//audit" note in package.json.
+        # `--omit=dev` would be the wrong escape here: dependencies is empty and electron
+        # itself is a devDependency, so it would audit nothing at all.
+        run: npm run audit:ci
 
       - name: Type check
         run: npm run typecheck

deploy/desktop/README.md

@@ -240,6 +240,15 @@ Honest inventory so nobody assumes more coverage than exists:
   for what each one is. Nothing ships from this: Forge is build-time only and `dependencies` is
   empty. Re-check the list whenever Forge is bumped; an override that Forge has caught up with is
   dead weight.
+- **One advisory cannot be overridden at all, and is excused by id.** `extract-zip`
+  (GHSA-jmr9-qjv8-65gv, high, reached through `@electron/packager`) has no patched release
+  anywhere — there is no version to point an override at, and `npm audit fix` cannot help.
+  The `desktop` job therefore runs `npm run audit:ci` (`scripts/audit-gate.mjs`) instead of a
+  bare `npm audit`: the audit stays full, and that single advisory is excused by id with its
+  reason recorded next to it. Anything else at moderate or above still fails the build, and the
+  gate prints a `stale` line once the excuse stops matching, which is the signal to delete it.
+  `--omit=dev` is deliberately not used: `dependencies` is empty and electron is a
+  devDependency, so it would audit nothing at all.
 - **The installer is unsigned unless you ask for a signature.** `Build-DesktopInstaller.ps1` alone
   never signs. Building through `deploy\Build-Artifact.ps1 -IncludeDesktopInstaller
   -InstallerSigningCertificateThumbprint <tp>` signs it as part of the run — which is where signing

src/nodepilot-desktop/package.json

@@ -21,6 +21,7 @@
     "typecheck": "tsc --noEmit",
     "test": "vitest",
     "test:run": "vitest run",
+    "audit:ci": "node scripts/audit-gate.mjs",
     "start": "npm run build && electron .",
     "package": "npm run build && electron-forge package",
     "make": "npm run build && electron-forge make"
@@ -31,6 +32,13 @@
     "not the newest release line. Bumping it past the runtime would type-check against",
     "APIs the shipped Electron does not have."
   ],
+  "//audit": [
+    "One advisory in Forge's tree has no fixed release anywhere, so no override can clear",
+    "it. It is excused by id in scripts/audit-gate.mjs, which CI runs in place of a bare",
+    "npm audit. Read the reason recorded there before adding a second entry.",
+    "`npm audit --omit=dev` is not the escape it looks like: dependencies is empty and",
+    "electron is a devDependency, so it would audit nothing at all."
+  ],
   "//overrides": [
     "Electron Forge's own dependency tree pulls three packages whose only patched",
     "releases sit in a newer major line than the one Forge asks for, so npm cannot",

src/nodepilot-desktop/scripts/audit-gate.mjs

@@ -0,0 +1,137 @@
+// Dependency-vulnerability gate for the Electron shell.
+//
+// The other three Node jobs in CI get away with a plain `npm audit --audit-level=moderate`.
+// This package cannot, because it has an advisory with no fixed release anywhere upstream,
+// and the two blunt escapes both destroy the check:
+//
+//   * `--omit=dev` audits nothing at all here. `dependencies` is empty and electron itself
+//     is a devDependency (Electron Forge requires that), so omitting dev dependencies would
+//     silently stop reporting Electron CVEs — the one package in this tree that reaches users.
+//   * `--audit-level=critical` would let every future high-severity advisory through.
+//
+// So the audit stays full and single advisories are excused by id, each with a reason and a
+// condition for dropping it again. Anything not on the list still fails the build.
+import { execFileSync } from 'node:child_process';
+import { pathToFileURL } from 'node:url';
+
+/**
+ * Reviewed advisories that cannot be resolved from this repository. Keep this list at zero
+ * entries whenever upstream allows it: an entry here is a vulnerability we are shipping past,
+ * not one we have fixed. `stale` output below tells you when an entry can go.
+ */
+export const ALLOWLIST = [
+  {
+    id: 'GHSA-jmr9-qjv8-65gv',
+    package: 'extract-zip',
+    reason:
+      'Unvalidated symlink path traversal, high. Reaches us only through the Electron Forge ' +
+      'dev tree (@electron/packager -> extract-zip) and has no patched release at all — ' +
+      'first_patched_version is null, so neither an override nor `npm audit fix` can clear ' +
+      'it. Forge is build-time only; nothing from this path is packaged into the installer. ' +
+      'Drop this entry once Forge stops depending on extract-zip or a fixed version ships.',
+  },
+];
+
+const SEVERITY_ORDER = ['info', 'low', 'moderate', 'high', 'critical'];
+
+function advisoryIdFrom(url) {
+  const match = /\/advisories\/(GHSA-[a-z0-9-]+)/i.exec(url ?? '');
+  return match ? match[1] : null;
+}
+
+/**
+ * Folds an `npm audit --json` report into one entry per advisory. npm reports the same
+ * advisory once per affected package — the extract-zip finding alone fans out across 15
+ * entries of the Forge chain — and expresses transitive hits as plain strings in `via`,
+ * which carry no advisory of their own.
+ */
+export function collectAdvisories(report) {
+  const byId = new Map();
+  for (const entry of Object.values(report?.vulnerabilities ?? {})) {
+    for (const via of entry?.via ?? []) {
+      if (typeof via !== 'object' || via === null) continue;
+      const id = advisoryIdFrom(via.url);
+      if (!id) continue;
+      if (!byId.has(id)) {
+        byId.set(id, {
+          id,
+          url: via.url,
+          title: via.title ?? '(no title)',
+          severity: via.severity ?? 'info',
+          packages: new Set(),
+        });
+      }
+      if (via.name) byId.get(id).packages.add(via.name);
+    }
+  }
+  return [...byId.values()]
+    .map((a) => ({ ...a, packages: [...a.packages].sort() }))
+    .sort((a, b) => a.id.localeCompare(b.id));
+}
+
+/**
+ * Splits the report into what must fail the build, what is excused, and which allowlist
+ * entries no longer match anything and should be deleted.
+ */
+export function evaluate(report, { allowlist = ALLOWLIST, minSeverity = 'moderate' } = {}) {
+  const floor = SEVERITY_ORDER.indexOf(minSeverity);
+  if (floor < 0) throw new Error(`unknown severity: ${minSeverity}`);
+  const advisories = collectAdvisories(report);
+  const excusedIds = new Set(allowlist.map((e) => e.id));
+  const atOrAboveFloor = advisories.filter((a) => SEVERITY_ORDER.indexOf(a.severity) >= floor);
+  const present = new Set(advisories.map((a) => a.id));
+  return {
+    blocking: atOrAboveFloor.filter((a) => !excusedIds.has(a.id)),
+    excused: atOrAboveFloor.filter((a) => excusedIds.has(a.id)),
+    stale: allowlist.filter((e) => !present.has(e.id)),
+  };
+}
+
+function runNpmAudit() {
+  const npm = process.platform === 'win32' ? 'npm.cmd' : 'npm';
+  try {
+    // shell: true on Windows because npm is a .cmd shim, which execFileSync has refused to
+    // spawn directly since Node 20 (CVE-2024-27980). The argument vector is a literal.
+    return execFileSync(npm, ['audit', '--json'], {
+      encoding: 'utf8',
+      maxBuffer: 64 * 1024 * 1024,
+      shell: process.platform === 'win32',
+    });
+  } catch (error) {
+    // npm audit exits non-zero the moment it finds anything; the report still lands on stdout.
+    if (error?.stdout) return error.stdout;
+    throw error;
+  }
+}
+
+function main() {
+  const { blocking, excused, stale } = evaluate(JSON.parse(runNpmAudit()));
+
+  for (const a of excused) {
+    const entry = ALLOWLIST.find((e) => e.id === a.id);
+    console.log(`excused  [${a.severity}] ${a.id} ${a.title}`);
+    console.log(`         packages: ${a.packages.join(', ')}`);
+    console.log(`         reason: ${entry.reason}`);
+  }
+  for (const e of stale) {
+    console.log(`stale    ${e.id} (${e.package}) no longer reported — remove it from ALLOWLIST`);
+  }
+  for (const a of blocking) {
+    console.error(`BLOCKING [${a.severity}] ${a.id} ${a.title}`);
+    console.error(`         packages: ${a.packages.join(', ')}`);
+    console.error(`         ${a.url}`);
+  }
+
+  if (blocking.length > 0) {
+    console.error(
+      `\nnpm audit found ${blocking.length} advisory/advisories at moderate or above that are ` +
+        'not on the reviewed allowlist in scripts/audit-gate.mjs.',
+    );
+    process.exit(1);
+  }
+  console.log(`audit-gate: clean (${excused.length} excused, ${stale.length} stale).`);
+}
+
+if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
+  main();
+}

src/nodepilot-desktop/scripts/audit-gate.test.mjs

@@ -0,0 +1,137 @@
+import { describe, expect, it } from 'vitest';
+
+import { ALLOWLIST, collectAdvisories, evaluate } from './audit-gate.mjs';
+
+/**
+ * The gate decides whether a dependency advisory fails the build. Its whole reason for
+ * existing is that one advisory is excused — so the tests that matter are the ones proving
+ * it excuses exactly that one and nothing else, and that it tells us when the excuse expires.
+ */
+
+const advisory = (overrides = {}) => ({
+  source: 1,
+  name: 'extract-zip',
+  dependency: 'extract-zip',
+  title: 'extract-zip unvalidated symlink path traversal',
+  url: 'https://github.com/advisories/GHSA-jmr9-qjv8-65gv',
+  severity: 'high',
+  ...overrides,
+});
+
+const report = (vulnerabilities) => ({ vulnerabilities });
+
+describe('collectAdvisories', () => {
+  it('folds one advisory reported against many packages into a single entry', () => {
+    // This is the real shape: npm repeats the extract-zip finding once per package in the
+    // Electron Forge chain — 15 entries in CI for one underlying problem.
+    const result = collectAdvisories(
+      report({
+        'extract-zip': { via: [advisory()] },
+        '@electron/packager': { via: [advisory({ name: '@electron/packager' })] },
+        '@electron-forge/core': { via: [advisory({ name: '@electron-forge/core' })] },
+      }),
+    );
+
+    expect(result).toHaveLength(1);
+    expect(result[0].id).toBe('GHSA-jmr9-qjv8-65gv');
+    expect(result[0].packages).toEqual([
+      '@electron-forge/core',
+      '@electron/packager',
+      'extract-zip',
+    ]);
+  });
+
+  it('ignores the plain-string via entries npm uses for transitive hits', () => {
+    const result = collectAdvisories(
+      report({
+        'extract-zip': { via: [advisory()] },
+        '@electron/packager': { via: ['extract-zip'] },
+      }),
+    );
+
+    expect(result).toHaveLength(1);
+    expect(result[0].packages).toEqual(['extract-zip']);
+  });
+
+  it('skips entries whose url is not a GitHub advisory', () => {
+    expect(collectAdvisories(report({ weird: { via: [advisory({ url: 'https://example.test' })] } }))).toEqual([]);
+  });
+
+  it('returns nothing for a clean report', () => {
+    expect(collectAdvisories(report({}))).toEqual([]);
+    expect(collectAdvisories({})).toEqual([]);
+    expect(collectAdvisories(undefined)).toEqual([]);
+  });
+});
+
+describe('evaluate', () => {
+  const allowlist = [{ id: 'GHSA-jmr9-qjv8-65gv', package: 'extract-zip', reason: 'test' }];
+
+  it('blocks an advisory that is not on the allowlist', () => {
+    const result = evaluate(report({ tar: { via: [advisory({ name: 'tar', url: 'https://github.com/advisories/GHSA-aaaa-bbbb-cccc' })] } }), { allowlist });
+
+    expect(result.blocking.map((a) => a.id)).toEqual(['GHSA-aaaa-bbbb-cccc']);
+    expect(result.excused).toEqual([]);
+  });
+
+  it('excuses an advisory that is on the allowlist', () => {
+    const result = evaluate(report({ 'extract-zip': { via: [advisory()] } }), { allowlist });
+
+    expect(result.blocking).toEqual([]);
+    expect(result.excused.map((a) => a.id)).toEqual(['GHSA-jmr9-qjv8-65gv']);
+  });
+
+  it('excuses only the listed advisory when both kinds are present', () => {
+    const result = evaluate(
+      report({
+        'extract-zip': { via: [advisory()] },
+        tar: { via: [advisory({ name: 'tar', url: 'https://github.com/advisories/GHSA-aaaa-bbbb-cccc' })] },
+      }),
+      { allowlist },
+    );
+
+    expect(result.blocking.map((a) => a.id)).toEqual(['GHSA-aaaa-bbbb-cccc']);
+    expect(result.excused.map((a) => a.id)).toEqual(['GHSA-jmr9-qjv8-65gv']);
+  });
+
+  it('ignores advisories below the severity floor', () => {
+    const low = report({ x: { via: [advisory({ name: 'x', severity: 'low', url: 'https://github.com/advisories/GHSA-dddd-eeee-ffff' })] } });
+
+    expect(evaluate(low, { allowlist }).blocking).toEqual([]);
+    expect(evaluate(low, { allowlist, minSeverity: 'low' }).blocking).toHaveLength(1);
+  });
+
+  it('rejects an unknown severity floor rather than silently passing everything', () => {
+    expect(() => evaluate(report({}), { minSeverity: 'catastrophic' })).toThrow(/unknown severity/);
+  });
+
+  it('reports an allowlist entry that no longer matches so it can be deleted', () => {
+    const result = evaluate(report({}), { allowlist });
+
+    expect(result.stale.map((e) => e.id)).toEqual(['GHSA-jmr9-qjv8-65gv']);
+  });
+
+  it('does not call a still-present entry stale', () => {
+    expect(evaluate(report({ 'extract-zip': { via: [advisory()] } }), { allowlist }).stale).toEqual([]);
+  });
+});
+
+describe('the shipped ALLOWLIST', () => {
+  it('excuses the extract-zip advisory that CI actually reports', () => {
+    // Guards against a typo in the id: the entry would then quietly excuse nothing and the
+    // desktop job would go red again for a reason nobody expects.
+    const result = evaluate(report({ 'extract-zip': { via: [advisory()] } }));
+
+    expect(result.blocking).toEqual([]);
+    expect(result.excused).toHaveLength(1);
+    expect(result.stale).toEqual([]);
+  });
+
+  it('carries a reason for every entry, so no exception is undocumented', () => {
+    for (const entry of ALLOWLIST) {
+      expect(entry.id).toMatch(/^GHSA-[a-z0-9-]+$/i);
+      expect(entry.package).toBeTruthy();
+      expect(entry.reason.length).toBeGreaterThan(60);
+    }
+  });
+});

src/nodepilot-desktop/vitest.config.ts

@@ -10,6 +10,8 @@ export default defineConfig({
   test: {
     environment: 'node',
     globals: true,
-    include: ['src/**/*.test.ts'],
+    // scripts/ carries the CI dependency-audit gate; its decision logic is pure and belongs
+    // under the same suite as the rest of the shell's pure logic.
+    include: ['src/**/*.test.ts', 'scripts/**/*.test.mjs'],
   },
 });