fix(threatcrush-scan): stop resolving a repository's alerts on a failed scan

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

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

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'),