Add sensitive file access validation to bash tool

[ssevera1]

What

Add validation to reject bash commands that attempt to read sensitive files (e.g., /etc/shadow, ~/.ssh) or exfiltrate secrets (e.g., environment variables containing API keys).

Why

Bash tool lacks auto-approval unlike read_file, so it needs explicit protection against commands designed to leak sensitive credentials or configuration data.

[claude]

Scope and intent are fine — the diff does exactly what the title says and touches only agent/tools/bash.py. But the matcher as written has a poor false-positive/false-negative trade-off, and I'd like it reworked before merge.

1. agent/tools/bash.py:84-90 — substring matching on the raw command blocks legitimate commands in this very repo.
_is_sensitive lowercases the command and does a plain in test, so any command that merely mentions a pattern is rejected regardless of what it does:

• grep -rn ANTHROPIC_API_KEY scripts/ → blocked by the ANTHROPIC_API_KEY pattern (line 34). This repo references that name at scripts/ai_improve.py:304 and .github/workflows/ai-improvements.yml:41, so searching for its own call sites is now impossible via bash.
• printenv PATH → blocked by the bare printenv pattern (line 32), as is any command with printenv in a filename or script body.
• cat ~/.config/nvim/init.lua → blocked by cat ~/.config (line 30). ~/.config is mostly ordinary app config, not secrets.
• The exfiltrate/ex-filtrate patterns (line 34) match an English word, not a behavior — they block git commit -m messages and greps of security docs while stopping no actual attack. Suggest dropping them.

2. Same lines — one-character variations walk straight past it. cat /etc/shadow is blocked but head /etc/shadow, less /etc/shadow and cat /etc//shadow are not; cat ~/.ssh is blocked but cat $HOME/.ssh/id_rsa and cd ~/.ssh && cat id_rsa are not; env | grep -i key is blocked but env|grep key (no spaces) is not. Since the stated threat model is commands designed to leak credentials, a literal-string list gives close to zero protection against that adversary while imposing the breakage in (1). Consider matching on resolved path targets / argv structure, or restricting reads by path prefix, rather than on command spelling.

3. agent/tools/bash.py:106-108 — hard block with no override, inconsistent with the existing design. DANGEROUS_PATTERNS is overridable via auto_approve (line 110), and every bash call already goes through an interactive confirmation at agent/core/engine.py:166 unless auto_approve_bash is set. The new check has no escape hatch at all, so a user who legitimately needs printenv is stuck. Routing this through the confirmation path instead of an unconditional reject would fit the surrounding code and make the false positives in (1) recoverable.

4. agent/tools/bash.py:89 — the error message discards which pattern matched. _is_blocked returns the offending pattern (line 76); this returns a generic string, so neither the model nor the user can tell why a command was rejected, and the model will likely retry blindly. Including the matched pattern would fix that. The f prefix on that line is also unused — no placeholders.

