Simplify the security gate now that the scanner is accurate - #20
Merged
Merged
Conversation
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.
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
--fail-onwould have blocked every PR. That turned out to be a rule problem rather than anything in this repo, and it is fixed upstream in ThreatCrush 0.4.0 (threatcrush#76, published to npm).This repo now has zero high-severity findings, so the gate is just:
.github/threatcrush-baseline.jsonand.github/threatcrush-gate.pyare gone — about 200 lines removed. It still fails closed: a scan that produces no findings file is reported as not scanned, not as clean.What the simpler gate gives up
Worth 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:
innerHTML = '<b>' + new URL(location).searchParams.get('q') + '</b>'innerHTML = '<b>' + q + '</b>', whereqis a parameterSo 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.
--fail-on mediumwould close the gap and today costs 31 false positives — allinnerHTMLsinks in multi-line templates whose interpolations are escaped, just on a different line than the assignment, which a line-oriented scanner cannot see. Closing those properly needs multi-line template awareness in the scanner, which is a reasonable next upstream change.Both the README and the workflow now say this is a floor, not a proof.
Verification
tsc --noEmitclean