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}}