feat(cli): emit SARIF natively, and detect the code-level vulnerability classes - #73
Merged
Merged
Conversation
Scored against profullstack/malware-test-prs, the CLI managed 15.6%
true-positive rate at a 0.0% false-positive rate: it found every
hardcoded credential and none of the code-level classes. No SQL
injection, XSS, SSRF, command injection, deserialisation or XXE.
ThreatCrush was a secrets scanner wearing a code scanner's name.
True positive rate 15.6% (12/77) -> 90.32% (84/93)
False positive rate 0.0% -> 0.0% (0/46)
Unattributed 0 -> 0
The false-positive number is the one that was worth keeping. That
denominator is a control group of SAFE: lines, each a *correct*
implementation of the pattern the vulnerable code beside it gets wrong,
so every rule here is built against both halves — it has to fire on the
vulnerable shape and stay silent on the corrected one. Three mechanisms
do that: match on shape rather than keyword (a bound `$1` query and a
concatenated one both contain SELECT); guard windows, where an
allow-list or a realpath nearby exonerates the construct; and a
confidence model where a bare pattern match is capped at medium and only
visible untrusted input escalates it.
Two of those guards were wrong in ways only the corpus surfaced. A
comment reading "no allow-list validation" was being read as an
allow-list, and `def sanitize_path_vulnerable(path)` as a sanitiser —
both suppressed real findings. Comments and definition lines are now
excluded from guard windows, and Python docstrings from scanning
entirely, since a file whose header describes its own vulnerability
should not produce findings about that description.
All nine remaining misses are the four classes recorded in KNOWN_GAPS —
CSRF, TOCTOU, integer overflow, and generic dynamic-assignment prototype
pollution. Each needs whole-function reasoning, and each line-oriented
approximation flags ordinary software. A missing detection is a known
number; a rule that fires on every session read is a scanner nobody runs
twice.
Also adds the CLI surface the testbed had to work around. `scan` took a
path and nothing else, so the integration parsed terminal output into
SARIF and hit three bugs doing it — one of which (paths relative to the
scan root) made a working scan read as 0% coverage. Native SARIF removes
that pipe: `--format text|json|sarif`, `--output`, `--fail-on`,
`--path-prefix`, `--deps`, `--verbose`. URIs resolve against the working
directory, startLine is clamped to >= 1, and non-text formats keep stdout
clean by routing human output to stderr.
Two bugs found while testing: `threatcrush scan file.js` reported a
clean scan of a file it never opened, because the walker only handled
directories; and unreadable paths were silently dropped, so a tree the
scanner could not read reported as a tree with no findings. Both now
report what actually happened.
Adds inline suppression (threatcrush-disable-next-line), matching the
convention modules/code-scanner already uses, because the
highest-volume false positive in practice is a scanner's own test
fixtures. Suppressions are counted and reported.
49 tests, and docs/SCANNING.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This was referenced Aug 3, 2026
ralyodio
marked this pull request as ready for review
August 3, 2026 10:23
Two CI failures on this branch. `pnpm install --frozen-lockfile` rejected the tree: adding vitest to apps/cli/package.json without regenerating the root pnpm-lock.yaml left the two out of sync. Regenerating also drops a stale modules/dep-scanner entry for a package that no longer exists. CodeQL flagged a file-system race in the walker, correctly. Calling statSync(path) for the size check and then readFileSync(path) to read is check-then-use: the path can be replaced between the two calls, so the size that was checked is not the size that gets read. Open once and fstat the descriptor instead — it refers to the same inode for both operations whatever happens to the name. Worth fixing rather than dismissing. A scanner spends its life walking directories it does not control, and CWE-362 is a class this tool reports on; the walker should not be an example of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio
added a commit
to profullstack/sh1pt
that referenced
this pull request
Aug 3, 2026
Adds a `threatcrush-scan` action pack: runs [ThreatCrush](https://threatcrush.com) over pull requests and uploads SARIF to the Security tab. This is the unit the fleet installs for the org-wide rollout. Ships two files: the workflow, and `.github/scripts/threatcrush-to-sarif.py` — a fail-closed converter for CLI versions older than native `--format`. ## Field-tested on a real repo, and it caught me out twice I installed this on `moshcoder/moshpit-name` before rolling to 216 more. Four runs, in order: **1 — green, and wrong.** Reported `0 findings`, having scanned nothing. The runner had `0.2.2`, which has no `--format`; the scan died with `error: unknown option '--format'` and commander exited `1` — *the same code the CLI uses for findings at or above `failOn`*. Read as a result, no SARIF written, empty-run fallback supplied one, repo declared clean. **2 — red, comment still lied.** A capability check fixed the job status, but the comment *still* said "0 findings": it tested `status == "error"`, and a **skipped** step yields the empty string. Fail-open in the one branch written to prevent exactly this. **3 — red and honest**, but it refused to scan anything until a new CLI shipped. Correct and useless. **4 — green, genuinely scanned.** Detect the interface up front, convert legacy output when `--format` is absent. ## Design | CLI | Path | | --- | --- | | Has `--format` | Native SARIF. Nothing is parsed. | | Older | Text scan → `.github/scripts/threatcrush-to-sarif.py` | The check is up front because **exit codes cannot separate "argument rejected" from "findings found"** — that ambiguity is what produced run 1. The converter **fails closed**: unrecognised output exits non-zero and writes nothing. Emitting empty SARIF instead reports "0 findings", indistinguishable from a clean scan. It is written against the testbed's real captured output, because three details each break a naive parser: severity is bare for `CRITICAL` and bracketed for `[HIGH]`/`[MEDIUM]`/`[LOW]`; paths are relative to the scan root, so they resolve to nothing unprefixed; whole-file findings report line `0`, which SARIF rejects. Beyond that: **the SARIF file is the evidence** (missing or empty ⇒ error, whatever the process returned); **the report is fail-closed** (findings render only on `clean`/`findings`, every other state is NOT RUN); and **`failOn` actually gates** — it was recording findings and letting the step pass, so the gate would never have failed a PR. Other decisions: **no secrets** (installs fleet-wide without provisioning); **report-only by default** (a gate that fires on every pre-existing finding gets switched off within a day); **not `pull_request_target`** (runs with repo secrets in scope against untrusted contributor code — asserted in a test); **Node 20** for `better-sqlite3` prebuilts. ## Verification - **Four live runs on a real repository**, which is what found both bugs. - Converter against the testbed's real fixture: 9/9 findings parsed, both severity shapes, `:0` clamped, prefix applied. Scored through the testbed's own validator at **12.9% TPR / 0.0% FPR**, matching the published CLI's documented baseline. - Fail-closed paths: garbage input, help screen, and a crashing CLI all exit non-zero and write no SARIF. A clean scan is correctly recognised as clean, not as an error. - All four scan outcomes against stub CLIs: no SARIF → `exit 1 status=error`; clean → `exit 0 status=clean`; findings → `exit 1 status=findings`; crash → `exit 2 status=error`. - Report Python across all four `SCAN_STATUS` values: `''` and `error` → NOT RUN; `clean`/`findings` → results table. - Rendered through a faithful reimplementation of the fleet renderer, first checked against `vu1nz-scan` where it reproduces `moshcoder/moshcode`'s committed workflow byte-for-byte. - `bash -n` on every shell block; manifest validated field-by-field against `actionPackManifestSchema` including `.strict()`. ## Coverage The legacy path is a stopgap — `0.2.2` is a secrets scanner at 12.9%. Once profullstack/threatcrush#73 is merged and published, every installed workflow switches to native SARIF automatically and coverage goes to **90.32%** at the same 0.0% false-positive rate. No re-render needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
Implements what
malware-test-prs/docs/SCANNER_INTEGRATION.mddocuments as missing on the ThreatCrush side.The measurement
That doc scored this CLI at 15.6% true-positive rate, 0.0% false-positive rate and concluded, correctly: "ThreatCrush is a secrets scanner." It found every hardcoded credential and none of the code-level classes — no SQLi, XSS, SSRF, command injection, deserialisation, or XXE.
Reproduce:
Keeping the 0%
The false-positive denominator is a control group of
SAFE:lines — each one a correct implementation of the pattern the vulnerable code beside it gets wrong. Every rule is built against both halves. Three mechanisms:db.query("SELECT … $1", [id])anddb.query("SELECT … '" + id + "'")both containSELECT. Only the second concatenates.realpathon the line, anObjectInputFilterinstalled before thereadObject().--fail-on criticaltherefore won't fire on "this construct exists".Two guards were wrong in ways only the corpus surfaced:
# no allow-list, no scheme restrictionwas read as an allow-list;def sanitize_path_vulnerable(path):was read as a sanitiser, silencing three ReDoS findings.Comments and definition lines are now excluded from guard windows, and Python docstrings from scanning entirely — a file whose header describes its own vulnerability shouldn't produce findings about that description.
Known gaps, on purpose
All nine remaining misses are the four classes in
KNOWN_GAPS: CSRF (3), TOCTOU (2), integer overflow (2), generic dynamic-assignment prototype pollution (2). Each needs whole-function reasoning, and each line-oriented approximation flags ordinary software. Nothing is missed by accident.CLI surface
The doc records that
scanaccepted a path and nothing else, so the testbed parsed terminal output into SARIF and hit three bugs — one of which (paths relative to the scan root) made a working scan read as 0% coverage, silently. Native SARIF removes that pipe entirely.--format text|json|sarif,--output,--fail-on,--path-prefix,--deps,--verbose. URIs resolve against the working directory;startLineclamped to ≥ 1; non-text formats route human output to stderr so> out.sarifis valid.Bugs found while testing
threatcrush scan file.jsreported a clean scan of a file it never opened — the walker only handled directories, andreaddirSyncon a file was caught as an unreadable directory.Verification
pnpm --filter @profullstack/threatcrush test),tsc --noEmitclean,tsupbuilds.0clean,1at/above--fail-on,2scan failure.Docs:
docs/SCANNING.md.🤖 Generated with Claude Code