diff --git a/internal/agent/command_prefix.go b/internal/agent/command_prefix.go index 43e666f7..c577f6de 100644 --- a/internal/agent/command_prefix.go +++ b/internal/agent/command_prefix.go @@ -2,6 +2,7 @@ package agent import ( "fmt" + "runtime" "strings" "github.com/Gitlawb/zero/internal/sandbox" @@ -27,10 +28,20 @@ func proposedCommandPrefix(toolName string, args map[string]any) []string { } return nil } - for _, tokens := range segments { - if !knownSafeCommandSegment(tokens) && sandbox.ValidCommandPrefix(tokens) { - return append([]string(nil), tokens...) + // Only propose approving a prefix of one segment when every other segment + // in the command is independently known-safe. Once a prefix is approved, + // shellExecutionArgsForApproval escalates the whole command (every + // segment, not just the approved one) to bypass the sandbox, so offering a + // prefix that leaves an MSYS-prone (or otherwise unsafe) segment uncovered + // would let that segment run unsandboxed without ever being reviewed. + for index, tokens := range segments { + if knownSafeCommandSegment(tokens) { + continue + } + if !sandbox.ValidCommandPrefix(tokens) || !otherSegmentsKnownSafe(segments, index) { + return nil } + return append([]string(nil), tokens...) } if len(segments) == 0 || !sandbox.ValidCommandPrefix(segments[0]) { return nil @@ -38,6 +49,20 @@ func proposedCommandPrefix(toolName string, args map[string]any) []string { return append([]string(nil), segments[0]...) } +// otherSegmentsKnownSafe reports whether every segment other than the one at +// skip is known-safe on its own. +func otherSegmentsKnownSafe(segments [][]string, skip int) bool { + for index, tokens := range segments { + if index == skip { + continue + } + if !knownSafeCommandSegment(tokens) { + return false + } + } + return true +} + func matchCommandPrefix(toolName string, args map[string]any, options Options) (sandbox.CommandPrefixGrant, bool, bool) { if !isShellCommandTool(toolName) || options.Sandbox == nil { return sandbox.CommandPrefixGrant{}, false, false @@ -275,7 +300,11 @@ func knownSafeCommandSegment(command []string) bool { if len(command) == 0 { return false } - switch commandName(command[0]) { + name := commandName(command[0]) + if runtime.GOOS == "windows" && tools.MsysProneCommandName(name) { + return false + } + switch name { case "cat", "cd", "cut", "echo", "expr", "false", "grep", "head", "id", "ls", "nl", "paste", "pwd", "rev", "seq", "stat", "tail", "tr", "true", "uname", "uniq", "wc", "which", "whoami": diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index eec799e7..69508a67 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -1,9 +1,11 @@ package agent import ( + "runtime" "testing" "github.com/Gitlawb/zero/internal/sandbox" + "github.com/Gitlawb/zero/internal/tools" ) func TestProposedCommandPrefixUsesSafeSimpleCommands(t *testing.T) { @@ -35,12 +37,37 @@ func TestProposedCommandPrefixHonorsValidatedRequestedPrefix(t *testing.T) { func TestProposedCommandPrefixSupportsSegmentedCommands(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}) + if runtime.GOOS == "windows" { + // head is MSYS-prone on Windows (#458), so proposedCommandPrefix must + // not offer "ps aux" as a reusable prefix here: approving it would + // escalate the whole command, including the uncovered head segment, + // to bypass the sandbox unreviewed. See + // TestProposedCommandPrefixRejectsPrefixLeavingUnsafeTailUncovered for + // the platform-independent regression coverage of this behavior. + if got != nil { + t.Fatalf("expected no prefix on Windows because head is MSYS-prone, got %#v", got) + } + return + } want := []string{"ps", "aux"} if !equalStringSlices(got, want) { t.Fatalf("prefix = %#v, want %#v", got, want) } } +// TestProposedCommandPrefixRejectsPrefixLeavingUnsafeTailUncovered guards +// against proposedCommandPrefix offering to approve one segment of a +// multi-segment command (e.g. "ps aux") while a different segment (e.g. "npm +// install") is not known-safe. shellExecutionArgsForApproval escalates the +// entire command once any prefix is approved, so an uncovered unsafe segment +// would bypass the sandbox unreviewed. Uses npm, which is never known-safe on +// any platform, so the assertion does not depend on runtime.GOOS. +func TestProposedCommandPrefixRejectsPrefixLeavingUnsafeTailUncovered(t *testing.T) { + if got := proposedCommandPrefix("bash", map[string]any{"command": "ps aux && npm install"}); got != nil { + t.Fatalf("expected no prefix because npm segment is not known-safe, got %#v", got) + } +} + func TestProposedCommandPrefixHonorsRequestedPrefixAcrossSegments(t *testing.T) { got := proposedCommandPrefix("bash", map[string]any{ "command": "git status --short && git status --branch", @@ -120,13 +147,35 @@ func TestProposedCommandPrefixRejectsUnsafeLaunchers(t *testing.T) { func TestMatchCommandPrefixCoversSegmentedCommandWithSafeTail(t *testing.T) { engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: t.TempDir()}) engine.GrantCommandPrefixForSession("bash", []string{"ps", "aux"}) + // head is MSYS-prone on Windows (#458) and must not count as a known-safe tail. + command := "ps aux | head -5" + if runtime.GOOS == "windows" { + command = "ps aux | echo ok" + } - grant, ok, session := matchCommandPrefix("bash", map[string]any{"command": "ps aux | head -5"}, Options{Sandbox: engine}) + grant, ok, session := matchCommandPrefix("bash", map[string]any{"command": command}, Options{Sandbox: engine}) if !ok || !session || !equalStringSlices(grant.Prefix, []string{"ps", "aux"}) { t.Fatalf("match = %#v ok=%v session=%v, want session ps aux prefix", grant, ok, session) } } +func TestKnownSafeCommandSegmentRejectsMsysProneOnWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only known-safe MSYS guard") + } + for _, command := range [][]string{{"head", "-5"}, {"cat", "file.txt"}, {"grep", "pat"}} { + if knownSafeCommandSegment(command) { + t.Fatalf("expected %q to be unsafe on Windows, got known-safe", command) + } + } + if !knownSafeCommandSegment([]string{"echo", "ok"}) { + t.Fatal("expected echo to remain known-safe on Windows") + } + if !tools.MsysProneCommandName("head") { + t.Fatal("expected head to be MSYS-prone") + } +} + func TestMatchCommandPrefixRejectsUncoveredSegment(t *testing.T) { engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: t.TempDir()}) engine.GrantCommandPrefixForSession("bash", []string{"ps", "aux"}) diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 88052d06..06be5328 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "path/filepath" "runtime" @@ -1945,6 +1946,11 @@ func TestRunCommandPrefixApprovalBypassesSandboxForMatchingShellCalls(t *testing func TestRunCommandPrefixApprovalCoversSegmentedShellWithSafeTail(t *testing.T) { root := t.TempDir() + segmentedCommand := `ps aux | head -5` + if runtime.GOOS == "windows" { + // head is MSYS-prone on Windows (#458) and no longer counts as a known-safe tail. + segmentedCommand = `ps aux | echo ok` + } retryTool := &sandboxDeniedRetryTool{} registry := tools.NewRegistry() registry.Register(retryTool) @@ -1958,7 +1964,7 @@ func TestRunCommandPrefixApprovalCoversSegmentedShellWithSafeTail(t *testing.T) }, { {Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-2", ToolName: "bash"}, - {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-2", ArgumentsFragment: `{"command":"ps aux | head -5"}`}, + {Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-2", ArgumentsFragment: fmt.Sprintf(`{"command":%q}`, segmentedCommand)}, {Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-2"}, {Type: zeroruntime.StreamEventDone}, }, @@ -2907,8 +2913,10 @@ func TestBuildSystemPromptInjectsHostShellContext(t *testing.T) { t.Fatalf("expected operating system in environment block, got %q", prompt) } if runtime.GOOS == "windows" { - if !strings.Contains(prompt, "Windows cmd.exe syntax") || !strings.Contains(prompt, "cwd argument") { - t.Fatalf("expected Windows cmd.exe shell guidance in prompt, got %q", prompt) + for _, want := range []string{"Windows cmd.exe syntax", "cwd argument", "MSYS binaries", "grep", "require_escalated"} { + if !strings.Contains(prompt, want) { + t.Fatalf("expected Windows shell guidance to mention %q, got %q", want, prompt) + } } } else if !strings.Contains(prompt, "/bin/sh syntax") { t.Fatalf("expected POSIX shell guidance in prompt, got %q", prompt) diff --git a/internal/agent/system_prompt.go b/internal/agent/system_prompt.go index 1a492695..46c51b62 100644 --- a/internal/agent/system_prompt.go +++ b/internal/agent/system_prompt.go @@ -286,7 +286,7 @@ func workspaceContext(cwd string) string { b.WriteString("Working directory: " + cwd + "\n") b.WriteString("Operating system: " + runtime.GOOS + "\n") if runtime.GOOS == "windows" { - b.WriteString("Shell syntax: Windows cmd.exe syntax for exec_command/bash tools; prefer the workdir/cwd argument instead of cd when changing directories.\n") + b.WriteString("Shell syntax: Windows cmd.exe syntax for exec_command/bash tools; prefer the workdir/cwd argument instead of cd when changing directories. Do not pipe to or invoke POSIX coreutils from Git for Windows (usr\\bin head/grep/tail/cat/...): they are MSYS binaries and fail under the write-restricted sandbox; use native Zero tools (grep, read_file, list_directory, glob) or cmd.exe findstr/more instead, or sandbox_permissions require_escalated only when host-level execution is truly required.\n") } else { b.WriteString("Shell syntax: /bin/sh syntax for exec_command/bash tools; prefer the workdir/cwd argument instead of cd when changing directories.\n") } diff --git a/internal/tools/bash.go b/internal/tools/bash.go index 7361c346..50489809 100644 --- a/internal/tools/bash.go +++ b/internal/tools/bash.go @@ -83,7 +83,12 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS if err != nil { return errorResult("Error: Invalid arguments for bash: " + err.Error()) } - if issue := detectShellCommandIssue(commandText, runtime.GOOS); issue != nil { + // Resolve the command engine before the MSYS preflight check so an + // approved require_escalated call (commandEngine == nil, truly + // unsandboxed) can actually bypass the MSYS guard instead of being + // hard-blocked by the same check it was meant to escalate past. + commandEngine := commandEngineForSandboxPermissions(engine, sandboxPermissions) + if issue := detectShellCommandIssue(commandText, runtime.GOOS); issue != nil && !msysGuardBypassed(issue, commandEngine) { return shellIssueBlockResult(*issue) } @@ -106,7 +111,6 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS "cwd": relativeCwd, "timeout_ms": strconv.Itoa(timeoutMS), } - commandEngine := commandEngineForSandboxPermissions(engine, sandboxPermissions) if commandEngine == nil && sandboxPermissions == SandboxPermissionsRequireEscalated { meta["sandbox_permissions"] = string(SandboxPermissionsRequireEscalated) } @@ -167,7 +171,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS outText, errText, truncated := budgetBashCapture(stdoutText, stdout.total, stderrText, stderrTotal, meta) return Result{ Status: StatusError, - Output: formatBashOutputWithShellHint(commandText, outText, errText, exitCode, meta), + Output: formatBashOutputWithShellHint(outText, errText, exitCode, meta), Truncated: truncated, Meta: meta, } @@ -178,7 +182,7 @@ func (tool bashTool) run(ctx context.Context, args map[string]any, engine *zeroS if meta[SandboxLikelyDeniedMeta] == "true" { return Result{ Status: StatusError, - Output: formatBashOutputWithShellHint(commandText, outText, errText, exitCode, meta), + Output: formatBashOutputWithShellHint(outText, errText, exitCode, meta), Truncated: truncated, Meta: meta, } @@ -198,6 +202,18 @@ func commandEngineForSandboxPermissions(engine *zeroSandbox.Engine, sandboxPermi return engine } +// msysGuardBypassed reports whether a windows_msys_sandbox preflight issue no +// longer applies because sandbox_permissions: require_escalated actually +// resolved to unsandboxed, host-level execution (commandEngine is nil only +// then, per commandEngineForSandboxPermissions). MSYS coreutils fail because +// of the sandbox's restricted token/handles, so running outside it removes +// the failure mode this guard exists for. Any other issue kind (e.g. a real +// cmd.exe syntax problem) still blocks regardless of escalation, since +// running outside the sandbox does not fix a syntax error. +func msysGuardBypassed(issue *shellIssue, commandEngine *zeroSandbox.Engine) bool { + return issue.Kind == windowsMsysSandboxKind && commandEngine == nil +} + // appendSandboxBlocks appends a block listing the denials // the sandbox log monitor captured, so the model can see what was blocked. With no // blocks the stderr is returned unchanged. @@ -520,14 +536,6 @@ func (b *boundedBuffer) retained() string { return string(b.head) + string(b.tail) } -// truncateHeadTail keeps the first and last halves of value when it exceeds -// maxBytes, dropping the middle behind a marker. Returns the possibly-truncated -// text, the raw byte length, and whether truncation happened. -func truncateHeadTail(value string, maxBytes int) (string, int, bool) { - // value holds the whole stream, so its length is the true raw size. - return truncateHeadTailWithTotal(value, len(value), maxBytes) -} - // truncateHeadTailWithTotal head+tail-truncates value to maxBytes, using total — // the full original byte count — for the "N bytes omitted" marker and the raw // count. total may exceed len(value) when the middle was already discarded during @@ -547,9 +555,9 @@ func truncateHeadTailWithTotal(value string, total, maxBytes int) (string, int, return utf8Prefix(value, head) + marker + utf8Suffix(value, tail), total, true } -func formatBashOutputWithShellHint(command string, stdout string, stderr string, exitCode int, meta map[string]string) string { +func formatBashOutputWithShellHint(stdout string, stderr string, exitCode int, meta map[string]string) string { output := formatBashOutput(stdout, stderr, exitCode) - if issue := detectShellOutputIssue(command, stdout+"\n"+stderr, runtime.GOOS); issue != nil { + if issue := detectShellOutputIssue(stdout+"\n"+stderr, runtime.GOOS); issue != nil { meta["shell_issue"] = issue.Kind output = appendShellIssueHint(output, *issue) } diff --git a/internal/tools/bash_tool_test.go b/internal/tools/bash_tool_test.go index a3270ee6..5fa7dfe4 100644 --- a/internal/tools/bash_tool_test.go +++ b/internal/tools/bash_tool_test.go @@ -181,9 +181,13 @@ func TestDetectShellCommandIssueFlagsPipedPosixUtilities(t *testing.T) { `cat file.txt | wc -l`, `some_command | tail -n 20`, } { - if issue := detectShellCommandIssue(command, "windows"); issue == nil { + issue := detectShellCommandIssue(command, "windows") + if issue == nil { t.Fatalf("expected POSIX utility pipeline to be flagged for %q", command) } + if issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected windows_msys_sandbox for %q, got %q", command, issue.Kind) + } } } @@ -195,7 +199,7 @@ func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) { `echo header text`, `tail-log.sh`, `grep-cli --version`, - `sed.exe --help`, + `mytool.exe --help`, } { if issue := detectShellCommandIssue(command, "windows"); issue != nil { t.Fatalf("expected unrelated command to pass for %q, got %#v", command, issue) @@ -204,7 +208,7 @@ func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) { } func TestDetectShellOutputIssueAddsWindowsSyntaxHint(t *testing.T) { - issue := detectShellOutputIssue(`cd /d/tmp/zero-pr-158 && ls -la`, "The syntax of the command is incorrect.", "windows") + issue := detectShellOutputIssue("The syntax of the command is incorrect.", "windows") if issue == nil { t.Fatal("expected Windows syntax error to get shell guidance") } @@ -434,6 +438,105 @@ func TestRegistryRunsWithDegradedUnavailableNativeSandbox(t *testing.T) { } } +// TestBashToolRequireEscalatedMsysGuard covers the boundary of the fix for an +// escalation-vs-preflight ordering bug: the MSYS sandbox guard exists only +// because MSYS/Cygwin coreutils fail under the write-restricted sandbox, so +// once sandbox_permissions: require_escalated is actually approved (running +// the command unsandboxed), the guard must get out of the way, but an +// unrelated windows_shell_syntax issue (a real cmd.exe syntax problem, not a +// sandbox interaction) must still block regardless of escalation. +func TestBashToolRequireEscalatedMsysGuard(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only MSYS sandbox guard") + } + root := t.TempDir() + newEngine := func() *sandbox.Engine { + return sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: root, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, + }) + } + registry := NewRegistry() + registry.Register(NewBashTool(root)) + + t.Run("default sandboxing still blocks an MSYS-prone command", func(t *testing.T) { + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": "cat somefile.txt", + }, RunOptions{ + PermissionGranted: true, + Sandbox: newEngine(), + PermissionMode: string(sandbox.PermissionModeAsk), + }) + if result.Meta["shell_issue"] != "windows_msys_sandbox" { + t.Fatalf("expected windows_msys_sandbox block without escalation, got %#v", result) + } + }) + + t.Run("approved require_escalated bypasses the MSYS guard", func(t *testing.T) { + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": "cat somefile.txt", + "sandbox_permissions": string(SandboxPermissionsRequireEscalated), + }, RunOptions{ + PermissionGranted: true, + Sandbox: newEngine(), + PermissionMode: string(sandbox.PermissionModeAsk), + }) + // Assert on the preflight block sentinel (exit_code "-1", set only by + // shellIssueBlockResult) rather than shell_issue: once the guard is + // bypassed, "cat somefile.txt" actually runs, and its real, + // PATH-dependent output could otherwise trip the unrelated + // post-execution detectShellOutputIssue heuristic and make this + // assertion flaky for reasons unrelated to the guard under test. + if result.Meta["exit_code"] == "-1" { + t.Fatalf("expected approved require_escalated to bypass the MSYS guard, got blocked: %#v", result) + } + }) + + t.Run("approved require_escalated still blocks an unrelated syntax issue", func(t *testing.T) { + result := registry.RunWithOptions(context.Background(), "bash", map[string]any{ + "command": `cd /d/tmp/zero-pr-158 && dir`, + "sandbox_permissions": string(SandboxPermissionsRequireEscalated), + }, RunOptions{ + PermissionGranted: true, + Sandbox: newEngine(), + PermissionMode: string(sandbox.PermissionModeAsk), + }) + if result.Meta["shell_issue"] != "windows_shell_syntax" { + t.Fatalf("expected windows_shell_syntax block to still apply under require_escalated, got %#v", result) + } + }) +} + +// TestBashToolIgnoresMsysMarkersInCommandArgumentsAfterFailure guards the +// post-execution MSYS heuristic against treating the command line itself as +// evidence. The helper command below fails for a reason unrelated to MSYS +// (a plain non-zero exit), but its own argument text quotes an MSYS crash +// marker the way a PR-comment or commit-message argument might. Before the +// fix, detectShellOutputIssue scanned command+output together and would have +// misdiagnosed this as an MSYS sandbox failure even though the real +// stdout/stderr never mentions MSYS at all. +func TestBashToolIgnoresMsysMarkersInCommandArgumentsAfterFailure(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only MSYS output guard") + } + root := t.TempDir() + // The helper only reads argv[2] ("fail"); this trailing quoted argument is + // otherwise unused by the helper but still part of the command line text. + command := helperCommand("fail") + ` "fatal error - CreateFileMapping S-1-5-21, Win32 error 5. Terminating. cygheap_user::init"` + + result := NewBashTool(root).Run(context.Background(), map[string]any{ + "command": command, + }) + + if result.Status != StatusError || result.Meta["exit_code"] != "7" { + t.Fatalf("expected the helper's real exit 7 failure, got %s: %#v", result.Status, result) + } + if result.Meta["shell_issue"] == "windows_msys_sandbox" { + t.Fatalf("expected the MSYS marker in the command's own argument text to be ignored, got %#v", result) + } +} + func TestBashToolRunsWithDegradedUnavailableNativeSandbox(t *testing.T) { root := t.TempDir() policy := sandbox.DefaultPolicy() diff --git a/internal/tools/exec_command.go b/internal/tools/exec_command.go index 0feed7e1..027eca89 100644 --- a/internal/tools/exec_command.go +++ b/internal/tools/exec_command.go @@ -517,7 +517,12 @@ func (tool execCommandTool) run(ctx context.Context, args map[string]any, engine if err != nil { return errorResult("Error: Invalid arguments for exec_command: " + err.Error()) } - if issue := detectShellCommandIssue(commandText, runtimeGOOS()); issue != nil { + // Resolve the command engine before the MSYS preflight check so an + // approved require_escalated call (commandEngine == nil, truly + // unsandboxed) can actually bypass the MSYS guard instead of being + // hard-blocked by the same check it was meant to escalate past. + commandEngine := commandEngineForSandboxPermissions(engine, sandboxPermissions) + if issue := detectShellCommandIssue(commandText, runtimeGOOS()); issue != nil && !msysGuardBypassed(issue, commandEngine) { return shellIssueBlockResult(*issue) } if interactive := zeroSandbox.DetectInteractiveCommand(commandText, runtimeGOOS()); interactive.Interactive { @@ -860,6 +865,12 @@ func execToolResult(input execToolResultInput) Result { status = StatusError } body := formatExecCommandOutput(output, input.sessionID, input.exited, input.exitCode, input.interrupted) + if status == StatusError && input.exited && !input.interrupted { + if issue := detectShellOutputIssue(output, runtimeGOOS()); issue != nil { + meta["shell_issue"] = issue.Kind + body = appendShellIssueHint(body, *issue) + } + } if input.outputBufferTruncated { // Appended after truncateExecOutput's own head/tail slicing, not // embedded in the text that goes through it — a marker inside that diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index b7743f50..92728526 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -141,6 +141,46 @@ func TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval(t *testin } } +// TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval mirrors +// TestBashToolRequireEscalatedMsysGuard for exec_command: the MSYS sandbox +// guard exists only because MSYS/Cygwin coreutils fail under the +// write-restricted sandbox, so once require_escalated is actually approved +// (unsandboxed execution), the guard must not block the same command it was +// meant to let escalate past. +func TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("windows-only MSYS sandbox guard") + } + root := t.TempDir() + manager := newExecSessionManager() + registry := NewRegistry() + registry.Register(NewScopedExecCommandTool(root, nil, manager)) + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: root, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Message: "native sandbox unavailable"}, + }) + + result := registry.RunWithOptions(context.Background(), ExecCommandToolName, map[string]any{ + "cmd": "cat somefile.txt", + "sandbox_permissions": string(SandboxPermissionsRequireEscalated), + }, RunOptions{ + PermissionGranted: true, + Sandbox: engine, + PermissionMode: string(sandbox.PermissionModeAsk), + }) + + // Assert on the preflight block sentinel (exit_code "-1", set only by + // shellIssueBlockResult) rather than shell_issue: once the guard is + // bypassed, "cat somefile.txt" actually runs, and its real, + // PATH-dependent output could otherwise trip the unrelated + // post-execution detectShellOutputIssue heuristic and make this + // assertion flaky for reasons unrelated to the guard under test. + if result.Meta["exit_code"] == "-1" { + t.Fatalf("expected approved require_escalated to bypass the MSYS guard, got blocked: %#v", result) + } +} + func TestExecCommandReturnsExitCodeWhenCommandCompletesDuringInitialYield(t *testing.T) { root := t.TempDir() manager := newExecSessionManager() diff --git a/internal/tools/shell_runtime.go b/internal/tools/shell_runtime.go index 9d5a0198..72f9c7cc 100644 --- a/internal/tools/shell_runtime.go +++ b/internal/tools/shell_runtime.go @@ -3,6 +3,7 @@ package tools import ( "regexp" "strings" + "unicode" ) type shellRuntime struct { @@ -17,14 +18,31 @@ type shellIssue struct { Suggestion string } +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." + +// 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 +// cmd.exe-native command, and so fail under the write-restricted Windows +// sandbox (#458). Every Windows MSYS-detection path (the preflight command +// scan below, the exported MsysProneCommandName, and the known-safe-segment +// guard in internal/agent/command_prefix.go) derives from this one set, so +// they cannot drift out of sync with each other. +var windowsMsysProneNames = map[string]bool{ + "cat": true, "cut": true, "expr": true, "grep": true, "head": true, + "id": true, "ls": true, "nl": true, "paste": true, "rev": true, + "seq": true, "stat": true, "tail": true, "tr": true, "uname": true, + "uniq": true, "wc": true, "which": true, "awk": true, "sed": true, + "xargs": true, +} + var ( windowsBashStyleCDPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)cd\s+/(?:[a-ce-z0-9_./~-]|d[a-z0-9_./~-])[a-z0-9_./~-]*`) - windowsLSCommandPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)ls\b(?:\s+|$)`) - // windowsPosixUtilityPattern catches the other common POSIX coreutils/grep - // family commands cmd.exe has no equivalent for (unlike `dir`/`findstr`, - // which sound similar enough that a model might reach for the POSIX name - // instead) — most often seen piped in, e.g. `git log ... | head`. - windowsPosixUtilityPattern = regexp.MustCompile(`(?i)(^|[&|;]\s*)(head|tail|grep|wc|awk|sed|cut|xargs|tr)(?:\s+|$)`) + // windowsMsysBinaryPathPattern catches explicit Git-for-Windows / MSYS usr\bin + // paths. These executables are valid Windows PE files but fail under the + // write-restricted sandbox with CreateFileMapping ACCESS_DENIED (#458). + windowsMsysBinaryPathPattern = regexp.MustCompile(`(?i)(?:\\usr\\bin\\|\\mingw64\\bin\\|msys-2\.0\.dll|cygwin1\.dll)`) ) func detectShellRuntime(goos string) shellRuntime { @@ -37,48 +55,162 @@ func detectShellRuntime(goos string) shellRuntime { func shellGuidanceForGOOS(goos string) string { runtime := detectShellRuntime(goos) if goos == "windows" { - return "Uses " + runtime.Syntax + " syntax on Windows; prefer cwd over cd when changing directories." + return "Uses " + runtime.Syntax + " syntax on Windows; prefer cwd over cd when changing directories. MSYS/Cygwin coreutils on PATH (Git for Windows usr\\bin) are not sandbox-compatible; prefer native Zero file tools." } guidance := "Uses " + runtime.Syntax + " syntax." if goos == "darwin" { - // `ps` is setuid root and cannot run under the macOS sandbox; `pgrep` needs a - // blocked system service. Point the model at the tools that DO work so it - // doesn't waste turns: lsof to find a process, kill to stop it. guidance += " To find or stop a process, use `lsof -i :PORT` (or `lsof -nP -iTCP -sTCP:LISTEN`) for the PID then `kill `; `ps` and `pgrep` do not work under the sandbox." } return guidance } +// MsysProneCommandName reports whether a bare command name commonly resolves to +// a Git-for-Windows MSYS binary that fails under the Windows restricted sandbox. +func MsysProneCommandName(name string) bool { + return windowsMsysProneNames[strings.ToLower(strings.TrimSpace(name))] +} + +func windowsMsysSandboxIssue(message string) *shellIssue { + return &shellIssue{ + Kind: windowsMsysSandboxKind, + Message: message, + Suggestion: windowsMsysSandboxSuggestion, + } +} + +// windowsCommandSegments splits a command into cmd.exe-operator-separated +// segments (&, |, and their doubled forms &&/||), respecting double quotes +// (cmd.exe's grouping construct) and the caret (^) escape character, so an +// operator or command name mentioned inside a quoted argument (e.g. a commit +// message or PR comment body), or an operator escaped with ^ (cmd.exe prints +// `echo ^| head` as literal text instead of piping to head), is not mistaken +// for a real segment boundary or invocation. Unlike bash, cmd.exe does not +// treat ; as a statement separator, so it is left as ordinary argument text +// (e.g. `echo foo; head` is a single `echo` invocation with literal args). +func windowsCommandSegments(command string) []string { + var segments []string + var current strings.Builder + inQuotes := false + runes := []rune(command) + for i := 0; i < len(runes); i++ { + c := runes[i] + if !inQuotes && c == '^' && i+1 < len(runes) { + current.WriteRune(c) + i++ + current.WriteRune(runes[i]) + continue + } + if c == '"' { + inQuotes = !inQuotes + current.WriteRune(c) + continue + } + if !inQuotes && (c == '&' || c == '|') { + if seg := strings.TrimSpace(current.String()); seg != "" { + segments = append(segments, seg) + } + current.Reset() + continue + } + current.WriteRune(c) + } + if seg := strings.TrimSpace(current.String()); seg != "" { + segments = append(segments, seg) + } + return segments +} + +// firstCommandWord returns the first token of a command segment. A leading +// double-quoted span counts as one token with its quotes stripped, since +// cmd.exe treats a quoted path as a single argument: the command invoked by +// `"C:\Program Files\Git\usr\bin\head.exe" file.txt` is the quoted path, not +// "C:\Program. For an unquoted command, the token ends at whitespace or a +// redirection operator (<, >): cmd.exe accepts redirection attached directly +// to the command name with no separating space (head>out.txt, catout.txt" as one word and +// miss the invoked command. +func firstCommandWord(segment string) string { + trimmed := strings.TrimSpace(segment) + if trimmed == "" { + return "" + } + if trimmed[0] == '"' { + if end := strings.IndexByte(trimmed[1:], '"'); end >= 0 { + return trimmed[1 : end+1] + } + return trimmed[1:] + } + if end := strings.IndexFunc(trimmed, isCommandWordBoundary); end >= 0 { + return trimmed[:end] + } + return trimmed +} + +func isCommandWordBoundary(r rune) bool { + return unicode.IsSpace(r) || r == '<' || r == '>' +} + +// msysProneCommandWord reports whether word (the first token of a command +// segment, as returned by firstCommandWord) names an MSYS-prone coreutil, +// bare or with a directory prefix and/or .exe suffix (head, head.exe, +// C:\...\usr\bin\head.exe, ...). +func msysProneCommandWord(word string) bool { + word = strings.Trim(word, `"`) + if i := strings.LastIndexAny(word, `\/`); i >= 0 { + word = word[i+1:] + } + word = strings.TrimSuffix(strings.ToLower(word), ".exe") + return MsysProneCommandName(word) +} + func detectShellCommandIssue(command string, goos string) *shellIssue { if goos != "windows" { return nil } trimmed := strings.TrimSpace(command) - if windowsBashStyleCDPattern.MatchString(trimmed) || - windowsLSCommandPattern.MatchString(trimmed) { + if windowsBashStyleCDPattern.MatchString(trimmed) { 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.", Suggestion: "Use the cwd argument instead of cd, use Windows cmd.exe syntax, or use native tools such as list_directory, read_file, grep, and glob.", } } - if windowsPosixUtilityPattern.MatchString(trimmed) { - return &shellIssue{ - Kind: "windows_shell_syntax", - Message: "Command uses a POSIX utility (head/tail/grep/wc/awk/sed/cut/xargs/tr) that Windows cmd.exe does not have.", - Suggestion: "Use cmd.exe equivalents (e.g. `findstr` for grep, `more` to page output) or Zero's native tools (grep, read_file with offset/limit) instead of piping to a POSIX utility.", + // Check the first word of each operator-separated segment (not the raw + // text anywhere in the command) against the MSYS binary-path pattern and + // the single MSYS-prone name set, covering bare names (head), .exe names + // (head.exe), and directory-prefixed forms (C:\...\head.exe) uniformly. + // Being segment/word anchored rather than a whole-string regex or scan, + // neither check matches text that only appears inside a quoted argument + // (e.g. a commit message or PR comment body discussing head.exe). + for _, segment := range windowsCommandSegments(trimmed) { + word := firstCommandWord(segment) + if windowsMsysBinaryPathPattern.MatchString(word) { + return windowsMsysSandboxIssue("Command invokes an MSYS/Cygwin binary path that cannot run under Zero's Windows sandbox.") + } + if msysProneCommandWord(word) { + return windowsMsysSandboxIssue("Command uses a POSIX coreutil (head/tail/grep/cat/...) that commonly resolves to Git-for-Windows MSYS binaries incompatible with the Windows sandbox.") } } return nil } -func detectShellOutputIssue(command string, output string, goos string) *shellIssue { +// 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 +// comment --body` quoting a sample failure), not something the shell +// produced, so treating it as evidence would reintroduce the same +// quoted-text false positives the preflight command-position check exists to +// avoid, just after execution instead of before it. +func detectShellOutputIssue(output string, goos string) *shellIssue { if goos != "windows" { return nil } - haystack := strings.ToLower(command + "\n" + output) - if strings.Contains(haystack, "the syntax of the command is incorrect") || - strings.Contains(haystack, "is not recognized as an internal or external command") { + lower := strings.ToLower(output) + if msysRuntimeFailedInOutput(lower) { + return windowsMsysSandboxIssue("An MSYS/Cygwin runtime failed under Zero's Windows sandbox (ACCESS_DENIED during MSYS startup).") + } + if strings.Contains(lower, "the syntax of the command is incorrect") || + strings.Contains(lower, "is not recognized as an internal or external command") { return &shellIssue{ Kind: "windows_shell_syntax", Message: "Windows cmd.exe rejected the command syntax.", @@ -88,6 +220,32 @@ func detectShellOutputIssue(command string, output string, goos string) *shellIs return nil } +func msysRuntimeFailedInOutput(lower string) bool { + if strings.Contains(lower, "fatal error - createfilemapping") { + return true + } + if strings.Contains(lower, "couldn't create signal pipe") && strings.Contains(lower, "win32 error 5") { + return true + } + if strings.Contains(lower, "cygheap_user::init") && strings.Contains(lower, "fatal error") { + return true + } + if strings.Contains(lower, "usr\\bin\\") && strings.Contains(lower, "fatal error") { + return true + } + if !strings.Contains(lower, "win32 error 5") || !strings.Contains(lower, "terminating") { + return false + } + // Anchor the broad win32-error-5 fallback to an MSYS-specific marker so + // unrelated access-denied failures are not mislabeled as MSYS sandbox + // incompatibilities. + return strings.Contains(lower, `usr\bin\`) || + strings.Contains(lower, "cygheap") || + strings.Contains(lower, "msys-2.0.dll") || + strings.Contains(lower, "cygwin1.dll") || + strings.Contains(lower, "[main]") +} + func appendShellIssueHint(output string, issue shellIssue) string { output = strings.TrimRight(output, "\r\n") hint := "[zero] shell issue: " + issue.Message diff --git a/internal/tools/shell_runtime_test.go b/internal/tools/shell_runtime_test.go new file mode 100644 index 00000000..6c3ac5da --- /dev/null +++ b/internal/tools/shell_runtime_test.go @@ -0,0 +1,208 @@ +package tools + +import ( + "strings" + "testing" +) + +func TestDetectShellCommandIssueFlagsMsysBinaryPaths(t *testing.T) { + for _, command := range []string{ + `for /F %i in ('whoami') do echo %i | "C:\Program Files\Git\usr\bin\head.exe" -1`, + `C:\Git\usr\bin\grep.exe pattern file.txt`, + } { + issue := detectShellCommandIssue(command, "windows") + if issue == nil { + t.Fatalf("expected MSYS path issue for %q", command) + } + if issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected windows_msys_sandbox kind, got %q", issue.Kind) + } + if !strings.Contains(issue.Suggestion, "require_escalated") { + t.Fatalf("expected escalation guidance, got %#v", issue) + } + } +} + +func TestDetectShellCommandIssueFlagsStandaloneCat(t *testing.T) { + issue := detectShellCommandIssue(`cat README.md`, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS sandbox issue for cat, got %#v", issue) + } +} + +func TestDetectShellOutputIssueFlagsMsysCreateFileMappingError(t *testing.T) { + output := `0 [main] head (3568) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - CreateFileMapping S-1-5-21-3149109338-1484423945-518236903-1001.1, Win32 error 5. Terminating.` + issue := detectShellOutputIssue(output, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS output issue, got %#v", issue) + } + if !strings.Contains(issue.Suggestion, "require_escalated") { + t.Fatalf("expected escalation guidance, got %#v", issue) + } +} + +func TestDetectShellOutputIssueFlagsMsysSignalPipeError(t *testing.T) { + output := `0 [main] head (39684) cygheap_user::init: NtSetInformationToken (TokenDefaultDacl), 0xC0000022 +648 [main] head (39684) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - couldn't create signal pipe, Win32 error 5` + issue := detectShellOutputIssue(output, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS output issue, got %#v", issue) + } +} + +func TestDetectShellOutputIssueFlagsMsysTerminatingWithMsysMarker(t *testing.T) { + output := `1 [main] tail (4321) tail: *** MapViewOfFileEx failed, Win32 error 5. Terminating.` + issue := detectShellOutputIssue(output, "windows") + if issue == nil || issue.Kind != "windows_msys_sandbox" { + t.Fatalf("expected MSYS output issue, got %#v", issue) + } +} + +func TestDetectShellOutputIssueIgnoresNonMsysWin32Error5(t *testing.T) { + output := `myapp.exe: unable to open service handle, Win32 error 5 (access denied). Terminating worker.` + issue := detectShellOutputIssue(output, "windows") + if issue != nil { + t.Fatalf("expected no issue for non-MSYS access-denied output, got %#v", issue) + } +} + +func TestShellIssueBlockResultMsysCommand(t *testing.T) { + result := shellIssueBlockResult(*detectShellCommandIssue(`cat README.md`, "windows")) + if result.Status != StatusError { + t.Fatalf("status = %q, want error", result.Status) + } + for _, want := range []string{"[zero] shell issue:", "MSYS/Cygwin", "grep", "read_file", "require_escalated"} { + if !strings.Contains(result.Output, want) { + t.Fatalf("expected %q in blocked output, got %q", want, result.Output) + } + } + if result.Meta["shell_issue"] != "windows_msys_sandbox" { + t.Fatalf("meta shell_issue = %q", result.Meta["shell_issue"]) + } +} + +func TestMsysProneCommandName(t *testing.T) { + if !MsysProneCommandName("HEAD") || MsysProneCommandName("echo") { + t.Fatalf("unexpected MsysProneCommandName results") + } +} + +// TestDetectShellCommandIssueFlagsExprAndLsConsistently guards against the +// preflight regex list drifting from the canonical windowsMsysProneNames set +// (both listed expr and ls as MSYS-prone, but the old regex alternations +// omitted expr entirely and let ls hit the older windows_shell_syntax branch +// first, so it never got MSYS-kind guidance). +func TestDetectShellCommandIssueFlagsExprAndLsConsistently(t *testing.T) { + for _, command := range []string{ + `expr 1 + 1`, + `expr.exe 1 + 1`, + `ls -la`, + `ls`, + } { + issue := detectShellCommandIssue(command, "windows") + if issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected windows_msys_sandbox for %q, got %#v", command, issue) + } + } +} + +// TestDetectShellCommandIssueIgnoresQuotedMsysMentions guards against +// treating an MSYS-prone name that only appears inside a quoted argument +// (e.g. a commit message, a PR comment body, or a doc string discussing the +// command) as an actual invocation. The preflight check must anchor on the +// first word of each command segment, not scan the raw text anywhere. +func TestDetectShellCommandIssueIgnoresQuotedMsysMentions(t *testing.T) { + for _, command := range []string{ + `git commit -m "fix head.exe crash"`, + `gh pr comment --body "grep.exe fails under MSYS"`, + `echo "log | head is broken on windows"`, + `git commit -m "note: | head does not work here"`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected quoted MSYS mention to pass for %q, got %#v", command, issue) + } + } + + // A real invocation alongside quoted text must still be caught. + if issue := detectShellCommandIssue(`echo "not a real command" && head file.txt`, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real head invocation to still be flagged, got %#v", issue) + } +} + +// TestDetectShellCommandIssueIgnoresQuotedMsysPathMentions guards the +// explicit MSYS-binary-path check the same way as the coreutil-name check: +// a full usr\bin\ path that only appears inside a quoted argument (e.g. a +// commit message describing the failure) must not be treated as an +// invocation, since the path check is now anchored to the first word of each +// command segment rather than scanned across the raw command text. +func TestDetectShellCommandIssueIgnoresQuotedMsysPathMentions(t *testing.T) { + for _, command := range []string{ + `git commit -m "C:\Program Files\Git\usr\bin\head.exe fails"`, + `gh pr comment --body "C:\Git\usr\bin\grep.exe is blocked"`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected quoted MSYS path mention to pass for %q, got %#v", command, issue) + } + } + + // A real invocation by full path must still be caught. + if issue := detectShellCommandIssue(`C:\Git\usr\bin\grep.exe pattern file.txt`, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real MSYS path invocation to still be flagged, got %#v", issue) + } +} + +// TestDetectShellCommandIssueRespectsCaretEscapedOperators guards against +// misreading cmd.exe's ^ escape character: `echo ^| head` prints the pipe and +// "head" as literal text (the caret escapes the pipe so it never splits into +// a separate `head` invocation), and `echo foo; head` is a single `echo` +// command with literal arguments since cmd.exe (unlike bash) does not treat +// ; as a statement separator. +func TestDetectShellCommandIssueRespectsCaretEscapedOperators(t *testing.T) { + for _, command := range []string{ + `echo ^| head`, + `echo ^& head`, + `echo foo; head`, + } { + if issue := detectShellCommandIssue(command, "windows"); issue != nil { + t.Fatalf("expected no issue for %q, got %#v", command, issue) + } + } + + // An unescaped pipe must still split into a real head invocation. + if issue := detectShellCommandIssue(`echo foo | head`, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real head invocation to still be flagged, got %#v", issue) + } +} + +// TestDetectShellCommandIssueFlagsRedirectionAttachedToCommand guards against +// firstCommandWord treating cmd.exe redirection operators as part of the +// command name. cmd.exe accepts redirection with no separating space +// (head>out.txt, catout.txt" as one word and miss the invoked command entirely. +func TestDetectShellCommandIssueFlagsRedirectionAttachedToCommand(t *testing.T) { + for _, command := range []string{ + `some-command | head>out.txt`, + `catmatches.txt pattern`, + } { + issue := detectShellCommandIssue(command, "windows") + if issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected windows_msys_sandbox for %q, got %#v", command, issue) + } + } +} + +// TestDetectShellOutputIssueSignatureOmitsCommandText documents, at the type +// level, that detectShellOutputIssue can no longer take the command line as +// evidence: it only accepts the real output. Harmless output must not be +// flagged, and output that genuinely carries the MSYS failure markers must +// still be flagged. +func TestDetectShellOutputIssueSignatureOmitsCommandText(t *testing.T) { + if issue := detectShellOutputIssue("hello from bash", "windows"); issue != nil { + t.Fatalf("expected no issue for harmless output, got %#v", issue) + } + output := `0 [main] head (3568) C:\Program Files\Git\usr\bin\head.exe: *** fatal error - CreateFileMapping ..., Win32 error 5. Terminating.` + if issue := detectShellOutputIssue(output, "windows"); issue == nil || issue.Kind != windowsMsysSandboxKind { + t.Fatalf("expected real MSYS output to still be flagged, got %#v", issue) + } +}