Skip to content

feat(actions): add the threatcrush-scan pack - #940

Merged
ralyodio merged 6 commits into
masterfrom
feat/threatcrush-scan-pack
Aug 3, 2026
Merged

feat(actions): add the threatcrush-scan pack#940
ralyodio merged 6 commits into
masterfrom
feat/threatcrush-scan-pack

Conversation

@ralyodio

@ralyodio ralyodio commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Adds a threatcrush-scan action pack: runs ThreatCrush 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/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 1the 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/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

Runs ThreatCrush over pull requests and uploads SARIF to the Security
tab. Needs no secrets — the scanner is local to the runner — which is
what makes it installable fleet-wide without provisioning anything
first.

Report-only by default. `failOn` is empty on purpose: a repository with
pre-existing findings should get a report on its first install, not a
blocked pull request. A gate that fires on everything gets switched off
within a day, and a gate that is off is worse than one never installed.

The scan step distinguishes the two non-clean endings. Exit 1 is
"findings at or above failOn" — a result. Exit 2 is "the scan itself
failed" — not a result, and the comment says NOT RUN rather than
rendering an empty findings table, because an unexamined diff is
indistinguishable from a clean one to whoever reads it. The empty-SARIF
fallback exists only so the upload does not fail on a missing file and
bury the real error.

Install retries three times before giving up, for the same reason
vu1nz-scan now does: a transient registry blip is not a security signal.

Deliberately not pull_request_target — that event runs with repository
secrets in scope against a checkout of untrusted contributor code. The
comment step 403s on fork PRs instead, and is continue-on-error; the
report is in the job summary and the artifact regardless. Asserted in a
test so it cannot regress.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

ThreatCrush security scan

0 finding(s) in files changed by this PR (scanned 7 changed file(s); 75 further finding(s) elsewhere in the repository are not attributed to this PR).

No new findings in the files this PR changes.

ThreatCrush CLI ran locally in the runner (npm i -g @profullstack/threatcrush); no code left the container.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

vu1nz Security Review

0 finding(s) in PR #?

No security issues found.

ralyodio and others added 2 commits August 3, 2026 10:01
…ARIF

Caught on a real install. moshcoder/moshpit-name run 30803607991 pulled
the published 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 --fail-on" — so the step
recorded a result, no SARIF was written, the empty-run fallback supplied
one, and the PR comment said "0 findings". A green check on a
repository that was never scanned: the exact failure the pack's other
three guards exist to prevent, arriving through the one path none of
them covered.

Exit codes cannot separate "argument rejected" from "findings found",
so stop trying. Check the interface before scanning and refuse to run
without it, and treat the SARIF file as the only evidence a scan
happened — absent or empty is an error whatever the process returned.

Also fixes the gate itself: exit 1 with findings recorded status and
then let the step succeed, so failOn would have reported findings
without ever failing a pull request. A gate that does not gate is worse
than none, because it is believed.

Verified by replaying all four outcomes against stub CLIs reproducing
the observed 0.2.2 behaviour: no SARIF -> exit 1 status=error; clean ->
exit 0 status=clean; findings -> exit 1 status=findings; crash -> exit 2
status=error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The capability check added in the previous commit worked — the job went
red and the scan was skipped — and the PR comment still said
"0 findings". The report tested `status == "error"`, but a skipped
step yields an empty string, not "error", so the one branch meant to
catch this fell through to the happy path.

Render findings only on positive evidence of a completed scan
("clean" or "findings"). Every other state, including states that do
not exist yet, is NOT RUN.

Verified across all four statuses: '' and 'error' render NOT RUN,
'clean' and 'findings' render the results table.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ralyodio
ralyodio marked this pull request as ready for review August 3, 2026 10:23
ralyodio and others added 3 commits August 3, 2026 10:28
…sing to run

The capability check added in 1.0.1 was right to fail closed, but it
left every repository installing a scanner that refuses to scan until a
new CLI is published. Correct, and useless.

Detect the interface up front and branch on it: native --format when
available, otherwise run the text scan and convert it. The converter
fails closed — unrecognised output exits non-zero and writes nothing, so
an unparseable scan still cannot arrive downstream looking clean.

Written against the real captured output in the testbed's fixture, not
against assumption, because three details of that format 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; and whole-file findings report line 0,
which SARIF rejects.

Verified end to end against stub CLIs: legacy -> 9 findings converted
from the real fixture, scoring 12.9% TPR / 0.0% FPR through the
testbed's own validator, matching the published CLI's documented
baseline; native -> uses --format untouched; legacy crash -> exit 1,
status=error, no SARIF written.

The legacy path is a stopgap. 0.2.2 is a secrets scanner; once a CLI
with --format ships, the workflow switches automatically and coverage
goes from 12.9% to 90.32%.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The destination allowlist had no entry any non-workflow file could match, so
a pack that ships a script its workflow invokes was unrepresentable. Add a
pattern scoped to a dedicated .github/scripts/ directory.

Scoped there rather than the top of .github so a pack cannot land a file
beside dependabot.yml or CODEOWNERS, and kept flat so every managed script
shows up in one listing. This grants no privilege a pack did not already
have — workflow `run:` blocks execute arbitrary code either way — it just
keeps that code in a reviewable file instead of a YAML heredoc.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nation

Every test in packages/actions failed with "Invalid action-pack manifest:
1 issue(s)" — the pack sent threatcrush-to-sarif.py to .github/, which the
destination allowlist rejects. Because each test loads the whole catalog,
one bad manifest took all 15 down, including the four unrelated packs.

Move the converter to .github/scripts/ and update the workflow, README and
test expectations to match.

Also fix the pull_request_target assertion, which the manifest error had
been masking: it substring-matched the entire rendered workflow, so the
comment explaining why the pack deliberately stays on `pull_request`
tripped it. Strip comments first and assert on the trigger block, so the
check tests the directive rather than forbidding its own rationale.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ralyodio
ralyodio merged commit aeb8958 into master Aug 3, 2026
7 checks passed
ralyodio added a commit that referenced this pull request Aug 3, 2026
Picks up the threatcrush-scan pack fix from #940: the converter now installs
to .github/scripts/.
ralyodio added a commit that referenced this pull request Aug 3, 2026
Regenerated from threatcrush-scan@1.1.0 as merged in #940, which moved the
legacy-output converter to .github/scripts/ — the only destination outside
.github/workflows/ the action-pack allowlist accepts.

Converter content is byte-identical; the workflow changes only in the path
it invokes and the managed header hash, which matches the pack source.
Rendered with default inputs, so the install stays report-only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio added a commit that referenced this pull request Aug 3, 2026
* ci: add ThreatCrush security scan

Installs threatcrush-scan@1.1.0 from the sh1pt Actions Store.
Scans pull requests for hardcoded credentials, injection, SSRF, unsafe
deserialisation and dependency tampering; uploads SARIF to the Security
tab.

Report-only — it will not fail a pull request. Set the pack's failOn
input to critical,high once the existing findings are triaged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* ci: re-render threatcrush-scan off master

Regenerated from threatcrush-scan@1.1.0 as merged in #940, which moved the
legacy-output converter to .github/scripts/ — the only destination outside
.github/workflows/ the action-pack allowlist accepts.

Converter content is byte-identical; the workflow changes only in the path
it invokes and the managed header hash, which matches the pack source.
Rendered with default inputs, so the install stays report-only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant