Fix the scanner's false positives upstream, and gate on high-severity findings - #19
Merged
Merged
Conversation
Two things: establish whether the 56 findings are real, and make a genuine
vulnerability stop the merge.
TRIAGE. All 56 are false positives. Verified individually rather than assumed:
- js-unescaped-html-sink (44). Extracted all 59 interpolations reaching an
HTML sink across public/*.js and classified them; the 17 that could not be
proven safe mechanically were read by hand. Every one is escaped through
esc()/aEsc(), is a number formatted by toFixed/toLocaleString, or comes
from a hardcoded constant. The HIGH one is a static string with no
interpolation at all. Credit values are unescaped but the server coerces
them with Number() and the packages are a literal array.
- js-ssrf-outbound-request (3). URLs are a constant base plus a code-literal
path; caller input reaches only URLSearchParams values, which are encoded
and cannot alter scheme, host or port.
- secret-* (2). Test fixtures. The auth one is a deliberately real-looking
fake whose test exists to prove a real-looking key cannot send under
NODE_ENV=test.
- sql-template-interpolation (4). Hardcoded table names, the static
ADDED_COLUMNS map, and '?' placeholders with bound arguments.
- redos-nested-quantifier (1). Measured, not argued: the quantifiers are
disjoint on their first character, and 60k-char adversarial input matches
in 2.6ms with linear growth.
- js-dynamic-code-execution (1). The jsdom harness eval'ing our own files.
- insecure-temp-file (1). A string literal in a fixture.
GATE. ThreatCrush 0.3.0 has no ignore file and no inline suppression; the only
control is --fail-on <severity>. With every current finding a false positive,
enabling it would block every PR and be switched off within a day. So the gate
is "no NEW findings": the triaged 56 are recorded in threatcrush-baseline.json
with justifications, and anything not listed fails.
Verified by injecting a real XSS (unescaped query text into innerHTML) and
confirming the gate fails with exit 1 naming that line, then removing it and
confirming green.
Keyed by rule + file + hash of the offending line rather than line number, so
edits above a finding do not spuriously fail; per-file counts are checked too,
so a second identical-looking sink is still caught. Findings located in the
baseline file itself are ignored — it quotes source lines so reviewers can see
what they are signing off, and those quotes are otherwise scanned as code.
Lives in a new workflow rather than in threatcrush-scan.yml, which is managed
by the sh1pt Actions Fleet and carries a content hash that a pack update would
restore over any local edit.
No product code changed: 490 tests pass, tsc clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan64 finding(s) HIGH/CRITICAL: 6 | MEDIUM: 58
…and 14 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
Contributor
Author
|
Simplified, now that the scanner itself is fixed. The baseline machinery existed for one reason: 56 findings here, all 56 false positives, six of them high — so This repo now has zero high-severity findings, so the gate is just:
What the simpler gate gives upWorth being explicit, because it is a real reduction in coverage. Severity depends on whether the scanner can see the taint source near the sink, so the same XSS is graded differently depending on how the code is arranged. Both measured against this tree:
So it stops a vulnerability written in one place and misses one whose source sits in another function. The old baseline gate would have caught both, because it failed on any new finding at any severity.
Both the README and the workflow now say this is a floor, not a proof. Verification
|
ralyodio
changed the title
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.
You were right that these are false positives — all 56 of them. I verified each one individually rather than assuming, and separately made a real vulnerability stop the merge.
Triage
js-unescaped-html-sinkpublic/*.jsand classified them; the 17 not provably safe mechanically were read by hand. Every one is escaped viaesc()/aEsc(), is a number throughtoFixed/toLocaleString, or comes from a hardcoded constant. The HIGH one (auth.js:189) is a static string — no interpolation at all.js-ssrf-outbound-requestURLSearchParamsvalues, which are percent-encoded and cannot alter scheme, host or port.secret-*NODE_ENV=test.sql-template-interpolationADDED_COLUMNSmigration map, and?placeholders with bound args. One points at aconsole.log.redos-nested-quantifier.), so there is nothing to backtrack through — 60,000-char adversarial input matches in 2.6ms, growing linearly.js-dynamic-code-executioninsecure-temp-file"/tmp/x"string literal in a fixture; nothing opens it.One honest note: a few credit values in
auth.jsare interpolated unescaped. They are safe because the server coerces them withNumber()and the packages are a literal array — so not a vulnerability, but it is safety held by an invariant elsewhere rather than at the sink. Happy to harden it if you want; it would not change the scanner output either way.The gate
ThreatCrush 0.3.0 has no ignore file and no inline suppression — the only control is
--fail-on <severity>. I checked the CLI and the package. With every current finding a false positive,--fail-on highwould block every PR and get switched off within a day.So the gate is "no new findings". The triaged 56 live in
.github/threatcrush-baseline.json, each with a written justification; anything not listed fails CI. A real vulnerability stops the merge, the known-clean ones do not.Verified by injecting one. I added a genuine XSS (unescaped query text into
innerHTML), and the gate failed:Removed it, green again. It also fails closed: no findings file means "NOT scanned", not "clean".
Design details worth reviewing:
threatcrush-scan.yml, which is managed by the sh1pt Actions Fleet and carries a content hash that a pack update would restore over any local edit.No product code changed: 490 tests pass,
tscclean.🤖 Generated with Claude Code