fix(deps): remediate advisories and make the dependency audit blocking - #44
Draft
MaazAhmed47 wants to merge 5 commits into
Draft
fix(deps): remediate advisories and make the dependency audit blocking#44MaazAhmed47 wants to merge 5 commits into
MaazAhmed47 wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The gate computed its verdict from advisory objects flattened out of each node's `via`, but a report declares vulnerabilities through `metadata` and per-node `severity` — fields the flattening never reads. A report naming one high-severity vulnerable package with entirely self-consistent metadata and `via: []` therefore validated, flattened to zero rows, printed "no advisories in this scope" and exited 0. Close it at every layer the verdict passes through: * advisory-graph completeness — every vulnerable node must reach a real advisory object. Empty `via`, an edge naming a package the report does not contain, and cycles that never terminate in evidence are all rejected, each with its own diagnostic. Object/string `via` mixtures and multi-hop string chains stay valid, because that is what npm actually emits; * advisory-object validation — severity, range, package name and the url/source identity must all be present and well-formed, since each is a fact an exception is matched against and a missing one silently became an empty string; * exact severity-bucket comparison — buckets are compared against the node severities one for one, so a finding cannot be restated on its way into the summary while the totals still add up; * a closing invariant in the flattening itself — a summary declaring vulnerable packages can never produce zero advisory rows, whatever path the report took to get there. Policy loading and date parsing move inside the fail-closed boundary. An unreadable policy file or an unparsable expiry used to escape as a traceback and exit 1 — indistinguishable from "policy violation". Both now exit 2 with a clean diagnostic. Verified against the live production and full npm trees: both still validate, still flatten to the same three advisories, and the real object/string via mixture in react-router-dom is still accepted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediates every reproducible dependency advisory and makes the
Dependency auditjob's green state mean something. Dependency pins, CI policy, and testsonly — no application code changed.
The problem
The
Dependency auditjob was green while a real advisory was present, and itnever looked at the frontend at all:
continue-on-error: truemeantpip-auditexiting 1 did not fail the job.npm audit, so three npmadvisories — including a high — were invisible while the job presented
itself as a general "Dependency audit".
permissions, so it inherited more than it needed.Advisories found (baseline at
9b34a17)pytestpostcssreact-routerreact-routerreact-router-domDependency paths:
Reachability:
/tmp/pytest-of-{user}handling allows local DoSor privilege escalation on shared UNIX runners. Does not ship to browsers.
Build-tool exposure is real, so it is fixed rather than waived.
vite buildin CI; does not ship to browsers.Arbitrary
.mapdisclosure viasourceMappingURLpath traversal.Remediation (smallest compatible upgrades)
requirements.txt:pytestpostcssnpm audit fix(no--force)nanoid(postcss dep)The lockfile diff moves exactly those two packages and nothing else.
package.jsonis unchanged,npm cistays deterministic, and no lockfile editwas made by hand.
npm audit fix --forcewas not used.After remediation:
pip-auditreports "No known vulnerabilities found"(exit 0) and the npm high advisory is gone.
The three advisories that remain, and why
react-router6.x has no fix. GitHub recordsfirst_patched_version: noneforreact-router-dom6.x on GHSA-jjmj-jmhj-qwj2; the other two are patched only in7.18.0. Removing them requires the react-router v7 major migration,
which is a framework migration rather than a dependency-security change — and it
would land directly on the dashboard routing that shipped in #43. That belongs
in its own PR with its own routing verification.
They are therefore accepted as narrow, owned, expiring exceptions in
.github/dependency-audit-policy.json, each backed by a control that exists:attacker-influenced navigation sink is
navigate(returnTo)inpages/OIDCCallback.tsx. Every producer ofreturnTopasses it throughsanitizeReturnToon both write and read, which requires the literal/dashboardprefix — so an authority-position payload (//host,/\host)can never reach the router. Pinned by
interlock-web/tests/returnToRedirect.test.ts(13 payloads).deserializeErrors()runs only during SSR hydration and this is a client-only Vite SPA with no
hydrateRoot/StaticRouter/renderToString/createStaticHandler.Pinned by
interlock-web/tests/noSsrHydration.test.ts, which fails the momentSSR is introduced — forcing the v7 upgrade instead of a stale waiver.
Every exception expires 2026-11-01, is owned by maaz@getinterlock.dev, and
names one exact GHSA id. Wildcards are rejected and
tests/test_dependency_audit_policy.pyfails on an expired, incomplete, unowned, duplicated, or stale entry, or one
whose cited control file does not exist.
CI enforcement
scripts/audit_npm.pyis a policy-aware gate.npm audit --audit-levelcannotexpress "block everything except these three, and only until they expire", and
the alternative — redirecting
npm auditto a file with|| true— is the sameexit-swallowing that caused this bug. The gate runs
npm audititself, so onecommand per scope both records JSON and decides pass/fail with no shell escape
hatch.
The job now:
npm ci;pip-auditblocking (nocontinue-on-error);npm audit --omit=dev) and build/developmentdeps separately, both blocking at
moderate;entry;
pip-audit.json+ both npm payloads as artifacts (if: always());permissions: contents: readand pins every action.Commands and results
npm ci(clean)npm audit --jsonnpm audit --omit=dev --jsonpip-audit(clean venv, before)pip-audit(after)npm testnpx tsc --noEmitnpm run buildindex.html42.46 kBpytest -q(full, on pytest 9.0.3)ruff/black --check(changed Python)actionlint(docker, pinned image)git diff --checkdist/,node_modules, audit JSON all ignored)Full suite is unchanged in shape on pytest 9.0.3 (1871 → 1929 passed; +58 are
the dependency-audit policy tests, same 134 skipped / 6 xfailed), so the major
test-runner bump is behaviour-preserving here.
Proof the fix is not cosmetic
Pre-fix, from
9b34a17: the audit job had exactly one audit step, it carriedcontinue-on-error: true, it ran no npm command, and it had no jobpermissions.pip-auditexited 1 on that tree — a reproduced advisorywith a green job.
Post-fix, against a controlled critical fixture the gate returns:
tests/test_dependency_audit_policy.py(58 tests) pins the full matrix below.Follow-up repair (this branch, commit
b71c448)An independent review found the first gate approved advisories too loosely and
could fail open. All four issues are fixed and each is pinned by a test.
1. Exceptions are now exact and fail closed. Matching was by GHSA id alone,
so the same id on a different package or escalated to critical was silently
accepted — both reproduced as exit 0 before the fix. An exception now applies
only when id, package, severity, scope,
installedVersionandaffectedRangeall still match the observed advisory; any drift blocks with a per-field diff
and demands re-review. Installed versions are parsed structurally from
package-lock.json, not taken from the audit payload.2. npm audit operational failures now fail closed.
{"error":{"code":"ENOAUDIT"}},{}, a nullvulnerabilities, a missing/unsupportedauditReportVersion, andmissing
metadataall previously read as a clean tree (exit 0 on thedevelopment scope). The gate now validates subprocess completion, stdout
presence, JSON parsability and full report structure, and exits 2 —
"an untrusted audit is not a clean tree" — while still writing evidence when the
payload is structurally usable.
3. The development gate no longer relabels production findings. It was
handed the full tree and called it "development", so the three production
react-router advisories were reported twice. Development is now full minus
production: 0 dev-only advisories.
4. Ecosystems run independently.
pip-auditwas blocking withoutif: always(), so a Python advisory skippednpm ciand the production gateentirely — no npm evidence at all. Every audit step now runs with
if: always()and records an outcome id, and a new Dependency audit verdictstep fails the job if any of them failed. No
continue-on-error,|| true,|| exit 0, orset +eanywhere.5. CVSS corrected by removal.
GHSA-wrjc-x8rr-h8h6carried"cvss": 0,which reads as "no impact" (the advisory is ~5.1). The gate never validated or
used any CVSS value, so the field is removed from all entries rather than
carrying an unvalidated score, and a test forbids reintroducing it.
Hostile-fixture matrix (58 tests, all passing)
Real CI on
b71c448shows production3 advisories, 0 blocking, 3 accepted, 0 stale — PASS, developmentno advisories in this scope — PASS, and theverdict step reporting
PIP_AUDIT success / NPM_PRODUCTION success / NPM_DEVELOPMENT success — all dependency audits passed.React Router is not migrated to v7 in this repair, as instructed; the
documented controls and the 2026-11-01 expiry are preserved, now behind an
exception gate proven exact.
Second follow-up repair (commit
973f4fe)A further read-only review found two more fail-open paths. Both were reproduced
against the previous head before being fixed.
B1 — production/full-tree differencing was unsafe. Development findings were
computed by subtracting production rows keyed on
(id, package)alone.Reproduced: a production row at moderate erased a full-tree row for the same
id/package at critical — the dev finding vanished entirely. Differencing now
compares the complete security identity (
id,package,severity,range,node_range):the row is retained, narrowed to just those dev-only paths, and evaluated;
(id, package)but different severity/range → the two reports disagreeabout the advisory itself, so the gate raises
AuditUnusablerather thanguessing which report is authoritative.
B1 — multiple installed versions were collapsed.
installed_versions()usedsetdefault, so a lockfile withnode_modules/postcss@8.5.25andnode_modules/some-tool/node_modules/postcss@7.0.1silently kept only the first.An exception could then be validated against a version that is not the
vulnerable one. Replaced by
installed_instances(), which keeps everylockfile path → version pair. Vulnerable
nodespaths are resolved to thespecific installed instance; when several distinct versions are implicated, one
recorded
installedVersioncannot describe them all and the exception isrefused.
B2 —
run_npm_auditignoredproc.returncode. Reproduced: npm exiting2 and 127 with valid-looking audit JSON on stdout was accepted as a
clean tree. Only
{0, 1}— the codes that mean "the audit completed" — are nowaccepted; every other code raises
AuditUnusableeven when stdout parses.The diagnostic carries the exit code plus a
sanitize_stderr()summary thatcaps output to 4 lines / 240 chars and replaces any credential-bearing line
wholesale, so tokens,
_authTokenentries, andAuthorizationheaders cannotreach a public CI log.
Added regression tests (+15, 26 → 41)
AuditUnusableAuditUnusableAll pre-existing ENOAUDIT, malformed-payload, exactness, expiry and staleness
tests remain green.
Third follow-up repair (commit
c186414)A further review found three more fail-open paths. All eight failure modes were
reproduced against the previous head before being fixed.
F1 — the superset invariant was never checked.
development_only()onlywalked the full tree, so it could find extra dev findings but could never notice
that a production finding had gone missing — which would mean the two npm runs
did not describe the same dependency graph. Reproduced: production reporting
GHSA-y (pkg-b, high)that the full tree omitted returned[]with no error;likewise a production vulnerable path absent from full. Differencing is now
bidirectional: every production identity and every production vulnerable
path must be present in the full tree, or the gate raises
AuditUnusable.Rows are also grouped by identity with their paths unioned, so a duplicate
identity can no longer overwrite an earlier row's paths. Full-only identities
and full-only paths still surface as development findings, and a security-field
disagreement still raises — now with the more specific "reports disagree"
diagnosis taking precedence over "missing".
F2 — partial node-path resolution was accepted.
implicated_versions()kept whichever
nodespaths happened to resolve and silently dropped the rest,so an advisory reporting a known and an unknown instance shrank to the known
one — potentially the very instance whose version matches an exception. A path
resolving to a different package was also accepted. Now:
nodesmust be alist of non-empty strings; when npm names instances, every one must resolve
to an instance of the expected package, or the gate fails closed. The
all-instances conservative fallback applies only when npm supplies no paths.
F3 — report metadata was never cross-checked.
{}withtotal: 1,a populated body with
total: 0, buckets not summing tototal, and negative,string, or boolean counts were all accepted. For
auditReportVersion 2the gatenow requires every severity bucket and
totalto be a non-negative integer(
boolis rejected explicitly, since it subclassesint),total == sum(buckets),and
total == len(vulnerabilities)— npm counts vulnerable package nodes, notflattened advisories. Each vulnerability node is validated for object shape,
severity,
vialist (which legitimately mixes advisory objects with plain-stringindirect edges),
nodeslist, andrangetype before flattening. Verifiedagainst the live production and full reports, which both satisfy
total == len(vulnerabilities) == sum(buckets) == 2.While fixing F2 I also found that an
AuditUnusableraised during exceptionmatching escaped the fail-closed boundary and surfaced as a traceback; the
evaluation is now inside that boundary and reports the standard
AUDIT UNUSABLE … RESULT: FAILwith exit 2.Added regression tests (+17, 41 → 58)
nodesvalues failviamixing objects and name strings is acceptedEvery earlier hostile regression (exactness, expiry, staleness, ENOAUDIT,
malformed payloads, exit-code validation, multi-version lockfiles) remains green.
Fourth follow-up repair (commit
4d4f935)A final review found the parser-to-decision path itself could still return a
clean verdict for a report that declares a vulnerability. Reproduced against
c186414before being fixed.The hole. The verdict is computed from advisory objects flattened out of
each node's
via. But a report declares vulnerabilities throughmetadataandper-node
severity— fields the flattening never reads. So a report naming onehigh-severity vulnerable package, with metadata that agrees with itself in
every way the F3 checks test (
total == len(vulnerabilities) == sum(buckets) == 1),and
via: [], validated cleanly, flattened to zero advisory rows, andprinted:
This repo's own three production exceptions happen to mask it — they would all
read as stale and exit 1 — but that is incidental cover, not a guarantee. A repo
with no exceptions, or any advisory nobody wrote an exception for, gets the PASS
above. The regression test uses an exception-free policy so it pins the real
verdict rather than the accident.
Post-fix, the same fixture returns exit 2:
G1 — advisory-graph completeness. npm states causation two ways inside
via: an advisory object is direct evidence, and a bare string names anothervulnerable package node that makes this one vulnerable. Every node must now
reach a real advisory object by walking that graph. Rejected: an empty
via; a dangling edge naming a package the report does not contain, so thereport is missing its own cause; and a cycle that never terminates in
evidence —
pkg-a → pkg-b → pkg-a, and self-reference — each with its owndiagnostic. The walk is cycle-safe and its visited set doubles as the
diagnostic. Preserved as valid, because this is what npm actually emits:
object/string mixtures on one node, multi-hop string chains that do terminate in
an advisory, and cycles that contain one.
G2 — advisory-object validation.
severity,range,nameand theurl/sourceidentity are each facts an exception is matched against, and amissing one silently became an empty string that was then compared as though it
were an observed fact. All are now required and well-formed:
severityin theknown buckets,
rangea non-empty string,namea non-empty string thatequals the package the advisory is listed under,
sourcea non-negativeinteger (
boolrejected explicitly),urla non-empty string when present, andat least one usable identity — without one, no exception could ever be matched
to the advisory.
G3 — exact severity-bucket comparison. F3 compared
sum(buckets)againsttotal; matching totals are not agreement. A report can hold exactly as manyvulnerable package nodes as
totalclaims, with the buckets summing to that sametotal, and still restate a critical finding as moderate — precisely the
drift the exception matcher exists to catch. Buckets are now compared against the
declared node severities one for one.
G4 — a closing invariant in the flattening. Independent of how a report
reached that point, a summary declaring vulnerable packages can never produce
zero advisory rows. Zero rows is the one result the gate cannot distinguish from
a clean tree, so it has to be earned. A genuinely empty report still flattens to
[]and still passes.G5 — no raw tracebacks. Policy loading and
--todayparsing sat outsidethe fail-closed boundary. An unreadable policy file, a malformed
expires, or anentry missing
id/compensatingControlescaped as a Python traceback and exit1 — the same exit code as "policy violation", so a broken policy file read as
a blocked build. Policy loading, per-entry validation and date parsing now run
inside the boundary and exit 2 with a clean diagnostic and no traceback.
Added regression tests (+21, 58 → 79)
via: []cannot pass end-to-endviais rejected at validationviaedge naming an absent package failsviacarries an advisoryviagraph without an advisory failsviawithout an advisory failsurlbut withsourceis accepted, id falls back--todayoverride fails closed without a tracebackEvery earlier hostile regression (exactness, expiry, staleness, ENOAUDIT,
malformed payloads, exit-code validation, multi-version lockfiles, superset
differencing, node-path resolution, metadata counts) remains green.
Verified against the live trees after the change: production still reports
3 advisories, 0 blocking, 3 accepted, 0 stale — PASS, development still reportsno advisories in this scope — PASS, and the real object/stringviamixture onreact-router-domis accepted unchanged. Full backend suite: 1950 passed,134 skipped, 6 xfailed.
ruff,black --check --target-version py312andmypyclean on both changed files.Limitations
risk is a react-router open redirect if
sanitizeReturnTois ever weakened —which is exactly what the pinned test prevents. Follow-up: react-router v7
migration before 2026-11-01.
pass locally and fail later on the same commit. That is intended.
affectedRangeandinstalledVersionare compared as exact strings, notparsed semver. An upstream range rewritten to an equivalent form blocks and
needs a one-line policy refresh — deliberately fail-closed.
auditReportVersion 2semantics(
totalcounts vulnerable package nodes). A future report version failsclosed until reviewed.
{0, 1}; a future npm that adds athird "completed" code fails closed until reviewed.
npm auditoutput only; it does not do its ownreachability analysis. Reachability claims here are manual and recorded in the
policy file.
npm auditadvisory counts are per-package inmetadatabut per-advisory inthe gate output (2 packages / 3 advisories). Both are reported.
requirements.txt; it does not pin transitive wheelswith hashes, so a hash-pinning/SBOM step is still future work.
created by the repository's branch-push integration;
mainand the productionbackend remain unchanged.
viastring edge must name a node present in the same report. Thatholds in both live trees (
react-router-dom -> react-routerresolves inthe production tree as well as the full one) and follows npm's own
semantics: the cause must be in the tree that contains the effect. If some
npm edge case ever emits an out-of-scope edge, the gate fails closed
(exit 2), not open.
namemust equal the package node it is listed under,which is what npm emits today. A future report format that hangs an advisory
under a different package fails closed until reviewed.