From 1c4c25161fdcf94c3ba9dcd6a5f9452b69b9e080 Mon Sep 17 00:00:00 2001 From: sec-check Date: Sat, 10 Oct 2026 22:46:56 -0400 Subject: [PATCH 1/2] fix(security): fail the audit gate when an allowlisted advisory goes stale scripts/audit-gate.mjs suppresses high/critical npm audit findings through ALLOWLISTED_ADVISORIES, on the premise -- stated in its header and in SECURITY.md -- that no patched release exists for the advisory. Nothing re-tested that premise, so the allowlist could only grow: the file asked for manual removal "the moment upstream publishes a fix", which is not an event anything in this repository observes. That premise has already been lost once here. SECURITY.md documented GHSA-ch52-4w7c-c8xp as unpatched while upstream had shipped 4.3.0, caught only by a hand re-check of the registry (#1050). Add unusedAllowlistEntries(), which reports allowlist entries that no advisory in the report reaches at any severity or any depth of the via chains, and fail the gate on them, naming both places to delete the entry from. An entry matching nothing suppresses nothing, and would silently accept the advisory again if a later dependency change reintroduced it. The check runs after the blocked-advisory check so a live unfixed advisory stays the first thing reported. This intentionally changes one contract: with a non-empty allowlist, a report with no vulnerabilities at all now fails, because that state means the allowlist is dead; the CLI test is updated to pin the new behaviour. The currently allowlisted braces advisory is still live (3.0.3 remains latest), so `npm run check:audit` still exits 0 against this lockfile. Closes #1295 Signed-off-by: sec-check --- SECURITY.md | 5 +++ scripts/audit-gate.mjs | 55 ++++++++++++++++++++++++++- tests/audit-gate.test.mjs | 80 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 136 insertions(+), 4 deletions(-) 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..efc6bcdb 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,71 @@ 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 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 +308,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', () => { From be9eac8de8dc56c05e5466b02da68553f2eb1168 Mon Sep 17 00:00:00 2001 From: Bob Killen Date: Sun, 11 Oct 2026 09:02:06 -0500 Subject: [PATCH 2/2] test(audit-gate): cover unusedAllowlistEntries on an entry with no via array The `entry.via ?? []` fallback in reportedAdvisoryIds() had no test reaching it, leaving scripts/audit-gate.mjs at 98.04% regions, the one source file under 100% and the one that would fail the ratchet in #1298. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen --- tests/audit-gate.test.mjs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/audit-gate.test.mjs b/tests/audit-gate.test.mjs index efc6bcdb..bfa60e11 100644 --- a/tests/audit-gate.test.mjs +++ b/tests/audit-gate.test.mjs @@ -238,6 +238,20 @@ 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(