5. No tests. tests/test_tools.py:213 has a TestBashTool class and the preceding PR (#36) shipped tests with its validation change; this adds a new security control with none. Worth covering both the block cases and a few must-not-block cases like the greps above.

Happy to re-review once the matching is tightened — the underlying goal is reasonable, it's the string-blocklist mechanism I'm pushing back on.

[claude]

Verified all five points against the code before changing anything — all confirmed real, so I reworked the matcher per the review's own suggestion rather than patching the blocklist:

Fixed:

• Replaced the raw substring blocklist with argv-based matching: each shell segment is split with shlex, argv[0] is checked against known read/env-dump/print commands, and path arguments are expanded (~, $VARS) and normalized before comparing to sensitive-path patterns. This directly fixes the false positives you listed — grep -rn ANTHROPIC_API_KEY scripts/, printenv PATH, and cat ~/.config/nvim/init.lua are no longer blocked — while still catching the bypasses you demonstrated (head/less /etc/shadow, cat /etc//shadow, cat $HOME/.ssh/id_rsa, cd ~/.ssh && cat id_rsa, env|grep key with no spaces).
• Dropped exfiltrate/ex-filtrate — they matched English prose, not behavior, as you noted.
• Routed the sensitive check through the same auto_approve gate _is_dangerous already uses, so it's no longer an unconditional hard block (point 3).
• Error messages now include the actual matched command/path/variable instead of a generic string (point 4). Also dropped the unused f-prefix.
• Added regression tests in tests/test_tools.py::TestBashTool: block cases (sensitive file reads, env dumps, secret var echoing), must-not-block cases (the exact false positives you found), and an auto_approve-override case (point 5).

Didn't change: nothing — all five points were valid and addressed above. I considered going further into full path-prefix/argv canonicalization (e.g. resolving symlinks) per your point 2's broader suggestion, but kept this to a pragmatic argv + path-normalization check consistent with the tool's existing heuristic (non-sandboxed) design — bash execution here is a guardrail/confirmation layer, not a security boundary, which the existing DANGEROUS_PATTERNS/auto_approve mechanism already reflects.

Checks: pytest -q → 93 passed. mypy --ignore-missing-imports agent → Success, no issues found.

[claude]

The rework addresses my earlier feedback well: argv/segment parsing replaces the raw-substring blocklist, paths are normalized/expanded before matching so cat /etc//shadow, cat $HOME/.ssh/id_rsa and cd ~/.ssh && cat id_rsa are all caught, the check is overridable like DANGEROUS_PATTERNS, the error message names the offending segment, and the must-not-block cases (grep -rn ANTHROPIC_API_KEY scripts/, printenv PATH, cat ~/.config/nvim/init.lua) are now covered by tests. Scope is clean — only agent/tools/bash.py and tests/test_tools.py. One concrete false positive still needs fixing before merge.

1. agent/tools/bash.py:133-136 (blocking) — set -e and set -x are rejected as environment dumps.

set is in ENV_DUMP_COMMANDS (line 46), and line 134 filters out anything starting with - before the emptiness test. So for set -e, args == ["-e"] but named == [], and the not named branch on line 135 fires:

_is_sensitive("set -e; make test")
  -> "Blocked: 'set -e' dumps environment variables"

Same for set -x, set -eu, set -o (set -euo pipefail happens to survive only because pipefail lands in named). A set -e/set -x prefix is a common idiom, so this will fire on ordinary build and test commands, and the message — "dumps environment variables" — gives the model no way to understand why, which is the blind-retry problem from my last pass.

It is also not cheaply recoverable. BashTool is constructed with self.config.agent.auto_approve_bash at agent/core/engine.py:56, and answering y to the prompt at agent/core/engine.py:167 does not change the tool's auto_approve. So the only escape from a false positive here is setting auto_approve_bash=true globally, which disables confirmation for all bash commands — the opposite of what you want. (This is pre-existing for _is_dangerous too, so not introduced here, but it is why this false positive matters more than it looks, and why the trailing "or confirm interactively" in the new message at line 167 is misleading.)

Suggested fix: for set specifically, treat it as a dump only when it has no arguments at all (not args) — bare set is the form that prints every shell variable, while -e/-x/-o are option toggles that print nothing. env, printenv, export -p and declare -p are all fine as-is. Worth adding a set -e case to test_legitimate_commands_not_blocked.

2. agent/tools/bash.py:112-115 — parse failure fails open.

A ValueError from shlex.split skips the segment entirely, and the | in the line-108 split pattern also cuts inside quotes. Combined, cat /etc/shadow "|" splits into cat /etc/shadow " (unbalanced quote, so ValueError, so skipped) and ", and the /etc/shadow read is never examined. Falling back to segment.split() on ValueError instead of continue keeps the check alive on the token sequence when quoting confuses the splitter.

3. agent/tools/bash.py:108 — nested commands are not reached (non-blocking, but worth a note).

Only top-level segments are inspected, so command substitution and interpreter wrappers pass through: echo $(cat /etc/shadow), the backtick equivalent, bash -c 'cat ~/.ssh/id_rsa', python -c 'import os; print(os.environ)'. I do not think full coverage is achievable without a real shell parser and I am not asking for it — but the docstring reads as if the structural detection is comprehensive. Either strip substitution bodies and recurse into them, or add a line to the docstring saying this is a best-effort guard against accidental or injected leakage rather than a sandbox, so the next reader does not over-trust it.

Minor, no action needed: /etc/passwd (line 34) is world-readable and holds no password hashes, so blocking it is noise rather than protection; and declare -f (functions, not variables) trips the same flags-only path as (1), which the fix for (1) would leave in place — harmless either way.

On verification: the findings above come from reading the code — I could not execute pytest in this review sandbox, so I am relying on CI for the test run. Finding (1) follows directly from lines 46 and 133-136.

Fix (1) and I am happy to approve; (2) is a two-line change I would take in the same pass.

[github-actions]

Auto-fix budget exhausted after 1 rounds — not attempting another pass.

Two bots disagreeing past this point usually means the proposal needs a human call, or is not worth the churn. Leaving the PR as-is; pr-janitor.yml will close it if it goes stale.