refactor(core): detect external command paths with a bash parser - #38822
refactor(core): detect external command paths with a bash parser#38822webpro wants to merge 1 commit into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: I found one related PR that should be checked: Related PR:
Why it's related: The PR description explicitly mentions that #34772 is a larger v1-targeted PR that this PR is aware of. Your current PR (38822) is a v2-specific refactor that picks up the same work of replacing the token-based external-directory advisory with a proper bash parser using |
|
Closing this because the shell-tool architecture has moved substantially and #39567 added parser-based permission handling, so this branch is no longer the right shape to merge. One distinction worth preserving for future work:
It also avoided treating paths that appear only in comments or heredoc bodies as references. That is a different concern from splitting commands for permission approval. If this coverage becomes useful later, the clean implementation now would be to extend Closing this to avoid keeping a stale and conflicting implementation open. |
Issue for this PR
No open issue — this closes a TODO left in
packages/core/src/tool/shell.ts. After realizing you're working on v2 and #34772 is too large and targets v1.Type of change
What does this PR do?
Picks up this TODO in the V2 shell tool:
The external-directory advisory scans the command with a regex tokenizer. That's position-blind — it takes any whitespace-delimited token that looks absolute, wherever it sits in the string — so it gets three kinds of thing wrong.
Paths it misses, because the token isn't absolute:
echo hi >/outside/out.txt— the token is>/outside/out.txt, so a write outside the workspace isn't flagged at allFOO=/outside/x cat ycat ${FILE:-/outside/x}Paths it reports incorrectly, because
unquoteonly strips quotes wrapping a whole token:cat /outside/my\ dir/filereports/outsidecat /outside/"my dir"/filereports/outside/"my dir", i.e. a literal"inside a filesystem pathPaths it reports that aren't references at all:
#commentSo this parses the command with
unbashand collects the words the parser produces. unbash is a small synchronous Bash parser with no dependencies and no WASM, so nothing about startup or the async init story changes. The walk stays small becausepath.isAbsoluteat the call site already discards the non-paths a parser surfaces:2>&1yields1,>&-yields-, a heredoc delimiter yieldsEOF.One deliberate omission: it does not read inside
sh -c "..."payloads. Doing that means maintaining a list of programs that re-interpret an argument as a script —shandenv, but equallysudo,timeout,xargs,ssh— which is open-ended and a separate question from parsing. The current tokenizer can't see into those either, so nothing regresses; it's worth its own PR if you want it.Disclosure: I author unbash — and knip, which depends on it (~11.7M downloads/week), so it runs against a lot of real-world scripts.
How did you verify your code works?
bun test test/util/bash.test.ts— 14 new specs covering each constructbun test test/tool-shell.test.ts test/shell.test.ts— 35 pass, existing shell tool tests unchangedbun typecheckfrom the repo root — 33/33The full suite has some pre-existing timeouts in
RepositoryCache,Gitandptythat also fail on a cleanv2checkout; they pass when run on their own.Screenshots / recordings
Not a UI change.
Checklist