From eee70587caafd5c5d54ac8dd8f2bbe9ca716ec16 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 3 Aug 2026 09:42:32 +0000 Subject: [PATCH 1/6] feat(actions): add the threatcrush-scan pack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runs ThreatCrush over pull requests and uploads SARIF to the Security tab. Needs no secrets — the scanner is local to the runner — which is what makes it installable fleet-wide without provisioning anything first. Report-only by default. `failOn` is empty on purpose: a repository with pre-existing findings should get a report on its first install, not a blocked pull request. A gate that fires on everything gets switched off within a day, and a gate that is off is worse than one never installed. The scan step distinguishes the two non-clean endings. Exit 1 is "findings at or above failOn" — a result. Exit 2 is "the scan itself failed" — not a result, and the comment says NOT RUN rather than rendering an empty findings table, because an unexamined diff is indistinguishable from a clean one to whoever reads it. The empty-SARIF fallback exists only so the upload does not fail on a missing file and bury the real error. Install retries three times before giving up, for the same reason vu1nz-scan now does: a transient registry blip is not a security signal. Deliberately not pull_request_target — that event runs with repository secrets in scope against a checkout of untrusted contributor code. The comment step 403s on fork PRs instead, and is continue-on-error; the report is in the job summary and the artifact regardless. Asserted in a test so it cannot regress. Co-Authored-By: Claude Opus 5 (1M context) --- packages/actions/src/index.test.ts | 58 +++++ packages/actions/threatcrush-scan/README.md | 74 ++++++ .../threatcrush-scan/sh1pt.actionpack.yaml | 69 ++++++ .../actions/threatcrush-scan/workflow.yml | 227 ++++++++++++++++++ 4 files changed, 428 insertions(+) create mode 100644 packages/actions/threatcrush-scan/README.md create mode 100644 packages/actions/threatcrush-scan/sh1pt.actionpack.yaml create mode 100644 packages/actions/threatcrush-scan/workflow.yml diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index eb138cb0..e4665d05 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -28,6 +28,18 @@ describe('built-in packs', () => { expect(entry?.manifest.secrets[0]?.name).toBe('ENV_FILE'); }); + it('loads the threatcrush-scan pack', async () => { + const catalog = await loadBuiltinPacks(); + const entry = catalog.get('threatcrush-scan'); + expect(entry).toBeDefined(); + expect(entry?.manifest.name).toBe('ThreatCrush Security Scan'); + expect(entry?.manifest.files[0]?.destination).toBe('.github/workflows/threatcrush-scan.yml'); + // No secrets: the scanner is entirely local to the runner. A pack that + // needs no credentials is a pack that can be installed fleet-wide without + // provisioning anything first. + expect(entry?.manifest.secrets).toHaveLength(0); + }); + it('loads the coinpay-invoice pack', async () => { const catalog = await loadBuiltinPacks(); const entry = catalog.get('coinpay-invoice'); @@ -145,4 +157,50 @@ describe('built-in packs', () => { expect(file?.content).toContain('${{ github.repository }}'); expect(file?.content).toContain('# Managed by sh1pt Actions Fleet'); }); + + it('renders threatcrush-scan report-only by default', async () => { + 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: {}, + }); + const file = result.files[0]; + expect(file?.destination).toBe('.github/workflows/threatcrush-scan.yml'); + expect(file?.content).toContain('node-version: "20"'); + expect(file?.content).toContain('--format sarif --output threatcrush.sarif'); + // Empty by default, so a first install reports rather than blocks. + expect(file?.content).toContain('FAIL_ON=""'); + expect(file?.content).toContain('# Managed by sh1pt Actions Fleet'); + }); + + it('renders threatcrush-scan with a failure gate when asked', async () => { + 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: { failOn: 'critical,high' }, + }); + expect(result.files[0]?.content).toContain('FAIL_ON="critical,high"'); + }); + + it('never uses pull_request_target', async () => { + // That event runs with repository secrets in scope; combined with a + // checkout of the PR head it executes untrusted contributor code with + // access to them. Asserted rather than documented so it cannot regress. + 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: {}, + }); + expect(result.files[0]?.content).not.toContain('pull_request_target'); + expect(entry.manifest.security.allowPullRequestTarget).toBe(false); + }); }); diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md new file mode 100644 index 00000000..e81d3692 --- /dev/null +++ b/packages/actions/threatcrush-scan/README.md @@ -0,0 +1,74 @@ +# threatcrush-scan + +Runs [ThreatCrush](https://threatcrush.com) over pull requests: hardcoded +credentials, injection, SSRF, unsafe deserialisation, XXE, and dependency +tampering. Results go to the GitHub Security tab as SARIF and to a PR comment. + +```bash +sh1pt actions install threatcrush-scan --repo owner/name --pr +``` + +## Inputs + +| Input | Default | Notes | +| --- | --- | --- | +| `scanPath` | `.` | Path to scan, relative to the repository root. | +| `nodeVersion` | `20` | See *Node 20, deliberately*, below. | +| `threatcrushPackageSpec` | `@profullstack/threatcrush@latest` | npm spec used to install the CLI. | +| `failOn` | *(empty)* | Comma-separated severities that fail the job, e.g. `critical,high`. Empty is report-only. | +| `uploadSarif` | `true` | Upload to the Security tab. | + +## Report-only by default + +`failOn` is empty on purpose. A repository with pre-existing findings should +get a report on its first install, not a blocked pull request — a gate that +fires on everything gets switched off within a day, and a gate that is off is +worse than one that was never installed. Tighten it to `critical,high` once the +backlog is triaged. + +## Exit codes are distinguished + +The scan step separates the two ways a scan can end without being clean: + +- **`1`** — findings at or above `failOn`. A result. Reported, and the job + fails if you asked it to. +- **`2`** — the scan itself failed. **Not** a result. The job fails and the + comment says `NOT RUN`, because an unexamined diff is not a clean one and + the two are indistinguishable to whoever reads the comment. + +The same reasoning drives the *Ensure SARIF exists* step. It writes a valid +empty run only so the upload does not fail on a missing file and bury the real +error; it never converts a failed scan into a clean-looking one. + +## Node 20, deliberately + +The CLI depends on `better-sqlite3`, a native module. Node 20 is the newest +runtime with reliable prebuilt binaries for it — newer runtimes fall through to +a `node-gyp` source build that fails without a full toolchain. If you raise +`nodeVersion`, verify the install still succeeds before trusting a run. + +## Fork pull requests + +`pull_request` gives fork PRs a read-only `GITHUB_TOKEN`, so the comment step +403s on fork submissions. It is `continue-on-error`, and the report is in the +job summary and the uploaded artifact regardless. + +This pack deliberately does **not** use `pull_request_target` to obtain a +writable token. That event runs with repository secrets in scope, and combined +with a checkout of the PR head it executes untrusted contributor code with +access to those secrets. If comments on fork PRs are required, add a separate +`workflow_run`-triggered job that downloads the artifact and comments — it +never checks out untrusted code. + +## Coverage + +Scored against [`profullstack/malware-test-prs`][testbed]: **90.32%** true +positive rate at a **0.0%** false positive rate against its `SAFE:` control +group, with zero unattributed findings. See `docs/SCANNING.md` in the +threatcrush repository for the method, the confidence model, and the four +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. + +[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 new file mode 100644 index 00000000..3282cb40 --- /dev/null +++ b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml @@ -0,0 +1,69 @@ +schemaVersion: 1 +id: threatcrush-scan +name: ThreatCrush Security Scan +description: >- + Scans pull requests for hardcoded credentials, injection, SSRF, unsafe + deserialisation and dependency tampering, and uploads SARIF to the Security + tab. +version: 1.0.0 +publisher: profullstack +visibility: public +license: MIT +categories: + - security + - ci +compatibility: + providers: + - github +pricing: + type: free +inputs: + scanPath: + type: string + default: '.' + description: Path to scan, relative to the repository root. + nodeVersion: + type: string + default: '20' + description: >- + Node version used to install the CLI. 20 deliberately — the CLI depends + on better-sqlite3, and 20 is the newest runtime with reliable prebuilt + binaries for it. Newer runtimes fall through to a node-gyp source build + that fails without a full toolchain. + threatcrushPackageSpec: + type: string + default: '@profullstack/threatcrush@latest' + description: npm spec used to install the CLI. + failOn: + type: string + default: '' + description: >- + Comma-separated severities that fail the job (e.g. "critical,high"). + Empty means report-only, which is the right default for a first install: + a repository with pre-existing findings should get a report, not a + blocked pull request. Tighten it once the backlog is triaged. + uploadSarif: + type: string + default: 'true' + enum: + - 'true' + - 'false' + description: >- + Upload results to the GitHub Security tab. Requires code scanning, which + is free on public repositories and needs Advanced Security on private + ones. The workflow skips the upload gracefully when it is unavailable. +secrets: [] +repoVariables: [] +files: + - source: workflow.yml + destination: .github/workflows/threatcrush-scan.yml + mergeStrategy: replace-managed +policies: + installMode: pull-request + managedComment: true + requiresReview: true +security: + leastPrivilegePermissions: true + pinThirdPartyActions: optional + allowPullRequestTarget: false + defaultTimeoutMinutes: 15 diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml new file mode 100644 index 00000000..0941e369 --- /dev/null +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -0,0 +1,227 @@ +name: threatcrush security scan + +on: + pull_request: + +permissions: + contents: read + pull-requests: write + security-events: write + +jobs: + scan: + name: Scan for credentials and vulnerable patterns + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + 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. + - name: Install ThreatCrush + run: | + for attempt in 1 2 3; do + if npm install -g "{{threatcrushPackageSpec}}"; then + exit 0 + fi + delay=$((attempt * 10)) + echo "::warning::ThreatCrush install attempt ${attempt}/3 failed; retrying in ${delay}s" + sleep "${delay}" + done + echo "::error::ThreatCrush install failed after 3 attempts" + exit 1 + + # 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 + + - name: Scan + id: scan + run: | + set -o pipefail + FAIL_ON="{{failOn}}" + ARGS=(scan "{{scanPath}}" --format sarif --output threatcrush.sarif) + if [ -n "$FAIL_ON" ]; then + ARGS+=(--fail-on "$FAIL_ON") + fi + + if threatcrush "${ARGS[@]}"; then + echo "status=clean" >> "$GITHUB_OUTPUT" + else + code=$? + # 1 is "findings at or above --fail-on" — a result. 2 is "the scan + # itself failed" — not a result, and it must not be reported as a + # clean bill of health. + if [ "$code" -eq 1 ]; then + echo "status=findings" >> "$GITHUB_OUTPUT" + else + echo "status=error" >> "$GITHUB_OUTPUT" + echo "::error::ThreatCrush scan failed with exit code ${code} — this diff was NOT scanned" + exit "$code" + fi + fi + + # The scan writes SARIF whenever it completes. If it did not, the upload + # below would fail on a missing file and bury the real error, so write a + # valid empty run and let the job's own exit code carry the failure. + - 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 + + - name: Upload to the Security tab + if: always() && '{{uploadSarif}}' == 'true' + continue-on-error: true + uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: threatcrush.sarif + category: threatcrush + + - name: Build the report + if: always() + run: | + python3 << 'PYEOF' + import json, os + + status = os.environ.get("SCAN_STATUS", "") + try: + with open("threatcrush.sarif") as handle: + results = json.load(handle)["runs"][0]["results"] + except Exception as err: + results = None + print(f"::warning::could not read SARIF: {err}") + + lines = ["## ThreatCrush Security Scan", ""] + + if status == "error" 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.", + ] + else: + counts = {"error": 0, "warning": 0, "note": 0} + for result in results: + level = result.get("level", "warning") + if level in counts: + counts[level] += 1 + + lines.append(f"**{len(results)}** finding(s)") + lines.append("") + + if results: + badges = [] + if counts["error"]: + badges.append(f"**HIGH/CRITICAL**: {counts['error']}") + if counts["warning"]: + badges.append(f"**MEDIUM**: {counts['warning']}") + if counts["note"]: + badges.append(f"**LOW**: {counts['note']}") + if badges: + lines += [" | ".join(badges), ""] + + lines += ["| Severity | Rule | Location |", "|---|---|---|"] + for result in results[:50]: + location = result["locations"][0]["physicalLocation"] + uri = location["artifactLocation"]["uri"] + line_no = location.get("region", {}).get("startLine", 1) + label = {"error": "HIGH", "warning": "MEDIUM", "note": "LOW"}.get( + result.get("level", "warning"), "INFO" + ) + lines.append(f"| {label} | `{result.get('ruleId','?')}` | `{uri}`:{line_no} |") + if len(results) > 50: + # Say so. A silent truncation reads as "that was everything". + lines += ["", f"_…and {len(results) - 50} more. Full results in the Security tab._"] + lines += ["", "Snippets are redacted; ThreatCrush never prints matched credential material."] + else: + lines.append("No findings.") + + 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 }} + + - name: Write report to job summary + if: always() + run: cat "$RUNNER_TEMP/threatcrush-comment.md" >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true + + - name: Upload SARIF artifact + if: always() + uses: actions/upload-artifact@v4 + with: + name: threatcrush-sarif + path: threatcrush.sarif + retention-days: 30 + + # 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. + - 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 + with: + script: | + const fs = require('fs'); + let body; + try { + body = fs.readFileSync(`${process.env.RUNNER_TEMP}/threatcrush-comment.md`, 'utf8'); + } catch { + body = '## ThreatCrush Security Scan\n\nScan completed but the report could not be read.'; + } + + try { + const { data: comments } = await github.rest.issues.listComments({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + }); + const existing = comments.find( + (c) => c.user.type === 'Bot' && c.body.includes('ThreatCrush Security Scan'), + ); + + if (existing) { + await github.rest.issues.updateComment({ + comment_id: existing.id, + owner: context.repo.owner, + repo: context.repo.repo, + body, + }); + } else { + await github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body, + }); + } + } catch (err) { + core.warning( + `Could not post PR comment (status ${err.status ?? 'unknown'}): ${err.message}. ` + + 'Findings are in the job summary.', + ); + } From b9b3ab886786e9de706c0bf5ddbb4401e90cb55b Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 3 Aug 2026 10:01:16 +0000 Subject: [PATCH 2/6] fix(actions/threatcrush-scan): fail closed when the CLI cannot emit SARIF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught on a real install. moshcoder/moshpit-name run 30803607991 pulled the published 0.2.2, which has no --format. The scan died with `error: unknown option '--format'` and commander exited 1 — the same code the CLI uses for "findings at or above --fail-on" — so the step recorded a result, no SARIF was written, the empty-run fallback supplied one, and the PR comment said "0 findings". A green check on a repository that was never scanned: the exact failure the pack's other three guards exist to prevent, arriving through the one path none of them covered. Exit codes cannot separate "argument rejected" from "findings found", so stop trying. Check the interface before scanning and refuse to run without it, and treat the SARIF file as the only evidence a scan happened — absent or empty is an error whatever the process returned. Also fixes the gate itself: exit 1 with findings recorded status and then let the step succeed, so failOn would have reported findings without ever failing a pull request. A gate that does not gate is worse than none, because it is believed. Verified by replaying all four outcomes against stub CLIs reproducing the observed 0.2.2 behaviour: no SARIF -> exit 1 status=error; clean -> exit 0 status=clean; findings -> exit 1 status=findings; crash -> exit 2 status=error. Co-Authored-By: Claude Opus 5 (1M context) --- packages/actions/src/index.test.ts | 23 +++++++ packages/actions/threatcrush-scan/README.md | 19 ++++++ .../threatcrush-scan/sh1pt.actionpack.yaml | 2 +- .../actions/threatcrush-scan/workflow.yml | 67 ++++++++++++++----- 4 files changed, 95 insertions(+), 16 deletions(-) diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index e4665d05..1e02c838 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -188,6 +188,29 @@ describe('built-in packs', () => { expect(result.files[0]?.content).toContain('FAIL_ON="critical,high"'); }); + it('refuses to run against a CLI that cannot emit SARIF', async () => { + // Regression: moshcoder/moshpit-name run 30803607991 installed the + // published 0.2.2, which has no --format. The scan died with + // `unknown option '--format'` and commander exited 1 — the same code the + // CLI uses for "findings at or above --fail-on" — so the step read a + // failure as a result and the PR comment said "0 findings". Green check, + // nothing scanned. Exit codes cannot separate those two cases, so the + // interface is checked up front and the SARIF file is treated as the only + // evidence a scan happened. + 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: {}, + }); + const content = result.files[0]?.content ?? ''; + expect(content).toContain("grep -q -- '--format'"); + expect(content).toContain('if [ ! -s threatcrush.sarif ]; then'); + expect(content).toContain('this diff was NOT scanned'); + }); + it('never uses pull_request_target', async () => { // That event runs with repository secrets in scope; combined with a // checkout of the PR head it executes untrusted contributor code with diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md index e81d3692..4dcc182a 100644 --- a/packages/actions/threatcrush-scan/README.md +++ b/packages/actions/threatcrush-scan/README.md @@ -26,6 +26,22 @@ fires on everything gets switched off within a day, and a gate that is off is worse than one that was never installed. Tighten it to `critical,high` once the backlog is triaged. +## Requires a CLI with native SARIF + +The workflow checks `threatcrush scan --help` for `--format` before scanning, +and fails the job if it is absent. + +This is not defensiveness for its own sake. The published `0.2.2` has no +`--format`: the scan died with `error: unknown option '--format'`, commander +exited `1` — the same code the CLI uses for *findings at or above `failOn`* — +and the step read a failure as a result. No SARIF was written, the empty-run +fallback supplied one, and the PR comment said **0 findings**. A green check on +a repository that was never scanned. + +Exit codes cannot separate "argument rejected" from "findings found", so the +pack does not try. It verifies the interface up front, and treats the SARIF +file as the only evidence that a scan actually happened. + ## Exit codes are distinguished The scan step separates the two ways a scan can end without being clean: @@ -36,6 +52,9 @@ The scan step separates the two ways a scan can end without being clean: comment says `NOT RUN`, because an unexamined diff is not a clean one and the two are indistinguishable to whoever reads the comment. +A missing or empty `threatcrush.sarif` is treated as `2` regardless of what the +process returned. + The same reasoning drives the *Ensure SARIF exists* step. It writes a valid empty run only so the upload does not fail on a missing file and bury the real error; it never converts a failed scan into a clean-looking one. diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index 3282cb40..dc0b40b2 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.0.0 +version: 1.0.1 publisher: profullstack visibility: public license: MIT diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index 0941e369..ba8d3366 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -45,6 +45,27 @@ jobs: threatcrush --version || true threatcrush scan --help || true + # Fail closed when the installed CLI cannot do what this workflow asks. + # + # Found the hard way. `@profullstack/threatcrush@0.2.2` has no `--format`, + # so the scan died with `error: unknown option '--format'` and commander + # exited 1 — the same code the CLI uses for "findings at or above + # --fail-on". The step read that as a result, no SARIF was written, the + # empty-run fallback below supplied one, and the PR comment said + # "0 findings". Green check, nothing scanned. + # + # Exit codes alone cannot separate "argument rejected" from "findings + # found", so do not try. Check the interface up front instead, and refuse + # to run a scan whose output we would not be able to trust. + - name: Verify the CLI supports SARIF + run: | + if ! threatcrush scan --help 2>&1 | grep -q -- '--format'; then + echo "::error::The installed ThreatCrush CLI ($(threatcrush --version 2>/dev/null || echo 'unknown')) does not support --format." + echo "::error::This workflow needs native SARIF output. Upgrade the CLI, or pin threatcrushPackageSpec to a version that has it." + echo "::error::Refusing to run: a scan whose output cannot be read would report 0 findings, which is indistinguishable from a clean result." + exit 1 + fi + - name: Scan id: scan run: | @@ -55,25 +76,41 @@ jobs: ARGS+=(--fail-on "$FAIL_ON") fi - if threatcrush "${ARGS[@]}"; then - echo "status=clean" >> "$GITHUB_OUTPUT" - else - code=$? - # 1 is "findings at or above --fail-on" — a result. 2 is "the scan - # itself failed" — not a result, and it must not be reported as a - # clean bill of health. - if [ "$code" -eq 1 ]; then + code=0 + threatcrush "${ARGS[@]}" || code=$? + + # The SARIF file is the evidence that a scan happened, and it is the + # only evidence worth trusting. An exit code says what the process + # thought; the file says what it produced. Absent the file there is + # nothing to report, and reporting nothing as "no findings" is the + # failure this whole workflow is arranged to avoid. + if [ ! -s threatcrush.sarif ]; then + echo "status=error" >> "$GITHUB_OUTPUT" + echo "::error::ThreatCrush produced no SARIF (exit ${code}) — this diff was NOT scanned" + exit 1 + fi + + 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. + 1) echo "status=findings" >> "$GITHUB_OUTPUT" - else + exit 1 + ;; + *) echo "status=error" >> "$GITHUB_OUTPUT" - echo "::error::ThreatCrush scan failed with exit code ${code} — this diff was NOT scanned" + echo "::error::ThreatCrush scan failed with exit code ${code} — results may be incomplete" exit "$code" - fi - fi + ;; + esac - # The scan writes SARIF whenever it completes. If it did not, the upload - # below would fail on a missing file and bury the real error, so write a - # valid empty run and let the job's own exit code carry the failure. + # 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: | From 3a77b9c2537b892084e38f71793fa35c2daf863a Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 3 Aug 2026 10:02:53 +0000 Subject: [PATCH 3/6] fix(actions/threatcrush-scan): make the PR comment fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The capability check added in the previous commit worked — the job went red and the scan was skipped — and the PR comment still said "0 findings". The report tested `status == "error"`, but a skipped step yields an empty string, not "error", so the one branch meant to catch this fell through to the happy path. Render findings only on positive evidence of a completed scan ("clean" or "findings"). Every other state, including states that do not exist yet, is NOT RUN. Verified across all four statuses: '' and 'error' render NOT RUN, 'clean' and 'findings' render the results table. Co-Authored-By: Claude Opus 5 (1M context) --- packages/actions/src/index.test.ts | 5 +++++ packages/actions/threatcrush-scan/sh1pt.actionpack.yaml | 2 +- packages/actions/threatcrush-scan/workflow.yml | 9 ++++++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index 1e02c838..69ad3982 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -209,6 +209,11 @@ describe('built-in packs', () => { expect(content).toContain("grep -q -- '--format'"); expect(content).toContain('if [ ! -s threatcrush.sarif ]; then'); expect(content).toContain('this diff was NOT scanned'); + // And the report must be fail-closed. Testing for status == "error" was + // fail-open: when the capability check fails the scan step is *skipped*, + // so status is the empty string, and the comment reported "0 findings" + // for a scan that never started. + expect(content).toContain('if status not in ("clean", "findings")'); }); it('never uses pull_request_target', async () => { diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index dc0b40b2..823b9723 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.0.1 +version: 1.0.2 publisher: profullstack visibility: public license: MIT diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index ba8d3366..57699281 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -148,7 +148,14 @@ jobs: lines = ["## ThreatCrush Security Scan", ""] - if status == "error" or results is None: + # 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. + 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. From ab44b30319616f5a7e5ee726b7a3278b94c70b1d Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 3 Aug 2026 10:28:18 +0000 Subject: [PATCH 4/6] feat(actions/threatcrush-scan): convert legacy output instead of refusing to run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The capability check added in 1.0.1 was right to fail closed, but it left every repository installing a scanner that refuses to scan until a new CLI is published. Correct, and useless. Detect the interface up front and branch on it: native --format when available, otherwise run the text scan and convert it. The converter fails closed — unrecognised output exits non-zero and writes nothing, so an unparseable scan still cannot arrive downstream looking clean. Written against the real captured output in the testbed's fixture, not against assumption, because three details of that format each break a naive parser: severity is bare for CRITICAL and bracketed for [HIGH]/[MEDIUM]/[LOW]; paths are relative to the scan root, so they resolve to nothing unprefixed; and whole-file findings report line 0, which SARIF rejects. Verified end to end against stub CLIs: legacy -> 9 findings converted from the real fixture, scoring 12.9% TPR / 0.0% FPR through the testbed's own validator, matching the published CLI's documented baseline; native -> uses --format untouched; legacy crash -> exit 1, status=error, no SARIF written. The legacy path is a stopgap. 0.2.2 is a secrets scanner; once a CLI with --format ships, the workflow switches automatically and coverage goes from 12.9% to 90.32%. Co-Authored-By: Claude Opus 5 (1M context) --- packages/actions/src/index.test.ts | 8 + packages/actions/threatcrush-scan/README.md | 49 ++-- .../threatcrush-scan/sh1pt.actionpack.yaml | 9 +- .../threatcrush-scan/threatcrush-to-sarif.py | 210 ++++++++++++++++++ .../actions/threatcrush-scan/workflow.yml | 66 ++++-- 5 files changed, 303 insertions(+), 39 deletions(-) create mode 100644 packages/actions/threatcrush-scan/threatcrush-to-sarif.py diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index 69ad3982..30897ae7 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -38,6 +38,11 @@ describe('built-in packs', () => { // needs no credentials is a pack that can be installed fleet-wide without // provisioning anything first. expect(entry?.manifest.secrets).toHaveLength(0); + // Workflow plus the legacy-output converter it falls back to. + expect(entry?.manifest.files.map((f) => f.destination)).toEqual([ + '.github/workflows/threatcrush-scan.yml', + '.github/threatcrush-to-sarif.py', + ]); }); it('loads the coinpay-invoice pack', async () => { @@ -208,6 +213,9 @@ describe('built-in packs', () => { const content = result.files[0]?.content ?? ''; expect(content).toContain("grep -q -- '--format'"); expect(content).toContain('if [ ! -s threatcrush.sarif ]; then'); + // A CLI without --format takes the converter path rather than failing the + // repo out of being scanned at all. + expect(content).toContain('.github/threatcrush-to-sarif.py'); expect(content).toContain('this diff was NOT scanned'); // And the report must be fail-closed. Testing for status == "error" was // fail-open: when the capability check fails the scan step is *skipped*, diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md index 4dcc182a..8a903af1 100644 --- a/packages/actions/threatcrush-scan/README.md +++ b/packages/actions/threatcrush-scan/README.md @@ -26,21 +26,40 @@ fires on everything gets switched off within a day, and a gate that is off is worse than one that was never installed. Tighten it to `critical,high` once the backlog is triaged. -## Requires a CLI with native SARIF - -The workflow checks `threatcrush scan --help` for `--format` before scanning, -and fails the job if it is absent. - -This is not defensiveness for its own sake. The published `0.2.2` has no -`--format`: the scan died with `error: unknown option '--format'`, commander -exited `1` — the same code the CLI uses for *findings at or above `failOn`* — -and the step read a failure as a result. No SARIF was written, the empty-run -fallback supplied one, and the PR comment said **0 findings**. A green check on -a repository that was never scanned. - -Exit codes cannot separate "argument rejected" from "findings found", so the -pack does not try. It verifies the interface up front, and treats the SARIF -file as the only evidence that a scan actually happened. +## Two output paths, chosen up front + +The workflow checks `threatcrush scan --help` for `--format` **before** +scanning, and picks accordingly: + +| CLI | Path | +| --- | --- | +| Has `--format` | Native SARIF. Preferred; nothing is parsed. | +| Older | Runs the text scan and converts it with `.github/threatcrush-to-sarif.py`. | + +The check happens up front because exit codes cannot tell the two failures +apart. The published `0.2.2` has no `--format`: the scan died with +`error: unknown option '--format'` and commander exited `1` — *the same code +the CLI uses for findings at or above `failOn`*. Read as a result, that +produced no SARIF, the empty-run fallback supplied one, and the PR comment +said **0 findings**. A green check on a repository that was never scanned. + +The converter **fails closed**: if it cannot recognise the output it exits +non-zero and writes nothing, dumping what it saw. Emitting empty SARIF instead +would report "0 findings", which is indistinguishable from a clean scan. + +Three details of the legacy format are load-bearing, and the converter is +tested against real captured output rather than assumption: + +- Severity is bare for `CRITICAL`, bracketed for `[HIGH]`/`[MEDIUM]`/`[LOW]`. + One regex shape misses half the findings. +- `File:` paths are relative to the scan root, not the repository root. + Unprefixed, every finding resolves to nothing in the consumer's view. +- Whole-file findings report line `:0`; SARIF requires `startLine >= 1`. + +**The legacy path is a stopgap, not a destination.** `0.2.2` is a secrets +scanner: it scores 12.9% against the testbed. Once a CLI with `--format` is +published the workflow switches to it automatically and coverage goes to +90.32%. ## Exit codes are distinguished diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index 823b9723..60a2e4bc 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.0.2 +version: 1.1.0 publisher: profullstack visibility: public license: MIT @@ -58,6 +58,13 @@ files: - source: workflow.yml destination: .github/workflows/threatcrush-scan.yml mergeStrategy: replace-managed + # Compatibility shim for CLI versions older than native `--format sarif`. + # Unused once the installed CLI can emit SARIF itself — the workflow picks + # the native path whenever it is available — but shipping it means a repo is + # scanned today rather than waiting on a release. + - source: threatcrush-to-sarif.py + destination: .github/threatcrush-to-sarif.py + mergeStrategy: replace-managed policies: installMode: pull-request managedComment: true diff --git a/packages/actions/threatcrush-scan/threatcrush-to-sarif.py b/packages/actions/threatcrush-scan/threatcrush-to-sarif.py new file mode 100644 index 00000000..e6c63207 --- /dev/null +++ b/packages/actions/threatcrush-scan/threatcrush-to-sarif.py @@ -0,0 +1,210 @@ +#!/usr/bin/env python3 +"""Convert ThreatCrush terminal output to SARIF 2.1.0. + +Compatibility shim for CLI versions older than native ``--format sarif``. +When the CLI can emit SARIF itself the workflow uses that and never runs this +file; parsing a human-readable stream is strictly worse and exists only so a +repository is not left unscanned while waiting for a release. + +It **fails closed**. If it cannot recognise the output it exits non-zero and +dumps what it saw. Emitting empty SARIF instead would report "0 findings", +which is indistinguishable from a clean scan and is the single most expensive +thing a security tool can get wrong. + +Three details of the format, each of which is load-bearing: + +* Severity is bare for ``CRITICAL`` and bracketed for ``[HIGH]``/``[MEDIUM]``/ + ``[LOW]``. One regex shape misses half the findings. +* ``File:`` paths are relative to the scan root, not the repository root. Left + unprefixed, every finding resolves to nothing in the consumer's view of the + repo. Hence ``--path-prefix``. +* Whole-file findings report line ``:0``. SARIF requires ``startLine >= 1``. + +``Code:`` lines are redacted excerpts of the match. They are skipped rather +than parsed, both because matching them would double-count every finding and +because a redacted excerpt tells a reader nothing the ``Info:`` line does not. +""" + +from __future__ import annotations + +import argparse +import json +import re +import sys + +ANSI = re.compile(r"\x1b\[[0-9;]*[A-Za-z]") + +# ` CRITICAL AWS Access Key` / ` [HIGH] Sensitive File` +SEVERITY_LINE = re.compile(r"^\s*(?:\[(CRITICAL|HIGH|MEDIUM|LOW|INFO)\]|(CRITICAL))\s+(.+?)\s*$") +FILE_LINE = re.compile(r"^\s*File:\s*(.+?):(\d+)\s*$") +INFO_LINE = re.compile(r"^\s*Info:\s*(.+?)\s*$") + +# Proof that a scan ran to completion. Without one of these we are looking at a +# crash, a help screen, or an unrecognised release — never at a clean result. +FOOTER = re.compile(r"^\s*(?:\d+\s+issue\(s\)\s+found|.*No security issues found)") + +LEVELS = {"CRITICAL": "error", "HIGH": "error", "MEDIUM": "warning", "LOW": "note", "INFO": "none"} +SECURITY_SEVERITY = {"CRITICAL": "9.0", "HIGH": "7.0", "MEDIUM": "5.0", "LOW": "3.0", "INFO": "1.0"} +RANK = {"info": 0, "low": 1, "medium": 2, "high": 3, "critical": 4} + + +class Unrecognised(Exception): + """The output did not look like a completed ThreatCrush scan.""" + + +def rule_id(title: str) -> str: + """Derive a stable rule id from a finding title. + + Old CLIs print `AWS Access Key`, not `secret-aws-access-key`. Slugifying + keeps SARIF results groupable and keeps fingerprints stable across runs, + which is what stops the Security tab treating every run as brand-new alerts. + """ + slug = re.sub(r"[^a-z0-9]+", "-", title.lower()).strip("-") + return f"threatcrush-{slug}" if slug else "threatcrush-finding" + + +def parse(text: str) -> list[dict]: + lines = ANSI.sub("", text).splitlines() + if not any(FOOTER.match(line) for line in lines): + raise Unrecognised("no scan-completion footer found") + + findings: list[dict] = [] + pending: dict | None = None + + for line in lines: + severity_match = SEVERITY_LINE.match(line) + if severity_match: + severity = severity_match.group(1) or severity_match.group(2) + pending = {"severity": severity.upper(), "title": severity_match.group(3).strip()} + continue + + if pending is None: + continue + + file_match = FILE_LINE.match(line) + if file_match: + pending["file"] = file_match.group(1).strip() + pending["line"] = int(file_match.group(2)) + continue + + info_match = INFO_LINE.match(line) + if info_match and "file" in pending: + pending["message"] = info_match.group(1).strip() + findings.append(pending) + pending = None + + return findings + + +def to_sarif(findings: list[dict], prefix: str, version: str) -> dict: + rules: dict[str, dict] = {} + results = [] + + for finding in findings: + rid = rule_id(finding["title"]) + rules.setdefault( + rid, + { + "id": rid, + "name": rid, + "shortDescription": {"text": finding["title"]}, + "fullDescription": {"text": finding["title"]}, + "defaultConfiguration": {"level": LEVELS[finding["severity"]]}, + "properties": { + "tags": ["security", "threatcrush"], + "security-severity": SECURITY_SEVERITY[finding["severity"]], + }, + }, + ) + + uri = finding["file"].lstrip("./") + if prefix: + uri = f"{prefix.strip('/')}/{uri}" + + results.append( + { + "ruleId": rid, + "level": LEVELS[finding["severity"]], + "message": {"text": finding.get("message", finding["title"])}, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {"uri": uri, "uriBaseId": "%SRCROOT%"}, + # Clamped: SARIF rejects 0, and a whole-file finding + # has no line to report. + "region": {"startLine": max(1, finding["line"])}, + } + } + ], + "partialFingerprints": { + "primaryLocationLineHash": f"{rid}:{uri}:{max(1, finding['line'])}" + }, + "properties": {"severity": finding["severity"].lower()}, + } + ) + + return { + "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "ThreatCrush", + "version": version, + "informationUri": "https://threatcrush.com", + "rules": list(rules.values()), + } + }, + "results": results, + "columnKind": "utf16CodeUnits", + } + ], + } + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--input", required=True, help="captured `threatcrush scan` output") + parser.add_argument("--output", required=True, help="SARIF file to write") + parser.add_argument("--path-prefix", default="", help="prepended to every file URI") + parser.add_argument("--tool-version", default="unknown") + parser.add_argument("--fail-on", default="", help="comma-separated severities that exit 1") + args = parser.parse_args() + + with open(args.input, encoding="utf-8", errors="replace") as handle: + text = handle.read() + + try: + findings = parse(text) + except Unrecognised as err: + print(f"error: unrecognised ThreatCrush output ({err})", file=sys.stderr) + print("--- first 40 lines ---", file=sys.stderr) + for line in ANSI.sub("", text).splitlines()[:40]: + print(line, file=sys.stderr) + return 2 + + with open(args.output, "w", encoding="utf-8") as handle: + json.dump(to_sarif(findings, args.path_prefix, args.tool_version), handle, indent=2) + handle.write("\n") + + print(f"converted {len(findings)} finding(s) to {args.output}") + + thresholds = [s.strip().lower() for s in args.fail_on.split(",") if s.strip()] + if thresholds: + unknown = [s for s in thresholds if s not in RANK] + if unknown: + # Silently ignoring a typo produces a gate that never fires, which + # looks exactly like a passing build. + print(f"error: unknown severity in --fail-on: {', '.join(unknown)}", file=sys.stderr) + return 2 + floor = min(RANK[s] for s in thresholds) + if any(RANK[f["severity"].lower()] >= floor for f in findings): + print(f"::error::findings at or above {args.fail_on}") + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index 57699281..10f951c2 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -45,25 +45,23 @@ jobs: threatcrush --version || true threatcrush scan --help || true - # Fail closed when the installed CLI cannot do what this workflow asks. + # Which interface does the installed CLI actually have? # - # Found the hard way. `@profullstack/threatcrush@0.2.2` has no `--format`, - # so the scan died with `error: unknown option '--format'` and commander - # exited 1 — the same code the CLI uses for "findings at or above - # --fail-on". The step read that as a result, no SARIF was written, the - # empty-run fallback below supplied one, and the PR comment said - # "0 findings". Green check, nothing scanned. - # - # Exit codes alone cannot separate "argument rejected" from "findings - # found", so do not try. Check the interface up front instead, and refuse - # to run a scan whose output we would not be able to trust. - - name: Verify the CLI supports SARIF + # Determined up front rather than inferred from an exit code, because + # exit codes cannot tell the two failures apart. `0.2.2` has no + # `--format`: the scan died with `error: unknown option '--format'` and + # commander exited 1 — the same code the CLI uses for "findings at or + # above --fail-on". Read as a result, that produced a green check and a + # "0 findings" comment on a repository nothing had scanned. + - name: Detect the CLI output interface + id: iface run: | - if ! threatcrush scan --help 2>&1 | grep -q -- '--format'; then - echo "::error::The installed ThreatCrush CLI ($(threatcrush --version 2>/dev/null || echo 'unknown')) does not support --format." - echo "::error::This workflow needs native SARIF output. Upgrade the CLI, or pin threatcrushPackageSpec to a version that has it." - echo "::error::Refusing to run: a scan whose output cannot be read would report 0 findings, which is indistinguishable from a clean result." - exit 1 + if threatcrush scan --help 2>&1 | grep -q -- '--format'; then + echo "native=true" >> "$GITHUB_OUTPUT" + echo "Native SARIF output available." + else + echo "native=false" >> "$GITHUB_OUTPUT" + echo "::notice::CLI $(threatcrush --version 2>/dev/null || echo unknown) predates --format; converting terminal output instead." fi - name: Scan @@ -71,13 +69,35 @@ jobs: run: | set -o pipefail FAIL_ON="{{failOn}}" - ARGS=(scan "{{scanPath}}" --format sarif --output threatcrush.sarif) - if [ -n "$FAIL_ON" ]; then - ARGS+=(--fail-on "$FAIL_ON") - fi - + SCAN_PATH="{{scanPath}}" code=0 - threatcrush "${ARGS[@]}" || code=$? + + if [ "${{ steps.iface.outputs.native }}" = "true" ]; then + ARGS=(scan "$SCAN_PATH" --format sarif --output threatcrush.sarif) + if [ -n "$FAIL_ON" ]; then + ARGS+=(--fail-on "$FAIL_ON") + fi + threatcrush "${ARGS[@]}" || code=$? + else + # Compatibility path for CLIs older than native SARIF. The + # converter fails closed: if it cannot recognise the output it + # exits non-zero and writes nothing, so an unparseable scan can + # never arrive downstream looking like a clean one. + threatcrush scan "$SCAN_PATH" 2>&1 | tee threatcrush-output.txt || true + PREFIX="" + if [ "$SCAN_PATH" != "." ]; then + # Paths in terminal output are relative to the scan root. Left + # unprefixed they resolve to nothing in the repository view, and + # every finding reads as out-of-scope. + PREFIX="$SCAN_PATH" + fi + python3 .github/threatcrush-to-sarif.py \ + --input threatcrush-output.txt \ + --output threatcrush.sarif \ + --path-prefix "$PREFIX" \ + --tool-version "$(threatcrush --version 2>/dev/null || echo unknown)" \ + --fail-on "$FAIL_ON" || code=$? + fi # The SARIF file is the evidence that a scan happened, and it is the # only evidence worth trusting. An exit code says what the process From 360055a687af287ed28818b3634cf2fd217b33e1 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 3 Aug 2026 10:42:25 +0000 Subject: [PATCH 5/6] fix(actions-fleet-core): allow pack helper scripts under .github/scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The destination allowlist had no entry any non-workflow file could match, so a pack that ships a script its workflow invokes was unrepresentable. Add a pattern scoped to a dedicated .github/scripts/ directory. Scoped there rather than the top of .github so a pack cannot land a file beside dependabot.yml or CODEOWNERS, and kept flat so every managed script shows up in one listing. This grants no privilege a pack did not already have — workflow `run:` blocks execute arbitrary code either way — it just keeps that code in a reviewable file instead of a YAML heredoc. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/action-pack/schema.test.ts | 19 +++++++++++++++++++ .../src/action-pack/validate.ts | 6 ++++++ 2 files changed, 25 insertions(+) diff --git a/packages/actions-fleet-core/src/action-pack/schema.test.ts b/packages/actions-fleet-core/src/action-pack/schema.test.ts index 0957e230..1bee06ee 100644 --- a/packages/actions-fleet-core/src/action-pack/schema.test.ts +++ b/packages/actions-fleet-core/src/action-pack/schema.test.ts @@ -139,6 +139,12 @@ describe('validateManifest path safety', () => { expect(isSafeDestination('docs/setup.md')).toBe(true); }); + it('allows workflow helper scripts under .github/scripts', () => { + expect(isSafeDestination('.github/scripts/to-sarif.py')).toBe(true); + expect(isSafeDestination('.github/scripts/prepare.sh')).toBe(true); + expect(isSafeDestination('.github/scripts/report.mjs')).toBe(true); + }); + it('rejects sneaky destinations', () => { expect(isSafeDestination('.github/workflows/../../etc/passwd')).toBe(false); expect(isSafeDestination('.github/workflows/')).toBe(false); @@ -147,6 +153,19 @@ describe('validateManifest path safety', () => { expect(isSafeDestination('')).toBe(false); expect(isSafeDestination('.github/workflows/ci.yml\0.bad')).toBe(false); }); + + it('confines helper scripts to .github/scripts', () => { + // A script alongside dependabot.yml / CODEOWNERS is out of bounds... + expect(isSafeDestination('.github/to-sarif.py')).toBe(false); + // ...as is anywhere outside .github, however plausible the name. + expect(isSafeDestination('scripts/to-sarif.py')).toBe(false); + // Flat directory only — no nesting to hide files in. + expect(isSafeDestination('.github/scripts/nested/to-sarif.py')).toBe(false); + // Executable formats a workflow would not invoke stay out. + expect(isSafeDestination('.github/scripts/setup.exe')).toBe(false); + expect(isSafeDestination('.github/scripts/.env')).toBe(false); + expect(isSafeDestination('.github/scripts/../workflows/ci.yml')).toBe(false); + }); }); describe('parseManifest', () => { diff --git a/packages/actions-fleet-core/src/action-pack/validate.ts b/packages/actions-fleet-core/src/action-pack/validate.ts index ac556347..7fd30400 100644 --- a/packages/actions-fleet-core/src/action-pack/validate.ts +++ b/packages/actions-fleet-core/src/action-pack/validate.ts @@ -8,6 +8,12 @@ const ALLOWED_DESTINATIONS: RegExp[] = [ /^\.github\/release-please\.ya?ml$/, /^\.github\/release-drafter\.ya?ml$/, /^\.github\/CODEOWNERS$/, + // Helper scripts a pack's workflow invokes. Confined to .github/scripts/ so a + // pack cannot land a file next to dependabot.yml or CODEOWNERS, and kept to + // one flat directory so every managed script is visible in one listing. This + // grants no privilege a pack lacks — workflow `run:` blocks already execute + // arbitrary code — it just keeps that code in a reviewable file. + /^\.github\/scripts\/[A-Za-z0-9._-]+\.(py|sh|js|mjs)$/, /^docs\/[A-Za-z0-9._/-]+\.(md|mdx)$/, ]; From 7394d7802f1db2d6c3f232e7ae2756ac66a93d74 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Mon, 3 Aug 2026 10:42:32 +0000 Subject: [PATCH 6/6] fix(actions/threatcrush-scan): ship the converter to an allowed destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every test in packages/actions failed with "Invalid action-pack manifest: 1 issue(s)" — the pack sent threatcrush-to-sarif.py to .github/, which the destination allowlist rejects. Because each test loads the whole catalog, one bad manifest took all 15 down, including the four unrelated packs. Move the converter to .github/scripts/ and update the workflow, README and test expectations to match. Also fix the pull_request_target assertion, which the manifest error had been masking: it substring-matched the entire rendered workflow, so the comment explaining why the pack deliberately stays on `pull_request` tripped it. Strip comments first and assert on the trigger block, so the check tests the directive rather than forbidding its own rationale. Co-Authored-By: Claude Opus 5 (1M context) --- packages/actions/src/index.test.ts | 15 ++++++++++++--- packages/actions/threatcrush-scan/README.md | 2 +- .../threatcrush-scan/sh1pt.actionpack.yaml | 2 +- packages/actions/threatcrush-scan/workflow.yml | 2 +- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index 30897ae7..c1294d1e 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -41,7 +41,7 @@ describe('built-in packs', () => { // Workflow plus the legacy-output converter it falls back to. expect(entry?.manifest.files.map((f) => f.destination)).toEqual([ '.github/workflows/threatcrush-scan.yml', - '.github/threatcrush-to-sarif.py', + '.github/scripts/threatcrush-to-sarif.py', ]); }); @@ -215,7 +215,7 @@ describe('built-in packs', () => { expect(content).toContain('if [ ! -s threatcrush.sarif ]; then'); // A CLI without --format takes the converter path rather than failing the // repo out of being scanned at all. - expect(content).toContain('.github/threatcrush-to-sarif.py'); + expect(content).toContain('.github/scripts/threatcrush-to-sarif.py'); expect(content).toContain('this diff was NOT scanned'); // And the report must be fail-closed. Testing for status == "error" was // fail-open: when the capability check fails the scan step is *skipped*, @@ -236,7 +236,16 @@ describe('built-in packs', () => { manifest: entry.manifest, inputs: {}, }); - expect(result.files[0]?.content).not.toContain('pull_request_target'); + // Comments are stripped first: the workflow documents why it stays on + // `pull_request`, and a raw substring check would forbid explaining the + // very decision it exists to protect. What matters is that no directive + // selects the event. + const directives = (result.files[0]?.content ?? '') + .split('\n') + .filter((line) => !/^\s*#/.test(line)) + .join('\n'); + expect(directives).not.toContain('pull_request_target'); + expect(directives).toMatch(/^on:\n\s+pull_request:\s*$/m); expect(entry.manifest.security.allowPullRequestTarget).toBe(false); }); }); diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md index 8a903af1..277fe33b 100644 --- a/packages/actions/threatcrush-scan/README.md +++ b/packages/actions/threatcrush-scan/README.md @@ -34,7 +34,7 @@ scanning, and picks accordingly: | CLI | Path | | --- | --- | | Has `--format` | Native SARIF. Preferred; nothing is parsed. | -| Older | Runs the text scan and converts it with `.github/threatcrush-to-sarif.py`. | +| Older | Runs the text scan and converts it with `.github/scripts/threatcrush-to-sarif.py`. | The check happens up front because exit codes cannot tell the two failures apart. The published `0.2.2` has no `--format`: the scan died with diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index 60a2e4bc..a556f44f 100644 --- a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml +++ b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml @@ -63,7 +63,7 @@ files: # the native path whenever it is available — but shipping it means a repo is # scanned today rather than waiting on a release. - source: threatcrush-to-sarif.py - destination: .github/threatcrush-to-sarif.py + destination: .github/scripts/threatcrush-to-sarif.py mergeStrategy: replace-managed policies: installMode: pull-request diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index 10f951c2..3e7fdba4 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -91,7 +91,7 @@ jobs: # every finding reads as out-of-scope. PREFIX="$SCAN_PATH" fi - python3 .github/threatcrush-to-sarif.py \ + python3 .github/scripts/threatcrush-to-sarif.py \ --input threatcrush-output.txt \ --output threatcrush.sarif \ --path-prefix "$PREFIX" \