Repository navigation
ci: the Security Scan check cannot fail on a security finding — every audit step ends in || true #13400
Description
Activity
- added a commit that references this issue
on Aug 4, 2026 Delivered and verified in the merged tree — closing.
Closes #13400did not fire because the PR merged toDev_new_gui, which is not the repository default branch.Evidence —
.github/workflows/frontend-test.yml:307-308, onDev_new_gui:# THE GATE — deliberately not suffixed with `|| true`. npm audit --audit-level=highThat is the only audit invocation in the file without
|| true, so a high-severity advisory now fails the job instead of being summarised and discarded. Theauditjsstep below stays non-blocking on purpose — it queries a third-party service whose availability is outside this repo's control, and its data duplicates the npm advisory feed the gate already reads. That decision is recorded inline rather than left implicit.Scope note — the rest of the audit surface is still ungated, and is tracked, not dropped. This issue covered the frontend gate only.
security.ymlstill wraps every scanner in|| true:line step 109-110 pip-audit115-116 safety check139-140 npm audit --audit-level=moderate186-187 bandit211 semgrep 224-225 flake8 That is #13543. Separately,
Security Scanis not in the required-context list forDev_new_gui, so even a failing run would not block a merge — structurally the same gap as #13162. Report-only alternative for this gate: #13542.Downstream of this fix: #17841
Linking because #17841 only exists because this issue was fixed. The
Dependency Security Scangate can now fail on a real finding instead of ending in|| true, and it is currently failing on three high frontend advisories inorigin/main's lockfile —axios1.0.0–1.19.0,brace-expansion4.0.0–5.0.11,undici8.0.0–8.10.1.Recording it here for two reasons:
- It is evidence this fix works as intended. The gate is catching base advisories and refusing to pass, which is exactly the behaviour this issue was filed to obtain.
- It is where to look if anyone proposes silencing the gate to unblock the queue. The gate's own inline comments record it having failed once on a package-keyed
--allow-id, because such an allowance admits whatever advisory appears in that package next. security(frontend): three high npm advisories fail the audit gate on every PR — and the known remedy predates axios #17841 carries that as an acceptance criterion: per-advisory allowances only, and only with evidence that no reachable fix exists.
See also #15467, the same gate failing on a different package set (
browserslist).- added a commit that references this issue
on Oct 3, 2026 Recording a deliberate yield, as this gate's workflow comment instructs
frontend-test.ymlsays above the audit step: "if an advisory ever has no published fix and the gate has to yield, change it deliberately and record why on #13400 — do not silence it as a flake." This is that record. PR #17889; the live follow-up for the expiry is #17890, because a date inside a closed issue's thread is not a home.The advisory, from the advisory API rather than the audit summary — the summary's "6 high" reads as six problems and its patched column's
*reads as a range, and neither is what it means:GHSA-vfj7-8cjw-p6xm braces — stack-exhaustion DoS through deeply nested patterns vulnerable_version_range <= 3.0.3 first_patched_version null <- there is no version to bump to published 2026-09-18Every lockfile in the repo already carries braces 3.0.3. The six reported packages are one root cause with five transitive dependents (micromatch, fast-glob, globby, stylelint,
@vue/eslint-config-typescript), and the chain isdev: truethroughout — tooling, not shipped code. The gate went red with no commit, which is how an unchanged lockfile behaves the day an advisory is published against it.What the gate could not do before.
npm_audit_gate.pytook--npmand--reportand failed on any high or critical. With no patched version in existence that blocks every frontend-touching PR indefinitely — two weeks at the time of writing — and the only options available were to fail forever or to silence, which this issue exists to forbid.What it does now, and why it is not the silence this issue was filed about.
ADVISORY_EXCEPTIONSis keyed by GHSA id and carries a justification and an expiry. Four conditions, each failing loudly:condition verdict the entry has expired FOUND, naming the date npm reports a fix available for any failing package FOUND — the exception is not honoured while a bump exists the entry is no longer reported FOUND — remove it, dead policy an advisory is not in the record FOUND, named A pass through an exception carries the advisory id and this issue number in the verdict, the job summary and the annotation, and the high count is still reported — excused, not hidden. Unreadable per-package detail stays FOUND, because a report that could not be read must not become a pass.
Scope of the yield: one advisory, dev-only, expiring 2026-11-14, owner decision 2026-10-03. Not a severity threshold change, not
--omit=dev, and no dependency or lockfile moved — the two files in the PR are the gate and its test.Mutation-verified one condition at a time, each failing exactly its own test: removing the fix-available withdrawal, the expiry check, the drained-entry check, or the restriction to recorded ids each breaks one test and nothing else. 52 passed restored.
- added 8 commits that reference this issue
on Oct 3, 2026 Owner decision, 2026-10-03 — recorded here so it is citable
This was given as a direct answer in a working session rather than as a comment, which made it
real but unquotable: nobody outside that session could cite it, and it would have been lost when
the session ended. Writing it down is the point of this comment.The question.
GHSA-vfj7-8cjw-p6xm(braces, stack-exhaustion DoS) reports
first_patched_version: null— no non-vulnerable version of the package exists — and the chain
reaching it is devDependencies only. The npm audit gate had no way to express that, so base was
blocked by an advisory with nothing to upgrade to.The ruling. An expiring recorded exception, with three conditions:
- keyed by GHSA id — never by package name, never by severity
- requires a written justification and an explicit expiry date
- accepted only for an advisory with no patched version; an advisory that has one is not
eligible, whatever else is true of it
Condition 3 is the guard the owner asked for specifically. The exception is a statement that
there is nothing to upgrade to, so it must be refused the moment that stops being true.What condition 3 turned out to require.
npm audit --jsonreportsfixAvailableper
package entry, not per advisory. Six entries reach this one advisory throughviachains, so
readingfixAvailableas a property of the advisory meant any one entry offering a fix withdrew
the exception for all of them — and an unrelated fixable advisory could revoke a valid ruling.
Patchability is now computed per advisory id, from only the entries naming that id.Three npm shapes look like a fix and are not one:
- a
fixAvailablenaming a different package — that is a parent bump, not a patch for this one range: "*"— npm's own statement that no safe version of the package exists- an offered version below the vulnerable range's lower bound — that is a rollback
All three apply here at once:
braceshasrange: "*", and itsfixAvailableoffers
stylelint 7.7.0against a declared^17.15.0— ten majors back, for a devDependency.A wording tension, resolved toward intent. An earlier description of the mechanism said the
gate "withdraws it the moment npm reportsfixAvailable". Per-package attribution means it does
not withdraw on a parent-bumpfixAvailable. That follows the question the gate is actually
asking — has this package published a fix? — and departs from the earlier mechanism sentence.
Intent wins; the earlier sentence was written before the per-package distinction was understood,
and this is not a reversal of it.Attribution failing is a failure, never a default pass. A report naming no advisory id, and
an entry whoseviachain reaches none, both fail with the packages named — so a registry schema
change or a parse failure cannot silently license an exception.Expiry is 2026-11-14, enforced by the gate rather than by the test suite, so a date passing
does not break every unrelated PR. The durable question — scoping the gate by dependency type —
stays open as #17890. The rollback itself is not an input to any of this: the gate declines a
downgrade as a fix whether or not the project ever takes one.- added 4 commits that reference this issue
on Oct 3, 2026
Problem
The
Security Scanjob infrontend-test.ymlcannot fail on a security finding. Every scanning step swallows its exit code:The only steps that can fail the job are
checkout,setup-node,npm ci, and the artifact upload. So a redSecurity Scanalways means infrastructure, and a green one means nothing about vulnerabilities —npm audit --audit-level=highcan report a high-severity advisory and the job still passes.Why this matters
The check is named
Security Scanand reads, to anyone scanning a PR's checks, as "no known vulnerabilities". It cannot support that reading. A control that always passes is worse than an absent one, because it displaces the attention an absent control would attract.Found while deciding whether to merge #13392: that PR is Python-only, yet
Security Scanwas red. Diagnosing it meant discovering the job structurally cannot fail on findings — which is what made the red safe to attribute to the offline self-hosted runner rather than to the change.Also
audit-results.jsonis uploaded as an artifact with 30-day retention, so the data is collected and then discarded — nothing reads it, and nothing gates on it.Proposal
Decide what the check is for, then make it say that:
|| truefromnpm audit --audit-level=highso a high-severity advisory fails the job. Expect an initial backlog; triage it once, then the gate holds.Dependency Audit (report only)), and have it surface the findings count in the job summary so the artifact is not the only record.Either is defensible. The present state — gate-sounding name, report-only behaviour, discarded output — is not.
Acceptance criteria
Security Scaneither fails on high-severity findings, or is renamed to reflect that it does not|| trueis not re-added as a "fix" for a red buildProvenance
Found while merging the #13162 cluster work. Related: #13392.