Skip to content

refactor(core): detect external command paths with a bash parser#38822

Open
webpro wants to merge 1 commit into
anomalyco:v2from
webpro:unbash-path-advisories
Open

refactor(core): detect external command paths with a bash parser#38822
webpro wants to merge 1 commit into
anomalyco:v2from
webpro:unbash-path-advisories

Conversation

@webpro

@webpro webpro commented Jul 25, 2026

Copy link
Copy Markdown

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

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Picks up this TODO in the V2 shell tool:

// TODO: Replace token-based command-argument external-directory advisories with parser-based detection.

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 all
  • FOO=/outside/x cat y
  • cat ${FILE:-/outside/x}

Paths it reports incorrectly, because unquote only strips quotes wrapping a whole token:

  • cat /outside/my\ dir/file reports /outside
  • cat /outside/"my dir"/file reports /outside/"my dir", i.e. a literal " inside a filesystem path

Paths it reports that aren't references at all:

  • one appearing only in a # comment
  • one appearing only in a heredoc body

So this parses the command with unbash and 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 because path.isAbsolute at the call site already discards the non-paths a parser surfaces: 2>&1 yields 1, >&- yields -, a heredoc delimiter yields EOF.

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 — sh and env, but equally sudo, 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 construct
  • bun test test/tool-shell.test.ts test/shell.test.ts — 35 pass, existing shell tool tests unchanged
  • bun typecheck from the repo root — 33/33
  • Diffed the old tokenizer against the new walk over 33 commands: 7 differ, all listed above; the other 26 are identical

The full suite has some pre-existing timeouts in RepositoryCache, Git and pty that also fail on a clean v2 checkout; they pass when run on their own.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions

Copy link
Copy Markdown
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 unbash. This is the v2 implementation of a similar concept to #34772, but focused narrowly on the shell tool TODO in v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant