From 3a4049d53225aab94a922f8261e26c60f14b630e Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Wed, 2 Sep 2026 11:24:17 -0700 Subject: [PATCH 1/2] fix: an unreadable scan report is not a clean scan (sable-fgk7) The composite action's results step coerced every jq failure into findings_count=0: a body that was not JSON, a 200 carrying an error object, or a parseable payload with no vulnerabilities key passed every severity threshold and rendered "No security findings detected". Validate the shape first; on failure exit 1 with status=unreadable, an actionable message, and no count outputs at all. Once the shape holds the counts cannot fail, so the fallbacks are removed rather than moved. rafter issues create from-scan (Node and Python) had the same shape: data.vulnerabilities || [] turned a processing or failed scan, an error object, or a keyless payload into "No findings to create issues for". Both now refuse with exit 1 and name the scan status. An empty array is still a clean result. Regression coverage: two end-to-end CI jobs drive the real action against the mock backend (a 3-way matrix of unreadable shapes asserting the build fails with status=unreadable and an EMPTY findings-count, plus an exact counts job asserting 3/1/1/0/1); three drift assertions; unit tests on the real functions in both runtimes. Every new guard was mutation-verified by hand: restoring the old behaviour fails the tests that exist to catch it. --- .github/workflows/test-github-action.yml | 119 ++++++++++++++++++ github-action/README.md | 4 +- github-action/action.yml | 27 +++- github-action/tests/mock-rafter-api.py | 46 ++++++- .../tests/test-action-yml-defaults.sh | 37 ++++++ node/src/commands/issues/from-scan.ts | 33 ++++- node/tests/issues.test.ts | 58 +++++++++ .../rafter_cli/commands/issues/issues_app.py | 39 +++++- python/tests/test_issues.py | 119 ++++++++++++++++++ shared-docs/CLI_SPEC.md | 3 + 10 files changed, 470 insertions(+), 15 deletions(-) diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index 3887552..1491b3f 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -227,6 +227,125 @@ jobs: fi echo "PASS: the results fetch retried and completed." + # sable-fgk7 — the results step used to coerce EVERY failure to read the + # report into findings_count=0, which passes every severity threshold and + # renders ":white_check_mark: No security findings detected". A report the + # action cannot read is not a clean scan. Three shapes, each of which used + # to land as a clean green: schema-valid-but-wrong (parses, no key), not + # JSON at all, and a 200 whose body is an error object. + test-results-unreadable-is-not-clean: + name: "Results: an unreadable report is not a clean scan (${{ matrix.shape }})" + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + shape: [missing-key, not-json, error-object] + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (poll completes; results body is ${{ matrix.shape }}) + env: + PORT: '8791' + FAIL_COUNT: '0' + COMPLETE_AFTER: '1' + RESULTS_SHAPE: ${{ matrix.shape }} + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8791/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8791/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8791' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the build failed and no count was fabricated + run: | + cat mock.log + FAIL=0 + if [ "${{ steps.scan.outcome }}" != "failure" ]; then + echo "FAIL: an unreadable report must fail the build (outcome='${{ steps.scan.outcome }}')." + FAIL=1 + fi + if [ "${{ steps.scan.outputs.status }}" != "unreadable" ]; then + echo "FAIL: expected status=unreadable, got '${{ steps.scan.outputs.status }}'." + FAIL=1 + fi + # The floor: a count that was never computed must be ABSENT, not 0. + # '0' here is the bug — it is what a consumer gating on the output + # reads as a clean scan. + if [ -n "${{ steps.scan.outputs.findings-count }}" ]; then + echo "FAIL: findings-count was fabricated as '${{ steps.scan.outputs.findings-count }}' from an unreadable report." + FAIL=1 + fi + [ "$FAIL" -eq 0 ] && echo "PASS: unreadable report (${{ matrix.shape }}) failed the build with status=unreadable and no counts." + exit $FAIL + + # The other half of the floor: when the report IS readable the counts must be + # exactly the report's and the build must pass. Without this, a "validation" + # that rejected everything would also land green above. + test-results-counts-exact: + name: "Results: counts are exactly the report's" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (poll completes; report has 3 findings) + env: + PORT: '8792' + FAIL_COUNT: '0' + COMPLETE_AFTER: '1' + RESULTS_SHAPE: 'with-findings' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8792/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8792/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action against the mock + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8792' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert every count is the report's, not a default + run: | + cat mock.log + FAIL=0 + check() { + if [ "$2" != "$3" ]; then + echo "FAIL: $1 expected '$3', got '$2'" + FAIL=1 + fi + } + check outcome "${{ steps.scan.outcome }}" "success" + check status "${{ steps.scan.outputs.status }}" "completed" + check findings-count "${{ steps.scan.outputs.findings-count }}" "3" + check critical-count "${{ steps.scan.outputs.critical-count }}" "1" + check high-count "${{ steps.scan.outputs.high-count }}" "1" + check medium-count "${{ steps.scan.outputs.medium-count }}" "0" + check low-count "${{ steps.scan.outputs.low-count }}" "1" + [ "$FAIL" -eq 0 ] && echo "PASS: counts are exactly the report's (3/1/1/0/1)." + exit $FAIL + test-yaml-validity: name: action.yml is valid YAML runs-on: ubuntu-latest diff --git a/github-action/README.md b/github-action/README.md index e9d94c1..eec9bf0 100644 --- a/github-action/README.md +++ b/github-action/README.md @@ -53,12 +53,12 @@ jobs: | Output | Description | |--------|-------------| | `scan-id` | The Rafter scan ID | -| `findings-count` | Total findings | +| `findings-count` | Total findings. Empty, never `0`, when the report could not be read (see `status`) | | `critical-count` | Critical severity findings | | `high-count` | High severity findings | | `medium-count` | Medium severity findings | | `low-count` | Low severity findings | -| `status` | Scan status | +| `status` | `completed`, `failed`, `timeout`, `unreadable` (the scan may have finished but its report could not be read or parsed), or `unreachable` (the API could not be contacted). Count outputs are only written when `completed` | ## Examples diff --git a/github-action/action.yml b/github-action/action.yml index e096efa..8193cc9 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -303,13 +303,28 @@ runs: fetch_results "${{ runner.temp }}/rafter-results.md" "${RAFTER_URL}/api/static/scan?scan_id=${SCAN_ID}&format=md" fetch_results "${{ runner.temp }}/rafter-results.sarif" "${RAFTER_URL}/api/static/scan?scan_id=${SCAN_ID}&format=sarif" - # Extract counts + # A report this step cannot read is NOT a clean scan (sable-fgk7). + # Every count below used to fall back to 0 on any jq failure, so a + # malformed body, a truncated write, or a 200 carrying an error object + # rendered as ":white_check_mark: No security findings detected" and + # passed every severity threshold. Validate the shape first. Once it + # holds, the count expressions cannot fail and need no fallback; if + # jq itself is broken the step fails, which is the correct outcome. RESULTS="${{ runner.temp }}/rafter-results.json" - FINDINGS_COUNT=$(jq '.vulnerabilities | length' "$RESULTS" 2>/dev/null || echo "0") - CRITICAL_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "critical")] | length' "$RESULTS" 2>/dev/null || echo "0") - HIGH_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "error" or .severity == "high")] | length' "$RESULTS" 2>/dev/null || echo "0") - MEDIUM_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "warning" or .severity == "medium")] | length' "$RESULTS" 2>/dev/null || echo "0") - LOW_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "note" or .severity == "low")] | length' "$RESULTS" 2>/dev/null || echo "0") + if ! jq -e 'type == "object" and (.vulnerabilities | type == "array") and all(.vulnerabilities[]; type == "object")' "$RESULTS" >/dev/null 2>&1; then + SNIPPET=$(head -c 300 "$RESULTS" 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) + echo "::error::Rafter returned a report for scan ${SCAN_ID} that this action cannot read: no 'vulnerabilities' array." + echo "::error::A report that cannot be parsed is not a clean scan, so no counts were produced. Check the scan in your dashboard at ${RAFTER_URL}/dashboard or retry with: rafter get ${SCAN_ID}" + echo "::error::Body started with: ${SNIPPET}" + echo "status=unreadable" >> "$GITHUB_OUTPUT" + exit 1 + fi + + FINDINGS_COUNT=$(jq '.vulnerabilities | length' "$RESULTS") + CRITICAL_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "critical")] | length' "$RESULTS") + HIGH_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "error" or .severity == "high")] | length' "$RESULTS") + MEDIUM_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "warning" or .severity == "medium")] | length' "$RESULTS") + LOW_COUNT=$(jq '[.vulnerabilities[] | select(.severity == "note" or .severity == "low")] | length' "$RESULTS") echo "findings_count=${FINDINGS_COUNT}" >> "$GITHUB_OUTPUT" echo "critical_count=${CRITICAL_COUNT}" >> "$GITHUB_OUTPUT" diff --git a/github-action/tests/mock-rafter-api.py b/github-action/tests/mock-rafter-api.py index 2b77ab9..23af6cc 100644 --- a/github-action/tests/mock-rafter-api.py +++ b/github-action/tests/mock-rafter-api.py @@ -26,6 +26,19 @@ i.e. as soon as the injected failures are done). Set it higher than the failure window to make the RESULTS fetch fail rather than the poll. + RESULTS_SHAPE what the completed JSON body looks like (sable-fgk7). Default + "ok": {"status":"completed","vulnerabilities":[]}. Others: + with-findings three findings: one critical, one high, one low + missing-key {"scan_id":..,"status":"completed"} — parses, + has no vulnerabilities array at all + not-json a 200 whose body is an HTML error page + error-object a 200 whose body is {"error": ...} + Each of the last three used to make the action report + "No security findings detected" and pass every threshold. + SHAPE_FROM GET index from which the JSON body takes RESULTS_SHAPE + (default COMPLETE_AFTER + 1, so the poll loop sees one healthy + "completed" and the RESULTS fetch gets the shaped body). + md/sarif fetches are never shaped. """ import json import os @@ -38,21 +51,47 @@ FAIL_FOREVER = os.environ.get("FAIL_FOREVER") == "1" FAIL_COUNT = int(os.environ.get("FAIL_COUNT", "1")) COMPLETE_AFTER = int(os.environ.get("COMPLETE_AFTER", str(FAIL_ON))) +RESULTS_SHAPE = os.environ.get("RESULTS_SHAPE", "ok") +SHAPE_FROM = int(os.environ.get("SHAPE_FROM", str(COMPLETE_AFTER + 1))) SCAN_ID = "repro-sable-l10k-0001" +# Severities chosen so every count output is pinned to a distinct value: +# findings=3, critical=1, high=1, medium=0, low=1. +WITH_FINDINGS = [ + {"rule_id": "sql-injection", "severity": "critical", "file_path": "db.php", "line_start": 12}, + {"rule_id": "xss-echo", "severity": "high", "file_path": "view.php", "line_start": 40}, + {"rule_id": "weak-hash", "severity": "low", "file_path": "auth.php", "line_start": 7}, +] + state = {"polls": 0} class Handler(BaseHTTPRequestHandler): def _send(self, code, payload): - body = json.dumps(payload).encode() + self._send_raw(code, json.dumps(payload).encode(), "application/json") + + def _send_raw(self, code, body, content_type): self.send_response(code) - self.send_header("Content-Type", "application/json") + self.send_header("Content-Type", content_type) self.send_header("Content-Length", str(len(body))) self.end_headers() self.wfile.write(body) + def _send_shaped_results(self): + """The completed JSON body under RESULTS_SHAPE (never for md/sarif).""" + if RESULTS_SHAPE == "with-findings": + return self._send(200, {"scan_id": SCAN_ID, "status": "completed", + "vulnerabilities": WITH_FINDINGS}) + if RESULTS_SHAPE == "missing-key": + return self._send(200, {"scan_id": SCAN_ID, "status": "completed"}) + if RESULTS_SHAPE == "not-json": + return self._send_raw(200, b"

