fix(health): the credential rule read the variable name, not the value #5613
ccantynzAI Reviewcommented 1h ago
AI Triage
(no summary)
Priority: medium Risk area: mixed
Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)
Suggestions only — nothing has been applied. The PR author stays in control.
gluecron[bot]🤖 botAI Reviewcommented 1h ago
AI review unavailable
The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.
Cross-repo impact
See what breaks downstream if this PR merges.
⮌ Merged
This pull request was merged into main.
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts
The Vapron instance opened all nine
criticalfindings this scanner reported against its repository and checked each against running production. Five were false. Every one failed the same way.The rule read the name, not the value
It matched a line because the variable was called
..._TOKEN_URLor..._API_KEY, then judged the literal only by whether it looked "unlike a lowercase identifier" — which a URL, an angle-bracket placeholder and a docs snippet all satisfy.criticalCELITECH_TOKEN_URL ?? "https://api.celitech.com/oauth2/token"vapron env set VAPRON_API_KEY="<your-api-key>"<pre>vpk_YOUR_API_KEY_HERE</pre>The first is the clearest: it matched only because of the variable's name. A rule whose top severity is routinely wrong teaches people to skim the whole list — which is what the innerHTML rule already cost us this week.
Three structural rejections, pure syntax, no dataflow: a
scheme://URL, an<angle-bracket>placeholder, andYOUR_..._HERE. Deliberately does not rejectchangeme/change-me/dev-secret— those are the classic dangerous defaults and the whole point of the rule. There's a test for that over-correction.The four TRUE findings are pinned with the real source lines
They're why this stays narrow. All four are
?? "<dev literal>"on a security-critical value, which fails OPEN. None is live — env vars are set, or the service is inactive — but each is one missing.envline from a service booting with a secret committed to the repo.Findings list capped per rule
199 findings, 162 of them one medium rule, the nine criticals somewhere in the middle. A rule firing 162 times is telling the reader one thing, not 162 things.
issuesis capped per rule and ordered by severity so a cap can never bury a critical.findings.byRule/bySeverityare never truncated, andscoreis still computed over every issue — the cap is presentational and must not move the verdict. There's a direction test that would fail if scoring ever moved onto the capped list.⚠️ Fallback scan restored — a bug I introduced two commits ago
Adding the git-grep path deleted the fallback's per-file loop, leaving it to slice the file list and return without reading any of it: zero findings, score 100,
method: "capped".A clean bill of health from never looking — in the module that exists to prevent exactly that, introduced by the change that fixed it everywhere else. Latent (the grep path answers on every real repo) and wrong regardless. Pinned by a test asserting both paths call the detector, because what broke was code being deleted, not logic altered.
manifestsScannednow has the test it should have hadI fixed it to count completions rather than attempts and shipped it untested — precisely how it got through the first time.
parseManifestDepsis split out so the contract can be asserted:null, not countednull, not countedThe measured 80-found / 76-scanned gap (four
Cargo.toml, no parser) is the fixture.Direction tests
Scoring tests now assert the direction of each term rather than that the output is well-formed — after the GateTest session found its own badge scorer had every input inverted against its name while passing any test that checked for "a number in 0..100". Positive control: negating a term fails them.
Verification