diff --git a/.github/workflows/test-github-action.yml b/.github/workflows/test-github-action.yml index 7a7665d8..5d524375 100644 --- a/.github/workflows/test-github-action.yml +++ b/.github/workflows/test-github-action.yml @@ -14,10 +14,15 @@ on: - main paths: - 'github-action/**' + - 'action.yml' - '.github/workflows/test-github-action.yml' pull_request: paths: - 'github-action/**' + # The ROOT action.yml is a SECOND action file, and rf-v2mj lived in it. + # Without this line the probe that guards it would never fire on a change + # to it — a check watching a path its subject is not on. + - 'action.yml' - '.github/workflows/test-github-action.yml' workflow_dispatch: @@ -404,6 +409,29 @@ jobs: [ "$FAIL" -eq 0 ] && echo "PASS: counts reached the gate and the gate failed the build." exit $FAIL + # sable-oubg / rf-7xv0 — a fork's branch name reached a shell holding + # RAFTER_API_KEY. Both probes FAIL on the pre-fix file and pass after, and + # both carry controls, so neither can pass by doing nothing. + test-trigger-injection: + name: "Trigger: a branch name cannot reach the key's shell" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Run the injection probe + run: bash github-action/tests/test-trigger-injection.sh + + # sable-oubg / rf-v2mj — the ROOT action.yml derived its finding count with + # `|| echo "0"`, so an unreadable report rendered as a clean scan. Note this + # is the OTHER action.yml: the same defect class in github-action/action.yml + # was fixed under sable-fgk7 and this file was never touched by it. + test-root-action-counts: + name: "Root action: an unreadable report is not a clean scan" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Run the count-derivation probe + run: bash github-action/tests/test-root-action-counts.sh + test-yaml-validity: name: action.yml is valid YAML runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index da672a00..4873307d 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] +## [0.10.6] - 2026-10-02 + +### Security + +- **Unhandled errors no longer print your API key.** A command that failed with an unhandled exception could include the key and other local values in its traceback, printed to the terminal or a CI log. Tracebacks are now printed without local variables. + +- **A project's `.env` can no longer supply Rafter's own settings.** Previously, if the working directory had a `.env` file, its `RAFTER_*` values (including `RAFTER_API_KEY`) were read and could override the key and settings you configured yourself — a repository you merely scanned could supply credentials the CLI would then use. `.env` can no longer set any `RAFTER_*` variable; your own shell environment and the value stored in `~/.rafter/config.json` are unaffected. **Behavior change:** if you were relying on `RAFTER_API_KEY` (or another `RAFTER_*` setting) in a project `.env`, move it to your shell environment or to the config file — the CLI will now report the key as missing if `.env` was its only source. + +- **`rafter secrets --diff ` and `rafter agent scan --diff ` no longer accept a value that looks like a command-line option.** A ref beginning with `-` could previously be misread by git as an option rather than a ref, which could overwrite an unrelated file and report a scan as clean with no secrets found. Such a value is now rejected before it reaches git. + +- **`rafter agent init --local --with-gemini` no longer runs through a shell.** A path containing shell metacharacters could previously have part of it executed as a command during skill registration. Paths are now passed directly to the subprocess, never interpreted by a shell. + +### Fixed + +- **`rafter run` now checks that an auto-detected branch has been pushed before scanning it.** Running `rafter run` without `--branch` from a local branch that doesn't exist on the remote used to queue a scan that failed later with a "branch not found" error. The CLI now checks first and fails immediately with a clear message to push the branch or pass `--branch` explicitly. If the branch exists on the remote but your local commit is ahead of it, the CLI now notes that the scan covers the pushed commit, not your local changes. + ## [0.10.5] - 2026-09-13 ### Security @@ -72,7 +88,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- **Rafter Sites CLI + MCP** (Rome-1/securable-bolt#151). New `rafter sites create|scan|list|get` commands and matching MCP tools (`sites_create`/`sites_scan`/`sites_list`/`sites_get`) for Rafter Sites — live-application security monitoring (exposed backends, DNS misconfig, SEO, accessibility) — calling the new API-key-authenticated `/api/static/sites*` endpoints. `sites scan` accepts either a project id or a URL. Node + Python parity, MCP tools resolve the API key from `RAFTER_API_KEY` or stored config rather than exiting, so a missing key fails the one tool call instead of the whole server. - Live-tested against production before release, which surfaced and fixed: `--format md` now fails with a clear error instead of silently returning JSON (the Sites API has no markdown representation yet); MCP `sites_scan` now rejects being given both `projectId` and `url` instead of silently preferring `projectId`; a double-slash in constructed request URLs; and unreachable per-status default error messages. ## [0.9.1] - 2026-07-21 diff --git a/README.md b/README.md index 05ef874a..6689a1ab 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ Requires Python 3.10+. Full feature parity with Node.js including local security Agentic security audits backed by a full SAST/SCA toolchain, via the Rafter API. The analysis engine examines your codebase the way a professional cybersecurity auditor would — following data flows across files, reasoning about authentication and authorization logic, and identifying vulnerabilities that pattern-matching alone cannot catch — then validates and enriches findings with industry-standard static analysis, dependency scanning, and secret detection. Runs against the **remote repository** on GitHub, not local files. Your code is deleted immediately after analysis completes. Auto-detection uses your local Git config to determine which repo and branch to analyze. ```sh -export RAFTER_API_KEY="your-key" # or use .env file +export RAFTER_API_KEY="your-key" rafter run # scan current repo (auto-detected) rafter scan --repo myorg/myrepo --branch main # scan specific repo @@ -153,7 +153,7 @@ rafter get SCAN_ID > scan_results.json 1. Sign up at [rafter.so](https://rafter.so) 2. Dashboard → Settings → API Keys -3. `export RAFTER_API_KEY="your-key"` or add to `.env` +3. `export RAFTER_API_KEY="your-key"` --- diff --git a/action.yml b/action.yml index f270f51f..9eb9628f 100644 --- a/action.yml +++ b/action.yml @@ -98,10 +98,30 @@ runs: # ({_note, scan_mode, triage_applied, results: [...]}); older versions # emitted a bare array. Handle both so users pinning `version:` to an # older release don't break. + # rf-v2mj — `|| echo "0"` here was the whole bug: jq fails on a truncated + # report, an HTML error page or an auth failure, and the fallback turned + # every one of those into finding-count=0. Zero findings and "I could not + # read the output" became the same value, so the gate passed PRECISELY + # when it could not see its input. An unreadable report is not a clean + # scan; the count is emitted only when a parse actually succeeded. if [ "${{ inputs.format }}" = "json" ]; then - COUNT=$(echo "${OUTPUT}" | jq '[(if type == "array" then . else .results end) | .[]?.matches[]?] | length' 2>/dev/null || echo "0") + if ! COUNT=$(printf '%s' "${OUTPUT}" | jq -e '[(if type == "array" then . else .results end) | .[]?.matches[]?] | length' 2>/dev/null); then + echo "::error::Rafter's output could not be parsed as JSON, so the finding count is unknown." + echo "::error::A report this action cannot read is not a clean scan. Re-run, or check the scanner's stderr above." + # Deliberately NOT written as 0: a consumer reading finding-count + # gets an empty string, never a fabricated zero. + echo "finding-count=" >> "$GITHUB_OUTPUT" + exit 1 + fi else - COUNT=$(echo "${OUTPUT}" | grep -c 'Secret:' 2>/dev/null || echo "0") + # grep exits 1 for "no matches", which is a CLEAN result and must not + # be confused with grep failing (exit >1). + COUNT=$(printf '%s' "${OUTPUT}" | grep -c 'Secret:'); GREP_STATUS=$? + if [ "$GREP_STATUS" -gt 1 ]; then + echo "::error::Could not scan Rafter's output for findings (grep exited ${GREP_STATUS})." + echo "finding-count=" >> "$GITHUB_OUTPUT" + exit 1 + fi fi echo "finding-count=${COUNT}" >> "$GITHUB_OUTPUT" diff --git a/github-action/action.yml b/github-action/action.yml index 8fe5ee3c..9552d0ff 100644 --- a/github-action/action.yml +++ b/github-action/action.yml @@ -68,20 +68,35 @@ runs: RAFTER_API_KEY: ${{ inputs.api-key }} RAFTER_URL: ${{ inputs.rafter-url }} SCAN_MODE: ${{ inputs.scan-mode }} + # rf-7xv0 — these two reach the script through the ENVIRONMENT, never + # through `${{ }}` inside `run:`. The runner expands `${{ }}` into the + # script TEXT before bash sees it, so an interpolated value is not data, + # it is source code — and `github.head_ref` is the branch name chosen by + # whoever opened the pull request. Any stranger can open one from a fork. + # This step's env carries RAFTER_API_KEY, so the branch name was running + # in a shell that could read the key. + GH_REPOSITORY: ${{ github.repository }} + GH_BRANCH: ${{ github.head_ref || github.ref_name }} run: | # --fail-with-body: non-2xx → body printed to stdout AND exit nonzero. # We capture body+status separately so future failures self-explain # (instead of just "curl exit 22"). API key never echoed. BODY_FILE="$(mktemp)" + # Built by jq from environment variables, not by pasting values into a + # quoted string. That fixes two things at once: the shell never sees the + # branch name as code, and a branch name containing a quote or backslash + # produces VALID JSON instead of a malformed body. Hand-quoting would + # have to get both right; jq --arg gets both right by construction. + PAYLOAD=$(jq -nc \ + --arg repository_name "$GH_REPOSITORY" \ + --arg branch_name "$GH_BRANCH" \ + --arg scan_mode "$SCAN_MODE" \ + '{repository_name: $repository_name, branch_name: $branch_name, scan_mode: $scan_mode}') 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 "{ - \"repository_name\": \"${{ github.repository }}\", - \"branch_name\": \"${{ github.head_ref || github.ref_name }}\", - \"scan_mode\": \"${SCAN_MODE}\" - }" \ + -d "$PAYLOAD" \ "${RAFTER_URL}/api/static/scan") || { echo "::error::curl transport error contacting ${RAFTER_URL}/api/static/scan" cat "$BODY_FILE" || true diff --git a/github-action/tests/test-root-action-counts.sh b/github-action/tests/test-root-action-counts.sh new file mode 100755 index 00000000..9f15ae4a --- /dev/null +++ b/github-action/tests/test-root-action-counts.sh @@ -0,0 +1,108 @@ +#!/usr/bin/env bash +# +# rf-v2mj — an unreadable report must not render as a clean scan. +# +# The root action derives `finding-count` from the scanner's stdout. It used to +# end that derivation with `|| echo "0"`, so a truncated report, an HTML error +# page or an auth failure all produced finding-count=0: the value that means +# CLEAN. A consumer gating on it was told "no findings" precisely when the +# action could not see any output at all. +# +# Corpus: the ROOT action.yml. The separate github-action/action.yml had the +# same class of defect in its five severity counts and was fixed under +# sable-fgk7; this file was never touched by that work. Two action.yml files, +# and only one of them had been fixed. +# +# The control is the point: a fix that fails on everything would pass the first +# case and be useless. A genuinely clean report must still succeed with count 0. +set -uo pipefail + +ROOT="$(cd "$(dirname "$0")/../.." && pwd)/action.yml" +TMP="$(mktemp -d)"; trap 'rm -rf "$TMP"' EXIT +failures=0 + +# The scan step, with the runner's `${{ inputs.* }}` expansion applied. +render() { + python3 - "$ROOT" "$1" <<'PY' +import sys, yaml, re +d = yaml.safe_load(open(sys.argv[1])); fmt = sys.argv[2] +for s in d["runs"]["steps"]: + if s.get("id") == "scan": + r = s["run"] + r = re.sub(r"\$\{\{\s*inputs\.format\s*\}\}", fmt, r) + r = re.sub(r"\$\{\{\s*inputs\.(scan-path|args)\s*\}\}", "", r) + r = re.sub(r"\$\{\{.*?\}\}", "", r) + print(r); break +else: + sys.exit("no step id 'scan'") +PY +} + +# A stub `rafter` that emits exactly what we want to test, with a chosen exit code. +stub() { + mkdir -p "$TMP/bin" + { echo '#!/usr/bin/env bash'; echo "cat <<'RAFTER_STUB_EOF'"; printf '%s\n' "$1"; echo 'RAFTER_STUB_EOF'; echo "exit ${2}"; } > "$TMP/bin/rafter" + chmod +x "$TMP/bin/rafter" +} + +run_scan() { + render "$1" > "$TMP/scan.sh" + : > "$TMP/gh_output" + ( cd "$TMP" && PATH="$TMP/bin:$PATH" GITHUB_OUTPUT="$TMP/gh_output" \ + timeout 30 bash "$TMP/scan.sh" >"$TMP/out" 2>&1 ) + echo $? +} +count_written() { sed -n 's/^finding-count=\(.*\)$/\1/p' "$TMP/gh_output" | tail -1; } + +echo "== rf-v2mj: an unreadable report is not a clean scan ==" + +# 1. THE BUG. Unparseable stdout with a success exit code — a truncated report, +# an error page, anything jq cannot read. +stub '502 Bad Gateway' 0 +rc=$(run_scan json); c=$(count_written) +if [ "$rc" -eq 0 ] && [ "$c" = "0" ]; then + echo "FAIL: an unparseable report produced finding-count=0 and exit 0 — a clean scan" + failures=$((failures+1)) +elif [ "$rc" -eq 0 ]; then + echo "FAIL: an unparseable report exited 0 (count written: '${c}')" + failures=$((failures+1)) +else + echo "PASS: an unparseable report fails the step (exit ${rc}, count '${c}')" +fi + +# 2. Truncated JSON — the likeliest real shape, and still not a clean scan. +stub '{"results": [{"matches": [{"rule":' 0 +rc=$(run_scan json); c=$(count_written) +if [ "$rc" -eq 0 ]; then + echo "FAIL: truncated JSON exited 0 (count written: '${c}')" + failures=$((failures+1)) +else + echo "PASS: truncated JSON fails the step (exit ${rc})" +fi + +# 3. THE CONTROL. A genuinely clean report must still succeed, with count 0. +# Without this, "fail on everything" would pass the two cases above. +stub '{"_note":"x","scan_mode":"fast","triage_applied":false,"results":[]}' 0 +rc=$(run_scan json); c=$(count_written) +if [ "$rc" -eq 0 ] && [ "$c" = "0" ]; then + echo "PASS: a genuinely clean report still passes, count 0" +else + echo "FAIL: CONTROL — a clean report no longer passes (exit ${rc}, count '${c}')" + failures=$((failures+1)) +fi + +# 4. Second control: real findings must still be counted, not just tolerated. +stub '{"results":[{"matches":[{"rule":"aws"},{"rule":"gh"}]}]}' 1 +rc=$(run_scan json); c=$(count_written) +if [ "$c" = "2" ]; then + echo "PASS: real findings are counted (2)" +else + echo "FAIL: CONTROL — findings miscounted: got '${c}', expected 2" + failures=$((failures+1)) +fi + +echo "" +echo "── results ──────────────────────────────────────────────" +echo "Failures: $failures" +[ "$failures" -eq 0 ] || exit 1 +echo "OK: the count is emitted only when the report was actually read" diff --git a/github-action/tests/test-trigger-injection.sh b/github-action/tests/test-trigger-injection.sh new file mode 100755 index 00000000..e589a90c --- /dev/null +++ b/github-action/tests/test-trigger-injection.sh @@ -0,0 +1,171 @@ +#!/usr/bin/env bash +# +# rf-7xv0 — a fork's BRANCH NAME must not reach a shell that holds the API key. +# +# `${{ … }}` is expanded by the runner into the script TEXT before bash ever +# sees it, so a value interpolated inside a `run:` block is not data — it is +# source code. The trigger step's env carries RAFTER_API_KEY, and +# `github.head_ref` is chosen by whoever opened the pull request. Anyone can +# open one from a fork. +# +# This probe reproduces the runner's expansion exactly — textual substitution +# into the script, then execute — with a branch name that tries to read the key. +# It asserts two different things, because either alone can pass while the bug +# is live: +# +# 1. INJECTION: the payload must not execute. Canary file must not appear. +# 2. FIDELITY: a hostile-but-legal branch name must still arrive intact in +# the request body. A fix that mangles or drops branch names is +# not a fix, it is a different bug. +# +# Corpus: github-action/action.yml. The ROOT action.yml has no head_ref +# interpolation (checked on main and at v1); rf-v2mj is the defect there. +set -uo pipefail + +ACTION="$(cd "$(dirname "$0")/.." && pwd)/action.yml" +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT +failures=0 + +# The run: block of the trigger step, as the runner would hand it to bash. +extract_trigger() { + python3 - "$ACTION" <<'PY' +import sys, yaml +d = yaml.safe_load(open(sys.argv[1])) +for s in d["runs"]["steps"]: + if s.get("id") == "scan": + print(s["run"]); break +else: + sys.exit("no step with id 'scan'") +PY +} + +# The runner also expands `${{ }}` inside the step's env: VALUES and exports +# them. Modelling only the script text would make this probe vacuous after the +# fix — the payload would simply never arrive anywhere, and the test would pass +# because nothing was tested. Emits `export` lines for the step's env. +step_env() { + python3 - "$ACTION" "$1" <<'PY' +import sys, yaml, re, shlex +d = yaml.safe_load(open(sys.argv[1])); branch = sys.argv[2] +for s in d["runs"]["steps"]: + if s.get("id") == "scan": + for k, v in (s.get("env") or {}).items(): + v = str(v) + v = re.sub(r"\$\{\{\s*github\.head_ref\s*\|\|\s*github\.ref_name\s*\}\}", lambda _: branch, v) + v = re.sub(r"\$\{\{\s*github\.repository\s*\}\}", "owner/repo", v) + v = re.sub(r"\$\{\{\s*inputs\.api-key\s*\}\}", "CANARY-KEY-DO-NOT-LEAK", v) + v = re.sub(r"\$\{\{\s*inputs\.scan-mode\s*\}\}", "fast", v) + v = re.sub(r"\$\{\{\s*inputs\.rafter-url\s*\}\}", "__URL__", v) + v = re.sub(r"\$\{\{.*?\}\}", "", v) + print(f"export {k}={shlex.quote(v)}") + break +PY +} + +# What the runner does: substitute the expression's VALUE into the script text. +expand() { + python3 - "$1" "$2" <<'PY' +import sys, re +script, branch = open(sys.argv[1]).read(), sys.argv[2] +script = re.sub(r"\$\{\{\s*github\.head_ref\s*\|\|\s*github\.ref_name\s*\}\}", + lambda _: branch, script) +script = re.sub(r"\$\{\{\s*github\.repository\s*\}\}", "owner/repo", script) +# Any OTHER ${{ }} left in the script would also be runner-expanded; leaving +# them makes bash choke, which would mask the result rather than test it. +script = re.sub(r"\$\{\{.*?\}\}", "", script) +sys.stdout.write(script) +PY +} + +run_case() { + local label="$1" branch="$2" + extract_trigger > "$TMP/raw.sh" || { echo "FAIL: could not extract the trigger step"; return 1; } + expand "$TMP/raw.sh" "$branch" > "$TMP/run.sh" + rm -f "$TMP/CANARY" "$TMP/body.json" + # No network: point the API at a closed port so curl fails fast. The question + # is never whether the request succeeds — it is whether the payload RAN. + step_env "$branch" | sed "s#__URL__#http://127.0.0.1:9#" > "$TMP/env.sh" + ( cd "$TMP" && \ + RAFTER_API_KEY="CANARY-KEY-DO-NOT-LEAK" \ + RAFTER_URL="http://127.0.0.1:9" \ + SCAN_MODE="fast" \ + GITHUB_OUTPUT="$TMP/gh_output" \ + CANARY_PATH="$TMP/CANARY" \ + timeout 30 bash -c 'set -a; . "$1/env.sh"; set +a; exec bash "$1/run.sh"' _ "$TMP" >"$TMP/out" 2>&1 ) + return 0 +} + +echo "== rf-7xv0: branch name must not execute in the key's shell ==" + +# The payload closes the JSON string and the shell's double quote, then writes +# the key to a file. If the file appears, a stranger could have sent it away. +PAYLOAD='x"; printf %s "$RAFTER_API_KEY" > "$CANARY_PATH"; echo "' +run_case "injection" "$PAYLOAD" +if [ -f "$TMP/CANARY" ]; then + echo "FAIL: INJECTION — a branch name executed and read the API key:" + echo " canary contains: $(cat "$TMP/CANARY")" + failures=$((failures+1)) +else + echo "PASS: a hostile branch name did not execute" +fi + +# Command substitution is the other half: it needs no quote-breaking at all. +run_case "subst" 'x$(printf %s "$RAFTER_API_KEY" > "$CANARY_PATH")' +if [ -f "$TMP/CANARY" ]; then + echo "FAIL: INJECTION via \$( ) — the key was read" + failures=$((failures+1)) +else + echo "PASS: command substitution in a branch name did not execute" +fi + +# Fidelity: a legal branch name with JSON-significant characters must arrive +# INTACT in the request body. Asserted against a real local listener that +# records what was actually sent — an earlier version of this check scraped the +# script text for the old inline-JSON shape, and once the fix replaced that +# shape it silently matched nothing and asserted nothing. +ODD='feature/"quote-and\backslash' +python3 - "$TMP" <<'PY' & +import sys, json +from http.server import BaseHTTPRequestHandler, HTTPServer +tmp = sys.argv[1] +class H(BaseHTTPRequestHandler): + def do_POST(self): + body = self.rfile.read(int(self.headers.get("Content-Length") or 0)) + open(tmp + "/sent.json", "wb").write(body) + self.send_response(200); self.send_header("Content-Length", "22"); self.end_headers() + self.wfile.write(b'{"scan_id":"probe-001"}') + def log_message(self, *a): pass +HTTPServer(("127.0.0.1", 8799), H).serve_forever() +PY +LISTENER=$! +for _ in $(seq 1 40); do curl -s -o /dev/null -X POST -d '{}' http://127.0.0.1:8799/ 2>/dev/null && break; sleep 0.1; done +rm -f "$TMP/sent.json" +extract_trigger > "$TMP/raw.sh" +expand "$TMP/raw.sh" "$ODD" > "$TMP/run.sh" +step_env "$ODD" | sed "s#__URL__#http://127.0.0.1:8799#" > "$TMP/env.sh" +( cd "$TMP" && RAFTER_API_KEY="CANARY-KEY-DO-NOT-LEAK" RAFTER_URL="http://127.0.0.1:8799" \ + SCAN_MODE="fast" GITHUB_OUTPUT="$TMP/gh_output" CANARY_PATH="$TMP/CANARY" \ + timeout 30 bash -c 'set -a; . "$1/env.sh"; set +a; exec bash "$1/run.sh"' _ "$TMP" >"$TMP/out2" 2>&1 ) +kill $LISTENER 2>/dev/null; wait $LISTENER 2>/dev/null + +if [ ! -s "$TMP/sent.json" ]; then + echo "FAIL: FIDELITY — no request body was captured, so this asserted nothing" + failures=$((failures+1)) +else + GOT=$(python3 -c "import json,sys;print(json.load(open(sys.argv[1])).get('branch_name','(missing)'))" "$TMP/sent.json" 2>/dev/null || echo "(unparseable JSON)") + if [ "$GOT" = "$ODD" ]; then + echo "PASS: a branch name with a quote and a backslash arrived intact, as valid JSON" + else + echo "FAIL: FIDELITY — branch name was mangled." + echo " sent: $GOT" + echo " expected: $ODD" + failures=$((failures+1)) + fi +fi + +echo "" +echo "── results ──────────────────────────────────────────────" +echo "Failures: $failures" +[ "$failures" -eq 0 ] || exit 1 +echo "OK: a branch name cannot reach the shell that holds the key" diff --git a/node/package.json b/node/package.json index f8c4ae6b..5a497ebd 100644 --- a/node/package.json +++ b/node/package.json @@ -1,6 +1,6 @@ { "name": "@rafter-security/cli", - "version": "0.10.5", + "version": "0.10.6", "type": "module", "repository": { "type": "git", diff --git a/node/resources/rafter-security-skill.md b/node/resources/rafter-security-skill.md index abef2d91..9328bc1e 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.5 +version: 0.10.6 homepage: https://rafter.so metadata: openclaw: diff --git a/node/src/commands/agent/init.ts b/node/src/commands/agent/init.ts index d19e3414..2873c42e 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, spawnSync } from "child_process"; +import { execFileSync, execSync, spawnSync } from "child_process"; import { fileURLToPath } from "url"; import { createRequire } from "module"; import { askYesNo } from "../../utils/prompt.js"; @@ -1184,10 +1184,15 @@ function registerGeminiSkills(skillsDir: string): void { const absPath = path.resolve(skillsDir, skill.name); if (!fs.existsSync(absPath)) continue; try { - execSync(`gemini skills link ${JSON.stringify(absPath)}`, { - stdio: ["ignore", "pipe", "pipe"], - timeout: 10000, - }); + const execOpts = { stdio: ["ignore", "pipe", "pipe"] as any, timeout: 10000 }; + // No POSIX shell: the path comes from the working directory, whose name + // may hold shell syntax. Windows needs cmd.exe to run gemini's .cmd shim, + // and cmd.exe treats a double-quoted path literally. + if (process.platform === "win32") { + execSync(`gemini skills link ${JSON.stringify(absPath)}`, execOpts); + } else { + execFileSync("gemini", ["skills", "link", absPath], execOpts); + } console.log(fmt.success(`Registered ${skill.name} with Gemini CLI`)); } catch (e: any) { const msg = (e?.stderr?.toString?.() || e?.message || "").trim(); diff --git a/node/src/commands/agent/install-hook.ts b/node/src/commands/agent/install-hook.ts index 90a79803..e99bd914 100644 --- a/node/src/commands/agent/install-hook.ts +++ b/node/src/commands/agent/install-hook.ts @@ -2,7 +2,7 @@ import { Command } from "commander"; import fs from "fs"; import os from "os"; import path from "path"; -import { execSync } from "child_process"; +import { execFileSync, execSync } from "child_process"; import { fileURLToPath } from 'url'; import { fmt } from "../../utils/formatter.js"; @@ -115,7 +115,7 @@ async function installGlobalHook(hookName: string, templateName: string): Promis fs.chmodSync(hookPath, 0o755); try { - execSync(`git config --global core.hooksPath "${globalHooksDir}"`, { stdio: "pipe" }); + execFileSync("git", ["config", "--global", "core.hooksPath", globalHooksDir], { stdio: "pipe" }); console.log(fmt.success(`Installed Rafter ${hookName} hook globally`)); console.log(` Location: ${hookPath}`); console.log(` Git config: core.hooksPath = ${globalHooksDir}`); diff --git a/node/src/commands/agent/scan.ts b/node/src/commands/agent/scan.ts index f18db67e..40232815 100644 --- a/node/src/commands/agent/scan.ts +++ b/node/src/commands/agent/scan.ts @@ -432,6 +432,11 @@ async function scanDiffFiles( scanPath?: string, suppressions: Suppression[] = [], ): Promise { + // git would parse a leading "-" as one of its own options, not a ref. + if (ref.startsWith("-")) { + console.error(`Error: invalid ref "${ref}" (a git ref cannot start with "-")`); + process.exit(2); + } await runGitAddedLineScan( ["diff", "-U0", "--no-color", "--diff-filter=ACM", ref], opts, diff --git a/node/src/commands/agent/status.ts b/node/src/commands/agent/status.ts index 57bf2638..efcfe16d 100644 --- a/node/src/commands/agent/status.ts +++ b/node/src/commands/agent/status.ts @@ -2,7 +2,7 @@ import { Command } from "commander"; import fs from "fs"; import path from "path"; import os from "os"; -import { execSync } from "child_process"; +import { execFileSync, execSync } from "child_process"; import { fileURLToPath } from "url"; import { getRafterDir, getAuditLogPath, getBinDir } from "../../core/config-defaults.js"; import { AuditLogger } from "../../core/audit-logger.js"; @@ -85,7 +85,7 @@ export function createStatusCommand(): Command { } catch { if (fs.existsSync(localBetterleaks)) { try { - const ver = execSync(`"${localBetterleaks}" version`, { timeout: 5000, encoding: "utf-8", stdio: ["pipe", "pipe", "ignore"] }).trim(); + const ver = execFileSync(localBetterleaks, ["version"], { timeout: 5000, encoding: "utf-8", stdio: ["pipe", "pipe", "ignore"] }).trim(); betterleaksStatus = `${ver} (local)`; } catch { betterleaksStatus = `${localBetterleaks} (binary error)`; diff --git a/node/src/commands/backend/run.ts b/node/src/commands/backend/run.ts index db46df9f..e4f15c61 100644 --- a/node/src/commands/backend/run.ts +++ b/node/src/commands/backend/run.ts @@ -1,6 +1,6 @@ import { Command } from "commander"; import ora from "ora"; -import { detectRepo } from "../../utils/git.js"; +import { detectRepo, git, remoteBranchSha } from "../../utils/git.js"; import { API, resolveKey, @@ -96,8 +96,9 @@ export async function runRemoteScan(opts: RunOpts): Promise { const ghToken = opts.githubToken || process.env.RAFTER_GITHUB_TOKEN; let repo: string | undefined, branch: string | undefined; let detectedProvider: string | undefined, detectedRepoUrl: string | undefined; + let localBranch: boolean | undefined; try { - ({ repo, branch, provider: detectedProvider, repo_url: detectedRepoUrl } = detectRepo({ + ({ repo, branch, provider: detectedProvider, repo_url: detectedRepoUrl, local_branch: localBranch } = detectRepo({ repo: opts.repo, branch: opts.branch, quiet: opts.quiet, @@ -111,6 +112,28 @@ export async function runRemoteScan(opts: RunOpts): Promise { process.exit(EXIT_GENERAL_ERROR); } + // The backend clones the remote, so an auto-detected branch that was never + // pushed can only fail there. Say so now instead of queueing that scan. + if (localBranch) { + const remoteSha = remoteBranchSha(branch!); + if (remoteSha === null) { + console.error( + `Branch "${branch}" does not exist on the remote (origin). Rafter scans the remote ` + + "repository: push the branch first, or pass --branch to scan one that exists." + ); + process.exit(EXIT_GENERAL_ERROR); + } + if (remoteSha && !opts.quiet) { + let head: string | undefined; + try { head = git("rev-parse HEAD"); } catch { head = undefined; } + if (head && head !== remoteSha) { + console.error( + `Note: local HEAD differs from origin/${branch}; the scan covers the pushed commit ${remoteSha.slice(0, 7)}.` + ); + } + } + } + // Explicit flags override inferred values. const provider = opts.provider ?? detectedProvider; const repoUrl = opts.repoUrl ?? detectedRepoUrl; diff --git a/node/src/index.ts b/node/src/index.ts index 4e76204a..655b9c55 100644 --- a/node/src/index.ts +++ b/node/src/index.ts @@ -24,7 +24,7 @@ import { checkForUpdate } from "./utils/update-checker.js"; import { setAgentMode } from "./utils/formatter.js"; import { createRequire } from "module"; -// rf-7dda: a repo `.env` must not be able to disable the hook or its timeouts. +// A repo `.env` must not be able to set any RAFTER_* variable (key, token, disables). guardSecurityEnvFromDotenv(() => dotenv.config()); const require = createRequire(import.meta.url); diff --git a/node/src/utils/env-guard.ts b/node/src/utils/env-guard.ts index 27804b16..8847aea8 100644 --- a/node/src/utils/env-guard.ts +++ b/node/src/utils/env-guard.ts @@ -1,22 +1,22 @@ /** - * Security-control env vars must never be settable by a project `.env`. + * A project `.env` must never supply a rafter setting. * * `dotenv.config()` runs at CLI startup (index.ts) and, with no path, loads - * `$CWD/.env` — which, when rafter runs inside an agent hook on a cloned repo, - * is a file IN THE UNTRUSTED REPOSITORY. dotenv does not override a variable - * already present in the real environment, but it DOES introduce one that was - * unset — so a repo shipping `RAFTER_DISABLE_HOOKS=1` (or any `RAFTER_DISABLE_*` - * / `RAFTER_HOOK_*` value) could switch off the victim's command policy and - * secret scanning. That defeats the control whose own contract (hook-control.ts) - * says the disable signal is honored only from the machine owner's environment. + * `$CWD/.env` — which, when rafter runs inside an agent hook or a scan on a + * cloned repo, is a file IN THE UNTRUSTED REPOSITORY. dotenv does not override + * a variable already present in the real environment, but it DOES introduce one + * that was unset. Every `RAFTER_*` variable is an operator setting: the disable + * switches and hook timeouts, but also the API key (which outranks the key the + * operator stored in ~/.rafter/config.json), the GitHub token, the notify + * webhook and the paid-scan confirmation. None of them may come from the repo + * being scanned. * - * This runs dotenv, then drops any `RAFTER_DISABLE_*` / `RAFTER_HOOK_*` variable - * that was NOT already set in the real environment before dotenv ran. The - * owner's real values are preserved untouched; legitimate `.env` keys that do - * not match those prefixes (RAFTER_API_KEY, RAFTER_GITHUB_TOKEN, …) are - * unaffected. rf-7dda / sable-nz4y sibling. + * This runs dotenv, then drops any `RAFTER_*` variable that was NOT already set + * in the real environment before dotenv ran. The owner's real values are + * preserved untouched. This matches the Python runtime, which never reads the + * working directory's `.env`. */ -const PROTECTED_PREFIX = /^RAFTER_(DISABLE_|HOOK_)/; +const PROTECTED_PREFIX = /^RAFTER_/; export function guardSecurityEnvFromDotenv( applyDotenv: () => void, diff --git a/node/src/utils/git.ts b/node/src/utils/git.ts index 99800b95..d8446ffe 100644 --- a/node/src/utils/git.ts +++ b/node/src/utils/git.ts @@ -1,4 +1,4 @@ -import { execSync } from "child_process"; +import { execFileSync, execSync } from "child_process"; export function git(cmd: string): string { return execSync(`git ${cmd}`, { stdio: ["ignore", "pipe", "ignore"] }) @@ -6,50 +6,160 @@ export function git(cmd: string): string { .trim(); } +/** + * Look up `branch` on the `origin` remote. + * + * Returns the remote commit SHA, `null` when the remote answered and has no + * such branch, or `undefined` when it could not be asked (offline, auth + * failure, timeout). Callers treat `undefined` as unknown and carry on. + */ +export function remoteBranchSha(branch: string, cwd?: string): string | null | undefined { + try { + const out = execFileSync( + "git", + ["ls-remote", "--exit-code", "--heads", "origin", `refs/heads/${branch}`], + { + cwd, + stdio: ["ignore", "pipe", "ignore"], + timeout: 15_000, + env: { ...process.env, GIT_TERMINAL_PROMPT: "0" }, + } + ).toString(); + return out.split(/\s+/)[0] || undefined; + } catch (e: any) { + // --exit-code: status 2 means the remote has no matching ref. + return e?.status === 2 ? null : undefined; + } +} + +/** + * Return the current branch name. + * + * Throws on a detached HEAD (or when there is no HEAD at all, e.g. an + * empty repo) instead of falling back to a commit SHA or a hardcoded + * default branch. Neither is a real branch: a SHA is guaranteed to 404 as + * a "branch" on the backend, and a hardcoded default is a guess that is + * often wrong and, even when right, doesn't reflect what is actually + * checked out. + */ export function safeBranch(gitFn: (c: string) => string): string { try { return gitFn("symbolic-ref --quiet --short HEAD"); } catch { - return gitFn("rev-parse --short HEAD"); + throw new Error( + "Could not determine the current branch (detached HEAD or no commits yet). " + + "Please pass --branch explicitly." + ); } } -export function parseRemote(url: string): string { - url = url.replace(/^(https?:\/\/|git@)/, "").replace(":", "/"); - if (url.endsWith(".git")) url = url.slice(0, -4); - const parts = url.split("/"); - return parts.slice(-2).join("/"); // owner/repo -} - export type Provider = "github" | "gitlab" | "gitea" | "bitbucket"; +const SCHEME_RE = /^(https?|ssh):\/\//i; + /** - * Map a git remote host to a provider. `github` is the backward-compatible - * default for any host we don't recognize — a GitHub user's request is - * unaffected, and unknown self-hosted hosts fall back to the legacy behavior. + * Split a git remote URL into its host + owner/repo slug, handling + * `https://[user[:token]@]host[:port]/owner/repo(.git)` (and http, ssh), + * and the scp-like `[user@]host:owner/repo(.git)`. Returns null when the + * URL can't be parsed into host + slug. + * + * Uses the URL parser (not a blanket ":" -> "/" substitution) so a colon + * inside userinfo — `https://user:token@host/...`, a real shape for + * CI-embedded credentials — is never mistaken for the scp host:path + * separator. Getting this wrong is a security bug, not just a parsing + * one: the naive substitution let `https://github.com:x@evil.com/a/b` + * read as host `github.com` (an allowed host) with the real host, + * evil.com, silently discarded. */ -export function providerForHost(host: string): Provider { +function splitRemote(url: string): { host: string; slug: string } | null { + let rest: string; + if (SCHEME_RE.test(url)) { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + return null; + } + if (!parsed.hostname) return null; + rest = `${parsed.hostname}${parsed.pathname}`; + } else if (url.includes(":")) { + // scp-like: "[user@]host:owner/repo(.git)". The user (if any) is + // whatever precedes the LAST "@" before this colon. + const colonIdx = url.indexOf(":"); + const head = url.slice(0, colonIdx); + const path = url.slice(colonIdx + 1); + const atIdx = head.lastIndexOf("@"); + const host = atIdx === -1 ? head : head.slice(atIdx + 1); + if (!host || host.includes("/")) return null; + rest = `${host}/${path}`; + } else { + // No scheme, no ":" -- e.g. a bare filesystem path. Treated opaquely: + // the leading segment stands in for "host" below, so it is rejected + // unless it happens to equal a real host (it never will for a real + // filesystem path). + rest = url; + } + + if (rest.endsWith(".git")) rest = rest.slice(0, -4); + const parts = rest.split("/").filter((p) => p.length > 0); + if (parts.length < 3) return null; // need host + owner + repo + const host = parts[0]; + const slug = parts.slice(-2).join("/"); + return { host, slug }; +} + +/** + * Map a git remote host to a provider we actually recognize. Unlike + * providerForHost, returns null for a host we don't recognize instead of + * defaulting to "github" — used where guessing is not safe. + */ +function knownProviderForHost(host: string): Provider | null { host = host.toLowerCase(); if (host === "github.com") return "github"; if (host === "gitlab.com" || host.endsWith(".gitlab.com")) return "gitlab"; if (host === "bitbucket.org") return "bitbucket"; if (host === "codeberg.org" || host.endsWith(".gitea.io")) return "gitea"; - return "github"; // backward-compatible default + return null; } /** - * Split a git remote URL into its host + owner/repo slug, handling both - * `git@host:owner/repo(.git)` (scp-like) and `https://host/owner/repo(.git)`. - * Returns null when the URL can't be parsed into host + slug. + * Parse a git remote URL into "owner/repo" format. + * + * Throws when the remote's host isn't one we recognize. Blindly slicing + * the last two path segments of an arbitrary URL (the old behavior) + * manufactures a wrong slug for anything that isn't GitHub/GitLab/ + * Bitbucket/Gitea shaped — e.g. an Azure DevOps remote + * (`.../org/proj/_git/repo`) becomes `_git/repo`, and a bare filesystem + * remote becomes `/`. The backend turns that slug into + * an invalid clone URL and 404s, burning a paid scan. */ -function splitRemote(url: string): { host: string; slug: string } | null { - let rest = url.replace(/^(https?:\/\/|git@)/, "").replace(":", "/"); - if (rest.endsWith(".git")) rest = rest.slice(0, -4); - const parts = rest.split("/").filter((p) => p.length > 0); - if (parts.length < 3) return null; // need host + owner + repo - const host = parts[0]; - const slug = parts.slice(-2).join("/"); - return { host, slug }; +export function parseRemote(url: string): string { + const parts = splitRemote(url); + if (!parts) { + throw new Error( + `Could not determine owner/repo from git remote "${url}". ` + + "Please pass --repo and --branch explicitly." + ); + } + if (!knownProviderForHost(parts.host)) { + throw new Error( + `Unsupported git remote host "${parts.host}" (from "${url}"). ` + + "Only GitHub, GitLab, Bitbucket, and Gitea remotes are auto-detected. " + + "Please pass --repo and --branch explicitly." + ); + } + return parts.slug; // owner/repo +} + +/** + * Map a git remote host to a provider. `github` is the backward-compatible + * default for any host we don't recognize — a GitHub user's request is + * unaffected, and unknown self-hosted hosts fall back to the legacy behavior. + * (Only used for the additive provider/repoUrl fields; parseRemote uses the + * stricter knownProviderForHost and rejects what this would silently default.) + */ +export function providerForHost(host: string): Provider { + return knownProviderForHost(host) ?? "github"; } /** @@ -71,8 +181,13 @@ export interface DetectedRepo { branch?: string; provider?: Provider; repo_url?: string; + /** Both repo and branch came from the local checkout (origin + HEAD). */ + local_branch?: boolean; } +const AUTO_DETECT_FAILURE = + "Could not auto-detect Git repository. Please pass --repo and --branch explicitly."; + export function detectRepo(opts: { repo?: string; branch?: string; quiet?: boolean }): DetectedRepo { // Both explicit — return them as-is. No provider/repo_url is inferred here; // the caller's --provider/--repo-url flags fill that in when needed. @@ -83,35 +198,48 @@ export function detectRepo(opts: { repo?: string; branch?: string; quiet?: boole let branch = opts.branch || branchEnv; let provider: Provider | undefined; let repoUrl: string | undefined; - try { - if (!repoSlug || !branch) { - if (git("rev-parse --is-inside-work-tree") !== "true") - throw new Error("not a repo"); - // Read the remote once when we need to detect the slug, and reuse it to - // infer the provider + canonical clone URL. When repo/branch come from - // env (CI), we never touch git here — behavior is byte-identical. - if (!repoSlug) { - const remoteUrl = git("remote get-url origin"); - repoSlug = parseRemote(remoteUrl); - const inferred = inferRemote(remoteUrl); - provider = inferred.provider; - repoUrl = inferred.repoUrl; - } - if (!branch) { - try { - branch = safeBranch(git); - } catch { - branch = "main"; - } - } - } - if ((!opts.repo || !opts.branch) && !opts.quiet) { - console.error(`Repo auto-detected: ${repoSlug} @ ${branch} (note: scanning remote)`); - } + + if (repoSlug && branch) { return { repo: repoSlug, branch, provider, repo_url: repoUrl }; + } + + let insideRepo: boolean; + try { + insideRepo = git("rev-parse --is-inside-work-tree") === "true"; } catch { - throw new Error( - "Could not auto-detect Git repository. Please pass --repo and --branch explicitly." - ); + insideRepo = false; + } + if (!insideRepo) { + throw new Error(AUTO_DETECT_FAILURE); + } + + // Read the remote once when we need to detect the slug, and reuse it to + // infer the provider + canonical clone URL. When repo/branch come from + // env (CI), we never touch git here — behavior is byte-identical. + // A rejection from parseRemote (unrecognized host) is deliberately NOT + // swallowed into the generic message below — it names the offending + // remote, which is the actionable part. + const repoFromOrigin = !repoSlug; + if (!repoSlug) { + let remoteUrl: string; + try { + remoteUrl = git("remote get-url origin"); + } catch { + throw new Error(AUTO_DETECT_FAILURE); + } + repoSlug = parseRemote(remoteUrl); + const inferred = inferRemote(remoteUrl); + provider = inferred.provider; + repoUrl = inferred.repoUrl; + } + + const localBranch = repoFromOrigin && !branch; + if (!branch) { + branch = safeBranch(git); + } + + if ((!opts.repo || !opts.branch) && !opts.quiet) { + console.error(`Repo auto-detected: ${repoSlug} @ ${branch} (note: scanning remote)`); } + return { repo: repoSlug, branch, provider, repo_url: repoUrl, local_branch: localBranch }; } diff --git a/node/tests/agent-init-gemini-link.test.ts b/node/tests/agent-init-gemini-link.test.ts new file mode 100644 index 00000000..1952f9e9 --- /dev/null +++ b/node/tests/agent-init-gemini-link.test.ts @@ -0,0 +1,56 @@ +/** + * `agent init --local --with-gemini` registers skills with `gemini skills link + * `, where the path is under the working directory. A directory name may + * hold shell syntax, so the path must reach gemini as one literal argument and + * never be interpreted by a shell. + * + * Runs the built CLI with a stub `gemini` on PATH that logs its arguments. + */ +import { describe, it, expect, beforeAll } from "vitest"; +import { execSync, spawnSync } from "child_process"; +import fs from "fs"; +import path from "path"; +import os from "os"; + +const PROJECT_ROOT = path.resolve(__dirname, ".."); +const CLI_DIST = path.join(PROJECT_ROOT, "dist", "index.js"); + +beforeAll(() => { + if (!fs.existsSync(CLI_DIST)) { + execSync("pnpm run build", { cwd: PROJECT_ROOT, stdio: "inherit" }); + } +}); + +describe.skipIf(process.platform === "win32")("agent init --local --with-gemini", () => { + it("passes a working-directory path with shell syntax to gemini literally", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "rafter-gemini-link-")); + try { + const bin = path.join(root, "bin"); + const log = path.join(root, "calls.log"); + fs.mkdirSync(bin); + fs.writeFileSync( + path.join(bin, "gemini"), + `#!/bin/sh\nfor a in "$@"; do printf '%s\\n' "$a" >> '${log}'; done\nexit 0\n`, + { mode: 0o755 }, + ); + const home = path.join(root, "home"); + fs.mkdirSync(home); + const project = path.join(root, "vendor", "$(touch MARKER)"); + fs.mkdirSync(project, { recursive: true }); + + const r = spawnSync(process.execPath, [CLI_DIST, "agent", "init", "--local", "--with-gemini"], { + cwd: project, + encoding: "utf-8", + timeout: 60_000, + env: { ...process.env, HOME: home, XDG_CONFIG_HOME: path.join(home, ".config"), PATH: `${bin}:${process.env.PATH}`, CI: "1" }, + }); + + expect(r.status).toBe(0); + expect(fs.existsSync(path.join(project, "MARKER"))).toBe(false); + const args = fs.readFileSync(log, "utf-8").split("\n"); + expect(args).toContain(path.join(project, ".agents", "skills", "rafter")); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/node/tests/e2e-cli.test.ts b/node/tests/e2e-cli.test.ts index 44f4919b..7a356746 100644 --- a/node/tests/e2e-cli.test.ts +++ b/node/tests/e2e-cli.test.ts @@ -380,17 +380,16 @@ describe("CLI e2e — dotenv loading", () => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); - it("loads RAFTER_API_KEY from .env file in cwd", () => { - // Write a .env file with a fake API key + it("ignores RAFTER_API_KEY from a .env file in cwd", () => { + // The working directory may be an untrusted cloned repo: its .env must not + // supply the operator's credential. With no key anywhere else, the CLI + // must report the key as missing rather than use the repo's. fs.writeFileSync(path.join(tmpDir, ".env"), "RAFTER_API_KEY=test-key-from-dotenv\n"); - // Run from tmpDir WITHOUT setting RAFTER_API_KEY in env — let .env provide it. - // The usage command will attempt to call the API (and fail), but it should NOT - // complain about a missing API key since .env provides one. - const envWithoutKey = { ...process.env }; + const envWithoutKey = { ...process.env, HOME: tmpDir, USERPROFILE: tmpDir }; delete envWithoutKey.RAFTER_API_KEY; const r = rafter("usage", { cwd: tmpDir, env: envWithoutKey as Record }); const combined = (r.stdout + r.stderr).toLowerCase(); - expect(combined).not.toContain("no api key"); + expect(combined).toContain("no api key"); }, 30000); }); diff --git a/node/tests/env-guard.test.ts b/node/tests/env-guard.test.ts index 9294184a..b0b8280c 100644 --- a/node/tests/env-guard.test.ts +++ b/node/tests/env-guard.test.ts @@ -29,14 +29,23 @@ describe("guardSecurityEnvFromDotenv (rf-7dda)", () => { expect(hookEnabled(env)).toBe(false); }); - it("preserves a legitimate non-security .env key (RAFTER_API_KEY)", () => { - const env: any = {}; + it("drops repo-.env credentials and approvals; the operator's real values survive", () => { + const env: any = { RAFTER_GITHUB_TOKEN: "ghp-operator" }; guardSecurityEnvFromDotenv( - () => applyDotenv({ RAFTER_API_KEY: "sk-legit", RAFTER_DISABLE_HOOKS: "1" }, env), + () => applyDotenv({ + RAFTER_API_KEY: "repo-key", + RAFTER_GITHUB_TOKEN: "ghp-repo", + RAFTER_CONFIRM: "1", + RAFTER_NOTIFY_WEBHOOK: "https://example.invalid/hook", + UNRELATED: "kept", + }, env), env, ); - expect(env.RAFTER_API_KEY).toBe("sk-legit"); - expect(hookEnabled(env)).toBe(true); + expect(env.RAFTER_API_KEY).toBeUndefined(); + expect(env.RAFTER_CONFIRM).toBeUndefined(); + expect(env.RAFTER_NOTIFY_WEBHOOK).toBeUndefined(); + expect(env.RAFTER_GITHUB_TOKEN).toBe("ghp-operator"); + expect(env.UNRELATED).toBe("kept"); }); it("drops the fail-open RAFTER_HOOK_STDIN_TIMEOUT_MS and every sub-part disable", () => { diff --git a/node/tests/git-utils.test.ts b/node/tests/git-utils.test.ts index 5bf1ef69..fede21f6 100644 --- a/node/tests/git-utils.test.ts +++ b/node/tests/git-utils.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { execSync } from "child_process"; import { parseRemote, safeBranch, detectRepo, providerForHost, inferRemote } from "../src/utils/git.js"; +vi.mock("child_process"); +const mockedExecSync = vi.mocked(execSync); + // ── parseRemote (pure function) ──────────────────────────────────── describe("parseRemote", () => { @@ -29,6 +33,66 @@ describe("parseRemote", () => { }); }); +// sable-pqmw: parseRemote used to slice the last two path segments of ANY +// remote with no host check at all, so a non-GitHub-shaped remote silently +// produced a wrong slug (e.g. Azure DevOps's `.../_git/repo` becomes +// `_git/repo`; a filesystem remote becomes `/`). The +// backend turns that slug into `https://github.com/{slug}` and 404s, +// burning a paid scan. It must now reject anything it can't recognize. +describe("parseRemote host validation", () => { + it.each([ + ["github https", "https://github.com/owner/repo", "owner/repo"], + ["github ssh", "git@github.com:owner/repo.git", "owner/repo"], + ["github https with .git", "https://github.com/owner/repo.git", "owner/repo"], + // GitLab stays supported -- separate multi-provider feature (sable-w79q) + // that already sends provider + repo_url alongside. + ["gitlab ssh", "git@gitlab.com:group/project.git", "group/project"], + ])("%s still parses", (_label, url, expected) => { + expect(parseRemote(url)).toBe(expected); + }); + + it.each([ + // Azure DevOps: naive last-two-segments yields "_git/repo". + ["azure devops", "https://dev.azure.com/my-org/my-proj/_git/my-repo"], + // Bare filesystem remote: naive last-two-segments yields + // "/" -- the "local/*" class seen in production. + ["bare filesystem path", "/home/ci/local/my-repo"], + ])("%s is rejected", (_label, url) => { + expect(() => parseRemote(url)).toThrow(/unsupported/i); + }); + + it("names the offending host in the error", () => { + expect(() => + parseRemote("https://dev.azure.com/my-org/my-proj/_git/my-repo") + ).toThrow(/dev\.azure\.com/); + }); + + // Found in security review of this fix: a naive "replace : with /" + // treats the userinfo separator the same as the SCP host:path + // separator, so `host` (the value checked against the allowlist) can be + // attacker-chosen credentials rather than the real host -- and + // legitimate credentialed remotes (PAT-embedded HTTPS, common in CI) + // hard-fail the same way. + it.each([ + // CI token-embedded remotes -- real shapes, must keep working. + ["github https with embedded token", "https://x-access-token:ghp_abc123@github.com/owner/repo.git", "owner/repo"], + ["gitlab https with embedded CI token", "https://gitlab-ci-token:glcbt-abc@gitlab.com/group/project.git", "group/project"], + // Explicit ssh:// scheme -- a normal, non-adversarial clone form. + ["explicit ssh:// scheme", "ssh://git@github.com/owner/repo.git", "owner/repo"], + ["explicit ssh:// scheme with port", "ssh://git@github.com:2222/owner/repo.git", "owner/repo"], + ])("%s still parses", (_label, url, expected) => { + expect(parseRemote(url)).toBe(expected); + }); + + it("does not let userinfo smuggle an unrecognized host past the check", () => { + // The real host is evil.com; "github.com" only appears as userinfo. + // Must be rejected (as evil.com), never accepted as github.com. + expect(() => + parseRemote("https://github.com:x@evil.com/foo/bar.git") + ).toThrow(/evil\.com/); + }); +}); + // ── providerForHost (host → provider inference) ──────────────────── describe("providerForHost", () => { @@ -145,13 +209,27 @@ describe("safeBranch", () => { expect(gitFn).toHaveBeenCalledWith("symbolic-ref --quiet --short HEAD"); }); - it("falls back to rev-parse on detached HEAD", () => { + // sable-pqmw: a detached HEAD must not submit a commit SHA as a branch + // name -- it is not a branch and is guaranteed to 404 on the backend. + // The old behavior fell back to `rev-parse --short HEAD`; assert that + // even when a SHA IS available, it is never returned, and rev-parse is + // never even attempted. + it("throws on detached HEAD even when a SHA is available", () => { const gitFn = vi.fn() .mockImplementationOnce(() => { throw new Error("not on a branch"); }) .mockReturnValueOnce("abc1234"); - expect(safeBranch(gitFn)).toBe("abc1234"); - expect(gitFn).toHaveBeenCalledTimes(2); - expect(gitFn).toHaveBeenNthCalledWith(2, "rev-parse --short HEAD"); + expect(() => safeBranch(gitFn)).toThrow(/branch/i); + expect(gitFn).toHaveBeenCalledTimes(1); + }); + + // sable-pqmw: total git failure (e.g. an empty repo with no commits) + // must not fall back to a hardcoded "main" -- that guesses the default + // branch and is often wrong, and is misleading even when it isn't. + it("throws rather than inventing a default branch on total failure", () => { + const gitFn = vi.fn().mockImplementation(() => { + throw new Error("fatal: not a git repository"); + }); + expect(() => safeBranch(gitFn)).toThrow(/branch/i); }); }); @@ -230,4 +308,29 @@ describe("detectRepo", () => { const result = detectRepo({}); expect(result).toEqual({ repo: "org/repo", branch: "gh-branch" }); }); + + // sable-pqmw: an unrecognized-host remote (e.g. Azure DevOps) must + // surface as a clear, catchable error through the full detection path, + // not a silently wrong repository slug. + describe("with a real git remote (host validation)", () => { + beforeEach(() => { + mockedExecSync.mockReset(); + mockedExecSync.mockImplementation((cmd: unknown) => { + const c = String(cmd); + if (c.includes("rev-parse --is-inside-work-tree")) return "true"; + if (c.includes("remote get-url origin")) { + return "https://dev.azure.com/my-org/my-proj/_git/my-repo"; + } + throw new Error(`unexpected git invocation in test: ${c}`); + }); + }); + + afterEach(() => { + mockedExecSync.mockReset(); + }); + + it("throws naming the host for an unrecognized remote", () => { + expect(() => detectRepo({})).toThrow(/dev\.azure\.com/); + }); + }); }); diff --git a/node/tests/remote-branch.test.ts b/node/tests/remote-branch.test.ts new file mode 100644 index 00000000..c9fea127 --- /dev/null +++ b/node/tests/remote-branch.test.ts @@ -0,0 +1,36 @@ +import { describe, it, expect } from "vitest"; +import { execFileSync } from "child_process"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { remoteBranchSha } from "../src/utils/git.js"; + +// `rafter run` scans the remote, so it must tell an unpushed local branch +// (remote answers, branch absent) apart from a pushed one and from a remote +// it cannot reach. Real git against a local bare origin, no network. +describe("remoteBranchSha", () => { + it("returns the SHA for a pushed branch, null for an unpushed one, undefined when origin is unreachable", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "rafter-remote-branch-")); + try { + const g = (cwd: string, ...args: string[]) => + execFileSync("git", args, { cwd, stdio: ["ignore", "pipe", "ignore"] }).toString().trim(); + const bare = path.join(root, "origin.git"); + const work = path.join(root, "work"); + fs.mkdirSync(work); + g(root, "init", "-q", "--bare", bare); + g(work, "init", "-q", "-b", "main"); + g(work, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "--allow-empty", "-m", "init"); + g(work, "remote", "add", "origin", bare); + g(work, "push", "-q", "origin", "main"); + g(work, "checkout", "-q", "-b", "task/unpushed"); + + expect(remoteBranchSha("main", work)).toBe(g(work, "rev-parse", "main")); + expect(remoteBranchSha("task/unpushed", work)).toBeNull(); + + g(work, "remote", "set-url", "origin", path.join(root, "missing.git")); + expect(remoteBranchSha("main", work)).toBeUndefined(); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/node/tests/secret-scanning-e2e.test.ts b/node/tests/secret-scanning-e2e.test.ts index 630dd0b9..6d11b759 100644 --- a/node/tests/secret-scanning-e2e.test.ts +++ b/node/tests/secret-scanning-e2e.test.ts @@ -489,6 +489,18 @@ describe("E2E: git --diff scanning", () => { expect(parsed.results[0].matches[0].pattern.name).toBe("AWS Access Key ID"); }); + it("rejects a --diff ref that git would parse as an option", () => { + const target = path.join(tmpDir, "untouched.txt"); + fs.writeFileSync(target, "keep\n"); + + const r = rafter( + ["scan", "local", tmpDir, "--diff", `--output=${target}`, "--engine", "patterns", "--quiet"], + { cwd: tmpDir }, + ); + expect(r.exitCode).toBe(2); + expect(fs.readFileSync(target, "utf-8")).toBe("keep\n"); + }); + it("exits 0 when changed files are clean", () => { const initialCommit = git("rev-parse HEAD"); diff --git a/python/README.md b/python/README.md index a0d86cc1..fc32a4a1 100644 --- a/python/README.md +++ b/python/README.md @@ -21,7 +21,7 @@ Requires Python 3.10+. ### Remote Code Analysis ```bash -export RAFTER_API_KEY="your-key" # or add to .env file +export RAFTER_API_KEY="your-key" rafter run # scan current repo (auto-detected) rafter scan --repo myorg/myrepo --branch main # scan specific repo @@ -96,8 +96,8 @@ Alias: `rafter scan` Trigger a new security scan for your repository. -- `-r, --repo ` — org/repo (default: auto-detected from git remote) -- `-b, --branch ` — branch (default: current branch or 'main') +- `-r, --repo ` — org/repo (default: auto-detected from git remote; errors on an unrecognized remote host) +- `-b, --branch ` — branch (default: current branch; errors on a detached HEAD) - `-k, --api-key ` — API key (or `RAFTER_API_KEY` env var) - `-f, --format ` — `json` or `md` (default: `md`) - `--skip-interactive` — don't wait for scan completion diff --git a/python/pyproject.toml b/python/pyproject.toml index 3395eaff..211a54fe 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "rafter-cli" -version = "0.10.5" +version = "0.10.6" description = "Rafter CLI — the default security agent for AI workflows. Free for individuals and open source." authors = ["Rafter Team "] license = "MIT" diff --git a/python/rafter_cli/__main__.py b/python/rafter_cli/__main__.py index fc1ae807..ed79863c 100644 --- a/python/rafter_cli/__main__.py +++ b/python/rafter_cli/__main__.py @@ -25,6 +25,8 @@ help="Rafter CLI — the default security agent for AI workflows. Free for individuals and open source. No account required.", add_completion=True, no_args_is_help=True, + # Never render frame locals in tracebacks: they hold the API key. + pretty_exceptions_show_locals=False, ) diff --git a/python/rafter_cli/commands/agent.py b/python/rafter_cli/commands/agent.py index 58acacdd..38fcf686 100644 --- a/python/rafter_cli/commands/agent.py +++ b/python/rafter_cli/commands/agent.py @@ -1736,6 +1736,13 @@ def _output_empty_diff_scan( ) +def _reject_option_like_ref(ref: str) -> None: + """git would parse a leading "-" as one of its own options, not a ref.""" + if ref.startswith("-"): + print(f'Error: invalid ref "{ref}" (a git ref cannot start with "-")', file=sys.stderr) + raise typer.Exit(code=2) + + def _run_git_added_line_scan( git_args: list[str], git_cwd: str | None, @@ -2176,6 +2183,7 @@ def scan( # --diff if diff: + _reject_option_like_ref(diff) _run_git_added_line_scan( ["diff", "-U0", "--no-color", "--diff-filter=ACM", diff], git_cwd, diff --git a/python/rafter_cli/commands/backend.py b/python/rafter_cli/commands/backend.py index 90ead2f4..198dd8a7 100644 --- a/python/rafter_cli/commands/backend.py +++ b/python/rafter_cli/commands/backend.py @@ -24,7 +24,7 @@ resolve_key, write_payload, ) -from ..utils.git import detect_repo +from ..utils.git import _run, branch_from_env, detect_repo, remote_branch_sha def _plus_approval_gate_enabled() -> bool: @@ -408,6 +408,30 @@ def _do_remote_scan( if not (repo and branch) and not quiet: print(f"Repo auto-detected: {repo_slug} @ {branch_name} (note: scanning remote)", file=sys.stderr) + # The backend clones the remote, so an auto-detected branch that was never + # pushed can only fail there. Say so now instead of queueing that scan. + # A repo_url is inferred only when the slug came from the origin remote. + if detected_repo_url and not branch and not branch_from_env(): + remote_sha = remote_branch_sha(branch_name) + if remote_sha is None: + print( + f'Branch "{branch_name}" does not exist on the remote (origin). Rafter scans ' + "the remote repository: push the branch first, or pass --branch to scan one that exists.", + file=sys.stderr, + ) + raise typer.Exit(code=EXIT_GENERAL_ERROR) + if remote_sha and not quiet: + try: + head = _run(["git", "rev-parse", "HEAD"]) + except Exception: + head = None + if head and head != remote_sha: + print( + f"Note: local HEAD differs from origin/{branch_name}; " + f"the scan covers the pushed commit {remote_sha[:7]}.", + file=sys.stderr, + ) + # Explicit flags override inferred values. resolved_provider = provider or detected_provider resolved_repo_url = repo_url or detected_repo_url diff --git a/python/rafter_cli/commands/issues/issues_app.py b/python/rafter_cli/commands/issues/issues_app.py index 2e74bc2a..71525bb7 100644 --- a/python/rafter_cli/commands/issues/issues_app.py +++ b/python/rafter_cli/commands/issues/issues_app.py @@ -13,6 +13,7 @@ import requests import typer +from rich.markup import escape from ...utils.api import api_url, api_get, EXIT_GENERAL_ERROR, resolve_key from ...utils.formatter import fmt, print_stderr @@ -66,7 +67,11 @@ def from_scan( try: target_repo, _, _, _ = detect_repo(repo) except RuntimeError as e: - print_stderr(fmt.error(str(e))) + # e can embed a remote URL/host (sable-pqmw); escape it before + # it reaches Rich's markup parser, or a value containing e.g. + # "[/bold]" crashes with an uncaught MarkupError instead of a + # clean error + exit code. + print_stderr(fmt.error(escape(str(e)))) raise typer.Exit(code=EXIT_GENERAL_ERROR) if not quiet: @@ -173,7 +178,11 @@ def from_text( try: target_repo, _, _, _ = detect_repo(repo) except RuntimeError as e: - print_stderr(fmt.error(str(e))) + # e can embed a remote URL/host (sable-pqmw); escape it before + # it reaches Rich's markup parser, or a value containing e.g. + # "[/bold]" crashes with an uncaught MarkupError instead of a + # clean error + exit code. + print_stderr(fmt.error(escape(str(e)))) raise typer.Exit(code=EXIT_GENERAL_ERROR) # Parse text diff --git a/python/rafter_cli/commands/scan.py b/python/rafter_cli/commands/scan.py index e4365872..eb9dfdee 100644 --- a/python/rafter_cli/commands/scan.py +++ b/python/rafter_cli/commands/scan.py @@ -117,6 +117,7 @@ def scan_local( _apply_exclude_paths, _load_baseline_entries, _run_git_added_line_scan, + _reject_option_like_ref, ) from ..core.config_manager import ConfigManager from ..core.custom_patterns import load_suppressions, policy_ignore_to_suppressions @@ -146,6 +147,7 @@ def scan_local( # --diff if diff: + _reject_option_like_ref(diff) _run_git_added_line_scan( ["diff", "-U0", "--no-color", "--diff-filter=ACM", diff], git_cwd, diff --git a/python/rafter_cli/resources/rafter-security-skill.md b/python/rafter_cli/resources/rafter-security-skill.md index abef2d91..9328bc1e 100644 --- a/python/rafter_cli/resources/rafter-security-skill.md +++ b/python/rafter_cli/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.5 +version: 0.10.6 homepage: https://rafter.so metadata: openclaw: diff --git a/python/rafter_cli/utils/api.py b/python/rafter_cli/utils/api.py index ee4ccb36..6d4e5fcb 100644 --- a/python/rafter_cli/utils/api.py +++ b/python/rafter_cli/utils/api.py @@ -7,7 +7,6 @@ import requests import typer -from dotenv import load_dotenv API_BASE = "https://rafter.so/api/" @@ -133,7 +132,9 @@ def resolve_key(cli_opt: str | None) -> str: """Resolve API key: --api-key flag > RAFTER_API_KEY env > global config.""" if cli_opt: return cli_opt - load_dotenv() + # No .env loading here: python-dotenv searches upward from this package's + # install path, which for a virtualenv inside a cloned repo reaches the + # repo's own .env. A repo must never supply the operator's credential. env_key = os.getenv("RAFTER_API_KEY") if env_key: return env_key diff --git a/python/rafter_cli/utils/git.py b/python/rafter_cli/utils/git.py index 4d7998d3..f1b91e1e 100644 --- a/python/rafter_cli/utils/git.py +++ b/python/rafter_cli/utils/git.py @@ -3,6 +3,7 @@ import re import subprocess +from urllib.parse import urlsplit def _run(cmd: list[str]) -> str: @@ -26,33 +27,117 @@ def is_inside_repo() -> bool: return False +def remote_branch_sha(branch: str, cwd: str | None = None) -> str | None | bool: + """Look up ``branch`` on the ``origin`` remote. + + Returns the remote commit SHA, ``None`` when the remote answered and has + no such branch, or ``False`` when it could not be asked (offline, auth + failure, timeout). Callers treat ``False`` as unknown and carry on. + """ + import os + + try: + out = subprocess.run( + ["git", "ls-remote", "--exit-code", "--heads", "origin", f"refs/heads/{branch}"], + cwd=cwd, + capture_output=True, + text=True, + timeout=15, + env={**os.environ, "GIT_TERMINAL_PROMPT": "0"}, + ) + except (OSError, subprocess.TimeoutExpired): + return False + if out.returncode == 2: # --exit-code: no matching ref on the remote + return None + if out.returncode != 0: + return False + return out.stdout.split()[0] if out.stdout.split() else False + + +def branch_from_env() -> str | None: + """The branch a CI provider names in its environment, if any.""" + import os + + return ( + os.getenv("GITHUB_REF_NAME") + or os.getenv("CI_COMMIT_BRANCH") + or os.getenv("CI_BRANCH") + ) + + def safe_branch() -> str: - """Return the current branch name, falling back to short HEAD.""" + """Return the current branch name. + + Raises RuntimeError on a detached HEAD (or when there is no HEAD at + all, e.g. an empty repo) instead of falling back to a commit SHA or a + hardcoded default branch. Neither is a real branch: a SHA is guaranteed + to 404 as a "branch" on the backend, and a hardcoded default is a guess + that is often wrong and, even when right, doesn't reflect what is + actually checked out. + """ try: return _run(["git", "symbolic-ref", "--quiet", "--short", "HEAD"]) except subprocess.CalledProcessError: - try: - return _run(["git", "rev-parse", "--short", "HEAD"]) - except subprocess.CalledProcessError: - return "main" + raise RuntimeError( + "Could not determine the current branch (detached HEAD or no " + "commits yet). Please pass --branch explicitly." + ) -def parse_remote(url: str) -> str: - """Parse a git remote URL into 'owner/repo' format.""" - url = re.sub(r"^(https?://|git@)", "", url) - url = url.replace(":", "/") - if url.endswith(".git"): - url = url[:-4] - parts = url.split("/") - return "/".join(parts[-2:]) +_SCHEME_RE = re.compile(r"^(https?|ssh)://", re.IGNORECASE) -def provider_for_host(host: str) -> str: - """Map a git remote host to a provider. +def _split_remote(url: str) -> tuple[str, str] | None: + """Split a git remote URL into (host, 'owner/repo'). - 'github' is the backward-compatible default for any host we don't - recognize — a GitHub user's request is unaffected, and unknown - self-hosted hosts fall back to the legacy behavior. + Handles 'https://[user[:token]@]host[:port]/owner/repo(.git)' (and + http, ssh), and the scp-like '[user@]host:owner/repo(.git)'. Returns + None when it can't be parsed into host + slug. + + Uses urlsplit (not a blanket ':' -> '/' substitution) so that a colon + inside userinfo -- 'https://user:token@host/...', a real shape for + CI-embedded credentials -- is never mistaken for the scp host:path + separator. Getting this wrong is a security bug, not just a parsing + one: the naive substitution let 'https://github.com:x@evil.com/a/b' + read as host 'github.com' (an allowed host) with the real host, + evil.com, silently discarded. + """ + if _SCHEME_RE.match(url): + parsed = urlsplit(url) + host = parsed.hostname + if not host: + return None + rest = f"{host}{parsed.path}" + elif ":" in url: + # scp-like: '[user@]host:owner/repo(.git)'. The user (if any) is + # whatever precedes the LAST '@' before this colon. + head, _, path = url.partition(":") + host = head.rsplit("@", 1)[-1] + if not host or "/" in host: + return None + rest = f"{host}/{path}" + else: + # No scheme, no ':' -- e.g. a bare filesystem path. Treated + # opaquely: the leading segment stands in for "host" below, so it + # is rejected unless it happens to equal a real host (it never + # will for a real filesystem path). + rest = url + + if rest.endswith(".git"): + rest = rest[:-4] + parts = [p for p in rest.split("/") if p] + if len(parts) < 3: # need host + owner + repo + return None + host = parts[0] + slug = "/".join(parts[-2:]) + return host, slug + + +def _known_provider_for_host(host: str) -> str | None: + """Map a git remote host to a provider we actually recognize. + + Unlike provider_for_host, returns None for a host we don't recognize + instead of defaulting to 'github' -- used where guessing is not safe. """ host = host.lower() if host == "github.com": @@ -63,26 +148,46 @@ def provider_for_host(host: str) -> str: return "bitbucket" if host == "codeberg.org" or host.endswith(".gitea.io"): return "gitea" - return "github" # backward-compatible default + return None -def _split_remote(url: str) -> tuple[str, str] | None: - """Split a git remote URL into (host, 'owner/repo'). +def parse_remote(url: str) -> str: + """Parse a git remote URL into 'owner/repo' format. + + Raises RuntimeError when the remote's host isn't one we recognize. + Blindly slicing the last two path segments of an arbitrary URL (the + old behavior) manufactures a wrong slug for anything that isn't + GitHub/GitLab/Bitbucket/Gitea shaped -- e.g. an Azure DevOps remote + ('.../org/proj/_git/repo') becomes '_git/repo', and a bare filesystem + remote becomes '/'. The backend turns that slug into + an invalid clone URL and 404s, burning a paid scan. + """ + parts = _split_remote(url) + if parts is None: + raise RuntimeError( + f"Could not determine owner/repo from git remote {url!r}. " + "Please pass --repo and --branch explicitly." + ) + host, slug = parts + if _known_provider_for_host(host) is None: + raise RuntimeError( + f"Unsupported git remote host {host!r} (from {url!r}). " + "Only GitHub, GitLab, Bitbucket, and Gitea remotes are " + "auto-detected. Please pass --repo and --branch explicitly." + ) + return slug - Handles both 'git@host:owner/repo(.git)' (scp-like) and - 'https://host/owner/repo(.git)'. Returns None when it can't be parsed - into host + slug. + +def provider_for_host(host: str) -> str: + """Map a git remote host to a provider. + + 'github' is the backward-compatible default for any host we don't + recognize — a GitHub user's request is unaffected, and unknown + self-hosted hosts fall back to the legacy behavior. (Only used for the + additive provider/repo_url fields; parse_remote uses the stricter + _known_provider_for_host and rejects what this would silently default.) """ - rest = re.sub(r"^(https?://|git@)", "", url) - rest = rest.replace(":", "/") - if rest.endswith(".git"): - rest = rest[:-4] - parts = [p for p in rest.split("/") if p] - if len(parts) < 3: # need host + owner + repo - return None - host = parts[0] - slug = "/".join(parts[-2:]) - return host, slug + return _known_provider_for_host(host) or "github" def infer_remote(url: str) -> tuple[str, str | None]: @@ -111,11 +216,7 @@ def detect_repo( import os repo_env = os.getenv("GITHUB_REPOSITORY") or os.getenv("CI_REPOSITORY") - branch_env = ( - os.getenv("GITHUB_REF_NAME") - or os.getenv("CI_COMMIT_BRANCH") - or os.getenv("CI_BRANCH") - ) + branch_env = branch_from_env() repo_slug = repo or repo_env branch_name = branch or branch_env provider: str | None = None diff --git a/python/tests/test_config_secret_handling.py b/python/tests/test_config_secret_handling.py index dd1168a9..05b6e974 100644 --- a/python/tests/test_config_secret_handling.py +++ b/python/tests/test_config_secret_handling.py @@ -90,3 +90,15 @@ def test_env_over_config(self, home, monkeypatch): def test_global_config_used_when_no_flag_or_env(self, home): # No longer a dead path. assert resolve_key(None) == "CONFIG-key" + + def test_dotenv_cannot_supply_key(self, home, monkeypatch): + # A .env that python-dotenv's search would find (for example in a + # cloned repo that also holds the virtualenv) must not outrank the + # operator's stored key. + dotenv_file = home / "repo.env" + dotenv_file.write_text("RAFTER_API_KEY=REPO-key\n") + monkeypatch.setattr("dotenv.main.find_dotenv", lambda *a, **k: str(dotenv_file)) + # Register RAFTER_API_KEY for restore even if a load sets it. + monkeypatch.setenv("RAFTER_API_KEY", "placeholder") + monkeypatch.delenv("RAFTER_API_KEY") + assert resolve_key(None) == "CONFIG-key" diff --git a/python/tests/test_git_utils.py b/python/tests/test_git_utils.py index 099b1488..52ba0734 100644 --- a/python/tests/test_git_utils.py +++ b/python/tests/test_git_utils.py @@ -14,6 +14,7 @@ get_git_root, provider_for_host, infer_remote, + remote_branch_sha, ) @@ -40,6 +41,72 @@ def test_http_no_tls(self): assert parse_remote("http://github.com/owner/repo.git") == "owner/repo" +# sable-pqmw: parse_remote used to slice the last two path segments of ANY +# remote with no host check at all, so a non-GitHub-shaped remote silently +# produced a wrong slug (e.g. Azure DevOps's `.../_git/repo` becomes +# `_git/repo`; a filesystem remote becomes `/`). The +# backend turns that slug into `https://github.com/{slug}` and 404s, +# burning a paid scan. It must now reject anything it can't recognize. +class TestParseRemoteHostValidation: + @pytest.mark.parametrize( + "url,expected", + [ + ("https://github.com/owner/repo", "owner/repo"), + ("git@github.com:owner/repo.git", "owner/repo"), + ("https://github.com/owner/repo.git", "owner/repo"), + # GitLab stays supported -- separate multi-provider feature + # (sable-w79q) that already sends provider + repo_url alongside. + ("git@gitlab.com:group/project.git", "group/project"), + ], + ) + def test_recognized_remote_shapes_still_parse(self, url, expected): + assert parse_remote(url) == expected + + @pytest.mark.parametrize( + "url", + [ + # Azure DevOps: naive last-two-segments yields "_git/repo". + "https://dev.azure.com/my-org/my-proj/_git/my-repo", + # Bare filesystem remote: naive last-two-segments yields + # "/" -- the "local/*" class seen in production. + "/home/ci/local/my-repo", + ], + ) + def test_unrecognized_host_remotes_are_rejected(self, url): + with pytest.raises(RuntimeError, match="[Uu]nsupported"): + parse_remote(url) + + def test_rejection_names_the_offending_host(self): + with pytest.raises(RuntimeError, match="dev.azure.com"): + parse_remote("https://dev.azure.com/my-org/my-proj/_git/my-repo") + + # Found in security review of this fix: a naive "replace : with /" + # treats the userinfo separator the same as the SCP host:path + # separator, so `parts[0]` (the value checked against the host + # allowlist) can be attacker-chosen credentials rather than the real + # host -- and legitimate credentialed remotes (PAT-embedded HTTPS, + # common in CI) hard-fail the same way. + @pytest.mark.parametrize( + "url,expected", + [ + # CI token-embedded remotes -- real shapes, must keep working. + ("https://x-access-token:ghp_abc123@github.com/owner/repo.git", "owner/repo"), + ("https://gitlab-ci-token:glcbt-abc@gitlab.com/group/project.git", "group/project"), + # Explicit ssh:// scheme -- a normal, non-adversarial clone form. + ("ssh://git@github.com/owner/repo.git", "owner/repo"), + ("ssh://git@github.com:2222/owner/repo.git", "owner/repo"), + ], + ) + def test_credentialed_and_ssh_scheme_remotes_still_parse(self, url, expected): + assert parse_remote(url) == expected + + def test_userinfo_cannot_smuggle_an_unrecognized_host_past_the_check(self): + # The real host is evil.com; "github.com" only appears as userinfo. + # Must be rejected (as evil.com), never accepted as github.com. + with pytest.raises(RuntimeError, match="evil.com"): + parse_remote("https://github.com:x@evil.com/foo/bar.git") + + # ── provider_for_host (host → provider inference) ─────────────────── @@ -139,21 +206,30 @@ def test_returns_branch_name(self): with patch("rafter_cli.utils.git._run", return_value="feature/abc"): assert safe_branch() == "feature/abc" - def test_falls_back_to_short_head(self): + # sable-pqmw: a detached HEAD must not submit a commit SHA as a branch + # name -- it is not a branch and is guaranteed to 404 on the backend. + # The old behavior fell back to `rev-parse --short HEAD`; assert that + # even when a SHA IS available, it is never returned. + def test_detached_head_raises_even_when_a_sha_is_available(self): def mock_run(cmd): if "symbolic-ref" in cmd: raise subprocess.CalledProcessError(1, cmd) - return "abc1234" + return "abc1234" # a real SHA is obtainable but must be refused with patch("rafter_cli.utils.git._run", side_effect=mock_run): - assert safe_branch() == "abc1234" + with pytest.raises(RuntimeError, match="branch"): + safe_branch() - def test_falls_back_to_main(self): + # sable-pqmw: total git failure (e.g. an empty repo with no commits) + # must not fall back to a hardcoded "main" -- that guesses the default + # branch and is often wrong, and is misleading even when it isn't. + def test_total_failure_does_not_invent_a_default_branch(self): with patch( "rafter_cli.utils.git._run", side_effect=subprocess.CalledProcessError(1, "git"), ): - assert safe_branch() == "main" + with pytest.raises(RuntimeError, match="branch"): + safe_branch() # ── is_inside_repo ────────────────────────────────────────────────── @@ -282,6 +358,24 @@ def test_infers_gitlab_provider_and_repo_url_from_remote(self, monkeypatch): "https://gitlab.com/group/project", ) + # sable-pqmw: an unrecognized-host remote (e.g. Azure DevOps) must + # surface as a clear, catchable error through the full detection path, + # not a silently wrong repository_name. + def test_raises_on_unrecognized_host_remote(self, monkeypatch): + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.delenv("CI_REPOSITORY", raising=False) + monkeypatch.delenv("GITHUB_REF_NAME", raising=False) + monkeypatch.delenv("CI_COMMIT_BRANCH", raising=False) + monkeypatch.delenv("CI_BRANCH", raising=False) + + with patch("rafter_cli.utils.git.is_inside_repo", return_value=True), \ + patch( + "rafter_cli.utils.git._run", + return_value="https://dev.azure.com/my-org/my-proj/_git/my-repo", + ): + with pytest.raises(RuntimeError, match="dev.azure.com"): + detect_repo() + def test_raises_when_not_in_repo_and_no_env(self, monkeypatch): monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) monkeypatch.delenv("CI_REPOSITORY", raising=False) @@ -292,3 +386,33 @@ def test_raises_when_not_in_repo_and_no_env(self, monkeypatch): with patch("rafter_cli.utils.git.is_inside_repo", return_value=False): with pytest.raises(RuntimeError, match="Could not auto-detect"): detect_repo() + + +# ── remote_branch_sha (real git, local bare origin) ───────────────── + + +def test_remote_branch_sha_tells_unpushed_from_pushed_and_unreachable(tmp_path): + """`rafter run` scans the remote, so it must tell an unpushed local branch + (remote answers, branch absent) apart from a pushed one and from a remote + it cannot reach.""" + + def g(cwd, *args): + return subprocess.check_output( + ["git", *args], cwd=cwd, text=True, stderr=subprocess.DEVNULL + ).strip() + + bare = tmp_path / "origin.git" + work = tmp_path / "work" + work.mkdir() + g(tmp_path, "init", "-q", "--bare", str(bare)) + g(work, "init", "-q", "-b", "main") + g(work, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "--allow-empty", "-m", "init") + g(work, "remote", "add", "origin", str(bare)) + g(work, "push", "-q", "origin", "main") + g(work, "checkout", "-q", "-b", "task/unpushed") + + assert remote_branch_sha("main", cwd=str(work)) == g(work, "rev-parse", "main") + assert remote_branch_sha("task/unpushed", cwd=str(work)) is None + + g(work, "remote", "set-url", "origin", str(tmp_path / "missing.git")) + assert remote_branch_sha("main", cwd=str(work)) is False diff --git a/python/tests/test_issues.py b/python/tests/test_issues.py index 5497d0dd..0ee5ecb7 100644 --- a/python/tests/test_issues.py +++ b/python/tests/test_issues.py @@ -802,3 +802,45 @@ def test_non_json_body_exits_1(self, monkeypatch, capsys): ) assert exc.value.exit_code == 1 assert "No findings to create issues for" not in capsys.readouterr().err + + +# sable-pqmw: detect_repo() can now raise a RuntimeError whose text embeds +# the (attacker-influenceable) remote URL/host. That text reaches +# print_stderr(fmt.error(...)), which renders through Rich with markup +# parsing on -- a value containing a bracketed sequence like "[/bold]" +# closes a tag that was never opened and crashes with an uncaught +# rich.errors.MarkupError instead of a clean error + exit code. +class TestDetectRepoFailureRendering: + def test_markup_like_error_text_does_not_crash_from_scan(self, monkeypatch): + import typer + + from rafter_cli.commands.issues import issues_app as mod + + def raise_malicious(*a, **k): + raise RuntimeError("Unsupported git remote host 'evil[/bold]host' (from '...').") + + monkeypatch.setattr(mod, "detect_repo", raise_malicious) + + with pytest.raises(typer.Exit) as exc: + mod.from_scan( + scan_id=None, from_local="/nonexistent.json", repo=None, api_key=None, + no_dedup=True, dry_run=True, quiet=False, + ) + assert exc.value.exit_code == 1 + + def test_markup_like_error_text_does_not_crash_from_text(self, monkeypatch): + import typer + + from rafter_cli.commands.issues import issues_app as mod + + def raise_malicious(*a, **k): + raise RuntimeError("Unsupported git remote host 'evil[/bold]host' (from '...').") + + monkeypatch.setattr(mod, "detect_repo", raise_malicious) + + with pytest.raises(typer.Exit) as exc: + mod.from_text( + text="a bug", file=None, title=None, labels=None, repo=None, + dry_run=True, quiet=False, + ) + assert exc.value.exit_code == 1 diff --git a/python/tests/test_scan_remote.py b/python/tests/test_scan_remote.py index 4acf0198..a241552a 100644 --- a/python/tests/test_scan_remote.py +++ b/python/tests/test_scan_remote.py @@ -200,6 +200,8 @@ def test_provider_without_any_repo_url_is_omitted(self, _mock_repo, mock_post): assert "provider" not in body assert "repo_url" not in body + # Auto-detected branch: keep the remote-branch lookup off the network. + @patch("rafter_cli.commands.backend.remote_branch_sha", new=lambda *a, **k: False) @patch("rafter_cli.commands.backend.api_post") @patch( "rafter_cli.commands.backend.detect_repo", @@ -224,6 +226,8 @@ def test_inferred_gitlab_provider_flows_into_body(self, _mock_repo, mock_post): assert body["provider"] == "gitlab" assert body["repo_url"] == "https://gitlab.com/group/project" + # Auto-detected branch: keep the remote-branch lookup off the network. + @patch("rafter_cli.commands.backend.remote_branch_sha", new=lambda *a, **k: False) @patch("rafter_cli.commands.backend.api_post") @patch( "rafter_cli.commands.backend.detect_repo", @@ -305,6 +309,8 @@ def test_prints_scan_id_when_not_quiet(self, _mock_repo, mock_post, capsys): err = capsys.readouterr().err assert "s-xyz" in err + # Auto-detected branch: keep the remote-branch lookup off the network. + @patch("rafter_cli.commands.backend.remote_branch_sha", new=lambda *a, **k: False) @patch("rafter_cli.commands.backend.api_post") @patch("rafter_cli.commands.backend.detect_repo", return_value=("owner/repo", "main", "github", "https://github.com/owner/repo")) def test_auto_detect_message_when_not_explicit(self, _mock_repo, mock_post, capsys): diff --git a/python/tests/test_secret_scanning_e2e.py b/python/tests/test_secret_scanning_e2e.py index 74818694..0de0171f 100644 --- a/python/tests/test_secret_scanning_e2e.py +++ b/python/tests/test_secret_scanning_e2e.py @@ -9,6 +9,7 @@ import json import os import subprocess +import sys from pathlib import Path import pytest @@ -391,6 +392,19 @@ def test_detects_secrets_in_changed_files(self, tmp_path): assert len(results) == 1 assert results[0].matches[0].pattern.name == "AWS Access Key ID" + def test_rejects_a_diff_ref_that_git_would_parse_as_an_option(self, tmp_path): + target = tmp_path / "untouched.txt" + target.write_text("keep\n") + + result = subprocess.run( + [sys.executable, "-m", "rafter_cli", "secrets", self.repo, + "--diff", f"--output={target}", "--engine", "patterns", "--quiet"], + capture_output=True, text=True, cwd=self.repo, timeout=60, + env={**os.environ, "HOME": str(tmp_path)}, + ) + assert result.returncode == 2 + assert target.read_text() == "keep\n" + def test_clean_changed_files_produce_no_results(self, tmp_path): initial = _git("rev-parse HEAD", self.repo) diff --git a/python/tests/test_traceback_no_locals.py b/python/tests/test_traceback_no_locals.py new file mode 100644 index 00000000..43255845 --- /dev/null +++ b/python/tests/test_traceback_no_locals.py @@ -0,0 +1,45 @@ +"""An unhandled exception must not print the API key. + +Typer's pretty tracebacks render every frame's local variables by default, +and the request helpers hold the key in ``api_key`` / ``headers`` locals. +A plain transport failure (here: an unreachable proxy) is enough to raise +out of a command, so the key would land on stderr, which in CI is the +build log. +""" +from __future__ import annotations + +import os +import subprocess +import sys + +SENTINEL = "RAFTER_TEST_SENTINEL_KEY_9f3c1a" + + +def test_unhandled_exception_does_not_print_api_key(tmp_path): + env = os.environ.copy() + env.pop("_TYPER_STANDARD_TRACEBACK", None) + env.update( + { + "HOME": str(tmp_path), + "RAFTER_API_KEY": SENTINEL, + "HTTPS_PROXY": "http://127.0.0.1:1", + "https_proxy": "http://127.0.0.1:1", + "NO_PROXY": "", + "no_proxy": "", + } + ) + result = subprocess.run( + [sys.executable, "-m", "rafter_cli", "usage"], + capture_output=True, + text=True, + cwd=tmp_path, + env=env, + timeout=60, + ) + + assert result.returncode != 0 + # Positive control: the command did fail with a traceback, so the + # assertion below is about its content rather than its absence. + assert "Traceback" in result.stderr or "Error" in result.stderr + assert SENTINEL not in result.stderr + assert SENTINEL not in result.stdout diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index 07b79012..dcb4c489 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -76,8 +76,8 @@ Aliases: `rafter scan`, `rafter scan remote` Trigger a new security scan for a repository. - `-k, --api-key TEXT` — API key. Resolution order: this flag → `RAFTER_API_KEY` env → `backend.apiKey` in global config (see `rafter agent config`) -- `-r, --repo TEXT` — org/repo (default: auto-detected from git remote) -- `-b, --branch TEXT` — branch (default: current branch or 'main') +- `-r, --repo TEXT` — org/repo (default: auto-detected from git remote; errors if the remote's host isn't a recognized GitHub/GitLab/Bitbucket/Gitea host — pass this flag explicitly for anything else) +- `-b, --branch TEXT` — branch (default: current branch; errors on a detached HEAD instead of submitting a commit SHA or guessing 'main' — pass this flag explicitly). When both repo and branch are auto-detected, rafter asks `origin` whether the branch exists and exits `1` if it does not, since the scan runs against the remote; if `origin` cannot be reached it proceeds. When the pushed commit differs from local `HEAD` it notes that the scan covers the pushed commit - `-f, --format [json|md]` — output format (default: md) - `-m, --mode [fast|plus]` — scan mode (default: fast). Fast runs SAST, secret detection, and dependency checks. Plus adds agentic deep-dive analysis that examines your codebase the way a professional cybersecurity auditor would — tracing data flows and reasoning about business logic on top of the full SAST/SCA toolchain. **Plus is a paid tier that consumes credits.** - `--github-token TEXT` — GitHub PAT for private repos (or `RAFTER_GITHUB_TOKEN` env var) @@ -1459,7 +1459,7 @@ rafter agent config set agent.riskLevel aggressive ## Notes -- API key: provided via `--api-key` flag, `RAFTER_API_KEY` env var, or `.env` file +- API key: provided via `--api-key` flag, `RAFTER_API_KEY` env var, or a key stored in the global `~/.rafter/config.json`. A `.env` file in the working directory is never read for `RAFTER_*` settings - Git auto-detection works in CI (supports `GITHUB_REPOSITORY`, `GITHUB_REF_NAME`, `CI_REPOSITORY`, `CI_COMMIT_BRANCH`, `CI_BRANCH`) - Remote code analysis targets the remote repository, not local files - All scan data to stdout, all status messages to stderr