diff --git a/CLAUDE.md b/CLAUDE.md index 0190449..b4ad15a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,7 @@ These run **when a session begins**: These run **before** a tool call is executed: - `mcp__.*very-good-cli__.*` matcher → `check-vgv-cli.sh` — reads `tool_name` from the payload and exits 0 unless the caller is a Very Good CLI tool, so a host that does not apply the matcher cannot have the decision land on an unrelated tool; for its own tools it auto-approves the call by returning a PreToolUse `allow` decision, so it is always permitted regardless of run mode (interactive, headless, or `skipAutoPermissionPrompt`) and never dead-ends when the tool isn't on `permissions.allow`; denies with an install/upgrade message if the CLI is missing or < 1.5.0, and stands aside when the CLI is present but its version cannot be read (`dart` missing from `PATH`), leaving normal permission handling to apply. The `.*` in the matcher covers both the bare `mcp__very-good-cli__*` server (repo-root `.mcp.json`) and the plugin-namespaced `mcp__plugin__very-good-cli__*` form used when installed from a marketplace -- `Bash` matcher → `block-cli-workarounds.sh` — prevents direct CLI bypass of VGV CLI commands through the host's shell tool (`Bash`, or `Shell` on other hosts); exits 0 when the payload names a non-shell tool, so an unrelated tool carrying a `command` argument is never inspected; when the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so rather than redirecting to an MCP server that cannot start; every denial also says the whole shell call was refused, so the agent re-runs any command chained with the blocked one instead of assuming it ran; exits 2 on failure (blocking) +- `Bash` matcher → `block-cli-workarounds.sh` — denies `flutter`/`dart` `test`/`create` and `very_good test`/`create`/`packages` in the shell and points to the MCP tool. Quoted text is data unless `eval`, `sh -c` or `$( )` runs it. A heredoc body with a quoted delimiter (`<<'EOF'`) is data too, unless the opening line feeds it to a shell (`bash <<'EOF'`, `cat <<'EOF' | sh`), which runs it; an unquoted `</dev/null; then - echo "jq is required for block-cli-workarounds hook but not found" >&2 - exit 1 -fi +# Fail loudly rather than stand aside: a missing tool here would let every shell call +# through unchecked, and silently. +for tool in jq tr; do + if ! command -v "$tool" &>/dev/null; then + echo "$tool is required for block-cli-workarounds hook but not found" >&2 + exit 1 + fi +done SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "$SCRIPT_DIR/vgv-cli-common.sh" @@ -34,54 +37,203 @@ fi # assumes the chained command's side effect happened and carries on without it. WHOLE_CALL_REFUSED="This whole shell call was refused, so none of it ran: run any other commands it chained in a call of their own." -# Deny with an install/upgrade message when the CLI is missing or outdated, with a PATH -# message when it is present but cannot run, and otherwise redirect to the MCP tool. +# #60 and #147 were both reports of a denial naming an action the operator never took. +# Saying where the match can come from turns a surprising denial into a legible one. +# Deliberately not phrased as "quote it and it will pass": `"flutter" test` still runs +# the command, so printing that remedy would hand out a bypass with every denial. +PAIR_MAY_BE_AN_ARGUMENT="The match is on an adjacent command and subcommand anywhere in the call, so an argument that reads like one is refused too." + +# Deny with a reason that fits the CLI status. deny_with_cli_check() { - local mcp_hint="$1" - local cli_status + local mcp_hint="$1" matched="$2" + local cli_status reason cli_status=$(check_vgv_cli) case "$cli_status" in not_installed) - deny "Very Good CLI is required but was not found. Install with: dart pub global activate very_good_cli. $WHOLE_CALL_REFUSED" + reason="Very Good CLI is required but was not found. Install with: dart pub global activate very_good_cli." ;; outdated:*) - local version="${cli_status#outdated:}" - deny "Very Good CLI ${version} is too old (requires >= ${MIN_VERSION}). Update with: dart pub global activate very_good_cli. $WHOLE_CALL_REFUSED" + reason="Very Good CLI ${cli_status#outdated:} is too old (requires >= ${MIN_VERSION}). Update with: dart pub global activate very_good_cli." ;; unverifiable) # Redirecting to the MCP tool here would be a dead end: the server starts through # the same very_good shim, which cannot exec dart from this PATH either. - deny "Very Good CLI was found but could not run: dart is not on the PATH available to hooks, so the very_good_cli MCP server cannot start either. Add the Dart SDK bin directory to PATH for non-interactive shells (e.g. in ~/.zprofile) and start a new session. $WHOLE_CALL_REFUSED" + reason="Very Good CLI was found but could not run: dart is not on the PATH available to hooks, so the very_good_cli MCP server cannot start either. Add the Dart SDK bin directory to PATH for non-interactive shells (e.g. in ~/.zprofile) and start a new session." ;; *) - deny "$mcp_hint $WHOLE_CALL_REFUSED" + reason="$mcp_hint" ;; esac + deny "$reason $WHOLE_CALL_REFUSED Matched: $matched. $PAIR_MAY_BE_AN_ARGUMENT" +} + +# --- what the shell would treat as a command --------------------------------- +# +# No lexer, and no list of wrappers to maintain. Quoted text counts as data, except +# where the shell would still run what is inside it, and every adjacent word pair is +# tested against its basename. + +INERT="_" # stands in for text the shell will not run +ANY_QUOTE="[\"']" + +# A backslash-escaped quote, $ or ` is literal text, not a quote or an expansion. +escape_literals() { + printf '%s' "${1//\\[\"\'\$\`]/$INERT}" +} + +# A heredoc whose delimiter is quoted (<<'X', <<-"X") expands nothing, so its body is +# data. An unquoted < . A kept span keeps its +# contents, because the shell would still run them; every other span becomes INERT. +collapse_quotes() { + local rest=$1 keep_single=$2 keep_double=$3 + local out="" before quote body keep + + while [ -n "$rest" ]; do + before=${rest%%$ANY_QUOTE*} + if [ "$before" = "$rest" ]; then # nothing quoted is left + out+=$rest + break + fi + + out+=$before + rest=${rest#"$before"} + quote=${rest:0:1} + rest=${rest:1} + + if [[ $rest != *"$quote"* ]]; then # unbalanced: leave the remainder alone + out+="$quote$rest" + break + fi + + body=${rest%%"$quote"*} + rest=${rest#"$body$quote"} + + if [ "$quote" = "'" ]; then keep=$keep_single; else keep=$keep_double; fi + if [ "$keep" = 1 ]; then out+=$body; else out+=$INERT; fi + done + + printf '%s' "$out" +} + +# The three readings of one call, in the order they are tried. +as_written() { collapse_quotes "$1" 0 0; } # quotes are data +as_substitution() { collapse_quotes "$1" 0 1; } # but "$( )" runs +as_eval_target() { collapse_quotes "$1" 1 1; } # and eval "" runs + +runs_substitution() { + case "$1" in *'$('* | *'`'*) return 0 ;; esac + return 1 +} + +runs_its_argument() { + local re='(^|[^[:alnum:]_])(eval|sh|bash|zsh) ' + [[ $1 =~ $re ]] +} + +# --- matching ---------------------------------------------------------------- + +# Prints the first " " pair the MCP tools cover, or nothing. +# The command is matched on its basename, so /usr/local/bin/flutter counts as flutter +# and wrappers (fvm, melos, sudo, timeout) need no list of their own. +blocked_pair() { + local -a words + local line i pair + + # tr, not ${1//$OPERATOR/ }: bash 3.2 runs a bracket-class // over a long string in + # quadratic time, which costs seconds on a command a few kilobytes long. Operators + # become line breaks, and a pair is only ever read within one line, so no pair can + # bridge the operator that separates two commands. + while IFS= read -r line || [ -n "$line" ]; do # the last line carries no newline + read -ra words <<< "$line" + + for ((i = 0; i < ${#words[@]} - 1; i++)); do + pair="${words[i]##*/} ${words[i + 1]}" + + case "$pair" in + "flutter test" | "dart test" | "flutter create" | "dart create" | \ + "very_good dart" | "very_good test" | "very_good create" | "very_good packages") + printf '%s' "$pair" + return 0 + ;; + esac + done + done < <(printf '%s' "$1" | tr ';&|(){}`' '\n') + + return 1 +} + +hint_for() { + case "$1" in + "flutter test" | "dart test") + printf '%s' "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." ;; + "flutter create" | "dart create") + printf '%s' "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." ;; + # Scanning left to right reaches this before the `dart test` inside it, so the + # denial names the command that was actually run. `very_good dart` has one subcommand. + "very_good dart") + printf '%s' "Do not use 'very_good dart test' via shell. Use the very_good_cli MCP 'test' tool instead." ;; + "very_good test") + printf '%s' "Do not use 'very_good test' via shell. Use the very_good_cli MCP 'test' tool instead." ;; + "very_good create") + printf '%s' "Do not use 'very_good create' via shell. Use the very_good_cli MCP 'create' tool instead." ;; + "very_good packages") + printf '%s' "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." ;; + esac +} + +CALL=$(escape_literals "$COMMAND") +CALL=$(strip_quoted_heredocs "$CALL") + +PAIR=$(blocked_pair "$(as_written "$CALL")") + +if [ -z "$PAIR" ] && runs_substitution "$CALL"; then + PAIR=$(blocked_pair "$(as_substitution "$CALL")") +fi + +if [ -z "$PAIR" ] && runs_its_argument "$CALL"; then + PAIR=$(blocked_pair "$(as_eval_target "$CALL")") +fi + +if [ -n "$PAIR" ]; then + deny_with_cli_check "$(hint_for "$PAIR")" "$PAIR" +fi + exit 0 diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index d5d182e..3c8ab2e 100755 --- a/hooks/scripts/block-cli-workarounds_test.sh +++ b/hooks/scripts/block-cli-workarounds_test.sh @@ -66,14 +66,17 @@ run_hook() { run_hook_payload "$(jq -n --arg c "$1" '{"tool_input":{"command":$c}}')" } +# Show newlines as ~ so multi-line commands keep the columns aligned. +label_of() { printf '%s' "$1" | tr '\n' '~'; } + assert_blocked() { local cmd="$1" run_hook "$cmd" if [ "$LAST_RESULT" = "deny" ]; then - printf " \033[32mPASS\033[0m blocked: %s\n" "$cmd" + printf " \033[32mPASS\033[0m blocked: %s\n" "$(label_of "$cmd")" PASSED=$((PASSED + 1)) else - printf " \033[31mFAIL\033[0m expected deny but got %s: %s\n" "$LAST_RESULT" "$cmd" + printf " \033[31mFAIL\033[0m expected deny but got %s: %s\n" "$LAST_RESULT" "$(label_of "$cmd")" FAILED=$((FAILED + 1)) fi } @@ -82,10 +85,10 @@ assert_allowed() { local cmd="$1" run_hook "$cmd" if [ "$LAST_RESULT" = "aside" ]; then - printf " \033[32mPASS\033[0m allowed: %s\n" "$cmd" + printf " \033[32mPASS\033[0m allowed: %s\n" "$(label_of "$cmd")" PASSED=$((PASSED + 1)) else - printf " \033[31mFAIL\033[0m expected aside but got %s: %s\n" "$LAST_RESULT" "$cmd" + printf " \033[31mFAIL\033[0m expected aside but got %s: %s\n" "$LAST_RESULT" "$(label_of "$cmd")" FAILED=$((FAILED + 1)) fi } @@ -104,6 +107,8 @@ assert_reason_contains() { fi } +# Edit with Write/Edit, not a heredoc: the hook reads the heredoc body and denies it. + echo "=== block-cli-workarounds tests ===" stub_cli 1.5.0 echo "" @@ -116,6 +121,10 @@ assert_blocked "dart create my_app" assert_blocked "flutter create my_app" assert_blocked "very_good create flutter_app --project-name my_app" assert_blocked "very_good test --coverage --min-coverage 100" +# `very_good dart` runs the Dart test suite: `very_good --help` describes it, and the +# `very_good test` entry points at it. Issue #147 lists it as allowed, which is an error +# in that table -- allowing it is a bypass, and the old script had one here. +assert_blocked "very_good dart test --coverage" assert_blocked "very_good packages check licenses" assert_blocked "cd /path && dart test" assert_blocked "ENV=1 && flutter test --coverage" @@ -128,14 +137,177 @@ assert_allowed "dart pub get" assert_allowed "dart fix --apply" assert_allowed "flutter pub get" assert_allowed "flutter analyze" +# The three repro commands from issue #60: a .dart path followed by a test/ path. assert_allowed "git add lib/router.dart test/router_test.dart" assert_allowed "dart analyze lib/foo.dart test/bar_test.dart" +assert_allowed "git commit -- lib/foo.dart test/bar.dart" assert_allowed "git commit -m 'fix dart test hook'" assert_allowed "echo 'flutter create is blocked'" assert_allowed "gh pr create --body 'use dart test instead'" assert_allowed "git log --grep='dart test'" assert_allowed "ls" assert_allowed "pwd" +assert_allowed "" +assert_allowed " " + +echo "" +echo "--- Quoted text is data (issue #147) ---" + +# A quoted `|` is not a pipe. Both positions, since the bug only fired mid-alternation. +assert_allowed "grep -nE \"very_good|flutter test|foo\" CLAUDE.md" +assert_allowed "grep -nE \"very_good|flutter test\" CLAUDE.md" +assert_allowed "grep -nE \"flutter test|very_good|foo\" AGENTS.md" +assert_allowed "rg 'flutter create|dart create' docs/" + +# A quoted phrase is a single word and runs nothing. +assert_allowed "echo \"flutter test\"" +assert_allowed "echo 'flutter test'" +assert_allowed "git commit -m \"ban flutter test | dart test\"" + +# One quote type is inert inside the other. +assert_allowed "echo 'say \"flutter test\" now'" +assert_allowed "echo \"say 'flutter test' now\"" + +# Escapes and unbalanced quotes. +assert_allowed "echo \\\"flutter test\\\"" +assert_allowed "grep \"flutter test file.md" +assert_blocked "flutter test --name \"my app\"" + +# A quoted string may span newlines and is still one word. +assert_allowed "$(printf 'echo "hello\nflutter test\nworld"')" + +echo "" +echo "--- Command position ---" + +# Unquoted separators open a command position. +assert_blocked "echo hi; flutter test" +assert_blocked "echo hi | flutter test" +assert_blocked "echo hi & flutter test" +assert_blocked "test -d lib || flutter test" + +# A prefix is still an invocation. +assert_blocked "ENV=1 flutter test" +assert_blocked "CI=true COVERAGE=1 dart test" +assert_blocked "(flutter test)" +assert_blocked "\$(flutter test)" +assert_blocked "\`flutter test\`" +assert_blocked "echo start && (very_good test)" + +# Wrappers need no list: every adjacent word pair is checked. +assert_blocked "fvm flutter test" +assert_blocked "command flutter test" +assert_blocked "env flutter test" +assert_blocked "env FOO=1 flutter test" +assert_blocked "env -i flutter test" +assert_blocked "sudo flutter test" +assert_blocked "sudo -u ci flutter test" +assert_blocked "nohup flutter test" +assert_blocked "exec flutter test" +assert_blocked "time flutter test" +assert_blocked "timeout 60 flutter test" +assert_blocked "nice -n 10 flutter test" +assert_blocked "xargs flutter test" + +# melos runs commands across a VGV monorepo. +assert_blocked "melos exec -- flutter test" +assert_blocked "melos exec --concurrency 1 -- dart test" + +# Shell keywords and brace groups are separators. +assert_blocked "if true; then flutter test; fi" +assert_blocked "for f in a; do flutter test; done" +assert_blocked "{ flutter test; }" +assert_blocked "while :; do dart test; done" + +# Match on the basename. +assert_blocked "/usr/local/bin/flutter test" +assert_blocked "./flutter test" +assert_blocked "\$FLUTTER_ROOT/bin/flutter test" +assert_blocked "../sdk/bin/dart test" + +# ...but only when the wrapped command is itself blocked. +assert_allowed "fvm flutter pub get" +assert_allowed "command -v flutter" +assert_allowed "env | grep PATH" +assert_allowed "melos exec -- dart analyze" +assert_allowed "timeout 60 dart pub get" +assert_allowed "/usr/local/bin/flutter analyze" + +# A wrapper with nothing after it has no pair to match. +assert_allowed "fvm" +assert_allowed "env -i" +assert_allowed "ENV=1" + +# A basename that only resembles the command. +assert_allowed "git add lib/router.dart test/router_test.dart" +assert_allowed "cp foo.dart test/" +assert_allowed "ls bin/flutter_tools" + +# Comments are not modelled. `; dart test` inside one is denied: accepted, since the +# agent does not write comments in tool calls. +assert_blocked "$(printf '# a comment\nflutter test')" +assert_blocked "$(printf 'ls # note\ncd pkg\nflutter test')" +assert_allowed "$(printf '# it%ss broken\nls -la' "'")" +assert_blocked "ls # fix; dart test" + +# `$( )` and backticks run inside double quotes. Single quotes and `\$` are inert. +assert_blocked "OUT=\"\$(flutter test)\"" +assert_blocked "echo \"\$(flutter test)\"" +assert_blocked "echo \"\`flutter test\`\"" +assert_blocked "if [ -z \"\$(dart test)\" ]; then echo x; fi" +assert_allowed "echo \"\\\$(flutter test)\"" +assert_allowed "echo '\$(flutter test)'" +assert_allowed "echo \"\$(date) building\"" +assert_allowed "VAR=\"\$(ls)\"; dart analyze" + +# Quoted spans are consumed left to right, so an apostrophe inside "..." cannot open +# a span of its own and hide what follows it. +assert_blocked "echo \"don't\" && flutter test && echo 'x'" +assert_blocked "git commit -m \"it's\" && flutter test && git commit -m \"don't\"" +assert_blocked "echo \"it's fine\"; dart test" +assert_allowed "echo \"don't\" && ls && echo 'x'" + +# eval and sh -c run their string. +assert_blocked "eval \"flutter test\"" +assert_blocked "bash -c 'flutter test'" +assert_blocked "sh -c \"flutter test\"" +assert_blocked "zsh -c \"cd pkg && dart test\"" + +echo "" +echo "--- Documented non-goals ---" +# +# Denied on purpose and pinned. The pair is matched anywhere in the call, so a command +# taking flutter or dart as an argument is refused too. Narrowing the match to a command +# position would need a list of wrappers, and fvm and melos are exactly what has to stay +# caught, so this is the cheaper side of the trade. +assert_blocked "echo flutter test" +assert_blocked "man flutter test" +assert_blocked "which flutter test" + +# Allowed on purpose and pinned. A variable needs execution to resolve; the rest are +# forms nobody types. +assert_allowed "F=flutter; \$F test" +assert_allowed "\"flutter\" test" +assert_allowed "'dart' test" +assert_allowed "$(printf 'flutter \\\ntest --coverage')" + +# A quoted delimiter stops the shell expanding the body, so it is data even inside +# "$( )", the usual way a commit message or PR body is passed. +assert_allowed $'git commit -m "$(cat <<\'EOF\'\nfix: stop running flutter test by hand\nEOF\n)"' +assert_allowed $'gh pr create --body "$(cat <<\'EOF\'\nThis PR makes flutter test unnecessary.\nEOF\n)"' + +# A quoted delimiter stops expansion, not execution. When the body is fed to a shell it +# is a script, so it stays in place and is scanned. +assert_blocked $'bash <<\'EOF\'\nflutter test\nEOF' +assert_blocked $'cat <<\'EOF\' | sh\nflutter test\nEOF' +assert_blocked $'sh <<\'X\'\nflutter test\nX' +assert_blocked $'bash <<-\'EOF\'\n\tflutter test\n\tEOF' +assert_blocked $'/bin/bash <<\'EOF\'\nflutter test\nEOF' + +# ...but only a shell runs the body. `push` is not `sh`. +assert_allowed $'git push <<\'EOF\'\nflutter test\nEOF' + +# An unquoted delimiter does expand $( ), so its body is still scanned and denied. +assert_blocked "$(printf 'cat <