fix(sandbox): AST second opinion for interactive-command bypasses (#473) - #745
Conversation
The interactive-command guard split shell commands with a hand-written parser (splitShellSegments), which mis-handles unusual quoting, command substitution, subshells, and newline separators — an interactive program hidden by one slips through and the agent hangs until the per-command timeout. Add astCommandFields(): mvdan.cc/sh/v3/syntax (already used by analyzer.go) to re-extract the real simple commands, then apply the SAME per-program checks the regex path uses (interactivePrograms + hasNonInteractiveFlag). This only ADDS detections and classifies every program exactly as before — ssh with a trailing command, python -c, etc. stay allowed — while catching the splitter's bypasses. Unparseable input (Windows cmd.exe, obfuscation) yields no commands and falls through to the regex path; the guard never hard-blocks on a parse error.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds shell AST command extraction and an AST-based second pass to interactive command detection. Literal-word filtering prevents dynamic program names from being fabricated, while tests cover parser bypasses, quoted arguments, boundaries, and recursive shell payloads. ChangesInteractive command detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DetectInteractiveCommand
participant astCommandFields
participant ShellASTParser
participant inspectCommandFields
DetectInteractiveCommand->>astCommandFields: extract command fields
astCommandFields->>ShellASTParser: parse shell command
ShellASTParser-->>astCommandFields: return call expression fields
astCommandFields-->>DetectInteractiveCommand: return literal command fields
DetectInteractiveCommand->>inspectCommandFields: inspect extracted commands
inspectCommandFields-->>DetectInteractiveCommand: return interactive match or no match
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/safe_command_test.go`:
- Around line 414-438: Extend TestDetectInteractiveCommandCatchesParserBypasses
with regression cases for `{ git rebase -i HEAD~1; }` and `{ sh -c 'vim
file.txt'; }`, asserting they remain non-interactive. Add a separate assertion
for `$(printf '%s' foo)vim file.txt` that also remains non-interactive, ensuring
AST parsing does not fabricate an interactive command name.
In `@internal/sandbox/safe_command.go`:
- Around line 231-253: Update the AST field loop in the interactive-command
detection function to run the same interactiveSegments check and
shellDashCPayload recursion used earlier in the pipeline before
interactivePrograms lookup. Apply both checks to each fields slice so grouped
commands and nested shell payloads such as git rebase or vim are detected
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8253c2ed-3230-415c-a37b-35cad9cc144b
📒 Files selected for processing (3)
internal/sandbox/analyzer.gointernal/sandbox/safe_command.gointernal/sandbox/safe_command_test.go
…ms (#473 review) Address review on #745: - The AST second opinion now applies the complete detection pipeline via a new inspectCommandFields helper (multi-word interactive segments, sh -c payload recursion, then the per-program lookup), so parser-hidden bypasses like { git rebase -i HEAD~1; } and { sh -c 'vim file.txt'; } are caught, not just bare interactive programs. - astCommandFields now skips a CallExpr whose program word is not a static literal (isLiteralWord), so a substitution concatenated with a literal ($(printf foo)vim -> foovim) can't fabricate an interactive program name. Regression tests added for both boundaries. (The un-braced $(printf foo)vim false positive is a separate, pre-existing hand-parser behavior — present on main before this PR — and is out of scope here.)
|
Addressed in f8c902c. Major (make the AST pass mirror the pipeline): the AST second opinion now runs the full pipeline via a new Fabricated program names: On @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames f8c902c
git diff --no-ext-diff --unified=80 f8c902c^ f8c902c -- \
internal/sandbox/analyzer.go \
internal/sandbox/safe_command.go \
internal/sandbox/safe_command_test.go
ast-grep outline internal/sandbox/analyzer.go --items all
ast-grep outline internal/sandbox/safe_command.go --items all
ast-grep outline internal/sandbox/safe_command_test.go --items all
rg -n -C 8 \
'func (astCommandFields|isLiteralWord|inspectCommandFields|DetectInteractiveCommand)|Test(AstCommandFieldsSkipsDynamicProgram|DetectInteractiveCommandASTPipelineBoundaries)' \
internal/sandbox/analyzer.go internal/sandbox/safe_command.go internal/sandbox/safe_command_test.goLength of output: 37292 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '315,420p' internal/sandbox/analyzer.go
sed -n '363,575p' internal/sandbox/safe_command.go
sed -n '1,150p' internal/sandbox/safe_command.go
rg -n -C 5 \
'(wordText|literalWordTexts|dashCPayload|shellDashCPayload|isLiteralWord|astCommandFields|DetectInteractiveCommand)' \
internal/sandbox \
-g '*_test.go'
git diff --check f8c902c^ f8c902cLength of output: 46429
For example: { git "$(printf foo)rebase" -i; }actually runs The same affects shell payload recursion: { sh -c "$(printf foo)vim file.txt"; }runs Please preserve dynamic-word information (or skip AST segment matching and ✅ Action performedReview finished.
|
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Confirmed regression at f8c902c: astCommandFields validates only the program word, then wordText discards expansions from every argument before inspectCommandFields classifies them. A brace-grouped git command whose rebase argument is prefixed by $(printf foo) executes git foorebase -i HEAD1 and is non-interactive, but this PR reconstructs git rebase -i HEAD1 and blocks it. The same regression test passes on the PR base, so this false positive is newly introduced. Please preserve argument literalness and avoid classification from lossy dynamic arguments, or skip those AST commands, and add this case as a regression test. CI and focused vet are otherwise clean.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving.
This is genuinely additive, which is the important property for a guard change: DetectInteractiveCommand still runs the hand-written passes first and both return early on a hit, so the new AST loop only runs when the hand path found nothing. It can add a detection but never suppress one. The AST path re-extracts the real simple commands via mvdan.cc/sh and runs them through inspectCommandFields, which reuses the same predicates the guard already has (interactive segments, sh -c recursion, interactivePrograms + hasNonInteractiveFlag with the ssh/sqlite3/redis-cli trailing-command suppression), so ssh host 'uptime', python -c, and node -e stay non-interactive. Fail-open is handled too: a parse error returns nil and falls through to the hand path, and isLiteralWord stops a dynamic first word like $(printf foo)vim from fabricating a match.
One suggestion, not a blocker: the issue calls out subshells, command substitution, and newline separators specifically, and the new tests cover the brace-group and newline cases but not a bare subshell ( vim ), a background echo ok & vim, or process substitution sort <(vim). Those are caught now, but a direct regression case for each would keep a future refactor from silently regressing them. Worth adding while you're in here.
CI green, sandbox package green locally.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on f8c902c0, and anand's regression reproduces. My earlier approval doesn't hold; apologies for missing it.
Confirmed on this branch versus the base:
{ git $(printf foo)rebase -i HEAD~1 ; }on the base (main) givesInteractive=false(correct: it runsgit foorebase, a non-existent, non-interactive subcommand). On this PR it givesInteractive=true, Command="git rebase -i", a false positive.
Root cause is exactly what anand described. astCommandFields guards only the program word (isLiteralWord(call.Args[0])), but each argument is then run through wordText, which keeps only literal/quoted parts and silently drops *syntax.CmdSubst / *syntax.ParamExp. So a dynamic argument ($(printf foo)rebase) is reconstructed lossily to rebase, and inspectCommandFields classifies the fabricated git rebase -i. The genuine git rebase -i HEAD~1 still classifies correctly, so this is purely a new false positive on dynamic arguments.
Cleanest fix is to extend the literalness guard from the program word to every word: if any word in the call carries an expansion (!isLiteralWord), skip that AST command rather than classify a lossy reconstruction, since you can't know what it actually runs. That is the safe direction (skip, not false-block), and the hand-written path still runs first. A regression case for { git $(printf foo)rebase -i HEAD~1 ; } expecting not-interactive would lock it in.
The rest still looks right (additive, hand-path-first, fails open on a parse error); this is the one thing blocking. Agreeing with anand's requested changes.
astCommandFields guarded only the program word, then ran every argument through wordText, which keeps literal/quoted parts and silently drops CmdSubst/ParamExp. A dynamic ARGUMENT was therefore reconstructed lossily: `git $(printf foo)rebase -i HEAD~1` runs as `git foorebase -i` (a non-existent, non-interactive subcommand) but collapsed to `git rebase -i` and was blocked -- a false positive on a command the user never wrote, and new on this branch (the base correctly reports not-interactive). Extend the literalness guard to every word in the call: if any word carries an expansion, skip that AST command rather than classify a reconstruction whose runtime value is unknowable. Skipping is the safe direction -- the hand-written passes still run first, and a miss falls through to the normal permission prompt instead of hard-blocking. Genuine detection is unchanged: `git rebase -i HEAD~1` and the #473 bypass cases still classify exactly as before.
|
@coderabbitai review Pushed
The guard now covers every word in the call: if any word carries an expansion, the AST command is skipped rather than classified from a lossy reconstruction. Skipping is the safe direction — the hand-written passes still run first and a miss falls through to the normal prompt instead of hard-blocking. Verified: the reported case is back to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --no-renames df2cd63
git diff --no-ext-diff --unified=80 df2cd63^ df2cd63 -- \
internal/sandbox/analyzer.go \
internal/sandbox/safe_command.go \
internal/sandbox/safe_command_test.go
ast-grep outline internal/sandbox/analyzer.go --items all
ast-grep outline internal/sandbox/safe_command_test.go --items all
rg -n -C 10 \
'func (astCommandFields|isLiteralWord|wordText)|Test(AstCommandFieldsSkipsDynamicArgument|AstCommandFieldsSkipsDynamicProgram|DetectInteractiveCommandASTPipelineBoundaries)' \
internal/sandbox/analyzer.go internal/sandbox/safe_command_test.go
git diff --check df2cd63^ df2cd63Length of output: 25603 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,260p' internal/sandbox/safe_command.go
sed -n '260,620p' internal/sandbox/safe_command.go
sed -n '140,380p' internal/sandbox/safe_command_test.go
sed -n '320,500p' internal/sandbox/analyzer.go
rg -n -C 6 \
'(splitShellSegments|inspectCommandFields|astCommandFields|firstProgram|interactivePrograms|brace|\\{)' \
internal/sandbox/safe_command.go internal/sandbox/safe_command_test.goLength of output: 44843
Skipping the entire AST call whenever any argument is dynamic creates a bypass for static interactive programs whose dynamic arguments do not affect whether they are interactive: { vim "$file"; }The handwritten splitter does not recognize Please preserve literalness per field rather than dropping the full call:
Add a regression such as: got := DetectInteractiveCommand(`{ vim "$file"; }`, "linux")
if !got.Interactive || got.Command != "vim" {
t.Fatalf("got %+v, want interactive vim", got)
}This retains the false-positive fix for dynamic |
|
@anandh8x @Vasanthdev2004 — ready for another look. Your reviews were both filed on What you reported: What changed: the literalness guard now covers every word in the call, not just Re-verified just now at
|
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Approved on the latest head. The AST fallback now applies the existing full detection pipeline and skips calls with dynamic words, avoiding the prior lossy-reconstruction false positive. Focused sandbox race tests and CI pass.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on the current head (df2cd633), and the fix resolves my earlier block. astCommandFields now requires EVERY word in the call to be a literal, not just the program name, so a dynamic argument makes it skip the whole AST command rather than reconstruct a lossy git rebase -i from git $(printf foo)rebase -i. That is exactly the fix, and skipping is the safe direction (the hand path already ran; a miss falls through to a prompt, not a hard block).
Confirmed empirically against the exact case I flagged plus the bypass classes #473 targets, all correct:
{ git $(printf foo)rebase -i HEAD~1 ; }-> Interactive=false (the regression, now correctly skipped)git rebase -i HEAD~1-> Interactive=true (genuine, preserved){ git rebase -i HEAD~1 ; }-> Interactive=true (brace-grouped genuine, still caught){ vim ; },sort <(vim),echo ok & vim-> Interactive=true (brace group, process substitution, background separator all still caught)
So the dynamic-argument false positive is gone and none of the real detections regressed. Approving. Thanks for the quick turnaround.
3 participants
Summary
Closes #473.
The interactive-command guard (
DetectInteractiveCommand, used by the bash and exec tools before launching a command) split shell commands with a large hand-written parser,splitShellSegments. That splitter mis-handles unusual quoting, command substitution, subshells, and newline separators, so an interactive program hidden by one of those constructs slips through — the agent has no TTY, so it hangs on the editor/pager/REPL until the per-command timeout.Fix — augment with the shell AST (not replace)
The repo already parses shell with
mvdan.cc/sh/v3/syntaxinanalyzer.go, andrisk.goalready uses it as an additive "second opinion." This applies the same pattern to interactive detection:astCommandFields()parses the command and re-extracts the real simple commands (program + args) from the tree, resolving the positions the regex splitter mis-reads.DetectInteractiveCommandruns the hand-written path first (unchanged), then, as a backstop, classifies each AST-extracted command with the same per-program predicates it already uses —interactivePrograms+hasNonInteractiveFlag(which includes thessh/sqlite3/redis-clitrailing-command suppression).Because the backstop reuses the existing predicates, every program is classified exactly as before (
ssh host 'uptime',python -c '…',mongosh --eval …stay allowed); it only ADDS detections for the splitter's bypasses. There are no false positives: the AST distinguishes a program position from an argument, so an interactive program name inside a quoted argument (echo "run vim later") is never flagged. A command the parser cannot handle (Windows cmd.exe, obfuscation) yields no commands and falls through to the regex path — the guard never hard-blocks on a parse error.Scope
internal/sandboxonly. No config or API change. No performance change expected.Testing
TestDetectInteractiveCommandCatchesParserBypasses(a newline-separated and a brace-group invocation the hand splitter misses are now caught, with the right program name), andTestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(an interactive name inside a quoted argument stays non-interactive). The bypass test was confirmed to fail without the fix; all existingsafe_commandtests (includingssh host 'uptime'non-interactive) still pass.go test -race ./internal/sandbox/...green.make fmt-check,go vet ./...,go run ./cmd/zero-release build,go run ./cmd/zero-release smoke,govulncheck,git diff HEAD --checkall clean.Note
An earlier draft used the AST's own
Interactiveflag directly, which over-blockedsshwith a trailing command (the AST's suppression is cruder than the guard's). The committed version reuses the guard'shasNonInteractiveFlag, so classification is identical to before and only the bypasses change.Summary by CodeRabbit
Bug Fixes
Tests