diff --git a/internal/sandbox/safe_command.go b/internal/sandbox/safe_command.go index 61488eda2..8cb79c976 100644 --- a/internal/sandbox/safe_command.go +++ b/internal/sandbox/safe_command.go @@ -26,6 +26,10 @@ type interactiveProgram struct { suggestion string // windowsOnly limits the match to GOOS == "windows" (e.g. notepad). windowsOnly bool + // windowsSuggestion overrides suggestion on GOOS == "windows", for cases + // where the default suggestion names POSIX-only tools (cat/head/tail) + // that cmd.exe does not have. + windowsSuggestion string } // interactivePrograms maps a bare command name to its non-interactive guidance. @@ -39,9 +43,21 @@ var interactivePrograms = map[string]interactiveProgram{ "emacs": {reason: "emacs opens an interactive session", suggestion: "Use `emacs --batch` for scripting, or the edit_file/write_file tools."}, "pico": {reason: "pico is a full-screen editor that waits for keystrokes", suggestion: "Use the edit_file/write_file tools or `sed -i`."}, // Pagers. - "less": {reason: "less is a pager that waits for navigation keys", suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively."}, - "more": {reason: "more is a pager that waits for navigation keys", suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively."}, - "most": {reason: "most is a pager that waits for navigation keys", suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively."}, + "less": { + reason: "less is a pager that waits for navigation keys", + suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively.", + windowsSuggestion: "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view.", + }, + "more": { + reason: "more is a pager that waits for navigation keys", + suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively.", + windowsSuggestion: "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view.", + }, + "most": { + reason: "most is a pager that waits for navigation keys", + suggestion: "Use `cat`, `head`, or `tail -n N` to print file contents non-interactively.", + windowsSuggestion: "Use `type` to print file contents non-interactively, or the read_file tool with offset/limit for a partial view.", + }, // Process/system monitors. "top": {reason: "top runs a live full-screen dashboard until you quit it", suggestion: "Use `ps aux` (optionally `| head`) for a one-shot snapshot."}, "htop": {reason: "htop runs a live full-screen dashboard until you quit it", suggestion: "Use `ps aux` (optionally `| head`) for a one-shot snapshot."}, @@ -186,11 +202,15 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes 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: program.suggestion, + Suggestion: suggestion, } } diff --git a/internal/sandbox/safe_command_test.go b/internal/sandbox/safe_command_test.go index fe89a220c..34a4518c5 100644 --- a/internal/sandbox/safe_command_test.go +++ b/internal/sandbox/safe_command_test.go @@ -88,6 +88,28 @@ func TestDetectInteractiveCommandHonorsWindows(t *testing.T) { } } +// TestDetectInteractiveCommandPagerSuggestionIsPlatformSpecific covers a fix +// for suggesting POSIX-only tools (cat/head/tail) as the escape hatch for a +// blocked pager on Windows, where cmd.exe has none of them. +func TestDetectInteractiveCommandPagerSuggestionIsPlatformSpecific(t *testing.T) { + for _, pager := range []string{"less", "more", "most"} { + linux := DetectInteractiveCommand(pager+" notes.txt", "linux") + if !linux.Interactive || !strings.Contains(linux.Suggestion, "cat") { + t.Fatalf("%s on linux: expected cat/head/tail suggestion, got %+v", pager, linux) + } + windows := DetectInteractiveCommand(pager+" notes.txt", "windows") + if !windows.Interactive { + t.Fatalf("%s on windows: expected interactive block, got %+v", pager, windows) + } + if strings.Contains(windows.Suggestion, "cat") || strings.Contains(windows.Suggestion, "tail") { + t.Fatalf("%s on windows: suggestion should not name POSIX-only tools, got %q", pager, windows.Suggestion) + } + if !strings.Contains(windows.Suggestion, "type") { + t.Fatalf("%s on windows: expected type suggestion, got %q", pager, windows.Suggestion) + } + } +} + func TestDetectInteractiveCommandFindsAcrossSeparators(t *testing.T) { // Interactive commands hidden after a shell operator should still be caught. for _, command := range []string{ diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index 5fa7dfe4b..ce8d93f6b 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -152,7 +152,7 @@ func TestDetectShellCommandIssueAllowsWindowsCDSwitch(t *testing.T) { } } -func TestDetectShellCommandIssueRequiresActualLSCommand(t *testing.T) { +func TestDetectShellCommandIssueFlagsWindowsBadCommands(t *testing.T) { for _, command := range []string{ `echo false ls -la`, `echo list -items`, @@ -172,6 +172,22 @@ func TestDetectShellCommandIssueRequiresActualLSCommand(t *testing.T) { t.Fatalf("expected actual ls command to be flagged for %q", command) } } + + // Common Unix filters used for truncation (e.g. gh output) must be caught. + for _, command := range []string{ + `gh pr view 465 | head -200`, + `some-cmd | tail -n 10`, + `cat file | head -5`, + `ps aux | head`, + // Issue #467's exact reported case: `| cat` alone (no other POSIX + // utility in the pipeline) must also be flagged before execution. + `gh pr view 465 --json reviews,comments -q '.reviews | length' 2>&1 | cat`, + `some-cmd | cat`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue == nil { + t.Fatalf("expected POSIX filter command to be flagged for %q", command) + } + } } func TestDetectShellCommandIssueFlagsPipedPosixUtilities(t *testing.T) { @@ -191,6 +207,55 @@ func TestDetectShellCommandIssueFlagsPipedPosixUtilities(t *testing.T) { } } +func TestDetectShellCommandIssueIgnoresQuotedPipeText(t *testing.T) { + for _, command := range []string{ + `gh pr comment --body "please do not use | cat here"`, + `git commit -m "grep for TODO later"`, + `git commit -m "fix | cat pipe"`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected POSIX-utility text inside a quoted argument to pass for %q, got %#v", command, issue) + } + } + + // The same utility names, actually piped (unquoted), must still be flagged. + for _, command := range []string{ + `echo test | cat`, + `echo test | grep TODO`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue == nil { + t.Fatalf("expected unquoted POSIX utility pipeline to still be flagged for %q", command) + } + } +} + +func TestDetectShellCommandIssueIgnoresCaretEscapedMetachars(t *testing.T) { + for _, command := range []string{ + `foo^|cat`, + `echo a^&cat`, + `echo a^;cat`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected caret-escaped metachar to read as a literal for %q, got %#v", command, issue) + } + } + + // The same shape, unescaped, must still be flagged. + if issue := detectShellCommandIssue(`foo|cat`, "windows"); issue == nil { + t.Fatal("expected unescaped pipe into cat to still be flagged") + } +} + +func TestDetectShellCommandIssueSuggestionOmitsBlockedMore(t *testing.T) { + issue := detectShellCommandIssue(`some_command | wc -l`, "windows") + if issue == nil { + t.Fatal("expected POSIX utility pipeline to be flagged") + } + if strings.Contains(issue.Suggestion, "more") { + t.Fatalf("suggestion should not recommend more, which is itself blocked as an interactive pager on Windows: %q", issue.Suggestion) + } +} + func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) { for _, command := range []string{ `git log --oneline`, @@ -668,8 +733,15 @@ func TestBashToolBlocksInteractiveCommandThroughSandbox(t *testing.T) { if result.Status != StatusError { t.Fatalf("expected error status, got %s: %s", result.Status, result.Output) } - if !strings.Contains(result.Output, "interactive") || !strings.Contains(result.Output, "cat") { - t.Fatalf("expected pager guard message with cat suggestion, got %q", result.Output) + // The suggested non-interactive alternative is platform-specific: cat + // doesn't exist on Windows cmd.exe, so the guard suggests `type` there + // instead (see safe_command.go's windowsSuggestion). + wantSuggestion := "cat" + if runtime.GOOS == "windows" { + wantSuggestion = "type" + } + if !strings.Contains(result.Output, "interactive") || !strings.Contains(result.Output, wantSuggestion) { + t.Fatalf("expected pager guard message with %q suggestion, got %q", wantSuggestion, result.Output) } } diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 72f9c7cc0..11e30e4fd 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -20,7 +20,7 @@ type shellIssue struct { const windowsMsysSandboxKind = "windows_msys_sandbox" -const windowsMsysSandboxSuggestion = "MSYS/Cygwin coreutils from Git for Windows cannot run under Zero's write-restricted Windows sandbox. Prefer Zero native tools (grep, read_file with offset/limit, list_directory, glob), cmd.exe findstr/more, or PowerShell Select-Object -First/-Last. If host-level execution is truly required, rerun with sandbox_permissions: \"require_escalated\" and a narrow justification." +const windowsMsysSandboxSuggestion = "MSYS/Cygwin coreutils from Git for Windows cannot run under Zero's write-restricted Windows sandbox. Prefer Zero native tools (grep, read_file with offset/limit, list_directory, glob), cmd.exe findstr, or PowerShell Select-Object -First/-Last. If host-level execution is truly required, rerun with sandbox_permissions: \"require_escalated\" and a narrow justification." // windowsMsysProneNames is the single source of truth for POSIX coreutil names // that commonly resolve to a Git-for-Windows MSYS/Cygwin binary rather than a @@ -168,7 +168,15 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { return nil } trimmed := strings.TrimSpace(command) - if windowsBashStyleCDPattern.MatchString(trimmed) { + // Blank out double-quoted spans before matching, so a `cd /foo`-shaped + // string that only appears inside a quoted argument value (e.g. a `gh` + // or `git commit` message) is not mistaken for an actual command cmd.exe + // would interpret. Then neutralize cmd.exe caret escapes of its own + // metacharacters (foo^|cat is the literal token foo|cat, not a pipe into + // cat), so an escaped metachar can't stand in as a fake segment boundary + // either. + unquoted := stripCmdCaretEscapes(stripDoubleQuotedSpans(trimmed)) + if windowsBashStyleCDPattern.MatchString(unquoted) { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Command looks like POSIX/Bash syntax, but Zero runs bash tool commands through Windows cmd.exe on this host.", @@ -194,6 +202,44 @@ func detectShellCommandIssue(command string, goos string) *shellIssue { return nil } +// stripDoubleQuotedSpans replaces the contents of every double-quoted span +// (quotes included) with spaces, preserving the string's length and the +// position of unquoted text. cmd.exe treats a double-quoted span as a single +// literal token, so operators/utility names inside one are not real command +// syntax; blanking them out keeps the Windows command-issue regexes anchored +// to text cmd.exe would actually interpret. +func stripDoubleQuotedSpans(command string) string { + var b strings.Builder + b.Grow(len(command)) + inQuotes := false + for _, c := range command { + if c == '"' { + inQuotes = !inQuotes + b.WriteByte(' ') + continue + } + if inQuotes { + b.WriteByte(' ') + continue + } + b.WriteRune(c) + } + return b.String() +} + +// cmdEscapedMetacharPattern matches a cmd.exe caret escape of one of its own +// metacharacters. cmd.exe treats the escaped character as a literal, not an +// operator, so e.g. `foo^|cat` is the single literal token foo|cat, not a +// pipe into cat. +var cmdEscapedMetacharPattern = regexp.MustCompile(`\^[&|;^<>]`) + +// stripCmdCaretEscapes blanks out cmd.exe caret-escape sequences (both +// characters), so an escaped metacharacter cannot be mistaken by the Windows +// command-issue regexes for a real operator/segment boundary. +func stripCmdCaretEscapes(command string) string { + return cmdEscapedMetacharPattern.ReplaceAllString(command, " ") +} + // detectShellOutputIssue looks for MSYS runtime crash markers and cmd.exe // syntax-error text in output only, never in the command that was run. The // command line is attacker/user-controlled argument text (e.g. a `gh pr @@ -214,7 +260,7 @@ func detectShellOutputIssue(output string, goos string) *shellIssue { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Windows cmd.exe rejected the command syntax.", - Suggestion: "Translate the command to Windows cmd.exe syntax, set the bash tool cwd argument instead of running cd, or prefer native Zero tools for file inspection.", + Suggestion: "Use Windows cmd.exe syntax. Quote args with | using double quotes (e.g. --jq \".a | b\"). Avoid | head; use --jq or PowerShell Select-Object -First N instead. Prefer native tools.", } } return nil