Skip to content

ci(security): advisory-ID allowlist for Dependency Audit - #94

Open
mattglory wants to merge 2 commits into
mainfrom
ci-audit-allowlist
Open

mattglory wants to merge 2 commits into
mainfrom
ci-audit-allowlist

Conversation

@mattglory

Copy link
Copy Markdown
Owner

Summary

Hillary's proposal (direct message, 2026-10-07): the Dependency Audit check has been permanently red since #85's accepted braces finding, and a check that's always red stops being read -- demonstrated directly, since two separate new advisories (source-map-js/PostCSS, then sharp) went unnoticed for about a day while it sat red for an unrelated reason.

scripts/audit-allowlist.mjs wraps npm audit --json and fails only on a finding at or above --audit-level whose advisory ID isn't already a documented, accepted-risk row in FINDINGS_REGISTER.md. Matched by advisory ID, not package name, so a new advisory on an already-allowlisted package still fails. It also refuses to run if an allowlisted ID isn't actually documented there -- the register stays the single source of truth, not a second list that can drift from it (same discipline KNOWN_TEST_PATHS already applies in mainnet-plan-guard.test.ts).

No new dependency; ~90 lines, no third-party tool added to the pipeline that polices dependencies.

Verification

Tested against this repo's real npm audit output in both trees before wiring in, not just plausible-looking code:

  • Current state, both root and web/ -> passes.
  • Removed braces' ID from the allowlist -> correctly fails, names braces (high): GHSA-vfj7-8cjw-p6xm.
  • Added an undocumented ID to the allowlist -> correctly refuses to run until it's either documented or removed.

One correction this surfaced: elliptic's GHSA-848j-6mx2-7j84 (DEP-2 sub-issue 2) is low severity, so it never independently triggered --audit-level=high on its own -- confirmed directly against both trees. Kept in the allowlist anyway, documented why, so a future lower --audit-level doesn't start failing on an already-accepted finding.

Test plan

  • node scripts/audit-allowlist.mjs --audit-level=high from root -- passes
  • cd web && node ../scripts/audit-allowlist.mjs --audit-level=high -- passes
  • Full suite: 279 passed + 1 expected fail, no regressions
  • Negative-path testing described above (allowlist removal, undocumented-ID refusal)

🤖 Generated with Claude Code

…'s proposal

Replaces the bare `npm audit --audit-level=high` at both call sites with
scripts/audit-allowlist.mjs. The check has been permanently red since #85
(the accepted braces finding, GHSA-vfj7-8cjw-p6xm), and that cost real
findings visibility: two separate new advisories (the source-map-js/
PostCSS trio, then sharp) went unnoticed for roughly a day because a
check that's always red stops getting read.

The script fails only on a finding at or above --audit-level whose
advisory ID isn't already a documented, accepted-risk row in
FINDINGS_REGISTER.md -- checked against that file directly, not a
second list that can drift from it, the same discipline
KNOWN_TEST_PATHS already applies in mainnet-plan-guard.test.ts. Matched
by advisory ID, not package name, so a new advisory on an
already-allowlisted package still fails.

Tested against this repo's real npm audit output in both trees before
wiring in: passes today, fails if an accepted ID is removed from the
allowlist, and refuses to run if an allowlisted ID isn't documented in
FINDINGS_REGISTER.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
web Ready Ready Preview Oct 9, 2026 9:35am UTC

Request Review

@unixwhisperer unixwhisperer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is the shape I hoped for, and checking each ID against FINDINGS_REGISTER.md is a good addition.

I ran it locally at 86d4ac8: root and web/ both pass. Removing an ID makes it fail, and the register check works.

One blocker: it passes when the audit itself fails. With an unreachable registry (npm_config_registry=http://127.0.0.1:1 node scripts/audit-allowlist.mjs), npm returns {"message": "...ECONNREFUSED...", "error": {...}} with no vulnerabilities key. The script parses that, finds nothing, prints "every finding … is on the allowlist", and exits 0. A registry outage or rate limit in CI would show as a green audit. Suggested fix, right after parsing:

if (report.error || !report.vulnerabilities) {
  console.error(`npm audit did not return results: ${report.message ?? JSON.stringify(report.error)}`);
  process.exit(2);
}

Nits (non-blocking):

  • The usage comment shows node ../scripts/audit-allowlist.mjs for the root call. From the root it's node scripts/audit-allowlist.mjs, which is what the workflow already does.
  • The threshold check uses each package's overall vuln.severity rather than each advisory's own severity. If a package has an allowlisted high advisory plus a new moderate one, the moderate one would fail the high-level run. That's stricter than intended, not looser, so it's fine to leave. Filtering on v.severity inside the via loop would make it exact.

I'll approve once the fail-open case is fixed.

Hillary Kibet's review on #94: with an unreachable registry
(npm_config_registry=http://127.0.0.1:1), npm audit --json returns
{"message": "...ECONNREFUSED...", "error": {...}} with no
`vulnerabilities` key. The script parsed that, found nothing to
iterate, and reported a clean audit -- a registry outage or rate
limit in CI would show as green instead of surfacing the real
problem. Now exits 2 with the actual error message whenever
`report.vulnerabilities` is missing.

Reproduced his exact repro before fixing, confirmed fail-open on the
old code and fail-closed (exit 2) on the fix, in both the root and
web/ trees. Also fixes the usage comment's root-call example, which
showed the web/-relative `../scripts/...` path for both calls.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mattglory

Copy link
Copy Markdown
Owner Author

The fail-open fix you flagged is already on this branch — `7d3c9eb`, pushed before this review landed:

```js
if (report.error || !report.vulnerabilities) {
console.error(`npm audit did not return results: ${report.message ?? JSON.stringify(report.error)}`);
process.exit(2);
}
```

Reproduced your exact repro (`npm_config_registry=http://127.0.0.1:1 node scripts/audit-allowlist.mjs`) before and after: fail-open on the old code, exit 2 with the real error message on the fix, in both the root and `web/` trees. Also fixed the usage-comment nit (root-call example was showing the `web/`-relative `../scripts/...` path).

Nits noted, agreed non-blocking as you said. Whenever you get a chance to re-check `7d3c9eb` — should be ready for your approval.

This branch was successfully deployed

1 active deployment
Preview — 7d3c9ebf Deployed Oct 9, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants