From a2b806e32b4b6ed50cd63085093c695e93b07528 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 17 Aug 2026 05:09:12 +0000 Subject: [PATCH] feat(threatcrush-scan): omit the write scopes and their steps, not just disable them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mac-developer-bridge declined the offered workflow on two points that were the pack's rather than the repository's. The two write scopes were requested unconditionally even though the workflow is described as report-only, and `uploadSarif: false` / `commentOnPr: false` still rendered the steps that use them, guarded by a condition. That makes "least privilege" a claim about a condition rather than a description of the file. A shipped-but-disabled Security-tab upload still asks a maintainer to read an upload and take on trust that the guard is right. So the template gained a whole-line `{{#if var}} … {{/if}}` block, and the two optional steps are removed from the render rather than switched off. Each scope is now emitted by the output that needs it — `pull-requests: write` from commentOnPr, `security-events: write` from uploadSarif — so the permissions block cannot drift out of step with what the workflow does. That retires the hand-assembled `extraPermissions` input, which made least privilege something a caller had to remember. `{{#if}}` is the only control construct and it is not nestable. It still takes a bare variable name, so the renderer's "only {{varName}}, no expressions" guarantee is unchanged: there is nothing to evaluate, only a value compared against the literal 'true'. Anything else drops the block, because pack inputs are strings and treating a stray value as truthy would turn a typo into a granted write scope. Blocks resolve before substitution, so a variable that only makes sense inside a kept block need not be supplied when it is dropped. The second point was size. With both outputs off the render is 291 lines and one file, against the 382-line workflow plus 235-line converter that was offered. The report builder is the same behaviour in ~half the lines; the install step's commentary is trimmed. Nothing was dropped from it: fail-closed NOT RUN, severity-first ordering, the scoped/backlog fold and the truncation notice all still render, verified against a real 6-finding SARIF across five states (scoped, scoped-with-no-touched-findings, unscoped, scan-skipped, SARIF-missing). One fix found while testing: the report's "could not read SARIF" warning went to stdout, which is now the report file itself. It goes to stderr. Also read-only outputs are the ones that keep working on fork pull requests, where GitHub downgrades GITHUB_TOKEN — so the richest reporting was least reliable exactly where an external scan is most useful. That is now in the README rather than discovered by a maintainer. --- .../src/action-pack/render.test.ts | 44 ++ .../src/action-pack/render.ts | 36 +- packages/actions/src/index.test.ts | 66 +++ packages/actions/threatcrush-scan/README.md | 28 +- .../threatcrush-scan/sh1pt.actionpack.yaml | 22 +- .../actions/threatcrush-scan/workflow.yml | 452 +++++++----------- 6 files changed, 362 insertions(+), 286 deletions(-) diff --git a/packages/actions-fleet-core/src/action-pack/render.test.ts b/packages/actions-fleet-core/src/action-pack/render.test.ts index 675766d8..9d852ead 100644 --- a/packages/actions-fleet-core/src/action-pack/render.test.ts +++ b/packages/actions-fleet-core/src/action-pack/render.test.ts @@ -67,6 +67,50 @@ describe('applyTemplate', () => { it('rejects unknown variables', () => { expect(() => applyTemplate('{{missing}}', {})).toThrow(TemplateRenderError); }); + + it('keeps a {{#if}} block when the variable is exactly "true"', () => { + const out = applyTemplate('a\n{{#if on}}\nb\n{{/if}}\nc\n', { on: 'true' }); + expect(out).toBe('a\nb\nc\n'); + }); + + it('drops a {{#if}} block, and its markers, otherwise', () => { + for (const value of ['false', '', 'TRUE', 'yes', '1']) { + const out = applyTemplate('a\n{{#if on}}\nb\n{{/if}}\nc\n', { on: value }); + // Anything but the literal 'true' drops it: pack inputs are strings with + // a 'true'/'false' enum, and treating a stray value as truthy would turn + // a typo into a granted write scope. + expect(out).toBe('a\nc\n'); + } + }); + + it('leaves variables inside a dropped block unresolved rather than erroring', () => { + // The block is removed before substitution, so a variable that only makes + // sense when the block is on does not have to be supplied when it is off. + const out = applyTemplate('a\n{{#if on}}\n{{onlyWhenOn}}\n{{/if}}\nc\n', { on: 'false' }); + expect(out).toBe('a\nc\n'); + }); + + it('still substitutes variables inside a kept block', () => { + const out = applyTemplate('{{#if on}}\nnode: {{nodeVersion}}\n{{/if}}\n', { + on: 'true', + nodeVersion: '22', + }); + expect(out).toBe('node: 22\n'); + }); + + it('rejects a {{#if}} on an unknown variable', () => { + expect(() => applyTemplate('{{#if nope}}\nx\n{{/if}}\n', {})).toThrow(TemplateRenderError); + }); + + it('rejects an unterminated {{#if}}', () => { + // Falls through to the scalar pass, where "#if on" is not a variable name. + expect(() => applyTemplate('{{#if on}}\nx\n', { on: 'true' })).toThrow(TemplateRenderError); + }); + + it('does not treat a GitHub expression inside a block as a template tag', () => { + const out = applyTemplate('{{#if on}}\nrun: ${{ github.sha }}\n{{/if}}\n', { on: 'true' }); + expect(out).toBe('run: ${{ github.sha }}\n'); + }); }); describe('resolveInputs', () => { diff --git a/packages/actions-fleet-core/src/action-pack/render.ts b/packages/actions-fleet-core/src/action-pack/render.ts index 0b721efd..ed9bad38 100644 --- a/packages/actions-fleet-core/src/action-pack/render.ts +++ b/packages/actions-fleet-core/src/action-pack/render.ts @@ -42,8 +42,42 @@ export class MissingInputError extends Error { const TAG_RE = /(?): string { + return template.replace(BLOCK_RE, (_match, rawExpr: string, body: string) => { + const expr = rawExpr.trim(); + if (!SAFE_VAR_RE.test(expr)) { + throw new TemplateRenderError( + `unsupported template expression "{{#if ${rawExpr}}}" — only {{#if varName}} is allowed`, + ); + } + if (!Object.prototype.hasOwnProperty.call(values, expr)) { + throw new TemplateRenderError(`template referenced unknown variable "${expr}"`); + } + // Anything other than the literal 'true' drops the block. Pack inputs are + // strings with a 'true'/'false' enum, and treating a stray value as truthy + // would turn a typo into a granted write scope. + return values[expr] === 'true' ? body : ''; + }); +} + export function applyTemplate(template: string, values: Record): string { - return template.replace(TAG_RE, (_match, rawExpr: string) => { + // Blocks first: a dropped block must not have its {{vars}} resolved, and an + // unresolvable variable inside a dropped block is not an error. + return applyBlocks(template, values).replace(TAG_RE, (_match, rawExpr: string) => { const expr = rawExpr.trim(); if (!SAFE_VAR_RE.test(expr)) { throw new TemplateRenderError( diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index 694c6ddd..93b560ac 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -350,4 +350,70 @@ describe('built-in packs', () => { expect(directives).toMatch(/^on:\n\s+pull_request:\s*$/m); expect(entry.manifest.security.allowPullRequestTarget).toBe(false); }); + + it('omits the write scopes and the steps that need them, rather than disabling them', async () => { + // The read-only install is the version a first-time reviewer is asked to + // trust, so "least privilege" has to be a property of the rendered file + // rather than of a condition inside it. A shipped-but-disabled Security + // tab upload still asks a maintainer to read and reason about an upload. + // + // mac-developer-bridge declined the earlier shape on exactly this: the two + // write scopes were requested unconditionally even though the workflow was + // described as report-only, and GitHub downgrades them on fork pull + // requests anyway — so the richest outputs were the least reliable ones + // precisely where the scan is most useful. + const catalog = await loadBuiltinPacks(); + const entry = catalog.get('threatcrush-scan'); + if (!entry) throw new Error('threatcrush-scan not in catalog'); + const result = await renderPack({ + packDir: entry.packDir, + manifest: entry.manifest, + inputs: { commentOnPr: 'false', uploadSarif: 'false' }, + }); + const content = result.files[0]?.content ?? ''; + const workflow = parseYaml(content); + + expect(workflow.permissions).toEqual({ contents: 'read' }); + + const names = (workflow?.jobs?.scan?.steps ?? []).map((step: { name?: string }) => step?.name); + expect(names).not.toContain('Upload to the Security tab'); + expect(names).not.toContain('Comment on PR'); + + // Gone from the file, not merely unreachable in it. `upload-sarif` and + // `github-script` are the two actions that would hold those scopes. + expect(content).not.toContain('upload-sarif'); + expect(content).not.toContain('github-script'); + expect(content).not.toContain('security-events'); + expect(content).not.toContain('pull-requests: write'); + + // The read-only outputs are the ones that survive, and they are also the + // two that work on a fork pull request. + expect(names).toContain('Build the report'); + expect(names).toContain('Upload SARIF artifact'); + expect(content).toContain('$GITHUB_STEP_SUMMARY'); + }); + + it('still asks for a write scope only when the output that needs it is on', async () => { + // Each scope is emitted by its own output, so the block cannot drift out + // of step with what the workflow does. It used to be one hand-assembled + // `extraPermissions` string, which made least privilege something a caller + // had to remember. + const catalog = await loadBuiltinPacks(); + const entry = catalog.get('threatcrush-scan'); + if (!entry) throw new Error('threatcrush-scan not in catalog'); + + const permissionsFor = async (inputs: Record) => { + const result = await renderPack({ packDir: entry.packDir, manifest: entry.manifest, inputs }); + return parseYaml(result.files[0]?.content ?? '').permissions; + }; + + expect(await permissionsFor({ commentOnPr: 'true', uploadSarif: 'false' })).toEqual({ + contents: 'read', + 'pull-requests': 'write', + }); + expect(await permissionsFor({ commentOnPr: 'false', uploadSarif: 'true' })).toEqual({ + contents: 'read', + 'security-events': 'write', + }); + }); }); diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md index f49f833e..4a50cab3 100644 --- a/packages/actions/threatcrush-scan/README.md +++ b/packages/actions/threatcrush-scan/README.md @@ -17,7 +17,33 @@ sh1pt actions install threatcrush-scan --repo owner/name --pr | `threatcrushPackageSpec` | `@profullstack/threatcrush@0.11.2` | npm spec used to install the CLI. Pinned rather than `@latest` so one bad publish cannot break every consumer at once; bump it in a pack release. | | `threatcrushIntegrity` | *(sha512 of 0.11.2)* | SRI hash of that tarball. The workflow downloads, hashes and compares before installing, and refuses to install on a mismatch. Bump it with the spec — read it from `npm view dist.integrity`. Empty skips the check. | | `failOn` | *(empty)* | Comma-separated severities that fail the job, e.g. `critical,high`. Empty is report-only. | -| `uploadSarif` | `true` | Upload to the Security tab. | +| `uploadSarif` | `true` | Upload to the Security tab. Emits `security-events: write`. | +| `commentOnPr` | `true` | Post the report as a pull request comment. Emits `pull-requests: write`. | + +## Least privilege is a property of the file, not a condition inside it + +Set `uploadSarif` and `commentOnPr` both to `false` and the rendered workflow +asks for `contents: read` and nothing else. The findings go to the job summary +and the SARIF artifact, neither of which needs a write scope. + +The two steps that would use those scopes are **not present** in that render — +not shipped-and-disabled. This is the difference the pack cares about: a +disabled Security-tab upload still asks a maintainer to read an upload, reason +about what it mutates, and take on trust that the condition guarding it is +correct. Dead surface in a security-sensitive file is surface all the same, and +a reviewer counting what they are being asked to trust counts it. + +Each scope is emitted by the output that needs it, so the `permissions:` block +cannot drift out of step with what the workflow actually does. It used to be a +single hand-assembled `extraPermissions` string, which made least privilege +something a caller had to remember rather than something the template +guaranteed. + +There is a second reason to prefer this shape on a first install, and it is not +about trust: **fork pull requests get a read-only `GITHUB_TOKEN`**. The comment +and the Security-tab upload are the outputs GitHub downgrades, so the richest +reporting is least reliable exactly where an external scan is most useful. The +job summary and the artifact work the same on every pull request. ## Pinned means pinned — including for fixes diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index d50ed968..b070855f 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.7.0 +version: 2.0.0 publisher: profullstack visibility: public license: MIT @@ -74,17 +74,15 @@ inputs: and the SARIF artifact instead. That is the configuration for a repository that wants the scan without granting a third-party CLI any write scope, which is a substantial part of what reviewers decline on. - extraPermissions: - type: string - default: " pull-requests: write\n security-events: write" - description: >- - The permission lines added beneath `contents: read`, computed from - uploadSarif and commentOnPr rather than set by hand. Two spaces of - indentation per line; empty when neither output is enabled. - - An input rather than a fixed block because a workflow that asks for a - write scope it will not use cannot argue it is least-privilege, and the - two scopes here only exist to serve features a consumer can switch off. + # No input controls the `permissions:` block. Each write scope is emitted by + # the output that needs it — `pull-requests: write` from commentOnPr, + # `security-events: write` from uploadSarif — so the block cannot drift out of + # step with what the workflow actually does. It used to be an `extraPermissions` + # string the caller assembled by hand, which made "least privilege" a thing a + # caller had to remember rather than a property of the template. + # + # Omitted, not disabled: with both outputs off the rendered file has no write + # scope and no step that would use one. failOn: type: string default: '' diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index 72446253..7f01cc81 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -3,18 +3,22 @@ name: threatcrush security scan on: pull_request: -# Only what the enabled outputs actually need. Both write scopes exist to -# serve an optional feature — the Security tab upload and the PR comment — and -# were requested unconditionally even when both were switched off. +# Only what the enabled outputs actually need. Both write scopes below serve an +# optional feature — the Security tab upload and the pull request comment — and +# are omitted entirely, not disabled, when those are switched off. # # With uploadSarif and commentOnPr both false this reads `contents: read` and -# nothing else, and the findings arrive in the job summary and the artifact. -# SAG declined partly on "an externally maintained CLI ... together with PR and -# security-reporting permissions"; a scanner that asks for write scopes it is -# not going to use has no answer to that, and now it does not have to ask. +# nothing else, and findings arrive in the job summary and the SARIF artifact. +# Those are also the two outputs that keep working on fork pull requests, where +# GitHub downgrades GITHUB_TOKEN to read-only. permissions: contents: read -{{extraPermissions}} +{{#if commentOnPr}} + pull-requests: write +{{/if}} +{{#if uploadSarif}} + security-events: write +{{/if}} jobs: scan: @@ -23,48 +27,41 @@ jobs: timeout-minutes: 15 steps: - # persist-credentials: false because nothing here pushes. Left at the - # default, checkout leaves a credential in .git/config for the rest of - # 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. + # persist-credentials: false because nothing here pushes, and the rest of + # this job runs a scanner installed from the network over the contents of + # a pull request. A token no step needs should not be sitting in + # .git/config while that happens. - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: persist-credentials: false # Two commits, so the merge ref's own parents are present and the - # report can tell this pull request's files from the rest of the - # repository. See "Determine which files this pull request touches". + # report can tell this pull request's files from the rest of the tree. fetch-depth: 2 # Which findings belong to this review? # # The scan covers the whole tree, and it should: a credential three - # directories away is still committed. But a *pull request comment* is a - # review artifact, and a review is about the change under review. Posting - # the repository's entire standing backlog on every pull request means an - # author who changed two files is handed ninety findings they did not - # write, cannot action, and did not ask about — and the one finding that - # is theirs sits somewhere in the middle of it. + # directories away is still committed. But a report is a review artifact, + # and a review is about the change under review. `refs/pull/N/merge` has + # the base branch as its first parent and the head as its second, so + # `HEAD^1..HEAD` is exactly this pull request's diff — no API call, no + # token. # - # `refs/pull/N/merge` has the base branch as its first parent and the - # pull request head as its second, so `HEAD^1..HEAD` is exactly this - # pull request's diff, with no API call and no token. - # - # That identity only holds for a real merge ref. When the pull request - # has conflicts GitHub cannot produce one, checkout falls back to the - # head commit, and `HEAD^1` silently becomes "the previous commit on the - # branch" — a plausible-looking answer to a different question. So the - # shape is verified before it is trusted, and a failure falls back to - # reporting everything unscoped rather than scoping to the wrong set. + # That identity only holds for a real merge ref. On a conflicted pull + # request GitHub cannot produce one, checkout falls back to the head + # commit, and `HEAD^1` silently becomes "the previous commit on the + # branch". So the shape is verified before it is trusted, and a failure + # reports everything unscoped rather than scoping to the wrong set. - name: Determine which files this pull request touches id: changed run: | + set -euo pipefail if [ "$(git rev-list --parents --max-count=1 HEAD | wc -w)" -eq 3 ]; then - git diff --name-only HEAD^1 HEAD > "$RUNNER_TEMP/threatcrush-changed.txt" + git diff --name-only HEAD^1 HEAD > "${RUNNER_TEMP}/threatcrush-changed.txt" echo "scoped=true" >> "$GITHUB_OUTPUT" - echo "Scoping the report to $(wc -l < "$RUNNER_TEMP/threatcrush-changed.txt") changed file(s)." + echo "Scoping the report to $(wc -l < "${RUNNER_TEMP}/threatcrush-changed.txt") changed file(s)." else - : > "$RUNNER_TEMP/threatcrush-changed.txt" + : > "${RUNNER_TEMP}/threatcrush-changed.txt" echo "scoped=false" >> "$GITHUB_OUTPUT" echo "::notice::No merge ref (conflicted pull request?) — reporting every finding, unscoped." fi @@ -73,35 +70,23 @@ jobs: with: node-version: "{{nodeVersion}}" - # An unretried `npm i -g` is a network call to a registry that decides - # whether a security gate runs at all. Retry before giving up; a - # transient registry blip is not a security signal and should not read - # like one. + # Downloaded, hashed, and only then installed. A version pin says which + # release to fetch; it does not say the bytes are the ones that release + # was published with, and the party answering "which version" is the same + # party serving the tarball. # # --ignore-scripts because a lifecycle script is arbitrary code from the - # dependency tree, and this job holds `pull-requests: write` and - # `security-events: write`. The CLI does not need them: it declares no - # install hook of its own, and `scan` was verified to run correctly from - # an --ignore-scripts install. A security gate that opens a shell for - # its own supply chain is not a gate. - # - # Downloaded, hashed, and only then installed. A pinned version says - # which release to fetch; it does not say the bytes are the ones that - # release was published with, and the party answering "which version" - # is the party serving the tarball. The hash is the half a version pin - # cannot give you, which is the distinction Haven's maintainer drew - # when they asked for "exact version + integrity hash" rather than - # treating the pin as the answer. - # - # Into RUNNER_TEMP, never the checkout: `npm pack` writes to the working - # directory by default, and a stray .tgz in the tree is something this - # workflow then scans and reports on. + # dependency tree; the CLI declares no install hook and `scan` runs + # correctly without one. Into RUNNER_TEMP rather than the checkout, + # because `npm pack` writes to the working directory by default and a + # stray .tgz is something this job would then scan. - name: Install ThreatCrush run: | set -euo pipefail spec='{{threatcrushPackageSpec}}' want='{{threatcrushIntegrity}}' + # Retried: a transient registry blip is not a security signal. name="" for attempt in 1 2 3; do if name=$(npm pack --silent --pack-destination "${RUNNER_TEMP}" "${spec}" | tail -1) \ @@ -109,22 +94,20 @@ jobs: break fi name="" - delay=$((attempt * 10)) - echo "::warning::ThreatCrush download attempt ${attempt}/3 failed; retrying in ${delay}s" - sleep "${delay}" + echo "::warning::ThreatCrush download attempt ${attempt}/3 failed; retrying in $((attempt * 10))s" + sleep "$((attempt * 10))" done if [ -z "${name}" ]; then echo "::error::ThreatCrush download failed after 3 attempts" exit 1 fi - tarball="${RUNNER_TEMP}/${name}" # Not retried, unlike the download. A blip and a mismatch are not the # same event: one is the network, the other is the registry handing # back bytes nobody signed off on, and retrying that just asks again # until it succeeds. if [ -n "${want}" ]; then - got="sha512-$(openssl dgst -sha512 -binary "${tarball}" | openssl base64 -A)" + got="sha512-$(openssl dgst -sha512 -binary "${RUNNER_TEMP}/${name}" | openssl base64 -A)" if [ "${got}" != "${want}" ]; then echo "::error::ThreatCrush integrity mismatch for ${spec}" echo "::error::expected ${want}" @@ -137,35 +120,30 @@ jobs: echo "::warning::no integrity hash pinned for ${spec}; installing unverified" fi - npm install -g --ignore-scripts "${tarball}" + npm install -g --ignore-scripts "${RUNNER_TEMP}/${name}" - # Recorded into every run log so a release that changes the interface - # shows up immediately, rather than silently scoring zero. - - name: Record the CLI interface - run: | - threatcrush --version || true - threatcrush scan --help || true + # Recorded so a release that changed the interface is visible in the + # log rather than inferred from a confusing failure downstream. + threatcrush --version # The CLI emits SARIF itself, so this asks for it and nothing converts # anything. # - # There used to be a second path here: a capability probe on `--format`, - # and a 235-line Python converter that parsed the terminal output when - # the probe said no. Both are gone, because the premise stopped holding. - # `threatcrushPackageSpec` pins an exact version and the step above - # refuses to install any other bytes, so "which interface does the - # installed CLI have" is answered by the pack, not discovered at - # runtime — the probe could only ever say yes. + # There used to be a capability probe here and a 235-line Python converter + # that parsed the CLI's terminal output when the probe said no. Both are + # gone, because the premise stopped holding: `threatcrushPackageSpec` pins + # an exact version and the install step refuses any other bytes, so "which + # interface does the installed CLI have" is answered by the pack, not + # discovered at runtime. # - # Deleting it is a security change more than a tidying one. The - # converter reconstructed findings by regex out of a display format that - # is free to change, which is a silent-undercount waiting to happen; and - # every file a pack installs into somebody else's repository is surface - # they have to review. This one now installs a single workflow. + # Nothing is lost by not probing. A CLI without `--format` writes no SARIF + # file, and the check below turns that into a hard failure that says the + # diff was not scanned — which is the same answer the probe gave, from + # evidence rather than from asking. - name: Scan id: scan run: | - set -o pipefail + set -euo pipefail FAIL_ON="{{failOn}}" SCAN_PATH="{{scanPath}}" code=0 @@ -187,12 +165,12 @@ jobs: exit 1 fi - case "$code" in + case "${code}" in 0) echo "status=clean" >> "$GITHUB_OUTPUT" ;; - # Exit 1 *with* a SARIF file is the documented "findings at or - # above --fail-on" result. Without one it was caught above. The CLI - # only returns 1 when --fail-on was passed, so propagate it: a gate - # that records the finding and then lets the job pass is not a gate. + # Exit 1 *with* a SARIF file is the documented "findings at or above + # --fail-on" result, and the CLI only returns it when --fail-on was + # passed. Propagate it: a gate that records the finding and then + # lets the job pass is not a gate. 1) echo "status=findings" >> "$GITHUB_OUTPUT" exit 1 @@ -200,191 +178,110 @@ jobs: *) echo "status=error" >> "$GITHUB_OUTPUT" echo "::error::ThreatCrush scan failed with exit code ${code} — results may be incomplete" - exit "$code" + exit "${code}" ;; esac - # 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' - && (steps.scan.outputs.status == 'clean' || steps.scan.outputs.status == 'findings') - && hashFiles('threatcrush.sarif') != '' - continue-on-error: true - uses: github/codeql-action/upload-sarif@f3712979fa5f215279b101dd0a2e3bdfb4353324 # v3 - with: - sarif_file: threatcrush.sarif - category: threatcrush - - name: Build the report if: always() + env: + SCAN_STATUS: ${{ steps.scan.outputs.status }} + # Empty when the changed-file step was skipped or found no merge ref, + # which reads as "not scoped" and reports everything. + SCAN_SCOPED: ${{ steps.changed.outputs.scoped }} run: | - python3 << 'PYEOF' - import json, os + set -euo pipefail + python3 - <<'PY' > "${RUNNER_TEMP}/threatcrush-report.md" + import json, os, sys + + LABELS = {"error": "HIGH", "warning": "MEDIUM", "note": "LOW"} + # Most serious first. SARIF order is file order, so the row cap would + # otherwise be decided by where a finding happens to sit in the tree — + # a HIGH in the last file cut while fifty LOWs from the first print. + RANK = {"error": 0, "warning": 1, "note": 2} + + def locate(result): + # SARIF permits a result with no locations; indexing [0] unguarded + # threw, and the run reported "could not be read" instead of the + # findings it actually had. + where = (result.get("locations") or [{}])[0].get("physicalLocation", {}) + return (where.get("artifactLocation", {}).get("uri", ""), + where.get("region", {}).get("startLine", 1)) + + def tally(rows): + seen = {k: sum(1 for r in rows if r.get("level", "warning") == k) for k in LABELS} + return " | ".join(f"**{LABELS[k]}**: {seen[k]}" for k in LABELS if seen[k]) + + def table(rows, limit): + out = ["| Severity | Rule | Location |", "|---|---|---|"] + for result in rows[:limit]: + uri, line = locate(result) + where = f"`{uri}`:{line}" if uri else "_(no location)_" + out.append(f"| {LABELS.get(result.get('level', 'warning'), 'INFO')} " + f"| `{result.get('ruleId', '?')}` | {where} |") + if len(rows) > limit: + # Say so. A silent truncation reads as "that was everything". + out += ["", f"_…and {len(rows) - limit} more; the full set is in the SARIF artifact._"] + return out status = os.environ.get("SCAN_STATUS", "") try: - with open("threatcrush.sarif") as handle: - results = json.load(handle)["runs"][0]["results"] + results = json.load(open("threatcrush.sarif"))["runs"][0]["results"] except Exception as err: + # stderr, not stdout: stdout is the report file. + print(f"::warning::could not read SARIF: {err}", file=sys.stderr) results = None - print(f"::warning::could not read SARIF: {err}") - lines = ["## ThreatCrush Security Scan", ""] + out = ["## ThreatCrush Security Scan", ""] - # Fail closed: render findings only on positive evidence that a scan - # completed. Testing for `status == "error"` was fail-open and got - # caught immediately — when the capability check failed, the scan - # step was *skipped*, so `status` was the empty string rather than - # "error", and the comment cheerfully reported "0 findings" for a - # scan that never started. Any state that is not a known-good - # outcome is NOT RUN. + # Fail closed. `status` is the empty string when an earlier step failed + # and the scan was *skipped*, and an earlier version read that as + # "no findings" — a clean report on a diff nothing had examined. Any + # state that is not a known-good outcome is NOT RUN. if status not in ("clean", "findings") or results is None: - # Never render "no issues found" for a scan that did not finish. - # An unexamined diff is not a clean one, and the two are - # indistinguishable to whoever reads the comment. - lines += [ - "**NOT RUN** — the scan did not complete, so this diff was not examined.", - "This is not a clean result. See the job log.", - ] + out += ["**NOT RUN** — the scan did not complete, so this diff was not examined.", + "This is not a clean result. See the job log."] + elif not results: + out.append("No findings.") else: - LABELS = {"error": "HIGH", "warning": "MEDIUM", "note": "LOW"} - # Most serious first. The old order was SARIF's, which is file - # order — so the 50-row cap was decided by where a finding sat in - # the tree, and a `high` in the last file scanned could be cut - # while fifty `note`s from the first file were printed in full. - RANK = {"error": 0, "warning": 1, "note": 2} - - def locate(result): - locations = result.get("locations") or [] - physical = (locations[0] if locations else {}).get("physicalLocation", {}) - uri = physical.get("artifactLocation", {}).get("uri", "") - return uri, physical.get("region", {}).get("startLine", 1) - - def tally(rows): - counts = {"error": 0, "warning": 0, "note": 0} - for row in rows: - level = row.get("level", "warning") - if level in counts: - counts[level] += 1 - return counts - - def badges(counts): - out = [] - if counts["error"]: - out.append(f"**HIGH/CRITICAL**: {counts['error']}") - if counts["warning"]: - out.append(f"**MEDIUM**: {counts['warning']}") - if counts["note"]: - out.append(f"**LOW**: {counts['note']}") - return " | ".join(out) - - def table(rows, limit): - out = ["| Severity | Rule | Location |", "|---|---|---|"] - for result in rows[:limit]: - # SARIF permits a result with no locations, and the native - # --format sarif path is written by the CLI rather than by - # the converter beside this file. Indexing [0] there threw - # out of the enclosing try, so the report file was never - # written and the comment fell back to "could not be read" - # — a message that hides real findings behind a wrong one. - uri, line_no = locate(result) - label = LABELS.get(result.get("level", "warning"), "INFO") - where = f"`{uri}`:{line_no}" if uri else "_(no location)_" - out.append(f"| {label} | `{result.get('ruleId','?')}` | {where} |") - if len(rows) > limit: - # Say so. A silent truncation reads as "that was everything". - out += ["", f"_…and {len(rows) - limit} more. Full results in the Security tab._"] - return out - try: - with open(os.environ["RUNNER_TEMP"] + "/threatcrush-changed.txt") as handle: - changed = {entry.strip() for entry in handle if entry.strip()} + changed = set(open(os.environ["RUNNER_TEMP"] + "/threatcrush-changed.txt").read().split()) except Exception: changed = set() scoped = os.environ.get("SCAN_SCOPED", "") == "true" results.sort(key=lambda r: (RANK.get(r.get("level", "warning"), 3), locate(r))) - - if scoped: - touched = [r for r in results if locate(r)[0] in changed] - backlog = [r for r in results if locate(r)[0] not in changed] - else: - touched, backlog = results, [] - - if not results: - lines.append("No findings.") - else: - if scoped: - lines += [ - f"**{len(touched)}** finding(s) in the {len(changed)} file(s) this " - "pull request changes.", - "", - ] - else: - lines += [f"**{len(results)}** finding(s)", ""] - - if touched: - badge_line = badges(tally(touched)) - if badge_line: - lines += [badge_line, ""] - lines += table(touched, 50) - elif scoped: - lines.append("Nothing in the files this pull request changes.") - - # The rest of the repository is reported, but not *at* the - # author of an unrelated change. It is a standing backlog, it - # was there before this branch, and it belongs behind a fold - # — not in ninety rows above the review. - if backlog: - summary = badges(tally(backlog)) or "no severities" - lines += [ - "", - "
", - f"{len(backlog)} pre-existing finding(s) elsewhere in the " - f"repository — {summary}", - "", - "Not introduced by this pull request. The full set is in the " - "Security tab.", - "", - ] - lines += table(backlog, 20) - lines += ["", "
"] - - lines += [ - "", - "Snippets are redacted; ThreatCrush never prints matched credential material.", - ] - - with open(os.environ["RUNNER_TEMP"] + "/threatcrush-comment.md", "w") as handle: - handle.write("\n".join(lines) + "\n") - PYEOF - env: - SCAN_STATUS: ${{ steps.scan.outputs.status }} - # Empty when the changed-file step was skipped or could not identify - # a merge ref, which reads as "not scoped" and reports everything. - SCAN_SCOPED: ${{ steps.changed.outputs.scoped }} - - - name: Write report to job summary - 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. + touched = [r for r in results if locate(r)[0] in changed] if scoped else results + backlog = [r for r in results if locate(r)[0] not in changed] if scoped else [] + + out += [f"**{len(touched)}** finding(s) in the {len(changed)} file(s) this pull request changes." + if scoped else f"**{len(results)}** finding(s)", ""] + + if touched: + badges = tally(touched) + out += ([badges, ""] if badges else []) + table(touched, 50) + elif scoped: + out.append("Nothing in the files this pull request changes.") + + # The rest of the repository is reported, but not *at* the author + # of an unrelated change. It is a standing backlog, it was there + # before this branch, and it belongs behind a fold. + if backlog: + out += ["", "
", + f"{len(backlog)} pre-existing finding(s) elsewhere in the repository" + f" — {tally(backlog) or 'no severities'}", "", + "Not introduced by this pull request.", ""] + out += table(backlog, 20) + ["", "
"] + + out += ["", "Snippets are redacted; ThreatCrush never prints matched credential material."] + + print("\n".join(out)) + PY + cat "${RUNNER_TEMP}/threatcrush-report.md" >> "$GITHUB_STEP_SUMMARY" + + # if-no-files-found: ignore, because nothing synthesises the file. 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@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 @@ -393,16 +290,35 @@ jobs: path: threatcrush.sarif if-no-files-found: ignore retention-days: 30 +{{#if uploadSarif}} - # Best-effort. `pull_request` gives fork PRs a read-only token, so this - # 403s on fork submissions — the report is in the job summary either way, - # and the scan's pass/fail is decided by the scan step, not by whether a - # comment posted. Deliberately NOT switching to pull_request_target to - # get a writable token: that event runs with repository secrets in scope - # against a checkout of untrusted contributor code. + # Never on a failed or empty run. 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. + - name: Upload to the Security tab + if: >- + always() + && (steps.scan.outputs.status == 'clean' || steps.scan.outputs.status == 'findings') + && hashFiles('threatcrush.sarif') != '' + continue-on-error: true + uses: github/codeql-action/upload-sarif@f3712979fa5f215279b101dd0a2e3bdfb4353324 # v3 + with: + sarif_file: threatcrush.sarif + category: threatcrush +{{/if}} +{{#if commentOnPr}} + + # Best-effort. `pull_request` gives fork pull requests a read-only token, + # so this 403s on fork submissions — the report is in the job summary + # either way, and pass/fail is decided by the scan step, not by whether a + # comment posted. Deliberately NOT pull_request_target to get a writable + # token: that event runs with repository secrets in scope against a + # checkout of untrusted contributor code. - name: Comment on PR if: >- - always() && '{{commentOnPr}}' == 'true' + always() && github.event.pull_request.head.repo.full_name == github.repository && github.actor != 'dependabot[bot]' continue-on-error: true @@ -412,7 +328,7 @@ jobs: const fs = require('fs'); let body; try { - body = fs.readFileSync(`${process.env.RUNNER_TEMP}/threatcrush-comment.md`, 'utf8'); + body = fs.readFileSync(`${process.env.RUNNER_TEMP}/threatcrush-report.md`, 'utf8'); } catch { body = '## ThreatCrush Security Scan\n\nScan completed but the report could not be read.'; } @@ -420,9 +336,9 @@ jobs: try { // 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. + // report falls off the page, is not found, and every 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, @@ -432,21 +348,12 @@ jobs: const existing = comments.find( (c) => c.user.type === 'Bot' && c.body.includes('ThreatCrush Security Scan'), ); + const target = { owner: context.repo.owner, repo: context.repo.repo, body }; if (existing) { - await github.rest.issues.updateComment({ - comment_id: existing.id, - owner: context.repo.owner, - repo: context.repo.repo, - body, - }); + await github.rest.issues.updateComment({ ...target, comment_id: existing.id }); } else { - await github.rest.issues.createComment({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - body, - }); + await github.rest.issues.createComment({ ...target, issue_number: context.issue.number }); } } catch (err) { core.warning( @@ -454,3 +361,4 @@ jobs: 'Findings are in the job summary.', ); } +{{/if}}