diff --git a/internal/agent/command_prefix_test.go b/internal/agent/command_prefix_test.go index 69508a67c..a6e2adfac 100644 --- a/internal/agent/command_prefix_test.go +++ b/internal/agent/command_prefix_test.go @@ -99,6 +99,43 @@ func TestProposedCommandPrefixRejectsUnsafeRequestedPrefix(t *testing.T) { } } +// The launcher denylist is only worth anything if it holds on the path a user +// actually sees. A model naming its own prefix_rule is that path: before +// launcher names were normalized, "python3.11" was offered as a one-token +// grant, and hasStringPrefix then matched every later python3.11 invocation. +func TestProposedCommandPrefixRejectsLauncherSpellingsInRequestedPrefix(t *testing.T) { + for _, testCase := range []struct { + command string + rule []any + }{ + {"python3.11 script.py", []any{"python3.11"}}, + {"python.exe script.py", []any{"python.exe"}}, + {"node.exe app.js", []any{"node.exe"}}, + {"cmd /c whoami", []any{"cmd"}}, + {"busybox sh -c id", []any{"busybox"}}, + {"uv run main.py", []any{"uv", "run"}}, + {"python3.11 -c import_os", []any{"python3.11", "-c"}}, + } { + got := proposedCommandPrefix("bash", map[string]any{ + "command": testCase.command, + "prefix_rule": testCase.rule, + }) + if got != nil { + t.Errorf("prefix_rule %v for %q was offered as %#v, want rejected", testCase.rule, testCase.command, got) + } + } +} + +func TestProposedCommandPrefixStillOffersOrdinaryCommands(t *testing.T) { + got := proposedCommandPrefix("bash", map[string]any{ + "command": "cargo build --release", + "prefix_rule": []any{"cargo", "build"}, + }) + if len(got) != 2 || got[0] != "cargo" || got[1] != "build" { + t.Fatalf("ordinary prefix rule = %#v, want [cargo build]", got) + } +} + func TestProposedCommandPrefixRejectsUnsafeShellForms(t *testing.T) { cases := []string{ "cat < in > out", diff --git a/internal/execution/contracts.go b/internal/execution/contracts.go index 3003513a4..dd861ac8f 100644 --- a/internal/execution/contracts.go +++ b/internal/execution/contracts.go @@ -9,7 +9,16 @@ import ( "strings" ) -const PolicyVersion = 1 +// PolicyVersion is the version of the approval policy that recorded grants were +// evaluated under. Bump it whenever a change narrows what may be approved: the +// grant store then backs the file up, keeps deny grants, invalidates prior +// approvals, and tells the user once, instead of silently honoring approvals +// that the current rules would have refused. +// +// v2 tightened command-prefix validation so a launcher spelled with a version +// or an executable extension ("python3.11", "python.exe") can no longer be +// approved as an ordinary command. +const PolicyVersion = 2 type Origin string diff --git a/internal/sandbox/command_prefix.go b/internal/sandbox/command_prefix.go index 53a4efd4c..ed2fe651a 100644 --- a/internal/sandbox/command_prefix.go +++ b/internal/sandbox/command_prefix.go @@ -148,8 +148,10 @@ func unsafeCommandPrefix(prefix []string) bool { return true } } + normalized := append([]string(nil), prefix...) + normalized[0] = normalizeLauncherName(normalized[0]) for _, banned := range bannedCommandPrefixSuggestions { - if sameStringSlice(prefix, banned) { + if sameStringSlice(prefix, banned) || sameStringSlice(normalized, banned) { return true } } @@ -165,25 +167,118 @@ func unsafeCommandPrefixPart(part string) bool { } func unsafeCommandPrefixLauncher(program string) bool { - program = strings.ToLower(strings.TrimSpace(program)) - if strings.ContainsAny(program, `/\`) { + // A path is already refused. ":" and "~" are refused for the same reason: + // on Windows they introduce names that resolve to a different file than the + // one matched here, and no list can keep up with them. "PYTHON~1.EXE" is + // python.exe under an 8.3 short name, and 8.3 truncates the stem, so + // "POWERS~1.EXE" cannot be recognized by name at all; "python.exe::$DATA" + // names the same executable through its default stream. Refusing the shape + // costs a permission prompt on names no real command uses. + if strings.ContainsAny(program, `/\:~`) { return true } - switch program { - case "bash", "sh", "zsh", "/bin/bash", "/bin/zsh", - "pwsh", "powershell", "powershell.exe", - "env", "sudo", "doas", "su", "run0", "osascript", + name := normalizeLauncherName(program) + return bannedLauncherName(name) || bannedLauncherName(strings.TrimRight(name, "0123456789")) +} + +func bannedLauncherName(name string) bool { + switch name { + case "bash", "sh", "zsh", "dash", "ash", "ksh", "csh", "tcsh", "fish", "busybox", + "pwsh", "powershell", "cmd", "wsl", + "env", "sudo", "sudoedit", "doas", "su", "run0", "osascript", "command", "eval", "exec", "time", "find", "xargs", "timeout", "nice", "nohup", "watch", "setsid", "stdbuf", "ionice", - "ssh", "make", "npm", "npx", - "python", "python3", "py", "pythonw", "pyw", "pypy", "pypy3", - "node", "perl", "ruby", "php", "lua", "deno", "bun": + "ssh", "make", "npm", "npx", "pnpm", "yarn", "bunx", + "python", "py", "pythonw", "pyw", "pypy", "uv", "uvx", + "node", "nodejs", "perl", "ruby", "php", "lua", "deno", "bun": return true default: return false } } +// normalizeLauncherName reduces a program name to the launcher it actually runs +// so the list above cannot be stepped around by spelling the same interpreter +// differently: "python3.11", "python.exe" and "PYTHON3" all reach it as a name +// the list matches. internal/agent's commandName already normalizes case and +// Windows executable extensions on the allow side; this side matched raw, so +// every versioned or .exe-suffixed launcher validated as an ordinary command +// and persisted as a prefix grant that auto-allows every later invocation. +// +// A name is only ever narrowed toward an existing entry, so the failure +// direction is an extra permission prompt, never a silent grant. +func normalizeLauncherName(program string) string { + name := strings.ToLower(strings.TrimSpace(program)) + // Windows discards trailing dots and spaces when resolving a filename, so + // "python." starts python.exe. Drop them before anything matches on the + // result, on every platform: the deny side must not depend on which OS is + // reading the grant, since the grants file travels with a synced home + // directory. + name = strings.TrimRight(name, ". ") + for _, suffix := range []string{".exe", ".cmd", ".bat", ".com", ".ps1"} { + if strings.HasSuffix(name, suffix) { + name = strings.TrimSuffix(name, suffix) + name = strings.TrimRight(name, ". ") + break + } + } + return trimLauncherVersion(trimLauncherABISuffix(trimLauncherBuildSuffix(name))) +} + +// trimLauncherBuildSuffix drops the build-channel suffixes distributions and +// upstreams append to an otherwise unchanged interpreter, so "python3.11-dbg", +// "bash-static" and "pwsh-preview" are the launchers they say they are. The set +// is closed on purpose: a general "-word" strip would also swallow +// "node-gyp", "python3-config" and "ruby-lsp", which are ordinary tools a user +// should still be able to approve. +func trimLauncherBuildSuffix(name string) string { + for _, suffix := range []string{"-dbg", "-debug", "-static", "-nightly", "-preview", "-beta"} { + if strings.HasSuffix(name, suffix) { + return strings.TrimSuffix(name, suffix) + } + } + return name +} + +// trimLauncherABISuffix drops CPython's ABI flags from a versioned interpreter +// name, so "python3.7m" and "python3.6dm" reduce to their version, and the +// free-threaded builds "python3.13t" and "python3.13td" reduce to theirs. The +// flags are only removed when a digit sits underneath them, which leaves +// ordinary names that happen to end in those letters alone: "sha256sum" keeps +// its "sum", "zstd" its "td", "cat" its "t". +func trimLauncherABISuffix(name string) string { + trimmed := strings.TrimRight(name, "dmut") + if trimmed == name || trimmed == "" { + return name + } + if last := trimmed[len(trimmed)-1]; last < '0' || last > '9' { + return name + } + return trimmed +} + +// trimLauncherVersion drops a trailing "" version from a +// program name, so "python3.11" becomes "python3" and "python2.7" becomes +// "python2". Digits without a separator are part of the name, which keeps +// "base64", "7z" and "sha256sum" intact; the caller handles the "python3" shape +// by also testing the digit-stripped form. +func trimLauncherVersion(name string) string { + for { + trimmed := strings.TrimRight(name, "0123456789") + if trimmed == name || trimmed == "" { + return name + } + if !strings.HasSuffix(trimmed, ".") && !strings.HasSuffix(trimmed, "-") { + return name + } + next := trimmed[:len(trimmed)-1] + if next == "" { + return name + } + name = next + } +} + func sameStringSlice(left []string, right []string) bool { if len(left) != len(right) { return false diff --git a/internal/sandbox/command_prefix_test.go b/internal/sandbox/command_prefix_test.go new file mode 100644 index 000000000..c06458c37 --- /dev/null +++ b/internal/sandbox/command_prefix_test.go @@ -0,0 +1,115 @@ +package sandbox + +import "testing" + +func TestUnsafeCommandPrefixLauncherIgnoresVersionAndExtension(t *testing.T) { + for _, program := range []string{ + "python3", "python", "node", "git", "sh", "bash", "sudo", + "python3.11", "python3.12", "python2.7", "python3.13.1", + "python.exe", "python3.exe", "node.exe", "git.exe", "bash.exe", + "PYTHON3.11", "Node.EXE", + "cmd", "cmd.exe", "wsl", "wsl.exe", + // Windows resolves these to the same executable. + "python.", "python3.", "python3.11.", "python.exe.", "PYTHON.", + "python.cmd", "python.bat", "python.com", "python.ps1", + "PYTHON~1.EXE", "POWERS~1.EXE", "python.exe::$DATA", + // Versioned and ABI-suffixed interpreters. + "python3.7m", "python3.6dm", "pythonw3.11", "python-3.11", + // Free-threaded CPython (PEP 703) ships beside the GIL build. + "python3.13t", "python3.13t.exe", "python3.14t", "python3.13td", + // Distribution and build-channel spellings of the same interpreter. + "nodejs", "nodejs.exe", "node-nightly", "python3-dbg", "python3.11-dbg", + "python3.13t-dbg", "python3-debug", "python3.11-debug", "node-beta", + "pwsh-preview", "bash-static", "sudoedit", + // Twins of npm and npx, which the list already refuses. + "pnpm", "yarn", "bunx", "pnpm.cmd", "yarn.exe", + "perl5.36", "ruby3.2", "php8", "lua5.4", "node22.exe", "bash5", + } { + if !unsafeCommandPrefix([]string{program}) { + t.Errorf("unsafeCommandPrefix([%q]) = false, want true", program) + } + } +} + +func TestUnsafeCommandPrefixKeepsOrdinaryCommandsGrantable(t *testing.T) { + for _, prefix := range [][]string{ + {"cargo", "build"}, + {"go", "test"}, + {"ls", "-la"}, + {"rg", "--json"}, + {"7z", "l"}, + {"base64", "-d"}, + {"kubectl", "get", "pods"}, + {"docker", "ps"}, + {"sha256sum", "file"}, + {"gcc-13", "-c"}, + {"node-gyp", "build"}, + {"python3-config", "--includes"}, + {"s3cmd", "ls"}, + {"cat", "file"}, + {"zstd", "-d"}, + {"sqlite3", "db"}, + {"yt-dlp", "url"}, + {"perl-doc", "-h"}, + {"php-fpm", "-t"}, + {"ruby-lsp", "--version"}, + {"lua-language-server", "--check"}, + {"nodemon", "app.js"}, + } { + if unsafeCommandPrefix(prefix) { + t.Errorf("unsafeCommandPrefix(%q) = true, want false", prefix) + } + } +} +func TestNormalizeLauncherName(t *testing.T) { + for _, testCase := range []struct{ in, want string }{ + {"python3.11", "python3"}, + {"python2.7", "python2"}, + {"pypy3", "pypy3"}, + {"node.exe", "node"}, + {"POWERSHELL.EXE", "powershell"}, + {"7z", "7z"}, + {"base64", "base64"}, + {"sha256sum", "sha256sum"}, + {"python.", "python"}, + {"python3.11.exe.", "python3"}, + {"python3.7m", "python3"}, + {"python3.6dm", "python3"}, + {"python3.13t", "python3"}, + {"python3.13td", "python3"}, + {"python3.11-dbg", "python3"}, + {"node-nightly", "node"}, + {"node-gyp", "node-gyp"}, + {"python3-config", "python3-config"}, + {"zstd", "zstd"}, + {"cat", "cat"}, + {"cargo", "cargo"}, + {"", ""}, + } { + if got := normalizeLauncherName(testCase.in); got != testCase.want { + t.Errorf("normalizeLauncherName(%q) = %q, want %q", testCase.in, got, testCase.want) + } + } +} + +// Session-scoped grants are the agent's other entry point (loop.go grants them +// for a single turn without touching the store), so they must refuse the same +// launcher spellings the persisted path does. +func TestGrantCommandPrefixForSessionRefusesLauncherVariants(t *testing.T) { + engine := &Engine{commandPrefixes: newCommandPrefixGrantSet()} + engine.GrantCommandPrefixForSession("bash", []string{"cargo", "build"}) + if _, ok := engine.LookupCommandPrefixForSession("bash", []string{"cargo", "build", "--release"}); !ok { + t.Fatal("CONTROL BROKEN: an ordinary session prefix grant did not match") + } + for _, prefix := range [][]string{ + {"python3.11", "-c"}, + {"python.exe", "-c"}, + {"cmd", "/c"}, + } { + engine.GrantCommandPrefixForSession("bash", prefix) + command := append(append([]string(nil), prefix...), "whatever") + if _, ok := engine.LookupCommandPrefixForSession("bash", command); ok { + t.Errorf("session grant %q matched %q, want refused", prefix, command) + } + } +} diff --git a/internal/sandbox/grants_test.go b/internal/sandbox/grants_test.go index 2f00d40b7..5b00b5f02 100644 --- a/internal/sandbox/grants_test.go +++ b/internal/sandbox/grants_test.go @@ -2,12 +2,15 @@ package sandbox import ( "errors" + "fmt" "os" "path/filepath" "runtime" "strings" "testing" "time" + + "github.com/Gitlawb/zero/internal/execution" ) func TestGrantStorePersistsListsRevokesAndClears(t *testing.T) { @@ -343,7 +346,8 @@ func TestGrantStoreMigratesExactV1GrantAndReportsOnce(t *testing.T) { if err != nil { t.Fatalf("read rewritten grant file: %v", err) } - if !strings.Contains(string(raw), `"schemaVersion": 3`) || !strings.Contains(string(raw), `"policyVersion": 1`) || !strings.Contains(string(raw), `"write_file": [`) { + policyLine := fmt.Sprintf(`"policyVersion": %d`, execution.PolicyVersion) + if !strings.Contains(string(raw), `"schemaVersion": 3`) || !strings.Contains(string(raw), policyLine) || !strings.Contains(string(raw), `"write_file": [`) { t.Fatalf("grant file was not rewritten as a versioned grant store:\n%s", raw) } backup, err := os.ReadFile(path + ".v1.backup") @@ -406,6 +410,62 @@ func TestGrantStorePolicyChangePreservesDeniesAndInvalidatesApprovals(t *testing } } +// A prefix grant recorded before launcher-name normalization existed (a +// versioned or .exe-suffixed interpreter) no longer validates. It must reach +// the user through the policy migration — backup, denies preserved, one notice +// — and never make the whole grants file unreadable, which would silently drop +// their persisted deny grants too. +func TestGrantStoreMigratesPreNormalizationPrefixInsteadOfRejectingTheFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "sandbox-grants.json") + original := `{"schemaVersion":3,"policyVersion":1,"grants":{"bash":[{"toolName":"bash","decision":"deny","approvedAt":"2026-06-05T14:30:00Z"}]},"commandPrefixes":{"bash":[{"toolName":"bash","prefix":["python3.11","-c"],"approvedAt":"2026-06-05T14:30:00Z"},{"toolName":"bash","prefix":["cargo","build"],"approvedAt":"2026-06-05T14:30:00Z"}]}}` + if err := writeText(path, original); err != nil { + t.Fatal(err) + } + store, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatal(err) + } + grants, err := store.List() + if err != nil || len(grants) != 1 || grants[0].Decision != GrantDeny { + t.Fatalf("grants after migration = %#v err=%v, want the deny preserved", grants, err) + } + prefixes, err := store.ListCommandPrefixes() + if err != nil || len(prefixes) != 0 { + t.Fatalf("prefixes after migration = %#v err=%v, want all re-approved", prefixes, err) + } + if notice, err := store.ConsumeMigrationNotice(); err != nil || !strings.Contains(notice, "invalidated 2") { + t.Fatalf("notice = %q err=%v", notice, err) + } + if backup, err := os.ReadFile(path + ".policy-v1.backup"); err != nil || string(backup) != original { + t.Fatalf("backup = %q err=%v", backup, err) + } +} + +// The legacy-schema path is a separate migration from the policy bump above and +// prunes per grant rather than wholesale, so a pre-normalization prefix must be +// dropped there while the valid prefix beside it survives. +func TestGrantStoreLegacyMigrationDropsOnlyThePreNormalizationPrefix(t *testing.T) { + path := filepath.Join(t.TempDir(), "sandbox-grants.json") + original := `{"schemaVersion":2,"grants":{},"commandPrefixes":{"bash":[{"toolName":"bash","prefix":["python3.11","-c"],"approvedAt":"2026-06-05T14:30:00Z"},{"toolName":"bash","prefix":["git","status"],"approvedAt":"2026-06-05T14:30:00Z"}]}}` + if err := writeText(path, original); err != nil { + t.Fatal(err) + } + store, err := NewGrantStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatal(err) + } + prefixes, err := store.ListCommandPrefixes() + if err != nil { + t.Fatalf("ListCommandPrefixes after legacy migration returned error: %v", err) + } + if len(prefixes) != 1 || !sameStringSlice(prefixes[0].Prefix, []string{"git", "status"}) { + t.Fatalf("prefixes = %#v, want only [git status]", prefixes) + } + if notice, err := store.ConsumeMigrationNotice(); err != nil || !strings.Contains(notice, "migrated 1, invalidated 1") { + t.Fatalf("notice = %q err=%v", notice, err) + } +} + func TestGrantStorePersistsCommandPrefixes(t *testing.T) { store, err := NewGrantStore(StoreOptions{ FilePath: filepath.Join(t.TempDir(), "sandbox-grants.json"), @@ -481,6 +541,11 @@ func TestGrantStoreRejectsUnsafeCommandPrefixes(t *testing.T) { {"python", "script.py"}, {"./script.sh"}, {"git"}, + {"python3.11", "-c"}, + {"python.exe", "-c"}, + {"node.exe", "-e"}, + {"git.exe"}, + {"cmd", "/c"}, } { if _, err := store.GrantCommandPrefix(CommandPrefixInput{ToolName: "bash", Prefix: prefix}); err == nil { t.Fatalf("GrantCommandPrefix(%#v) succeeded, want validation error", prefix)