scan: cut false positives without losing coverage (0.4.0) - #76
Merged
Conversation
Measured against a real repository the scanner reported 56 findings, all 56 of
which were false positives, including every one of its 6 high-severity ones.
That is not a tuning problem, it is a fatal one: a gate that cannot be enabled
without blocking every pull request gets switched off, and `--fail-on` was
unusable anywhere.
Four changes, each aimed at a class the triage showed was systematically wrong.
STATIC HTML SINKS. `innerHTML = '<div class="spinner"></div>'` was reported as
XSS. An assignment of a string with no interpolation and no concatenation
carries no data, so it cannot carry attacker data. A UI built with innerHTML
reported every static heading and spinner it had.
This needed a new `lineGuard`, checked against the matched line only. The
existing `guard` also searches the context window, which is right for "the
value was sanitised three lines up" but wrong here: a static assignment says
nothing about a dynamic one two lines below, and a context-scoped guard would
have vetoed the dynamic one too. There is a test for exactly that.
ESCAPER ALIASES. The guard matched `escapeHtml(` and `sanitize(` but not
`esc(`, which is what the escaper is actually called in code that escapes every
interpolation. The codebases escaping most rigorously were therefore the ones
reported most often. Now matches short aliases, with the identifier required to
END at the escaper (plus a known output-context suffix). An earlier, looser
form also matched `describe(` — which would have silenced every finding inside
every test file in every repository. Also tested.
SEARCHPARAMS IS A READ AND A WRITE API. `searchParams` counted as untrusted
input, so `url.searchParams.set('limit', 50)` — building an outbound URL —
marked every client of every third-party API as taking attacker input. That is
what fired the SSRF rule on requests whose host is a compile-time constant.
Only the reading half (`get`/`getAll`/`entries`/…) is evidence now.
CREDENTIALS IN TESTS. A fixture is not a leak, and one of the flagged ones
existed precisely to prove a real-looking key still cannot send. These are
downgraded to `low` in test paths rather than dropped: "nearly always a
fixture" is not "always", a genuine key does get pasted into a test, and
hiding it would be worse than the noise. Still reported, no longer blocking.
Result on that repository: 56 findings -> 33, and 6 high-severity -> 0, so
`--fail-on high` is now a usable gate. Verified in the other direction too,
against a file of deliberate vulnerabilities: reflected and interpolated XSS,
SSRF from `req.query` and from `searchParams.get`, SQL injection by
concatenation and by interpolation, `eval` of a request body, and a live
Stripe key are all still reported at high or critical. Nothing was traded away.
62 scan tests pass.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| it('stays silent on an assignment with no interpolation', () => { | ||
| // A UI built with innerHTML reports every static heading and spinner. That | ||
| // was the largest single source of noise in the corpus. | ||
| expect(ruleIds('a.js', `el.innerHTML = '<div class="spinner"></div>';`)).toHaveLength(0); |
| // A UI built with innerHTML reports every static heading and spinner. That | ||
| // was the largest single source of noise in the corpus. | ||
| expect(ruleIds('a.js', `el.innerHTML = '<div class="spinner"></div>';`)).toHaveLength(0); | ||
| expect(ruleIds('a.js', 'el.innerHTML = `<h2>Verifying your email…</h2>`;')).toHaveLength(0); |
| // was the largest single source of noise in the corpus. | ||
| expect(ruleIds('a.js', `el.innerHTML = '<div class="spinner"></div>';`)).toHaveLength(0); | ||
| expect(ruleIds('a.js', 'el.innerHTML = `<h2>Verifying your email…</h2>`;')).toHaveLength(0); | ||
| expect(ruleIds('a.js', 'body.innerHTML = "<p>done</p>"')).toHaveLength(0); |
| }); | ||
|
|
||
| it('still flags interpolation and concatenation', () => { | ||
| expect(ruleIds('a.js', 'el.innerHTML = `<b>Results for ${q}</b>`;')).toContain( |
| expect(ruleIds('a.js', 'el.innerHTML = `<b>Results for ${q}</b>`;')).toContain( | ||
| 'js-unescaped-html-sink', | ||
| ); | ||
| expect(ruleIds('a.js', 'el.innerHTML = "<b>Results for " + q + "</b>";')).toContain( |
| const source = [ | ||
| "const url = new URL('https://api.example.com/v1/bars');", | ||
| "url.searchParams.set('symbols', symbols.join(','));", | ||
| 'const res = await fetch(url, { headers });', |
| it('still flags a request whose URL comes from the caller', () => { | ||
| const source = [ | ||
| 'const target = req.query.url;', | ||
| 'const res = await fetch(target);', |
| it('treats reading searchParams as untrusted input', () => { | ||
| const source = [ | ||
| 'const target = new URL(req.url).searchParams.get("next");', | ||
| 'const res = await fetch(target);', |
| it('does not treat describe() as an escaper', () => { | ||
| // A looser form of the alias pattern matched `describe(`, which would have | ||
| // silenced every finding inside every test file in every repository. | ||
| const source = ['describe("thing", () => {', ' el.innerHTML = `<b>${name}</b>`;'].join('\n'); |
| }); | ||
|
|
||
| it('does not treat an arbitrary identifier ending in -esc- as one', () => { | ||
| expect(ruleIds('a.js', 'el.innerHTML = `<b>${rescale(name)}</b>`;')).toContain( |
ThreatCrush Security Scan159 finding(s) HIGH/CRITICAL: 52 | MEDIUM: 107
…and 109 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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
2 participants
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.
Scanning a real repository produced 56 findings, all 56 false positives — including every one of its 6 high-severity ones. That makes
--fail-onunusable anywhere: a gate that blocks every pull request gets switched off within a day.Result: 56 findings → 33, and 6 high → 0. No true positives were traded away (verified below).
Four changes
1. Static HTML sinks.
innerHTML = '<div class="spinner"></div>'was reported as XSS. An assignment of a string with no interpolation and no concatenation carries no data, so it cannot carry attacker data. A UI built withinnerHTMLreported every static heading and spinner it had.This needed a new
lineGuard, checked against the matched line only. The existingguardalso searches the context window — right for "sanitised three lines up", wrong here, because a static assignment says nothing about a dynamic one two lines below and a context-scoped guard would veto the dynamic one too. There is a test for exactly that.2. Escaper aliases. The guard matched
escapeHtml(andsanitize(but notesc(— which is what the escaper is actually called in code that escapes every interpolation. So the codebases escaping most rigorously were the ones reported most often.Now matches short aliases, with the identifier required to end at the escaper (plus a known output-context suffix). A looser form I tried first also matched
describe(— which would have silenced every finding inside every test file in every repo. Also tested.3.
searchParamsis a read and a write API. It counted as untrusted input, sourl.searchParams.set(...)— building an outbound URL — marked every client of every third-party API as taking attacker input. That is what fired the SSRF rule on requests whose host is a compile-time constant. Only the reading half (get/getAll/entries/…) is evidence now.4. Credentials in tests →
low, not dropped. A fixture is not a leak, and one flagged case existed precisely to prove a real-looking key cannot send. But "nearly always a fixture" is not "always" — a genuine key does get pasted into a test, and hiding it would be worse than the noise. Still reported, no longer blocking.Coverage is intact
Verified in the other direction against a file of deliberate vulnerabilities. All still reported at high or critical:
+ req.query.qinto innerHTML)${req.query.name})req.query.urlsearchParams.get(...)eval(req.body.code)62 scan tests pass.
Note
GitHub push protection rejected my first commit because a test fixture used a well-formed
sk_live_string. Correctly — which is a reasonable endorsement of the rule this file tests. The fixture is vendor-less now.Releasing
npm-publish.ymlfires on av*tag and publishesapps/cli; the version is bumped to 0.4.0 here. Merging does not publish — tagging does. I have not tagged.🤖 Generated with Claude Code