Threatcrush triage - #21
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>
The reviewed-baseline machinery existed for one reason: the scanner reported 56 findings here and all 56 were false positives, six of them high-severity, so `--fail-on` would have blocked every pull request and been switched off. That was a rule problem, not a repository problem, and it is fixed upstream in ThreatCrush 0.4.0 (profullstack/threatcrush#76) — static innerHTML assignments, the escaper guard not knowing `esc()`, `searchParams.set` counted as untrusted input, and test fixtures read as live credentials. This repository now has zero high-severity findings, so `threatcrush scan . --fail-on high` does the job and ~200 lines of baseline and gate script go away. It still fails closed: a scan producing no findings file is reported as NOT scanned rather than as clean. What it gives up is written down rather than glossed. 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 — measured both ways against this repository: innerHTML = '<b>' + searchParams.get('q') + '</b>' high, blocks innerHTML = '<b>' + q + '</b>' // q is a parameter medium, does not So the gate stops a vulnerability written in one place and misses one whose source sits in another function. `--fail-on medium` would close that gap and today costs 31 false positives. The README and the workflow both say so: this is a floor, not a proof, and not a substitute for review. 490 tests pass, tsc clean, and the gate verified locally against the published 0.4.0 — passes on this tree, exits 1 on an injected XSS with a visible source.
ThreatCrush Security Scan33 finding(s) MEDIUM: 31 | LOW: 2
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
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.
No description provided.