diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0469b168..3f9e41a5 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -32,4 +32,4 @@ Rafter is a security CLI for AI coding agents. It ships as two feature-identical - Scanners use dual-engine: Betterleaks binary first, regex fallback. Patterns defined in `secret-patterns.ts` / `secret_patterns.py` - Risk classification: critical > high > medium > low - Audit log: JSONL format, append-only, documented schema in CLI_SPEC.md -- MCP server: 4 tools + 2 resources over stdio transport +- MCP server: 4 tools + 3 resources over stdio transport diff --git a/.github/scripts/classifier_battery_floor.py b/.github/scripts/classifier_battery_floor.py new file mode 100644 index 00000000..11f2fbaa --- /dev/null +++ b/.github/scripts/classifier_battery_floor.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +"""Drift check for the command-classifier battery. + +The battery and the differential catch different things, and CI needs both: + + * The DIFFERENTIAL (PR vs origin/main) catches a REGRESSION — this branch + weaker than main. It is what a hand-written battery misses, because a + battery only asks the questions someone thought to ask. + * The BATTERY catches a MISSING fix and an OVER-BLOCK. It is what the + differential misses, because a fix absent on BOTH sides is not a permissive + *move* and shows up as nothing: the differential ran CLEAN against a branch + that had lost a live P0 fix entirely. + +That makes the battery's CONTENTS load-bearing, and a load-bearing list nobody +can see shrink is a list that will shrink. This is sable-d2x2 rule C applied to +the gate itself rather than to the suite it guards. + +Three assertions: + + * the battery has not shrunk below the floor; + * it still has rows gating the UNDER-block direction (want includes + "critical") — the rows that catch a fix going missing; + * it still has rows gating the OVER-block direction (want is exactly + ["low"]) — without them, blocking everything passes the gate. #230 was an + over-block report, so a battery with no low rows would have been happy to + ship it. + +Lower the floor in the same PR that removes cases, so a shrink is a reviewed +decision rather than an accident. +""" +from __future__ import annotations + +import argparse +import json +import os +import sys + +HERE = os.path.dirname(os.path.abspath(__file__)) +BATTERY = os.path.join(HERE, "..", "..", "rf-6pqx-newline-heredoc-battery.json") + + +def main() -> int: + ap = argparse.ArgumentParser() + ap.add_argument("--min-cases", type=int, required=True) + ap.add_argument("--min-critical", type=int, default=15) + ap.add_argument("--min-low", type=int, default=10) + ap.add_argument("--battery", default=BATTERY) + args = ap.parse_args() + + try: + cases = json.load(open(args.battery)) + except (OSError, ValueError) as e: + print(f"FAIL: battery unreadable at {args.battery}: {e}") + return 1 + + if not isinstance(cases, list) or not cases: + print( + "FAIL: battery is not a non-empty list — a gate with no cases " + "passes everything, which is worse than no gate at all" + ) + return 1 + + critical = [c for c in cases if "critical" in c.get("want", [])] + low = [c for c in cases if c.get("want") == ["low"]] + + failures = [] + if len(cases) < args.min_cases: + failures.append( + f"battery shrank: {len(cases)} cases, floor is {args.min_cases}. " + "If cases were removed on purpose, lower the floor in the same PR." + ) + if len(critical) < args.min_critical: + failures.append( + f"only {len(critical)} rows gate the under-block direction " + f"(want includes 'critical'), floor is {args.min_critical}. Those " + "are the rows that catch a fix going missing." + ) + if len(low) < args.min_low: + failures.append( + f"only {len(low)} rows gate the over-block direction " + f"(want is exactly ['low']), floor is {args.min_low}. Without " + "those, blocking everything passes the gate." + ) + + # A malformed row is a row that cannot fail. Catch it here rather than + # letting a harness quietly skip it. + for i, c in enumerate(cases): + if not isinstance(c.get("cmd"), str) or not c["cmd"]: + failures.append(f"case {i} ({c.get('label', '?')!r}) has no command") + want = c.get("want") + if not isinstance(want, list) or not want: + failures.append(f"case {i} ({c.get('label', '?')!r}) has no expectations") + + if failures: + print("FAIL: command-classifier battery drift") + for f in failures: + print(f" - {f}") + return 1 + + print( + f"PASS: battery has {len(cases)} cases " + f"({len(critical)} gate under-blocking, {len(low)} gate over-blocking)" + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/test_floor.py b/.github/scripts/test_floor.py new file mode 100644 index 00000000..a148d326 --- /dev/null +++ b/.github/scripts/test_floor.py @@ -0,0 +1,170 @@ +#!/usr/bin/env python3 +"""Fail CI when the test suite quietly shrinks or a file's tests all skip. + +sable-d2x2 detection rule C ("assert non-emptiness"). Two ways a green run can +be vacuous that the runner's own exit code does not catch: + + 1. A whole file's tests are skipped. sable-cazq: 40 parity tests were + `describe.skip`'d because Python was missing, and the release path exited + 0. A total-count floor does NOT catch this (2112 - 40 is still a big + number); a per-file "every test skipped" rule does. + 2. The suite shrinks sharply: a config change, a renamed directory, a broken + glob, and the runner cheerfully runs the 30 tests it found. + +Reads a vitest JSON report (--vitest) or a pytest JUnit XML written with +`-o junit_family=xunit1` (--junit; xunit1 is what carries the per-test `file` +attribute). Exits 1 when: + + * executed tests (passed + failed) < --min-executed, or + * any file has >= 1 test and every one of them was skipped, unless that + file is listed in --allow-all-skipped (a visible, reviewed exception). + +Always writes the executed / skipped counts and every skipped test's name to +$GITHUB_STEP_SUMMARY when set, so a skip is never invisible even when it is +allowed. Stdlib only: this runs before any project dependency is guaranteed. +""" +from __future__ import annotations + +import argparse +import json +import os +import sys +import xml.etree.ElementTree as ET +from collections import defaultdict + +SKIPPED_STATES = {"skipped", "pending", "todo", "disabled"} + + +def read_vitest(path: str) -> dict[str, dict[str, list[str]]]: + """{file: {"executed": [names], "skipped": [names]}} from a vitest JSON report.""" + with open(path, encoding="utf-8") as fh: + report = json.load(fh) + files: dict[str, dict[str, list[str]]] = {} + cwd = os.getcwd() + os.sep + for result in report.get("testResults", []): + name = result.get("name", "") + rel = name[len(cwd):] if name.startswith(cwd) else name + bucket = files.setdefault(rel, {"executed": [], "skipped": []}) + for case in result.get("assertionResults", []): + title = case.get("fullName") or case.get("title") or "" + state = case.get("status", "") + (bucket["skipped"] if state in SKIPPED_STATES else bucket["executed"]).append(title) + return files + + +def read_junit(path: str) -> dict[str, dict[str, list[str]]]: + """Same shape from a pytest JUnit XML (xunit1 family, which carries `file`).""" + root = ET.parse(path).getroot() + files: dict[str, dict[str, list[str]]] = defaultdict(lambda: {"executed": [], "skipped": []}) + missing_file_attr = 0 + for case in root.iter("testcase"): + file_attr = case.get("file") + if not file_attr: + missing_file_attr += 1 + # xunit2 drops `file`; fall back to the module part of classname so + # the per-file rule still has something to group by. + classname = case.get("classname", "") + parts = [p for p in classname.split(".") if p and not p[:1].isupper()] + file_attr = "/".join(parts) + ".py" if parts else "" + title = f'{case.get("classname", "")}::{case.get("name", "")}' + skipped = case.find("skipped") is not None + (files[file_attr]["skipped"] if skipped else files[file_attr]["executed"]).append(title) + if missing_file_attr: + print( + f"::warning::{missing_file_attr} testcase(s) had no `file` attribute; " + "run pytest with `-o junit_family=xunit1` for exact per-file grouping.", + flush=True, + ) + return dict(files) + + +def summarize(label: str, files: dict[str, dict[str, list[str]]], executed: int, + skipped: int, min_executed: int, all_skipped: list[str], + allowed: set[str]) -> str: + lines = [f"### Test floor — {label}", ""] + lines.append("| Executed | Skipped | Floor | Files |") + lines.append("|---------:|--------:|------:|------:|") + lines.append(f"| {executed} | {skipped} | {min_executed} | {len(files)} |") + lines.append("") + if all_skipped: + lines.append("**Files with every test skipped:**") + for f in all_skipped: + tag = " (allowed by --allow-all-skipped)" if f in allowed else " **← FAIL**" + lines.append(f"- `{f}`{tag}") + lines.append("") + if skipped: + lines.append("
Skipped tests") + lines.append("") + for f, bucket in sorted(files.items()): + for name in bucket["skipped"]: + lines.append(f"- `{f}` — {name}") + lines.append("") + lines.append("
") + lines.append("") + return "\n".join(lines) + + +def main(argv: list[str]) -> int: + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + src = ap.add_mutually_exclusive_group(required=True) + src.add_argument("--vitest", help="vitest JSON report (--reporter=json)") + src.add_argument("--junit", help="pytest JUnit XML (-o junit_family=xunit1 --junitxml=...)") + ap.add_argument("--min-executed", type=int, required=True, + help="fail if fewer than this many tests actually ran (passed + failed)") + ap.add_argument("--allow-all-skipped", default="", + help="comma-separated files allowed to have every test skipped") + ap.add_argument("--label", default=None, help="label for the step summary") + args = ap.parse_args(argv) + + if args.vitest: + files = read_vitest(args.vitest) + label = args.label or "vitest" + else: + files = read_junit(args.junit) + label = args.label or "pytest" + + allowed = {f.strip() for f in args.allow_all_skipped.split(",") if f.strip()} + executed = sum(len(b["executed"]) for b in files.values()) + skipped = sum(len(b["skipped"]) for b in files.values()) + all_skipped = sorted(f for f, b in files.items() if b["skipped"] and not b["executed"]) + offending = [f for f in all_skipped if f not in allowed] + + summary = summarize(label, files, executed, skipped, args.min_executed, all_skipped, allowed) + print(summary, flush=True) + step_summary = os.environ.get("GITHUB_STEP_SUMMARY") + if step_summary: + with open(step_summary, "a", encoding="utf-8") as fh: + fh.write(summary + "\n") + + failed = False + if not files: + print(f"::error::{label}: the report lists no test files at all — nothing ran.", flush=True) + failed = True + if executed < args.min_executed: + print( + f"::error::{label}: only {executed} tests executed, floor is {args.min_executed}. " + "If tests were deliberately removed, lower the floor in the workflow in the same PR " + "so the shrink is a reviewed decision, not a silent one.", + flush=True, + ) + failed = True + for f in offending: + print( + f"::error::{label}: every test in {f} was skipped ({len(files[f]['skipped'])} tests). " + "A file that runs nothing is a broken prerequisite, not a passing file. Fix the " + "prerequisite, or list the file in --allow-all-skipped with a reason in the workflow.", + flush=True, + ) + failed = True + if not failed: + allowed_note = ( + f", {len(all_skipped)} fully-skipped file(s) on the allow-list" if all_skipped + else ", no file fully skipped" + ) + print(f"OK: {label}: {executed} executed (floor {args.min_executed}), " + f"{skipped} skipped{allowed_note}.", flush=True) + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/.github/workflows/publish.yaml b/.github/workflows/publish.yaml index 176b1899..1b1704ab 100644 --- a/.github/workflows/publish.yaml +++ b/.github/workflows/publish.yaml @@ -26,14 +26,85 @@ jobs: with: node-version: "20" + # sable-cazq — tests/cross-runtime-parity.test.ts gates its whole + # describe block on `python3 -c "import typer"` succeeding, and + # describe.skip is silent: with no Python here the release path ran + # `pnpm test`, skipped all 40 parity assertions and reported green. + # Those are the tests that enforce the dual-implementation contract, + # so they are the ones a release least wants to skip. Mirrors the + # setup in test-comprehensive.yml's test-node. + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + - name: Enable pnpm run: corepack enable && corepack prepare pnpm@10 --activate - name: Install dependencies run: pnpm install --frozen-lockfile + - name: Install Python dependencies (for cross-runtime parity tests) + working-directory: ./python + run: | + pip install -e ".[dev]" 2>/dev/null || pip install -e . + + # The rf-6pqx differential FAILS rather than skips when main's classifier is + # unobtainable, and this job runs the FULL suite — so the publish path needs + # the baseline too, or the release breaks at the moment it fires. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + - name: Run tests - run: pnpm test + run: pnpm exec vitest run --reporter=default --reporter=json --outputFile.json=vitest-report.json + + # sable-d2x2 rule C — the release path is exactly where sable-cazq's 40 + # silently-skipped tests reported green. Same floor as + # test-comprehensive.yml; keep the two in step. + - name: Assert the suite actually ran + run: python3 ../.github/scripts/test_floor.py --vitest vitest-report.json --min-executed 1990 --label "release test-node (vitest)" + + # sable-cazq — python/tests/ ran in exactly one place in this repo + # (test-comprehensive.yml) and it was not the release path. publish.yaml + # had no Python test job at all and validate-release.yml only built the + # wheel, so a Python-only regression reached PyPI green — and PyPI is not + # somewhere you can quietly unship from. Mirrors test-comprehensive.yml's + # test-python; runs in parallel with test-node, so it costs no wall clock + # the release was not already spending. + test-python: + runs-on: ubuntu-latest + defaults: + run: + working-directory: ./python + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + + # pyproject.toml is Poetry-style: dev deps live under + # [tool.poetry.group.dev.dependencies], which is not a PEP 621 extra, + # so `.[dev]` always falls through to the bare install. The explicit + # pytest line is what actually provides the test deps. + - name: Install dependencies + run: | + pip install -e ".[dev]" 2>/dev/null || pip install -e . + pip install pytest pytest-mock pytest-asyncio + + # The rf-6pqx differential FAILS rather than skips when main's classifier is + # unobtainable, and this job runs the FULL suite — so the publish path needs + # the baseline too, or the release breaks at the moment it fires. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + + - name: Run all tests + run: python -m pytest tests/ -v -o junit_family=xunit1 --junitxml=pytest-report.xml + env: + RAFTER_API_KEY: ${{ secrets.RAFTER_API_KEY }} + + # sable-d2x2 rule C — same floor as test-comprehensive.yml; keep in step. + - name: Assert the suite actually ran + run: python3 ../.github/scripts/test_floor.py --junit pytest-report.xml --min-executed 1530 --label "release test-python (pytest)" test-package: runs-on: ubuntu-latest @@ -82,7 +153,12 @@ jobs: echo "OK: pre-commit hook installed end-to-end" publish-node: - needs: [test-node, test-package] + # test-python is a gate on the npm release too, not only the PyPI one. + # The two registries are published from one push and version parity is + # enforced, so a Python suite that fails after npm has already published + # leaves the two runtimes at different versions on the two indexes — + # the divergence is the failure mode, whichever half breaks. (sable-cazq) + needs: [test-node, test-python, test-package] runs-on: ubuntu-latest # Trusted Publishing requires id-token: write in scope for THIS job (the # top-level grant covers it, but documented here for the job-local audit @@ -143,6 +219,16 @@ jobs: run: npm publish --access public --provenance publish-python: + # needs: is the union of two fixes that landed together. + # sable-bm5k (#221): publish-node has needed the test jobs since it was + # written; this one never did, so a red suite blocked the npm release and + # shipped the PyPI one anyway. In a dual-implementation product that + # diverges the two runtimes at the registry — the one place users cannot + # see it. + # sable-cazq (#222): adds test-python, which is what makes the Python + # tests run on the release path at all. #221 alone only stops a red NODE + # suite shipping Python; it does not make Python tests run. + needs: [test-node, test-python, test-package] runs-on: ubuntu-latest defaults: run: diff --git a/.github/workflows/test-comprehensive.yml b/.github/workflows/test-comprehensive.yml index 225aca9e..c1b8828d 100644 --- a/.github/workflows/test-comprehensive.yml +++ b/.github/workflows/test-comprehensive.yml @@ -4,14 +4,96 @@ on: pull_request: branches: - prod + - main workflow_dispatch: permissions: contents: read jobs: + # ── Who gets the suite ──────────────────────────────────────────── + # The two unit-test jobs (test-node, test-python) run on EVERY PR — see + # sable-bm5k. The rest of the matrix runs for PRs into prod (the release + # gate) and for outside contributions; for our own PRs into main it is + # skipped, because re-running the 6-way cross-platform grid on work that + # was reviewed before it was pushed mostly burns runner minutes. + # + # Note this is `pull_request`, not `pull_request_target` — fork PRs run with + # a read-only token and no access to secrets. Do not "fix" that. + gate: + runs-on: ubuntu-latest + outputs: + run: ${{ steps.decide.outputs.run }} + run_core: ${{ steps.decide.outputs.run_core }} + steps: + # Checked out only so the skip notice below can read the list of gated + # jobs from this file instead of carrying a copy of it. The first + # hand-typed copy omitted e2e-node within the hour it was written. + - uses: actions/checkout@v4 + with: + sparse-checkout: .github/workflows/test-comprehensive.yml + sparse-checkout-cone-mode: false + + - id: decide + # Values go through env rather than direct ${{ }} interpolation into + # the script, so nothing from the PR can be shell-injected. + env: + EVENT: ${{ github.event_name }} + BASE: ${{ github.event.pull_request.base.ref }} + HEAD_OWNER: ${{ github.event.pull_request.head.repo.owner.login }} + AUTHOR: ${{ github.event.pull_request.user.login }} + run: | + # `run` — the full matrix, including the 6-way cross-platform grid. + # `run_core` — the two unit-test jobs. These now run on EVERY PR. + # + # sable-bm5k: the original gate skipped everything on internal PRs into + # main, on the premise that our own work is tested locally first. On + # #220 — which changed both the Node and the Python client — that meant + # neither test-node nor test-python ran. The premise is also weaker + # than it looks: this repo has test files that fail locally for + # environmental reasons, so "green on my machine" is not a signal you + # can act on. test-node (234s) and test-python (100s) run in parallel, + # so this costs ~4 minutes of wall clock. The expensive part — the + # cross-platform grid, 6 jobs and 3 of them macOS — stays gated. + echo "run_core=true" >> "$GITHUB_OUTPUT" + + if [ "$EVENT" != "pull_request" ] || [ "$BASE" != "main" ]; then + echo "run=true" >> "$GITHUB_OUTPUT" + elif [ "$HEAD_OWNER" = "Raftersecurity" ] || [ "$AUTHOR" = "Rome-1" ]; then + echo "run=false" >> "$GITHUB_OUTPUT" + # sable-d2x2 rule B — a skipped job renders as a grey check and + # satisfies a required status check, so the skip has to be said + # out loud, by name, where a reviewer looks: the checks annotation + # and the run summary. The list is READ FROM THIS FILE (every job + # whose `if:` is gated on needs.gate.outputs.run), never typed by + # hand, so it cannot drift from the jobs it describes. + SKIPPED_JOBS=$(awk ' + /^ [A-Za-z0-9_-]+:[[:space:]]*$/ { job=$1; sub(":", "", job) } + /^[[:space:]]*if:.*needs\.gate\.outputs\.run == .true./ { if (job != "" && !seen[job]++) print job } + ' .github/workflows/test-comprehensive.yml) + SKIPPED_CSV=$(printf '%s' "$SKIPPED_JOBS" | paste -sd ',' - | sed 's/,/, /g') + if [ -z "$SKIPPED_JOBS" ]; then + echo "::error::gate: found no jobs gated on needs.gate.outputs.run — the skip notice would be empty. Either the gate is now pointless or this awk no longer matches the file." + exit 1 + fi + echo "::warning::Internal PR into main: extended matrix NOT run — ${SKIPPED_CSV}. Unit tests (test-node, test-python) still run. To run everything, push to a branch and open the PR from a fork, or use workflow_dispatch." + { + echo "### Extended matrix skipped on this run" + echo "" + echo "Internal PR into main (author=\`$AUTHOR\`, head repo owner=\`$HEAD_OWNER\`). These jobs did **not** run and their grey checks mean *skipped*, not *passed*:" + echo "" + printf '%s\n' "$SKIPPED_JOBS" | sed 's/.*/- `&`/' + echo "" + echo "\`test-node\` and \`test-python\` ran. Trigger the full matrix with **workflow_dispatch** if this PR touches packaging, SARIF output, secret patterns, or platform-specific code." + } >> "$GITHUB_STEP_SUMMARY" + else + echo "run=true" >> "$GITHUB_OUTPUT" + fi + # ── Unit & integration tests (both languages) ───────────────────── test-node: + needs: gate + if: needs.gate.outputs.run_core == 'true' runs-on: ubuntu-latest defaults: run: @@ -38,6 +120,15 @@ jobs: run: | pip install -e ".[dev]" 2>/dev/null || pip install -e . + # The rf-6pqx differential compares this branch's classifier against + # main's, and it FAILS rather than skips when the baseline is missing — + # correctly, since a differential that silently skips is a vacuous gate. + # actions/checkout fetches only the PR ref, so `origin/main` is not in the + # clone and the gate cannot run at all. A depth-1 fetch is enough: the + # gate reads one blob, not the history. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + - name: Build run: pnpm run build @@ -50,11 +141,31 @@ jobs: run: node ./dist/index.js --version - name: Run all tests - run: pnpm test + # The JSON report feeds the floor check below; the default reporter + # keeps the log readable. + run: pnpm exec vitest run --reporter=default --reporter=json --outputFile.json=vitest-report.json env: RAFTER_API_KEY: ${{ secrets.RAFTER_API_KEY }} + # sable-d2x2 rule C — a green run must have RUN something. Fails if the + # executed count falls below the floor or any file's tests all skipped + # (sable-cazq: 40 parity tests describe.skip'd, exit 0). The floor is + # ~95% of the count on 2026-09-02 (2099 executed); lower it in the same + # PR that removes tests, so a shrink is a reviewed decision. + - name: Assert the suite actually ran + run: python3 ../.github/scripts/test_floor.py --vitest vitest-report.json --min-executed 1990 --label "test-node (vitest)" + + # The battery is the gate that catches a MISSING fix; the differential + # cannot, because a fix absent on both sides is not a permissive move. + # That makes the battery's CONTENTS load-bearing, so they get a floor of + # their own — in both directions, since a fix bought with an over-block + # is how #230 happened. + - name: Assert the classifier battery still gates both directions + run: python3 ../.github/scripts/classifier_battery_floor.py --min-cases 44 + test-python: + needs: gate + if: needs.gate.outputs.run_core == 'true' runs-on: ubuntu-latest defaults: run: @@ -71,13 +182,28 @@ jobs: pip install -e ".[dev]" 2>/dev/null || pip install -e . pip install pytest pytest-mock pytest-asyncio + # Same reason as test-node: the differential FAILS rather than skips + # when main's classifier is unobtainable, and actions/checkout fetches + # only the PR ref. One blob is all the gate needs. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + - name: Run all tests - run: python -m pytest tests/ -v + # xunit1 is the JUnit family that records `file` per test case, which + # the floor check below groups by. + run: python -m pytest tests/ -v -o junit_family=xunit1 --junitxml=pytest-report.xml env: RAFTER_API_KEY: ${{ secrets.RAFTER_API_KEY }} + # sable-d2x2 rule C — see test-node. Floor is ~95% of 1614 executed on + # 2026-09-02. + - name: Assert the suite actually ran + run: python3 ../.github/scripts/test_floor.py --junit pytest-report.xml --min-executed 1530 --label "test-python (pytest)" + # ── E2E CLI tests ───────────────────────────────────────────────── e2e-node: + needs: gate + if: needs.gate.outputs.run == 'true' runs-on: ubuntu-latest defaults: run: @@ -102,6 +228,8 @@ jobs: # ── Secret detection accuracy ────────────────────────────────────── secret-detection-accuracy: + needs: gate + if: needs.gate.outputs.run == 'true' runs-on: ubuntu-latest defaults: run: @@ -124,6 +252,8 @@ jobs: # ── SARIF output validation ──────────────────────────────────────── sarif-validation: + needs: gate + if: needs.gate.outputs.run == 'true' runs-on: ubuntu-latest defaults: run: @@ -160,6 +290,8 @@ jobs: # ── Remote API integration (only when key available) ─────────────── backend-api: + needs: gate + if: needs.gate.outputs.run == 'true' runs-on: ubuntu-latest env: RAFTER_API_KEY: ${{ secrets.RAFTER_API_KEY }} @@ -183,8 +315,28 @@ jobs: if: ${{ env.RAFTER_API_KEY != '' }} run: pnpm exec vitest run tests/backend-api.test.ts + # sable-bm5k — without this the job renders identically whether it tested + # the backend or tested nothing. RAFTER_API_KEY has never been set on this + # repo, so "backend-api ✓" has always meant "checked out and built". + # A skipped step must not look like a passing one. + - name: Say so when the backend tests did not run + if: ${{ env.RAFTER_API_KEY == '' }} + run: | + echo "::warning::backend-api tested NOTHING — RAFTER_API_KEY is not set, so tests/backend-api.test.ts was skipped." + { + echo "### :warning: backend-api ran no tests" + echo "" + echo "\`RAFTER_API_KEY\` is unset, so \`tests/backend-api.test.ts\` was skipped." + echo "This job checked out and built the package and nothing else." + echo "" + echo "The remote scan path is covered without a key by the mock-backed jobs" + echo "in \`test-github-action.yml\`. See sable-bm5k." + } >> "$GITHUB_STEP_SUMMARY" + # ── Package build verification ───────────────────────────────────── package-integrity: + needs: gate + if: needs.gate.outputs.run == 'true' runs-on: ubuntu-latest defaults: run: @@ -231,11 +383,21 @@ jobs: # ── Cross-platform smoke test ────────────────────────────────────── cross-platform: + needs: gate + if: needs.gate.outputs.run == 'true' strategy: fail-fast: false matrix: os: [ubuntu-latest, macos-latest] - node: ["18", "20", "22"] + # Node 18 is deliberately absent: vitest 4.1.0 declares + # engines.node ^20 || ^22 || >=24, so an 18 leg runs the suite on a + # runner that does not support it. It does not refuse outright — it + # runs, reports, and the report means nothing about the shipped code. + # engines in package.json stays >=18 because the BUILT ARTIFACT does + # work on 18 (measured 200/200 on dist). If we want real Node 18 + # signal, smoke-test the built artifact under 18 — that is what users + # run — rather than the test runner. + node: ["20", "22"] runs-on: ${{ matrix.os }} defaults: run: @@ -250,6 +412,13 @@ jobs: - name: Enable pnpm run: corepack enable && corepack prepare pnpm@10 --activate + # The rf-6pqx differential compares this branch's classifier against main's + # and FAILS rather than skips when the baseline is missing — correctly, but + # that means EVERY job running the suite has to supply it. actions/checkout + # fetches only the PR ref. A depth-1 fetch is enough: one blob, not history. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + - name: Install and build run: | pnpm install --frozen-lockfile diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index b7836ee7..7a7665d8 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -5,8 +5,9 @@ name: Test github-action/ Composite Action # are pure-bash unit tests of the threshold-eval and PR-comment logic # plus a drift detector on action.yml's load-bearing defaults. # -# An end-to-end test against the real Rafter API is a future addition -# (would require an injectable RAFTER_API_KEY secret and a fixture repo). +# The poll-path jobs DO drive the action end to end, against a localhost mock +# backend (github-action/tests/mock-rafter-api.py) rather than the real API, so +# they need no API key and spend no credits. on: push: branches: @@ -48,6 +49,361 @@ jobs: - name: Run action.yml defaults / drift check run: bash github-action/tests/test-action-yml-defaults.sh + # sable-l10k — a paying customer's run died on a single transient 500 during + # polling ("Failed to fetch report from storage: Object not found"). The + # report is not durable the instant a scan flips to completed, so that 500 is + # survivable and must be retried. These two jobs pin both halves of the + # contract: ride out the transient failure, still fail on a missing report. + test-poll-transient-500: + name: "Poll: rides out a transient 500" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (500 on poll #2, then healthy) + env: + PORT: '8787' + FAIL_ON: '2' + 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:8787/api/static/scan >/dev/null && break + sleep 1 + done + + - 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:8787' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the scan survived the 500 + run: | + cat mock.log + echo "status output: '${{ steps.scan.outputs.status }}'" + if [ "${{ steps.scan.outputs.status }}" != "completed" ]; then + echo "FAIL: a single transient 500 during polling killed the run." + exit 1 + fi + echo "PASS: the action retried the transient 500 and completed." + + test-poll-report-never-readable: + name: "Poll: fails clearly when the report is really missing" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (every poll 500s) + env: + PORT: '8788' + FAIL_ON: '2' + FAIL_FOREVER: '1' + 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:8788/api/static/scan >/dev/null && break + sleep 1 + done + + - 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:8788' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert it failed, with the right status + run: | + cat mock.log + FAIL=0 + if [ "${{ steps.scan.outputs.status }}" != "unreadable" ]; then + echo "FAIL: expected status=unreadable, got '${{ steps.scan.outputs.status }}'" + FAIL=1 + fi + if [ "${{ steps.scan.outcome }}" != "failure" ]; then + echo "FAIL: an unreadable report should fail the build." + FAIL=1 + fi + # A composite action's log is not capturable from the calling step, + # so the CONTENT of the give-up message is asserted by the drift + # detector (github-action/tests/test-action-yml-defaults.sh) instead. + exit $FAIL + + # The 404-as-transient branch is the subtlest thing in the poll loop: it is + # correct only because the trigger step has already handed us a scan_id. + # Nothing else in CI exercises it, so a "simplification" that drops `-eq 404` + # from the transient condition would otherwise land green. + test-poll-transient-404: + name: "Poll: rides out a transient 404" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (404 on poll #2, then healthy) + env: + PORT: '8789' + FAIL_ON: '2' + FAIL_STATUS: '404' + 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:8789/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8789/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:8789' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the scan survived the 404 + run: | + cat mock.log + if [ "${{ steps.scan.outputs.status }}" != "completed" ]; then + echo "FAIL: a transient 404 mid-poll killed the run." + exit 1 + fi + echo "PASS: the action treated a mid-poll 404 as read-after-write lag." + + # The results fetch runs the instant the scan reports completed — the + # likeliest moment for the report object to be unreadable. Its retry loop had + # no coverage at all, and it is where a failed read used to be reported to + # consumers as status=completed. + test-results-fetch-transient-500: + name: "Results fetch: rides out a transient 500" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Start mock backend (poll succeeds, first results fetch 500s) + env: + PORT: '8790' + FAIL_ON: '2' + FAIL_COUNT: '1' + COMPLETE_AFTER: '1' + 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:8790/api/static/scan >/dev/null && break + sleep 1 + done + curl -sf -X POST -d '{}' http://127.0.0.1:8790/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:8790' + timeout-minutes: '2' + upload-sarif: 'false' + comment-on-pr: 'false' + + - name: Assert the results fetch retried rather than failing the build + run: | + cat mock.log + if [ "${{ steps.scan.outputs.status }}" != "completed" ]; then + echo "FAIL: a transient 500 on the results fetch killed the run (status='${{ steps.scan.outputs.status }}')." + exit 1 + 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 + + # 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/workflows/validate-release.yml b/.github/workflows/validate-release.yml index a8b459c2..32c8d35a 100644 --- a/.github/workflows/validate-release.yml +++ b/.github/workflows/validate-release.yml @@ -76,6 +76,16 @@ jobs: steps: - uses: actions/checkout@v4 + # The rf-6pqx differential FAILS LOUDLY when main's classifier is + # unobtainable — correctly, since a differential that silently skips is a + # vacuous gate. actions/checkout fetches only the PR ref, so origin/main + # is absent and the gate cannot run at all. This job only fires on PRs + # into prod, which is why nothing before #237 caught it: the same fetch + # was added to test-comprehensive.yml and not here. One blob is all the + # gate needs. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + - uses: actions/setup-node@v4 with: node-version: "20" @@ -84,6 +94,13 @@ jobs: with: python-version: "3.11" + # The rf-6pqx differential compares this branch's classifier against main's + # and FAILS rather than skips when the baseline is missing — correctly, but + # that means EVERY job running the suite has to supply it. actions/checkout + # fetches only the PR ref. A depth-1 fetch is enough: one blob, not history. + - name: Fetch main for the differential gate + run: git fetch --depth=1 origin main:refs/remotes/origin/main + - name: Build Node package run: | cd node @@ -93,11 +110,18 @@ jobs: pnpm run build pnpm test - - name: Build Python package + # sable-cazq — this job ran `pnpm test` for Node and nothing but a + # wheel build for Python, so the pre-release gate asserted that the + # Python package *compiles*, never that it works. ~80s of pytest is + # the difference between those two claims. + - name: Build and test Python package run: | cd python python -m pip install --upgrade build python -m build + pip install -e . + pip install pytest pytest-mock pytest-asyncio + python -m pytest tests/ -q - name: Verify all artifacts run: | diff --git a/.gitignore b/.gitignore index c0769cf7..edb07dea 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,12 @@ venv/ uv.lock # Node +# Both forms on purpose: a trailing slash matches DIRECTORIES ONLY, so a stray +# `node_modules` SYMLINK slips past it and `git add -A` commits it. That +# happened (removed in f1132ad); on checkout it leaves a broken self- +# referential link where the install belongs, and pnpm/tsc/vitest all fail +# there — tsc exits 216 with no output, which is an unhelpful way to find out. +node_modules node_modules/ dist/ *.tgz diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f5b87fa..ac5501cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`rafter agent exec --force` no longer skips approval, and approval needs a person at a terminal** (rf-ss67, reported in the secbolt audit se-ezvc). `--force ""` ran any HIGH-tier command unprompted: the PreToolUse hook classified the quoted argument as prose, and `exec` then skipped its own prompt. `--force` is now a hidden no-op kept only so old invocations parse; a command that needs approval is prompted only when stdin is an interactive TTY and is otherwise denied, so a piped `yes` is not an approval either. `--dry-run`, which three shipped docs already advertised, now exists: it prints the verdict and runs nothing (exit 0 allowed, 1 blocked, 2 needs approval). The documented `-- ` form is accepted, with the words re-quoted so the classifier evaluates exactly what the shell would run. Both runtimes. + +- **A transient 500 during scan polling no longer kills the run** (sable-l10k). An AppSumo customer's GitHub Actions build died on `Rafter scan poll failed: HTTP 500 — Failed to fetch report from storage: Object not found`. A report is not durable the instant a scan flips to `completed`, so a 5xx on that read is survivable — but every poll path treated any non-2xx as fatal, while the transport-error branch three lines above already retried. All three surfaces (the composite action, `rafter run`, `rafter get --interactive`) now retry transient failures with exponential backoff (2s/4s/8s/16s) before giving up, and the give-up message names the scan id, the `rafter get ` retry, and the dashboard instead of leaking storage-layer wording. Full contract in `shared-docs/CLI_SPEC.md`. Both runtimes; end-to-end CI coverage against a mock backend, so no API key or credits are needed to exercise it. +- **Python: a failed poll could be written out as if it were scan results** (sable-l10k). The mid-poll loop called `.json()` on the response without checking the status code, so a 500 carrying a JSON error body parsed cleanly, yielded no `status`, fell out of the loop, and was emitted as the scan payload with exit code `0`. A non-JSON error body raised an unhandled `JSONDecodeError`. Both now fail loudly. **Behavior change:** genuine non-transient mid-poll failures that previously exited `0` with an error payload on stdout now exit `1` — check any pipeline that consumed that output. +- **GitHub Action: a failed results fetch reported the scan as `completed`** (sable-l10k). The declared `status` output read only from the results step, which does not run when the fetch fails. Consumers gating on `status == 'completed'` saw a clean scan, and the artifact upload published the error body as `rafter-results.json`. Both give-up paths in the results fetch now record `status=unreadable`, and the artifact upload is gated on a successful results fetch. +- **GitHub Action: server-controlled error text is sanitized before it reaches workflow commands** (sable-l10k). A response body containing a newline could forge `::error::`, `::add-mask::`, or `::stop-commands::` annotations. Error text from the API is now stripped of newlines and length-capped at every site that echoes it, and the server-supplied `scan_id` is rejected unless it matches `^[A-Za-z0-9_-]+$` before it reaches `$GITHUB_OUTPUT` (where a newline would forge step outputs, including `status=completed`). +- **GitHub Action: an unreachable API is reported as unreachable** (sable-l10k). Transport errors retried on a flat 10s interval without counting toward the failure budget, so a bad `rafter-url` or a down backend burned the whole `timeout-minutes` window and then reported `Scan did not complete within N minutes` — a timeout message for a DNS failure. They now share the same retry budget and exit with `status=unreachable`. + +### Changed + +- **`timeout-minutes` on the GitHub Action is now a wall-clock deadline**, not a poll count. Previously the action ran `timeout-minutes * 6` polls, each costing 10s *plus* API latency, so a slow API pushed real elapsed time past the documented budget. It is now enforced as a real deadline. **This can fail workflows that were relying on the overrun** — if a scan sits near the boundary, raise `timeout-minutes`. +- `rafter get ` (without `--interactive`) now retries transient failures too. It is the command the poll loop's give-up message recommends, so a remedy defeated by the same transient failure it is recommended for was not a remedy. +- HTTP requests on the poll and results paths now carry connect/read timeouts (`--connect-timeout 10 --max-time 60` for curl, 30s for axios), so a hung server cannot stall inside a request that the retry loop only checks between attempts. + ## [0.10.0] - 2026-07-29 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index d52bd9c4..84b92d69 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,7 +60,7 @@ cd python && poetry install && pytest **Secret scanning**: Dual-engine — tries Betterleaks binary first (higher accuracy), falls back to built-in regex patterns (21+ patterns, zero dependencies). Deterministic for a given version. Betterleaks is the gitleaks successor maintained by the original gitleaks authors. Existing installs with a leftover `~/.rafter/bin/gitleaks` are detected by `agent verify`/`status` so users get an upgrade hint, but the legacy CLI flags (`--with-gitleaks`, `--engine gitleaks`, `update-gitleaks`) have been removed. -**MCP server**: `rafter mcp serve` exposes 4 tools (`scan_secrets`, `evaluate_command`, `read_audit_log`, `get_config`) and 2 resources (`rafter://config`, `rafter://policy`) over stdio. +**MCP server**: `rafter mcp serve` exposes 4 tools (`scan_secrets`, `evaluate_command`, `read_audit_log`, `get_config`) and 3 resources (`rafter://config`, `rafter://policy`, `rafter://docs`) over stdio. ## Development diff --git a/demo/README.md b/demo/README.md index 6e564b24..30d0df57 100644 --- a/demo/README.md +++ b/demo/README.md @@ -18,7 +18,7 @@ The `/rafter-showcase` skill walks through all 9 core features with live command 4. Audit logging (JSONL trail) 5. Pre-commit hooks 6. CI/CD integration (GitHub Actions) -7. MCP server (4 tools, 2 resources) +7. MCP server (4 tools, 3 resources) 8. Skill auditing 9. Remote SAST/SCA (requires API key) diff --git a/github-action/README.md b/github-action/README.md index e9d94c15..eec9bf02 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 32a9b812..8fe5ee3c 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -27,7 +27,7 @@ inputs: required: false default: 'true' timeout-minutes: - description: 'Maximum time to wait for scan completion (minutes)' + description: 'Maximum wall-clock time to wait for scan completion (minutes). Enforced as a real deadline: before v0.11 this was a poll COUNT, so a slow API could overrun it.' required: false default: '10' rafter-url: @@ -55,8 +55,8 @@ outputs: description: 'Number of low/note findings' value: ${{ steps.results.outputs.low_count }} status: - description: 'Scan status (completed, failed, timeout)' - value: ${{ steps.results.outputs.status }} + description: 'Scan status: completed, failed, timeout, unreadable (the scan may have finished but its report could not be read), or unreachable (the Rafter API could not be contacted)' + value: ${{ steps.results.outputs.status || steps.poll.outputs.status }} runs: using: 'composite' @@ -73,7 +73,8 @@ runs: # We capture body+status separately so future failures self-explain # (instead of just "curl exit 22"). API key never echoed. BODY_FILE="$(mktemp)" - HTTP_CODE=$(curl -sS -o "$BODY_FILE" -w "%{http_code}" -X POST \ + HTTP_CODE=$(curl -sS --connect-timeout 10 --max-time 60 \ + -o "$BODY_FILE" -w "%{http_code}" -X POST \ -H "Content-Type: application/json" \ -H "x-api-key: ${RAFTER_API_KEY}" \ -d "{ @@ -92,19 +93,28 @@ runs: rm -f "$BODY_FILE" if [ "$HTTP_CODE" -lt 200 ] || [ "$HTTP_CODE" -ge 300 ]; then - ERROR=$(echo "$RESPONSE" | jq -r '.error // empty' 2>/dev/null || true) + ERROR=$(echo "$RESPONSE" | jq -r '.error // empty' 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) echo "::error::Rafter scan trigger failed: HTTP ${HTTP_CODE}" if [ -n "$ERROR" ]; then echo "::error::Server: ${ERROR}" else - echo "Server response: ${RESPONSE}" + echo "Server response: $(printf '%s' "$RESPONSE" | tr -d '\r\n' | cut -c1-500)" fi exit 1 fi SCAN_ID=$(echo "$RESPONSE" | jq -r '.scan_id // empty') + # $GITHUB_OUTPUT is a key=value file: a newline in a server-controlled + # scan_id forges arbitrary step outputs, including status=completed. + # It also reaches ::error:: annotations and a request URL. + case "$SCAN_ID" in + *[!A-Za-z0-9_-]*) + echo "::error::Rafter returned a malformed scan id; refusing to continue" + exit 1 + ;; + esac if [ -z "$SCAN_ID" ]; then - ERROR=$(echo "$RESPONSE" | jq -r '.error // "Unknown error triggering scan"') + ERROR=$(echo "$RESPONSE" | jq -r '.error // "Unknown error triggering scan"' | tr -d '\r\n' | cut -c1-200) echo "::error::Failed to trigger scan (HTTP ${HTTP_CODE}): ${ERROR}" exit 1 fi @@ -121,38 +131,97 @@ runs: SCAN_ID: ${{ steps.scan.outputs.scan_id }} TIMEOUT_MINUTES: ${{ inputs.timeout-minutes }} run: | - MAX_POLLS=$(( TIMEOUT_MINUTES * 6 )) # Poll every 10s + # sable-l10k — a report is not durable the instant the scan flips to + # completed, so a poll can legitimately hit a 5xx (in practice + # "Failed to fetch report from storage: Object not found") on a scan + # that is perfectly healthy and readable seconds later. Retry those with + # backoff. Only give up once the failures stop looking transient. + # + # 404 counts as transient HERE and only here: the trigger step already + # handed us a scan_id, so a missing scan mid-poll is read-after-write + # lag rather than a wrong id. + case "$TIMEOUT_MINUTES" in + ''|*[!0-9]*) + echo "::error::timeout-minutes must be a whole number of minutes, got '${TIMEOUT_MINUTES}'" + exit 1 + ;; + esac + + MAX_TRANSIENT_FAILURES=5 + TRANSIENT_FAILURES=0 + LAST_ERROR="" + + # Wall-clock deadline so retry backoff cannot quietly stretch the + # documented timeout-minutes budget. + DEADLINE=$(( $(date +%s) + TIMEOUT_MINUTES * 60 )) POLL_COUNT=0 STATUS="pending" - while [ $POLL_COUNT -lt $MAX_POLLS ]; do + while [ "$(date +%s)" -lt "$DEADLINE" ]; do BODY_FILE="$(mktemp)" - HTTP_CODE=$(curl -sS -o "$BODY_FILE" -w "%{http_code}" \ + HTTP_CODE=$(curl -sS --connect-timeout 10 --max-time 60 \ + -o "$BODY_FILE" -w "%{http_code}" \ -H "x-api-key: ${RAFTER_API_KEY}" \ "${RAFTER_URL}/api/static/scan?scan_id=${SCAN_ID}") || { - echo "::warning::curl transport error during poll (will retry)" - cat "$BODY_FILE" || true + # A transport error is exactly as transient as a 5xx, and counts + # the same. Previously it retried on a flat 10s forever, which + # meant an unreachable backend reported "scan did not complete + # within N minutes" — a timeout message for a DNS failure. rm -f "$BODY_FILE" - sleep 10 + LAST_ERROR="curl transport error contacting ${RAFTER_URL}" + TRANSIENT_FAILURES=$((TRANSIENT_FAILURES+1)) + if [ "$TRANSIENT_FAILURES" -ge "$MAX_TRANSIENT_FAILURES" ]; then + echo "::error::Rafter could not reach the API for scan ${SCAN_ID} after ${MAX_TRANSIENT_FAILURES} attempts." + echo "::error::Check that ${RAFTER_URL} is reachable from this runner." + echo "::error::Last error: ${LAST_ERROR}" + echo "status=unreachable" >> "$GITHUB_OUTPUT" + exit 1 + fi + BACKOFF=$(( 2 ** TRANSIENT_FAILURES )) + echo "::warning::${LAST_ERROR}; retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" + sleep "$BACKOFF" POLL_COUNT=$((POLL_COUNT+1)) continue } RESPONSE=$(cat "$BODY_FILE") rm -f "$BODY_FILE" + if [ "$HTTP_CODE" -ge 500 ] || [ "$HTTP_CODE" -eq 408 ] || [ "$HTTP_CODE" -eq 404 ]; then + ERROR=$(echo "$RESPONSE" | jq -r '.error // empty' 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) + LAST_ERROR="HTTP ${HTTP_CODE}${ERROR:+ — $ERROR}" + TRANSIENT_FAILURES=$((TRANSIENT_FAILURES+1)) + + if [ "$TRANSIENT_FAILURES" -ge "$MAX_TRANSIENT_FAILURES" ]; then + echo "::error::Rafter could not read the report for scan ${SCAN_ID} after ${MAX_TRANSIENT_FAILURES} attempts." + echo "::error::The scan itself may have finished — check it in your dashboard at ${RAFTER_URL}/dashboard" + echo "::error::Last response from the server: ${LAST_ERROR}" + echo "status=unreadable" >> "$GITHUB_OUTPUT" + exit 1 + fi + + BACKOFF=$(( 2 ** TRANSIENT_FAILURES )) # 2s, 4s, 8s, 16s + echo "::warning::Report not readable yet (${LAST_ERROR}); retrying in ${BACKOFF}s (${TRANSIENT_FAILURES}/${MAX_TRANSIENT_FAILURES})" + sleep "$BACKOFF" + POLL_COUNT=$((POLL_COUNT+1)) + continue + fi + if [ "$HTTP_CODE" -lt 200 ] || [ "$HTTP_CODE" -ge 300 ]; then - ERROR=$(echo "$RESPONSE" | jq -r '.error // empty' 2>/dev/null || true) + # 4xx other than 404/408: a bad key or a malformed request. Retrying + # will not help and would only delay a clear answer. + ERROR=$(echo "$RESPONSE" | jq -r '.error // empty' 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) echo "::error::Rafter scan poll failed: HTTP ${HTTP_CODE}${ERROR:+ — $ERROR}" exit 1 fi + TRANSIENT_FAILURES=0 STATUS=$(echo "$RESPONSE" | jq -r '.status // "unknown"') if [ "$STATUS" = "completed" ] || [ "$STATUS" = "failed" ]; then break fi - echo "Scan status: ${STATUS} (poll $((POLL_COUNT+1))/${MAX_POLLS})" + echo "Scan status: ${STATUS} (poll $((POLL_COUNT+1)), $(( (DEADLINE - $(date +%s)) / 60 ))m of ${TIMEOUT_MINUTES}m budget left)" sleep 10 POLL_COUNT=$((POLL_COUNT+1)) done @@ -180,36 +249,82 @@ runs: run: | # fetch_results : HTTP-status aware GET that surfaces # server error body on non-2xx (avoids silent curl exit 22 failures). + # + # sable-l10k — same read-after-write race as the poll loop, and worse + # here: this runs the instant the scan reports completed, which is the + # likeliest moment for the report object to not be readable yet. Retry + # transient failures with backoff rather than failing the build. fetch_results() { local out="$1" local url="$2" - local code - code=$(curl -sS -o "$out" -w "%{http_code}" \ - -H "x-api-key: ${RAFTER_API_KEY}" "$url") || { - echo "::error::curl transport error fetching ${url}" - cat "$out" || true + local attempt=1 + local max_attempts=5 + local code body err last="" + + while :; do + if code=$(curl -sS --connect-timeout 10 --max-time 60 \ + -o "$out" -w "%{http_code}" \ + -H "x-api-key: ${RAFTER_API_KEY}" "$url"); then + if [ "$code" -ge 200 ] && [ "$code" -lt 300 ]; then + return 0 + fi + body=$(cat "$out" || true) + err=$(echo "$body" | jq -r '.error // empty' 2>/dev/null | tr -d '\r\n' | cut -c1-200 || true) + last="HTTP ${code}${err:+ — $err}" + if [ "$code" -lt 500 ] && [ "$code" -ne 408 ] && [ "$code" -ne 404 ]; then + # Not transient — a bad key or malformed request. Say so now. + echo "::error::Rafter results fetch failed: ${last}" + echo "status=unreadable" >> "$GITHUB_OUTPUT" + return 1 + fi + else + last="curl transport error fetching ${url}" + fi + + if [ "$attempt" -ge "$max_attempts" ]; then + echo "::error::Rafter could not read the report for scan ${SCAN_ID} after ${max_attempts} attempts." + echo "::error::The scan itself finished — check it in your dashboard at ${RAFTER_URL}/dashboard" + echo "::error::Last response from the server: ${last}" + # Without this the declared `status` output falls back to the poll + # step, which already said `completed` — a failed report read would + # be reported to consumers as a clean scan. + echo "status=unreadable" >> "$GITHUB_OUTPUT" return 1 - } - if [ "$code" -lt 200 ] || [ "$code" -ge 300 ]; then - local body err - body=$(cat "$out" || true) - err=$(echo "$body" | jq -r '.error // empty' 2>/dev/null || true) - echo "::error::Rafter results fetch failed: HTTP ${code}${err:+ — $err}" - return 1 - fi + fi + + local backoff=$(( 2 ** attempt )) + echo "::warning::Report not readable yet (${last}); retrying in ${backoff}s (${attempt}/${max_attempts})" + sleep "$backoff" + attempt=$((attempt+1)) + done } fetch_results "${{ runner.temp }}/rafter-results.json" "${RAFTER_URL}/api/static/scan?scan_id=${SCAN_ID}" 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" @@ -241,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 @@ -279,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" @@ -297,7 +412,7 @@ runs: gh pr comment "${{ github.event.pull_request.number }}" --body-file "$COMMENT_FILE" - name: Upload artifacts - if: always() && steps.poll.outputs.status == 'completed' + if: steps.results.outputs.status == 'completed' uses: actions/upload-artifact@v4 with: name: rafter-security-results @@ -317,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 00000000..ab38d2fb --- /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/mock-rafter-api.py b/github-action/tests/mock-rafter-api.py new file mode 100644 index 00000000..23af6cc5 --- /dev/null +++ b/github-action/tests/mock-rafter-api.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Minimal stand-in for the Rafter backend, used to reproduce sable-l10k. + +Serves the two endpoints the GitHub Action talks to and injects exactly one +transient 500 into the poll sequence: + + POST /api/static/scan -> 200 {"scan_id": ...} + GET /api/static/scan?scan_id=.. -> poll 1: 200 {"status": "processing"} + poll 2: 500 {"error": "Failed to fetch + report from storage: Object not found"} + poll 3: 200 {"status": "completed", ...} + +A backend that is eventually consistent about report objects looks exactly like +this from the client's side. The question the repro answers is whether the +action survives it. + +Env: + PORT listen port (default 8787) + FAIL_ON 1-based GET index that starts failing (default 2) + FAIL_STATUS status code to fail with (default 500; 404 exercises the + read-after-write-lag branch) + FAIL_FOREVER if "1", every GET from FAIL_ON onward fails (persistent case) + FAIL_COUNT how many consecutive GETs fail starting at FAIL_ON (default 1; + ignored when FAIL_FOREVER is set) + COMPLETE_AFTER GET index from which status is "completed" (default FAIL_ON, + 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 +from http.server import BaseHTTPRequestHandler, HTTPServer +from urllib.parse import urlparse, parse_qs + +PORT = int(os.environ.get("PORT", "8787")) +FAIL_ON = int(os.environ.get("FAIL_ON", "2")) +FAIL_STATUS = int(os.environ.get("FAIL_STATUS", "500")) +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): + 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", 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"}) + length = int(self.headers.get("Content-Length") or 0) + self.rfile.read(length) + self._send(200, {"scan_id": SCAN_ID}) + + def do_GET(self): + parsed = urlparse(self.path) + if parsed.path != "/api/static/scan": + return self._send(404, {"error": "not found"}) + + qs = parse_qs(parsed.query) + fmt = (qs.get("format") or ["json"])[0] + + state["polls"] += 1 + n = state["polls"] + + failing = (FAIL_FOREVER and n >= FAIL_ON) or ( + FAIL_ON <= n < FAIL_ON + FAIL_COUNT + ) + if failing: + # The verbatim customer-facing body. + return self._send( + FAIL_STATUS, + {"error": "Failed to fetch report from storage: Object not found"}, + ) + + 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" + elif fmt == "sarif": + completed = {"version": "2.1.0", "runs": []} + return self._send(200, completed) + + def log_message(self, fmt, *args): + # Keep the runner log readable: one line per request, to stderr. + super().log_message(fmt, *args) + + +if __name__ == "__main__": + print(f"mock rafter api on :{PORT} (500 on poll #{FAIL_ON}, forever={FAIL_FOREVER})", flush=True) + HTTPServer(("127.0.0.1", PORT), Handler).serve_forever() diff --git a/github-action/tests/test-action-yml-defaults.sh b/github-action/tests/test-action-yml-defaults.sh index 084748e6..d68b4462 100755 --- a/github-action/tests/test-action-yml-defaults.sh +++ b/github-action/tests/test-action-yml-defaults.sh @@ -45,25 +45,183 @@ 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" +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 ───────────────────────────────── +# These properties are subtle and cheap to "simplify" away. Each one, if +# dropped, reproduces a bug a paying customer already hit. + +# 5. 404 must be in the poll loop's TRANSIENT condition. It is safe only +# because the trigger step already handed us a scan_id, so a missing scan +# mid-poll is read-after-write lag rather than a wrong id. +if grep -qE '\$HTTP_CODE" -ge 500 \] \|\| \[ "\$HTTP_CODE" -eq 408 \] \|\| \[ "\$HTTP_CODE" -eq 404' "$ACTION_YML"; then + echo "PASS: poll loop treats 5xx/408/404 as transient" +else + echo "FAIL: poll loop's transient condition changed — 404/408/5xx must all retry" + failures=$((failures+1)) +fi + +# 6. A transport error must count toward the SAME failure budget as a 5xx. +# When it did not, an unreachable backend reported "scan did not complete +# within N minutes" — a timeout message for a DNS failure. +if awk '/curl transport error contacting/,/^ \}/' "$ACTION_YML" \ + | grep -q 'TRANSIENT_FAILURES=\$((TRANSIENT_FAILURES+1))'; then + echo "PASS: transport errors count toward the transient-failure budget" +else + echo "FAIL: poll loop's transport-error branch no longer counts toward the budget" + failures=$((failures+1)) +fi + +# 7. The give-up message must be actionable: name the scan, and offer a next +# step. Raw storage wording ("Object not found") alone is not a message a +# customer can act on. +if grep -q 'could not read the report for scan \${SCAN_ID}' "$ACTION_YML" \ + && grep -q 'check it in your dashboard at' "$ACTION_YML"; then + echo "PASS: give-up message names the scan and offers a next step" +else + echo "FAIL: give-up message no longer names the scan id or a next step" + failures=$((failures+1)) +fi + +# 8. Both give-up paths in the results fetch must record status=unreadable. +# Without it the declared `status` output falls back to the poll step's +# `completed`, and a failed report read is reported as a clean scan. +unreadable_writes=$(grep -c 'status=unreadable' "$ACTION_YML" || true) +if [ "$unreadable_writes" -ge 3 ]; then + echo "PASS: poll and both results-fetch give-up paths record status=unreadable" +else + echo "FAIL: expected >=3 status=unreadable writes, found ${unreadable_writes}" + failures=$((failures+1)) +fi + +# 9. The artifact upload must be gated on the RESULTS step, not the poll step. +# Gated on the poll step it published the error body as rafter-results.json. +if grep -qE "if: steps\.results\.outputs\.status == 'completed'" "$ACTION_YML"; then + echo "PASS: artifact upload gated on a successful results fetch" +else + echo "FAIL: artifact upload is not gated on steps.results.outputs.status" failures=$((failures+1)) +fi + +# 10. Backoff must be exponential. A flat or zeroed backoff gives an +# eventually-consistent object store no time to converge. +if grep -q 'BACKOFF=\$(( 2 \*\* TRANSIENT_FAILURES ))' "$ACTION_YML" \ + && grep -q 'backoff=\$(( 2 \*\* attempt ))' "$ACTION_YML"; then + echo "PASS: both retry loops back off exponentially" +else + echo "FAIL: a retry loop's backoff is no longer exponential" + failures=$((failures+1)) +fi + + +# 11. Server-controlled error text must be newline-stripped and length-capped +# before it reaches a workflow command. A newline forges ::add-mask:: / +# ::stop-commands:: / fabricated ::error:: annotations. +sanitized=$(grep -cF 'cut -c1-' "$ACTION_YML" || true) +stripped=$(grep -cF "tr -d " "$ACTION_YML" || true) +if [ "$sanitized" -ge 5 ] && [ "$stripped" -ge 5 ]; then + echo "PASS: server-controlled text newline-stripped and capped at ${sanitized} sites" +else + echo "FAIL: expected >=5 sanitized sites, found cut=${sanitized} tr=${stripped}" + failures=$((failures+1)) +fi + +# 12. TIMEOUT_MINUTES is evaluated inside bash arithmetic, where a value like +# 'x[$(cmd)]' executes. It must be validated first. +if grep -q 'case "\$TIMEOUT_MINUTES" in' "$ACTION_YML"; then + echo "PASS: timeout-minutes validated before arithmetic evaluation" +else + echo "FAIL: timeout-minutes is no longer validated before arithmetic use" + failures=$((failures+1)) +fi + +# 13. The server-controlled scan id must be validated before it reaches +# \$GITHUB_OUTPUT, where a newline forges step outputs. +if grep -q 'case "\$SCAN_ID" in' "$ACTION_YML"; then + echo "PASS: scan id validated before it reaches \$GITHUB_OUTPUT" else - echo "PASS: 'none' branch of threshold-eval does not set FAIL=1" + echo "FAIL: scan id is no longer validated" + 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 "" diff --git a/github-action/tests/test-pr-comment-tip.sh b/github-action/tests/test-pr-comment-tip.sh index 01e8ba68..1ac97a14 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 99dbfc30..eddb5c68 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/package.json b/node/package.json index 6abfb82e..6da04ccc 100644 --- a/node/package.json +++ b/node/package.json @@ -1,6 +1,6 @@ { "name": "@rafter-security/cli", - "version": "0.10.0", + "version": "0.10.1", "type": "module", "repository": { "type": "git", diff --git a/node/resources/rafter-security-skill.md b/node/resources/rafter-security-skill.md index 758c1d5f..84445a0a 100644 --- a/node/resources/rafter-security-skill.md +++ b/node/resources/rafter-security-skill.md @@ -1,7 +1,7 @@ --- name: rafter-security description: Security toolkit for AI workflows. Use when scanning code or repos for vulnerabilities, auditing third-party skills/MCPs/agent configs before installing, evaluating shell commands before running them, or generating secure design questions for new features. Provides `rafter run` (remote SAST + SCA, needs RAFTER_API_KEY), `rafter secrets` (offline secrets-only), `rafter agent exec --dry-run` (command-risk classification), and `rafter skill review`. -version: 0.10.0 +version: 0.10.1 homepage: https://rafter.so metadata: openclaw: diff --git a/node/resources/skills/rafter/docs/cli-reference.md b/node/resources/skills/rafter/docs/cli-reference.md index 23d33a68..78d72e35 100644 --- a/node/resources/skills/rafter/docs/cli-reference.md +++ b/node/resources/skills/rafter/docs/cli-reference.md @@ -64,7 +64,7 @@ When: before firing multiple remote scans, or when the user asks about limits. Classify and optionally run a shell command through Rafter's risk tiers (critical / high / medium / low). -When: any time a destructive-looking command is about to be executed by an agent. Use `--dry-run` to classify without running. +When: any time a destructive-looking command is about to be executed by an agent. Use `--dry-run` to classify without running: exit 0 = allowed, 1 = blocked, 2 = needs a person's approval. Without `--dry-run`, a command that needs approval is only ever approved by a person at an interactive terminal; from an agent's shell it is denied. Example: `rafter agent exec --dry-run -- rm -rf $WORK_DIR` diff --git a/node/resources/skills/rafter/docs/guardrails.md b/node/resources/skills/rafter/docs/guardrails.md index 9a604e67..d1e238c4 100644 --- a/node/resources/skills/rafter/docs/guardrails.md +++ b/node/resources/skills/rafter/docs/guardrails.md @@ -10,7 +10,7 @@ Rafter exposes two hook handlers over stdio: - `rafter hook posttool` — read a JSON event after a tool ran; log to audit trail, optionally rescan written files for secrets. For platforms without hooks, the same classifier is reachable as: -- `rafter agent exec --dry-run -- ` (returns risk, exits 0/1) +- `rafter agent exec --dry-run -- ` (prints the risk tier and runs nothing; exits 0 allowed, 1 blocked, 2 needs a person's approval) - `rafter mcp serve` → MCP tool `evaluate_command` ## Risk Tiers @@ -66,7 +66,7 @@ If the block is a false positive **for this specific context**, the right path i allow: - "^terraform destroy -target=module\\.sandbox" ``` -2. Or run once with an explicit ack flag: `rafter agent exec --force -- ` (logged to audit trail; still shows up in `rafter agent audit` history). +2. Or have a person run it: `rafter agent exec -- ` prompts for approval only at an interactive terminal and logs the override to the audit trail. There is no acknowledgement flag — `--force` no longer skips the prompt and a piped `yes` is not an approval — because any flag or input an agent can supply is not a person's decision. 3. Never disable the hook globally to get past one command — that silently drops protection for every future call. ## Audit Trail diff --git a/node/src/commands/agent/exec.ts b/node/src/commands/agent/exec.ts index 3a9441d9..bd9947e4 100644 --- a/node/src/commands/agent/exec.ts +++ b/node/src/commands/agent/exec.ts @@ -1,4 +1,4 @@ -import { Command } from "commander"; +import { Command, Option } from "commander"; import { CommandInterceptor } from "../../core/command-interceptor.js"; import { scanAddedDiffLines } from "../../scanners/git-diff-scan.js"; import { parseUnifiedDiffAddedLines } from "../../utils/git-diff.js"; @@ -6,18 +6,57 @@ import { execSync } from "child_process"; import readline from "readline"; import { fmt } from "../../utils/formatter.js"; +// Approval model (rf-ss67): the only party who can approve a command that the +// policy says needs approval is a person at an interactive terminal. There is +// no flag, env var or stdin trick that stands in for that, because every one +// of those can be produced by the agent whose command is being gated: +// * `--force` used to skip the prompt. Combined with the PreToolUse hook +// treating a quoted argument as prose, `rafter agent exec --force ""` +// ran any HIGH-tier command unprompted with the hook blind. The flag is +// kept only so old invocations parse; it changes nothing. +// * A piped "yes" is not a person. Approval is offered only when stdin is a +// TTY; otherwise the command is denied and says why. +// The machine owner widens policy in ~/.rafter/config.json, not per call. + +const DRY_RUN_EXIT = { allowed: 0, blocked: 1, approval: 2 } as const; + export function createExecCommand(): Command { return new Command("exec") .description("Execute command with security validation") - .argument("", "Command to execute") + .argument("", "Command to execute (quote it, or pass it after --)") .option("--skip-scan", "Skip pre-execution file scanning") - .option("--force", "Skip approval prompts (use with caution)") - .action(async (command, opts) => { + .option( + "--dry-run", + "Classify the command and exit without running it (exit 0 allowed, 1 blocked, 2 needs approval)", + ) + .addOption( + new Option("--force", "Deprecated: no longer skips approval (rf-ss67)").hideHelp(), + ) + .action(async (parts: string[], opts) => { + const command = joinCommandParts(parts); const interceptor = new CommandInterceptor(); // Step 1: Evaluate command const evaluation = interceptor.evaluate(command); + // Step 1b: --dry-run reports the classification and stops. Nothing runs, + // nothing is scanned, nothing is logged as executed. + if (opts.dryRun) { + const blocked = !evaluation.allowed && !evaluation.requiresApproval; + const verdict = blocked ? "BLOCKED" : evaluation.requiresApproval ? "REQUIRES APPROVAL" : "ALLOWED"; + console.log(`Dry run: ${verdict}`); + console.log(`Risk Level: ${evaluation.riskLevel.toUpperCase()}`); + console.log(`Requires approval: ${evaluation.requiresApproval ? "yes" : "no"}`); + if (evaluation.reason) { + console.log(`Reason: ${evaluation.reason}`); + } + console.log(`Command: ${command}`); + console.log("Not executed (--dry-run)."); + process.exit( + blocked ? DRY_RUN_EXIT.blocked : evaluation.requiresApproval ? DRY_RUN_EXIT.approval : DRY_RUN_EXIT.allowed, + ); + } + // Step 2: Handle blocked commands if (!evaluation.allowed && !evaluation.requiresApproval) { console.error(`\n${fmt.error("Command BLOCKED")}\n`); @@ -42,8 +81,13 @@ export function createExecCommand(): Command { } } - // Step 4: Handle approval required - if (evaluation.requiresApproval && !opts.force) { + // Step 4: Handle approval required — only a person at a terminal can. + if (evaluation.requiresApproval) { + if (opts.force) { + console.log( + `\n${fmt.warning("--force no longer skips approval (rf-ss67); approval needs a person at an interactive terminal")}\n`, + ); + } console.log(`\n${fmt.warning("Command requires approval")}\n`); console.log(`Risk Level: ${evaluation.riskLevel.toUpperCase()}`); console.log(`Command: ${command}`); @@ -52,6 +96,16 @@ export function createExecCommand(): Command { } console.log(); + if (!process.stdin.isTTY) { + console.log(`${fmt.error("Command denied: approval needs an interactive terminal, and stdin is not one")}`); + console.log( + "Run the command yourself at a terminal, or have the machine owner adjust " + + "commandPolicy in ~/.rafter/config.json.\n", + ); + interceptor.logEvaluation(evaluation, "blocked"); + process.exit(1); + } + const approved = await promptApproval(); if (!approved) { @@ -62,16 +116,13 @@ export function createExecCommand(): Command { console.log(`\n${fmt.success("Command approved by user")}\n`); interceptor.logEvaluation(evaluation, "overridden"); - } else if (opts.force && evaluation.requiresApproval) { - console.log(`\n${fmt.warning("Forcing execution (--force flag)")}\n`); - interceptor.logEvaluation(evaluation, "overridden"); } else { interceptor.logEvaluation(evaluation, "allowed"); } // Step 5: Execute command try { - const output = execSync(command, { + execSync(command, { stdio: "inherit", encoding: "utf-8" }); @@ -85,6 +136,22 @@ export function createExecCommand(): Command { }); } +/** + * One quoted argument is the command verbatim. Several (the `-- rm -rf x` form + * the docs show) are re-joined with shell quoting, so what the classifier sees + * is what the shell will run — `-- echo "a b"` becomes `echo 'a b'`, not `echo a b`. + */ +export function joinCommandParts(parts: string[]): string { + if (parts.length === 1) return parts[0]; + return parts.map(shellQuote).join(" "); +} + +function shellQuote(token: string): string { + if (token === "") return "''"; + if (/^[A-Za-z0-9_\/:=@.,+%-]+$/.test(token)) return token; + return `'${token.replace(/'/g, `'\\''`)}'`; +} + function isGitCommand(command: string): boolean { return command.trim().startsWith("git commit") || command.trim().startsWith("git push"); diff --git a/node/src/commands/agent/init.ts b/node/src/commands/agent/init.ts index eadb97dc..d19e3414 100644 --- a/node/src/commands/agent/init.ts +++ b/node/src/commands/agent/init.ts @@ -7,7 +7,7 @@ import { SkillManager } from "../../utils/skill-manager.js"; import fs from "fs"; import path from "path"; import os from "os"; -import { execSync } from "child_process"; +import { execSync, spawnSync } from "child_process"; import { fileURLToPath } from "url"; import { createRequire } from "module"; import { askYesNo } from "../../utils/prompt.js"; @@ -18,6 +18,65 @@ import yaml from "js-yaml"; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); +/** + * Resolve an ABSOLUTE, self-contained hook command (rf-er8a). Agents run a hook + * through `sh -c ""` in a minimal environment that need not have + * `rafter` — or even `node` — on PATH; a bare `rafter hook pretool` then exits + * 127, which those tools treat as a silent allow, so the gate is inert while + * settings.json says it is installed. Pinning BOTH the node interpreter and the + * CLI entrypoint makes the command resolvable regardless of PATH. (dist/index.js + * carries `#!/usr/bin/env node`, so an absolute entrypoint ALONE still fails + * where node is off PATH — the interpreter must be pinned too.) + */ +function hookEntrypoint(): string { + let entrypoint = process.argv[1] || ""; + try { entrypoint = fs.realpathSync(entrypoint); } catch { /* keep as-is */ } + return entrypoint; +} + +export function absoluteHookCommand(args: string): string { + return `${process.execPath} ${hookEntrypoint()} hook ${args}`; +} + +/** + * After writing the hooks, confirm the exact command we wrote actually runs and + * enforces — and refuse to report a clean success if it does not (rf-er8a / + * rf-fuwy). Returns human-readable warnings; empty means the gate is live. + */ +function installedHookWarnings(): string[] { + const warnings: string[] = []; + const entrypoint = hookEntrypoint(); + if (entrypoint.includes("/_npx/") || entrypoint.includes("\\_npx\\")) { + warnings.push( + `The rafter entrypoint is inside an npx cache (${entrypoint}), which is ephemeral: the ` + + `installed hook will stop resolving when the cache is cleared. Install globally ` + + `(npm install -g @rafter-security/cli) and re-run 'rafter agent init'.`, + ); + } + try { + const cmd = absoluteHookCommand("pretool"); + const res = spawnSync("sh", ["-c", cmd], { + input: JSON.stringify({ + hook_event_name: "PreToolUse", + tool_name: "Bash", + tool_input: { command: "rm -rf / --no-preserve-root" }, + permission_mode: "default", + cwd: process.cwd(), + }), + encoding: "utf-8", + timeout: 10_000, + }); + let decision: string | null = null; + try { decision = JSON.parse(res.stdout || "")?.hookSpecificOutput?.permissionDecision ?? null; } catch { decision = null; } + if (res.error || res.status === 127) { + warnings.push(`The installed PreToolUse hook did not execute (exit ${res.status ?? "spawn error"}) — the gate is inert. Command: ${cmd}`); + } else if (decision !== "deny") { + warnings.push(`The installed PreToolUse hook ran but did not block a synthetic critical command (decision=${decision ?? "none"}).`); + } + } catch { /* best-effort confirmation */ } + return warnings; +} + /** * Skills installed by `rafter agent init` for Claude Code / Codex. * @@ -297,20 +356,20 @@ function installClaudeCodeHooks(root: string): void { if (!settings.hooks.PreToolUse) settings.hooks.PreToolUse = []; if (!settings.hooks.PostToolUse) settings.hooks.PostToolUse = []; - const preHook = { type: "command", command: "rafter hook pretool" }; - const postHook = { type: "command", command: "rafter hook posttool" }; + const preHook = { type: "command", command: absoluteHookCommand("pretool") }; + const postHook = { type: "command", command: absoluteHookCommand("posttool") }; // Remove any existing Rafter hooks to avoid duplicates settings.hooks.PreToolUse = settings.hooks.PreToolUse.filter( (entry: any) => { const hooks = entry.hooks || []; - return !hooks.some((h: any) => h.command === "rafter hook pretool"); + return !hooks.some((h: any) => String(h.command ?? "").includes("hook pretool")); } ); settings.hooks.PostToolUse = settings.hooks.PostToolUse.filter( (entry: any) => { const hooks = entry.hooks || []; - return !hooks.some((h: any) => h.command === "rafter hook posttool"); + return !hooks.some((h: any) => String(h.command ?? "").includes("hook posttool")); } ); // Strip legacy SessionStart entry left over from <=0.7.4 installs. @@ -318,7 +377,7 @@ function installClaudeCodeHooks(root: string): void { settings.hooks.SessionStart = settings.hooks.SessionStart.filter( (entry: any) => { const hooks = entry.hooks || []; - return !hooks.some((h: any) => h.command === "rafter hook session-start"); + return !hooks.some((h: any) => String(h.command ?? "").includes("hook session-start")); } ); if (settings.hooks.SessionStart.length === 0) delete settings.hooks.SessionStart; @@ -364,15 +423,15 @@ function installCodexHooks(root: string): void { if (!config.hooks.PostToolUse) config.hooks.PostToolUse = []; // Codex uses the same hookSpecificOutput protocol as Claude Code (format=claude) - const preHook = { type: "command", command: "rafter hook pretool" }; - const postHook = { type: "command", command: "rafter hook posttool" }; + const preHook = { type: "command", command: absoluteHookCommand("pretool") }; + const postHook = { type: "command", command: absoluteHookCommand("posttool") }; // Remove existing rafter hooks config.hooks.PreToolUse = config.hooks.PreToolUse.filter( - (entry: any) => !(entry.hooks || []).some((h: any) => h.command?.startsWith("rafter hook pretool")) + (entry: any) => !(entry.hooks || []).some((h: any) => String(h.command ?? "").includes("hook pretool")) ); config.hooks.PostToolUse = config.hooks.PostToolUse.filter( - (entry: any) => !(entry.hooks || []).some((h: any) => h.command?.startsWith("rafter hook posttool")) + (entry: any) => !(entry.hooks || []).some((h: any) => String(h.command ?? "").includes("hook posttool")) ); // PreToolUse intercepts the tools Codex documents support for: Bash and @@ -428,15 +487,15 @@ function installCursorHooks(root: string): void { if (!config.hooks) config.hooks = {}; const events: { event: string; command: string }[] = [ - { event: "preToolUse", command: "rafter hook pretool --format cursor" }, - { event: "postToolUse", command: "rafter hook posttool --format cursor" }, - { event: "beforeShellExecution", command: "rafter hook pretool --format cursor" }, + { event: "preToolUse", command: absoluteHookCommand("pretool --format cursor") }, + { event: "postToolUse", command: absoluteHookCommand("posttool --format cursor") }, + { event: "beforeShellExecution", command: absoluteHookCommand("pretool --format cursor") }, ]; for (const { event, command } of events) { if (!Array.isArray(config.hooks[event])) config.hooks[event] = []; config.hooks[event] = config.hooks[event].filter( - (entry: any) => !entry?.command?.includes("rafter hook"), + (entry: any) => !(String(entry?.command ?? "").includes("hook pretool") || String(entry?.command ?? "").includes("hook posttool")), ); config.hooks[event].push({ command, type: "command", timeout: 5000 }); } @@ -572,10 +631,10 @@ function installGeminiHooks(root: string): void { // Remove existing rafter hooks settings.hooks.BeforeTool = settings.hooks.BeforeTool.filter( - (entry: any) => !(entry.hooks || []).some((h: any) => h.command?.includes("rafter hook pretool")) + (entry: any) => !(entry.hooks || []).some((h: any) => String(h.command ?? "").includes("hook pretool")) ); settings.hooks.AfterTool = settings.hooks.AfterTool.filter( - (entry: any) => !(entry.hooks || []).some((h: any) => h.command?.includes("rafter hook posttool")) + (entry: any) => !(entry.hooks || []).some((h: any) => String(h.command ?? "").includes("hook posttool")) ); // Gemini matchers are regexes against built-in tool names per @@ -584,11 +643,11 @@ function installGeminiHooks(root: string): void { // verification 2026-05-03 — schema confirmed against current Gemini docs.) settings.hooks.BeforeTool.push({ matcher: "run_shell_command|write_file|replace|edit", - hooks: [{ type: "command", command: "rafter hook pretool --format gemini", timeout: 5000 }], + hooks: [{ type: "command", command: absoluteHookCommand("pretool --format gemini"), timeout: 5000 }], }); settings.hooks.AfterTool.push({ matcher: ".*", - hooks: [{ type: "command", command: "rafter hook posttool --format gemini", timeout: 5000 }], + hooks: [{ type: "command", command: absoluteHookCommand("posttool --format gemini"), timeout: 5000 }], }); fs.writeFileSync(settingsPath, JSON.stringify(settings, null, 2), "utf-8"); @@ -1619,7 +1678,16 @@ export function createInitCommand(): Command { }, root, scope); console.log(); - console.log(fmt.success("Agent security initialized!")); + // rf-er8a: do not report a clean success if the hook we just wrote cannot + // execute and enforce. A silent 127 (or an npx-cache entrypoint) means the + // gate is inert even though settings.json looks configured. + const hookWarnings = claudeCodeOk ? installedHookWarnings() : []; + if (hookWarnings.length > 0) { + for (const w of hookWarnings) console.log(fmt.warning(w)); + console.log(fmt.warning("Agent security initialized WITH WARNINGS — the command gate may NOT be active (see above). Run 'rafter agent verify' to confirm.")); + } else { + console.log(fmt.success("Agent security initialized!")); + } console.log(); const anyIntegration = openclawOk || claudeCodeOk || codexOk || geminiOk || cursorOk || windsurfOk || continueOk || aiderOk || hermesOk || openCodeOk; diff --git a/node/src/commands/agent/scan.ts b/node/src/commands/agent/scan.ts index 8b6425b6..f18db67e 100644 --- a/node/src/commands/agent/scan.ts +++ b/node/src/commands/agent/scan.ts @@ -16,7 +16,7 @@ import { policyIgnoreToSuppressions, } from "../../core/custom-patterns.js"; import type { ScanIgnoreRule } from "../../core/config-schema.js"; -import { execSync, execFileSync } from "child_process"; +import { execFileSync } from "child_process"; import fs from "fs"; import os from "os"; import path from "path"; @@ -492,7 +492,9 @@ async function runGitAddedLineScan( if (!patch.trim()) { if (!opts.quiet) { - console.log(`\n${fmt.success(emptyMessage)}\n`); + // Status line, so stderr — stdout must stay parseable as JSON under + // --json, and outputScanResults owns the single stdout success line. + console.error(fmt.success(emptyMessage)); } outputScanResults([], opts, contextLabel, true, suppressions); return; @@ -501,7 +503,9 @@ async function runGitAddedLineScan( const addedLines = parseUnifiedDiffAddedLines(patch); if (addedLines.length === 0) { if (!opts.quiet) { - console.log(`\n${fmt.success(emptyMessage)}\n`); + // Status line, so stderr — stdout must stay parseable as JSON under + // --json, and outputScanResults owns the single stdout success line. + console.error(fmt.success(emptyMessage)); } outputScanResults([], opts, contextLabel, true, suppressions); return; diff --git a/node/src/commands/agent/verify.ts b/node/src/commands/agent/verify.ts index 10fee20f..31b3b7b1 100644 --- a/node/src/commands/agent/verify.ts +++ b/node/src/commands/agent/verify.ts @@ -2,6 +2,7 @@ import { Command } from "commander"; import { ConfigManager } from "../../core/config-manager.js"; import { BinaryManager } from "../../utils/binary-manager.js"; import { SkillManager } from "../../utils/skill-manager.js"; +import { resolveHookControl } from "../../core/hook-control.js"; import { spawnSync } from "child_process"; import fs from "fs"; import path from "path"; @@ -9,6 +10,58 @@ import os from "os"; import yaml from "js-yaml"; import { fmt } from "../../utils/formatter.js"; +/** + * Run a configured PreToolUse hook command EXACTLY as Claude Code would — through + * `sh -c ""`, with a synthetic payload on + * stdin — and return its exit status and the permissionDecision it emitted. + * + * This is the whole point of rf-fuwy: `agent verify` used to confirm only that + * the hook was CONFIGURED (a substring match) and reported a completely inert + * gate as healthy, byte-identical to a working one. A command that does not + * resolve exits 127; Claude Code blocks only on exit 2, so 127 is a silent + * allow. Executing the command is the only check that cannot be fooled by that. + */ +export function runConfiguredHook( + command: string, + toolCommand: string, +): { status: number | null; decision: string | null; error?: string; stdout: string } { + const payload = JSON.stringify({ + session_id: `rafter-verify-${process.pid}-${Date.now()}`, + transcript_path: "", + cwd: process.cwd(), + permission_mode: "default", + hook_event_name: "PreToolUse", + tool_name: "Bash", + tool_input: { command: toolCommand }, + }); + // `sh -c` reproduces exactly how Claude Code invokes a shell-form hook, so a + // command that resolves in this terminal but not in the editor (or vice versa) + // is exercised the same way the editor would exercise it. Retry a transient + // spawn failure (e.g. EAGAIN under process pressure) so a resource hiccup does + // not get reported as an inert gate — a false alarm is worse than a slow check. + let result = spawnSync("sh", ["-c", command], { input: payload, encoding: "utf-8", timeout: 10_000 }); + // Retry a spawn-level error OR a signal kill: under the same process pressure + // that produces EAGAIN, the OOM killer sends SIGKILL, which sets `signal` (not + // `error`) with a null status — the identical false-alarm scenario by another + // route (measured by achebe). A persistent failure still returns decision:null, + // so a genuinely inert hook is still reported inert — no fail-open. + for (let attempt = 0; attempt < 2 && (result.error || result.signal); attempt++) { + result = spawnSync("sh", ["-c", command], { input: payload, encoding: "utf-8", timeout: 10_000 }); + } + if (result.error) { + return { status: null, decision: null, error: result.error.message, stdout: "" }; + } + const stdout = result.stdout ?? ""; + let decision: string | null = null; + try { + const parsed = JSON.parse(stdout); + decision = parsed?.hookSpecificOutput?.permissionDecision ?? null; + } catch { + decision = null; + } + return { status: result.status, decision, stdout }; +} + interface CheckResult { name: string; passed: boolean; @@ -86,13 +139,67 @@ function checkClaudeCode(): CheckResult { // Substring match — Python install writes an absolute path // (/home/foo/bin/rafter hook pretool), Node writes the bare command. const hooks = settings?.hooks?.PreToolUse || []; - const hasRafterHook = hooks.some((entry: any) => - (entry.hooks || []).some((h: any) => String(h?.command ?? "").includes("rafter hook pretool")) - ); - if (!hasRafterHook) { + const commands: string[] = []; + for (const entry of hooks) { + for (const h of entry.hooks || []) { + const cmd = String(h?.command ?? ""); + if (cmd.includes("hook pretool")) commands.push(cmd); + } + } + if (commands.length === 0) { return { name, passed: false, optional: true, detail: "Rafter hooks not installed — run 'rafter agent init --with-claude-code'" }; } - return { name, passed: true, detail: "Hooks installed" }; + + // A configured hook that has been deliberately switched off is a valid + // state, not a failure: expecting a "deny" from a disabled hook would fail a + // machine whose owner turned it off on purpose (rf-fuwy amendment). + const control = resolveHookControl(); + if (!control.commandPolicyEnabled) { + return { + name, + passed: false, + optional: true, + detail: `Hook installed but command interception is DISABLED (${control.source.commandPolicy}) — no command is blocked. Re-enable to enforce.`, + }; + } + + // rf-fuwy: EXECUTE the configured command, do not just read it. A gate that + // cannot run (exit 127) is inert but was reported "installed"; a gate that + // runs but does not block a CRITICAL command is misconfigured. Assert both + // directions with synthetic payloads. + const hookCmd = commands[0]; + const danger = runConfiguredHook(hookCmd, "rm -rf / --no-preserve-root"); + if (danger.error || danger.status === 127) { + return { + name, + passed: false, + detail: + `Hook is CONFIGURED but NOT EXECUTABLE (${danger.error ? danger.error : "exit 127"}): "${hookCmd}" does not resolve, so the gate is INERT ` + + `(Claude Code blocks only on exit 2; anything else is allowed). Reinstall with 'rafter agent init --with-claude-code', which writes an absolute path.`, + }; + } + if (danger.decision !== "deny") { + return { + name, + passed: false, + detail: + `Hook executes (exit ${danger.status}) but did NOT block a synthetic CRITICAL command (decision=${danger.decision ?? "none/unparseable"}). ` + + `The gate is not enforcing.`, + }; + } + const benign = runConfiguredHook(hookCmd, "echo hello"); + if (benign.decision === "deny") { + return { + name, + passed: false, + detail: `Hook blocks even a benign command (echo) — over-blocking; check policy/config.`, + }; + } + // Note the caveat honestly: verify runs in the terminal's environment, which + // may differ from the editor's — a pass proves the command runs and enforces + // HERE, and a failure proves it is broken; it is not proof the editor's PATH + // resolves it too. + return { name, passed: true, detail: `Hooks installed and enforcing (blocked a synthetic 'rm -rf /'; verify runs in this shell's env)` }; } catch (e) { return { name, passed: false, optional: true, detail: `Cannot read settings: ${e}` }; } @@ -401,9 +508,24 @@ function probeClaudeCode(): CheckResult { const auditPath = path.join(home, ".rafter", "audit.jsonl"); const sizeBefore = fs.existsSync(auditPath) ? fs.statSync(auditPath).size : 0; - // Resolve the rafter binary the same way Claude Code would: `rafter hook - // pretool` on PATH. Fall back to argv[0] if PATH lookup fails. - const result = spawnSync(process.execPath, [process.argv[1], "hook", "pretool"], { + // Run the command EXACTLY as configured in settings.json, through `sh -c`, the + // way Claude Code would. (Earlier this spawned verify's own node + argv[1], + // which always resolves and therefore passed even when the CONFIGURED command + // did not exist — the rf-fuwy defect. Reading and running the real command is + // the only faithful probe.) + let configuredCmd = "rafter hook pretool"; + try { + const settings = JSON.parse(fs.readFileSync(settingsPath, "utf-8")); + for (const entry of settings?.hooks?.PreToolUse || []) { + for (const h of entry.hooks || []) { + const cmd = String(h?.command ?? ""); + if (cmd.includes("hook pretool")) { configuredCmd = cmd; break; } + } + } + } catch { + // fall back to the bare command below + } + const result = spawnSync("sh", ["-c", configuredCmd], { input: stdinPayload, encoding: "utf-8", timeout: 10_000, diff --git a/node/src/commands/backend/get.ts b/node/src/commands/backend/get.ts index 76019d12..40471d1a 100644 --- a/node/src/commands/backend/get.ts +++ b/node/src/commands/backend/get.ts @@ -1,5 +1,4 @@ import { Command } from "commander"; -import axios from "axios"; import { API, resolveKey, @@ -7,7 +6,7 @@ import { EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND } from "../../utils/api.js"; -import { handleScanStatus } from "./scan-status.js"; +import { handleScanStatus, fetchScanWithRetry, PollGaveUpError } from "./scan-status.js"; export function createGetCommand(): Command { return new Command("get") @@ -20,9 +19,14 @@ export function createGetCommand(): Command { const key = resolveKey(opts.apiKey); if (!opts.interactive) { try { - const { data } = await axios.get( - `${API}/static/scan`, - { params: { scan_id, format: opts.format }, headers: { "x-api-key": key } } + // sable-l10k — retried, because this is the command the poll loop's + // give-up message recommends. A remedy defeated by the same transient + // failure it is recommended for is not a remedy. + const { data } = await fetchScanWithRetry( + scan_id, + { "x-api-key": key }, + opts.format, + opts.quiet ); const exitCode = writePayload(data, opts.format, opts.quiet); process.exit(exitCode); @@ -30,6 +34,8 @@ export function createGetCommand(): Command { if (e.response?.status === 404) { console.error(`Scan '${scan_id}' not found`); process.exit(EXIT_SCAN_NOT_FOUND); + } else if (e instanceof PollGaveUpError) { + console.error(e.message); } else if (e.response?.data) { console.error(e.response.data); } else if (e instanceof Error) { diff --git a/node/src/commands/backend/run.ts b/node/src/commands/backend/run.ts index 6fb73162..db46df9f 100644 --- a/node/src/commands/backend/run.ts +++ b/node/src/commands/backend/run.ts @@ -1,5 +1,4 @@ import { Command } from "commander"; -import axios from "axios"; import ora from "ora"; import { detectRepo } from "../../utils/git.js"; import { @@ -8,8 +7,9 @@ import { EXIT_GENERAL_ERROR, EXIT_QUOTA_EXHAUSTED, EXIT_CONFIRMATION_REQUIRED, - handle403 -} from "../../utils/api.js"; + handle403, + apiClient, + apiUrl} from "../../utils/api.js"; import { ConfigManager } from "../../core/config-manager.js"; import { loadPolicy } from "../../core/policy-loader.js"; import { askYesNo } from "../../utils/prompt.js"; @@ -133,8 +133,8 @@ export async function runRemoteScan(opts: RunOpts): Promise { if (!opts.quiet) { const spinner = ora("Submitting scan").start(); try { - const { data } = await axios.post( - `${API}/static/scan`, + const { data } = await apiClient.post( + apiUrl("static/scan"), body, { headers: { "x-api-key": key } } ); @@ -161,8 +161,8 @@ export async function runRemoteScan(opts: RunOpts): Promise { } } else { try { - const { data } = await axios.post( - `${API}/static/scan`, + const { data } = await apiClient.post( + apiUrl("static/scan"), body, { headers: { "x-api-key": key } } ); diff --git a/node/src/commands/backend/scan-status.ts b/node/src/commands/backend/scan-status.ts index ea551f20..2d7090ac 100644 --- a/node/src/commands/backend/scan-status.ts +++ b/node/src/commands/backend/scan-status.ts @@ -1,64 +1,300 @@ -import axios from "axios"; import ora from "ora"; import { API, + API_TIMEOUT_SHORT_MS, writePayload, EXIT_GENERAL_ERROR, - EXIT_SCAN_NOT_FOUND -} from "../../utils/api.js"; + EXIT_SCAN_NOT_FOUND, + apiClient, + apiUrl} from "../../utils/api.js"; import { fmt as output } from "../../utils/formatter.js"; +/** + * sable-l10k — the report a scan writes is not durable the instant the scan + * flips to completed, so a poll can legitimately hit a 5xx (commonly + * "Failed to fetch report from storage: Object not found") on an otherwise + * healthy scan. Retry those instead of failing the whole run; a scan that + * would have succeeded 10 seconds later must not die on one bad read. + * + * 404 is transient only AFTER the scan is known to exist: once the first poll + * has succeeded, a missing scan is read-after-write lag rather than a wrong id. + * On the first poll a 404 is still fatal. + */ +export const MAX_TRANSIENT_POLL_FAILURES = 5; + +/** + * Total transient failures tolerated across one `handleScanStatus` call. + * + * The consecutive counter resets on every success, which is what we want — a + * twenty-minute scan with one blip at minute 2 and another at minute 18 should + * not die. But reset-on-success alone means a backend alternating 200/500 + * forever never exhausts the budget, and the CLI has no wall-clock deadline to + * stop it. This is the backstop for that. + */ +export const MAX_TOTAL_TRANSIENT_POLL_FAILURES = 20; + +/** Longest single error detail we will echo back. Servers can be verbose. */ +const MAX_ERROR_DETAIL_CHARS = 200; + +/** + * Transient = the request never got an answer, or got one the server itself + * describes as temporary. + * + * `scanExists` gates 404: before the first successful poll a 404 means the + * scan id is wrong, and retrying it just delays a clear answer. + */ +function isTransientPollError(e: any, scanExists: boolean): boolean { + const status = e?.response?.status; + if (status === undefined) { + // Retry only errors that came from the HTTP layer. A TypeError thrown from + // our own code also has no `response`, and must not be mistaken for a flaky + // backend and retried five times. + return Boolean(e?.isAxiosError || e?.request); + } + if (status === 404) return scanExists; + return status >= 500 || status === 408; +} + +function truncate(value: unknown): string { + // A server is free to answer {"error": {"message": "..."}}. Coerce before + // touching string methods — this used to throw, which turned a retryable + // failure into an immediate crash with a nonsense message. + const s = typeof value === "string" ? value : JSON.stringify(value) ?? String(value); + const flat = s.replace(/[\r\n]+/g, " ").trim(); + return flat.length > MAX_ERROR_DETAIL_CHARS + ? `${flat.slice(0, MAX_ERROR_DETAIL_CHARS)}…` + : flat; +} + +function describeHttpError(e: any): string { + const status = e?.response?.status; + const data = e?.response?.data; + let detail: unknown = ""; + if (typeof data === "string") { + detail = data; + } else if (data && typeof data === "object") { + detail = (data as any).error ?? data; + } else if (e instanceof Error) { + detail = e.message; + } + const detailText = truncate(detail); + return status + ? `HTTP ${status}${detailText ? ` — ${detailText}` : ""}` + : detailText || String(e); +} + +/** + * The message a customer actually sees when the report never becomes readable. + * Storage-layer wording ("Object not found") is kept as supporting detail, not + * as the whole explanation, and the next action is spelled out. + */ +export function unreadableReportMessage( + scan_id: string, + lastError: string, + attempts: number = MAX_TRANSIENT_POLL_FAILURES, + reachedServer: boolean = true +): string { + if (!reachedServer) { + return ( + `Rafter could not reach the API after ${attempts} attempts.\n` + + `Check your network and that https://rafter.so is reachable from here.\n` + + `Your scan id is ${scan_id} — the scan may still be running.\n` + + `Last error: ${lastError}` + ); + } + return ( + `Rafter could not read the report for scan ${scan_id} after ` + + `${attempts} attempts.\n` + + `The scan itself may have finished — retry with: rafter get ${scan_id}\n` + + `or open the scan in your dashboard at https://rafter.so/dashboard\n` + + `Last response from the server: ${lastError}` + ); +} + +export const BASE_BACKOFF_MS = 2000; + +/** 2s, 4s, 8s, 16s — the 5th failure gives up rather than sleeping again. */ +export function backoffMs(consecutiveFailures: number): number { + return BASE_BACKOFF_MS * 2 ** (consecutiveFailures - 1); +} + +/** + * Thrown when polling gives up after repeated transient failures. Carries the + * customer-facing message so callers do not have to rebuild it. + */ +export class PollGaveUpError extends Error {} + +/** + * A failure budget shared across every poll in one `handleScanStatus` call. + * + * Counting per-request would let a backend that alternates 200/500 forever + * reset the counter on each success and never exhaust it — the CLI has no + * wall-clock deadline, so that loop would never end. + */ +class FailureBudget { + consecutive = 0; + total = 0; + last = ""; + /** False once any failure carried no HTTP response at all. */ + lastReachedServer = true; + + record(detail: string, reachedServer: boolean): number { + this.consecutive += 1; + this.total += 1; + this.last = detail; + this.lastReachedServer = reachedServer; + return this.consecutive; + } + + /** A success clears the consecutive run, but never refunds the total. */ + reset(): void { + this.consecutive = 0; + } + + get exhausted(): boolean { + return ( + this.consecutive >= MAX_TRANSIENT_POLL_FAILURES || + this.total >= MAX_TOTAL_TRANSIENT_POLL_FAILURES + ); + } +} + +type RetryNotice = (attempt: number, waitMs: number, detail: string) => void; + +/** + * One poll, with retry/backoff over transient failures. + * Non-transient errors are rethrown for the caller to classify. + */ +async function pollUntilReadable( + scan_id: string, + headers: any, + fmt: string, + budget: FailureBudget, + scanExists: boolean, + onRetry?: RetryNotice +): Promise { + for (;;) { + try { + const res = await apiClient.get(apiUrl("static/scan"), { + params: { scan_id, format: fmt }, + headers, + // Without this a hung server stalls inside a single request, and the + // retry loop can only notice between attempts. + timeout: API_TIMEOUT_SHORT_MS, + }); + budget.reset(); + return res; + } catch (e: any) { + if (!isTransientPollError(e, scanExists)) throw e; + + const attempt = budget.record( + describeHttpError(e), + e?.response?.status !== undefined + ); + if (budget.exhausted) { + throw new PollGaveUpError( + unreadableReportMessage( + scan_id, + budget.last, + budget.total, + budget.lastReachedServer + ) + ); + } + + const waitMs = backoffMs(attempt); + onRetry?.(attempt, waitMs, budget.last); + await new Promise((r) => setTimeout(r, waitMs)); + } + } +} + +/** + * A single scan fetch with the same retry budget the poll loop uses. + * + * `rafter get ` is what the give-up message tells customers to run, so it + * must not be defeated by exactly the transient failure that produced the + * message. A 404 here is still fatal — that is a wrong id, not lag. + */ +export async function fetchScanWithRetry( + scan_id: string, + headers: any, + fmt: string, + quiet?: boolean +): Promise { + const budget = new FailureBudget(); + const onRetry: RetryNotice | undefined = quiet + ? undefined + : (attempt, waitMs, detail) => { + console.error( + `Report not readable yet (${detail}); retrying in ${Math.round(waitMs / 1000)}s ` + + `(${attempt}/${MAX_TRANSIENT_POLL_FAILURES})` + ); + }; + return pollUntilReadable(scan_id, headers, fmt, budget, false, onRetry); +} + +const IN_PROGRESS = ["queued", "pending", "processing"]; + export async function handleScanStatus(scan_id: string, headers: any, fmt: string, quiet?: boolean): Promise { - // First poll + const budget = new FailureBudget(); + + // Retries are printed to stderr, not just into the spinner: ora renders + // nothing on a non-TTY, and CI is exactly where this diagnostic matters. + const onRetry: RetryNotice | undefined = quiet + ? undefined + : (attempt, waitMs, detail) => { + console.error( + `Report not readable yet (${detail}); retrying in ${Math.round(waitMs / 1000)}s ` + + `(${attempt}/${MAX_TRANSIENT_POLL_FAILURES})` + ); + }; + + // First poll. A 404 here really does mean "no such scan" — do not retry it. + // Transient 5xx IS retried, so that the `rafter get ` this command + // recommends on failure is not itself defeated by one bad read. let poll; try { - poll = await axios.get( - `${API}/static/scan`, - { params: { scan_id, format: fmt }, headers } - ); + poll = await pollUntilReadable(scan_id, headers, fmt, budget, false, onRetry); } catch (e: any) { - if (e.response?.status === 404) { + if (e?.response?.status === 404) { console.error(output.error(`Scan '${scan_id}' not found`)); return EXIT_SCAN_NOT_FOUND; } - console.error(output.error(`${e.response?.data || e.message}`)); + console.error( + output.error(e instanceof PollGaveUpError ? e.message : describeHttpError(e)) + ); return EXIT_GENERAL_ERROR; } let status = poll.data.status; - if (["queued", "pending", "processing"].includes(status)) { - if (!quiet) { - const spinner = ora("Waiting for scan to complete... (this could take several minutes)").start(); - while (["queued", "pending", "processing"].includes(status)) { + if (IN_PROGRESS.includes(status)) { + const spinner = quiet + ? undefined + : ora("Waiting for scan to complete... (this could take several minutes)").start(); + + try { + while (IN_PROGRESS.includes(status)) { await new Promise((r) => setTimeout(r, 10000)); - poll = await axios.get( - `${API}/static/scan`, - { params: { scan_id, format: fmt }, headers } - ); + poll = await pollUntilReadable(scan_id, headers, fmt, budget, true, onRetry); status = poll.data.status; if (status === "completed") { - spinner.succeed("Scan completed"); + spinner?.succeed("Scan completed"); return writePayload(poll.data, fmt, quiet); } else if (status === "failed") { - spinner.fail("Scan failed"); + spinner?.fail("Scan failed"); return EXIT_GENERAL_ERROR; } } + } catch (e: any) { + spinner?.fail("Could not retrieve scan report"); + console.error( + output.error(e instanceof PollGaveUpError ? e.message : describeHttpError(e)) + ); + return EXIT_GENERAL_ERROR; + } + if (!quiet) { console.error(`Scan status: ${status}`); - } else { - while (["queued", "pending", "processing"].includes(status)) { - await new Promise((r) => setTimeout(r, 10000)); - poll = await axios.get( - `${API}/static/scan`, - { params: { scan_id, format: fmt }, headers } - ); - status = poll.data.status; - if (status === "completed") { - return writePayload(poll.data, fmt, quiet); - } else if (status === "failed") { - return EXIT_GENERAL_ERROR; - } - } } } else if (status === "completed") { if (!quiet) { diff --git a/node/src/commands/backend/usage.ts b/node/src/commands/backend/usage.ts index 5a1c2c0e..3c18412a 100644 --- a/node/src/commands/backend/usage.ts +++ b/node/src/commands/backend/usage.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { API, resolveKey, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { API, resolveKey, EXIT_GENERAL_ERROR, apiClient, apiUrl} from "../../utils/api.js"; export function createUsageCommand(): Command { return new Command("usage") @@ -8,7 +7,7 @@ export function createUsageCommand(): Command { .action(async (opts) => { const key = resolveKey(opts.apiKey); try { - const { data } = await axios.get(`${API}/static/usage`, { headers: { "x-api-key": key } }); + const { data } = await apiClient.get(apiUrl("static/usage"), { headers: { "x-api-key": key } }); console.log(JSON.stringify(data, null, 2)); } catch (e: any) { if (e.response?.data) { diff --git a/node/src/commands/issues/from-scan.ts b/node/src/commands/issues/from-scan.ts index 88d01575..aca9f0f8 100644 --- a/node/src/commands/issues/from-scan.ts +++ b/node/src/commands/issues/from-scan.ts @@ -7,8 +7,7 @@ */ import { Command } from "commander"; import fs from "fs"; -import axios from "axios"; -import { API, resolveKey, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { API, resolveKey, EXIT_GENERAL_ERROR, apiClient, apiUrl} from "../../utils/api.js"; import { detectRepo } from "../../utils/git.js"; import { fmt } from "../../utils/formatter.js"; import { createIssue, listOpenIssues } from "./github-client.js"; @@ -165,21 +164,50 @@ 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 ): Promise { const key = resolveKey(apiKey); - const { data } = await axios.get(`${API}/static/scan`, { + const { data } = await apiClient.get(apiUrl("static/scan"), { params: { scan_id: scanId, format: "json" }, 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[] { +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 1298263a..947f2e0f 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 04853ab0..b9209d51 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/src/commands/mcp/server.ts b/node/src/commands/mcp/server.ts index 821f0534..48ffd243 100644 --- a/node/src/commands/mcp/server.ts +++ b/node/src/commands/mcp/server.ts @@ -15,9 +15,8 @@ import { AuditLogger } from "../../core/audit-logger.js"; import { ConfigManager, redactConfigSecrets, isSecretConfigKey, maskSecretValue } from "../../core/config-manager.js"; import { listDocs, resolveDocSelector, fetchDoc } from "../../core/docs-loader.js"; import { writeSuppression } from "../../core/suppression-writer.js"; -import { apiUrl } from "../../utils/api.js"; +import { apiUrl, apiClient} from "../../utils/api.js"; import { describeSitesError, resolveMcpApiKey } from "../sites/errors.js"; -import axios from "axios"; import { createRequire } from "module"; const _require = createRequire(import.meta.url); @@ -361,7 +360,7 @@ export function createServer(): Server { const key = resolveMcpApiKey(); if (!key) return errorResult("No API key configured. Set RAFTER_API_KEY or run 'rafter agent config set backend.apiKey '."); try { - const { data } = await axios.post(apiUrl("static/sites"), { url }, { headers: { "x-api-key": key } }); + const { data } = await apiClient.post(apiUrl("static/sites"), { url }, { headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); @@ -378,7 +377,7 @@ export function createServer(): Server { const body: Record = projectId ? { projectId } : { url }; if (Array.isArray(args?.sections)) body.sections = (args!.sections as unknown[]).map((s) => String(s)); try { - const { data } = await axios.post(apiUrl("static/sites/scan"), body, { headers: { "x-api-key": key } }); + const { data } = await apiClient.post(apiUrl("static/sites/scan"), body, { headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); @@ -393,7 +392,7 @@ export function createServer(): Server { if (args?.offset !== undefined) params.offset = String(args.offset); if (args?.include_archived) params.include_archived = "true"; try { - const { data } = await axios.get(apiUrl("static/sites"), { params, headers: { "x-api-key": key } }); + const { data } = await apiClient.get(apiUrl("static/sites"), { params, headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); @@ -406,7 +405,7 @@ export function createServer(): Server { const key = resolveMcpApiKey(); if (!key) return errorResult("No API key configured. Set RAFTER_API_KEY or run 'rafter agent config set backend.apiKey '."); try { - const { data } = await axios.get(apiUrl(`static/sites/${encodeURIComponent(id)}`), { headers: { "x-api-key": key } }); + const { data } = await apiClient.get(apiUrl(`static/sites/${encodeURIComponent(id)}`), { headers: { "x-api-key": key } }); return textResult(data); } catch (e: any) { return errorResult(describeSitesError(e).message); diff --git a/node/src/commands/notify.ts b/node/src/commands/notify.ts index 459f6597..782632eb 100644 --- a/node/src/commands/notify.ts +++ b/node/src/commands/notify.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { API, resolveKey, EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND } from "../utils/api.js"; +import { API, resolveKey, EXIT_GENERAL_ERROR, EXIT_SCAN_NOT_FOUND, apiClient, apiUrl} from "../utils/api.js"; import { validateWebhookUrl } from "../core/audit-logger.js"; import { ConfigManager } from "../core/config-manager.js"; import { fmt, isAgentMode } from "../utils/formatter.js"; @@ -221,7 +220,7 @@ export function createNotifyCommand(): Command { if (scanId) { const key = resolveKey(opts?.apiKey as string | undefined); try { - const { data } = await axios.get(`${API}/static/scan`, { + const { data } = await apiClient.get(apiUrl("static/scan"), { params: { scan_id: scanId, format: "json" }, headers: { "x-api-key": key }, }); diff --git a/node/src/commands/sites/create.ts b/node/src/commands/sites/create.ts index dc35d4c8..d8b36f38 100644 --- a/node/src/commands/sites/create.ts +++ b/node/src/commands/sites/create.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR, apiClient} from "../../utils/api.js"; import { describeSitesError, rejectUnsupportedFormat } from "./errors.js"; export interface SitesCreateOpts { @@ -14,7 +13,7 @@ export async function runSitesCreate(url: string, opts: SitesCreateOpts): Promis if (rejectUnsupportedFormat(opts.format)) return EXIT_GENERAL_ERROR; const key = resolveKey(opts.apiKey); try { - const { data } = await axios.post( + const { data } = await apiClient.post( apiUrl("static/sites"), { url }, { headers: { "x-api-key": key } } diff --git a/node/src/commands/sites/get.ts b/node/src/commands/sites/get.ts index 1a163417..e002f209 100644 --- a/node/src/commands/sites/get.ts +++ b/node/src/commands/sites/get.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR, apiClient} from "../../utils/api.js"; import { describeSitesError, rejectUnsupportedFormat } from "./errors.js"; export interface SitesGetOpts { @@ -14,7 +13,7 @@ export async function runSitesGet(id: string, opts: SitesGetOpts): Promise { if (opts.includeArchived) params.include_archived = "true"; try { - const { data } = await axios.get( + const { data } = await apiClient.get( apiUrl("static/sites"), { params, headers: { "x-api-key": key } } ); diff --git a/node/src/commands/sites/scan.ts b/node/src/commands/sites/scan.ts index b584585b..b57e8c55 100644 --- a/node/src/commands/sites/scan.ts +++ b/node/src/commands/sites/scan.ts @@ -1,6 +1,5 @@ import { Command } from "commander"; -import axios from "axios"; -import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR } from "../../utils/api.js"; +import { apiUrl, resolveKey, writePayload, EXIT_GENERAL_ERROR, apiClient} from "../../utils/api.js"; import { describeSitesError, rejectUnsupportedFormat } from "./errors.js"; const VALID_SECTIONS = new Set(["flight", "security", "dns"]); @@ -42,7 +41,7 @@ export async function runSitesScan(projectIdOrUrl: string, opts: SitesScanOpts): } try { - const { data } = await axios.post( + const { data } = await apiClient.post( apiUrl("static/sites/scan"), body, { headers: { "x-api-key": key } } diff --git a/node/src/core/risk-rules.ts b/node/src/core/risk-rules.ts index 491027dd..0d8c947e 100644 --- a/node/src/core/risk-rules.ts +++ b/node/src/core/risk-rules.ts @@ -162,7 +162,9 @@ interface Piece { } function isOpChar(c: string): boolean { - return c === ";" || c === "&" || c === "|" || c === ">" || c === "<"; + // \n and \r are statement separators (rf-6pqx): a newline ends a command + // exactly as ";" does, so a payload on a later line is classified on its own. + return c === ";" || c === "&" || c === "|" || c === ">" || c === "<" || c === "\n" || c === "\r"; } /** Read a `$(…)` substitution starting at `i`; returns its contents and the next index. */ @@ -191,18 +193,32 @@ function readBacktick(s: string, i: number): { inner: string; next: number } { return { inner, next: Math.min(j + 1, s.length) }; } -/** Split a command line into words and operators, respecting quotes and substitutions. */ -function tokenize(s: string): Piece[] { +/** + * Split a command line into words and operators, respecting quotes and substitutions. + * `unterminated` is set when a quote is never closed — the parse is then unreliable + * and the caller must FAIL CLOSED (match the raw string) rather than trust a + * desynchronized sanitization (se-y6vo: `$'a\'b'` swallows a trailing payload). + */ +function tokenize(s: string): { pieces: Piece[]; unterminated: boolean } { const pieces: Piece[] = []; + let unterminated = false; let i = 0; while (i < s.length) { const c = s[i]; - if (/\s/.test(c)) { i++; continue; } + // Whitespace EXCEPT newlines is skipped; a newline falls through to the + // operator branch below so it becomes a statement separator (rf-6pqx). + if (/\s/.test(c) && c !== "\n" && c !== "\r") { i++; continue; } if (isOpChar(c)) { const start = i; + if (c === "\n" || c === "\r") { + // Normalize a line break (incl. CRLF) to a ";" separator piece. + i += 1; + pieces.push({ start, end: i, op: ";", text: ";", quoted: false, substs: [] }); + continue; + } const two = s.slice(i, i + 2); const op = (two === "&&" || two === "||" || two === ">>" || two === "<<") ? two : c; i += op.length; @@ -220,6 +236,12 @@ function tokenize(s: string): Piece[] { if (/\s/.test(ch) || isOpChar(ch)) break; if (ch === "\\") { + // Line continuation: `\` immediately before a newline (incl. CRLF) is + // deleted by the shell — `r\m` is `rm`, so the newline must not be + // absorbed into the word (rf-6pqx/se-y6vo). + const nxt = s[i + 1] ?? ""; + if (nxt === "\n") { i += 2; continue; } + if (nxt === "\r") { i += 2; if (s[i] === "\n") i++; continue; } i++; if (i < s.length) { text += s[i]; i++; } continue; @@ -229,8 +251,12 @@ function tokenize(s: string): Piece[] { if (ch === "'") { i++; quoted = true; - while (i < s.length && s[i] !== "'") { text += s[i]; i++; } - i++; + let closed = false; + while (i < s.length) { + if (s[i] === "'") { closed = true; i++; break; } + text += s[i]; i++; + } + if (!closed) unterminated = true; continue; } @@ -238,8 +264,13 @@ function tokenize(s: string): Piece[] { if (ch === '"') { i++; quoted = true; - while (i < s.length && s[i] !== '"') { + let closed = false; + while (i < s.length) { + if (s[i] === '"') { closed = true; i++; break; } if (s[i] === "\\") { + const nxt = s[i + 1] ?? ""; + if (nxt === "\n") { i += 2; continue; } + if (nxt === "\r") { i += 2; if (s[i] === "\n") i++; continue; } i++; if (i < s.length) { text += s[i]; i++; } continue; @@ -253,7 +284,7 @@ function tokenize(s: string): Piece[] { text += s[i]; i++; } - i++; + if (!closed) unterminated = true; continue; } @@ -271,7 +302,7 @@ function tokenize(s: string): Piece[] { pieces.push({ start, end: i, op: null, text, quoted, substs }); } - return pieces; + return { pieces, unterminated }; } /** `/usr/bin/rm` → `rm`; used to classify the executable of a segment. */ @@ -283,6 +314,16 @@ function execName(text: string): string { const ENV_ASSIGNMENT = /^[A-Za-z_][A-Za-z0-9_]*=/; const SHELL_C_FLAG = /^-[a-z]*c$/; const LONG_FLAG_WITH_VALUE = /^(--[a-z][a-z-]*)=/; +const NUMERIC_ARG = /^\d+[a-z]*$/i; + +/** + * A heredoc introducer: `<<`, optional `-`/`~` (indented-terminator forms), an + * optional quote around the delimiter, and the delimiter word. Group 1 is the + * dash/tilde, group 3 is the delimiter name. `g` so we can find all on a line. + */ +const HEREDOC_START = /(? `rm`), used to ask + * what the NEXT stage of a pipeline does with this stage's output. + */ +function segmentExec(pieces: Piece[]): string { + const isRedirectTarget = new Array(pieces.length).fill(false); + for (let i = 1; i < pieces.length; i++) { + const prev = pieces[i - 1]; + if (prev.op && REDIRECT_OPS.has(prev.op) && !pieces[i].op) isRedirectTarget[i] = true; + } + for (let i = 0; i < pieces.length; i++) { + const p = pieces[i]; + if (p.op || isRedirectTarget[i]) continue; + if (!p.quoted && ENV_ASSIGNMENT.test(p.text)) continue; + if (!p.quoted && TAIL_WRAPPERS.has(execName(p.text))) { + let j = i + 1; + while (j < pieces.length) { + const q = pieces[j]; + if (q.op || isRedirectTarget[j]) { j++; continue; } + if (q.text.startsWith("-") || /^\d+[a-z]*$/i.test(q.text)) { j++; continue; } + break; + } + i = j - 1; + continue; + } + return execName(p.text); + } + return ""; +} + +function processSegment( + pieces: Piece[], + depth: number, + out: Replacement[], + pipedIntoShell = false, + outputExecuted = false +): void { // A word is a redirect target when the piece before it is `>`/`>>`/`<`. const isRedirectTarget = new Array(pieces.length).fill(false); for (let i = 1; i < pieces.length; i++) { @@ -337,6 +414,15 @@ function processSegment(pieces: Piece[], depth: number, out: Replacement[]): voi } const codeCarrying = hasShellExec || hasEvalFlag || EVAL_EXECS.has(exec); + // sable-c6an. Two questions the code conflated, and conflating them gets one + // of them wrong: + // codeCarrying — this segment RUNS a command string it was handed + // executesOutput — this segment's STDOUT becomes code somewhere else + // (`… | bash`, or a substitution used as a -c script) + // `bash -c "echo 'rm -rf /'"` is the first and not the second, so its operand + // stays data; `bash -c "$(echo rm -rf /)"` is the second, so it is code. + const executesOutput = pipedIntoShell || outputExecuted; + let seenShell = false; let pendingScript = false; let prevTextFlag = false; @@ -352,7 +438,15 @@ function processSegment(pieces: Piece[], depth: number, out: Replacement[]): voi // `bash -c