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.

[claude]

Reviewed agent/tools/bash.py and the new tests. The argv-based approach is a clear improvement over substring matching, and the scope is clean (only bash.py + tests, matching the PR description). One blocking false positive plus two notes.

Blocking: export / env assignments are rejected as "dumps environment variables" — agent/tools/bash.py:141-150

export and env are in ENV_DUMP_COMMANDS (line 46), and the SECRET_NAME_RE check at line 149 runs over every non-flag arg, so assignment forms are treated as reads:

• export ANTHROPIC_API_KEY=sk-... → named = ["ANTHROPIC_API_KEY=sk-..."], regex matches → blocked
• env GITHUB_TOKEN=xxx ./script.sh → same

Setting a variable isn't exfiltration, and the returned message ("dumps environment variables") misdescribes what the command does. This matters more than a cosmetic false positive because _is_sensitive is a hard block that interactive confirmation cannot lift (lines 176-184): the only escape hatch is auto_approve_bash=true, which disables confirmation for every bash command — a worse posture than the one being prevented. Suggest excluding assignment tokens before the name match:

named = [a for a in args if not a.startswith("-")]
reads = [a for a in named if "=" not in a]
...
if dumps or any(SECRET_NAME_RE.search(a) for a in reads):

printenv ANTHROPIC_API_KEY and bare env / export / declare -p stay blocked, which are the cases the check is for.

Note (non-blocking), lines 152-159: shlex.split strips quotes before VAR_REF_RE runs, so echo 'set $API_KEY first' is blocked even though bash does not expand inside single quotes. Erring safe here is defensible; flagging in case you'd rather track quoting.

Note (non-blocking), line 29: FILE_READ_COMMANDS omits several very common readers — grep, sort, od, xxd, base64, cp — and plain redirection (cat < /etc/shadow has no path in argv at all). The docstring is appropriately honest that this is best-effort rather than a sandbox, but grep . /etc/shadow succeeding while cat /etc/shadow is blocked is worth closing since grep is a one-token addition.

One caveat on verification: python execution was not permitted in my review environment, so the new tests were read for correctness rather than run. CI should confirm them.

[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.

[claude]

Re-reviewed agent/tools/bash.py and the new tests. The two items from my last pass that were addressed look good: set -e/set -x/set -o pipefail now survive via the not args branch (line 174), the ValueError fallback to segment.split() keeps cat /etc/shadow "|" from slipping through (lines 139-145), and the docstring is now honest that this is best-effort rather than a sandbox. The new xtrace guard is a sensible addition and its tests look correct. Scope is still clean — only bash.py plus tests.

Blocking (carried over, still unfixed): assignment forms are rejected as "dumps environment variables" — agent/tools/bash.py:169-177

export and env are in ENV_DUMP_COMMANDS (line 46), and the SECRET_NAME_RE check on line 177 runs over every non-flag arg in named, including NAME=value tokens:

• export ANTHROPIC_API_KEY=sk-... → named == ["ANTHROPIC_API_KEY=sk-..."], regex matches API_KEY → blocked
• env GITHUB_TOKEN=xxx ./script.sh → same
• export GITHUB_TOKEN=$(gh auth token) → same

Setting a variable is not exfiltration, and the message ("dumps environment variables") misdescribes what the command does, so the model gets no usable signal and will retry blindly. This is the hard-block path: _is_sensitive at lines 204-213 rejects outright, and answering y at agent/core/engine.py:167 does not change the tool's auto_approve (it is fixed at construction from config.agent.auto_approve_bash, engine.py:56), so the only escape is setting auto_approve_bash=true globally — which disables confirmation for every bash command, a worse posture than the one being prevented.

The fix from my last pass, unchanged:

named = [a for a in args if not a.startswith("-")]
reads = [a for a in named if "=" not in a]
...
if dumps or any(SECRET_NAME_RE.search(a) for a in reads):

printenv ANTHROPIC_API_KEY, bare env/export, and declare -p all stay blocked — those are the cases the check exists for. Worth adding export ANTHROPIC_API_KEY=sk-test to test_legitimate_commands_not_blocked.

Note (non-blocking), line 29: FILE_READ_COMMANDS still omits common readers, so grep . /etc/shadow, base64 ~/.ssh/id_rsa and cat < /etc/shadow (no path in argv at all) succeed while cat /etc/shadow is blocked. Adding grep, sort, od, xxd, base64 is a one-line change and closes the cheapest bypasses; redirection would need more work and I am not asking for it given the stated threat model.

Note (non-blocking), lines 180-187: shlex.split strips quotes before VAR_REF_RE runs, so echo 'see $API_KEY docs' is blocked even though bash does not expand inside single quotes. Erring safe is defensible; flagging only so it is a deliberate choice.

On verification: python execution was not permitted in this environment, so I read the tests rather than ran them — CI should confirm. The blocking finding follows directly from line 46 and line 177 and does not depend on execution. Fix that one and I am happy to approve.

[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.

[claude]

Re-reviewed agent/tools/bash.py and the new tests. The blocking finding from my last pass is fixed: reads = [a for a in named if "=" not in a] (line 288) means export ANTHROPIC_API_KEY=sk-... and env GITHUB_TOKEN=xxx ./script.sh are no longer treated as dumps, while printenv ANTHROPIC_API_KEY, bare env/export and declare -p still are — and export ANTHROPIC_API_KEY=sk-test is now covered in test_legitimate_commands_not_blocked. The earlier non-blocking note is addressed too: grep, sort, od, xxd, base64 are in FILE_READ_COMMANDS (line 29), so grep . /etc/shadow and base64 ~/.ssh/id_rsa are caught.

I hand-traced the newer logic rather than only reading it. _unwrap_env parses getopt-style and fails closed on unrecognised options, and I checked it against each test case (-uFOO, -iu FOO, -S'FOO=1 printenv', --split-string=, -- FOO=1 printenv, -Z, -a sh); /usr/bin/env python script.py correctly falls through unblocked, which is the important non-regression. The wrapper loop terminates — the WRAPPER_COMMANDS branch always advances by inner >= 1, and each env unwrap consumes the env token so total input shrinks even with -S re-splitting. The xtrace guard only fires when the line also references a secret-looking variable, so bare set -x/set -eu/set -o pipefail stay allowed. Scope is clean: only bash.py plus tests, matching the PR description.

Remaining items are minor and not worth holding the PR: a grep pattern that looks like a sensitive path (grep -rn id_rsa notes.txt) is reported as "reads sensitive file", command -v env resolves to a dump, and command substitution / bash -c / redirection stay out of reach — the docstring is honest that this is best-effort rather than a sandbox. Python execution was not permitted in my review environment, so the tests were traced by hand rather than run; auto-merge will hold until CI confirms.