From 6101d8f348f2527ebc34669a41a05feb151a3c39 Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Wed, 16 Sep 2026 22:40:43 -0700 Subject: [PATCH 01/10] fix(action): a fork's branch name must not reach the shell holding the API key (rf-7xv0, rf-v2mj) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P0, authorized by Rome 2026-09-13. Both defects are live AT THE v1 TAG, which is what consumers pin. MERGING THIS WITHOUT MOVING v1 CHANGES NOTHING FOR ANY CONSUMER — v1 is 34850d46, 115 commits and five months behind main, so no fix that has ever landed on main has reached them. rf-7xv0 — KEY EXFILTRATION, github-action/action.yml. "branch_name": "${{ github.head_ref || github.ref_name }}", sat inside a `run:` block whose env carries RAFTER_API_KEY. The runner expands `${{ }}` into the script TEXT before bash sees it, so that value was not data — it was source code, chosen by whoever opened the pull request, and anyone can open one from a fork. Measured, not argued: with a branch name of x"; printf %s "$RAFTER_API_KEY" > "$CANARY_PATH"; echo " the canary file came back containing the key. `$( )` works too and needs no quote-breaking at all. Fixed the way GitHub documents: the values reach the script through `env:` (GH_REPOSITORY, GH_BRANCH) and never through `${{ }}` in `run:`. The body is then built with `jq -nc --arg` rather than pasted into a hand-quoted JSON string, which fixes a second bug in the same line — a branch name containing a quote or backslash produced MALFORMED JSON even with no attacker involved. Hand-quoting would have to get shell and JSON escaping both right; jq --arg gets both right by construction. rf-v2mj — AN UNREADABLE REPORT RENDERED AS A CLEAN SCAN, root action.yml. `COUNT=$(... | jq ... || echo "0")` made "no findings" and "I could not read the output" the same value, so the gate passed precisely when it could not see its input. Now the count is written only when a parse actually succeeded; otherwise the step fails and finding-count is left EMPTY rather than a fabricated 0. The text branch distinguishes grep's exit 1 (no matches — clean) from grep failing (>1). Note which file that is: the same defect class in github-action/action.yml was fixed under sable-fgk7 and this ROOT file was never touched by it. Two action.yml files; only one had been fixed. That is the measurement trap the bead warned about, and it is why the corpus is stated below. PROBES — each FAILS on the pre-fix file and passes after, each with a control so it cannot pass by doing nothing: test-trigger-injection.sh unfixed 3 failures -> fixed 0 quote-breaking injection reads the key; $( ) injection reads the key; and a FIDELITY control that a legal branch name containing a quote and a backslash still arrives intact as valid JSON, checked against a real local listener that records the body actually sent. test-root-action-counts.sh unfixed 2 failures -> fixed 0 unparseable report and truncated JSON must fail the step; CONTROLS that a genuinely clean report still passes with count 0 and that real findings are still counted as 2. Without those controls a "fail on everything" change would have passed. Writing the fidelity control honestly cost two rewrites: the first scraped the script text for the old inline-JSON shape and silently matched nothing once the fix removed that shape, and the probe did not model the step's `env:` block, so after the fix the payload was not arriving anywhere and the injection tests were passing vacuously. Both are why the probe now renders the env mapping the way the runner does. CI: both probes wired into test-github-action.yml. The workflow's path filter watched only `github-action/**`, so a change to the ROOT action.yml would not have run the probe that guards it; `action.yml` added to both the pull_request and push filters. CORPUS SEARCHED, as the bead requires. Every action file at both refs: origin/main action.yml head_ref/ref_name: 0 origin/main github-action/action.yml head_ref/ref_name: 1 v1 action.yml head_ref/ref_name: 0 v1 github-action/action.yml head_ref/ref_name: 1 plus .github/workflows/test-action.yml and test-github-action.yml, which are workflows rather than actions and carry no such interpolation. NOT FIXED HERE, reported rather than silently left. A sweep of every `${{ }}` inside a `run:` block found more of the same CLASS, none of them attacker-controlled from a fork: root action.yml interpolates inputs.scan-path, inputs.args, inputs.format and inputs.version directly into run blocks, so a consumer whose workflow passes untrusted text into those inputs has the same shape of problem one level out. github-action/action.yml also interpolates github.repository and github.event.pull_request.number, both runner-supplied and narrowly typed. Worth its own pass; not smuggled into a P0. --- .github/workflows/test-github-action.yml | 28 +++ action.yml | 24 ++- github-action/action.yml | 25 ++- .../tests/test-root-action-counts.sh | 108 +++++++++++ github-action/tests/test-trigger-injection.sh | 171 ++++++++++++++++++ 5 files changed, 349 insertions(+), 7 deletions(-) create mode 100755 github-action/tests/test-root-action-counts.sh create mode 100755 github-action/tests/test-trigger-injection.sh 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/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" From 86893d61a32bba7d3b09c9ab44715a5c8ef7d2bc Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Fri, 18 Sep 2026 19:00:39 -0700 Subject: [PATCH 02/10] fix(git): reject unrecognized-host remotes instead of guessing owner/repo parse_remote/parseRemote took the last two path segments of ANY git remote with no host check, so a non-GitHub-shaped remote silently produced a wrong slug: an Azure DevOps remote (.../org/proj/_git/repo) became "_git/repo", and a bare filesystem remote became "/". The backend turns that into https://github.com/{slug} and 404s, burning a paid scan every time. Both runtimes now require a recognized host (GitHub, GitLab, Bitbucket, Gitea -- the existing multi-provider set) and raise a clear error naming the remote otherwise. safe_branch/safeBranch had the same shape of bug: on a detached HEAD it fell back to a short commit SHA, and on total git failure to a hardcoded "main" -- submitting either as a branch name is a guaranteed branch-not-found failure, and the hardcoded default is also just a guess that can be wrong. Both now raise instead of fabricating a value. Table-driven tests over real remote shapes (github https/ssh/.git, gitlab, azure devops, a bare filesystem path) and a detached HEAD, in both runtimes; each new/changed assertion was confirmed to fail against the pre-fix code before the fix landed. sable-pqmw --- node/src/utils/git.ts | 160 ++++++++++++++++++++++----------- node/tests/git-utils.test.ts | 86 +++++++++++++++++- python/README.md | 4 +- python/rafter_cli/utils/git.py | 104 ++++++++++++++------- python/tests/test_git_utils.py | 77 ++++++++++++++-- shared-docs/CLI_SPEC.md | 4 +- 6 files changed, 338 insertions(+), 97 deletions(-) diff --git a/node/src/utils/git.ts b/node/src/utils/git.ts index 99800b95..160b2458 100644 --- a/node/src/utils/git.ts +++ b/node/src/utils/git.ts @@ -6,37 +6,29 @@ export function git(cmd: string): string { .trim(); } +/** + * 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"; -/** - * 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. - */ -export function providerForHost(host: string): Provider { - 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 -} - /** * 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)`. @@ -52,6 +44,60 @@ function splitRemote(url: string): { host: string; slug: string } | null { 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 null; +} + +/** + * 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. + */ +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"; +} + /** * Infer the provider and a canonical `https:////` clone URL * from a git remote (either scp-like `git@` or `https://` form). Falls back to @@ -73,6 +119,9 @@ export interface DetectedRepo { repo_url?: string; } +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 +132,46 @@ 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. + 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; + } + + 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 }; } diff --git a/node/tests/git-utils.test.ts b/node/tests/git-utils.test.ts index 5bf1ef69..37072388 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,41 @@ 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/); + }); +}); + // ── providerForHost (host → provider inference) ──────────────────── describe("providerForHost", () => { @@ -145,13 +184,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 +283,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/python/README.md b/python/README.md index a0d86cc1..7519c532 100644 --- a/python/README.md +++ b/python/README.md @@ -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/rafter_cli/utils/git.py b/python/rafter_cli/utils/git.py index 4d7998d3..2717fc48 100644 --- a/python/rafter_cli/utils/git.py +++ b/python/rafter_cli/utils/git.py @@ -27,43 +27,22 @@ def is_inside_repo() -> bool: 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" - - -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:]) - - -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. - """ - host = host.lower() - if host == "github.com": - return "github" - if host == "gitlab.com" or host.endswith(".gitlab.com"): - return "gitlab" - if host == "bitbucket.org": - return "bitbucket" - if host == "codeberg.org" or host.endswith(".gitea.io"): - return "gitea" - return "github" # backward-compatible default + raise RuntimeError( + "Could not determine the current branch (detached HEAD or no " + "commits yet). Please pass --branch explicitly." + ) def _split_remote(url: str) -> tuple[str, str] | None: @@ -85,6 +64,63 @@ def _split_remote(url: str) -> tuple[str, str] | None: 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": + return "github" + if host == "gitlab.com" or host.endswith(".gitlab.com"): + return "gitlab" + if host == "bitbucket.org": + return "bitbucket" + if host == "codeberg.org" or host.endswith(".gitea.io"): + return "gitea" + return None + + +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 + + +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.) + """ + return _known_provider_for_host(host) or "github" + + def infer_remote(url: str) -> tuple[str, str | None]: """Infer (provider, repo_url) from a git remote URL. diff --git a/python/tests/test_git_utils.py b/python/tests/test_git_utils.py index 099b1488..1a1453d6 100644 --- a/python/tests/test_git_utils.py +++ b/python/tests/test_git_utils.py @@ -40,6 +40,46 @@ 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") + + # ── provider_for_host (host → provider inference) ─────────────────── @@ -139,21 +179,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 +331,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) diff --git a/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index 07b79012..30f59668 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) - `-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) From 129a09da88685b4fa4931e9dbec1edbc2538468e Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Fri, 18 Sep 2026 19:14:54 -0700 Subject: [PATCH 03/10] fix(git): close host-check bypass via userinfo, ssh:// rejection, markup crash Security review of the parent commit found three real problems in the new host check: - A naive ":" -> "/" substitution treated the userinfo separator in "https://user:token@host/..." the same as the SCP host:path separator, so the value checked against the host allowlist could be attacker-chosen credentials rather than the real host. Concretely, "https://github.com:x@evil.com/foo/bar" parsed as host "github.com" (allowed) while the real host, evil.com, was silently discarded -- a bypass of the check this fix exists to add. - The same bug hard-fails legitimate credentialed HTTPS remotes (PAT-embedded clone URLs, common in CI) that used to at least produce a slug. - An explicit "ssh://" scheme was never stripped, so a normal "ssh://git@github.com/owner/repo.git" remote -- not an adversarial shape -- was rejected as host "ssh". _split_remote/splitRemote now use the URL parser (urlsplit / URL) for scheme-based remotes instead of a blanket colon substitution, so userinfo and port are stripped the same way a browser or curl would strip them, and "ssh://" is recognized alongside "https?://". Also: the new RuntimeError messages embed the remote URL/host, which is attacker-influenceable (a malicious repo's own remote config). Two call sites (issues create from-scan/from-text) render caught errors through Rich markup; a value containing something like "[/bold]" closed a tag that was never opened and crashed with an uncaught MarkupError instead of a clean error + exit code. Escaped before rendering. (Node has no equivalent -- its error rendering is a plain template literal, not a markup language.) Each new/changed test was confirmed to fail against the pre-fix code first. sable-pqmw --- node/src/utils/git.ts | 46 +++++++++++++++++-- node/tests/git-utils.test.ts | 25 ++++++++++ .../rafter_cli/commands/issues/issues_app.py | 13 +++++- python/rafter_cli/utils/git.py | 41 +++++++++++++++-- python/tests/test_git_utils.py | 26 +++++++++++ python/tests/test_issues.py | 42 +++++++++++++++++ 6 files changed, 182 insertions(+), 11 deletions(-) diff --git a/node/src/utils/git.ts b/node/src/utils/git.ts index 160b2458..38618753 100644 --- a/node/src/utils/git.ts +++ b/node/src/utils/git.ts @@ -29,13 +29,51 @@ export function safeBranch(gitFn: (c: string) => string): string { export type Provider = "github" | "gitlab" | "gitea" | "bitbucket"; +const SCHEME_RE = /^(https?|ssh):\/\//i; + /** - * 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. + * 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. */ function splitRemote(url: string): { host: string; slug: string } | null { - let rest = url.replace(/^(https?:\/\/|git@)/, "").replace(":", "/"); + 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 diff --git a/node/tests/git-utils.test.ts b/node/tests/git-utils.test.ts index 37072388..fede21f6 100644 --- a/node/tests/git-utils.test.ts +++ b/node/tests/git-utils.test.ts @@ -66,6 +66,31 @@ describe("parseRemote host validation", () => { 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) ──────────────────── 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/utils/git.py b/python/rafter_cli/utils/git.py index 2717fc48..79acd4cb 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: @@ -45,15 +46,45 @@ def safe_branch() -> str: ) +_SCHEME_RE = re.compile(r"^(https?|ssh)://", re.IGNORECASE) + + def _split_remote(url: str) -> tuple[str, str] | None: """Split a git remote URL into (host, 'owner/repo'). - 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. + 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. """ - rest = re.sub(r"^(https?://|git@)", "", url) - rest = rest.replace(":", "/") + 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] diff --git a/python/tests/test_git_utils.py b/python/tests/test_git_utils.py index 1a1453d6..23f2662c 100644 --- a/python/tests/test_git_utils.py +++ b/python/tests/test_git_utils.py @@ -79,6 +79,32 @@ 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) ─────────────────── 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 From 5d5f37ac37f99eca10530578626473e23b767f55 Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Sat, 19 Sep 2026 19:04:38 -0700 Subject: [PATCH 04/10] docs(changelog): drop an entry that referenced a private repository The Rafter Sites entry cited a private repo and PR number, and described the internal endpoint surface it calls. This is a public changelog. --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da672a00..05b8a1f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,7 +72,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 From b69b1eff83b62cd1281ad8e31c29249d9fdc1cea Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Thu, 1 Oct 2026 16:38:34 -0700 Subject: [PATCH 05/10] fix(python): do not render local variables in tracebacks Typer's pretty tracebacks print every frame's locals by default. Request helpers keep credentials in locals, so an unhandled exception could echo them to stderr. Turn show_locals off on the root app. Adds a subprocess test that forces a transport failure and asserts a sentinel credential never reaches stdout or stderr. --- python/rafter_cli/__main__.py | 2 ++ python/tests/test_traceback_no_locals.py | 45 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 python/tests/test_traceback_no_locals.py 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/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 From dba2961b807e8e01c4853ee2f278c312f276a761 Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Thu, 1 Oct 2026 16:57:04 -0700 Subject: [PATCH 06/10] fix: never take RAFTER_* settings from a project .env The working directory can be an untrusted repository, so its .env must not supply operator settings such as the API key, GitHub token, notify webhook or paid-scan confirmation. Node: the startup dotenv guard now drops every RAFTER_* variable that .env introduced, not only the disable and hook switches. Values already in the real environment are untouched. Python: resolve_key no longer calls load_dotenv(). Its upward search starts from the install path, which reaches a repository's .env when the virtualenv lives inside it. Docs no longer suggest putting the API key in .env; use the environment or the global config file. --- README.md | 4 +-- node/src/index.ts | 2 +- node/src/utils/env-guard.ts | 28 ++++++++++----------- node/tests/e2e-cli.test.ts | 13 +++++----- node/tests/env-guard.test.ts | 19 ++++++++++---- python/README.md | 2 +- python/rafter_cli/utils/api.py | 5 ++-- python/tests/test_config_secret_handling.py | 12 +++++++++ shared-docs/CLI_SPEC.md | 2 +- 9 files changed, 54 insertions(+), 33 deletions(-) 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/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/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/python/README.md b/python/README.md index 7519c532..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 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/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/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index 30f59668..34a8fbe0 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -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 From f0ee218af63b9c503c3c9dcbc2a17a13529e683b Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Thu, 1 Oct 2026 17:09:41 -0700 Subject: [PATCH 07/10] fix(run): refuse an auto-detected branch that is not on the remote `rafter run` scans the remote repository, but when it auto-detected the current local branch it never checked that the branch had been pushed. The scan was queued and then failed on the backend minutes later. When both repo and branch come from the local checkout, ask origin with `git ls-remote --exit-code --heads`. If the remote answers without the branch, exit 1 with a message to push it or pass --branch. If origin cannot be reached, proceed as before. If the pushed commit differs from local HEAD, note that the scan covers the pushed commit. Explicit --branch and CI-provided branches are unchanged. Same behavior in the Node and Python CLIs, each with a test against a real local bare remote. --- node/src/commands/backend/run.ts | 27 ++++++++++++++-- node/src/utils/git.ts | 34 +++++++++++++++++++-- node/tests/remote-branch.test.ts | 36 ++++++++++++++++++++++ python/rafter_cli/commands/backend.py | 26 +++++++++++++++- python/rafter_cli/utils/git.py | 44 ++++++++++++++++++++++++--- python/tests/test_git_utils.py | 31 +++++++++++++++++++ python/tests/test_scan_remote.py | 6 ++++ shared-docs/CLI_SPEC.md | 2 +- 8 files changed, 195 insertions(+), 11 deletions(-) create mode 100644 node/tests/remote-branch.test.ts 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/utils/git.ts b/node/src/utils/git.ts index 38618753..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,6 +6,32 @@ 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. * @@ -155,6 +181,8 @@ 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 = @@ -191,6 +219,7 @@ export function detectRepo(opts: { repo?: string; branch?: string; quiet?: boole // 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 { @@ -204,6 +233,7 @@ export function detectRepo(opts: { repo?: string; branch?: string; quiet?: boole repoUrl = inferred.repoUrl; } + const localBranch = repoFromOrigin && !branch; if (!branch) { branch = safeBranch(git); } @@ -211,5 +241,5 @@ export function detectRepo(opts: { repo?: string; branch?: string; quiet?: boole 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 }; + return { repo: repoSlug, branch, provider, repo_url: repoUrl, local_branch: localBranch }; } 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/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/utils/git.py b/python/rafter_cli/utils/git.py index 79acd4cb..f1b91e1e 100644 --- a/python/rafter_cli/utils/git.py +++ b/python/rafter_cli/utils/git.py @@ -27,6 +27,44 @@ 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. @@ -178,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_git_utils.py b/python/tests/test_git_utils.py index 23f2662c..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, ) @@ -385,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_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/shared-docs/CLI_SPEC.md b/shared-docs/CLI_SPEC.md index 30f59668..3fa3e74e 100644 --- a/shared-docs/CLI_SPEC.md +++ b/shared-docs/CLI_SPEC.md @@ -77,7 +77,7 @@ 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; 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) +- `-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) From e708bb0fef906c5598cfaa2f30b3642715c7e2bd Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Thu, 1 Oct 2026 17:17:59 -0700 Subject: [PATCH 08/10] fix(secrets): reject a --diff ref that starts with "-" The --diff value is passed to `git diff` as a positional argument, and git parses anything starting with "-" as one of its own options. Reject such a value with exit 2 (invalid ref) before running git, in both the Node and Python CLIs, for `rafter secrets` and `rafter agent scan`. Each runtime gets an end-to-end test that passes an option-shaped ref and asserts exit 2 with the target file left untouched. --- node/src/commands/agent/scan.ts | 5 +++++ node/tests/secret-scanning-e2e.test.ts | 12 ++++++++++++ python/rafter_cli/commands/agent.py | 8 ++++++++ python/rafter_cli/commands/scan.py | 2 ++ python/tests/test_secret_scanning_e2e.py | 14 ++++++++++++++ 5 files changed, 41 insertions(+) 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/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/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/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/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) From 63b3b46250b96b5c52abe9ee4dc58b87c11635d7 Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Thu, 1 Oct 2026 17:24:06 -0700 Subject: [PATCH 09/10] fix(node): pass paths to child processes as arguments, not shell strings `agent init --with-gemini` built `gemini skills link ""` as a shell string, quoting the path with JSON.stringify. Double quotes do not stop a POSIX shell from expanding $(...) or backticks, and the path comes from the working directory. Run gemini with an argument array instead; Windows keeps cmd.exe, which it needs for gemini's .cmd shim and which treats a double-quoted path literally. Same change for two siblings rooted in the home directory: the global `git config core.hooksPath` call in install-hook and the local betterleaks version probe in status. Adds an end-to-end test that runs init from a directory whose name contains shell syntax, with a stub gemini on PATH, and asserts the path arrives literally and nothing is executed. --- node/src/commands/agent/init.ts | 15 ++++-- node/src/commands/agent/install-hook.ts | 4 +- node/src/commands/agent/status.ts | 4 +- node/tests/agent-init-gemini-link.test.ts | 56 +++++++++++++++++++++++ 4 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 node/tests/agent-init-gemini-link.test.ts 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/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/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 }); + } + }); +}); From 298f0a25aacdf1c4cd4c351fc02bd6d405c5115f Mon Sep 17 00:00:00 2001 From: Rome-1 Date: Thu, 1 Oct 2026 21:38:15 -0700 Subject: [PATCH 10/10] release: v0.10.6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bump node, python and both ClawHub skill manifests 0.10.5 -> 0.10.6, and record what this release ships. 0.10.6 carries five fixes already on main: - Unhandled errors no longer print the API key in a traceback (#264) - A project .env can no longer supply RAFTER_* settings, including the API key (#265) — behavior change, noted in the CHANGELOG - rafter run now checks that an auto-detected branch has been pushed before scanning it (#266) - rafter secrets --diff / rafter agent scan --diff reject an option-shaped ref before it reaches git (#267) - rafter agent init --local --with-gemini no longer runs gemini through a shell (#268) Verified via scripts/check-version-unpublished.sh that 0.10.6 is not yet on npm or PyPI. --- CHANGELOG.md | 16 ++++++++++++++++ node/package.json | 2 +- node/resources/rafter-security-skill.md | 2 +- python/pyproject.toml | 2 +- .../resources/rafter-security-skill.md | 2 +- 5 files changed, 20 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05b8a1f3..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 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/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/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: