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