mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 17:57:54 -10:00
* fix(tools): block MSYS coreutils under Windows sandbox Git for Windows usr\bin coreutils (head, grep, cat, ...) are MSYS PE binaries that fail with ACCESS_DENIED and cygheap/signal-pipe errors when Zero runs them in the write-restricted Windows sandbox. Detect and block these commands before execution, surface MSYS runtime failures from stderr, steer agents toward native Zero tools or require_escalated, and exclude MSYS-prone names from known-safe command tails. Refs #458 * fix(tools): anchor the last MSYS failure heuristic to MSYS markers CodeRabbit review feedback: the win32-error-5 plus terminating check had no MSYS-specific anchor, so unrelated access-denied failures could be mislabeled as windows_msys_sandbox. Require an MSYS marker like the neighboring checks do. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(tools): let escalation bypass the MSYS guard and make detection quote-aware Addresses jatmn's review on PR #476: - bash and exec_command called detectShellCommandIssue before resolving sandbox_permissions into a command engine, so a require_escalated retry of a blocked command was still hard-blocked by the same preflight check. Resolve the command engine first and only bypass the windows_msys_sandbox issue kind when that resolves to true unsandboxed execution; other issue kinds (real cmd.exe syntax problems) still block regardless of escalation. - Unify all MSYS-prone command name checks (MsysProneCommandName, the preflight scan, and transitively command_prefix.go's known-safe guard) behind one windowsMsysProneNames set. This fixes expr (in the name set but missing from the old regex alternations, so never proactively flagged) and ls (hit the older windows_shell_syntax branch first, so it got different metadata than every other MSYS-prone name). - Make the preflight scan quote-aware: split the command into cmd.exe- operator-separated segments respecting double-quote grouping, and only check the first word of each segment instead of scanning raw text anywhere. Fixes false positives on quoted text like commit messages or PR comment bodies that merely mention a coreutil name. * test(tools): decouple MSYS-guard bypass tests from real command output TestBashToolRequireEscalatedMsysGuard and TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval asserted shell_issue == "" to prove the require_escalated bypass worked. Once bypassed, "cat somefile.txt" actually runs unsandboxed, and its real, PATH-dependent output could trip the unrelated post-execution detectShellOutputIssue heuristic, failing the assertion for reasons unrelated to the preflight guard under test. Assert on the exit_code "-1" sentinel (set only by shellIssueBlockResult) instead, which directly reflects whether the preflight guard blocked the command. * fix(tools): anchor MSYS path detection to command position, fix prefix approval gap Address jatmn's latest review on PR #476: - shell_runtime.go: the explicit MSYS binary-path check (usr\bin\, mingw64\bin\, msys-2.0.dll, cygwin1.dll) ran against the raw command string, so a quoted mention (e.g. a commit message describing a usr\bin\head.exe failure) was still hard-blocked. It now runs against the first word of each operator-separated segment, same as the coreutil-name check. windowsCommandSegments also stopped treating ; as a command separator (cmd.exe does not split on ;, unlike bash) and now respects cmd.exe's ^ escape character, so escaped operators like echo ^| head are no longer misread as a real head invocation. - command_prefix.go: proposedCommandPrefix could offer to approve a single segment (e.g. "ps aux" in "ps aux | head -5") without checking that every other segment was independently safe. Since approving a prefix escalates the whole command to bypass the sandbox, an MSYS-prone or otherwise unsafe segment could ride along uncovered. It now only proposes a segment as the prefix when every other segment in the command is known-safe on its own. Added regression tests for quoted MSYS path mentions, caret-escaped operators, semicolon text, and the prefix-approval gap. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(tools): treat redirection as a command boundary, stop using command text as output evidence Addresses jatmn's latest review on PR #476: - firstCommandWord only split on whitespace, so cmd.exe redirection attached directly to a command name (head>out.txt, cat<in.txt, grep>matches.txt) was read as one word and never matched against the MSYS-prone name set. It now also stops at < and >. - detectShellOutputIssue scanned command+output together, so a command whose own argument text merely quoted an MSYS failure string (e.g. a gh pr comment --body describing the bug) could be mislabeled after running even though the real stdout/stderr was unrelated. Removed the command parameter entirely from detectShellOutputIssue and both call sites (bash.go, exec_command.go), so the command line can never be used as post-execution evidence again. * fix(tools): remove unused truncateHeadTail wrapper CodeRabbit nitpick: truncateHeadTailWithTotal is called directly everywhere, so this wrapper is dead code that trips golangci-lint's unused check and fails CI. --------- Co-authored-by: Claude Fable 5 <[email protected]>
197 lines
6.6 KiB
Go
197 lines
6.6 KiB
Go
package agent
|
|
|
|
import (
|
|
"runtime"
|
|
"testing"
|
|
|
|
"github.com/Gitlawb/zero/internal/sandbox"
|
|
"github.com/Gitlawb/zero/internal/tools"
|
|
)
|
|
|
|
func TestProposedCommandPrefixUsesSafeSimpleCommands(t *testing.T) {
|
|
got := proposedCommandPrefix("bash", map[string]any{"command": "go test ./..."})
|
|
want := []string{"go", "test", "./..."}
|
|
if !equalStringSlices(got, want) {
|
|
t.Fatalf("prefix = %#v, want %#v", got, want)
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixSupportsExecCommand(t *testing.T) {
|
|
got := proposedCommandPrefix("exec_command", map[string]any{"cmd": "go test ./..."})
|
|
want := []string{"go", "test", "./..."}
|
|
if !equalStringSlices(got, want) {
|
|
t.Fatalf("prefix = %#v, want %#v", got, want)
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixHonorsValidatedRequestedPrefix(t *testing.T) {
|
|
got := proposedCommandPrefix("bash", map[string]any{
|
|
"command": "git status --short",
|
|
"prefix_rule": []any{"git", "status"},
|
|
})
|
|
want := []string{"git", "status"}
|
|
if !equalStringSlices(got, want) {
|
|
t.Fatalf("prefix = %#v, want %#v", got, want)
|
|
}
|
|
}
|
|
|
|
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",
|
|
"prefix_rule": []any{"git", "status"},
|
|
})
|
|
want := []string{"git", "status"}
|
|
if !equalStringSlices(got, want) {
|
|
t.Fatalf("prefix = %#v, want %#v", got, want)
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixRejectsRequestedPrefixThatDoesNotCoverSegments(t *testing.T) {
|
|
got := proposedCommandPrefix("bash", map[string]any{
|
|
"command": "ps aux && npm install",
|
|
"prefix_rule": []any{"ps", "aux"},
|
|
})
|
|
if got != nil {
|
|
t.Fatalf("partial requested prefix should be rejected, got %#v", got)
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixRejectsUnsafeRequestedPrefix(t *testing.T) {
|
|
got := proposedCommandPrefix("bash", map[string]any{
|
|
"command": "git status --short",
|
|
"prefix_rule": []any{"git"},
|
|
})
|
|
if got != nil {
|
|
t.Fatalf("broad requested prefix should be rejected, got %#v", got)
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixRejectsUnsafeShellForms(t *testing.T) {
|
|
cases := []string{
|
|
"cat < in > out",
|
|
"FOO=bar go test",
|
|
"echo $(whoami)",
|
|
"cat *.go",
|
|
"bash -lc go test",
|
|
}
|
|
for _, command := range cases {
|
|
t.Run(command, func(t *testing.T) {
|
|
if got := proposedCommandPrefix("bash", map[string]any{"command": command}); got != nil {
|
|
t.Fatalf("unsafe command got prefix %#v", got)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixRejectsUnsafeLaunchers(t *testing.T) {
|
|
cases := []string{
|
|
"find . -type f",
|
|
"xargs rm -rf /tmp/x",
|
|
"timeout 5 go test ./...",
|
|
"nice go test ./...",
|
|
"nohup go test ./...",
|
|
"watch ls",
|
|
"ssh host ls",
|
|
"make test",
|
|
"npm run dev",
|
|
"command git status",
|
|
"eval echo hi",
|
|
"exec echo hi",
|
|
"python script.py",
|
|
"node script.js",
|
|
"./script.sh --flag",
|
|
"/tmp/script.sh --flag",
|
|
}
|
|
for _, command := range cases {
|
|
t.Run(command, func(t *testing.T) {
|
|
if got := proposedCommandPrefix("bash", map[string]any{"command": command}); got != nil {
|
|
t.Fatalf("unsafe launcher got prefix %#v", got)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
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": 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"})
|
|
|
|
if grant, ok, session := matchCommandPrefix("bash", map[string]any{"command": "ps aux && npm install"}, Options{Sandbox: engine}); ok {
|
|
t.Fatalf("match = %#v session=%v, want no match because npm segment is uncovered", grant, session)
|
|
}
|
|
}
|
|
|
|
func TestProposedCommandPrefixRejectsRequestedUnsafeLauncherPrefix(t *testing.T) {
|
|
got := proposedCommandPrefix("bash", map[string]any{
|
|
"command": "find . -type f",
|
|
"prefix_rule": []any{"find", "."},
|
|
})
|
|
if got != nil {
|
|
t.Fatalf("unsafe requested launcher prefix should be rejected, got %#v", got)
|
|
}
|
|
}
|