From 0a40b3806fc08c6d7b2f9fd9b7ade07fdf1841f1 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 14 Aug 2026 14:30:52 +0200 Subject: [PATCH] fix(agents): let the scratch-dir hooks own their permission prompt (#10702) --- .claude/hooks/allow-fileops-in-tmp.sh | 58 ++++++++++-------- .claude/hooks/guard-rm-outside-tmp.sh | 49 +++++++++------ .claude/hooks/lib-guarded-verb.sh | 57 +++++++++++++++++ .claude/hooks/test-hooks.sh | 88 +++++++++++++++++++++++++++ .claude/settings.json | 3 - docs/validation.md | 1 + 6 files changed, 212 insertions(+), 44 deletions(-) create mode 100644 .claude/hooks/lib-guarded-verb.sh create mode 100644 .claude/hooks/test-hooks.sh diff --git a/.claude/hooks/allow-fileops-in-tmp.sh b/.claude/hooks/allow-fileops-in-tmp.sh index ce2ce81831..665c23cf0a 100755 --- a/.claude/hooks/allow-fileops-in-tmp.sh +++ b/.claude/hooks/allow-fileops-in-tmp.sh @@ -2,10 +2,12 @@ # PreToolUse allowance for scratch file ops: auto-allow a single, plain, single-line # `mkdir` / `cp` / `mv` / `touch` / `chmod` / `tar` / `unzip` whose every path operand # resolves under /tmp. Anything else makes no decision (exit 0) and falls back to the normal -# permission flow — where `Bash(mv:*)` and `Bash(chmod:*)` in the `ask` list prompt. A -# PreToolUse `allow` overrides those ask rules, which is why this is a hook and not an allow -# rule: permission rules match a command prefix, so they can only constrain the FIRST operand. -# `cp /tmp/x ~/.zshrc` matches a `cp /tmp/` prefix, and requiring every operand is the point. +# permission flow, except for `mv` and `chmod`: those get an explicit `ask`, the only prompt +# they get (see lib-guarded-verb.sh). +# +# This is a hook rather than an allow rule because permission rules match a command prefix, so +# they can only constrain the FIRST operand. `cp /tmp/x ~/.zshrc` matches a `cp /tmp/` prefix, +# and requiring every operand is the point. # # Requiring the sources under /tmp too (not just the destination) keeps this from becoming a # read-exfiltration path around the `Read(**/.env)` / `Read(**/secrets/**)` deny rules: a copy @@ -31,6 +33,7 @@ # # Assumes GNU `realpath` (-m) and `jq`, both present in this repo's Linux dev env. set -uo pipefail +. "${BASH_SOURCE[0]%/*}/lib-guarded-verb.sh" input=$(cat) command -v jq >/dev/null 2>&1 || exit 0 @@ -38,8 +41,19 @@ cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // empty' 2>/dev/null) [ -z "$cmd" ] && exit 0 cwd=$(printf '%s' "$input" | jq -r '.cwd // empty' 2>/dev/null) +# Every bail-out below goes through `defer`: `mv` and `chmod` prompt from here, since no rule +# covers them, while the other verbs stay silent and leave the decision to the normal flow. +guarded=0 +for verb in mv chmod; do + runs_verb "$verb" "$cmd" && { guarded=1; break; } +done +defer() { + [ "$guarded" = 1 ] && decide ask "$1" + exit 0 +} + # A newline separates commands, and the tokenizer below only reads the first line — defer. -case "$cmd" in *$'\n'*) exit 0 ;; esac +case "$cmd" in *$'\n'*) defer "multi-line command" ;; esac read -r -a toks <<< "$cmd" @@ -65,11 +79,6 @@ under_tmp() { return 1 } -allow() { - jq -nc --arg r "$1" '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:"allow",permissionDecisionReason:$r}}' - exit 0 -} - # Bare command word only; wrappers (`timeout cp`), env prefixes, and `/bin/cp` defer. # Options are an allowlist per command, so anything that changes how symlinks are followed # defers instead of needing enumeration. `cp -L` / `-H` matter most: they dereference while @@ -84,7 +93,7 @@ case "${toks[0]:-}" in chmod) takes_mode=1; ok_opts='Rvfc' ;; # chmod's first operand is a mode, not a path tar) ok_flags='xctzjJavfC'; val_flags='fC' ;; unzip) ok_flags='oqnljvd'; val_flags='d' ;; - *) exit 0 ;; + *) defer "not the leading command word" ;; esac # ---------------------------------------------------------------- tar / unzip @@ -101,18 +110,18 @@ if [ -n "${ok_flags:-}" ]; then flags="${t#-}" # Allowlist: a long option, -P/--absolute-names, --transform, -I and friends all # leave a residue here and defer rather than being enumerated as denials. - [ -n "$(printf '%s' "$flags" | tr -d "$ok_flags")" ] && exit 0 + [ -n "$(printf '%s' "$flags" | tr -d "$ok_flags")" ] && defer "unrecognized option \`$t\`" case "$flags" in *x*) extracting=1 ;; esac case "${toks[0]}$flags" in unzip*[lv]*) listing=1 ;; esac # A flag consuming the next token must be alone in its bundle's final position # (`-xzf a.tar`), else the token it eats is ambiguous. - case "${flags%?}" in *[$val_flags]*) exit 0 ;; esac + case "${flags%?}" in *[$val_flags]*) defer "ambiguous option bundle \`$t\`" ;; esac case "${flags: -1}" in [$val_flags]) val="${toks[$i]:-}" i=$((i + 1)) - [ -n "$val" ] || exit 0 - under_tmp "$val" || exit 0 + [ -n "$val" ] || defer "option \`$t\` has no value" + under_tmp "$val" || defer "\`$val\` is outside /tmp" case "${flags: -1}" in f) saw_archive=1 ;; C | d) saw_dest=1 ;; @@ -126,17 +135,18 @@ if [ -n "${ok_flags:-}" ]; then # Positional. For tar these are sources (create) or member names (extract); for unzip the # first is the archive. Requiring every one under /tmp is conservative for member names, # which are not filesystem paths — those defer rather than being wrongly allowed. - under_tmp "$t" || exit 0 + under_tmp "$t" || defer "\`$t\` is outside /tmp" [ "${toks[0]}" = "unzip" ] && saw_archive=1 done - [ "$saw_archive" = 1 ] || exit 0 # tar without -f reads a tape/stdin; unzip needs an archive + # tar without -f reads a tape/stdin; unzip needs an archive + [ "$saw_archive" = 1 ] || defer "no archive operand" # Writes land relative to the working directory unless a destination was given. `unzip -l` # and `-v` only list, so they need no destination. if [ "$extracting" = 1 ] || { [ "${toks[0]}" = "unzip" ] && [ "$listing" = 0 ]; }; then - [ "$saw_dest" = 1 ] || under_tmp "${cwd:-$PWD}" || exit 0 + [ "$saw_dest" = 1 ] || under_tmp "${cwd:-$PWD}" || defer "extraction target is outside /tmp" fi - allow "archive paths and extraction target are under /tmp" + decide allow "archive paths and extraction target are under /tmp" fi # ------------------------------------------- mkdir / cp / mv / touch / chmod @@ -155,7 +165,7 @@ while [ "$i" -lt "${#toks[@]}" ]; do case "$t" in -?*) # Allowlist: long options and the dereferencing flags leave a residue and defer. - [ -n "$(printf '%s' "${t#-}" | tr -d "$ok_opts")" ] && exit 0 + [ -n "$(printf '%s' "${t#-}" | tr -d "$ok_opts")" ] && defer "unrecognized option \`$t\`" continue ;; esac @@ -165,15 +175,15 @@ while [ "$i" -lt "${#toks[@]}" ]; do if [ "$takes_mode" = 1 ] && [ "$seen_mode" = 0 ]; then case "$t" in [0-7] | [0-7][0-7] | [0-7][0-7][0-7] | [0-7][0-7][0-7][0-7]) ;; - *) printf '%s' "$t" | grep -Eq '^[ugoa]*[+=-][rwxXst]*(,[ugoa]*[+=-][rwxXst]*)*$' || exit 0 ;; + *) printf '%s' "$t" | grep -Eq '^[ugoa]*[+=-][rwxXst]*(,[ugoa]*[+=-][rwxXst]*)*$' || defer "unrecognized mode \`$t\`" ;; esac seen_mode=1 continue fi - under_tmp "$t" || exit 0 + under_tmp "$t" || defer "\`$t\` is outside /tmp" path_operand=1 done -[ "$path_operand" = 1 ] || exit 0 -allow "every path operand is under /tmp" +[ "$path_operand" = 1 ] || defer "no path operand" +decide allow "every path operand is under /tmp" diff --git a/.claude/hooks/guard-rm-outside-tmp.sh b/.claude/hooks/guard-rm-outside-tmp.sh index 66d4dd27b5..5aff497d89 100755 --- a/.claude/hooks/guard-rm-outside-tmp.sh +++ b/.claude/hooks/guard-rm-outside-tmp.sh @@ -1,9 +1,9 @@ #!/usr/bin/env bash # PreToolUse guard for `rm`: auto-allow ONLY a single, plain, single-line `rm` whose every # operand is a whitelisted target — under /tmp, or inside a git working tree located in $HOME -# (a version-controlled project dir). Anything else makes no decision (exit 0) and falls back -# to the normal permission flow, where the `Bash(rm:*)` ask rule prompts (classifier as a -# backstop). +# (a version-controlled project dir). Any other command that runs `rm` gets an explicit `ask`, +# which is the ordinary permission prompt and the only one `rm` gets (see lib-guarded-verb.sh); +# a command that runs no `rm` at all makes no decision (exit 0). # # The git-tree allowance trades on "this is a project under version control" being lower-stakes # than a delete elsewhere — NOT on full recoverability: committed content is restorable via git, @@ -19,13 +19,14 @@ # # The git-repo allowance covers targets inside a git working tree under $HOME, and the tree's # own root folder only when it is a linked worktree (`.git` is a pointer file, so history in -# the main repo survives); a primary checkout's root (`.git` is a history dir) and any `.git` -# path are never auto-allowed. Globs auto-allow only under /tmp — elsewhere their expansion +# the main repo survives); a primary checkout's root (`.git` is a history dir) and any `.git`, +# `.claude` or `.env` path are never auto-allowed. Globs auto-allow only under /tmp — elsewhere their expansion # could reach `.git` or a dotfile the literal checks never see. Relative operands resolve -# against the command's cwd (from the hook input). A PreToolUse `allow` overrides the ask rule. +# against the command's cwd (from the hook input). # # Assumes GNU `realpath` (-m) and `jq`, both present in this repo's Linux dev env. set -uo pipefail +. "${BASH_SOURCE[0]%/*}/lib-guarded-verb.sh" input=$(cat) command -v jq >/dev/null 2>&1 || exit 0 @@ -33,12 +34,20 @@ cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // empty' 2>/dev/null) [ -z "$cmd" ] && exit 0 cwd=$(printf '%s' "$input" | jq -r '.cwd // empty' 2>/dev/null) +# Every bail-out below goes through `defer`, so the forms this guard refuses to reason about — +# compound, quoted, wrapped — still reach the user as a prompt whenever an `rm` runs among them. +runs_verb rm "$cmd" && guarded=1 || guarded=0 +defer() { + [ "$guarded" = 1 ] && decide ask "$1" + exit 0 +} + # A newline separates commands, and the tokenizer below only reads the first line — defer. -case "$cmd" in *$'\n'*) exit 0 ;; esac +case "$cmd" in *$'\n'*) defer "multi-line command" ;; esac read -r -a toks <<< "$cmd" # Bare leading `rm` only; wrappers (`timeout rm`), env prefixes, and `/bin/rm` defer. -[ "${toks[0]:-}" = "rm" ] || exit 0 +[ "${toks[0]:-}" = "rm" ] || defer "rm is not the leading command word" # 0 (allow) iff the canonical path is an auto-allowable rm target: under /tmp, or strictly # inside a git working tree located under $HOME. The walk stops at $HOME, so a dotfiles repo at @@ -48,7 +57,13 @@ allowed_target() { case "$canon" in /tmp/?*) return 0 ;; esac [ -n "${HOME:-}" ] || return 1 case "$canon" in "$HOME"/?*) ;; *) return 1 ;; esac - case "$canon" in *"/.git" | *"/.git/"*) return 1 ;; esac # protect history, not recoverable + # Never auto-allow: history, and the two kinds of path the "it's under version control" + # premise doesn't hold for — the agent's own guards and settings (deleting them is what + # removes the prompt on everything else), and gitignored `.env` files. + case "$canon" in + *"/.git" | *"/.git/"* | *"/.claude" | *"/.claude/"*) return 1 ;; + *"/.env" | *"/.env."*) return 1 ;; + esac d="$canon" while [ "$d" != "/" ] && [ "$d" != "$HOME" ]; do [ -e "$d/.git" ] && { root="$d"; break; } @@ -73,10 +88,10 @@ while [ "$i" -lt "${#toks[@]}" ]; do i=$((i + 1)) # Whitelist every token (flags included, so an operator hidden in a flag like `-rf;rm` # can't slip past): any character outside the safe set makes it unsafe to reason about. - [ -n "$(printf '%s' "$t" | tr -d 'A-Za-z0-9._/*?[]-')" ] && exit 0 + [ -n "$(printf '%s' "$t" | tr -d 'A-Za-z0-9._/*?[]-')" ] && defer "unsafe characters in \`$t\`" # A glob in an option-looking token (`-[-]`) can expand to `--` and turn a later `-name` # into an operand — never a real option, so defer. - case "$t" in -*[*?[]*) exit 0 ;; esac + case "$t" in -*[*?[]*) defer "glob inside the option \`$t\`" ;; esac if [ "$end_opts" = 0 ]; then [ "$t" = "--" ] && { end_opts=1; continue; } # Skip real options only before the first operand. A bare `-` is a filename, and under @@ -89,18 +104,18 @@ while [ "$i" -lt "${#toks[@]}" ]; do had_operand=1 # No wildcard in a non-final path segment (`a/*/b`): it can expand through a symlink # realpath can't see. A slashless glob (`*.rs`) is a final-segment match — fine. - case "$t" in */*) case "${t%/*}" in *[*?[]*) exit 0 ;; esac ;; esac + case "$t" in */*) case "${t%/*}" in *[*?[]*) defer "glob in a non-final segment of \`$t\`" ;; esac ;; esac case "$t" in /*) canon=$(realpath -m -- "$t" 2>/dev/null) ;; *) canon=$(realpath -m -- "${cwd:-$PWD}/$t" 2>/dev/null) ;; esac - [ -n "$canon" ] || exit 0 + [ -n "$canon" ] || defer "cannot resolve \`$t\`" # A glob may auto-allow only under /tmp, where everything is deletable. Elsewhere its # expansion could match `.git`, a dotfile like `.*`, or a nested checkout root that the # literal-path checks never see — so require literal operands in git repos. - case "$t" in *[*?[]*) case "$canon" in /tmp/?*) ;; *) exit 0 ;; esac ;; esac - allowed_target "$canon" || exit 0 + case "$t" in *[*?[]*) case "$canon" in /tmp/?*) ;; *) defer "glob \`$t\` is outside /tmp" ;; esac ;; esac + allowed_target "$canon" || defer "\`$canon\` is outside /tmp and not inside a git checkout in \$HOME" done -[ "$had_operand" = 1 ] || exit 0 -jq -nc '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:"allow",permissionDecisionReason:"rm operands are under /tmp or inside a git checkout in $HOME"}}' +[ "$had_operand" = 1 ] || defer "no operand" +decide allow 'rm operands are under /tmp or inside a git checkout in $HOME' diff --git a/.claude/hooks/lib-guarded-verb.sh b/.claude/hooks/lib-guarded-verb.sh new file mode 100644 index 0000000000..28ededaf8b --- /dev/null +++ b/.claude/hooks/lib-guarded-verb.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# Sourced by the PreToolUse guards; not a hook itself. +# +# A permission rule beats a hook: an `ask` rule prompts whatever a PreToolUse hook returns, which +# makes the hook's `allow` dead weight. So settings.json carries no `ask` rule for `rm`, `mv` or +# `chmod`, and the guards own both halves — `allow` what they can prove safe, `ask` for the rest. +# Removing a guard's `ask` path therefore removes that verb's prompt entirely. +# +# `set -f` is global to the sourcing script so that the unquoted word split in runs_verb cannot +# expand a glob operand against the filesystem. Neither guard relies on pathname expansion. +set -f + +# 0 iff ($1) runs as a command word anywhere in ($2). Mirrors how a Bash +# permission rule matches, so that owning the prompt here doesn't narrow what used to prompt: +# the command splits on `; & |` and newlines, and a leading env assignment or process wrapper +# (`timeout 5 rm`, `xargs rm`) is skipped before the command word is read. +# +# The split set also carries the characters that open a nested command — `$(`, backticks, `( )` +# and `{ }` — because a rule matches the verb inside one (`echo $(rm -rf ~)` prompts), and a +# separator that only ends statements would read that as an `echo`. +# +# The scan is textual, so a heredoc body that merely contains the verb (`cat > s.sh <'* | *'<'*) ;; # leading redirect, `>/dev/null rm ...` + '!' | if | then | elif | else | while | until | do) ;; # keywords, never the command + timeout | time | nice | nohup | stdbuf | command | builtin | noglob | xargs | sudo | env) wrapped=1 ;; + # A wrapper's option value is indistinguishable from a command name (`stdbuf -o L rm`), + # so past a wrapper the whole segment is scanned instead of stopping at the first + # ordinary word. Before one, that word is the command and the verb cannot follow it. + *) [ "$wrapped" = 1 ] || break ;; + esac + done + # `tr` and not `${2//[...]}`: a `}` inside the bracket expression closes the expansion + # itself, which silently leaves the command unsplit and every separator unseen. + done <<< "$(printf '%s' "$2" | tr ';&|(){}`' '\n')" + return 1 +} + +# Emit a PreToolUse decision and exit. `ask` is the ordinary permission prompt. +decide() { + jq -nc --arg d "$1" --arg r "$2" \ + '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:$d,permissionDecisionReason:$r}}' + exit 0 +} diff --git a/.claude/hooks/test-hooks.sh b/.claude/hooks/test-hooks.sh new file mode 100644 index 0000000000..905b251a4e --- /dev/null +++ b/.claude/hooks/test-hooks.sh @@ -0,0 +1,88 @@ +#!/usr/bin/env bash +# Decision table for the two scratch-dir PreToolUse guards. Run: bash .claude/hooks/test-hooks.sh +# +# What this pins is the `ask` column: a matcher change that turns one into a no-decision drops +# that command's only prompt (see lib-guarded-verb.sh). The wrapper, nested-command and quoted +# rows are the ones that catch it. +set -uo pipefail +H="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" +CWD="$(git -C "$H" rev-parse --show-toplevel)" +OUT="$HOME/not-a-git-tree" # never written to; only the guards' path checks look at it +fails=0 + +run() { # run + local hook="$1" want="$2" cmd="$3" out got + out=$(jq -nc --arg c "$cmd" --arg w "$CWD" \ + '{tool_name:"Bash",tool_input:{command:$c},cwd:$w}' | "$H/$hook" 2>&1) + if [ -z "$out" ]; then + got=none + else + got=$(printf '%s' "$out" | jq -r '.hookSpecificOutput.permissionDecision // "PARSE-ERROR"' 2>/dev/null || echo PARSE-ERROR) + fi + if [ "$got" = "$want" ]; then + printf ' ok %-5s %s\n' "$got" "$cmd" + else + printf 'FAIL want=%-5s got=%-5s %s\n %s\n' "$want" "$got" "$cmd" "$out" + fails=$((fails + 1)) + fi +} + +echo "== guard-rm-outside-tmp.sh ==" +G=guard-rm-outside-tmp.sh +run $G allow "rm -rf /tmp/scratch/x" +run $G allow "rm -rf /tmp/scratch/*" +run $G allow "rm -rf $CWD/frontend/scratch" +run $G ask "rm -rf /tmp" +run $G ask "rm -rf $OUT" +run $G ask "rm -rf $CWD/.git" +run $G ask "rm -rf $CWD/.claude/hooks" # the guards may not delete themselves +run $G ask "rm $CWD/.claude/settings.json" +run $G ask "rm $CWD/.claude/settings.local.json" +run $G ask "rm -rf $CWD/backend/.env" +run $G ask "rm -rf $CWD/.env.local" +run $G ask "rm -rf $CWD" +run $G ask "rm -rf $CWD/*" +run $G ask "rm -rf /etc/passwd" +run $G ask 'rm -rf "$HOME/x"' +run $G ask "rm -rf /tmp/../$OUT" +run $G ask "ls /tmp && rm -rf /tmp/x" +run $G ask 'echo $(rm -rf /etc)' +run $G ask 'echo `rm -rf /etc`' +run $G ask "{ rm -rf /etc; }" +run $G ask "find . -name x | xargs rm" +run $G ask "timeout 5 rm -rf /tmp/x" +run $G ask "stdbuf -o L rm -rf /etc" +run $G ask "FOO=bar rm -rf /tmp/x" +run $G ask "/bin/rm -rf /tmp/x" +run $G ask "'rm' -rf /etc" +run $G ask 'r\m -rf /etc' +run $G ask "! rm -rf /etc" +run $G ask "if true; then rm -rf /etc; fi" +run $G ask ">/dev/null rm -rf $OUT" +run $G none "git rm frontend/foo.ts" +run $G none 'echo $(ls /tmp)' +run $G none 'grep -rn "rm" backend/' +run $G none "cargo build --release" + +echo +echo "== allow-fileops-in-tmp.sh ==" +A=allow-fileops-in-tmp.sh +run $A allow "mv /tmp/a /tmp/b" +run $A allow "chmod 755 /tmp/a" +run $A allow "cp -r /tmp/a /tmp/b" +run $A allow "tar -xzf /tmp/a.tar.gz -C /tmp/out" +run $A ask "mv /tmp/a $OUT" +run $A ask "mv $CWD/AGENTS.md /tmp/a" +run $A ask "chmod -R 777 $CWD" +run $A ask "ls && mv /tmp/a /tmp/b" +run $A ask 'echo $(mv /tmp/a /etc)' +run $A ask "timeout --signal KILL 5 mv /tmp/a /etc" +run $A ask "time -f FORMAT chmod 777 $OUT" +run $A ask "'mv' /tmp/a /etc" +run $A ask 'ch\mod 777 /etc' +run $A none "cp $CWD/AGENTS.md /tmp/a" +run $A none "tar -xzf /tmp/a.tar.gz -C $OUT" +run $A none "cargo build" + +echo +[ "$fails" = 0 ] && echo "ALL PASS" || { echo "$fails FAILURES"; exit 1; } diff --git a/.claude/settings.json b/.claude/settings.json index 641ea70b53..a464ca3719 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -73,10 +73,7 @@ "Edit(**/.env.*)" ], "ask": [ - "Bash(rm:*)", "Bash(rmdir:*)", - "Bash(mv:*)", - "Bash(chmod:*)", "Bash(chown:*)", "Bash(truncate:*)", "Bash(shred:*)", diff --git a/docs/validation.md b/docs/validation.md index 3bfd1325ce..050eacd75b 100644 --- a/docs/validation.md +++ b/docs/validation.md @@ -39,6 +39,7 @@ After all code changes are done, run `./update_sqlx.sh` from `backend/` to regen | Modified Flow structures | Also update `openflow.openapi.yaml` | | Changed DB schema | Update `backend/summarized_schema.txt` if needed | | Enterprise file changes | Companion PR in `windmill-ee-private` (see `docs/enterprise.md`) | +| Changed a hook in `.claude/hooks/` | `bash .claude/hooks/test-hooks.sh` — pins which commands prompt | ## When to Write Tests