502 Bad Gateway

", + "text/html") + if RESULTS_SHAPE == "error-object": + return self._send(200, {"error": "Failed to fetch report from storage: Object not found"}) + raise SystemExit(f"unknown RESULTS_SHAPE {RESULTS_SHAPE!r}") + def do_POST(self): if urlparse(self.path).path != "/api/static/scan": return self._send(404, {"error": "not found"}) @@ -84,6 +123,9 @@ def do_GET(self): if n < COMPLETE_AFTER: return self._send(200, {"scan_id": SCAN_ID, "status": "processing"}) + if fmt == "json" and RESULTS_SHAPE != "ok" and n >= SHAPE_FROM: + return self._send_shaped_results() + completed = {"scan_id": SCAN_ID, "status": "completed", "vulnerabilities": []} if fmt == "md": completed["markdown"] = "# Rafter\n\nNo findings.\n" diff --git a/github-action/tests/test-action-yml-defaults.sh b/github-action/tests/test-action-yml-defaults.sh index 59444a3..5bc8740 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -163,6 +163,43 @@ else failures=$((failures+1)) fi +# ── sable-fgk7: an unreadable report is not a clean scan ───────────────── +# The results step used to coerce every jq failure into findings_count=0, +# which passed every threshold and rendered "No security findings detected". +# Reproduced with a 200 whose body was not JSON, a 200 carrying an error +# object, and a parseable payload with no vulnerabilities key. + +# 14. The payload shape must be validated before any count is computed. +if grep -qF "jq -e 'type == \"object\" and (.vulnerabilities | type == \"array\") and all(.vulnerabilities[]; type == \"object\")'" "$ACTION_YML"; then + echo "PASS: results step validates the payload shape before counting" +else + echo "FAIL: results step no longer validates that .vulnerabilities is an array of objects" + failures=$((failures+1)) +fi + +# 15. No count may fall back to 0 on a jq failure. That fallback IS the bug: +# the error path and the clean path produced the same number. +zero_fallbacks=$(grep -c '|| echo "0"' "$ACTION_YML" || true) +if [ "$zero_fallbacks" -eq 0 ]; then + echo "PASS: no count falls back to 0 on a parse failure" +else + echo "FAIL: ${zero_fallbacks} count(s) still fall back to 0 on a jq failure — an unreadable report would render as clean" + failures=$((failures+1)) +fi + +# 16. The unreadable-payload path must record status=unreadable and exit 1, +# so the declared status output cannot fall back to the poll step's +# 'completed' for a report that was never read. +if awk '/no .vulnerabilities. array/,/^ fi$/' "$ACTION_YML" \ + | grep -q 'status=unreadable' \ + && awk '/no .vulnerabilities. array/,/^ fi$/' "$ACTION_YML" \ + | grep -q 'exit 1'; then + echo "PASS: unreadable payload records status=unreadable and fails the step" +else + echo "FAIL: unreadable-payload branch no longer records status=unreadable and exits 1" + failures=$((failures+1)) +fi + echo "" echo "── results ───────────────────────────────────────────────────────────" echo "Failures: $failures" diff --git a/node/src/commands/issues/from-scan.ts b/node/src/commands/issues/from-scan.ts index 38852bc..f73f2ba 100644 --- a/node/src/commands/issues/from-scan.ts +++ b/node/src/commands/issues/from-scan.ts @@ -164,6 +164,36 @@ async function runFromScan(opts: { } } +/** + * The findings list from a scan payload, or an error — never a silent []. + * + * A payload without a `vulnerabilities` array is not "no findings". It is a + * scan that has not completed, a failed scan, or a report this client cannot + * read; filing zero issues from it would report a clean codebase for work + * that was never done (sable-fgk7). An empty array IS a legitimate clean + * result and is returned as such. + */ +export function vulnerabilitiesFromPayload( + data: unknown, + scanId: string +): BackendVulnerability[] { + const payload = data as { vulnerabilities?: unknown; status?: unknown } | null; + if (payload && Array.isArray(payload.vulnerabilities)) { + return payload.vulnerabilities as BackendVulnerability[]; + } + const status = payload && typeof payload.status === "string" ? payload.status : undefined; + if (status && status !== "completed") { + throw new Error( + `Scan ${scanId} is ${status}, not completed — there are no findings to file yet. ` + + `Retry once it completes: rafter get ${scanId}` + ); + } + throw new Error( + `Scan ${scanId} returned no 'vulnerabilities' array; refusing to treat an unreadable ` + + `report as zero findings. Check it with: rafter get ${scanId}` + ); +} + async function draftsFromBackendScan( scanId: string, apiKey?: string @@ -174,8 +204,7 @@ async function draftsFromBackendScan( headers: { "x-api-key": key }, }); - const vulns: BackendVulnerability[] = data.vulnerabilities || []; - return vulns.map(buildFromBackendVulnerability); + return vulnerabilitiesFromPayload(data, scanId).map(buildFromBackendVulnerability); } function draftsFromLocalScan(filePath: string): IssueDraft[] { diff --git a/node/tests/issues.test.ts b/node/tests/issues.test.ts index 4e81f42..aca114f 100644 --- a/node/tests/issues.test.ts +++ b/node/tests/issues.test.ts @@ -2,6 +2,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import crypto from "crypto"; import fs from "fs"; import { execFileSync } from "child_process"; +// The real function, not a mirror: these tests exist to fail when the source +// changes (sable-fgk7, sable-d2x2). +import { vulnerabilitiesFromPayload } from "../src/commands/issues/from-scan.js"; // ── dedup logic (mirrored from src/commands/issues/dedup.ts) ───────── @@ -657,6 +660,61 @@ describe("from-scan: backend vulnerability drafts", () => { }); }); +// ── from-scan: a payload with no findings list is an error, not zero ── +// +// sable-fgk7. `data.vulnerabilities || []` turned a still-running scan, a +// failed scan, a 200 carrying an error object, and a schema-valid payload with +// no key into "No findings to create issues for". Delete-the-subject test: +// with the fallback restored, every case below except the first two passes +// with [] and this suite fails. + +describe("from-scan: vulnerabilitiesFromPayload (sable-fgk7)", () => { + const SCAN = "scan-abc"; + + it("returns the list when present", () => { + const v = [{ ruleId: "r", level: "error", message: "m", file: "f" }]; + expect(vulnerabilitiesFromPayload({ status: "completed", vulnerabilities: v }, SCAN)).toBe(v); + }); + + it("returns an empty list as a legitimate clean result", () => { + expect(vulnerabilitiesFromPayload({ status: "completed", vulnerabilities: [] }, SCAN)).toEqual([]); + }); + + it("refuses a completed payload with no vulnerabilities key", () => { + expect(() => vulnerabilitiesFromPayload({ scan_id: SCAN, status: "completed" }, SCAN)) + .toThrow(/no 'vulnerabilities' array/); + }); + + it("names the status when the scan has not completed", () => { + expect(() => vulnerabilitiesFromPayload({ status: "processing" }, SCAN)) + .toThrow(/is processing, not completed/); + expect(() => vulnerabilitiesFromPayload({ status: "failed" }, SCAN)) + .toThrow(/is failed, not completed/); + }); + + it("refuses a 200 whose body is an error object", () => { + expect(() => + vulnerabilitiesFromPayload({ error: "Failed to fetch report from storage: Object not found" }, SCAN) + ).toThrow(/no 'vulnerabilities' array/); + }); + + it("refuses a vulnerabilities value that is not a list", () => { + expect(() => vulnerabilitiesFromPayload({ vulnerabilities: "3" }, SCAN)).toThrow(); + expect(() => vulnerabilitiesFromPayload({ vulnerabilities: null }, SCAN)).toThrow(); + }); + + it("refuses non-object payloads", () => { + expect(() => vulnerabilitiesFromPayload(null, SCAN)).toThrow(); + expect(() => vulnerabilitiesFromPayload("502", SCAN)).toThrow(); + }); + + it("points at the scan id in every refusal", () => { + for (const bad of [{ status: "completed" }, { status: "processing" }, {}]) { + expect(() => vulnerabilitiesFromPayload(bad, SCAN)).toThrow(new RegExp(`rafter get ${SCAN}`)); + } + }); +}); + // ── from-scan: --repo flag override ────────────────────────────────── describe("from-scan: repo flag", () => { diff --git a/python/rafter_cli/commands/issues/issues_app.py b/python/rafter_cli/commands/issues/issues_app.py index ec198c4..2e74bc2 100644 --- a/python/rafter_cli/commands/issues/issues_app.py +++ b/python/rafter_cli/commands/issues/issues_app.py @@ -14,7 +14,7 @@ import requests import typer -from ...utils.api import api_url, API_BASE, api_get, EXIT_GENERAL_ERROR, EXIT_SUCCESS, resolve_key +from ...utils.api import api_url, api_get, EXIT_GENERAL_ERROR, resolve_key from ...utils.formatter import fmt, print_stderr from ...utils.git import detect_repo from .dedup import find_duplicates @@ -74,7 +74,13 @@ def from_scan( # Build drafts if scan_id: - drafts = _drafts_from_backend(scan_id, api_key) + try: + drafts = _drafts_from_backend(scan_id, api_key) + except (UnreadableScanPayload, requests.RequestException, ValueError) as e: + # ValueError covers a non-JSON body from resp.json(). None of these + # is "no findings"; each is a scan whose report could not be read. + print_stderr(fmt.error(str(e))) + raise typer.Exit(code=EXIT_GENERAL_ERROR) else: drafts = _drafts_from_local(from_local) # type: ignore[arg-type] @@ -211,6 +217,33 @@ def from_text( # ── Internal helpers ────────────────────────────────────────────────── +class UnreadableScanPayload(ValueError): + """The scan payload carries no findings list this command can file from.""" + + +def vulnerabilities_from_payload(data: object, scan_id: str) -> list[dict]: + """The findings list from a scan payload, or an error — never a silent []. + + A payload without a ``vulnerabilities`` list is not "no findings". It is a + scan that has not completed, a failed scan, or a report this client cannot + read; filing zero issues from it would report a clean codebase for work + that was never done (sable-fgk7). An empty list IS a legitimate clean + result and is returned as such. + """ + if isinstance(data, dict) and isinstance(data.get("vulnerabilities"), list): + return data["vulnerabilities"] + status = data.get("status") if isinstance(data, dict) else None + if isinstance(status, str) and status != "completed": + raise UnreadableScanPayload( + f"Scan {scan_id} is {status}, not completed — there are no findings to " + f"file yet. Retry once it completes: rafter get {scan_id}" + ) + raise UnreadableScanPayload( + f"Scan {scan_id} returned no 'vulnerabilities' array; refusing to treat an " + f"unreadable report as zero findings. Check it with: rafter get {scan_id}" + ) + + def _drafts_from_backend(scan_id: str, api_key: str | None) -> list[IssueDraft]: key = resolve_key(api_key) resp = api_get( @@ -222,7 +255,7 @@ def _drafts_from_backend(scan_id: str, api_key: str | None) -> list[IssueDraft]: resp.raise_for_status() data = resp.json() - vulns = data.get("vulnerabilities", []) + vulns = vulnerabilities_from_payload(data, scan_id) return [ build_from_backend_vulnerability( BackendVulnerability( diff --git a/python/tests/test_issues.py b/python/tests/test_issues.py index f24bdf2..5497d0d 100644 --- a/python/tests/test_issues.py +++ b/python/tests/test_issues.py @@ -683,3 +683,122 @@ def _parse(self, text): def test_labels_are_unique(self): result = self._parse("Critical security vulnerability with credentials and tokens") assert len(result["labels"]) == len(set(result["labels"])) + + +# ── from-scan: a payload with no findings list is an error, not zero ── +# +# sable-fgk7. `data.get("vulnerabilities", [])` turned a still-running scan, a +# failed scan, a 200 carrying an error object, and a schema-valid payload with +# no key into "No findings to create issues for". Delete-the-subject test: +# with the fallback restored, every case below except the first two passes +# with [] and this class fails. Mirrors the Node suite of the same name. + + +class TestVulnerabilitiesFromPayload: + SCAN = "scan-abc" + + def _call(self, data): + from rafter_cli.commands.issues.issues_app import vulnerabilities_from_payload + + return vulnerabilities_from_payload(data, self.SCAN) + + def test_returns_the_list_when_present(self): + v = [{"ruleId": "r", "level": "error", "message": "m", "file": "f"}] + assert self._call({"status": "completed", "vulnerabilities": v}) is v + + def test_empty_list_is_a_legitimate_clean_result(self): + assert self._call({"status": "completed", "vulnerabilities": []}) == [] + + def test_refuses_completed_payload_with_no_key(self): + from rafter_cli.commands.issues.issues_app import UnreadableScanPayload + + with pytest.raises(UnreadableScanPayload, match="no 'vulnerabilities' array"): + self._call({"scan_id": self.SCAN, "status": "completed"}) + + def test_names_the_status_when_not_completed(self): + from rafter_cli.commands.issues.issues_app import UnreadableScanPayload + + with pytest.raises(UnreadableScanPayload, match="is processing, not completed"): + self._call({"status": "processing"}) + with pytest.raises(UnreadableScanPayload, match="is failed, not completed"): + self._call({"status": "failed"}) + + def test_refuses_error_object(self): + from rafter_cli.commands.issues.issues_app import UnreadableScanPayload + + with pytest.raises(UnreadableScanPayload, match="no 'vulnerabilities' array"): + self._call({"error": "Failed to fetch report from storage: Object not found"}) + + def test_refuses_vulnerabilities_that_is_not_a_list(self): + from rafter_cli.commands.issues.issues_app import UnreadableScanPayload + + with pytest.raises(UnreadableScanPayload): + self._call({"vulnerabilities": "3"}) + with pytest.raises(UnreadableScanPayload): + self._call({"vulnerabilities": None}) + + def test_refuses_non_dict_payloads(self): + from rafter_cli.commands.issues.issues_app import UnreadableScanPayload + + with pytest.raises(UnreadableScanPayload): + self._call(None) + with pytest.raises(UnreadableScanPayload): + self._call("502") + + def test_points_at_the_scan_id_in_every_refusal(self): + from rafter_cli.commands.issues.issues_app import UnreadableScanPayload + + for bad in ({"status": "completed"}, {"status": "processing"}, {}): + with pytest.raises(UnreadableScanPayload, match=f"rafter get {self.SCAN}"): + self._call(bad) + + +class TestFromScanCommandRefusesUnreadablePayload: + """The command surface: an unreadable payload exits 1 with the message on + stderr, and never reaches the "No findings to create issues for" path.""" + + def test_unreadable_payload_exits_1(self, monkeypatch, capsys): + from unittest.mock import MagicMock + + import typer + + from rafter_cli.commands.issues import issues_app as mod + + resp = MagicMock() + resp.status_code = 200 + resp.json.return_value = {"scan_id": "scan-abc", "status": "completed"} + resp.raise_for_status.return_value = None + monkeypatch.setattr(mod, "api_get", lambda *a, **k: resp) + monkeypatch.setattr(mod, "resolve_key", lambda _k: "key") + + with pytest.raises(typer.Exit) as exc: + mod.from_scan( + scan_id="scan-abc", from_local=None, repo="org/repo", api_key="k", + no_dedup=True, dry_run=True, quiet=False, + ) + assert exc.value.exit_code == 1 + err = capsys.readouterr().err + assert "no 'vulnerabilities' array" in err + assert "No findings to create issues for" not in err + + def test_non_json_body_exits_1(self, monkeypatch, capsys): + from unittest.mock import MagicMock + + import typer + + from rafter_cli.commands.issues import issues_app as mod + + resp = MagicMock() + resp.status_code = 200 + resp.json.side_effect = ValueError("Expecting value: line 1 column 1 (char 0)") + resp.raise_for_status.return_value = None + monkeypatch.setattr(mod, "api_get", lambda *a, **k: resp) + monkeypatch.setattr(mod, "resolve_key", lambda _k: "key") + + with pytest.raises(typer.Exit) as exc: + mod.from_scan( + scan_id="scan-abc", from_local=None, repo="org/repo", api_key="k", + no_dedup=True, dry_run=True, quiet=False, + ) + assert exc.value.exit_code == 1 + assert "No findings to create issues for" not in capsys.readouterr().err diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index cd23373..ba6e070 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -149,6 +149,7 @@ After either budget is exhausted the command exits `1`. If the failures reached - It has no "first poll" distinction: by the time it polls, the trigger step has already returned a `scan_id`, so **every** 404 there is treated as read-after-write lag. A scan id the backend accepted but never persisted therefore fails after the 5-failure budget rather than immediately. - Its poll loop is additionally bounded by a wall-clock deadline derived from `timeout-minutes`. Before v0.11 that input was a poll *count*, so a slow API could overrun it; it is now a real deadline. - Its `status` output is `completed`, `failed`, `timeout`, `unreadable` (the scan may have finished but its report could not be read), or `unreachable` (the API could not be contacted). +- Its results step validates the payload **before** counting. A body with no `vulnerabilities` array — not JSON, a `200` carrying an error object, or a parseable payload missing the key — is `status=unreadable`, the job fails, and **no count outputs are written**: a consumer reading `findings-count` sees an empty string, never a fabricated `0`. A report the action cannot read is not a clean scan. An empty array is a clean scan and counts as `0`. ### rafter usage [OPTIONS] @@ -1245,6 +1246,8 @@ Create GitHub issues from scan results. - `--dry-run` — show issues that would be created without actually creating them - `--quiet` — suppress status messages +**A scan payload without a `vulnerabilities` array is an error, not zero findings.** With `--scan-id`, a response that has no `vulnerabilities` list — the scan is still `processing`, it `failed`, the body is not JSON, or it is an error object — exits `1` with a message on stderr that names the scan id and `rafter get `. It never prints "No findings to create issues for". An empty list is a legitimate clean result. Both runtimes. + #### rafter issues create from-text [OPTIONS] Create a GitHub issue from natural language text (stdin, file, or inline). From bae67f3b16ca3c927b1f54336e169fd2cb4097e0 Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Wed, 2 Sep 2026 11:37:20 -0700 Subject: [PATCH 2/2] test: run the real code instead of hand-copied mirrors (sable-1drb) The action's threshold-eval and PR-comment-tip tests each carried their own transcription of the bash they tested, so they could pass in full while action.yml was broken. The logic now lives once in github-action/lib/severity.sh, sourced by both action.yml steps and by both tests. Behaviour unchanged. A new end-to-end job runs the real action with findings above the threshold and asserts status=completed AND outcome=failure, which proves the counts reach the gate; drift check 17 asserts action.yml sources the library in both steps and carries no inline copy, so the tests cannot be silently detached again. node/tests/issues.test.ts mirrored four modules (dedup, issue-builder, from-text, from-scan) and tested the mirrors; one had already drifted. The copies are removed and every describe imports the shipped function. Three source functions gained `export` for that. Every guard was mutated by hand: each mutation fails named tests that were untouched by the same mutation before this change. --- .github/workflows/test-github-action.yml | 58 +++++ github-action/action.yml | 36 +-- github-action/lib/severity.sh | 54 +++++ .../tests/test-action-yml-defaults.sh | 42 +++- github-action/tests/test-pr-comment-tip.sh | 36 +-- github-action/tests/test-threshold-eval.sh | 129 +++++----- node/src/commands/issues/from-scan.ts | 2 +- node/src/commands/issues/from-text.ts | 2 +- node/src/commands/issues/issue-builder.ts | 2 +- node/tests/issues.test.ts | 224 ++---------------- 10 files changed, 261 insertions(+), 324 deletions(-) create mode 100644 github-action/lib/severity.sh diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index 1491b3f..7a7665d 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -346,6 +346,64 @@ jobs: [ "$FAIL" -eq 0 ] && echo "PASS: counts are exactly the report's (3/1/1/0/1)." exit $FAIL + # sable-1drb — the threshold gate is now unit-tested against the real + # lib/severity.sh, but a unit test cannot prove action.yml WIRES it: that + # the counts reach the gate and the gate's verdict reaches the job. One + # end-to-end run with real findings and a threshold they exceed does. + test-threshold-gate-end-to-end: + name: "Threshold gate: real findings above the threshold fail the build" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (report has 1 critical, 1 high, 1 low) + env: + PORT: '8793' + FAIL_COUNT: '0' + COMPLETE_AFTER: '1' + RESULTS_SHAPE: 'with-findings' + run: | + nohup python3 github-action/tests/mock-rafter-api.py > mock.log 2>&1 & + for _ in $(seq 1 30); do + curl -sf -X POST -d '{}' http://127.0.0.1:8793/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8793/api/static/scan >/dev/null || { + echo "FAIL: mock backend never started"; cat mock.log; exit 1; } + + - name: Run the action with severity-threshold high + id: scan + continue-on-error: true + uses: ./github-action + with: + api-key: 'not-a-real-key' + rafter-url: 'http://127.0.0.1:8793' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + severity-threshold: 'high' + + - name: Assert the gate, not an error, failed the build + run: | + cat mock.log + FAIL=0 + # status=completed AND outcome=failure is the gate's signature: the + # report was read and counted, then the threshold rejected it. + if [ "${{ steps.scan.outputs.status }}" != "completed" ]; then + echo "FAIL: expected status=completed (report read), got '${{ steps.scan.outputs.status }}'" + FAIL=1 + fi + if [ "${{ steps.scan.outcome }}" != "failure" ]; then + echo "FAIL: 1 critical + 1 high with severity-threshold=high must fail the build (outcome='${{ steps.scan.outcome }}')" + FAIL=1 + fi + if [ "${{ steps.scan.outputs.findings-count }}" != "3" ]; then + echo "FAIL: findings-count expected 3, got '${{ steps.scan.outputs.findings-count }}'" + FAIL=1 + fi + [ "$FAIL" -eq 0 ] && echo "PASS: counts reached the gate and the gate failed the build." + exit $FAIL + test-yaml-validity: name: action.yml is valid YAML runs-on: ubuntu-latest diff --git a/github-action/action.yml b/github-action/action.yml index 8193cc9..8fe5ee3 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -356,6 +356,9 @@ runs: LOW_COUNT: ${{ steps.results.outputs.low_count }} SEVERITY_THRESHOLD: ${{ inputs.severity-threshold }} run: | + # Shared with the tests under tests/ — see lib/severity.sh (sable-1drb). + source "${{ github.action_path }}/lib/severity.sh" + MD_REPORT=$(jq -r '.markdown // empty' "${{ runner.temp }}/rafter-results.md" 2>/dev/null || cat "${{ runner.temp }}/rafter-results.md") if [ "$FINDINGS_COUNT" -eq 0 ]; then @@ -394,10 +397,7 @@ runs: echo "" echo "" echo "" - if [ "$FINDINGS_COUNT" -gt 0 ] && [ "$SEVERITY_THRESHOLD" = "none" ]; then - echo "> :information_source: This run is report-only. To fail the build on critical/high findings, set \`severity-threshold: high\` in your workflow." - echo "" - fi + rafter_report_only_tip "$FINDINGS_COUNT" "$SEVERITY_THRESHOLD" echo "---" echo "Scan ID: ${SCAN_ID} | Powered by [Rafter](https://rafter.so)" } >> "$COMMENT_FILE" @@ -432,31 +432,11 @@ runs: MEDIUM_COUNT: ${{ steps.results.outputs.medium_count }} LOW_COUNT: ${{ steps.results.outputs.low_count }} run: | - FAIL=0 - - case "$SEVERITY_THRESHOLD" in - critical) - [ "$CRITICAL_COUNT" -gt 0 ] && FAIL=1 - ;; - high) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] && FAIL=1 - ;; - medium) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] || [ "$MEDIUM_COUNT" -gt 0 ] && FAIL=1 - ;; - low) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] || [ "$MEDIUM_COUNT" -gt 0 ] || [ "$LOW_COUNT" -gt 0 ] && FAIL=1 - ;; - none) - FAIL=0 - ;; - *) - echo "::warning::Unknown severity threshold '${SEVERITY_THRESHOLD}', defaulting to 'high'" - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] && FAIL=1 - ;; - esac + # The case statement lives in lib/severity.sh so the unit tests under + # tests/ run the same code, not a transcription of it (sable-1drb). + source "${{ github.action_path }}/lib/severity.sh" - if [ "$FAIL" -eq 1 ]; then + if rafter_threshold_fails "$SEVERITY_THRESHOLD" "$CRITICAL_COUNT" "$HIGH_COUNT" "$MEDIUM_COUNT" "$LOW_COUNT"; then echo "::error::Security findings exceed severity threshold '${SEVERITY_THRESHOLD}'" exit 1 fi diff --git a/github-action/lib/severity.sh b/github-action/lib/severity.sh new file mode 100644 index 0000000..ab38d2f --- /dev/null +++ b/github-action/lib/severity.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# +# Severity-threshold logic shared by github-action/action.yml and the tests +# under github-action/tests/. ONE copy, sourced by both, so the tests exercise +# the code the action runs rather than a transcription of it (sable-1drb). +# +# Sourced, never executed: no `set -e`, no side effects at load time. Every +# function takes explicit arguments so a test can call it without staging +# environment variables, and prints only what the action wants in its log. + +# rafter_threshold_fails THRESHOLD CRITICAL HIGH MEDIUM LOW +# +# Returns 0 when the findings exceed THRESHOLD (the build should fail) and 1 +# otherwise. 'none' never fails. An unrecognised threshold behaves like +# 'high' and says so with a ::warning:: annotation. +rafter_threshold_fails() { + local threshold="$1" critical="$2" high="$3" medium="$4" low="$5" + local fail=0 + case "$threshold" in + critical) + [ "$critical" -gt 0 ] && fail=1 + ;; + high) + [ "$critical" -gt 0 ] || [ "$high" -gt 0 ] && fail=1 + ;; + medium) + [ "$critical" -gt 0 ] || [ "$high" -gt 0 ] || [ "$medium" -gt 0 ] && fail=1 + ;; + low) + [ "$critical" -gt 0 ] || [ "$high" -gt 0 ] || [ "$medium" -gt 0 ] || [ "$low" -gt 0 ] && fail=1 + ;; + none) + fail=0 + ;; + *) + echo "::warning::Unknown severity threshold '${threshold}', defaulting to 'high'" + [ "$critical" -gt 0 ] || [ "$high" -gt 0 ] && fail=1 + ;; + esac + [ "$fail" -eq 1 ] +} + +# rafter_report_only_tip FINDINGS_COUNT THRESHOLD +# +# Prints the report-only tip block for the PR comment iff there are findings +# AND the threshold is 'none' (the default), i.e. the run reported problems +# but was configured never to fail on them. Prints nothing otherwise. +rafter_report_only_tip() { + local findings="$1" threshold="$2" + if [ "$findings" -gt 0 ] && [ "$threshold" = "none" ]; then + echo "> :information_source: This run is report-only. To fail the build on critical/high findings, set \`severity-threshold: high\` in your workflow." + echo "" + fi +} diff --git a/github-action/tests/test-action-yml-defaults.sh b/github-action/tests/test-action-yml-defaults.sh index 5bc8740..d68b446 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -45,25 +45,49 @@ else failures=$((failures+1)) fi -# 3. The report-only tip block must be present and gated on both conditions. -if grep -qE '\[ "\$FINDINGS_COUNT" -gt 0 \] && \[ "\$SEVERITY_THRESHOLD" = "none" \]' "$ACTION_YML"; then - echo "PASS: report-only tip block gated on (findings > 0) AND (threshold == 'none')" +# The threshold case statement and the report-only tip live in lib/severity.sh +# (sable-1drb), sourced by action.yml AND by the unit tests, so checks 3, 4 +# and 17 look there. Check 17 is what stops a "simplification" from inlining +# a copy back into action.yml, which would silently detach the tests again. +SEVERITY_LIB="$(cd "$(dirname "$0")/.." && pwd)/lib/severity.sh" +if [ ! -f "$SEVERITY_LIB" ]; then + echo "FAIL: $SEVERITY_LIB not found" + exit 1 +fi + +# 3. The report-only tip must be gated on both conditions. +if grep -qE '\[ "\$findings" -gt 0 \] && \[ "\$threshold" = "none" \]' "$SEVERITY_LIB"; then + echo "PASS: report-only tip gated on (findings > 0) AND (threshold == 'none')" else - echo "FAIL: report-only tip block missing or mis-gated in $ACTION_YML" + echo "FAIL: report-only tip missing or mis-gated in $SEVERITY_LIB" failures=$((failures+1)) fi -# 4. The threshold-eval step must still handle 'none' as a no-op -# (no FAIL=1 in the none branch). +# 4. The threshold-eval must still handle 'none' as a no-op +# (no fail=1 in the none branch). if awk ' /none\)/ { in_none=1; next } in_none && /;;/ { in_none=0; next } in_none { print } -' "$ACTION_YML" | grep -qE "FAIL *= *1"; then - echo "FAIL: 'none' branch of threshold-eval sets FAIL=1 — that would break the default" +' "$SEVERITY_LIB" | grep -qE "fail *= *1"; then + echo "FAIL: 'none' branch of threshold-eval sets fail=1 — that would break the default" failures=$((failures+1)) else - echo "PASS: 'none' branch of threshold-eval does not set FAIL=1" + echo "PASS: 'none' branch of threshold-eval does not set fail=1" +fi + +# 17. action.yml must SOURCE the library in both steps that use it, and must +# not carry its own copy of the case statement. If either regresses, the +# unit tests go back to testing a transcription. +lib_sources=$(grep -cF 'source "${{ github.action_path }}/lib/severity.sh"' "$ACTION_YML" || true) +inline_cases=$(grep -cE '^\s*(critical|medium|low)\)\s*$' "$ACTION_YML" || true) +if [ "$lib_sources" -ge 2 ] && [ "$inline_cases" -eq 0 ] \ + && grep -q 'rafter_threshold_fails "\$SEVERITY_THRESHOLD"' "$ACTION_YML" \ + && grep -q 'rafter_report_only_tip "\$FINDINGS_COUNT" "\$SEVERITY_THRESHOLD"' "$ACTION_YML"; then + echo "PASS: action.yml sources lib/severity.sh in both steps and carries no inline copy" +else + echo "FAIL: action.yml sources=${lib_sources} (need >=2), inline case branches=${inline_cases} (need 0), or a call site is missing" + failures=$((failures+1)) fi # ── sable-l10k: poll-path retry contract ───────────────────────────────── diff --git a/github-action/tests/test-pr-comment-tip.sh b/github-action/tests/test-pr-comment-tip.sh index 01e8ba6..1ac97a1 100755 --- a/github-action/tests/test-pr-comment-tip.sh +++ b/github-action/tests/test-pr-comment-tip.sh @@ -1,39 +1,38 @@ #!/usr/bin/env bash # -# Unit test for the new PR-comment "report-only tip" block in -# github-action/action.yml. Re-implements the if block verbatim and -# exercises every input combination. +# Unit test for the PR-comment "report-only tip" block in +# github-action/action.yml. Sources github-action/lib/severity.sh — the SAME +# file action.yml sources at run time — and exercises every input +# combination of rafter_report_only_tip. # # The tip should appear iff (FINDINGS_COUNT > 0) AND (SEVERITY_THRESHOLD == 'none'). +# +# This test used to carry its own copy of the if block (sable-1drb). It now +# runs the code the action runs. set -u +# shellcheck source=../lib/severity.sh +source "$(cd "$(dirname "$0")/.." && pwd)/lib/severity.sh" + failures=0 total=0 -# Mirror of the new block under the "Comment on PR" step's COMMENT_FILE builder. -emit_tip_if_applicable() { - if [ "$FINDINGS_COUNT" -gt 0 ] && [ "$SEVERITY_THRESHOLD" = "none" ]; then - echo "> :information_source: This run is report-only. To fail the build on critical/high findings, set \`severity-threshold: high\` in your workflow." - echo "" - fi -} - TIP_NEEDLE="report-only" # assert_tip assert_tip() { local name="$1"; local expected="$2" - FINDINGS_COUNT="$3"; SEVERITY_THRESHOLD="$4" + local findings="$3" threshold="$4" total=$((total+1)) local out - out=$(emit_tip_if_applicable) + out=$(rafter_report_only_tip "$findings" "$threshold") local has_tip="no" if echo "$out" | grep -q "$TIP_NEEDLE"; then has_tip="yes"; fi if [ "$has_tip" != "$expected" ]; then - echo "FAIL: $name — findings=$FINDINGS_COUNT threshold=$SEVERITY_THRESHOLD → expected tip=$expected got $has_tip" + echo "FAIL: $name — findings=$findings threshold=$threshold → expected tip=$expected got $has_tip" failures=$((failures+1)) else echo "PASS: $name (tip=$has_tip)" @@ -53,6 +52,15 @@ assert_tip "findings + low" no 5 low assert_tip "no findings + high" no 0 high assert_tip "no findings + critical" no 0 critical +echo "── the tip must tell the reader what to set ─────────────────────────" +total=$((total+1)) +if rafter_report_only_tip 5 none | grep -q 'severity-threshold: high'; then + echo "PASS: tip names the input to set" +else + echo "FAIL: tip no longer names severity-threshold: high" + failures=$((failures+1)) +fi + echo "" echo "── results ───────────────────────────────────────────────────────────" echo "Total: $total Failures: $failures" diff --git a/github-action/tests/test-threshold-eval.sh b/github-action/tests/test-threshold-eval.sh index 99dbfc3..eddb5c6 100755 --- a/github-action/tests/test-threshold-eval.sh +++ b/github-action/tests/test-threshold-eval.sh @@ -1,108 +1,95 @@ #!/usr/bin/env bash # # Unit test for the "Evaluate severity threshold" step in -# github-action/action.yml. Re-implements the case statement verbatim and -# exercises every branch with deliberate inputs. +# github-action/action.yml. Sources github-action/lib/severity.sh — the SAME +# file action.yml sources at run time — and exercises every branch of +# rafter_threshold_fails with deliberate inputs. # -# If you change the case body in action.yml, you MUST change it here too — -# the test-action-yml-defaults check enforces drift detection on the default -# value, but the case body itself is duplicated by design (sourcing bash out -# of YAML at test time is fragile). +# This test used to carry its own copy of the case statement, so it could +# pass in full while action.yml was broken (sable-1drb). It now runs the code +# the action runs. The drift detector (test-action-yml-defaults.sh) separately +# asserts that action.yml still sources the library rather than inlining a +# copy again. # # Exit 0 = all cases pass. Exit 1 = at least one case failed. set -u +# shellcheck source=../lib/severity.sh +source "$(cd "$(dirname "$0")/.." && pwd)/lib/severity.sh" + failures=0 total=0 -# Mirror of the case body in github-action/action.yml under the -# "Evaluate severity threshold" step. Returns 1 if the threshold would -# fail the build given the current *_COUNT envs, else 0. -evaluate_threshold() { - local FAIL=0 - case "$SEVERITY_THRESHOLD" in - critical) - [ "$CRITICAL_COUNT" -gt 0 ] && FAIL=1 - ;; - high) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] && FAIL=1 - ;; - medium) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] || [ "$MEDIUM_COUNT" -gt 0 ] && FAIL=1 - ;; - low) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] || [ "$MEDIUM_COUNT" -gt 0 ] || [ "$LOW_COUNT" -gt 0 ] && FAIL=1 - ;; - none) - FAIL=0 - ;; - *) - [ "$CRITICAL_COUNT" -gt 0 ] || [ "$HIGH_COUNT" -gt 0 ] && FAIL=1 - ;; - esac - return $FAIL -} - -# assert_threshold +# assert_threshold assert_threshold() { local name="$1"; local expected="$2" - SEVERITY_THRESHOLD="$3" - CRITICAL_COUNT="$4"; HIGH_COUNT="$5"; MEDIUM_COUNT="$6"; LOW_COUNT="$7" + local threshold="$3" crit="$4" high="$5" med="$6" low="$7" total=$((total+1)) - evaluate_threshold - local actual=$? + local actual="pass" + if rafter_threshold_fails "$threshold" "$crit" "$high" "$med" "$low" >/dev/null; then + actual="fail" + fi if [ "$actual" != "$expected" ]; then - echo "FAIL: $name — threshold=$SEVERITY_THRESHOLD crit=$CRITICAL_COUNT high=$HIGH_COUNT med=$MEDIUM_COUNT low=$LOW_COUNT → expected exit=$expected got $actual" + echo "FAIL: $name — threshold=$threshold crit=$crit high=$high med=$med low=$low → expected build=$expected got $actual" failures=$((failures+1)) else echo "PASS: $name" fi } -echo "── 'none' threshold (the new default) — must never fail ─────────────" -assert_threshold "none + no findings" 0 none 0 0 0 0 -assert_threshold "none + low only" 0 none 0 0 0 7 -assert_threshold "none + medium only" 0 none 0 0 3 0 -assert_threshold "none + high only" 0 none 0 5 0 0 -assert_threshold "none + critical only" 0 none 2 0 0 0 -assert_threshold "none + everything" 0 none 9 9 9 9 +echo "── 'none' threshold (the default) — must never fail ────────────────" +assert_threshold "none + no findings" pass none 0 0 0 0 +assert_threshold "none + low only" pass none 0 0 0 7 +assert_threshold "none + medium only" pass none 0 0 3 0 +assert_threshold "none + high only" pass none 0 5 0 0 +assert_threshold "none + critical only" pass none 2 0 0 0 +assert_threshold "none + everything" pass none 9 9 9 9 echo "── 'critical' threshold — fail only on critical ────────────────────" -assert_threshold "critical + clean" 0 critical 0 0 0 0 -assert_threshold "critical + only high" 0 critical 0 4 0 0 -assert_threshold "critical + only medium" 0 critical 0 0 4 0 -assert_threshold "critical + only low" 0 critical 0 0 0 4 -assert_threshold "critical + critical=1" 1 critical 1 0 0 0 -assert_threshold "critical + critical+high" 1 critical 1 5 0 0 +assert_threshold "critical + clean" pass critical 0 0 0 0 +assert_threshold "critical + only high" pass critical 0 4 0 0 +assert_threshold "critical + only medium" pass critical 0 0 4 0 +assert_threshold "critical + only low" pass critical 0 0 0 4 +assert_threshold "critical + critical=1" fail critical 1 0 0 0 +assert_threshold "critical + critical+high" fail critical 1 5 0 0 echo "── 'high' threshold — fail on critical or high ─────────────────────" -assert_threshold "high + clean" 0 high 0 0 0 0 -assert_threshold "high + only medium" 0 high 0 0 4 0 -assert_threshold "high + only low" 0 high 0 0 0 4 -assert_threshold "high + critical only" 1 high 1 0 0 0 -assert_threshold "high + high only" 1 high 0 1 0 0 -assert_threshold "high + critical+high" 1 high 1 1 0 0 +assert_threshold "high + clean" pass high 0 0 0 0 +assert_threshold "high + only medium" pass high 0 0 4 0 +assert_threshold "high + only low" pass high 0 0 0 4 +assert_threshold "high + critical only" fail high 1 0 0 0 +assert_threshold "high + high only" fail high 0 1 0 0 +assert_threshold "high + critical+high" fail high 1 1 0 0 echo "── 'medium' threshold — fail on crit/high/medium ───────────────────" -assert_threshold "medium + clean" 0 medium 0 0 0 0 -assert_threshold "medium + only low" 0 medium 0 0 0 4 -assert_threshold "medium + critical only" 1 medium 1 0 0 0 -assert_threshold "medium + high only" 1 medium 0 1 0 0 -assert_threshold "medium + medium only" 1 medium 0 0 1 0 +assert_threshold "medium + clean" pass medium 0 0 0 0 +assert_threshold "medium + only low" pass medium 0 0 0 4 +assert_threshold "medium + critical only" fail medium 1 0 0 0 +assert_threshold "medium + high only" fail medium 0 1 0 0 +assert_threshold "medium + medium only" fail medium 0 0 1 0 echo "── 'low' threshold — fail on anything ──────────────────────────────" -assert_threshold "low + clean" 0 low 0 0 0 0 -assert_threshold "low + only low" 1 low 0 0 0 1 -assert_threshold "low + critical only" 1 low 1 0 0 0 +assert_threshold "low + clean" pass low 0 0 0 0 +assert_threshold "low + only low" fail low 0 0 0 1 +assert_threshold "low + critical only" fail low 1 0 0 0 echo "── unknown threshold — falls back to 'high' behavior ───────────────" -assert_threshold "unknown + clean" 0 badvalue 0 0 0 0 -assert_threshold "unknown + critical" 1 badvalue 1 0 0 0 -assert_threshold "unknown + high" 1 badvalue 0 1 0 0 -assert_threshold "unknown + medium only" 0 badvalue 0 0 3 0 +assert_threshold "unknown + clean" pass badvalue 0 0 0 0 +assert_threshold "unknown + critical" fail badvalue 1 0 0 0 +assert_threshold "unknown + high" fail badvalue 0 1 0 0 +assert_threshold "unknown + medium only" pass badvalue 0 0 3 0 + +echo "── unknown threshold — must say so in the log ──────────────────────" +total=$((total+1)) +if rafter_threshold_fails badvalue 0 0 0 0 | grep -q "::warning::Unknown severity threshold 'badvalue'"; then + echo "PASS: unknown threshold emits a ::warning:: naming the value" +else + echo "FAIL: unknown threshold no longer warns" + failures=$((failures+1)) +fi echo "" echo "── results ───────────────────────────────────────────────────────────" diff --git a/node/src/commands/issues/from-scan.ts b/node/src/commands/issues/from-scan.ts index f73f2ba..aca9f0f 100644 --- a/node/src/commands/issues/from-scan.ts +++ b/node/src/commands/issues/from-scan.ts @@ -207,7 +207,7 @@ async function draftsFromBackendScan( return vulnerabilitiesFromPayload(data, scanId).map(buildFromBackendVulnerability); } -function draftsFromLocalScan(filePath: string): IssueDraft[] { +export function draftsFromLocalScan(filePath: string): IssueDraft[] { const raw = fs.readFileSync(filePath, "utf-8"); const parsed = JSON.parse(raw); // New shape: { _note, scan_mode, triage_applied, results: [...] } diff --git a/node/src/commands/issues/from-text.ts b/node/src/commands/issues/from-text.ts index 1298263..947f2e0 100644 --- a/node/src/commands/issues/from-text.ts +++ b/node/src/commands/issues/from-text.ts @@ -130,7 +130,7 @@ async function readInput(opts: { * - File paths → mentioned in body * - Security keywords → security label */ -function parseNaturalText(text: string): ParsedIssue { +export function parseNaturalText(text: string): ParsedIssue { const lines = text.trim().split("\n"); const labels: string[] = []; diff --git a/node/src/commands/issues/issue-builder.ts b/node/src/commands/issues/issue-builder.ts index 04853ab..b9209d5 100644 --- a/node/src/commands/issues/issue-builder.ts +++ b/node/src/commands/issues/issue-builder.ts @@ -35,7 +35,7 @@ export interface LocalScanResult { }>; } -function severityLabel(level: string): string { +export function severityLabel(level: string): string { const map: Record = { error: "critical", critical: "critical", diff --git a/node/tests/issues.test.ts b/node/tests/issues.test.ts index aca114f..77fe6a4 100644 --- a/node/tests/issues.test.ts +++ b/node/tests/issues.test.ts @@ -1,47 +1,29 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import crypto from "crypto"; +import { describe, it, expect, beforeEach, afterEach } from "vitest"; import fs from "fs"; -import { execFileSync } from "child_process"; -// The real function, not a mirror: these tests exist to fail when the source -// changes (sable-fgk7, sable-d2x2). -import { vulnerabilitiesFromPayload } from "../src/commands/issues/from-scan.js"; - -// ── dedup logic (mirrored from src/commands/issues/dedup.ts) ───────── - -const FINGERPRINT_PREFIX = ""; - -function fingerprint(file: string, ruleId: string): string { - return crypto - .createHash("sha256") - .update(`${file}:${ruleId}`) - .digest("hex") - .slice(0, 12); -} - -function embedFingerprint(body: string, fp: string): string { - return `${body}\n\n${FINGERPRINT_PREFIX}${fp}${FINGERPRINT_SUFFIX}`; -} - -function extractFingerprint(body: string): string | null { - const idx = body.indexOf(FINGERPRINT_PREFIX); - if (idx === -1) return null; - const start = idx + FINGERPRINT_PREFIX.length; - const end = body.indexOf(FINGERPRINT_SUFFIX, start); - if (end === -1) return null; - return body.slice(start, end); -} - -type GHIssue = { number: number; title: string; body: string; labels: string[]; html_url: string; state: string }; - -function findDuplicates(existingIssues: GHIssue[], newFingerprints: string[]): Set { - const existingFps = new Set(); - for (const issue of existingIssues) { - const fp = extractFingerprint(issue.body); - if (fp) existingFps.add(fp); - } - return new Set(newFingerprints.filter((fp) => existingFps.has(fp))); -} +// The real functions, not mirrors. This file used to carry hand-copied +// re-implementations of dedup, issue-builder, from-text and from-scan and +// tested those; it could pass in full while the source was broken +// (sable-1drb, specimen 11 of sable-d2x2). Every describe below now +// exercises the code the CLI ships. +import { + fingerprint, + embedFingerprint, + extractFingerprint, + findDuplicates, +} from "../src/commands/issues/dedup.js"; +import type { GitHubIssue as GHIssue } from "../src/commands/issues/github-client.js"; +import { + severityLabel, + buildFromBackendVulnerability, + buildFromLocalMatch, + type IssueDraft, + type BackendVulnerability, + type LocalScanResult, +} from "../src/commands/issues/issue-builder.js"; +import { parseNaturalText } from "../src/commands/issues/from-text.js"; +import { vulnerabilitiesFromPayload, draftsFromLocalScan } from "../src/commands/issues/from-scan.js"; + +type LocalMatch = LocalScanResult["matches"][number]; describe("fingerprint", () => { it("produces deterministic 12-char hex hash", () => { @@ -111,91 +93,6 @@ describe("findDuplicates", () => { }); }); -// ── issue-builder logic (mirrored from src/commands/issues/issue-builder.ts) ── - -function severityLabel(level: string): string { - const map: Record = { - error: "critical", - critical: "critical", - warning: "high", - high: "high", - note: "medium", - medium: "medium", - low: "low", - }; - return map[level.toLowerCase()] || "medium"; -} - -function severityEmoji(level: string): string { - const sev = severityLabel(level); - const emojis: Record = { - critical: "\u{1F534}", - high: "\u{1F7E0}", - medium: "\u{1F7E1}", - low: "\u{1F7E2}", - }; - return emojis[sev] || "\u{1F7E1}"; -} - -interface IssueDraft { - title: string; - body: string; - labels: string[]; - fingerprint: string; -} - -interface BackendVulnerability { - ruleId: string; - level: string; - message: string; - file: string; - line?: number; -} - -function buildFromBackendVulnerability(vuln: BackendVulnerability): IssueDraft { - const sev = severityLabel(vuln.level); - const emoji = severityEmoji(vuln.level); - const fp = fingerprint(vuln.file, vuln.ruleId); - const title = `${emoji} [${sev.toUpperCase()}] ${vuln.ruleId}: ${vuln.message.length > 80 ? vuln.message.slice(0, 77) + "..." : vuln.message}`; - let body = `## Security Finding\n\n`; - body += `**Rule:** \`${vuln.ruleId}\`\n`; - body += `**Severity:** ${sev}\n`; - body += `**File:** \`${vuln.file}\``; - if (vuln.line) body += ` (line ${vuln.line})`; - body += `\n\n`; - body += `### Description\n\n${vuln.message}\n\n`; - body += `### Remediation\n\nReview and fix the finding in \`${vuln.file}\`.\n`; - body += `\n---\n*Created by [Rafter CLI](https://rafter.so) — security for AI builders*\n`; - const labels = ["security", `severity:${sev}`, `rule:${vuln.ruleId}`]; - return { title, body: embedFingerprint(body, fp), labels, fingerprint: fp }; -} - -type LocalMatch = { pattern: { name: string; severity: string; description?: string }; line?: number; column?: number; redacted?: string }; - -function buildFromLocalMatch(file: string, match: LocalMatch): IssueDraft { - const sev = severityLabel(match.pattern.severity); - const emoji = severityEmoji(match.pattern.severity); - const fp = fingerprint(file, match.pattern.name); - const basename = file.split("/").pop() || file; - const title = `${emoji} [${sev.toUpperCase()}] Secret detected: ${match.pattern.name} in ${basename}`; - let body = `## Secret Detection\n\n`; - body += `**Pattern:** \`${match.pattern.name}\`\n`; - body += `**Severity:** ${sev}\n`; - body += `**File:** \`${file}\``; - if (match.line) body += ` (line ${match.line})`; - body += `\n`; - if (match.redacted) body += `**Match:** \`${match.redacted}\`\n`; - body += `\n`; - if (match.pattern.description) body += `### Description\n\n${match.pattern.description}\n\n`; - body += `### Remediation\n\n`; - body += `1. Rotate the exposed credential immediately\n`; - body += `2. Remove the secret from source code\n`; - body += `3. Use environment variables or a secrets manager instead\n`; - body += `\n---\n*Created by [Rafter CLI](https://rafter.so) — security for AI builders*\n`; - const labels = ["security", "secret-detected", `severity:${sev}`]; - return { title, body: embedFingerprint(body, fp), labels, fingerprint: fp }; -} - describe("severityLabel", () => { it("maps error to critical", () => expect(severityLabel("error")).toBe("critical")); it("maps warning to high", () => expect(severityLabel("warning")).toBe("high")); @@ -288,63 +185,6 @@ describe("buildFromLocalMatch", () => { }); }); -// ── from-text parsing logic (mirrored from src/commands/issues/from-text.ts) ── - -interface ParsedIssue { - title: string; - body: string; - labels: string[]; -} - -function parseNaturalText(text: string): ParsedIssue { - const lines = text.trim().split("\n"); - const labels: string[] = []; - let title = ""; - let bodyStart = 0; - for (let i = 0; i < lines.length; i++) { - const line = lines[i].trim(); - if (line) { - title = line.replace(/^#+\s*/, "").trim(); - bodyStart = i + 1; - break; - } - } - if (!title) title = "Security issue reported via Rafter CLI"; - if (title.length > 120) title = title.slice(0, 117) + "..."; - const bodyLines = lines.slice(bodyStart); - let body = bodyLines.join("\n").trim(); - if (!body) body = text.trim(); - const textLower = text.toLowerCase(); - if (textLower.includes("critical") || textLower.includes("p0")) { - labels.push("severity:critical"); - } else if (textLower.includes("high severity") || textLower.includes("high risk") || textLower.includes("p1")) { - labels.push("severity:high"); - } else if (textLower.includes("medium") || textLower.includes("p2")) { - labels.push("severity:medium"); - } else if (textLower.includes("low") || textLower.includes("p3")) { - labels.push("severity:low"); - } - const securityKeywords = [ - "security", "vulnerability", "cve", "cwe", "owasp", "secret", - "credential", "token", "password", "injection", "xss", "csrf", "ssrf", "exploit", - ]; - if (securityKeywords.some((kw) => textLower.includes(kw))) { - labels.push("security"); - } - const fileRefs = text.match(/(?:^|\s)([a-zA-Z0-9_./-]+\.[a-zA-Z]{1,10})(?::(\d+))?/gm); - if (fileRefs && fileRefs.length > 0) { - const files = fileRefs.map((f) => f.trim()).filter((f) => f.includes("/") || f.includes(".")); - if (files.length > 0) { - body += `\n\n### Referenced Files\n\n`; - for (const f of files.slice(0, 10)) { - body += `- \`${f}\`\n`; - } - } - } - body += `\n\n---\n*Created by [Rafter CLI](https://rafter.so) — security for AI builders*\n`; - return { title, body, labels: [...new Set(labels)] }; -} - describe("parseNaturalText", () => { it("extracts first line as title", () => { const result = parseNaturalText("SQL injection in login form\nDetails here"); @@ -416,20 +256,6 @@ describe("parseNaturalText", () => { }); }); -// ── from-scan command logic (mirrored from src/commands/issues/from-scan.ts) ── - -function draftsFromLocalScan(filePath: string): IssueDraft[] { - const raw = fs.readFileSync(filePath, "utf-8"); - const results: Array<{ file: string; matches: LocalMatch[] }> = JSON.parse(raw); - const drafts: IssueDraft[] = []; - for (const result of results) { - for (const match of result.matches) { - drafts.push(buildFromLocalMatch(result.file, match)); - } - } - return drafts; -} - describe("from-scan: draftsFromLocalScan", () => { const sampleScanJson: Array<{ file: string; matches: LocalMatch[] }> = [ {