Repository navigation
fix(tool): apply external_directory: deny to every literal path a bash command names - #1646
RaghwendraSingh wants to merge 4 commits into
Conversation
…and names The bash tool only checked a fixed list of file commands (`cat`, `ls`, `cd`, `rm`, ...) against `external_directory`, so under a configured deny a command could still read, write or execute outside the project: `head /abs/file`, `echo x > /tmp/x`, `/host/.venv/bin/python`, `cd /outside && ...`. - When the effective `external_directory` rule for `*` is `deny`, also check absolute, `~` and `..` arguments, a command run by path, redirection targets and every directory a command runs in, using the existing tree-sitter walk - A denied path refuses the call with an error naming the path; a path the config allows (or sets to `ask`) goes through the normal permission request - Permitted out-of-project paths are recorded as `metadata.external_paths` - Unset, the default `ask`, and `allow` keep today's behaviour unchanged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Mock-gateway capture:
|
| # | command | main |
this branch |
|---|---|---|---|
| 1 | echo inside > notes.md (inside the project) |
executed | executed |
| 2 | cat /etc/hosts |
refused (old command-list check) | refused, names /private/etc/hosts |
| 3 | ls ~ |
refused (old check) | refused, names the home directory |
| 4 | echo x > /tmp/sandbox-evidence-leak.txt |
executed — file written outside the project | refused, names /private/tmp/sandbox-evidence-leak.txt; file absent afterwards |
| 5 | cd <sibling> && cat marker.txt |
refused (old check) | refused |
| 6 | <venv>/bin/python --version |
executed — printed FAKE_VENV_PYTHON |
refused, names the interpreter path |
| 7 | cat <sibling>/marker.txt |
refused (old check) | refused |
So main already blocks the file commands it knows about (cat, ls, cd), with a generic "rule prevents this tool call" error; what escaped were redirection targets and programs run by path, which is exactly what our sandboxed headless runs did (/tmp scratch files, the host checkout's .venv/bin/python). This branch refuses those too, with an error that names the offending path, and does not touch the in-project command.
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Requesting changes. The guard is a real improvement, but literal paths still escape and in-project commands get refused.
Checked at head f9c19c74f9:
- The 20 new tests pass. On the base, 13 of the 17 tool tests fail and the 4 unchanged-behaviour tests pass, as the PR says.
test/session,test/toolandtest/agenton a heavily loaded machine: no test fails on head that does not also fail on the base (54 on head, 55 on the base).- When the rule is unset,
askorallow, all new code sits behindguarded, so requests and metadata match main. - The redirection and program-by-path cases from the mock-gateway table are refused with the path named.
- Also refused when run: quoted paths,
2>,&>,<,<>, heredoc redirects, process and command substitution,find -exec, literalxargsarguments, and the/privateand/System/Volumes/Dataaliases. - URLs, git remotes, heredoc bodies, the tool-output directory and allowed scratch directories still work.
Why changes are needed:
- Writes outside the project still run through a redirect after
cd, words after a redirection, option values and in-project symlinks. - The host interpreter still runs through an assignment.
- Common commands such as
cd a && head ../inside.txtare now refused. - The guard appears to do nothing on Windows.
Some of these forms overlap limits the PR lists. Each comment says where. Fixing them or narrowing the title and docs are both acceptable for those; the redirect cwd, cd false refusal, trailing-word and Windows findings need code changes.
None of the sibling PRs (#1631, #1632, #1639, #1641, #1643) touch bash.ts or the permission code, so I see no conflict. Docs and nits are in a separate PR comment.
| if (node.type === "file_redirect") { | ||
| const destination = node.childForFieldName("destination") ?? node.namedChildren.at(-1) ?? null | ||
| const target = literalWord(destination) | ||
| if (target && namesPath(target)) candidates.add(path.resolve(cwd, target)) |
There was a problem hiding this comment.
Major: redirection targets are resolved against the initial cwd, so .. targets are wrong in both directions.
Run at head under external_directory: deny:
- Workdir
<project>/a/b:cd ../.. && echo pwn > ../markerexecuted and wrote next to the project. The target was judged as<project>/a/marker. cd a && echo ok > ../r.txtlands inside the project but is refused, naming the project's parent directory.
Fix: resolve each file_redirect against the cwds of the command that owns it, for example by collecting redirects inside the shellCommands walk instead of a separate stack walk.
There was a problem hiding this comment.
Fixed in 1dd20cd: redirects are collected in the guard's own walk and resolved in the owning command's directory (the parser attaches a && b > f to the list, so it goes to the last command); both of your cases are tests.
| const arg = literalWord(word) | ||
| // Option values (`--out=/x`) and URLs are not read as paths. | ||
| if (!arg || arg.startsWith("-") || !namesPath(arg)) continue | ||
| for (const commandCwd of cwds) candidates.add(path.resolve(commandCwd, arg)) |
There was a problem hiding this comment.
Major: .. arguments after a cd inside the project are refused, a regression under deny.
afterCd keeps the previous cwd because cd may fail, and each candidate cwd is checked here. That only added aliases before; it is now a hard refusal for every command.
Run at head, all refused although they stay in the project, and head ran on main:
cd a && head -c 3 ../inside.txtcd a && sh ../inside.txt(cd a && head ../inside.txt)cd a && ../nope.sh
cd src && python ../script.py is an ordinary form for a sandboxed run. Fix: after cd x && the earlier cwd is unreachable, so drop it for this check. Add a test.
There was a problem hiding this comment.
Fixed in 1dd20cd: after cd x && only x is judged (;/|| keep the earlier directory); your four forms plus cd a && echo ok > ../r.txt are allowed-case tests.
| while (stack.length) { | ||
| const node = stack.pop()! | ||
| if (node.type === "file_redirect") { | ||
| const destination = node.childForFieldName("destination") ?? node.namedChildren.at(-1) ?? null |
There was a problem hiding this comment.
Major: words after a redirection are never checked, so a literal outside path gets through.
file_redirect.destination is multiple: true in the tree-sitter-bash grammar, and childForFieldName returns only the first word. Later words are real arguments at run time.
Run at head:
cat > stolen.txt /outside/s.txtran and copied the outside file into the project.head > /dev/null /outside/s.txtran.
This ordering is a small rewrite away from a form the guard does refuse. Fix: treat every destination child as a candidate, and add the case to the tests.
There was a problem hiding this comment.
Fixed in 1dd20cd: every file_redirect destination word is a candidate; cat > stolen.txt /outside/s.txt and head > /dev/null /outside/s.txt are refusal tests.
| const candidates = new Set<string>() | ||
| for (const { node, cwds } of shellCommands(root, [cwd])) { | ||
| for (const commandCwd of cwds) candidates.add(commandCwd) | ||
| const words = shellWords(node, true) |
There was a problem hiding this comment.
Major: assignment values are not scanned, so the host interpreter still runs.
Run at head:
PY=/outside/py; $PYexecuted the outside program.export PATH=/outside:$PATH; pyexecuted it too.export FOO=/outside/s.txt; head "$FOO"read the file.
The PR lists OUT=/tmp/x cmd as a limit, so this is a known gap. I still rate it major: the path is literal in the command text, and the outcome is the one issue #1645 sets out to stop (running a host checkout's .venv/bin/python). Fix: scan variable_assignment values, including those under declaration_command, with the same namesPath check. Split PATH-style values on :.
There was a problem hiding this comment.
Fixed in 1dd20cd: variable_assignment values (prefix, standalone, export/declare) are checked, split on :; your three forms plus OUT=/x sh -c … are tests.
| for (const word of words.slice(1)) { | ||
| const arg = literalWord(word) | ||
| // Option values (`--out=/x`) and URLs are not read as paths. | ||
| if (!arg || arg.startsWith("-") || !namesPath(arg)) continue |
There was a problem hiding this comment.
Major: option values skip the check, so writes and reads outside the project still run.
Run at head:
sort --output=/outside/w inside.txtwrote outside.sort -o/outside/w inside.txtwrote outside.dd if=inside.txt of=/outside/wwrote outside.dd if=/outside/s.txtread outside.
The separate form sort -o /outside/w is refused, so the outcome depends on spelling. The PR lists --out=/x as skipped, but not -o/x or of=/x, and the title says every path. The test at echo --out=/etc/hosts also locks in the skip. Fix: check the value after = in --x=v and key=v, and the path part of -oPATH. The first-segment check already filters non-paths.
There was a problem hiding this comment.
Fixed in 1dd20cd: values of --x=v, key=v and attached -oPATH are checked; your four forms are tests, and the echo --out=/etc/hosts allow case is now a refusal.
| } | ||
| } | ||
| for (const word of words.slice(1)) { | ||
| const arg = literalWord(word) |
There was a problem hiding this comment.
Minor, not a blocker (edge input): two literal spellings are not recognised.
Run at head:
head $'/outside/s.txt'read the file, and$'/outside/py'executed it.shellWordshas noansi_c_stringtype.head ~root/xwas not checked, becauseliteralWordreturns nothing for~user.
Fix: handle ansi_c_string when it has no escapes, and refuse or expand ~user.
There was a problem hiding this comment.
Fixed in 1dd20cd: $'…' without escapes is read as literal; ~user for another user is refused (the current user's is expanded); tests for both.
| for (const target of commandPathCandidates(root, cwd)) { | ||
| const real = await realpathThroughParents(target) | ||
| if (!real || isDevice(real)) continue | ||
| // Text whose first segment does not exist (an awk or sed program such as `/x/ {print}`) |
There was a problem hiding this comment.
Minor, not a blocker (edge input, fails closed and names the path): a regex starting with an existing top-level directory is refused.
Run at head, all refused:
awk '/var/ {print}' fsed -n '/usr/p' fgrep -c /etc/ f
These run on main under deny. The first-segment check only covers programs like /x/, and the test only uses /inside/. Fix: do not treat the program operand of awk or sed, or words holding spaces and {, as paths. Add a /var/ test.
There was a problem hiding this comment.
Fixed in 1dd20cd: the program/pattern operand (and -e value) of awk/sed/grep/rg is not a path; your three forms plus grep -e /usr/ are allowed-case tests. Words with spaces stay checked so quoted outside paths are refused.
| const named = await externalCommandPaths(tree.rootNode, cwd) | ||
| // A directory the operand check found is listed only when no named path inside it is. | ||
| const found = [ | ||
| ...Array.from(directories) |
There was a problem hiding this comment.
Minor, not a blocker (pre-existing on main under deny): the device exemption does not reach the old operand list.
Run at head, all refused: cat /dev/null, cat /dev/null > f, ls /dev/null. The operand check adds /dev to directories, and this filter keeps it because the device was already removed from named.
The PR says standard devices are exempt, and cat /dev/null > f is a common way to truncate a file. Fix: leave device paths out of directories when guarded.
There was a problem hiding this comment.
Fixed in 1dd20cd: under deny the guard replaces the operand check's directories, so devices are never requested; cat /dev/null, cat /dev/null > f and ls /dev/null are allowed-case tests (ask mode still requests /dev/* as on main, also tested).
| if (name?.includes("/")) { | ||
| for (const commandCwd of cwds) { | ||
| const target = path.resolve(commandCwd, name) | ||
| if (!SYSTEM_BIN_DIRS.has(path.dirname(target))) candidates.add(target) |
There was a problem hiding this comment.
Minor, not a blocker (edge input, fails closed): the system-bin exemption applies only in program position.
Run at head, all refused: nohup /usr/bin/true, /usr/bin/env /usr/bin/true, command -v /usr/bin/true. /usr/bin/env true runs.
Fix: skip a system-bin path in the argument loop too when the program is a known wrapper such as env, nohup, command, time or xargs.
There was a problem hiding this comment.
Fixed in 1dd20cd: wrappers (env, nohup, command, time, xargs, …) are followed to the program, and a system-bin argument is skipped for them; your three forms are tests.
| ["home directory listing", () => `ls ~`, undefined], | ||
| ["home-relative read", () => `cat ~/.profile-that-may-not-exist`, undefined], | ||
| ["cd outside the project", (o: string) => `cd ${o}/sibling && echo hi`, /sibling/], | ||
| ["redirection outside the project", (o: string) => `echo x > ${o}/written.txt`, /written\.txt/], |
There was a problem hiding this comment.
Minor, not a blocker (test-only): the refusal tests miss most forms this guard is meant to cover.
There is no refusal case for 2>, &>, <, <>, a quoted path with spaces, an assignment, --flag=, a symlink inside the project, or a redirect after cd, and nothing covers an in-project .. after cd.
The home-directory cases match only the home basename. In test/agent/bash-external-directory-gate.test.ts:63, the test named "allow or ask" never exercises allow: its config is *: ask plus a /srv deny. Please add these along with the fixes above.
There was a problem hiding this comment.
Added in 1dd20cd: refusal tests for 2>, &>, <, <>, a quoted path with spaces, assignments, --flag=, symlinks and redirect after cd, an allowed in-project .. after cd, and a real allow case in the agent gate test.
|
Findings that do not fit a diff line, plus nits, from the review of this PR at Minor, docs (not a blocker on its own, safe in a follow-up once behaviour settles)
Nits
|
…, every literal form Address review on the bash path guard. Under `external_directory: deny` the guard now walks the script itself: - redirections resolve in the directory of the command that owns them (`a && b > f` applies to `b`), and every word of a `file_redirect` destination is checked (`cat > out /outside/x`) - `cd x && y` runs `y` only in `x`, so in-project `..` after `cd` is no longer refused; `cd x; y` / `||` keep the earlier directory possible; `pushd`/`popd` move the tracked directory like `cd` - assignment values (prefix, standalone, `export`/`declare`), split on `:` for `PATH`-style values - option values: `--x=v`, `key=v` (`dd of=`), attached `-oPATH` - literal `sh|bash|zsh|dash|ksh -c` strings and `eval` are parsed with the same parser (depth cap 3; deeper is refused) - relative words are resolved through symlinks, so an in-project link to outside is judged by where it points - Windows: the first-segment check takes the segment after the drive root (`firstSegmentPath`, unit-tested with `path.win32`) - `for` values, `[[ ]]` words, `case` words, `$'...'` without escapes; `~user` for another user is refused - awk/sed/grep program or pattern operands are not paths; devices are left out of the request; a wrapper's system-binary argument (`nohup /usr/bin/true`) is not an escape - a not-yet-created allowed directory is matched as itself - at most 2000 candidates per command, and `ctx.abort` is honoured - the external_directory request reuses `directoryGlob` Unset, `ask` and `allow` keep the main-branch behaviour: all new code stays behind `guarded`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- `security-faq.md`: what the deny check covers, that skill/temp directories lose their default allow under a `*` deny, and the limits (literal text only; guard rail, not a sandbox) - `permissions.md`: scope and limits next to the strict example Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
RaghwendraSingh
changed the title
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Addressed review in 1dd20cd (guard v2) and 1cd6c84 (docs): redirect cwd, |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Issue for this PR
Closes #1645
Type of change
What does this PR do?
The
bashtool appliedexternal_directoryonly to a fixed list of file commands (cd,rm,cp,mv,mkdir,touch,chmod,chown,cat,ls) and toworkdir. Underpermission: { external_directory: "deny" }any other command could still read, write or execute outside the project (head /abs/file,echo x > /tmp/x,/host/.venv/bin/python, …).Code changes are in
src/tool/bash.ts, insidealtimate_changemarkers, and reuse the existing tree-sitter parser,literalWordandrealpathThroughParents.When it engages. Only when the effective
external_directoryrule for*in the call's ruleset (ctx.permissions) isdeny. Unset, the defaultaskandallowskip the new code entirely: same permission requests, same outputs, same metadata keys asmain. Among the built-in agents the guard is on only for agents that already deny bash (analyst, data-explorer, compaction, summary, title). A test checks this against the real agent definitions.What it checks (v2, after review). The guard walks the script itself and tracks the directory each command runs in:
cd x && yrunsyonly inx;cd x; yandcd x || ykeep the earlier directory possible;pushd/popdmove it likecd; subshells, pipeline stages and substitutions keep changes local. Every literal path the command text names is resolved in its command's directory and through symlinks:--opt=v,key=v(dd of=) and attached-oPATH, and relative words (so an in-project symlink pointing outside is caught)env,nohup,command,time,xargsare followed), except standard system bin dirscat > out /x), resolved in the directory of the command that owns themexport/declare), split on:forPATH-style valuesforvalues,[[ ]]words,casewords,$'…'without escapes;~userfor another user is refusedsh|bash|zsh|dash|ksh -c '…'strings andeval "…", parsed with the same parser (depth cap 3; deeper is refused)cd/pushdtargets,workdir)Not treated as paths: the program or pattern operand of awk/sed/grep/rg (and
-evalues), standard devices (/dev/null, …), a wrapper's system-binary argument, text whose first path segment does not exist. The decision uses the same rules the file tools use (PermissionEvaluate.evaluateResolvedon<dir>/*), so configured allow-lists such as the tool-output directory or an allowed scratch dir keep working; a not-yet-created directory is also matched as itself.Decision.
deny: refused before anything runs, withexternal_directory is denied: this command touches a path outside the project (<root>): <path>. Use paths inside the project instead.alloworaskrule for that directory: added to the existingexternal_directoryrequest, soaskprompts through the normal permission path.metadata.external_paths.ctx.abortis checked betweenrealpathcalls.Docs.
docs/docs/reference/security-faq.mdanddocs/docs/configure/permissions.mdnow describe the deny check, that a*deny also removes the default allow for skill/reference directories and the CLI temp directory (so bundled skill scripts there are refused), and the limits below.Known limits (guard rail, not an OS sandbox)
$HOME/x,$(pwd)/../x,cd "$DIR", globs, variables set outside the command.python -c "open('/etc/hosts')", a script that writes to/tmp, config files that point outside, a barepythonthat an inheritedPATHresolves to an outside virtualenv. (git -C /outsideis refused: the path is a literal argument.)cd x; y,if cd x; …orcd x || y, both directories are judged, so an in-project..there can be refused.echo "/etc is a dir"may be refused. Fails closed, names the path.external_directory: { "*": "ask" }(the default) keeps today's coverage. Only a*deny turns the guard on; a deny on a narrower pattern alone does not.path.win32values; not run on Windows.How did you verify your code works?
test/tool/bash-external-directory.test.ts(now 52 tests), with a ruleset shaped like the agent's and anaskstub that mirrorsPermission.ask:cdfrom a subdirectory workdir, words after a redirection (cat > stolen.txt /o/s.txt,head > /dev/null /o/s.txt),PY=/o/py; $PY,export PATH=/o:$PATH; py,export FOO=/o/s.txt; head "$FOO",OUT=/o/w sh -c …,sort --output=,sort -o/o/w,dd of=,dd if=,echo --out=/etc/hosts,bash -c,sh -c,eval,env bash -lc,head lnk/s.txt,head ./lnk/s.txt,echo x > lnk/w,pushd ../.. && cp …,for,[[ -f ]],[ -f ],case,$'…'argument and program,~root/x,2>,&>,<,<>, a quoted path with spacescd a && head -c 3 ../inside.txt,cd a && sh ../run.sh,(cd a && head ../inside.txt),cd a && ../run.sh,cd a && echo ok > ../r.txt,pushd a && head ../inside.txt,awk '/var/ {print}',sed -n '/usr/p',grep -c /etc/,grep -e /usr/,cat /dev/null,cat /dev/null > f,ls /dev/null,nohup /usr/bin/true,/usr/bin/env /usr/bin/true,command -v /usr/bin/true, an in-projectbash -c, URLs; an allowed-but-missing directory;firstSegmentPathwithpath.win32(drive, UNC) and POSIX valuesask/allow: a fixed set of commands (now alsocat /dev/nullandsort -o<outside>/w.txt) produces the same outputs,external_directoryrequests and metadata keys asmaintest/agent/bash-external-directory-gate.test.ts(4 tests): no built-in agent that can run bash turns the guard on;denyturns it on forbuilder;allowleaves it off;*: askwith a narrower deny leaves it off.f9c19c74f9merged withmain,bash.tsunchanged), 30 failed: the 27 new refusal cases other than2>/&>/</<>/quoted-space (those were already refused), the allowed-in-project case, the allowed-missing-directory case and the Windows first-segment case. After the fix: 0 fail.bun test test/tool test/permission test/session test/agent test/permission-task.test.ts(packages/opencode):main,e0bb5d2f18): 2370 tests, 0 fail.(fail)list: none.1dd20cd720): 2406 tests, 0 fail.(fail)list: none.(fail)sets: none.test/cli/run/run-mode.test.ts,test/release-validation/question-937*,test/skill/release-v0.12.1-adversarial.test.ts: 78 pass, 0 fail.bun run typecheckclean;script/check-tracker-leaks.tsandscript/upstream/analyze.ts --markers --strictpass.Cost side effects
None. No model calls, queries or resources. Under a configured deny the tool does one
realpathand up to twostatcalls per literal word (capped at 2000 candidates); otherwise no extra work.Screenshots / recordings
N/A
Checklist
🤖 Generated with Claude Code
Summary by cubic
The bash tool only applied
external_directorypermission checks to a fixed list of file commands (cd,rm,cp,cat,ls, …), so under a configuredexternal_directory: denyany other command could still read, write, or execute outside the project. The tool now checks every literal out-of-project path a command names and refuses the call before anything runs.~, and..arguments, a command run by path, redirection targets, andcdtargets; option values and URLs are skipped.askpaths go through the normalexternal_directorypermission request as before.metadata.external_paths.*isdeny; unset, defaultask, andallowbehavior is unchanged.$HOME/x,$(pwd)/../x, globs) and paths opened by the program itself are not seen.Closes #1645.
Written for commit f9c19c7. Summary will update on new commits.