diff --git a/SECURITY.md b/SECURITY.md index 368f9983..04c37da5 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -57,6 +57,11 @@ listed here until upstream published `4.3.0` — outside that advisory's no longer appears, which is why the count above dropped from 34 to 28. When an entry here is said to have no fix, re-check the registry before trusting it: "no patched version" is only true as of the day it was written. +`npm run check:audit` now re-checks it for you on every run: once an allowlisted +advisory stops appearing in the lockfile, the gate fails and names the entry to +remove from both `scripts/audit-gate.mjs` and the table above, so a stale "no +patched version" claim cannot sit here unnoticed the way GHSA-ch52-4w7c-c8xp +did. CI enforces this table: `npm run check:audit` (`scripts/audit-gate.mjs`) fails the build on any high or critical advisory other than the allowlisted ones diff --git a/scripts/audit-gate.mjs b/scripts/audit-gate.mjs index 7a3f051c..359e6542 100644 --- a/scripts/audit-gate.mjs +++ b/scripts/audit-gate.mjs @@ -24,7 +24,9 @@ import { spawnSync } from 'node:child_process'; // must correspond to a row in SECURITY.md's "Known unpatched dependency // advisories" table; remove the entry the moment upstream publishes a fix // (for braces: when `npm view braces versions` lists a release above 3.0.3), -// so the gate starts enforcing it. +// so the gate starts enforcing it. unusedAllowlistEntries() below fails the +// gate once an entry stops matching anything in the lockfile, so that removal +// is prompted rather than left to be noticed. export const ALLOWLISTED_ADVISORIES = new Set([ // braces <= 3.0.3 via micromatch in @docusaurus/utils globbing; build-time // only, never shipped to the published site. @@ -94,6 +96,40 @@ export function classifyAudit(report, allowlist = ALLOWLISTED_ADVISORIES) { return { blocked, allowlisted }; } +// Collects every advisory id the report mentions, at any severity and at any +// depth of the `via` chains. Severity is deliberately not filtered: an +// advisory that is rescored from high to moderate is still present in the +// lockfile, and an allowlist entry covering it is still doing work. +function reportedAdvisoryIds(report) { + const ids = new Set(); + for (const entry of Object.values(report.vulnerabilities ?? {})) { + for (const via of entry.via ?? []) { + if (typeof via !== 'string') ids.add(advisoryId(via)); + } + } + return ids; +} + +// Reports allowlist entries that no advisory in the report reaches. +// +// Every entry here suppresses a high/critical finding on the stated premise +// that no patched release exists. Nothing re-tests that premise: once the +// advisory leaves the lockfile -- because upstream published a fix, or the +// dependency was dropped -- the entry becomes a standing suppression for a +// problem that is now fixable, and the gate goes on accepting the advisory if +// a later dependency change reintroduces it. Since nothing but this check +// would ever notice, an unused entry fails the gate the same way an +// unidentifiable advisory does, so removing it (and its SECURITY.md row) is +// forced at the moment the reason for it expires rather than whenever someone +// next happens to re-read the allowlist. +export function unusedAllowlistEntries( + report, + allowlist = ALLOWLISTED_ADVISORIES, +) { + const reported = reportedAdvisoryIds(report); + return [...allowlist].filter((id) => !reported.has(id)).sort(); +} + function main() { // --package-lock-only audits the lockfile without needing node_modules, // which is also what keeps the result identical between a fresh CI @@ -154,6 +190,23 @@ function main() { ); process.exit(1); } + // Reported after the blocked check: a live advisory outside the allowlist is + // the more urgent signal, and naming a dead exemption first would bury it. + const unused = unusedAllowlistEntries(report); + if (unused.length > 0) { + console.error( + 'audit-gate: allowlisted advisories no longer present in the lockfile:', + ); + for (const id of unused) console.error(` ${id}`); + console.error( + 'The advisory is fixed or the dependency is gone, so the allowlist ' + + 'entry now suppresses nothing and would silently accept the advisory ' + + 'if it came back. Remove it from ALLOWLISTED_ADVISORIES in ' + + "scripts/audit-gate.mjs and drop its row from SECURITY.md's " + + '"Known unpatched dependency advisories" table.', + ); + process.exit(1); + } console.log( `audit-gate: ok — ${allowlisted.length} high/critical finding(s), all ` + 'tracing to allowlisted advisories documented in SECURITY.md', diff --git a/tests/audit-gate.test.mjs b/tests/audit-gate.test.mjs index 715505a2..bfa60e11 100644 --- a/tests/audit-gate.test.mjs +++ b/tests/audit-gate.test.mjs @@ -25,6 +25,7 @@ import { ALLOWLISTED_ADVISORIES, advisoryId, classifyAudit, + unusedAllowlistEntries, } from '../scripts/audit-gate.mjs'; const repoRoot = fileURLToPath(new URL('..', import.meta.url)); @@ -184,6 +185,85 @@ test('the allowlist matches the advisories SECURITY.md documents', () => { } }); +// --- unusedAllowlistEntries --- + +test('unusedAllowlistEntries reports nothing while the advisory is present', () => { + const report = { + vulnerabilities: { + braces: entry('high', [advisory(BRACES)]), + micromatch: entry('high', ['braces']), + }, + }; + assert.deepEqual(unusedAllowlistEntries(report, new Set([BRACES])), []); +}); + +test('unusedAllowlistEntries names an entry the report no longer reaches', () => { + const report = { + vulnerabilities: { newdep: entry('high', [advisory(NEW_BAD)]) }, + }; + assert.deepEqual(unusedAllowlistEntries(report, new Set([BRACES])), [BRACES]); +}); + +test('unusedAllowlistEntries names every entry on a report with no findings', () => { + assert.deepEqual( + unusedAllowlistEntries({ vulnerabilities: {} }, new Set([BRACES, NEW_BAD])), + [NEW_BAD, BRACES].sort(), + ); +}); + +test('unusedAllowlistEntries counts an advisory at any severity as present', () => { + // An advisory rescored below high is still in the lockfile, so the entry + // covering it is still doing work and must not be called stale. + const report = { + vulnerabilities: { + braces: entry('moderate', [advisory(BRACES, 'moderate')]), + }, + }; + assert.deepEqual(unusedAllowlistEntries(report, new Set([BRACES])), []); +}); + +test('unusedAllowlistEntries finds an advisory nested behind a string via', () => { + // The advisory object can sit on a package the top-level entry only names + // by string; the scan must reach it rather than call the entry stale. + const report = { + vulnerabilities: { + micromatch: entry('high', ['braces']), + braces: entry('high', [advisory(BRACES)]), + }, + }; + assert.deepEqual(unusedAllowlistEntries(report, new Set([BRACES])), []); +}); + +test('unusedAllowlistEntries tolerates a report with no vulnerabilities key', () => { + assert.deepEqual(unusedAllowlistEntries({}, new Set([BRACES])), [BRACES]); +}); + +test('unusedAllowlistEntries tolerates an entry without a via array', () => { + // Mirrors the classifyAudit case above: an entry with no `via` reaches no + // advisory, so it neither crashes the scan nor keeps an entry alive. + const report = { + vulnerabilities: { + bare: { severity: 'high' }, + braces: entry('high', [advisory(BRACES)]), + }, + }; + assert.deepEqual(unusedAllowlistEntries(report, new Set([BRACES, NEW_BAD])), [ + NEW_BAD, + ]); +}); + +test('unusedAllowlistEntries defaults to the shipped allowlist', () => { + const report = { + vulnerabilities: Object.fromEntries( + [...ALLOWLISTED_ADVISORIES].map((id, index) => [ + `pkg-${index}`, + entry('high', [advisory(id)]), + ]), + ), + }; + assert.deepEqual(unusedAllowlistEntries(report), []); +}); + // --- CLI entry, with npm stubbed --- // Stub placed first on PATH so the script under test never reaches the real @@ -242,12 +322,20 @@ function runGate({ stdout = '', status = '0', withNpm = true } = {}) { } } -test('cli: exits 0 on a clean report', () => { +test('cli: exits 1 on a clean report, because the allowlist is then stale', () => { + // A report with no vulnerabilities at all means every allowlisted advisory + // has left the lockfile. The suppression is then pointless and, worse, + // still live for a future reintroduction, so the gate asks for it to be + // removed rather than reporting a green audit over a dead exemption. The + // "exits 0 when every finding is allowlisted" test below covers the ok + // path, which is what a clean *and* empty allowlist would also produce. const result = runGate({ stdout: JSON.stringify({ vulnerabilities: {} }), }); - assert.equal(result.status, 0, result.stderr); - assert.match(result.stdout, /audit-gate: ok/); + assert.equal(result.status, 1); + assert.match(result.stderr, /no longer present in the lockfile/); + assert.match(result.stderr, new RegExp(BRACES)); + assert.doesNotMatch(result.stdout, /audit-gate: ok/); }); test('cli: exits 0 when every finding is allowlisted, despite npm exiting 1', () => {