diff --git a/CLAUDE.md b/CLAUDE.md index e2a900a..d875e9b 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.3.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. Every adjacent word pair is checked, so wrappers (`fvm`, `melos`, `sudo`, `timeout`) and `/path/to/flutter` need no list. The denial names the match and says the whole call was refused. Pinned gaps: `"flutter" test` and `$F test` pass; unquoted `echo flutter test` is denied The first two PreToolUse hooks are plugin-level (defined in `hooks.json`) and share common utilities from `vgv-cli-common.sh`. The following hook is **agent-scoped** — it is declared in the diff --git a/README.md b/README.md index d5981e5..efa690c 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ This plugin includes SessionStart, PreToolUse, and PostToolUse hooks that valida | ---- | ------- | -------- | | **Warn Missing MCP** (`warn-missing-mcp.sh`) | SessionStart | Warns if the Very Good CLI is missing, older than 1.3.0, or present but unable to run because `dart` is not on the `PATH` hooks inherit; non-blocking | | **Check VGV CLI** (`check-vgv-cli.sh`) | PreToolUse (`mcp__.*very-good-cli__.*`) | Verifies from the payload that the caller is a Very Good CLI tool and stands aside otherwise, so the decision never lands on an unrelated tool; for its own tools, auto-approves the call in every run mode via a PreToolUse `allow` decision, so they never dead-end when the tool isn't on `permissions.allow` (including under `skipAutoPermissionPrompt`); denies with an install/upgrade message if the CLI is missing or < 1.3.0 | -| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | Blocks direct CLI bypass of Very Good CLI commands through the host's shell tool; inspects the command only when the payload identifies a shell call, so an unrelated tool carrying a `command` argument is left alone; when the CLI is present but cannot run because `dart` is missing from `PATH`, the denial says so instead of redirecting to an MCP server that cannot start; exits 2 on failure (blocking) | +| **Block CLI Workarounds** (`block-cli-workarounds.sh`) | PreToolUse (`Bash`) | 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. Every adjacent word pair is checked, so wrappers need no list. The denial names the match and says the whole call was refused | | **Allow Read-only Git** (`allow-readonly-git.sh`) | PreToolUse (`Bash`, `flutter-reviewer` agent only) | Restricts the `flutter-reviewer` agent's Bash to single-line `git diff`/`git status`, without the `--output` or `--ext-diff` options, and denies anything else (blocking). Scoped via the agent's frontmatter, not `hooks.json` | | **Analyze** (`analyze.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart analyze` on the modified `.dart` file; exits 2 on failure (blocking — Claude must fix issues before continuing) | | **Format** (`format.sh`) | PostToolUse (`Edit`/`Write`) | Runs `dart format` on the modified `.dart` file; always exits 0 (non-blocking — formatting is applied silently) | diff --git a/config/cspell.json b/config/cspell.json index 99c4f7b..279d32e 100644 --- a/config/cspell.json +++ b/config/cspell.json @@ -33,6 +33,8 @@ "GHSA", "goldens", "gradeable", + "gsub", + "heredocs", "hoverable", "icontains", "idgetbook", @@ -49,6 +51,7 @@ "mocktail", "monorepo", "Mundo", + "nohup", "opencode", "pasteable", "pipefail", @@ -65,6 +68,7 @@ "subagent", "subagents", "subclassing", + "subshell", "tappable", "tooltipped", "unmirrored", diff --git a/hooks/scripts/block-cli-workarounds.sh b/hooks/scripts/block-cli-workarounds.sh index 98f2985..4ca16f2 100644 --- a/hooks/scripts/block-cli-workarounds.sh +++ b/hooks/scripts/block-cli-workarounds.sh @@ -1,7 +1,6 @@ #!/bin/bash -# PreToolUse hook: block Bash commands that bypass MCP tools. -# Denies flutter create, dart create, very_good create, very_good test, -# very_good packages, flutter test, dart test. +# PreToolUse hook: deny shell calls to CLI commands the MCP tools cover. +# To block another command, add a hint line below and a test case. if ! command -v jq &>/dev/null; then echo "jq is required for block-cli-workarounds hook but not found" >&2 @@ -34,54 +33,71 @@ 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. +# 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" } -# Split on shell operators and check the first two tokens of each subcommand. -# This avoids false positives from file paths (.dart) or quoted strings. -BLOCKED=$(echo "$COMMAND" | awk '{ - n = split($0, parts, /[;&|]+/) - for (i = 1; i <= n; i++) { - gsub(/^[[:space:]]+/, "", parts[i]) - split(parts[i], w, /[[:space:]]+/) - b = w[1]; s = w[2] - if ((b == "flutter" || b == "dart") && s == "create") { print "create"; exit } - if ((b == "flutter" || b == "dart") && s == "test") { print "test"; exit } - if (b == "very_good" && s == "create") { print "vg_create"; exit } - if (b == "very_good" && s == "test") { print "vg_test"; exit } - if (b == "very_good" && s == "packages") { print "vg_packages"; exit } +# Quoted text is data unless eval, sh -c or $( ) runs it. Every adjacent word pair is +# checked, so wrappers (fvm, melos, sudo, timeout) and /path/to/flutter need no list. +read -r -d '' find_invocation <<'AWK' || true # read, not $(cat): unbalanced parens inside +BEGIN { + RS = "\001" # whole command is one record + hint["flutter test"] = "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." + hint["dart test"] = hint["flutter test"] + hint["flutter create"] = "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." + hint["dart create"] = hint["flutter create"] + hint["very_good test"] = "Do not use 'very_good test' via shell. Use the very_good_cli MCP 'test' tool instead." + hint["very_good create"] = "Do not use 'very_good create' via shell. Use the very_good_cli MCP 'create' tool instead." + hint["very_good packages"] = "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." +} +# First blocked "cmd sub" pair in s, or "". Extra params are awk locals. +function scan(s, n, w, i, b, pair) { + gsub(/[;&|(){}`\n]+/, " ; ", s) # operators end a command + n = split(s, w, /[[:space:]]+/) + for (i = 1; i < n; i++) { + b = w[i]; sub(/^.*\//, "", b) # basename + pair = b " " w[i + 1] + if (pair in hint) return pair } -}') + return "" +} +{ + gsub(/\\["'$`]/, "_") # escaped: literal + text = $0; gsub(/'[^']*'/, "_", text); gsub(/"[^"]*"/, "_", text) # quotes are data + subst = $0; gsub(/'[^']*'/, "_", subst); gsub(/"/, "", subst) # but "$( )" runs + bare = $0; gsub(/["']/, "", bare) # and eval "" runs + + pair = scan(text) + if (pair == "" && $0 ~ /\$\(|`/) pair = scan(subst) + if (pair == "" && $0 ~ /(^|[^[:alnum:]_])(eval|sh|bash|zsh) /) pair = scan(bare) + if (pair != "") print hint[pair] "\t" pair +} +AWK + +RESULT=$(printf '%s\n' "$COMMAND" | awk "$find_invocation") # printf: echo eats -n -case "$BLOCKED" in - create) deny_with_cli_check "Do not use 'flutter create' or 'dart create'. Use the very_good_cli MCP 'create' tool instead." ;; - test) deny_with_cli_check "Do not use 'flutter test' or 'dart test'. Use the very_good_cli MCP 'test' tool instead." ;; - vg_create) deny_with_cli_check "Do not use 'very_good create' via shell. Use the very_good_cli MCP 'create' tool instead." ;; - vg_test) deny_with_cli_check "Do not use 'very_good test' via shell. Use the very_good_cli MCP 'test' tool instead." ;; - vg_packages) deny_with_cli_check "Do not use 'very_good packages' via shell. Use the very_good_cli MCP 'packages_get' or 'packages_check_licenses' tool instead." ;; -esac +if [ -n "$RESULT" ]; then + deny_with_cli_check "${RESULT%%$'\t'*}" "${RESULT#*$'\t'}" +fi -# Not a blocked command — allow exit 0 diff --git a/hooks/scripts/block-cli-workarounds_test.sh b/hooks/scripts/block-cli-workarounds_test.sh index d5d182e..13a4dbc 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 "" @@ -136,6 +141,136 @@ 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" + +# 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 ---" +# +# 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')" + +# Heredoc bodies are not modelled; denied as a known limitation. +assert_blocked "$(printf 'cat <