feat(threatcrush-scan): one file, native SARIF — pack 1.7.0 - #967
Merged
Conversation
The pack installed two files into a consumer's repository: the workflow, and a
235-line Python converter that reconstructed findings by regex from the CLI's
terminal output. This removes the second one, along with the capability probe
that chose between them, and pins 0.11.2.
The premise for the converter stopped holding. `threatcrushPackageSpec` names
an exact version and the install step refuses any other bytes, so which
interface the CLI has is decided by the pack rather than discovered on the
runner — `grep -q -- '--format'` could only ever answer yes. Dead code, but
dead code that was still being written into other people's repositories.
Removing it is a security change more than a tidying one:
* It parsed a *display* format, which is free to change between releases.
The failure mode is a silent undercount that still looks like a completed
scan, which is the one thing this workflow is arranged to prevent.
* Every file a pack writes into somebody else's repository is surface their
reviewer has to read. This came straight from review on
erfan138057/microbot#6, where the maintainer would not merge until the
supply chain was clearer. Two files became one, and the answer to "what
does this add to my repo" is now a single workflow.
0.11.0 -> 0.11.2 with the matching integrity hash. 0.11.2 is the release that
fixes the false-positive class flagging `access_token: 'test-token'`-style
fixtures in test files, which is what that repository's own scan tripped on.
Verified by rendering the template both ways: with both outputs enabled it
still asks for `pull-requests: write` and `security-events: write`, and with
both disabled it renders `contents: read` and nothing else. Both parse as
YAML, neither mentions the converter, both use `--format sarif`. The 18 pack
tests pass, with the two that asserted the shim's presence inverted to assert
its absence.
sh1pt's own installed workflow is left alone: it is a fleet-managed file at
pack 1.1.0 with a content hash, and its copy of the converter is still on
disk, so hand-editing it here would forge the hash and fix nothing.
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
ThreatCrush Security Scan84 finding(s) HIGH/CRITICAL: 24 | MEDIUM: 51 | LOW: 9
…and 34 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
to profullstack/threatcrush
that referenced
this pull request
Aug 16, 2026
…too (#152) Two gaps found by working erfan138057/microbot#6 by hand. **check could not see a maintainer.** It read workflow runs and nothing else, so a request blocked for a week on a question reported as healthy. microbot was exactly that: no red check anywhere, and a maintainer asking why the workflow wanted `pull-requests: write`. The runs said "waiting for the maintainer to approve" while the maintainer was waiting on us. `check` now reads three threads — the pull request conversation, its reviews, and the issue that asked first, whose number comes out of our own pull request body — and prints anything newer than our last word in them, as NEEDS A REPLY with an excerpt. Run against microbot it immediately surfaced a reply nobody had noticed. It never writes one. A commit that fixes a build is a mechanical claim this can verify from a log; a reply to "explain why you need pull-requests: write" is a technical argument about somebody else's repository, and a canned answer reads as a bot to precisely the audience already unsure about installing a third-party scanner. The whole pitch is that a person asked. The unanswered test is deliberately crude — newest not-ours is newer than newest ours — so it over-reports when a maintainer says "thanks" after our reply. That is the right direction to be wrong: a spurious line costs a glance, a missed question costs the request. **--fix now implies --refresh.** The split did not survive contact: --fix repaired a broken build and left the request pinned to whatever version it was opened with, so a repository that took a week to answer got a scanner months out of date — including, for the batch opened before 0.11.2, one whose known false positives were the thing the maintainer was reading. --refresh alone keeps its old meaning for pushing a pack release across the fleet. Refresh runs after the check, not before: it pushes a commit, which starts runs against a head SHA that has none yet, so the other order would read "no runs" for every repository it had just written to. **Refresh can now remove a file.** Rewriting the workflow was not enough — requests opened before pack 1.7.0 have the SARIF converter committed to their branch, so leaving it means the diff still adds two files after we told a maintainer it adds one. Note for whoever runs this next: that last part needs profullstack/sh1pt#967 merged and pulled first. `packDir()` resolves to the sh1pt checkout beside this one, and until that lands it renders pack 1.5.1 — which still installs the converter this is meant to remove.
#966 landed the same version bump from the other direction — 0.11.0 -> 0.11.2 with the identical integrity hash — while this branch was open. The only conflict was the pack version line, and it is a conflict about arithmetic rather than about content: master said 1.6.1 for the pin, this says 1.7.0 for removing a file the pack installs. Removing an installed file is the larger change, so 1.7.0 stands and subsumes the patch bump. Nothing else disagreed. Both sides wrote the same spec and the same hash, which is the reassuring outcome — two people reading `npm view` got the same answer.
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.
Pack
threatcrush-scan1.6.0 → 1.7.0. Installs one file instead of two.What changes
The pack wrote two files into a consumer's repository: the workflow, and
threatcrush-to-sarif.py— 235 lines that reconstructed findings by regex from the CLI's terminal output. That converter and the capability probe that chose between it and the native path are both removed.The premise stopped holding.
threatcrushPackageSpecnames an exact version and the install step refuses any other bytes, so which interface the CLI has is decided by the pack, not discovered on the runner.grep -q -- '--format'could only ever answer yes.Why it is a security change, not tidying
Also
threatcrushPackageSpec0.11.0→0.11.2, with the matchingthreatcrushIntegrity. 0.11.2 is the release that fixes the false-positive class flaggingaccess_token: 'test-token'-style fixtures in test files — which is what microbot's own scan tripped on (2 findings on 0.11.1, 0 on 0.11.2).Verification
Rendered the template both ways and parsed the result:
contents: read,pull-requests: write,security-events: writecontents: readand nothing elseThe least-privilege path still works, which is the configuration a reviewer declining on write scopes actually wants.
18/18 pack tests pass. The two that asserted the shim's presence are inverted to assert its absence, rather than deleted.
Deliberately not touched
.github/workflows/threatcrush-scan.yml— sh1pt's own installed copy. It is a fleet-managed file at pack 1.1.0 carrying a content hash, and its copy of the converter is still on disk, so its CI keeps working. Hand-editing it here would forge the hash and fix nothing; it should be re-synced by the install tooling.🤖 Generated with Claude Code