From 4fa0e5b0ba250bc18792406669593c7fa50d539c Mon Sep 17 00:00:00 2001 From: KRATOS Date: Sun, 19 Jul 2026 10:23:22 +0530 Subject: [PATCH 1/3] fix(sandbox): AST second opinion for interactive-command bypasses (#473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/sandbox/analyzer.go | 28 +++++++++++++++++++++++ internal/sandbox/safe_command.go | 33 +++++++++++++++++++++++++++ internal/sandbox/safe_command_test.go | 30 ++++++++++++++++++++++++ 3 files changed, 91 insertions(+) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 9509c5a81..9d88982a2 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -65,6 +65,34 @@ func AnalyzeCommand(script string) AnalysisResult { return result } +// astCommandFields parses command with the shell parser and returns each simple +// command as its literal field slice (program + args as text), resolving the +// real command positions across quoting, command substitution, subshells, and +// newline separators — the constructs the hand-written splitter in +// safe_command.go mis-handles (issue #473). It returns nil when the command +// cannot be parsed (e.g. a Windows cmd.exe string), so callers fall through to +// the regex path rather than hard-blocking. +func astCommandFields(command string) [][]string { + file, err := syntax.NewParser().Parse(strings.NewReader(command), "") + if err != nil { + return nil + } + var commands [][]string + syntax.Walk(file, func(node syntax.Node) bool { + call, ok := node.(*syntax.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + fields := make([]string, 0, len(call.Args)) + for _, word := range call.Args { + fields = append(fields, wordText(word)) + } + commands = append(commands, fields) + return true + }) + return commands +} + // analyzeInto parses script and folds its interactive/destructive/network usage // into result, sharing seen so program names are de-duplicated across recursion. func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, depth int) { diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index d6c2c8ea3..13de7df16 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -219,6 +219,39 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes } } + // AST second opinion (issue #473): the hand-written segment split above + // mis-handles interactive programs hidden by unusual quoting, command + // substitution, subshells, or newline separators. Re-extract the real simple + // commands from the parsed shell tree and apply the SAME per-program checks + // used above, so a bypass is caught while every program stays classified + // exactly as before (ssh with a trailing command, python -c, etc. remain + // allowed via hasNonInteractiveFlag). A command the parser cannot handle + // (Windows cmd.exe, obfuscation) yields no commands and falls through + // unchanged — the guard never hard-blocks on a parse error. + for _, fields := range astCommandFields(command) { + first := firstProgram(fields) + if first == "" { + continue + } + program, ok := interactivePrograms[first] + if !ok || (program.windowsOnly && goos != "windows") { + continue + } + if hasNonInteractiveFlag(first, fields) { + continue + } + suggestion := program.suggestion + if goos == "windows" && program.windowsSuggestion != "" { + suggestion = program.windowsSuggestion + } + return InteractiveCommandResult{ + Interactive: true, + Command: first, + Reason: program.reason, + Suggestion: suggestion, + } + } + return InteractiveCommandResult{} } diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index 3e817b66f..653d97548 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -406,3 +406,33 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) { } } } + +// The hand-written segment splitter misses interactive programs hidden by +// constructs only a real shell parser resolves (a newline separator collapsed +// to a space; a brace group that shifts the real command position). The AST +// second opinion must catch them (issue #473). +func TestDetectInteractiveCommandCatchesParserBypasses(t *testing.T) { + for _, cmd := range []string{ + "echo hi\nvim file.txt", // newline separator collapsed to a space by the regex path + "{ vim file.txt; }", // brace group hides the real program position + } { + got := DetectInteractiveCommand(cmd, "linux") + if !got.Interactive { + t.Errorf("DetectInteractiveCommand(%q) = not interactive, want caught (parser bypass)", cmd) + continue + } + if got.Command != "vim" { + t.Errorf("DetectInteractiveCommand(%q).Command = %q, want vim", cmd, got.Command) + } + } +} + +// The AST second opinion must not flag an interactive program NAME that appears +// only inside a quoted argument (not a real command position) — the parser +// distinguishes program from argument, so this must stay non-interactive. +func TestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(t *testing.T) { + got := DetectInteractiveCommand(`echo "please run vim later"`, "linux") + if got.Interactive { + t.Fatalf("interactive name inside a quoted argument must not be flagged, got %#v", got) + } +} From f8c902c08793adac3724ad009fcf52a6cde0f569 Mon Sep 17 00:00:00 2001 From: KRATOS Date: Sun, 19 Jul 2026 11:05:18 +0530 Subject: [PATCH 2/3] fix(sandbox): AST pass runs the full pipeline + guards dynamic programs (#473 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.) --- internal/sandbox/analyzer.go | 31 +++++++++++ internal/sandbox/safe_command.go | 75 +++++++++++++++++---------- internal/sandbox/safe_command_test.go | 29 +++++++++++ 3 files changed, 109 insertions(+), 26 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 9d88982a2..19fa82d3c 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -83,6 +83,12 @@ func astCommandFields(command string) [][]string { if !ok || len(call.Args) == 0 { return true } + // The program name must be a static literal. A dynamic first word (e.g. + // `$(printf foo)vim`, which runs as `foovim`) would otherwise yield a + // misleading partial literal ("vim") and fabricate an interactive match. + if !isLiteralWord(call.Args[0]) { + return true + } fields := make([]string, 0, len(call.Args)) for _, word := range call.Args { fields = append(fields, wordText(word)) @@ -93,6 +99,31 @@ func astCommandFields(command string) [][]string { return commands } +// isLiteralWord reports whether every part of word is a static literal (bare or +// quoted). A word containing a command substitution, parameter/arithmetic +// expansion, process substitution, etc. is dynamic — its runtime value is +// unknown, so its wordText (a partial literal) must not be trusted as a program +// name. +func isLiteralWord(word *syntax.Word) bool { + if word == nil { + return false + } + for _, part := range word.Parts { + switch typed := part.(type) { + case *syntax.Lit, *syntax.SglQuoted: + case *syntax.DblQuoted: + for _, inner := range typed.Parts { + if _, ok := inner.(*syntax.Lit); !ok { + return false + } + } + default: + return false + } + } + return true +} + // analyzeInto parses script and folds its interactive/destructive/network usage // into result, sharing seen so program names are de-duplicated across recursion. func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, depth int) { diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index 13de7df16..02f618824 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -222,39 +222,62 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes // AST second opinion (issue #473): the hand-written segment split above // mis-handles interactive programs hidden by unusual quoting, command // substitution, subshells, or newline separators. Re-extract the real simple - // commands from the parsed shell tree and apply the SAME per-program checks - // used above, so a bypass is caught while every program stays classified - // exactly as before (ssh with a trailing command, python -c, etc. remain - // allowed via hasNonInteractiveFlag). A command the parser cannot handle - // (Windows cmd.exe, obfuscation) yields no commands and falls through - // unchanged — the guard never hard-blocks on a parse error. + // commands from the parsed shell tree and apply the SAME full pipeline + // (interactive segments, sh -c payload recursion, per-program lookup), so a + // bypass is caught while every program stays classified exactly as before + // (ssh with a trailing command, python -c, etc. remain allowed). A command + // the parser cannot handle (Windows cmd.exe, obfuscation) yields no commands + // and falls through unchanged — the guard never hard-blocks on a parse error. for _, fields := range astCommandFields(command) { - first := firstProgram(fields) - if first == "" { - continue - } - program, ok := interactivePrograms[first] - if !ok || (program.windowsOnly && goos != "windows") { - continue - } - if hasNonInteractiveFlag(first, fields) { - continue - } - suggestion := program.suggestion - if goos == "windows" && program.windowsSuggestion != "" { - suggestion = program.windowsSuggestion - } - return InteractiveCommandResult{ - Interactive: true, - Command: first, - Reason: program.reason, - Suggestion: suggestion, + if result, ok := inspectCommandFields(fields, goos); ok { + return result } } return InteractiveCommandResult{} } +// inspectCommandFields applies the full interactive-detection pipeline to one +// already-split command's fields: the multi-word interactive segments, then the +// `sh -c ` recursion, then the per-program lookup with its +// non-interactive suppressions (hasNonInteractiveFlag covers REPL flags and +// trailing-command clients like ssh). It mirrors the hand-written passes above +// so an AST-extracted command is classified identically to a plainly-split one. +func inspectCommandFields(fields []string, goos string) (InteractiveCommandResult, bool) { + body := strings.ToLower(commandBody(fields)) + for _, seg := range interactiveSegments { + if body == seg.match || strings.HasPrefix(body, seg.match+" ") { + suggestion := seg.suggestion + if goos == "windows" && seg.windowsSuggestion != "" { + suggestion = seg.windowsSuggestion + } + return InteractiveCommandResult{Interactive: true, Command: seg.command, Reason: seg.reason, Suggestion: suggestion}, true + } + } + first := firstProgram(fields) + if first == "" { + return InteractiveCommandResult{}, false + } + if payload := shellDashCPayload(first, fields); payload != "" { + if inner := DetectInteractiveCommand(payload, goos); inner.Interactive { + return inner, true + } + return InteractiveCommandResult{}, false + } + program, ok := interactivePrograms[first] + if !ok || (program.windowsOnly && goos != "windows") { + return InteractiveCommandResult{}, false + } + if hasNonInteractiveFlag(first, fields) { + return InteractiveCommandResult{}, false + } + suggestion := program.suggestion + if goos == "windows" && program.windowsSuggestion != "" { + suggestion = program.windowsSuggestion + } + return InteractiveCommandResult{Interactive: true, Command: first, Reason: program.reason, Suggestion: suggestion}, true +} + // wrapperPrograms are launcher prefixes that precede the real program. After // one of these we keep scanning for the actual executable. var wrapperPrograms = map[string]bool{ diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index 653d97548..b30e12507 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -436,3 +436,32 @@ func TestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(t *testing.T) { t.Fatalf("interactive name inside a quoted argument must not be flagged, got %#v", got) } } + +// The AST second opinion must run the FULL pipeline — multi-word interactive +// segments and sh -c payload recursion, not just the per-program lookup — so a +// grouped/nested bypass is caught; and it must not fabricate a program name from +// a command substitution concatenated with a literal (issue #473, review). +func TestDetectInteractiveCommandASTPipelineBoundaries(t *testing.T) { + interactive := []struct{ cmd, wantCmd string }{ + {"{ git rebase -i HEAD~1; }", "git rebase -i"}, // multi-word segment in a brace group + {"{ sh -c 'vim file.txt'; }", "vim"}, // sh -c payload in a brace group + } + for _, tc := range interactive { + got := DetectInteractiveCommand(tc.cmd, "linux") + if !got.Interactive || got.Command != tc.wantCmd { + t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", tc.cmd, got, tc.wantCmd) + } + } +} + +// The AST pass must not fabricate a program name from a dynamic (non-literal) +// program word: `$(printf '%s' foo)vim` runs as `foovim`, not vim. Tested at the +// extractor so the hand-written pass (which pre-empts DetectInteractiveCommand +// with its own, separate handling of that construct) does not mask the guard. +func TestAstCommandFieldsSkipsDynamicProgram(t *testing.T) { + for _, fields := range astCommandFields("$(printf '%s' foo)vim file.txt") { + if firstProgram(fields) == "vim" { + t.Fatalf("astCommandFields fabricated a vim command from a substitution: %v", fields) + } + } +} From df2cd633f9a6669afac2d526d88213888d9e25b9 Mon Sep 17 00:00:00 2001 From: KRATOS Date: Mon, 20 Jul 2026 11:38:54 +0530 Subject: [PATCH 3/3] fix(sandbox): require literal words for the whole AST call (#473 review) 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. --- internal/sandbox/analyzer.go | 21 ++++++++++++++++----- internal/sandbox/safe_command_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/internal/sandbox/analyzer.go b/internal/sandbox/analyzer.go index 19fa82d3c..d674dd08e 100644 --- a/internal/sandbox/analyzer.go +++ b/internal/sandbox/analyzer.go @@ -83,11 +83,22 @@ func astCommandFields(command string) [][]string { if !ok || len(call.Args) == 0 { return true } - // The program name must be a static literal. A dynamic first word (e.g. - // `$(printf foo)vim`, which runs as `foovim`) would otherwise yield a - // misleading partial literal ("vim") and fabricate an interactive match. - if !isLiteralWord(call.Args[0]) { - return true + // EVERY word must be a static literal, not just the program name. + // wordText keeps only the literal/quoted parts of a word and silently + // drops expansions, so a dynamic word anywhere in the call reconstructs + // to something the shell will never run: `$(printf foo)vim` (runs as + // `foovim`) collapses to "vim", and `git $(printf foo)rebase -i` (runs + // as `git foorebase -i`, non-interactive) collapses to `git rebase -i` + // and fabricates an interactive match. Since the runtime value of an + // expansion is unknowable here, skip the whole call rather than classify + // a lossy reconstruction. Skipping is the safe direction: the + // hand-written passes above already ran, and a missed detection falls + // through to the normal permission prompt instead of hard-blocking a + // command the user never wrote. + for _, word := range call.Args { + if !isLiteralWord(word) { + return true + } } fields := make([]string, 0, len(call.Args)) for _, word := range call.Args { diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index b30e12507..15a2c5d6f 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -465,3 +465,28 @@ func TestAstCommandFieldsSkipsDynamicProgram(t *testing.T) { } } } + +// The literalness guard covers every word, not just the program name. A dynamic +// ARGUMENT is dropped by wordText the same way a dynamic program word is, so +// `git $(printf foo)rebase -i HEAD~1` (which runs as `git foorebase -i`, a +// non-existent and non-interactive subcommand) would otherwise be reconstructed +// as `git rebase -i` and blocked — a false positive on a command the user never +// wrote. Asserted through DetectInteractiveCommand as well as the extractor, +// because the whole point is what the caller ends up blocking. +func TestAstCommandFieldsSkipsDynamicArgument(t *testing.T) { + const dynamic = "{ git $(printf foo)rebase -i HEAD~1 ; }" + for _, fields := range astCommandFields(dynamic) { + if firstProgram(fields) == "git" { + t.Fatalf("astCommandFields reconstructed a git command from a dynamic argument: %v", fields) + } + } + if got := DetectInteractiveCommand(dynamic, "linux"); got.Interactive { + t.Errorf("DetectInteractiveCommand(%q) = %+v, want not interactive", dynamic, got) + } + // The genuine form must still be detected — the guard skips lossy + // reconstructions, it does not weaken real detection. + const genuine = "{ git rebase -i HEAD~1 ; }" + if got := DetectInteractiveCommand(genuine, "linux"); !got.Interactive || got.Command != "git rebase -i" { + t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", genuine, got, "git rebase -i") + } +}