diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 93ea541..9c979e0 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/deploy/desktop/README.md b/deploy/desktop/README.md index 0fd2ca9..8f43f19 100644 --- a/deploy/desktop/README.md +++ b/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 ` signs it as part of the run — which is where signing diff --git a/src/nodepilot-desktop/package.json b/src/nodepilot-desktop/package.json index a58abd0..2ba0ac5 100644 --- a/src/nodepilot-desktop/package.json +++ b/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", diff --git a/src/nodepilot-desktop/scripts/audit-gate.mjs b/src/nodepilot-desktop/scripts/audit-gate.mjs new file mode 100644 index 0000000..26a012e --- /dev/null +++ b/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(); +} diff --git a/src/nodepilot-desktop/scripts/audit-gate.test.mjs b/src/nodepilot-desktop/scripts/audit-gate.test.mjs new file mode 100644 index 0000000..1f203bb --- /dev/null +++ b/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); + } + }); +}); diff --git a/src/nodepilot-desktop/vitest.config.ts b/src/nodepilot-desktop/vitest.config.ts index f80fd9c..49cb372 100644 --- a/src/nodepilot-desktop/vitest.config.ts +++ b/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'], }, });