From 55dda8ae1fa30f8079018789285ac8fb1b8d9277 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Fri, 14 Aug 2026 02:10:16 +0000 Subject: [PATCH] fix(threatcrush-scan): stop resolving a repository's alerts on a failed scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-Authored-By: Claude Opus 5 --- packages/actions/threatcrush-scan/README.md | 21 +++++++ .../threatcrush-scan/sh1pt.actionpack.yaml | 9 ++- .../actions/threatcrush-scan/workflow.yml | 58 +++++++++++-------- 3 files changed, 62 insertions(+), 26 deletions(-) diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md index d6dd18b0..9541f179 100644 --- a/packages/actions/threatcrush-scan/README.md +++ b/packages/actions/threatcrush-scan/README.md @@ -110,4 +110,25 @@ weakness classes that are deliberately not implemented. Snippets in the report are redacted — the CLI never prints matched credential material, because CI logs are retained and, on public forks, published. +## What the integrity check does not cover + +`threatcrushIntegrity` covers the published CLI tarball and nothing else. +`npm install -g` still resolves that package's own runtime dependencies from +version ranges, so the code the CLI actually loads at runtime is **not** fully +covered by the hash. Raised in review by the SAG maintainers, who verified the +hash matched and then pointed at the gap behind it. + +Two ways to close it, neither free: + +- **A committed lockfile with `npm ci`.** Complete — it pins integrity for + every package in the tree. It is also 210 packages and about 2,500 lines of + `package-lock.json` landing in the consuming repository, which is a large + diff to put in front of a maintainer who did not ask for it. +- **A bundled artifact** whose tarball contains its runtime. Only partial here: + the CLI depends on `better-sqlite3`, which is a native module and cannot be + bundled into a single JavaScript file. + +Neither is the default. A repository that wants the first should say so, and +the lockfile can be supplied for it. + [testbed]: https://github.com/profullstack/malware-test-prs diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index 4dfec37e..097a0c91 100644 --- a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml +++ b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml @@ -5,7 +5,7 @@ description: >- Scans pull requests for hardcoded credentials, injection, SSRF, unsafe deserialisation and dependency tampering, and uploads SARIF to the Security tab. -version: 1.3.0 +version: 1.4.0 publisher: profullstack visibility: public license: MIT @@ -96,6 +96,11 @@ policies: requiresReview: true security: leastPrivilegePermissions: true - pinThirdPartyActions: optional + # Required, not 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, and the SAG maintainers read it: the job holds + # pull-requests: write and security-events: write, so every action in it is + # the same class of grant the npm pin exists to close. + pinThirdPartyActions: required allowPullRequestTarget: false defaultTimeoutMinutes: 15 diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index c5e1708e..7be0b18e 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -20,11 +20,11 @@ jobs: # the job — and the rest of this job runs a scanner installed from the # network over the contents of a pull request. A token that no step # needs should not be sitting in the working tree while that happens. - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: persist-credentials: false - - uses: actions/setup-node@v4 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: "{{nodeVersion}}" @@ -183,27 +183,27 @@ jobs: ;; esac - # Reached only when the scan step already failed the job. The empty run - # exists so the upload does not error on a missing file and bury the real - # cause; it is not a result. The scan step has already set status=error, - # so the report says NOT RUN rather than rendering this as a clean scan. - - name: Ensure SARIF exists - if: always() - run: | - if [ ! -f threatcrush.sarif ]; then - cat > threatcrush.sarif <<'JSON' - { - "version": "2.1.0", - "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", - "runs": [{ "tool": { "driver": { "name": "ThreatCrush", "rules": [] } }, "results": [] }] - } - JSON - fi - + # Uploaded only when a scan actually produced results. Never on failure, + # and never as a synthesised empty file. + # + # This used to write a zero-result SARIF when the file was missing, so the + # upload would not error and bury the real cause. That reasoning covered + # the wrong path. Code scanning treats a new analysis in a category as the + # current truth for that category, so an empty run does not read as "no + # data" — it resolves every open ThreatCrush alert the repository already + # had. A scanner that fails and marks the findings it previously reported + # as fixed is worse than one that does not run. + # + # Found in review by the SAG maintainers, who were right: the old comment + # defended the PR comment path (which does say NOT RUN) and said nothing + # about the upload, because nobody had looked at the upload. - name: Upload to the Security tab - if: always() && '{{uploadSarif}}' == 'true' + if: >- + always() && '{{uploadSarif}}' == 'true' + && (steps.scan.outputs.status == 'clean' || steps.scan.outputs.status == 'findings') + && hashFiles('threatcrush.sarif') != '' continue-on-error: true - uses: github/codeql-action/upload-sarif@v3 + uses: github/codeql-action/upload-sarif@f3712979fa5f215279b101dd0a2e3bdfb4353324 # v3 with: sarif_file: threatcrush.sarif category: threatcrush @@ -286,12 +286,16 @@ jobs: if: always() run: cat "$RUNNER_TEMP/threatcrush-comment.md" >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true + # if-no-files-found: ignore, because nothing synthesises the file any + # more. A run that never produced SARIF has no artifact to keep, and that + # is the honest outcome rather than a reason to invent one. - name: Upload SARIF artifact if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: name: threatcrush-sarif path: threatcrush.sarif + if-no-files-found: ignore retention-days: 30 # Best-effort. `pull_request` gives fork PRs a read-only token, so this @@ -303,7 +307,7 @@ jobs: - name: Comment on PR if: always() && github.event.pull_request.head.repo.full_name == github.repository && github.actor != 'dependabot[bot]' continue-on-error: true - uses: actions/github-script@v7 + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7 with: script: | const fs = require('fs'); @@ -315,10 +319,16 @@ jobs: } try { - const { data: comments } = await github.rest.issues.listComments({ + // Paginated. listComments returns the first thirty and stops, so + // on a pull request with more discussion than that the existing + // report falls off the page, is not found, and every subsequent + // run posts another one. The bug only appears on the requests + // people actually engage with, which is the worst place for it. + const comments = await github.paginate(github.rest.issues.listComments, { issue_number: context.issue.number, owner: context.repo.owner, repo: context.repo.repo, + per_page: 100, }); const existing = comments.find( (c) => c.user.type === 'Bot' && c.body.includes('ThreatCrush Security Scan'),