CodeIssuesPull RequestsActionsSecurityInsights
✨ AI
More
Settings

fix(health): the credential rule read the variable name, not the value #5613

MergedXSccantynz wants to mergefix/credential-value-precisionmainopened 1h ago
ccantynzcommented 1h ago

The Vapron instance opened all nine critical findings 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_URL or ..._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.

flagged critical what the literal actually is
CELITECH_TOKEN_URL ?? "https://api.celitech.com/oauth2/token" a URL
vapron env set VAPRON_API_KEY="<your-api-key>" a placeholder in a docs guide
<pre>vpk_YOUR_API_KEY_HERE</pre> the snippet we show the customer

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, and YOUR_..._HERE. Deliberately does not reject changeme / 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 .env line 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.

issues is capped per rule and ordered by severity so a cap can never bury a critical. findings.byRule / bySeverity are never truncated, and score is 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.

manifestsScanned now has the test it should have had

I fixed it to count completions rather than attempts and shipped it untested — precisely how it got through the first time. parseManifestDeps is split out so the contract can be asserted:

  • unparseable → null, not counted
  • a manifest kind with no parser → null, not counted
  • a manifest with genuinely zero dependencies → empty-but-present, which is a different and real answer

The 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

  • 28 new tests (10 credential values, 12 findings summary, 6 manifest honesty).
  • Full CI-safe suite: 5,343 pass, 11 fail — identical to the pre-change baseline.
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.

Analyze →
⮌ Merged

This pull request was merged into main.

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts