fix(threatcrush-scan): stop resolving a repository's alerts on a failed scan - #960
Merged
Conversation
…ed scan Review from the SAG maintainers on Zleap-AI/SAG#93. Three of their four findings are fixed here; the fourth is documented rather than pretended away. The first one is not a readiness complaint, it is a defect that hurts repositories that already installed this. On any failure the workflow synthesised a zero-result SARIF so the upload would not error on a missing file, then uploaded it under category: threatcrush. Code scanning treats a new analysis in a category as the current truth for that category. An empty run does not read as "no data" — it resolves every open ThreatCrush alert the repository had. A scanner that fails and marks its own previous findings as fixed is worse than one that never ran. The inline comment defending that step covered the PR comment path, which does correctly say NOT RUN, and said nothing about the upload, because nobody had looked at the upload. Now: no synthesised file at all, and the upload is gated on the scan step actually reporting clean or findings with a SARIF present. The artifact upload takes if-no-files-found: ignore, since nothing invents the file any more. Second, every action is pinned to a full commit SHA, and the manifest says pinThirdPartyActions: required rather than optional. Asking a repository to trust a pinned npm package while the workflow around it floats on mutable tags is an argument that does not survive being read. The job holds pull-requests: write and security-events: write; every action in it is the same class of grant the npm pin exists to close. actions/checkout 11d5960 actions/setup-node 49933ea github/codeql-action/upload-sarif f371297 actions/upload-artifact ea165f8 actions/github-script f28e40c Third, the existing-comment lookup is paginated. listComments returns thirty and stops, so on a pull request with more discussion than that the report falls off the page, is not found, and every run posts another one — a bug that only appears on the requests people engage with. Fourth is the dependency tree, and it is correct and not fixed. The hash covers the published tarball; npm install -g still resolves that package's own dependencies from ranges. Closing it means either a committed lockfile (complete, but 210 packages and ~2,500 lines landing in the consuming repository) or a bundled artifact (impossible in full here — better-sqlite3 is native). Written into the README as a stated limitation, with both options, rather than left for the next reviewer to find. Pack to 1.4.0. Signed-off-by: Anthony Ettinger <anthony@chovy.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
ThreatCrush Security Scan311 finding(s) HIGH/CRITICAL: 24 | MEDIUM: 51 | LOW: 236
…and 261 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
This was referenced Aug 14, 2026
ralyodio
added a commit
that referenced
this pull request
Aug 14, 2026
) Re-targeted at master. This landed in #961, but #961 was based on sag-review rather than master and #960 was squash-merged, so merging #961 put the change on sag-review and left master without it. The consumer side is already on threatcrush master, so right now TCFEED_LEAST_PRIVILEGE=1 sets inputs the pack has no placeholders for and silently changes nothing — the permission block is still static. The change itself is unchanged from #961: The permission block requested `pull-requests: write` and `security-events: write` unconditionally, including in the configuration where both the upload and the comment are switched off. A workflow that asks for a write scope it will not use cannot call itself least privilege, which is awkward for one whose request body argues about supply-chain hygiene. commentOnPr joins uploadSarif as a switch, and extraPermissions carries the lines beneath `contents: read` — computed from the two rather than set by hand, so the block cannot drift out of step with what the workflow actually does. With both false the rendered workflow requests `contents: read` and nothing else, and findings arrive in the job summary and the SARIF artifact. Verified by parsing the rendered output: default {"contents":"read","pull-requests":"write","security-events":"write"} least privilege {"contents":"read"} Pack to 1.5.0. Signed-off-by: Anthony Ettinger <anthony@chovy.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Review from the SAG maintainers on Zleap-AI/SAG#93. Three of their four findings are fixed here; the fourth is documented rather than pretended away.
1. The empty SARIF upload — a defect, not a readiness complaint
This one already affects repositories that installed the pack.
On any failure the workflow synthesised a zero-result SARIF so the upload wouldn't error on a missing file, then uploaded it under
category: threatcrush. Code scanning treats a new analysis in a category as the current truth for that category. An empty run doesn't read as "no data" — it resolves every open ThreatCrush alert the repository had.A scanner that fails and marks its own previous findings as fixed is worse than one that never ran.
The inline comment defending that step covered the PR comment path — which does correctly say
NOT RUN— and said nothing about the upload, because nobody had looked at the upload.Now: no synthesised file at all, and the upload is gated on the scan step actually reporting
cleanorfindingswith a SARIF present. The artifact upload takesif-no-files-found: ignore, since nothing invents the file any more.2. Actions pinned to full SHAs
Asking a repository to trust a pinned npm package while the workflow around it floats on mutable tags is an argument that doesn't survive being read. The job holds
pull-requests: writeandsecurity-events: write; every action in it is the same class of grant the npm pin exists to close.actions/checkout11d5960actions/setup-node49933eagithub/codeql-action/upload-sariff371297actions/upload-artifactea165f8actions/github-scriptf28e40cManifest now says
pinThirdPartyActions: requiredrather thanoptional.3. Paginated comment lookup
listCommentsreturns thirty and stops, so on a PR with more discussion than that the existing report falls off the page, isn't found, and every run posts another one — a bug that only shows up on the requests people actually engage with.4. The dependency tree — correct, and not fixed
The hash covers the published tarball;
npm install -gstill resolves that package's own dependencies from ranges. They verified the hash matched and then pointed straight at the gap behind it.Closing it means one of:
npm ci— complete, but 210 packages and ~2,500 lines ofpackage-lock.jsonlanding in the consuming repositorybetter-sqlite3is native and can't be bundledNeither is a sane default, so it's written into the README as a stated limitation with both options, rather than left for the next reviewer to find.
Pack to 1.4.0.
🤖 Generated with Claude Code