diff --git a/packages/actions/src/index.test.ts b/packages/actions/src/index.test.ts index 1fb7cfc8..694c6ddd 100644 --- a/packages/actions/src/index.test.ts +++ b/packages/actions/src/index.test.ts @@ -41,10 +41,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. + // One file. The legacy-output converter was removed in 1.7.0 once the + // pinned CLI could emit SARIF itself; every file a pack writes into + // somebody else's repository is surface their reviewer has to read. expect(entry?.manifest.files.map((f) => f.destination)).toEqual([ '.github/workflows/threatcrush-scan.yml', - '.github/scripts/threatcrush-to-sarif.py', ]); }); @@ -310,11 +311,13 @@ describe('built-in packs', () => { inputs: {}, }); const content = result.files[0]?.content ?? ''; - expect(content).toContain("grep -q -- '--format'"); + expect(content).toContain('--format sarif --output threatcrush.sarif'); 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/scripts/threatcrush-to-sarif.py'); + // The capability probe and the converter it guarded are both gone. The + // spec is pinned and the install refuses other bytes, so the interface is + // decided by the pack rather than discovered on the runner. + expect(content).not.toContain("grep -q -- '--format'"); + expect(content).not.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*, diff --git a/packages/actions/threatcrush-scan/README.md b/packages/actions/threatcrush-scan/README.md index e172b9b4..f49f833e 100644 --- a/packages/actions/threatcrush-scan/README.md +++ b/packages/actions/threatcrush-scan/README.md @@ -83,40 +83,37 @@ print in SARIF order, which is file order, so the 50-row cap was decided by wher a finding sat in the tree: a `high` in the last file scanned could be truncated away while fifty `note`s from the first file printed in full. -## 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/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 -`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%. +## One output path + +The CLI emits SARIF itself. The workflow asks for it and nothing parses +anything: + +``` +threatcrush scan "$SCAN_PATH" --format sarif --output threatcrush.sarif +``` + +There used to be a second path — a capability probe on `--format`, and a +235-line Python converter that reconstructed findings by regex from the +terminal output when the probe said no. Both were removed in 1.7.0, because +the premise stopped holding: `threatcrushPackageSpec` pins an exact version +and the install step refuses any other bytes, so which interface the CLI has +is decided by the pack rather than discovered on the runner. The probe could +only ever answer yes. + +Removing it is a security change more than a tidying one. The converter read a +*display* format, which is free to change between releases — the failure mode +being a silent undercount that still looks like a completed scan. And every +file a pack writes into somebody else's repository is surface a reviewer has +to read; this pack now installs one workflow and nothing else. That was a +direct ask from a maintainer reviewing the supply chain before merging. + +The history is worth keeping, because it is the reason the exit-code handling +below is written the way it is. The published `0.2.2` had 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 +comment said **0 findings**. A green check on a repository that was never +scanned. ## Exit codes are distinguished diff --git a/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml b/packages/actions/threatcrush-scan/sh1pt.actionpack.yaml index 744aa500..d50ed968 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.6.1 +version: 1.7.0 publisher: profullstack visibility: public license: MIT @@ -109,13 +109,6 @@ 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/scripts/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 deleted file mode 100644 index f349d995..00000000 --- a/packages/actions/threatcrush-scan/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()) diff --git a/packages/actions/threatcrush-scan/workflow.yml b/packages/actions/threatcrush-scan/workflow.yml index bdbadf7c..72446253 100644 --- a/packages/actions/threatcrush-scan/workflow.yml +++ b/packages/actions/threatcrush-scan/workflow.yml @@ -146,65 +146,35 @@ jobs: threatcrush --version || true threatcrush scan --help || true - # 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 second path here: a capability probe on `--format`, + # and a 235-line Python converter that parsed the terminal output when + # the probe said no. Both are gone, because the premise stopped holding. + # `threatcrushPackageSpec` pins an exact version and the step above + # refuses to install any other bytes, so "which interface does the + # installed CLI have" is answered by the pack, not discovered at + # runtime — the probe could only ever say yes. + # + # Deleting it is a security change more than a tidying one. The + # converter reconstructed findings by regex out of a display format that + # is free to change, which is a silent-undercount waiting to happen; and + # every file a pack installs into somebody else's repository is surface + # they have to review. This one now installs a single workflow. - 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="{{failOn}}" SCAN_PATH="{{scanPath}}" 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