From f278b8d8f26c68c91a845a31cd746a9b17d5735c Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Fri, 14 Aug 2026 04:16:11 +0000 Subject: [PATCH 1/3] ci: add the ThreatCrush security scan workflow Signed-off-by: Anthony Ettinger --- .github/scripts/threatcrush-to-sarif.py | 235 +++++++++++++++ .github/workflows/threatcrush-scan.yml | 382 ++++++++++++++++++++++++ 2 files changed, 617 insertions(+) create mode 100644 .github/scripts/threatcrush-to-sarif.py create mode 100644 .github/workflows/threatcrush-scan.yml diff --git a/.github/scripts/threatcrush-to-sarif.py b/.github/scripts/threatcrush-to-sarif.py new file mode 100644 index 0000000000..f349d9959c --- /dev/null +++ b/.github/scripts/threatcrush-to-sarif.py @@ -0,0 +1,235 @@ +#!/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*(?:(?P\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() + footer = next((m for line in lines if (m := FOOTER.match(line))), None) + if footer is None: + raise Unrecognised("no scan-completion footer found") + # "No security issues found" has no number; that branch means zero. + expected = int(footer.group("count") or 0) + + 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 + + # Fail closed on anything left half-read. + # + # A footer proves the scan finished. It does not prove this converter + # understood what the scan printed. A finding whose Info: line moved, or + # whose block gained a field, is dropped silently here — the next severity + # line overwrites `pending` and nobody hears about it. The workflow then + # reports a clean or under-counted scan, which is the failure this file + # exists to prevent rather than cause. + # + # Raised by CodeRabbit on ShadowSafin/AndroLLM#7. + if pending is not None: + raise Unrecognised(f"incomplete finding block: {pending.get('title', 'untitled')!r}") + if len(findings) != expected: + raise Unrecognised(f"footer reported {expected} finding(s), parsed {len(findings)}") + + 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"]], + }, + }, + ) + + # removeprefix, not lstrip. lstrip takes a *set* of characters, so + # lstrip("./") eats every leading dot and slash: `.github/workflows/x.yml` + # became `github/workflows/x.yml` and `.env` became `env`. Both then point + # at a path that does not exist, and `.env` is exactly the sort of file a + # credential scanner has findings in. + uri = finding["file"].removeprefix("./") + 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/.github/workflows/threatcrush-scan.yml b/.github/workflows/threatcrush-scan.yml new file mode 100644 index 0000000000..33e9868c32 --- /dev/null +++ b/.github/workflows/threatcrush-scan.yml @@ -0,0 +1,382 @@ +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. +# +# 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. +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: + # 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. + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 + with: + persist-credentials: false + + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: "20" + + # 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. + # + # --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. + - name: Install ThreatCrush + run: | + set -euo pipefail + spec='@profullstack/threatcrush@0.11.0' + want='sha512-EKcaxsgiydi7qCH0FhvNviKUpyVi/CImwNS6Kx3IbWMuUjUPCXISAUIzFnYo8BiC+jG9dxfFDMBlwZdhqhwWfQ==' + + name="" + for attempt in 1 2 3; do + if name=$(npm pack --silent --pack-destination "${RUNNER_TEMP}" "${spec}" | tail -1) \ + && [ -n "${name}" ] && [ -f "${RUNNER_TEMP}/${name}" ]; then + break + fi + name="" + delay=$((attempt * 10)) + echo "::warning::ThreatCrush download attempt ${attempt}/3 failed; retrying in ${delay}s" + sleep "${delay}" + 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)" + if [ "${got}" != "${want}" ]; then + echo "::error::ThreatCrush integrity mismatch for ${spec}" + echo "::error::expected ${want}" + echo "::error::received ${got}" + echo "::error::refusing to install — this is not a transient failure" + exit 1 + fi + echo "Integrity verified for ${spec}: ${got}" + else + echo "::warning::no integrity hash pinned for ${spec}; installing unverified" + fi + + npm install -g --ignore-scripts "${tarball}" + + # 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 + + # Which interface does the installed CLI actually have? + # + # 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 "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 + id: scan + # Through env rather than expanded into the script. The value comes from + # our own iface step so it is not attacker-controlled, but "a workflow + # expression interpolated into a shell body" is the shape of a template + # injection and static analysis reads the shape, not the provenance. + env: + NATIVE: ${{ steps.iface.outputs.native }} + run: | + set -o pipefail + FAIL_ON="" + SCAN_PATH="." + code=0 + + if [ "$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/scripts/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 + # 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" + exit 1 + ;; + *) + echo "status=error" >> "$GITHUB_OUTPUT" + echo "::error::ThreatCrush scan failed with exit code ${code} — results may be incomplete" + 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() && 'true' == '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() + 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", ""] + + # 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. + 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]: + # 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. + locations = result.get("locations") or [] + location = (locations[0] if locations else {}).get("physicalLocation", {}) + uri = location.get("artifactLocation", {}).get("uri", "(no location)") + 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 + + # if-no-files-found: ignore, because nothing synthesises the file any + # more. A run that never produced SARIF has no artifact to keep, and that + # is the honest outcome rather than a reason to invent one. + - name: Upload SARIF artifact + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: threatcrush-sarif + path: threatcrush.sarif + if-no-files-found: ignore + retention-days: 30 + + # Best-effort. `pull_request` gives fork PRs a read-only token, so this + # 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() && 'true' == 'true' + && github.event.pull_request.head.repo.full_name == github.repository + && github.actor != 'dependabot[bot]' + continue-on-error: true + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # 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 { + // Paginated. listComments returns the first thirty and stops, so + // on a pull request with more discussion than that the existing + // report falls off the page, is not found, and every subsequent + // run posts another one. The bug only appears on the requests + // people actually engage with, which is the worst place for it. + const comments = await github.paginate(github.rest.issues.listComments, { + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + per_page: 100, + }); + const existing = comments.find( + (c) => c.user.type === 'Bot' && c.body.includes('ThreatCrush Security Scan'), + ); + + 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 c6ba3b6be28803075f139f0681cce97c2f353b6b Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Tue, 8 Sep 2026 14:11:33 +0000 Subject: [PATCH 2/3] ci: update the ThreatCrush scan workflow to the reviewed pack Signed-off-by: Anthony Ettinger --- .github/workflows/threatcrush-scan.yml | 424 ++++++++++++------------- 1 file changed, 199 insertions(+), 225 deletions(-) diff --git a/.github/workflows/threatcrush-scan.yml b/.github/workflows/threatcrush-scan.yml index 33e9868c32..3fc9aa30bf 100644 --- a/.github/workflows/threatcrush-scan.yml +++ b/.github/workflows/threatcrush-scan.yml @@ -3,15 +3,14 @@ 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 pull-requests: write @@ -24,48 +23,66 @@ 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 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 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. + # + # 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" + echo "scoped=true" >> "$GITHUB_OUTPUT" + echo "Scoping the report to $(wc -l < "${RUNNER_TEMP}/threatcrush-changed.txt") changed file(s)." + else + : > "${RUNNER_TEMP}/threatcrush-changed.txt" + echo "scoped=false" >> "$GITHUB_OUTPUT" + echo "::notice::No merge ref (conflicted pull request?) — reporting every finding, unscoped." + fi - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: "20" - # 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='@profullstack/threatcrush@0.11.0' - want='sha512-EKcaxsgiydi7qCH0FhvNviKUpyVi/CImwNS6Kx3IbWMuUjUPCXISAUIzFnYo8BiC+jG9dxfFDMBlwZdhqhwWfQ==' + spec='@profullstack/threatcrush@0.11.9' + want='sha512-ZjIYeGPBNIcD2LjanW8pv8NiPclI8IZCaY/8su2vusYklQUROguSusJe+nhoQDLMSGhNM7d/kNJn6snEZrvL9g==' + # 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) \ @@ -73,22 +90,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}" @@ -101,74 +116,39 @@ 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 - # Which interface does the installed CLI actually have? + # The CLI emits SARIF itself, so this asks for it and nothing converts + # anything. # - # 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 "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 - + # 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. + # + # 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 - # Through env rather than expanded into the script. The value comes from - # our own iface step so it is not attacker-controlled, but "a workflow - # expression interpolated into a shell body" is the shape of a template - # injection and static analysis reads the shape, not the provenance. - env: - NATIVE: ${{ steps.iface.outputs.native }} run: | - set -o pipefail + set -euo pipefail FAIL_ON="" SCAN_PATH="." code=0 - if [ "$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/scripts/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=$? + ARGS=(scan "$SCAN_PATH" --format sarif --output threatcrush.sarif) + if [ -n "$FAIL_ON" ]; then + ARGS+=(--fail-on "$FAIL_ON") fi + 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 @@ -181,12 +161,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 @@ -194,123 +174,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() && 'true' == '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: - 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]: - # 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. - locations = result.get("locations") or [] - location = (locations[0] if locations else {}).get("physicalLocation", {}) - uri = location.get("artifactLocation", {}).get("uri", "(no location)") - 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 }} + try: + changed = set(open(os.environ["RUNNER_TEMP"] + "/threatcrush-changed.txt").read().split()) + except Exception: + changed = set() - - name: Write report to job summary - if: always() - run: cat "$RUNNER_TEMP/threatcrush-comment.md" >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true + scoped = os.environ.get("SCAN_SCOPED", "") == "true" + results.sort(key=lambda r: (RANK.get(r.get("level", "warning"), 3), locate(r))) + 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-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. + 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 @@ -320,15 +287,31 @@ jobs: if-no-files-found: ignore 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. + # 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 + + # 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() && 'true' == 'true' + always() && github.event.pull_request.head.repo.full_name == github.repository && github.actor != 'dependabot[bot]' continue-on-error: true @@ -338,7 +321,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.'; } @@ -346,9 +329,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, @@ -358,21 +341,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( From a161978f0cad392ab06d599b705c1cf474e877c0 Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Tue, 8 Sep 2026 14:11:34 +0000 Subject: [PATCH 3/3] ci: drop the ThreatCrush SARIF converter, no longer installed by the pack Signed-off-by: Anthony Ettinger --- .github/scripts/threatcrush-to-sarif.py | 235 ------------------------ 1 file changed, 235 deletions(-) delete mode 100644 .github/scripts/threatcrush-to-sarif.py diff --git a/.github/scripts/threatcrush-to-sarif.py b/.github/scripts/threatcrush-to-sarif.py deleted file mode 100644 index f349d9959c..0000000000 --- a/.github/scripts/threatcrush-to-sarif.py +++ /dev/null @@ -1,235 +0,0 @@ -#!/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*(?:(?P\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() - footer = next((m for line in lines if (m := FOOTER.match(line))), None) - if footer is None: - raise Unrecognised("no scan-completion footer found") - # "No security issues found" has no number; that branch means zero. - expected = int(footer.group("count") or 0) - - 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 - - # Fail closed on anything left half-read. - # - # A footer proves the scan finished. It does not prove this converter - # understood what the scan printed. A finding whose Info: line moved, or - # whose block gained a field, is dropped silently here — the next severity - # line overwrites `pending` and nobody hears about it. The workflow then - # reports a clean or under-counted scan, which is the failure this file - # exists to prevent rather than cause. - # - # Raised by CodeRabbit on ShadowSafin/AndroLLM#7. - if pending is not None: - raise Unrecognised(f"incomplete finding block: {pending.get('title', 'untitled')!r}") - if len(findings) != expected: - raise Unrecognised(f"footer reported {expected} finding(s), parsed {len(findings)}") - - 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"]], - }, - }, - ) - - # removeprefix, not lstrip. lstrip takes a *set* of characters, so - # lstrip("./") eats every leading dot and slash: `.github/workflows/x.yml` - # became `github/workflows/x.yml` and `.env` became `env`. Both then point - # at a path that does not exist, and `.env` is exactly the sort of file a - # credential scanner has findings in. - uri = finding["file"].removeprefix("./") - 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())