fix(security): fail the audit gate when an allowlisted advisory goes stale

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

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',

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', () => {