refactor(core): detect external command paths with a bash parser#38822
Open
webpro wants to merge 1 commit into
Open
refactor(core): detect external command paths with a bash parser#38822webpro wants to merge 1 commit into
webpro wants to merge 1 commit into
Conversation
Contributor
|
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 |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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