From f6e019151ad859614f120fa4d27a69b2e3d29a40 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 21:42:47 +0200 Subject: [PATCH 01/17] fix(sandbox): protect daemon token file --- internal/sandbox/manager_test.go | 37 +++++++++++++++++++++++++++----- internal/sandbox/profile.go | 21 ++++++++++++------ internal/sandbox/runner.go | 1 + internal/sandbox/runner_test.go | 1 + 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index a2b8550f2..90ed371b8 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -385,9 +385,13 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { if err := os.WriteFile(keyFile, []byte("{}"), 0o600); err != nil { t.Fatal(err) } + daemonTokenFile := filepath.Join(home, "daemon-token") + if err := os.WriteFile(daemonTokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } - paths := credentialDenyReadPathsIn(home, keyFile, nil) - for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile}) { + paths := credentialDenyReadPathsIn(home, keyFile, daemonTokenFile, nil) + for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile, daemonTokenFile}) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) } @@ -399,22 +403,45 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } // An explicit AllowRead entry covering a store is an opt-out. - optedOut := credentialDenyReadPathsIn(home, keyFile, []string{awsDir}) + optedOut := credentialDenyReadPathsIn(home, keyFile, daemonTokenFile, []string{awsDir, daemonTokenFile}) if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) } if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } + if stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop daemon token file", optedOut) + } - if got := credentialDenyReadPathsIn(" ", "", nil); len(got) != 0 { + if got := credentialDenyReadPathsIn(" ", "", "", nil); len(got) != 0 { t.Errorf("credential deny paths for blank home = %#v, want none", got) } // The GOOGLE_APPLICATION_CREDENTIALS target stays protected even when no // home directory is resolvable. - homeless := credentialDenyReadPathsIn("", keyFile, nil) + homeless := credentialDenyReadPathsIn("", keyFile, daemonTokenFile, nil) if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths without home = %#v, want key file included", homeless) } + if !stringSliceContains(homeless, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths without home = %#v, want daemon token file included", homeless) + } +} + +func TestPermissionProfileDeniesDaemonTokenFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read paths are disabled on Windows pending the ACL model") + } + tokenFile := filepath.Join(t.TempDir(), "daemon-token") + if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", tokenFile) + + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{tokenFile})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want daemon token file %q", profile.FileSystem.DenyRead, want) + } } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 194736667..bc818add7 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -148,9 +148,10 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop } // credentialDenyReadPaths returns default deny-read entries for well-known -// credential stores (~/.aws, ~/.config/gcloud, ~/.azure, and the file -// GOOGLE_APPLICATION_CREDENTIALS points to) so sandboxed commands cannot read -// cloud secrets under the read-all workspace posture. Two deliberate limits: +// credential stores (~/.aws, ~/.config/gcloud, ~/.azure, and the files named by +// GOOGLE_APPLICATION_CREDENTIALS and ZERO_DAEMON_REMOTE_TOKEN_FILE) so +// sandboxed commands cannot read secrets under the read-all workspace posture. +// Two deliberate limits: // // - Windows is skipped: a non-empty profile DenyRead switches the Windows // runner onto the capability-SID/ACL deny path and away from the @@ -167,14 +168,19 @@ func credentialDenyReadPaths(policy Policy) []string { return nil } // A failed home lookup only drops the home-based candidates; the - // GOOGLE_APPLICATION_CREDENTIALS target must be protected regardless. + // environment-selected credential targets must be protected regardless. home, _ := os.UserHomeDir() - return credentialDenyReadPathsIn(home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), policy.AllowRead) + return credentialDenyReadPathsIn( + home, + os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), + os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"), + policy.AllowRead, + ) } // credentialDenyReadPathsIn is the pure core of credentialDenyReadPaths, // separated so tests can exercise it against a synthetic home directory. -func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead []string) []string { +func credentialDenyReadPathsIn(home string, googleCredentials string, daemonTokenFile string, allowRead []string) []string { var candidates []string if home = strings.TrimSpace(home); home != "" { candidates = append(candidates, @@ -186,6 +192,9 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead if target := strings.TrimSpace(googleCredentials); target != "" { candidates = append(candidates, target) } + if target := strings.TrimSpace(daemonTokenFile); target != "" { + candidates = append(candidates, target) + } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) for _, path := range normalizeProfilePaths(candidates) { diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 506acb393..5cc256fae 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -1006,6 +1006,7 @@ func scrubSensitiveEnv(env []string, additionalKeys ...string) []string { "GH_TOKEN", "ZERO_WEBSEARCH_API_KEY", "ZERO_DAEMON_REMOTE_TOKEN", + "ZERO_DAEMON_REMOTE_TOKEN_FILE", } for _, descriptor := range providercatalog.All() { for _, key := range descriptor.AuthEnvVars { diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 67b66866c..882a007f7 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -698,6 +698,7 @@ func TestScrubSensitiveEnv(t *testing.T) { "ZERO_OAUTH_MY_SVC_CLIENT_SECRET=oauth-secret", "zero_oauth_second_client_secret=case-insensitive-secret", "ZERO_OAUTH_CLIENT_SECRET=not-a-provider-secret", + "ZERO_DAEMON_REMOTE_TOKEN_FILE=/home/user/daemon-token", "AWS_PROFILE=staging", "SAFE_VAR=hello", } From caccae4f67411d03ba952711d80f73ff5564506b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 12:51:05 +0200 Subject: [PATCH 02/17] fix(sandbox): protect the daemon token from Zero's own tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four P1 findings on #685. In-process tool boundary (findings 1 and 2). The token file only entered PermissionProfile.FileSystem.DenyRead, which shapes the OS sandbox profile for wrapped shell commands. read_file resolves scoped paths itself and grep/glob build exclusions from the policy, so a remote-controlled agent whose token file sits in its session workspace could read it with read_file/grep/glob, or truncate and replace it with a write tool. A new automatic protected-credential list is enforced by validatePathWithPolicy (read, write, and out-of-workspace) and by ReadExclusions/ReadExclusionGlobs. It is deliberately not Policy.DenyRead — whose emptiness gates escalated execution — and it is not re-includable through AllowRead, AllowWrite, a granted permission, or a session/turn permission profile. It applies on Windows too, where filesystem deny-read has no sandbox representation. Seatbelt write direction (finding 2, macOS). A read-denied path kept its file-write-unlink deny only, while the broad file-write* allow covered every workspace and temp root, leaving a credential file there truncatable and replaceable. Read denies now also deny file-write*, which subsumes unlink. Daemon boundary (findings 3 and 4). serve-remote canonicalizes ZERO_DAEMON_REMOTE_TOKEN_FILE to the absolute, symlink-resolved path it actually reads before any worker inherits it, so a relative value no longer resolves against each session's directory (protecting a path that holds no token) and no link pathname reaches the derived deny rules — bubblewrap cannot mount over a symlink destination. It stays a no-op when ZERO_DAEMON_REMOTE_TOKEN supplies the token, so an unused or dangling file pointer neither changes the outcome nor fails the start. The in-process list protects both the configured pathname and its resolved target, so a stale symlinked pointer inherited from elsewhere leaves the link unreplaceable through Zero's tools. --- internal/cli/daemon.go | 6 + internal/daemon/remote/auth.go | 42 ++++ internal/daemon/remote/auth_test.go | 81 +++++++ internal/sandbox/engine.go | 7 +- internal/sandbox/pathlists.go | 102 ++++++++- .../sandbox/protected_credentials_test.go | 210 ++++++++++++++++++ internal/sandbox/runner.go | 8 +- internal/sandbox/runner_test.go | 8 +- internal/tools/daemon_token_exclusion_test.go | 129 +++++++++++ 9 files changed, 581 insertions(+), 12 deletions(-) create mode 100644 internal/sandbox/protected_credentials_test.go create mode 100644 internal/tools/daemon_token_exclusion_test.go diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index 5074312e4..dafede623 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -477,6 +477,12 @@ func runDaemonServeRemote(args []string, stdout io.Writer, stderr io.Writer) int if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Pin the token file to the path this process reads BEFORE any worker + // inherits the variable, so a relative or symlinked value cannot make a + // session's sandbox profile protect a different path than the live bearer file. + if err := remote.CanonicalizeTokenFileEnv(); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } token, err := remote.TokenFromEnv() if err != nil { return writeAppError(stderr, err.Error(), exitCrash) diff --git a/internal/daemon/remote/auth.go b/internal/daemon/remote/auth.go index 4f1574a9d..abc44de52 100644 --- a/internal/daemon/remote/auth.go +++ b/internal/daemon/remote/auth.go @@ -23,6 +23,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/Gitlawb/zero/internal/daemon" @@ -86,6 +87,47 @@ func TokenFromEnv() (string, error) { return "", fmt.Errorf("remote: set %s or %s", EnvToken, EnvTokenFile) } +// CanonicalizeTokenFileEnv rewrites EnvTokenFile in this process's environment +// to the absolute, symlink-resolved path TokenFromEnv actually reads, so every +// child process — and the sandbox profile derived for it — refers to the same +// file this bridge authenticated against. `zero daemon serve-remote` calls it +// before it starts serving. +// +// Two mismatches motivate it. TokenFromEnv passes the value to os.ReadFile, so a +// relative value resolves against the STARTING process's working directory, +// while a worker inherits the same string and resolves it against its own +// session directory — the profile would then protect a path that holds no token +// while the real bearer file stays readable. Resolving symlinks up front also +// keeps a link pathname out of the derived deny rules, which matters because +// bubblewrap cannot mount over a symlink destination. +// +// It is a deliberate no-op when EnvToken supplies the token: TokenFromEnv +// prefers the inline value, so an unused (even dangling) file pointer must not +// change the outcome or fail the start. +func CanonicalizeTokenFileEnv() error { + if strings.TrimSpace(os.Getenv(EnvToken)) != "" { + return nil + } + configured := strings.TrimSpace(os.Getenv(EnvTokenFile)) + if configured == "" { + return nil + } + absolute, err := filepath.Abs(configured) + if err != nil { + return fmt.Errorf("remote: resolve token file %q: %w", configured, err) + } + resolved, err := filepath.EvalSymlinks(absolute) + if err != nil { + // TokenFromEnv would fail on the same path a moment later; reporting it here + // names the resolution step that failed. + return fmt.Errorf("remote: resolve token file %q: %w", configured, err) + } + if resolved == configured { + return nil + } + return os.Setenv(EnvTokenFile, resolved) +} + // Attestation is an optional post-token hook (e.g. workload attestation). The // default is a no-op; a deployment can supply a stricter implementation. type Attestation interface { diff --git a/internal/daemon/remote/auth_test.go b/internal/daemon/remote/auth_test.go index 448a6d920..4fef66b82 100644 --- a/internal/daemon/remote/auth_test.go +++ b/internal/daemon/remote/auth_test.go @@ -57,6 +57,87 @@ func TestTokenFromEnv(t *testing.T) { } } +// TestCanonicalizeTokenFileEnv pins the value every child process (and the +// sandbox profile derived for it) inherits to the file this process reads. +func TestCanonicalizeTokenFileEnv(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "tok") + if err := os.WriteFile(token, []byte("from-file\n"), 0o600); err != nil { + t.Fatalf("write token file: %v", err) + } + + t.Run("relative value becomes absolute", func(t *testing.T) { + // A worker resolves the inherited value against its own session directory, + // so a relative value must not survive the daemon boundary. + t.Chdir(base) + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "tok") + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) + } + if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { + t.Fatalf("TokenFromEnv after canonicalization = %q, %v", tok, err) + } + }) + + t.Run("symlinked pathname is resolved", func(t *testing.T) { + link := filepath.Join(base, "tok-link") + if err := os.Symlink(token, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, link) + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want the resolved target %q", EnvTokenFile, got, token) + } + }) + + t.Run("an inline token keeps precedence over a dangling pointer", func(t *testing.T) { + // TokenFromEnv prefers EnvToken, so an unused (even dangling) file pointer + // must neither fail the start nor be rewritten. + dangling := filepath.Join(base, "missing", "tok") + t.Setenv(EnvToken, "from-env") + t.Setenv(EnvTokenFile, dangling) + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv with an inline token: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != dangling { + t.Fatalf("%s = %q, want it left alone", EnvTokenFile, got) + } + if tok, err := TokenFromEnv(); err != nil || tok != "from-env" { + t.Fatalf("TokenFromEnv = %q, %v, want the inline token", tok, err) + } + }) + + t.Run("a selected but unreadable pointer fails closed", func(t *testing.T) { + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, filepath.Join(base, "missing", "tok")) + if err := CanonicalizeTokenFileEnv(); err == nil { + t.Fatal("a selected token file that cannot be resolved must error") + } + }) + + t.Run("no pointer is a no-op", func(t *testing.T) { + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "") + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv without a pointer: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != "" { + t.Fatalf("%s = %q, want empty", EnvTokenFile, got) + } + }) +} + func TestServerTLSConfigRequiresCertKey(t *testing.T) { if _, err := ServerTLSConfig("", ""); err == nil { t.Fatal("ServerTLSConfig must require a cert and key (TLS mandatory)") diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index db66d54a9..92ef514fa 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -154,9 +154,10 @@ func (engine *Engine) ReadExclusions() *ReadExclusions { return nil } return &ReadExclusions{ - workspaceRoot: engine.workspaceRoot, - denyRoots: resolvePolicyPaths(policy.DenyRead), - allowRoots: resolvePolicyPaths(policy.AllowRead), + workspaceRoot: engine.workspaceRoot, + denyRoots: resolvePolicyPaths(policy.DenyRead), + allowRoots: resolvePolicyPaths(policy.AllowRead), + protectedRoots: protectedCredentialPaths(), } } diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index f6815a2c7..5700aa793 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -41,6 +41,62 @@ func resolvePolicyPath(entry string) (string, bool) { return resolved, true } +// daemonRemoteTokenFileEnv names the file holding the remote bridge's bearer +// token. It is duplicated from internal/daemon/remote (which cannot be imported +// here) exactly like the copy scrubSensitiveEnv keeps. +const daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" + +// protectedCredentialPaths returns credential files that Zero's own in-process +// file tools must never read or modify, independent of Policy. +// +// This is deliberately separate from credentialDenyReadPaths, which only shapes +// the OS sandbox profile for wrapped shell commands: read_file resolves scoped +// paths itself, and grep/glob build their exclusions from the policy, so a +// profile-only rule leaves the in-process tool boundary open. It is also +// separate from Policy.DenyRead, whose emptiness gates escalated (unsandboxed) +// execution and must keep reflecting user configuration alone. +// +// Entries here are NOT re-includable through AllowRead/AllowWrite or a +// session/turn permission profile: the bridge bearer token grants control of +// this daemon, so a remote-controlled agent must not be able to read it, nor +// replace it to hijack the next bridge start, even when the file sits inside +// its own session workspace. Unlike the profile list this applies on Windows +// too, where filesystem deny-read has no sandbox representation (#662). +// +// Both the configured pathname and the target it currently resolves to are +// protected: `zero daemon serve-remote` canonicalizes the value it selects (see +// remote.CanonicalizeTokenFileEnv), but a stale symlinked value inherited from +// elsewhere must not leave the link replaceable. +func protectedCredentialPaths() []string { + // os.ReadFile — the daemon's own reader — treats the value literally, so a + // relative path resolves against the working directory and a leading "~" is + // NOT expanded. resolvePolicyPath would expand it and protect the wrong file. + configured := strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) + if configured == "" { + return nil + } + absolute, err := filepath.Abs(configured) + if err != nil { + return nil + } + paths := []string{absolute} + if resolved, err := filepath.EvalSymlinks(absolute); err == nil && resolved != absolute { + paths = append(paths, resolved) + } + return dedupeStrings(paths) +} + +// protectedPathDenied reports whether path targets one of the protected +// credential files. There is no allow-list consultation by design. +func protectedPathDenied(protected []string, workspaceRoot, path string) bool { + for _, entry := range protected { + if pathUnderPolicyRoot(path, entry, workspaceRoot) { + return true + } + } + return false +} + // resolvePolicyPaths resolves and de-duplicates a list of policy path entries, // dropping blanks and non-existent entries. Files and directories are both kept // (a DenyRead/DenyWrite entry may target a single sensitive file). @@ -151,20 +207,28 @@ type ReadExclusions struct { workspaceRoot string denyRoots []string allowRoots []string + // protectedRoots are the automatic credential exclusions + // (protectedCredentialPaths); AllowRead never re-includes them. + protectedRoots []string } -// Active reports whether any DenyRead root is configured. When false the -// exclusions are a no-op and the search behaves exactly as before. +// Active reports whether anything is excluded: a configured DenyRead root or an +// automatic protected credential path. When false the exclusions are a no-op and +// the search behaves exactly as before. func (rx *ReadExclusions) Active() bool { - return rx != nil && len(rx.denyRoots) > 0 + return rx != nil && (len(rx.denyRoots) > 0 || len(rx.protectedRoots) > 0) } // PathExcluded reports whether reading path is excluded by DenyRead, honoring a -// more-specific AllowRead re-inclusion. It is the per-file predicate for a walk. +// more-specific AllowRead re-inclusion, or by an automatic credential exclusion, +// which no allow entry re-includes. It is the per-file predicate for a walk. func (rx *ReadExclusions) PathExcluded(path string) bool { if !rx.Active() { return false } + if protectedPathDenied(rx.protectedRoots, rx.workspaceRoot, path) { + return true + } return readDeniedResolved(rx.workspaceRoot, rx.denyRoots, rx.allowRoots, path) } @@ -176,6 +240,11 @@ func (rx *ReadExclusions) DirExcluded(path string) bool { if !rx.Active() { return false } + // A protected credential entry is a file, so it only prunes a directory when + // the directory IS that entry; PathExcluded still filters it during the walk. + if protectedPathDenied(rx.protectedRoots, rx.workspaceRoot, path) { + return true + } if !readDeniedResolved(rx.workspaceRoot, rx.denyRoots, rx.allowRoots, path) { return false } @@ -212,6 +281,16 @@ func allowWriteScope(policy Policy) *Scope { // enforceWorkspace; the workspace boundary itself applies only when // enforceWorkspace. It never bypasses the symlink/out-of-workspace guards. func validateWritePath(scope *Scope, policy Policy, enforceWorkspace bool, workspaceRoot, path string) *pathBlock { + // The protected credential files outrank every allow: overwriting or + // truncating the bridge token denies service, and replacing it hands the next + // bridge start an attacker-chosen secret. + if protectedPathDenied(protectedCredentialPaths(), workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never writable", + } + } // DenyWrite wins regardless of workspace enforcement. for _, deny := range resolvePolicyPaths(policy.DenyWrite) { if pathUnderPolicyRoot(path, deny, workspaceRoot) { @@ -252,7 +331,9 @@ func validatePathWithPolicy(scope *Scope, policy Policy, sideEffect SideEffect, // when there is anything to enforce; otherwise it is a no-op (unchanged from the // pre-path-list behavior, where an empty workspace root skipped validation). if workspaceRoot == "" && !filepath.IsAbs(path) { - if enforceWorkspace || policyHasPathLists(policy) { + // A configured bridge token counts as something to enforce: the relative + // path cannot be anchored, so it cannot be proven to miss the token file. + if enforceWorkspace || policyHasPathLists(policy) || len(protectedCredentialPaths()) > 0 { return &pathBlock{ Code: BlockOutsideWorkspace, Path: path, @@ -263,6 +344,13 @@ func validatePathWithPolicy(scope *Scope, policy Policy, sideEffect SideEffect, } switch sideEffect { case SideEffectRead: + if protectedPathDenied(protectedCredentialPaths(), workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never readable", + } + } if readDenied(policy, workspaceRoot, path) { return &pathBlock{ Code: BlockDenied, @@ -325,7 +413,9 @@ func workspaceRelGlob(workspaceRoot, target string) (string, bool) { // consumer. Empty when DenyRead is unset (the default), so search behavior is // unchanged. func ReadExclusionGlobs(policy Policy, scope *Scope) []string { - denyRoots := resolvePolicyPaths(policy.DenyRead) + // The automatic credential exclusions ride along so an rg-based consumer never + // walks the bridge token when it happens to live inside the workspace. + denyRoots := dedupeStrings(append(resolvePolicyPaths(policy.DenyRead), protectedCredentialPaths()...)) if len(denyRoots) == 0 || scope == nil { return nil } diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go new file mode 100644 index 000000000..34720d1a2 --- /dev/null +++ b/internal/sandbox/protected_credentials_test.go @@ -0,0 +1,210 @@ +package sandbox + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// protectedTokenFixture writes a bridge token inside the workspace and points +// ZERO_DAEMON_REMOTE_TOKEN_FILE at it. +func protectedTokenFixture(t *testing.T) (string, string) { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(daemonRemoteTokenFileEnv, token) + return ws, token +} + +func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "token") + if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + + t.Run("absent variable protects nothing", func(t *testing.T) { + t.Setenv(daemonRemoteTokenFileEnv, "") + if got := protectedCredentialPaths(); len(got) != 0 { + t.Fatalf("protected paths = %#v, want none", got) + } + }) + + t.Run("relative value resolves against the working directory", func(t *testing.T) { + // os.ReadFile — what the daemon uses — resolves a relative value against the + // working directory, so the protected path must do the same. + t.Chdir(base) + t.Setenv(daemonRemoteTokenFileEnv, "token") + if got := protectedCredentialPaths(); !stringSliceContains(got, token) { + t.Fatalf("protected paths = %#v, want %q", got, token) + } + }) + + t.Run("a literal tilde is not home-expanded", func(t *testing.T) { + // os.ReadFile treats "~" as an ordinary directory name; expanding it here + // would protect a path the daemon never reads. + t.Chdir(base) + t.Setenv(daemonRemoteTokenFileEnv, filepath.Join("~", "token")) + want := filepath.Join(base, "~", "token") + got := protectedCredentialPaths() + if !stringSliceContains(got, want) { + t.Fatalf("protected paths = %#v, want literal %q", got, want) + } + home, err := os.UserHomeDir() + if err == nil && stringSliceContains(got, filepath.Join(home, "token")) { + t.Fatalf("protected paths = %#v, must not home-expand the value", got) + } + }) + + t.Run("a symlinked pathname protects the link and its target", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevation on Windows") + } + link := filepath.Join(base, "token-link") + if err := os.Symlink(token, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + t.Setenv(daemonRemoteTokenFileEnv, link) + got := protectedCredentialPaths() + for _, want := range []string{link, token} { + if !stringSliceContains(got, want) { + t.Fatalf("protected paths = %#v, want %q", got, want) + } + } + }) +} + +// TestProtectedCredentialsSurviveAllowRead locks in the non-opt-out guarantee: +// the bridge token grants control of the daemon, so neither AllowRead, an +// AllowWrite root, nor a granted permission may re-include it. +func TestProtectedCredentialsSurviveAllowRead(t *testing.T) { + ws, token := protectedTokenFixture(t) + policy := Policy{ + Mode: ModeEnforce, + EnforceWorkspace: true, + AllowRead: []string{ws, token}, + AllowWrite: []string{ws}, + } + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace} { + block := validatePathWithPolicy(scope, policy, sideEffect, true, ws, token) + if block == nil || !strings.Contains(block.Reason, "remote bridge token") { + t.Fatalf("%s on the bridge token: block = %#v, want a bridge-token deny", sideEffect, block) + } + } + + // The search-walk matcher enforces the same exclusion without consulting + // AllowRead, and it is active even though DenyRead is empty. + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: policy, Scope: scope}) + rx := engine.ReadExclusions() + if !rx.Active() { + t.Fatal("read exclusions must be active for the automatic credential deny") + } + if !rx.PathExcluded(token) { + t.Fatalf("read exclusions must exclude the bridge token %q", token) + } + if rx.PathExcluded(filepath.Join(ws, "main.go")) { + t.Fatal("read exclusions must not exclude ordinary workspace files") + } + if globs := ReadExclusionGlobs(policy, scope); !stringSliceContains(globs, "!bridge-token") { + t.Fatalf("read exclusion globs = %#v, want the bridge token excluded", globs) + } +} + +// TestProtectedCredentialsRejectSessionPermissionProfile covers the other +// re-inclusion route: a session/turn permission profile that asks for the token +// path must not be auto-applicable. +func TestProtectedCredentialsRejectSessionPermissionProfile(t *testing.T) { + ws, token := protectedTokenFixture(t) + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true}, + Scope: scope, + }) + if engine.CoversRequestPermissions(RequestPermissionProfile{ + FileSystem: &FileSystemPermissions{Read: []string{token}}, + }) { + t.Fatal("a permission request covering the bridge token must not read as already-granted") + } + if !engine.CoversRequestPermissions(RequestPermissionProfile{ + FileSystem: &FileSystemPermissions{Read: []string{filepath.Join(ws, "main.go")}}, + }) { + t.Fatal("an ordinary workspace read request must stay covered by policy") + } +} + +// TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile covers the macOS +// backend: a token under a writable root was read-denied but still truncatable +// through the broad write allow. +func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { + token := "/private/tmp/zero-bridge-token" + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: "/repo"}}, + DenyRead: []string{token}, + AllowTemp: true, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + escaped := sandboxProfileString(normalizeProfilePath(token)) + denyRead := `(deny file-read* (literal "` + escaped + `"))` + denyWrite := `(deny file-write* (literal "` + escaped + `"))` + for _, want := range []string{denyRead, denyWrite} { + if !strings.Contains(sbpl, want) { + t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) + } + } + // Seatbelt is last-match-wins, so both denials must follow the broad allow. + if allow := strings.Index(sbpl, "(allow file-write*"); allow < 0 || strings.Index(sbpl, denyWrite) < allow { + t.Fatalf("the write denial must follow the broad write allow:\n%s", sbpl) + } +} + +// TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert +// for everyone who does not run the remote bridge. +func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { + t.Setenv(daemonRemoteTokenFileEnv, "") + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true}, + }) + if rx := engine.ReadExclusions(); rx.Active() { + t.Fatal("read exclusions must stay inactive without a configured token file") + } + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action == ActionDeny { + t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) + } +} diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index fa99b3aba..d7a4702ea 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -839,7 +839,13 @@ func denySeatbeltPathRules(action string, paths []string) []string { for _, filter := range filters { out = append(out, "(deny "+action+" "+filter+")") if action == "file-read*" { - out = append(out, "(deny file-write-unlink "+filter+")") + // Denying reads does not imply denying writes, and the broad + // (allow file-write* ...) emitted above covers every workspace root + // plus the default temp roots. A credential file under one of those + // stayed truncatable and replaceable — enough to deny service, or to + // swap the secret a later process reads. file-write* subsumes the + // unlink denial this previously emitted on its own. + out = append(out, "(deny file-write* "+filter+")") } } } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index a29302989..f28c85279 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -484,7 +484,11 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { normalizedSecretRead := sandboxProfileString(normalizeProfilePath("/repo/secret-read")) normalizedSecretWrite := sandboxProfileString(normalizeProfilePath("/repo/secret-write")) denySecretReadRule := `(deny file-read* (subpath "` + normalizedSecretRead + `"))` - denySecretReadUnlinkRule := `(deny file-write-unlink (subpath "` + normalizedSecretRead + `"))` + // A read-denied path must also be write-denied: the broad write allow above + // covers every workspace and temp root, so a credential file under one of them + // would otherwise stay truncatable and replaceable. file-write* subsumes the + // file-write-unlink denial this used to assert on its own. + denySecretReadWriteRule := `(deny file-write* (subpath "` + normalizedSecretRead + `"))` denySecretWriteRule := `(deny file-write* (subpath "` + normalizedSecretWrite + `"))` for _, want := range []string{ `(deny file-write* (literal "/repo/vendor"))`, @@ -492,7 +496,7 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`, `(deny file-write* (regex #"^/repo/\.zero(/.*)?$"))`, denySecretReadRule, - denySecretReadUnlinkRule, + denySecretReadWriteRule, denySecretWriteRule, } { if !strings.Contains(sbpl, want) { diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go new file mode 100644 index 000000000..0ae96cd8d --- /dev/null +++ b/internal/tools/daemon_token_exclusion_test.go @@ -0,0 +1,129 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/daemon/remote" + "github.com/Gitlawb/zero/internal/sandbox" +) + +// daemonTokenFixture builds a workspace holding the remote bridge's token file +// alongside an ordinary file, with ZERO_DAEMON_REMOTE_TOKEN_FILE pointing at it — +// the shape a remote daemon session takes when its token lives in the session +// workspace. AllowRead deliberately covers the whole workspace so the tests prove +// the exclusion is not re-includable. +func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + if err := os.WriteFile(filepath.Join(ws, "main.go"), []byte("package main // bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvTokenFile, token) + + scope, err := sandbox.NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: ws, + Policy: sandbox.Policy{ + Mode: sandbox.ModeEnforce, + EnforceWorkspace: true, + AllowRead: []string{ws}, + }, + Scope: scope, + }) + return ws, token, engine +} + +func TestGrepSkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + tool, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("grep tool must be sandbox-aware") + } + args := map[string]any{"pattern": "bridge-secret", "output_mode": "files_with_matches"} + + sandboxed := tool.RunWithSandbox(context.Background(), args, engine) + if sandboxed.Status != StatusOK { + t.Fatalf("grep failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("grep must still match ordinary workspace files, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "bridge-token") { + t.Fatalf("grep must NOT surface the remote bridge token file, got:\n%s", sandboxed.Output) + } +} + +func TestGlobSkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + tool, ok := NewScopedGlobTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("glob tool must be sandbox-aware") + } + + sandboxed := tool.RunWithSandbox(context.Background(), map[string]any{"pattern": "**/*"}, engine) + if sandboxed.Status != StatusOK { + t.Fatalf("glob failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("glob must still match ordinary workspace files, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "bridge-token") { + t.Fatalf("glob must NOT surface the remote bridge token file, got:\n%s", sandboxed.Output) + } +} + +// TestEngineDeniesDaemonTokenFileTools covers the request gate the direct file +// tools go through (read_file, write_file, edit_file, apply_patch): the bridge +// token must be neither readable nor writable, even though AllowRead covers the +// whole workspace and the file is inside it. +func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { + ws, token, engine := daemonTokenFixture(t) + for _, tc := range []struct { + name string + toolName string + sideEffect sandbox.SideEffect + }{ + {name: "read_file", toolName: "read_file", sideEffect: sandbox.SideEffectRead}, + {name: "write_file", toolName: "write_file", sideEffect: sandbox.SideEffectWrite}, + {name: "edit_file", toolName: "edit_file", sideEffect: sandbox.SideEffectWrite}, + } { + t.Run(tc.name, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), sandbox.Request{ + ToolName: tc.toolName, + WorkspaceRoot: ws, + SideEffect: tc.sideEffect, + Args: map[string]any{"path": token}, + // A granted permission must not override the exclusion either. + Permission: sandbox.PermissionAllow, + }) + if decision.Action != sandbox.ActionDeny || !strings.Contains(decision.Reason, "remote bridge token") { + t.Fatalf("%s on the bridge token: action = %q reason = %q, want a bridge-token deny", tc.toolName, decision.Action, decision.Reason) + } + }) + } + + // An ordinary workspace file in the same directory stays usable. + decision := engine.Evaluate(context.Background(), sandbox.Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: sandbox.SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action == sandbox.ActionDeny { + t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) + } +} From e109db32c2ac87835d99c8521fdcf1028b282c3b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 13:13:25 +0200 Subject: [PATCH 03/17] fix(sandbox): scope the credential write-deny and keep it under a disabled policy Addresses the CodeRabbit review on the previous commit. - The Seatbelt write-deny no longer applies to every user-configured DenyRead entry, only to the automatic credential entries (cloud stores plus the bridge token). A read-denied cache or generated directory that the build legitimately writes stays writable on macOS; user entries keep the read + unlink denial they already had. - ModeDisabled no longer drops the automatic credential exclusion. Disabling the sandbox drops user-configured restrictions, but the bridge token authenticates the caller driving these tools, so Evaluate, ReadExclusions, and the glob export keep protecting it. Everything else stays allowed. - The direct-tool table now includes the apply_patch case its comment promised. --- internal/sandbox/engine.go | 26 +++++++-- internal/sandbox/pathlists.go | 30 ++++++++++ .../sandbox/protected_credentials_test.go | 56 +++++++++++++++++-- internal/sandbox/runner.go | 37 +++++++++--- internal/sandbox/runner_test.go | 15 +++-- internal/tools/daemon_token_exclusion_test.go | 1 + 6 files changed, 141 insertions(+), 24 deletions(-) diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index 92ef514fa..883c27ce0 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -144,14 +144,22 @@ func (engine *Engine) LookupCommandPrefixForSession(toolName string, command []s // don't re-run Abs/EvalSymlinks per visited path. Returns nil for a nil engine // (the matcher's methods treat nil as "exclude nothing"). func (engine *Engine) ReadExclusions() *ReadExclusions { - // A disabled policy enforces nothing, so it must not filter search results - // either (Evaluate already allows every request under ModeDisabled). + // A disabled policy enforces nothing the USER configured, so it must not + // filter search results by DenyRead either (Evaluate likewise allows every + // request under ModeDisabled). The automatic credential exclusion is not + // policy-derived and survives: the bridge token authenticates the channel + // driving these tools, so turning the sandbox off must not hand a remote + // caller its own credential. if engine == nil { return nil } policy := engine.effectivePolicy(engine.policy) if policy.Mode == ModeDisabled { - return nil + protected := protectedCredentialPaths() + if len(protected) == 0 { + return nil + } + return &ReadExclusions{workspaceRoot: engine.workspaceRoot, protectedRoots: protected} } return &ReadExclusions{ workspaceRoot: engine.workspaceRoot, @@ -165,13 +173,14 @@ func (engine *Engine) ReadExclusions() *ReadExclusions { // engine's policy + scope (see the package-level ReadExclusionGlobs). Empty when // DenyRead is unset or the engine has no scope. func (engine *Engine) ReadExclusionGlobs() []string { - // A disabled policy filters nothing (parity with ReadExclusions / Evaluate). + // A disabled policy filters nothing the user configured, but keeps the + // automatic credential exclusion (parity with ReadExclusions / Evaluate). if engine == nil { return nil } policy := engine.effectivePolicy(engine.policy) if policy.Mode == ModeDisabled { - return nil + return ReadExclusionGlobs(Policy{}, engine.scope) } return ReadExclusionGlobs(policy, engine.scope) } @@ -319,6 +328,13 @@ func (engine *Engine) Evaluate(ctx context.Context, request Request) Decision { risk := classifyWithScope(request, scope) if policy.Mode == ModeDisabled { + // Disabling the sandbox drops every user-configured restriction, but not the + // automatic credential exclusion: the remote bridge token authenticates the + // caller driving these tools, so it stays unreadable and unwritable through + // them (a shell command is a separate, OS-level boundary). + if block := protectedCredentialPathBlock(request, request.WorkspaceRoot); block != nil { + return deny(request, risk, block.Code, block.Path, block.Reason, false) + } return Decision{Action: ActionAllow, Risk: risk, Reason: "sandbox disabled"} } if request.Permission == PermissionDeny { diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 5700aa793..558219ab2 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -86,6 +86,36 @@ func protectedCredentialPaths() []string { return dedupeStrings(paths) } +// protectedCredentialPathBlock returns the block for the first requested path +// that targets a protected credential file, or nil. It exists for the callers +// that bypass validatePathWithPolicy — currently the ModeDisabled short-circuit, +// where the exclusion still applies because it is not policy-derived. +func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBlock { + switch request.SideEffect { + case SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace: + default: + return nil + } + protected := protectedCredentialPaths() + if len(protected) == 0 { + return nil + } + verb := "readable" + if request.SideEffect != SideEffectRead { + verb = "writable" + } + for _, path := range requestPaths(request) { + if protectedPathDenied(protected, workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never " + verb, + } + } + } + return nil +} + // protectedPathDenied reports whether path targets one of the protected // credential files. There is no allow-list consultation by design. func protectedPathDenied(protected []string, workspaceRoot, path string) bool { diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index 34720d1a2..bc5c82123 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -155,20 +155,22 @@ func TestProtectedCredentialsRejectSessionPermissionProfile(t *testing.T) { // TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile covers the macOS // backend: a token under a writable root was read-denied but still truncatable -// through the broad write allow. +// through the broad write allow. A user-configured DenyRead entry keeps the write +// direction (see TestSeatbeltProfileProtectsMetadataAndDenyOrdering). func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { - token := "/private/tmp/zero-bridge-token" + ws, token := protectedTokenFixture(t) + userDenied := filepath.Join(ws, "generated") profile := PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, ReadRoots: []string{string(filepath.Separator)}, - WriteRoots: []WritableRoot{{Root: "/repo"}}, - DenyRead: []string{token}, + WriteRoots: []WritableRoot{{Root: ws}}, + DenyRead: []string{token, userDenied}, AllowTemp: true, }, Network: NetworkPolicy{Mode: NetworkDeny}, } - sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce, DenyRead: []string{userDenied}}, "") escaped := sandboxProfileString(normalizeProfilePath(token)) denyRead := `(deny file-read* (literal "` + escaped + `"))` denyWrite := `(deny file-write* (literal "` + escaped + `"))` @@ -177,12 +179,54 @@ func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) } } - // Seatbelt is last-match-wins, so both denials must follow the broad allow. + if strings.Contains(sbpl, `(deny file-write* (literal "`+sandboxProfileString(normalizeProfilePath(userDenied))+`"))`) { + t.Fatalf("a user-configured DenyRead path must stay writable:\n%s", sbpl) + } + // Seatbelt is last-match-wins, so the denial must follow the broad allow. if allow := strings.Index(sbpl, "(allow file-write*"); allow < 0 || strings.Index(sbpl, denyWrite) < allow { t.Fatalf("the write denial must follow the broad write allow:\n%s", sbpl) } } +// TestProtectedCredentialsSurviveDisabledPolicy covers the one route that skips +// validatePathWithPolicy entirely: ModeDisabled drops every user-configured +// restriction, but the bridge token authenticates the caller driving these tools. +func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: Policy{Mode: ModeDisabled}}) + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite} { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: sideEffect, + Args: map[string]any{"path": token}, + }) + if decision.Action != ActionDeny || !strings.Contains(decision.Reason, "remote bridge token") { + t.Fatalf("%s under a disabled policy: action = %q reason = %q, want a bridge-token deny", sideEffect, decision.Action, decision.Reason) + } + } + + // Everything else stays allowed: a disabled sandbox is still disabled. + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action != ActionAllow { + t.Fatalf("ordinary read under a disabled policy: action = %q reason = %q, want allow", decision.Action, decision.Reason) + } + + rx := engine.ReadExclusions() + if !rx.Active() || !rx.PathExcluded(token) { + t.Fatalf("read exclusions under a disabled policy must still exclude %q", token) + } + if rx.PathExcluded(filepath.Join(ws, "main.go")) { + t.Fatal("read exclusions under a disabled policy must not exclude ordinary files") + } +} + // TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert // for everyone who does not run the remote bridge. func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index d7a4702ea..339fd3a18 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -11,6 +11,7 @@ import ( "os/exec" "path/filepath" "runtime" + "slices" "strings" "sync/atomic" @@ -661,6 +662,7 @@ func seatbeltProfileFromPermissionProfile(profile PermissionProfile, policy Poli writeRule, } rules = append(rules, denyReadRules(profile.FileSystem)...) + rules = append(rules, credentialDenyWriteRules(profile.FileSystem, policy)...) rules = append(rules, writeRootCarveoutDenyRules(profile.FileSystem)...) rules = append(rules, denyWriteRulesFromPaths(profile.FileSystem.DenyWrite)...) rules = append(rules, networkRule) @@ -823,6 +825,33 @@ func denyWriteRulesFromPaths(paths []string) []string { return denySeatbeltPathRules("file-write*", paths) } +// credentialDenyWriteRules write-denies the AUTOMATIC credential entries of +// DenyRead — the cloud credential stores and the remote bridge token file. +// Denying reads does not imply denying writes, and the broad +// (allow file-write* ...) above covers every workspace root plus the default +// temp roots, so a credential file under one of those stayed truncatable and +// replaceable: enough to deny service, or to swap the secret the next process +// reads. denySeatbeltPathRules keeps emitting read + unlink denials for the +// whole DenyRead list, so a user-configured read-denied path that their build +// legitimately writes (a cache or generated directory) stays writable — only +// Zero's own credential entries lose the write direction. +func credentialDenyWriteRules(fs FileSystemPolicy, policy Policy) []string { + automatic := normalizeProfilePaths(append(credentialDenyReadPaths(policy), protectedCredentialPaths()...)) + if len(automatic) == 0 { + return nil + } + denied := normalizeProfilePaths(fs.DenyRead) + paths := make([]string, 0, len(automatic)) + for _, path := range automatic { + // Only paths the profile actually read-denies: credentialDenyReadPaths + // already drops AllowRead opt-outs, and this keeps the two lists in step. + if slices.Contains(denied, path) { + paths = append(paths, path) + } + } + return denyWriteRulesFromPaths(dedupeStrings(paths)) +} + func denySeatbeltPathRules(action string, paths []string) []string { resolved := normalizeProfilePaths(paths) if len(resolved) == 0 { @@ -839,13 +868,7 @@ func denySeatbeltPathRules(action string, paths []string) []string { for _, filter := range filters { out = append(out, "(deny "+action+" "+filter+")") if action == "file-read*" { - // Denying reads does not imply denying writes, and the broad - // (allow file-write* ...) emitted above covers every workspace root - // plus the default temp roots. A credential file under one of those - // stayed truncatable and replaceable — enough to deny service, or to - // swap the secret a later process reads. file-write* subsumes the - // unlink denial this previously emitted on its own. - out = append(out, "(deny file-write* "+filter+")") + out = append(out, "(deny file-write-unlink "+filter+")") } } } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index f28c85279..eb55e56e8 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -484,11 +484,7 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { normalizedSecretRead := sandboxProfileString(normalizeProfilePath("/repo/secret-read")) normalizedSecretWrite := sandboxProfileString(normalizeProfilePath("/repo/secret-write")) denySecretReadRule := `(deny file-read* (subpath "` + normalizedSecretRead + `"))` - // A read-denied path must also be write-denied: the broad write allow above - // covers every workspace and temp root, so a credential file under one of them - // would otherwise stay truncatable and replaceable. file-write* subsumes the - // file-write-unlink denial this used to assert on its own. - denySecretReadWriteRule := `(deny file-write* (subpath "` + normalizedSecretRead + `"))` + denySecretReadUnlinkRule := `(deny file-write-unlink (subpath "` + normalizedSecretRead + `"))` denySecretWriteRule := `(deny file-write* (subpath "` + normalizedSecretWrite + `"))` for _, want := range []string{ `(deny file-write* (literal "/repo/vendor"))`, @@ -496,13 +492,20 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`, `(deny file-write* (regex #"^/repo/\.zero(/.*)?$"))`, denySecretReadRule, - denySecretReadWriteRule, + denySecretReadUnlinkRule, denySecretWriteRule, } { if !strings.Contains(sbpl, want) { t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) } } + // A user-configured read-denied path keeps the write direction: a cache or + // generated directory the build legitimately writes must not become read-only + // just because it is excluded from reads. Only Zero's own automatic credential + // entries are write-denied (see TestProtectedCredentialsDenyReadAndWrite...). + if strings.Contains(sbpl, `(deny file-write* (subpath "`+normalizedSecretRead+`"))`) { + t.Fatalf("a user-configured DenyRead path must stay writable:\n%s", sbpl) + } allowIdx := strings.Index(sbpl, "(allow file-write*") denyReadIdx := strings.Index(sbpl, denySecretReadRule) metadataIdx := strings.Index(sbpl, `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`) diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 0ae96cd8d..e98a1676f 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -100,6 +100,7 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { {name: "read_file", toolName: "read_file", sideEffect: sandbox.SideEffectRead}, {name: "write_file", toolName: "write_file", sideEffect: sandbox.SideEffectWrite}, {name: "edit_file", toolName: "edit_file", sideEffect: sandbox.SideEffectWrite}, + {name: "apply_patch", toolName: "apply_patch", sideEffect: sandbox.SideEffectWrite}, } { t.Run(tc.name, func(t *testing.T) { decision := engine.Evaluate(context.Background(), sandbox.Request{ From b7823318700fb7f814c57a7e328e9ed816bf7f83 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 17:18:07 +0200 Subject: [PATCH 04/17] fix(sandbox): match the bridge token across case and allow-read The protected-credential comparison ends in pathWithinRoot -> filepath.Rel, which folds case on Windows but not on darwin, whose default APFS volume is case-insensitive. A request for .../BRIDGE-TOKEN therefore missed a protected .../bridge-token while the OS opened the same bearer-token file, letting a remote session read or replace the token through the direct file tools. Fold case in the protected-path check on both case-insensitive platforms. The OS-sandbox profile also dropped the token candidate when a user AllowRead entry covered it, so a wrapped shell command could read a token the in-process boundary treats as non-overrideable. Keep the AllowRead opt-out for the ordinary credential stores and make the bridge token the documented exception, so both layers give the same guarantee. --- internal/sandbox/manager_test.go | 8 ++- internal/sandbox/pathlists.go | 59 +++++++++++++++++-- internal/sandbox/profile.go | 24 ++++++-- .../sandbox/protected_credentials_test.go | 37 ++++++++++++ 4 files changed, 117 insertions(+), 11 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 90ed371b8..b04fa13fc 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -410,8 +410,12 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } - if stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { - t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop daemon token file", optedOut) + // The bridge bearer token is the exception: the in-process tool boundary + // (protectedCredentialPaths) refuses to re-include it through AllowRead, so + // the OS-sandbox profile must not either — otherwise the guarantee would + // depend on whether a wrapped shell command or a built-in tool reads it. + if !stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths = %#v, want the daemon token file denied despite AllowRead", optedOut) } if got := credentialDenyReadPathsIn(" ", "", "", nil); len(got) != 0 { diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 558219ab2..2dbef2af6 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -3,6 +3,7 @@ package sandbox import ( "os" "path/filepath" + "runtime" "strings" ) @@ -120,13 +121,50 @@ func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBl // credential files. There is no allow-list consultation by design. func protectedPathDenied(protected []string, workspaceRoot, path string) bool { for _, entry := range protected { - if pathUnderPolicyRoot(path, entry, workspaceRoot) { + if pathUnderProtectedRoot(path, entry, workspaceRoot) { return true } } return false } +// protectedPathFoldsCase reports whether a case-variant spelling of a path opens +// the SAME file on this platform, so the protected-credential comparison must +// fold case to stay closed. +// +// pathWithinRoot ends in filepath.Rel, which ALREADY folds case on Windows +// (path_windows.go's sameWord uses strings.EqualFold) but never on Unix. macOS +// volumes are case-insensitive by default (APFS), so without folding a request +// for `.../Bridge-Token` misses a protected `.../bridge-token` while the OS +// opens the very same bearer-token file. Windows is listed too so the guarantee +// does not silently depend on a filepath.Rel implementation detail. +// +// Case-sensitive outliers (a case-sensitive APFS volume) only make this +// over-deny a genuinely different file that happens to differ by case alone, +// which is the safe direction for a credential the sandbox must never expose. +func protectedPathFoldsCase() bool { + return runtime.GOOS == "windows" || runtime.GOOS == "darwin" +} + +// pathUnderProtectedRoot is pathUnderPolicyRoot for the automatic credential +// exclusions: identical anchoring and symlink normalization, plus the platform's +// filesystem case semantics. Only the final containment comparison folds — the +// normalization above it keeps operating on the path as spelled, so symlink +// resolution is unaffected. +func pathUnderProtectedRoot(requestedPath, root, workspaceRoot string) bool { + normalized, ok := normalizePathForPolicyRoot(requestedPath, root, workspaceRoot) + if !ok { + return false + } + if pathWithinRoot(root, normalized) { + return true + } + if !protectedPathFoldsCase() { + return false + } + return pathWithinRoot(strings.ToLower(root), strings.ToLower(normalized)) +} + // resolvePolicyPaths resolves and de-duplicates a list of policy path entries, // dropping blanks and non-existent entries. Files and directories are both kept // (a DenyRead/DenyWrite entry may target a single sensitive file). @@ -178,18 +216,29 @@ func resolveWriteRootPaths(entries []string) []string { // symlink prefix cannot evade the match. root must be an already-resolved // absolute path. func pathUnderPolicyRoot(requestedPath, root, workspaceRoot string) bool { - if root == "" { + normalized, ok := normalizePathForPolicyRoot(requestedPath, root, workspaceRoot) + if !ok { return false } + return pathWithinRoot(root, normalized) +} + +// normalizePathForPolicyRoot anchors requestedPath (a relative one against +// workspaceRoot) and symlink-normalizes the portion outside root, yielding the +// path pathUnderPolicyRoot compares. ok is false when there is nothing to +// compare against: a blank root, or a relative path with no workspace root. +func normalizePathForPolicyRoot(requestedPath, root, workspaceRoot string) (string, bool) { + if root == "" { + return "", false + } abs := requestedPath if !filepath.IsAbs(abs) { if workspaceRoot == "" { - return false + return "", false } abs = filepath.Join(workspaceRoot, abs) } - normalized := NormalizePrefixForRoot(abs, root) - return pathWithinRoot(root, normalized) + return NormalizePrefixForRoot(abs, root), true } // readDenied reports whether path is excluded by the DenyRead list with no diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 93bfe7ac5..ed214c0d1 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -155,7 +155,11 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit // once the Windows deny-read model is settled. // - A candidate nested under a user-configured AllowRead entry is dropped, -// so `allowRead: ["~/.aws"]` remains an explicit opt-out. +// so `allowRead: ["~/.aws"]` remains an explicit opt-out. The bridge bearer +// token is the one exception: the in-process tool boundary +// (protectedCredentialPaths) treats it as non-overrideable, and the +// guarantee must not depend on whether a wrapped shell command or a built-in +// tool does the reading. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -189,11 +193,15 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, daemonToke if target := strings.TrimSpace(googleCredentials); target != "" { candidates = append(candidates, target) } + // The bridge bearer token grants control of this daemon, so unlike the + // opt-outable credential stores above it stays denied even when AllowRead + // covers it — matching protectedCredentialPaths at the in-process boundary. + var mandatory []string if target := strings.TrimSpace(daemonTokenFile); target != "" { - candidates = append(candidates, target) + mandatory = normalizeProfilePaths([]string{target}) } allowRoots := normalizeProfilePaths(allowRead) - out := make([]string, 0, len(candidates)) + out := make([]string, 0, len(candidates)+len(mandatory)) for _, path := range normalizeProfilePaths(candidates) { // Only stores that actually exist on this host need a deny rule. if _, err := os.Stat(path); err != nil { @@ -210,7 +218,15 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, daemonToke out = append(out, path) } } - return out + for _, path := range mandatory { + // Existence filtering still applies: a deny rule for a path that is not + // there protects nothing, and Bubblewrap cannot bind a missing target. + if _, err := os.Stat(path); err != nil { + continue + } + out = append(out, path) + } + return dedupeStrings(out) } // userGitConfigReadPaths returns the user's global git config FILES so a diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index bc5c82123..f022a3bee 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -227,6 +227,43 @@ func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { } } +// TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems covers the +// bypass a case-variant spelling opened: pathWithinRoot ends in filepath.Rel, +// which folds case on Windows but NOT on darwin, whose default APFS volume is +// case-insensitive — so `.../BRIDGE-TOKEN` missed the protected `.../bridge-token` +// while the OS opened the same bearer-token file. On a case-sensitive filesystem +// the variant is a genuinely different file and must stay unblocked. +func TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems(t *testing.T) { + ws, token := protectedTokenFixture(t) + variant := filepath.Join(filepath.Dir(token), strings.ToUpper(filepath.Base(token))) + if variant == token { + t.Fatalf("fixture token %q has no case variant", token) + } + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + policy := Policy{Mode: ModeEnforce, EnforceWorkspace: true, AllowRead: []string{ws}, AllowWrite: []string{ws}} + wantDenied := protectedPathFoldsCase() + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace} { + block := validatePathWithPolicy(scope, policy, sideEffect, true, ws, variant) + denied := block != nil && strings.Contains(block.Reason, "remote bridge token") + if denied != wantDenied { + t.Fatalf("%s on case variant %q: denied = %t, want %t (block = %#v)", sideEffect, variant, denied, wantDenied, block) + } + } + + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: policy, Scope: scope}) + if excluded := engine.ReadExclusions().PathExcluded(variant); excluded != wantDenied { + t.Fatalf("read exclusions on case variant %q: excluded = %t, want %t", variant, excluded, wantDenied) + } + // The exact spelling is denied on every platform regardless. + if block := validatePathWithPolicy(scope, policy, SideEffectRead, true, ws, token); block == nil { + t.Fatalf("the configured token path %q must always be denied", token) + } +} + // TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert // for everyone who does not run the remote bridge. func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { From f3a5a4a6316939502faeca0c3b89689a478713df Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 20:42:38 +0000 Subject: [PATCH 05/17] fix(sandbox): honor inline daemon token precedence Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-e3c8-705b-9627-3a7564f8d66a Co-authored-by: Pierre Bruno --- internal/sandbox/manager_test.go | 11 ++++++- internal/sandbox/pathlists.go | 29 ++++++++++++++----- internal/sandbox/profile.go | 2 +- .../sandbox/protected_credentials_test.go | 14 +++++++++ internal/tools/daemon_token_exclusion_test.go | 1 + 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index b04fa13fc..a9e0160a3 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -441,11 +441,20 @@ func TestPermissionProfileDeniesDaemonTokenFile(t *testing.T) { if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { t.Fatal(err) } - t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", tokenFile) + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, tokenFile) profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) want := normalizeProfilePaths([]string{tokenFile})[0] if !stringSliceContains(profile.FileSystem.DenyRead, want) { t.Fatalf("DenyRead = %#v, want daemon token file %q", profile.FileSystem.DenyRead, want) } + + // TokenFromEnv selects the inline token when both variables are set, so the + // unused file pointer must not become an automatic OS-sandbox deny. + t.Setenv(daemonRemoteTokenEnv, "from-env") + profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + if stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, must not protect unused token file %q when the inline token takes precedence", profile.FileSystem.DenyRead, want) + } } diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 2dbef2af6..79a82a617 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -42,10 +42,23 @@ func resolvePolicyPath(entry string) (string, bool) { return resolved, true } -// daemonRemoteTokenFileEnv names the file holding the remote bridge's bearer -// token. It is duplicated from internal/daemon/remote (which cannot be imported -// here) exactly like the copy scrubSensitiveEnv keeps. -const daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" +// These name the alternative sources of the remote bridge's bearer token. They +// are duplicated from internal/daemon/remote (which cannot be imported here) +// exactly like the copies scrubSensitiveEnv keeps. +const ( + daemonRemoteTokenEnv = "ZERO_DAEMON_REMOTE_TOKEN" + daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" +) + +// selectedDaemonRemoteTokenFile returns the token-file pointer only when the +// daemon would use it. TokenFromEnv gives the inline token precedence, so an +// inherited file pointer is not a credential when both variables are set. +func selectedDaemonRemoteTokenFile() string { + if strings.TrimSpace(os.Getenv(daemonRemoteTokenEnv)) != "" { + return "" + } + return strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) +} // protectedCredentialPaths returns credential files that Zero's own in-process // file tools must never read or modify, independent of Policy. @@ -64,15 +77,15 @@ const daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" // its own session workspace. Unlike the profile list this applies on Windows // too, where filesystem deny-read has no sandbox representation (#662). // -// Both the configured pathname and the target it currently resolves to are +// Both the selected pathname and the target it currently resolves to are // protected: `zero daemon serve-remote` canonicalizes the value it selects (see -// remote.CanonicalizeTokenFileEnv), but a stale symlinked value inherited from -// elsewhere must not leave the link replaceable. +// remote.CanonicalizeTokenFileEnv), but a symlinked selected value inherited +// from elsewhere must not leave the link replaceable. func protectedCredentialPaths() []string { // os.ReadFile — the daemon's own reader — treats the value literally, so a // relative path resolves against the working directory and a leading "~" is // NOT expanded. resolvePolicyPath would expand it and protect the wrong file. - configured := strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) + configured := selectedDaemonRemoteTokenFile() if configured == "" { return nil } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index ed214c0d1..4927fd07c 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -174,7 +174,7 @@ func credentialDenyReadPaths(policy Policy) []string { return credentialDenyReadPathsIn( home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), - os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"), + selectedDaemonRemoteTokenFile(), policy.AllowRead, ) } diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index f022a3bee..1270525e7 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -21,6 +21,7 @@ func protectedTokenFixture(t *testing.T) (string, string) { if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { t.Fatalf("write token: %v", err) } + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, token) return ws, token } @@ -36,16 +37,26 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { } t.Run("absent variable protects nothing", func(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, "") if got := protectedCredentialPaths(); len(got) != 0 { t.Fatalf("protected paths = %#v, want none", got) } }) + t.Run("inline token leaves the unused file pointer unprotected", func(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "from-env") + t.Setenv(daemonRemoteTokenFileEnv, token) + if got := protectedCredentialPaths(); len(got) != 0 { + t.Fatalf("protected paths = %#v, want none when the inline token takes precedence", got) + } + }) + t.Run("relative value resolves against the working directory", func(t *testing.T) { // os.ReadFile — what the daemon uses — resolves a relative value against the // working directory, so the protected path must do the same. t.Chdir(base) + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, "token") if got := protectedCredentialPaths(); !stringSliceContains(got, token) { t.Fatalf("protected paths = %#v, want %q", got, token) @@ -56,6 +67,7 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { // os.ReadFile treats "~" as an ordinary directory name; expanding it here // would protect a path the daemon never reads. t.Chdir(base) + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, filepath.Join("~", "token")) want := filepath.Join(base, "~", "token") got := protectedCredentialPaths() @@ -76,6 +88,7 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { if err := os.Symlink(token, link); err != nil { t.Skipf("symlink unsupported: %v", err) } + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, link) got := protectedCredentialPaths() for _, want := range []string{link, token} { @@ -267,6 +280,7 @@ func TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems(t *tes // TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert // for everyone who does not run the remote bridge. func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, "") ws, err := filepath.EvalSymlinks(t.TempDir()) if err != nil { diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index e98a1676f..19e71dcbe 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -29,6 +29,7 @@ func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { t.Fatalf("write token: %v", err) } + t.Setenv(remote.EnvToken, "") t.Setenv(remote.EnvTokenFile, token) scope, err := sandbox.NewScope(ws, nil) From d3bc4232452cd1682abfe2379c28be79c91e5ea8 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 27 Jul 2026 07:38:11 +0000 Subject: [PATCH 06/17] fix(sandbox): close daemon token tool bypasses Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-e3c8-705b-9627-3a7564f8d66a Co-authored-by: Pierre Bruno --- internal/sandbox/engine.go | 5 +- internal/tools/bash_auto_allow_test.go | 7 ++ internal/tools/daemon_token_exclusion_test.go | 41 ++++++++++++ internal/tools/exec_command_test.go | 7 ++ internal/tools/list_directory.go | 27 +++++--- internal/tools/read_exclusions_test.go | 65 +++++++++++++++++++ 6 files changed, 142 insertions(+), 10 deletions(-) diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index ec882ae64..50a4a057b 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -193,7 +193,8 @@ func (engine *Engine) effectiveNetworkMode(policy Policy) NetworkMode { } // UnsandboxedExecutionAllowed reports whether an escalated shell attempt may -// bypass the native sandbox without dropping active denied-read restrictions. +// bypass the native sandbox without dropping active denied-read restrictions, +// including the automatic remote bridge token exclusion. func (engine *Engine) UnsandboxedExecutionAllowed() bool { if engine == nil { return true @@ -202,7 +203,7 @@ func (engine *Engine) UnsandboxedExecutionAllowed() bool { if policy.Mode == ModeDisabled { return true } - return len(normalizeProfilePaths(policy.DenyRead)) == 0 + return len(normalizeProfilePaths(policy.DenyRead)) == 0 && len(protectedCredentialPaths()) == 0 } // toolNetworkExempt reports whether a request is exempt from the engine-level diff --git a/internal/tools/bash_auto_allow_test.go b/internal/tools/bash_auto_allow_test.go index ef335229d..3543ca5f2 100644 --- a/internal/tools/bash_auto_allow_test.go +++ b/internal/tools/bash_auto_allow_test.go @@ -130,6 +130,13 @@ func TestBashRequireEscalatedKeepsSandboxWhenDeniedReadsActive(t *testing.T) { } } +func TestBashRequireEscalatedKeepsSandboxWhenDaemonTokenProtected(t *testing.T) { + engine := defaultPolicyDaemonTokenEngine(t) + if commandEngineForSandboxPermissions(engine, SandboxPermissionsRequireEscalated) == nil { + t.Fatal("bash require_escalated must preserve the sandbox when the default remote-daemon policy protects a token file") + } +} + func TestBashStillPromptsWithoutActiveSandbox(t *testing.T) { root := t.TempDir() registry := NewRegistry() diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 19e71dcbe..d72d235a5 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -48,6 +48,28 @@ func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { return ws, token, engine } +// defaultPolicyDaemonTokenEngine is the remote-daemon shape relevant to shell +// escalation: a selected token file with the default policy and no user +// DenyRead entries. +func defaultPolicyDaemonTokenEngine(t *testing.T) *sandbox.Engine { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvToken, "") + t.Setenv(remote.EnvTokenFile, token) + policy := sandbox.DefaultPolicy() + if len(policy.DenyRead) != 0 { + t.Fatalf("default policy DenyRead = %#v, want none", policy.DenyRead) + } + return sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: policy}) +} + func TestGrepSkipsDaemonTokenFile(t *testing.T) { ws, _, engine := daemonTokenFixture(t) tool, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) @@ -87,6 +109,25 @@ func TestGlobSkipsDaemonTokenFile(t *testing.T) { } } +func TestListDirectorySkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + + result := registry.RunWithOptions(context.Background(), "list_directory", map[string]any{ + "path": ".", + }, RunOptions{Sandbox: engine}) + if result.Status != StatusOK { + t.Fatalf("list_directory failed: %s", result.Output) + } + if !strings.Contains(result.Output, "main.go") { + t.Fatalf("list_directory must still show ordinary workspace files, got:\n%s", result.Output) + } + if strings.Contains(result.Output, "bridge-token") { + t.Fatalf("list_directory must NOT surface the remote bridge token file, got:\n%s", result.Output) + } +} + // TestEngineDeniesDaemonTokenFileTools covers the request gate the direct file // tools go through (read_file, write_file, edit_file, apply_patch): the bridge // token must be neither readable nor writable, even though AllowRead covers the diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index 8b4f11c8f..e0ba65510 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -173,6 +173,13 @@ func TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval(t *testin } } +func TestExecCommandRequireEscalatedKeepsSandboxWhenDaemonTokenProtected(t *testing.T) { + engine := defaultPolicyDaemonTokenEngine(t) + if commandEngineForSandboxPermissions(engine, SandboxPermissionsRequireEscalated) == nil { + t.Fatal("exec_command require_escalated must preserve the sandbox when the default remote-daemon policy protects a token file") + } +} + // TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval mirrors // TestBashToolRequireEscalatedMsysGuard for exec_command: the MSYS sandbox // guard exists only because MSYS/Cygwin coreutils fail under the diff --git a/internal/tools/list_directory.go b/internal/tools/list_directory.go index dd764bd6f..008e69e5a 100644 --- a/internal/tools/list_directory.go +++ b/internal/tools/list_directory.go @@ -46,14 +46,14 @@ func NewScopedListDirectoryTool(workspaceRoot string, scope PathScope) Tool { } func (tool listDirectoryTool) Run(_ context.Context, args map[string]any) Result { - return tool.run(args, true) + return tool.run(args, readExcluder{}, true) } -func (tool listDirectoryTool) RunWithOptions(_ context.Context, args map[string]any, _ RunOptions) Result { - return tool.run(args, false) +func (tool listDirectoryTool) RunWithOptions(_ context.Context, args map[string]any, options RunOptions) Result { + return tool.run(args, sandboxReadExcluder(options.Sandbox), false) } -func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result { +func (tool listDirectoryTool) run(args map[string]any, exclude readExcluder, directBudget bool) Result { // Optional with a "." default: treat an explicit empty path (a common // weak-model quirk) the same as the key being absent rather than erroring. requestedPath, err := aliasedStringArg(args, []string{"path", "directory", "dir"}, ".", false, true) @@ -80,7 +80,7 @@ func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result return errorResult("Error listing directory " + requestedPath + ": " + err.Error()) } - entries, err := listDirectoryEntries(absolutePath, 0, maxDepth) + entries, err := listDirectoryEntries(absolutePath, 0, maxDepth, exclude) if err != nil { return errorResult("Error listing directory " + relativePath + ": " + err.Error()) } @@ -95,7 +95,7 @@ func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result return result } -func listDirectoryEntries(path string, depth int, maxDepth int) ([]string, error) { +func listDirectoryEntries(path string, depth int, maxDepth int, exclude readExcluder) ([]string, error) { dirEntries, err := os.ReadDir(path) if err != nil { return nil, err @@ -112,18 +112,29 @@ func listDirectoryEntries(path string, depth int, maxDepth int) ([]string, error if entry.IsDir() && shouldSkipDirectory(entry.Name()) { continue } + entryPath := filepath.Join(path, entry.Name()) + if entry.IsDir() && exclude.dirExcluded(entryPath) { + continue + } indent := strings.Repeat(" ", depth) if entry.IsDir() { - results = append(results, indent+entry.Name()+"/") + // A denied directory with a nested AllowRead cannot be pruned, but the + // denied directory entry itself must still stay out of the listing. + if !exclude.fileExcluded(entryPath) { + results = append(results, indent+entry.Name()+"/") + } if depth < maxDepth { - children, err := listDirectoryEntries(filepath.Join(path, entry.Name()), depth+1, maxDepth) + children, err := listDirectoryEntries(entryPath, depth+1, maxDepth, exclude) if err == nil { results = append(results, children...) } } continue } + if exclude.fileExcluded(entryPath) { + continue + } results = append(results, fmt.Sprintf("%s%s", indent, entry.Name())) } diff --git a/internal/tools/read_exclusions_test.go b/internal/tools/read_exclusions_test.go index 1feb9880b..0ca174a99 100644 --- a/internal/tools/read_exclusions_test.go +++ b/internal/tools/read_exclusions_test.go @@ -97,3 +97,68 @@ func TestGlobSkipsDenyReadSubtree(t *testing.T) { t.Fatalf("non-sandboxed glob should include the secret file, got:\n%s", plain.Output) } } + +func TestListDirectorySkipsDenyReadSubtree(t *testing.T) { + ws, engine := denyReadFixture(t) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + args := map[string]any{"path": ".", "recursive": true, "max_depth": 2} + + sandboxed := registry.RunWithOptions(context.Background(), "list_directory", args, RunOptions{Sandbox: engine}) + if sandboxed.Status != StatusOK { + t.Fatalf("list_directory failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("list_directory must still show the non-denied file, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "secret") || strings.Contains(sandboxed.Output, "creds.go") { + t.Fatalf("list_directory must NOT surface a DenyRead subtree, got:\n%s", sandboxed.Output) + } + + plain := NewScopedListDirectoryTool(ws, nil).Run(context.Background(), args) + if !strings.Contains(plain.Output, "secret/") || !strings.Contains(plain.Output, "creds.go") { + t.Fatalf("non-sandboxed list_directory should include the secret subtree, got:\n%s", plain.Output) + } +} + +func TestListDirectoryDescendsToNestedAllowRead(t *testing.T) { + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + secret := filepath.Join(ws, "secret") + allowed := filepath.Join(secret, "allowed") + if err := os.MkdirAll(allowed, 0o755); err != nil { + t.Fatalf("mkdir allowed: %v", err) + } + if err := os.WriteFile(filepath.Join(secret, "hidden.txt"), []byte("hidden\n"), 0o600); err != nil { + t.Fatalf("write hidden: %v", err) + } + if err := os.WriteFile(filepath.Join(allowed, "visible.txt"), []byte("visible\n"), 0o600); err != nil { + t.Fatalf("write visible: %v", err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: ws, + Policy: sandbox.Policy{ + Mode: sandbox.ModeEnforce, + EnforceWorkspace: true, + DenyRead: []string{secret}, + AllowRead: []string{allowed}, + }, + }) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + + result := registry.RunWithOptions(context.Background(), "list_directory", map[string]any{ + "path": ".", "recursive": true, "max_depth": 3, + }, RunOptions{Sandbox: engine}) + if result.Status != StatusOK { + t.Fatalf("list_directory failed: %s", result.Output) + } + if !strings.Contains(result.Output, "allowed/") || !strings.Contains(result.Output, "visible.txt") { + t.Fatalf("list_directory must descend to the nested AllowRead subtree, got:\n%s", result.Output) + } + if strings.Contains(result.Output, "secret/") || strings.Contains(result.Output, "hidden.txt") { + t.Fatalf("list_directory must hide denied entries outside the nested AllowRead subtree, got:\n%s", result.Output) + } +} From fc777ca8c1326ac6300a72e8db458a95b2511475 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 28 Jul 2026 08:33:04 +0000 Subject: [PATCH 07/17] fix(sandbox): deny daemon token aliases Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno --- internal/sandbox/pathlists.go | 28 ++++++++++ internal/tools/daemon_token_exclusion_test.go | 55 +++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 79a82a617..b49b26dab 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -138,6 +138,34 @@ func protectedPathDenied(protected []string, workspaceRoot, path string) bool { return true } } + + // Keep the lexical check above: in particular, it protects a configured + // pathname even when the file is absent, preventing its replacement. For an + // existing request, also compare the object reached by the filesystem. This + // closes aliases created after the token path was selected: EvalSymlinks + // catches symbolic links, while SameFile catches hard links (and any other + // platform-specific names for the same file). + abs := path + if !filepath.IsAbs(abs) { + if workspaceRoot == "" { + return false + } + abs = filepath.Join(workspaceRoot, abs) + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + return false + } + requestInfo, err := os.Stat(resolved) + if err != nil { + return false + } + for _, entry := range protected { + protectedInfo, err := os.Stat(entry) + if err == nil && os.SameFile(requestInfo, protectedInfo) { + return true + } + } return false } diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index d72d235a5..f2bb8e8f0 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -170,3 +170,58 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) } } + +func TestDaemonTokenAliasesDeniedEndToEnd(t *testing.T) { + for _, aliasKind := range []string{"symlink", "hardlink"} { + t.Run(aliasKind, func(t *testing.T) { + ws, token, engine := daemonTokenFixture(t) + alias := filepath.Join(ws, "token-alias") + var err error + switch aliasKind { + case "symlink": + err = os.Symlink(token, alias) + case "hardlink": + err = os.Link(token, alias) + } + if err != nil { + t.Skipf("%s unsupported: %v", aliasKind, err) + } + + registry := NewRegistry() + registry.Register(NewScopedReadFileTool(ws, nil)) + registry.Register(NewScopedWriteFileTool(ws, nil)) + + read := registry.RunWithOptions(context.Background(), "read_file", map[string]any{"path": alias}, RunOptions{Sandbox: engine}) + if read.Status == StatusOK || strings.Contains(read.Output, "bridge-secret") { + t.Fatalf("read_file followed protected %s: status=%s output=%q", aliasKind, read.Status, read.Output) + } + + write := registry.RunWithOptions(context.Background(), "write_file", map[string]any{"path": alias, "content": "attacker-controlled\n"}, RunOptions{Sandbox: engine}) + if write.Status == StatusOK { + t.Fatalf("write_file followed protected %s: output=%q", aliasKind, write.Output) + } + contents, err := os.ReadFile(token) + if err != nil || string(contents) != "bridge-secret\n" { + t.Fatalf("token changed after denied write through %s: contents=%q err=%v", aliasKind, contents, err) + } + + grep, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("grep tool must be sandbox-aware") + } + result := grep.RunWithSandbox(context.Background(), map[string]any{ + "pattern": "bridge-secret", + "output_mode": "files_with_matches", + }, engine) + if result.Status != StatusOK { + t.Fatalf("grep failed: %s", result.Output) + } + if strings.Contains(result.Output, "token-alias") || strings.Contains(result.Output, "bridge-token") { + t.Fatalf("grep surfaced protected %s target:\n%s", aliasKind, result.Output) + } + if !strings.Contains(result.Output, "main.go") { + t.Fatalf("grep omitted ordinary file while filtering %s:\n%s", aliasKind, result.Output) + } + }) + } +} From 643eb6e32a00e4d943b6320d55325a655ca07515 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 28 Jul 2026 22:34:03 +0200 Subject: [PATCH 08/17] docs(sandbox): state the daemon-token boundary at the OS layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two properties of the token protection were only implicit, and both are the kind that get mistaken for bugs or, worse, for guarantees. Inode aliasing is closed by the in-process tools alone. They see a path before opening it, so a symlink or hard link resolves back to the protected inode. Seatbelt and Bubblewrap rules name paths, so a sandboxed shell on macOS can still hard-link the token to a fresh name and read that — the same pathname model a user-configured DenyRead has always had, and not something either backend's policy language can express otherwise. ModeDisabled is shell-open by design. In that mode no wrapper is built at all, so there is no layer left to confine a command and nothing for an escalation to bypass; re-wrapping shell for this one file would undo the switch the operator flipped without holding. The exclusion is a guarantee about Zero's own file tools, not about the machine. Both are now documented where the decision lives, and the ModeDisabled boundary is pinned by a test so changing it has to be deliberate. Co-Authored-By: Claude Opus 5 (1M context) --- internal/sandbox/engine.go | 19 +++++++- internal/sandbox/pathlists.go | 19 ++++++++ .../sandbox/protected_credentials_test.go | 45 +++++++++++++++++++ internal/sandbox/runner.go | 7 +++ internal/tools/daemon_token_exclusion_test.go | 10 +++++ 5 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index 50a4a057b..f179619e4 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -195,6 +195,12 @@ func (engine *Engine) effectiveNetworkMode(policy Policy) NetworkMode { // UnsandboxedExecutionAllowed reports whether an escalated shell attempt may // bypass the native sandbox without dropping active denied-read restrictions, // including the automatic remote bridge token exclusion. +// +// ModeDisabled short-circuits to true before the token is consulted, and that is +// the intended reading of the switch rather than an oversight: with the sandbox +// off there is no wrapper for an escalation to bypass, so refusing the +// escalation would deny a capability the operator already granted globally +// without protecting anything. See the ModeDisabled branch in Evaluate. func (engine *Engine) UnsandboxedExecutionAllowed() bool { if engine == nil { return true @@ -332,7 +338,18 @@ func (engine *Engine) Evaluate(ctx context.Context, request Request) Decision { // Disabling the sandbox drops every user-configured restriction, but not the // automatic credential exclusion: the remote bridge token authenticates the // caller driving these tools, so it stays unreadable and unwritable through - // them (a shell command is a separate, OS-level boundary). + // them. + // + // Shell is deliberately left open here, and it is worth being blunt about + // why. ModeDisabled means no OS wrapper is built at all — the runner sets + // SandboxPreferenceForbid and PermissionProfileFromPolicy returns an + // unrestricted filesystem — so there is no layer left that could confine a + // command. Re-wrapping shell just for this file would quietly undo the + // switch the operator flipped, and would not hold anyway: a shell that can + // run anything can read anything this process can. The exclusion below is + // therefore what it says — a guarantee about Zero's own file tools, not + // about the machine. Running a remote bridge with the sandbox disabled + // means trusting whoever can drive that bridge with the token. if block := protectedCredentialPathBlock(request, request.WorkspaceRoot); block != nil { return deny(request, risk, block.Code, block.Path, block.Reason, false) } diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index b49b26dab..811254c37 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -104,6 +104,12 @@ func protectedCredentialPaths() []string { // that targets a protected credential file, or nil. It exists for the callers // that bypass validatePathWithPolicy — currently the ModeDisabled short-circuit, // where the exclusion still applies because it is not policy-derived. +// +// Only the side effects that name a path are covered. SideEffectShell is not, +// and cannot be: a shell request carries a command line, not a file path, so +// there is nothing here to compare against the token. Shell is confined by the +// OS wrapper instead — which under ModeDisabled does not exist. See the +// ModeDisabled short-circuit in Engine.Evaluate for what that boundary means. func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBlock { switch request.SideEffect { case SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace: @@ -145,6 +151,19 @@ func protectedPathDenied(protected []string, workspaceRoot, path string) bool { // closes aliases created after the token path was selected: EvalSymlinks // catches symbolic links, while SameFile catches hard links (and any other // platform-specific names for the same file). + // + // This inode-level closure is specific to Zero's in-process tools, which see + // every requested path before opening it. The OS layer is pathname-based and + // stays that way: seatbelt and Bubblewrap rules name paths, so a sandboxed + // shell on macOS can still `ln alias && cat alias` — a hard link is a + // second name for the same inode, and no path-based rule covers a name that + // did not exist when the profile was built. That is the same model a + // user-configured DenyRead has always had, deliberately: an aliasing defense + // at the OS layer would have to resolve every path at open time, which is not + // something either backend's policy language expresses. Shell access to a + // host running a remote bridge is therefore access to the token, and the + // protection here is the in-process boundary plus the pathname deny rules, + // not an inode-tight OS guarantee. abs := path if !filepath.IsAbs(abs) { if workspaceRoot == "" { diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index 1270525e7..cc2f46cfb 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -240,6 +240,51 @@ func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { } } +// TestDisabledPolicyLeavesShellOutsideTheTokenBoundary pins the boundary jatmn +// asked to see stated for #685: under ModeDisabled the bridge-token exclusion +// covers Zero's in-process file tools and nothing else. No OS wrapper is built +// at all in that mode, so a shell command is confined by nothing and an +// escalation has nothing to bypass. This test exists so that stops being an +// implicit property — changing any of it should mean changing this test on +// purpose, not discovering the behavior later. +func TestDisabledPolicyLeavesShellOutsideTheTokenBoundary(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: Policy{Mode: ModeDisabled}}) + + shell := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", + WorkspaceRoot: ws, + SideEffect: SideEffectShell, + Args: map[string]any{"command": "cat " + token}, + }) + if shell.Action != ActionAllow { + t.Fatalf("shell under a disabled policy = %q (%s); the token boundary is documented as in-process only", shell.Action, shell.Reason) + } + + // The same command's payload IS blocked when it arrives as a path-carrying + // request, which is the whole of the guarantee. + read := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": token}, + }) + if read.Action != ActionDeny { + t.Fatalf("in-process read under a disabled policy = %q (%s), want deny", read.Action, read.Reason) + } + + if !engine.UnsandboxedExecutionAllowed() { + t.Fatal("escalation under a disabled policy must stay allowed: there is no wrapper for it to bypass") + } + + // With the sandbox on, the same configured token flips both: the profile is + // built, so escalating out of it would drop a real deny rule. + enforcing := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: DefaultPolicy()}) + if enforcing.UnsandboxedExecutionAllowed() { + t.Fatal("escalation must be refused while a bridge token is protected by an active profile") + } +} + // TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems covers the // bypass a case-variant spelling opened: pathWithinRoot ends in filepath.Rel, // which folds case on Windows but NOT on darwin, whose default APFS volume is diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index a30030f01..128655eb7 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -789,6 +789,13 @@ func seatbeltProtectedMetadataRegex(root string, name string) string { return "^" + escapedRoot + "/" + escapedName + "(/.*)?$" } +// denyReadRules emits seatbelt deny rules for the profile's read-denied paths. +// The rules name paths — `(literal …)` for files, `(subpath …)` for directories +// — because that is the whole of seatbelt's vocabulary here. A name created +// afterwards for the same inode, most obviously a hard link made by a sandboxed +// shell, is not covered by any of them. That applies equally to a +// user-configured DenyRead and to the automatic remote-bridge-token deny; the +// in-process tools close inode aliases separately (see protectedPathDenied). func denyReadRules(fs FileSystemPolicy) []string { return denySeatbeltPathRules("file-read*", fs.DenyRead) } diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index f2bb8e8f0..c28db439a 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -171,6 +171,16 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { } } +// TestDaemonTokenAliasesDeniedEndToEnd covers the in-process tools, which are +// the layer that can close inode aliases: they see every requested path before +// opening it, so a symlink or hard link to the token resolves back to the +// protected inode and is refused. +// +// The OS layer deliberately does not match this. Seatbelt and Bubblewrap rules +// name paths, so a sandboxed shell on macOS can `ln alias && cat alias` +// — the same pathname model a user-configured DenyRead has always had. See +// protectedPathDenied and denyReadRules in internal/sandbox for why that is the +// boundary rather than an omission. func TestDaemonTokenAliasesDeniedEndToEnd(t *testing.T) { for _, aliasKind := range []string{"symlink", "hardlink"} { t.Run(aliasKind, func(t *testing.T) { From 6e561b63fc88ef02a8dbb1fcca5902846d4912af Mon Sep 17 00:00:00 2001 From: Amp Date: Wed, 29 Jul 2026 23:58:55 +0000 Subject: [PATCH 09/17] test(sandbox): pin daemon token protections Amp-Thread-ID: https://ampcode.com/threads/T-019fafa2-7d9d-75bc-8801-6d0efc8db1df Co-authored-by: Pierre Bruno --- internal/cli/daemon.go | 1 + internal/cli/daemon_test.go | 80 +++++++++++++++++++ internal/daemon/remote/auth_test.go | 5 +- internal/sandbox/pathlists.go | 3 + .../sandbox/protected_credentials_test.go | 21 +++++ 5 files changed, 109 insertions(+), 1 deletion(-) diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index dafede623..e7e563474 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -63,6 +63,7 @@ Commands: daemon. Requires a bearer token in $ZERO_DAEMON_REMOTE_TOKEN (or $ZERO_DAEMON_REMOTE_TOKEN_FILE). --bundle-dir enables git-bundle uploads, extracted into per-link work trees. + An inline token takes precedence, so a stale token-file pointer is intentionally not protected as the live credential. link --remote --repo --id [--out ] Upload repo's git history to the remote as a bundle and print the extracted remote path. --out saves a session diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index 5418537cc..f37145d1a 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -2,8 +2,19 @@ package cli import ( "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "os" + "path/filepath" "strings" "testing" + "time" ) // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches @@ -121,3 +132,72 @@ func TestDaemonSubcommandsRejectExtraArgs(t *testing.T) { } } } + +func TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers(t *testing.T) { + isolateDaemonPaths(t) + certFile, keyFile := writeDaemonTestCertificate(t) + startDir := t.TempDir() + t.Chdir(startDir) + if err := os.WriteFile("token", []byte("bridge-token"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN", "") + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", "token") + + code, _, _ := runDaemonCLI(t, "serve-remote", "--addr", "127.0.0.1:not-a-port", "--tls-cert", certFile, "--tls-key", keyFile) + if code != exitCrash { + t.Fatalf("serve-remote exit = %d, want bind failure", code) + } + want := filepath.Join(startDir, "token") + if got := os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"); got != want { + t.Fatalf("ZERO_DAEMON_REMOTE_TOKEN_FILE = %q, want daemon-pinned path %q", got, want) + } +} + +func writeDaemonTestCertificate(t *testing.T) (string, string) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "zero-daemon-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + dir := t.TempDir() + certFile, keyFile := filepath.Join(dir, "cert.pem"), filepath.Join(dir, "key.pem") + certOut, err := os.Create(certFile) + if err != nil { + t.Fatal(err) + } + if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: der}); err != nil { + t.Fatal(err) + } + if err := certOut.Close(); err != nil { + t.Fatal(err) + } + keyDER, err := x509.MarshalECPrivateKey(key) + if err != nil { + t.Fatal(err) + } + keyOut, err := os.Create(keyFile) + if err != nil { + t.Fatal(err) + } + if err := pem.Encode(keyOut, &pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}); err != nil { + t.Fatal(err) + } + if err := keyOut.Close(); err != nil { + t.Fatal(err) + } + return certFile, keyFile +} diff --git a/internal/daemon/remote/auth_test.go b/internal/daemon/remote/auth_test.go index 4fef66b82..25c21acee 100644 --- a/internal/daemon/remote/auth_test.go +++ b/internal/daemon/remote/auth_test.go @@ -81,8 +81,11 @@ func TestCanonicalizeTokenFileEnv(t *testing.T) { if got := os.Getenv(EnvTokenFile); got != token { t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) } + // Workers run from session directories, so prove the selected path remains + // pinned after crossing that boundary rather than merely inspecting the env. + t.Chdir(t.TempDir()) if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { - t.Fatalf("TokenFromEnv after canonicalization = %q, %v", tok, err) + t.Fatalf("TokenFromEnv from a worker directory after canonicalization = %q, %v", tok, err) } }) diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 811254c37..9d6d2e748 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -139,6 +139,9 @@ func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBl // protectedPathDenied reports whether path targets one of the protected // credential files. There is no allow-list consultation by design. func protectedPathDenied(protected []string, workspaceRoot, path string) bool { + if len(protected) == 0 { + return false + } for _, entry := range protected { if pathUnderProtectedRoot(path, entry, workspaceRoot) { return true diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index cc2f46cfb..25d5e7cc9 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -140,6 +140,27 @@ func TestProtectedCredentialsSurviveAllowRead(t *testing.T) { } } +func TestProtectedCredentialDirExcluded(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true, AllowRead: []string{token}}, + }) + + if !engine.ReadExclusions().DirExcluded(token) { + t.Fatalf("DirExcluded must enforce the protected credential path %q", token) + } +} + +func TestProtectedCredentialPreventsUnsandboxedExecution(t *testing.T) { + ws, _ := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: DefaultPolicy()}) + + if engine.UnsandboxedExecutionAllowed() { + t.Fatal("UnsandboxedExecutionAllowed must stay false while a credential path is protected") + } +} + // TestProtectedCredentialsRejectSessionPermissionProfile covers the other // re-inclusion route: a session/turn permission profile that asks for the token // path must not be auto-applicable. From d402545a69d123d7883c06cd190bc595317066ca Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 30 Jul 2026 09:25:15 +0200 Subject: [PATCH 10/17] test(cli): compare canonical daemon token path Amp-Thread-ID: https://ampcode.com/threads/T-019fb1dc-2159-77ee-b020-f471d016544e Co-authored-by: Amp --- internal/cli/daemon_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index f37145d1a..ee01cd207 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -149,6 +149,10 @@ func TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers(t *testing t.Fatalf("serve-remote exit = %d, want bind failure", code) } want := filepath.Join(startDir, "token") + want, err := filepath.EvalSymlinks(want) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", want, err) + } if got := os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"); got != want { t.Fatalf("ZERO_DAEMON_REMOTE_TOKEN_FILE = %q, want daemon-pinned path %q", got, want) } From fefbdf88b2083e55528f2803b25d78e8335587cc Mon Sep 17 00:00:00 2001 From: Amp Date: Fri, 31 Jul 2026 16:24:19 +0000 Subject: [PATCH 11/17] fix(sandbox): fail closed for daemon tokens Amp-Thread-ID: https://ampcode.com/threads/T-019fb8cf-a980-7568-80f1-fe34faaaecae Co-authored-by: Pierre Bruno --- internal/sandbox/manager.go | 61 +++++++++++++++- internal/sandbox/manager_test.go | 120 +++++++++++++++++++++++++++++++ internal/sandbox/runner.go | 8 +-- internal/sandbox/runner_test.go | 15 ++++ 4 files changed, 198 insertions(+), 6 deletions(-) diff --git a/internal/sandbox/manager.go b/internal/sandbox/manager.go index 44fd6f0a0..c2f2f5630 100644 --- a/internal/sandbox/manager.go +++ b/internal/sandbox/manager.go @@ -228,8 +228,15 @@ func (manager SandboxManager) BuildExecutionRequest(request SandboxManagerReques if request.ValidateExecution && preference == SandboxPreferenceRequire && backend.SupportLevel() != BackendSupportNative { return SandboxExecutionRequest{}, nativeSandboxUnavailableError(backend) } - if request.ValidateExecution && preference != SandboxPreferenceForbid && backend.SupportLevel() != BackendSupportNative && policyHasExplicitDeny(policy) { - return SandboxExecutionRequest{}, errors.New("native sandbox unavailable: configured deny_read or deny_write rules cannot be enforced") + protectedCredentials := protectedCredentialPaths() + protectedCredentialNeedsNative := policy.Mode != ModeDisabled && len(protectedCredentials) > 0 + if request.ValidateExecution && preference != SandboxPreferenceForbid && backend.SupportLevel() != BackendSupportNative && + (policyHasExplicitDeny(policy) || protectedCredentialNeedsNative) { + return SandboxExecutionRequest{}, errors.New("native sandbox unavailable: configured deny rules or protected credentials cannot be enforced") + } + if request.ValidateExecution && preference != SandboxPreferenceForbid && policy.Mode != ModeDisabled && manager.goos == "darwin" && + protectedCredentialInWritableMacOSRoot(profile, protectedCredentials) { + return SandboxExecutionRequest{}, errors.New("macOS sandbox cannot protect the remote token file inside a shell-writable root; move the token file outside the workspace and temporary directories") } // Windows: the FULL OS sandbox needs a one-time elevated `zero sandbox setup` // (it applies WFP network filters + workspace ACLs and writes a marker). @@ -290,6 +297,56 @@ func policyHasExplicitDeny(policy Policy) bool { return len(normalizeProfilePaths(policy.DenyRead)) > 0 || len(normalizeProfilePaths(policy.DenyWrite)) > 0 } +func protectedCredentialInWritableMacOSRoot(profile PermissionProfile, protected []string) bool { + if len(protected) == 0 { + return false + } + if profile.FileSystem.Kind == FileSystemUnrestricted { + return true + } + if profile.FileSystem.Kind != FileSystemRestricted { + return false + } + writeRoots := make([]string, 0, len(profile.FileSystem.WriteRoots)+len(sandboxWritableSubpaths)) + for _, root := range profile.FileSystem.WriteRoots { + writeRoots = append(writeRoots, normalizeProfilePath(root.Root)) + } + if profile.FileSystem.AllowTemp { + writeRoots = append(writeRoots, normalizeProfilePaths(sandboxWritableSubpaths)...) + } + for _, credential := range protected { + credential = filepath.Clean(strings.TrimSpace(credential)) + if credential == "." || credential == "" { + continue + } + for _, root := range writeRoots { + if pathWithinMacOSRoot(root, credential) { + return true + } + } + } + return false +} + +func pathWithinMacOSRoot(root, candidate string) bool { + if pathWithinRoot(root, candidate) || pathWithinRoot(strings.ToLower(root), strings.ToLower(candidate)) { + return true + } + rootInfo, err := os.Stat(root) + if err != nil { + return false + } + for current := candidate; ; current = filepath.Dir(current) { + if info, err := os.Stat(current); err == nil && os.SameFile(rootInfo, info) { + return true + } + parent := filepath.Dir(current) + if parent == current { + return false + } + } +} + func (manager SandboxManager) BuildCommandPlan(request SandboxManagerRequest) (CommandPlan, error) { execRequest, err := manager.BuildExecutionRequest(request) if err != nil { diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 15fe54093..9c1ffcf75 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -250,6 +250,126 @@ func TestSandboxManagerDegradesUnavailableCommandPlan(t *testing.T) { } } +func TestSandboxManagerRejectsUnavailableBackendForProtectedToken(t *testing.T) { + workspace, _ := protectedTokenFixture(t) + policy := DefaultPolicy() + backend := Backend{Name: BackendUnavailable, Platform: "linux", Fallback: true, Message: "native sandbox unavailable"} + _, err := NewSandboxManager(SandboxManagerOptions{GOOS: "linux", Backend: backend}).BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: workspace, + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: workspace}, + Policy: policy, + Profile: PermissionProfileFromPolicy(workspace, policy, nil), + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil || !strings.Contains(err.Error(), "protected credentials cannot be enforced") { + t.Fatalf("BuildCommandPlan error = %v, want protected-credential enforcement failure", err) + } +} + +func TestSandboxManagerRejectsMacOSTokenInsideWritableWorkspace(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "/workspace/bridge-token") + workspace := "/workspace" + policy := DefaultPolicy() + backend := Backend{ + Name: BackendMacOSSeatbelt, + Available: true, + Executable: "/usr/bin/sandbox-exec", + Platform: "darwin", + CommandWrapping: true, + NativeIsolation: true, + } + _, err := NewSandboxManager(SandboxManagerOptions{GOOS: "darwin", Backend: backend}).BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: workspace, + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: workspace}, + Policy: policy, + Profile: PermissionProfileFromPolicy(workspace, policy, nil), + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil || !strings.Contains(err.Error(), "inside a shell-writable root") { + t.Fatalf("BuildCommandPlan error = %v, want macOS writable-token failure", err) + } +} + +func TestProtectedCredentialInWritableMacOSRootMatchesSeatbeltWrites(t *testing.T) { + restricted := PermissionProfile{FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: "/Users/Test/Workspace"}}, + }} + if !protectedCredentialInWritableMacOSRoot(restricted, []string{"/users/test/workspace/token"}) { + t.Fatal("case-variant token under a macOS write root should be rejected") + } + if protectedCredentialInWritableMacOSRoot(restricted, []string{"/Users/Test/Credentials/token"}) { + t.Fatal("token outside every macOS write root should remain allowed") + } + restricted.FileSystem.AllowTemp = true + if !protectedCredentialInWritableMacOSRoot(restricted, []string{"/private/tmp/bridge-token"}) { + t.Fatal("token under an allowed temporary root should be rejected") + } + unrestricted := PermissionProfile{FileSystem: FileSystemPolicy{Kind: FileSystemUnrestricted}} + if !protectedCredentialInWritableMacOSRoot(unrestricted, []string{"/credentials/token"}) { + t.Fatal("an unrestricted macOS filesystem makes every token path shell-writable") + } + + writable := t.TempDir() + targetDir := t.TempDir() + target := filepath.Join(targetDir, "token") + if err := os.WriteFile(target, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(writable, "token-link") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + symlinkProfile := PermissionProfile{FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: writable}}, + }} + if !protectedCredentialInWritableMacOSRoot(symlinkProfile, []string{link, target}) { + t.Fatal("selected symlink inside a write root must be rejected even when its target is outside") + } +} + +func TestSandboxManagerLeavesProtectedTokenShellOpenWhenSandboxDisabled(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "/workspace/bridge-token") + policy := DefaultPolicy() + policy.Mode = ModeDisabled + backend := Backend{Name: BackendUnavailable, Platform: "darwin", Fallback: true, Message: "native sandbox unavailable"} + request, err := NewSandboxManager(SandboxManagerOptions{GOOS: "darwin", Backend: backend}).BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: "/workspace", + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: "/workspace"}, + Policy: policy, + Profile: PermissionProfileFromPolicy("/workspace", policy, nil), + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest disabled policy: %v", err) + } + if request.EnforcementLevel != EnforcementDisabled || request.CommandWrapped { + t.Fatalf("disabled request = %#v, want intentionally open shell", request) + } + + policy = DefaultPolicy() + request, err = NewSandboxManager(SandboxManagerOptions{GOOS: "darwin", Backend: backend}).BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: "/workspace", + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: "/workspace"}, + Policy: policy, + Profile: PermissionProfileFromPolicy("/workspace", policy, nil), + Preference: SandboxPreferenceForbid, + ValidateExecution: true, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest forbidden sandbox preference: %v", err) + } + if request.EnforcementLevel != EnforcementDisabled || request.CommandWrapped { + t.Fatalf("forbidden-sandbox request = %#v, want intentionally open shell", request) + } +} + func TestSandboxManagerSelectsPlatformBackend(t *testing.T) { tests := []struct { name string diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 128655eb7..f798692f4 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -1001,10 +1001,10 @@ func seatbeltPlatformRuntimeRules() string { ` (literal "/dev/zero"))`, `(allow file-read-data file-test-existence file-write-data (subpath "/dev/fd"))`, `(allow file-read* file-test-existence file-write-data file-ioctl (literal "/dev/dtracehelper"))`, - `(allow file-read* file-test-existence file-write* (subpath "/tmp"))`, - `(allow file-read* file-write* (subpath "/private/tmp"))`, - `(allow file-read* file-write* (subpath "/var/tmp"))`, - `(allow file-read* file-write* (subpath "/private/var/tmp"))`, + `(allow file-read* file-test-existence (subpath "/tmp"))`, + `(allow file-read* (subpath "/private/tmp"))`, + `(allow file-read* (subpath "/var/tmp"))`, + `(allow file-read* (subpath "/private/var/tmp"))`, `(allow file-read* file-test-existence`, ` (literal "/System/Library/CoreServices")`, ` (literal "/System/Library/CoreServices/.SystemVersionPlatform.plist")`, diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 52d4c779a..35ddd2d99 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -515,6 +515,21 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { } } +func TestSeatbeltTemporaryWritesFollowPermissionProfile(t *testing.T) { + runtimeRules := seatbeltPlatformRuntimeRules() + if strings.Contains(runtimeRules, `file-write* (subpath "/tmp")`) { + t.Fatal("platform runtime rules must not bypass FileSystemPolicy.AllowTemp") + } + withoutTemp := seatbeltWriteRule(FileSystemPolicy{Kind: FileSystemRestricted}) + if strings.Contains(withoutTemp, `(subpath "/tmp")`) { + t.Fatal("restricted profile with AllowTemp=false unexpectedly permits /tmp writes") + } + withTemp := seatbeltWriteRule(FileSystemPolicy{Kind: FileSystemRestricted, AllowTemp: true}) + if !strings.Contains(withTemp, `(subpath "/tmp")`) { + t.Fatal("restricted profile with AllowTemp=true must permit /tmp writes") + } +} + // TestSeatbeltProfileAllowsGitWritesExceptHooksAndConfig locks in the fix for // git subprocesses (fetch, commit, add, ...) failing under the sandbox: the // default profile must stop write-denying the whole .git tree and only carve From 83f85782920c1d395ea3c8f8846a18e5d9fc16b5 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Fri, 31 Jul 2026 19:28:00 +0200 Subject: [PATCH 12/17] test(sandbox): normalize macOS credential paths --- internal/sandbox/manager_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 9c1ffcf75..a0075a448 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -298,14 +298,14 @@ func TestProtectedCredentialInWritableMacOSRootMatchesSeatbeltWrites(t *testing. Kind: FileSystemRestricted, WriteRoots: []WritableRoot{{Root: "/Users/Test/Workspace"}}, }} - if !protectedCredentialInWritableMacOSRoot(restricted, []string{"/users/test/workspace/token"}) { + if !protectedCredentialInWritableMacOSRoot(restricted, []string{normalizeProfilePath("/users/test/workspace/token")}) { t.Fatal("case-variant token under a macOS write root should be rejected") } - if protectedCredentialInWritableMacOSRoot(restricted, []string{"/Users/Test/Credentials/token"}) { + if protectedCredentialInWritableMacOSRoot(restricted, []string{normalizeProfilePath("/Users/Test/Credentials/token")}) { t.Fatal("token outside every macOS write root should remain allowed") } restricted.FileSystem.AllowTemp = true - if !protectedCredentialInWritableMacOSRoot(restricted, []string{"/private/tmp/bridge-token"}) { + if !protectedCredentialInWritableMacOSRoot(restricted, []string{normalizeProfilePath("/private/tmp/bridge-token")}) { t.Fatal("token under an allowed temporary root should be rejected") } unrestricted := PermissionProfile{FileSystem: FileSystemPolicy{Kind: FileSystemUnrestricted}} From 35ce8b9fbdb5dfe33c4f1c05704eec07fce07260 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 1 Aug 2026 15:55:07 +0000 Subject: [PATCH 13/17] fix(sandbox): parse quoted patch paths Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfb-b1e9-765e-ac9c-240512dfcd2f Co-authored-by: Pierre Bruno --- internal/sandbox/risk.go | 22 +++++++++++-- internal/tools/daemon_token_exclusion_test.go | 33 +++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index a0e784f67..18ffeb043 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -325,15 +325,31 @@ func patchHeaderPaths(patch string) []string { oldRemaining, newRemaining = parsePatchHunkCounts(line) inHunk = oldRemaining > 0 || newRemaining > 0 case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "): - fields := strings.Fields(line) - if len(fields) >= 2 { - paths = append(paths, stripPatchPrefix(fields[1])) + if path := patchFileHeaderPath(line); path != "" { + paths = append(paths, stripPatchPrefix(path)) } } } return paths } +// patchFileHeaderPath mirrors the apply_patch parser's handling of unified-diff +// file headers: paths may be C-quoted and may contain spaces, while an optional +// timestamp is separated by a tab. +func patchFileHeaderPath(line string) string { + rest := line[len("--- "):] // "--- " and "+++ " are both 4 bytes + if tab := strings.IndexByte(rest, '\t'); tab >= 0 { + rest = rest[:tab] + } + rest = strings.TrimSpace(rest) + if len(rest) >= 2 && rest[0] == '"' && rest[len(rest)-1] == '"' { + if unquoted, err := strconv.Unquote(rest); err == nil { + return unquoted + } + } + return rest +} + func parsePatchHunkCounts(line string) (int, int) { _, rest, ok := strings.Cut(line, "@@") if !ok { diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index c28db439a..0a1636e9a 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -171,6 +171,39 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { } } +func TestApplyPatchDeniesQuotedDaemonTokenPathWithSpaces(t *testing.T) { + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvToken, "") + t.Setenv(remote.EnvTokenFile, token) + engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: sandbox.DefaultPolicy()}) + + registry := NewRegistry() + registry.Register(NewScopedApplyPatchTool(ws, nil)) + patch := "diff --git \"a/bridge token\" \"b/bridge token\"\n" + + "--- \"a/bridge token\"\n" + + "+++ \"b/bridge token\"\n" + + "@@ -1 +1 @@\n" + + "-bridge-secret\n" + + "+attacker-controlled\n" + result := registry.RunWithOptions(context.Background(), "apply_patch", map[string]any{ + "patch": patch, + }, RunOptions{Sandbox: engine, PermissionGranted: true}) + if result.Status == StatusOK || !strings.Contains(result.Output, "remote bridge token") { + t.Fatalf("apply_patch on quoted protected path: status=%s output=%q, want bridge-token denial", result.Status, result.Output) + } + contents, err := os.ReadFile(token) + if err != nil || string(contents) != "bridge-secret\n" { + t.Fatalf("token changed after denied patch: contents=%q err=%v", contents, err) + } +} + // TestDaemonTokenAliasesDeniedEndToEnd covers the in-process tools, which are // the layer that can close inode aliases: they see every requested path before // opening it, so a symlink or hard link to the token resolves back to the From ab2b7d495bdc44d9015efca3069948d1ccc16107 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 1 Aug 2026 20:13:28 +0000 Subject: [PATCH 14/17] fix(sandbox): parse git patch paths consistently Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-bf93-702d-9a2a-35f0335e1e0c Co-authored-by: Pierre Bruno --- internal/sandbox/risk.go | 148 +++++++++++++++++- internal/tools/apply_patch.go | 127 +-------------- internal/tools/apply_patch_paths_test.go | 15 +- internal/tools/daemon_token_exclusion_test.go | 130 +++++++++++++++ 4 files changed, 290 insertions(+), 130 deletions(-) diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index 18ffeb043..b3ddcc40b 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -261,7 +261,7 @@ func applyPatchRequestPaths(args map[string]any) []string { } cwd := firstArgString(args, "cwd") var paths []string - for _, path := range patchHeaderPaths(patch) { + for _, path := range PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -281,7 +281,7 @@ func applyPatchPathBlock(request Request) *pathBlock { if patch == "" { return nil } - for _, path := range patchHeaderPaths(patch) { + for _, path := range PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -296,7 +296,10 @@ func applyPatchPathBlock(request Request) *pathBlock { return nil } -func patchHeaderPaths(patch string) []string { +// PatchHeaderPaths returns every source and destination path declared by a +// patch. Keeping this parser shared with apply_patch ensures the sandbox gate +// and the tool's own path validation agree on what git will read or write. +func PatchHeaderPaths(patch string) []string { var paths []string oldRemaining, newRemaining := 0, 0 inHunk := false @@ -317,9 +320,32 @@ func patchHeaderPaths(patch string) []string { inHunk = false switch { case strings.HasPrefix(line, "diff --git "): - fields := strings.Fields(line) - if len(fields) >= 4 { - paths = append(paths, stripPatchPrefix(fields[2]), stripPatchPrefix(fields[3])) + if source, destination, ok := parseDiffGitPaths(line[len("diff --git "):]); ok { + paths = append(paths, stripPatchPrefix(source), stripPatchPrefix(destination)) + } + case strings.HasPrefix(line, "copy from "): + if path, ok := parseExtendedGitPath(line[len("copy from "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "copy to "): + if path, ok := parseExtendedGitPath(line[len("copy to "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename from "): + if path, ok := parseExtendedGitPath(line[len("rename from "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename to "): + if path, ok := parseExtendedGitPath(line[len("rename to "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename old "): + if path, ok := parseExtendedGitPath(line[len("rename old "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename new "): + if path, ok := parseExtendedGitPath(line[len("rename new "):]); ok { + paths = append(paths, filepath.ToSlash(path)) } case strings.HasPrefix(line, "@@"): oldRemaining, newRemaining = parsePatchHunkCounts(line) @@ -333,6 +359,115 @@ func patchHeaderPaths(patch string) []string { return paths } +func parseDiffGitPaths(line string) (string, string, bool) { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "\"") { + source, rest, ok := consumeGitPath(line) + if !ok { + return "", "", false + } + destination, ok := parseWholeGitPath(rest) + return source, destination, ok + } + + // When only the destination is quoted, its opening quote uniquely separates + // the two operands even if the unquoted source contains ordinary spaces. + for separator := strings.Index(line, " \""); separator >= 0; { + if destination, ok := parseWholeGitPath(line[separator+1:]); ok { + return line[:separator], destination, true + } + next := strings.Index(line[separator+1:], " \"") + if next < 0 { + break + } + separator += next + 1 + } + + // Git commonly leaves ordinary spaces unquoted. For a non-rename diff, the + // synthetic a/ and b/ operands name the same path, which identifies the split + // without treating a filename space as an operand separator. + for separator := 0; separator < len(line); { + next := strings.IndexByte(line[separator:], ' ') + if next < 0 { + break + } + separator += next + source, destination := line[:separator], strings.TrimLeft(line[separator:], " ") + if strings.HasPrefix(source, "a/") && strings.HasPrefix(destination, "b/") && source[2:] == destination[2:] { + return source, destination, true + } + separator++ + } + + // Different unquoted names are disambiguated by the copy/rename headers. Keep + // the simple no-space form for ordinary diffs with distinct path operands. + fields := strings.Fields(line) + if len(fields) == 2 { + return fields[0], fields[1], true + } + return "", "", false +} + +func parseWholeGitPath(input string) (string, bool) { + input = strings.TrimSpace(input) + if input == "" { + return "", false + } + if input[0] != '"' { + return input, true + } + path, rest, ok := consumeGitPath(input) + return path, ok && strings.TrimSpace(rest) == "" +} + +// Extended copy/rename headers consume the whole unquoted remainder as the +// pathname, including leading spaces. For a C-quoted name Git consumes the +// first complete quoted string, so trailing header text is not pathname data. +func parseExtendedGitPath(input string) (string, bool) { + if input == "" { + return "", false + } + if input[0] != '"' { + return input, true + } + path, _, ok := consumeGitPath(input) + return path, ok +} + +// consumeGitPath reads one diff --git path operand. Git uses C-style quoted +// strings for paths containing characters that need escaping; strconv.Unquote +// handles its escaped quotes, backslashes, control characters, and octal bytes. +func consumeGitPath(input string) (string, string, bool) { + input = strings.TrimLeft(input, " \t") + if input == "" { + return "", "", false + } + if input[0] != '"' { + end := strings.IndexAny(input, " \t") + if end < 0 { + return input, "", true + } + return input[:end], input[end:], true + } + + escaped := false + for i := 1; i < len(input); i++ { + switch { + case escaped: + escaped = false + case input[i] == '\\': + escaped = true + case input[i] == '"': + path, err := strconv.Unquote(input[:i+1]) + if err != nil { + return "", "", false + } + return path, input[i+1:], true + } + } + return "", "", false +} + // patchFileHeaderPath mirrors the apply_patch parser's handling of unified-diff // file headers: paths may be C-quoted and may contain spaces, while an optional // timestamp is separated by a tab. @@ -382,7 +517,6 @@ func patchHunkCount(spec string) int { } func stripPatchPrefix(path string) string { - path = strings.TrimSpace(path) if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { path = path[2:] } diff --git a/internal/tools/apply_patch.go b/internal/tools/apply_patch.go index ab6fab952..96c564188 100644 --- a/internal/tools/apply_patch.go +++ b/internal/tools/apply_patch.go @@ -6,8 +6,9 @@ import ( "os" "os/exec" "path/filepath" - "strconv" "strings" + + "github.com/Gitlawb/zero/internal/sandbox" ) type applyPatchTool struct { @@ -118,7 +119,7 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a func missingPatchTargets(root string, patch string) []string { seen := map[string]bool{} var missing []string - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -160,7 +161,7 @@ func recordCreatedPatchTargets(tracker *FileTracker, missingBefore []string) { func changedFilesFromPatch(relativeRoot string, patch string) []string { seen := map[string]bool{} var paths []string - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -178,7 +179,7 @@ func changedFilesFromPatch(relativeRoot string, patch string) []string { } func validatePatchPaths(root string, patch string) error { - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -193,7 +194,7 @@ func validatePatchPaths(root string, patch string) error { } func recheckPatchWriteTargets(root string, patch string) error { - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -203,119 +204,3 @@ func recheckPatchWriteTargets(root string, patch string) error { } return nil } - -// patchHeaderPaths returns the file paths declared in a unified diff's headers -// (`diff --git` and `---`/`+++` lines). It tracks hunk state by counting body -// lines from each `@@ -a,b +c,d @@` header, so a removed/added content line that -// merely begins with "--- "/"+++ " (e.g. the removal of a markdown line "-- x") -// is NOT mistaken for a file header. This mirrors how `git apply` parses hunks, -// so a line this skips is content git won't write to either — no security gap. -func patchHeaderPaths(patch string) []string { - var paths []string - oldRemaining, newRemaining := 0, 0 - inHunk := false - for _, line := range strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") { - if inHunk && (oldRemaining > 0 || newRemaining > 0) { - switch { - case strings.HasPrefix(line, "-"): - oldRemaining-- - case strings.HasPrefix(line, "+"): - newRemaining-- - case strings.HasPrefix(line, "\\"): - // "\ No newline at end of file" — not a content line. - default: // context line (" ...") or a blank context line - oldRemaining-- - newRemaining-- - } - continue - } - inHunk = false - switch { - case strings.HasPrefix(line, "diff --git "): - fields := strings.Fields(line) - if len(fields) >= 4 { - paths = append(paths, stripPatchPrefix(fields[2]), stripPatchPrefix(fields[3])) - } - case strings.HasPrefix(line, "@@"): - oldRemaining, newRemaining = parseHunkCounts(line) - inHunk = oldRemaining > 0 || newRemaining > 0 - case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "): - // Take everything after the fixed 4-char prefix instead of splitting on - // spaces, so a path that contains spaces survives. Drop a trailing - // tab-delimited timestamp (unified-diff convention) and unquote a - // C-quoted git path (git quotes names with spaces/specials) (L18). - rest := line[len("--- "):] // "--- " and "+++ " are both 4 bytes - if tab := strings.IndexByte(rest, '\t'); tab >= 0 { - rest = rest[:tab] - } - if p := strings.TrimSpace(unquoteGitPath(rest)); p != "" && p != "/dev/null" { - paths = append(paths, stripPatchPrefix(p)) - } - } - } - return paths -} - -// parseHunkCounts reads the old/new line counts from a "@@ -a,b +c,d @@" header. -// A missing count (e.g. "@@ -a +c @@") means 1 per unified-diff convention. -// -// Only the range section BETWEEN the opening and closing "@@" is parsed. A hunk -// header may carry a free-form section heading after the closing "@@" (e.g. -// "@@ -1,1 +1,1 @@ func foo()"), and that text can itself contain "+"/"-" -// tokens. Scanning the whole line would let a crafted heading like -// "@@ -1,1 +1,1 @@ +1,999999" overwrite the real count, keep the parser stuck in -// hunk mode, and swallow later "--- "/"+++ " file headers so they escape -// validatePatchPaths / recheckPatchWriteTargets — a workspace-confinement bypass. -func parseHunkCounts(line string) (int, int) { - _, rest, ok := strings.Cut(line, "@@") - if !ok { - return 0, 0 - } - rangeSection := rest - if before, _, ok := strings.Cut(rest, "@@"); ok { - rangeSection = before // drop the section heading after the closing "@@" - } - old, next := 0, 0 - for _, field := range strings.Fields(rangeSection) { - switch { - case strings.HasPrefix(field, "-"): - old = hunkCount(field[1:]) - case strings.HasPrefix(field, "+"): - next = hunkCount(field[1:]) - } - } - return old, next -} - -func hunkCount(spec string) int { - if _, count, ok := strings.Cut(spec, ","); ok { - if n, err := strconv.Atoi(count); err == nil { - return n - } - return 0 - } - return 1 -} - -// unquoteGitPath undoes git's C-style quoting of a diff path. Git wraps a path in -// double quotes and backslash-escapes special bytes (spaces, tabs, high bytes as -// octal) when it contains anything unusual; an unquoted path is returned as-is. -func unquoteGitPath(s string) string { - s = strings.TrimSpace(s) - if len(s) >= 2 && s[0] == '"' && s[len(s)-1] == '"' { - if unquoted, err := strconv.Unquote(s); err == nil { - return unquoted - } - } - return s -} - -func stripPatchPrefix(path string) string { - path = strings.TrimSpace(path) - // A unified-diff path carries exactly one of the a/ or b/ prefixes; strip a - // single one so a real directory literally named "a" or "b" is preserved. - if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { - path = path[2:] - } - return filepath.ToSlash(path) -} diff --git a/internal/tools/apply_patch_paths_test.go b/internal/tools/apply_patch_paths_test.go index 2d255a358..7244bfe14 100644 --- a/internal/tools/apply_patch_paths_test.go +++ b/internal/tools/apply_patch_paths_test.go @@ -3,6 +3,8 @@ package tools import ( "slices" "testing" + + "github.com/Gitlawb/zero/internal/sandbox" ) func TestPatchHeaderPathsHandlesQuotedAndSpacedNames(t *testing.T) { @@ -13,7 +15,7 @@ func TestPatchHeaderPathsHandlesQuotedAndSpacedNames(t *testing.T) { patch := "--- \"a/dir/file name.go\"\t2024-01-01 00:00:00\n" + "+++ \"b/dir/file name.go\"\n" + "@@ -1 +1 @@\n-old\n+new\n" - got := patchHeaderPaths(patch) + got := sandbox.PatchHeaderPaths(patch) if !slices.Contains(got, "dir/file name.go") { t.Fatalf("quoted spaced path not extracted: %v", got) } @@ -21,7 +23,7 @@ func TestPatchHeaderPathsHandlesQuotedAndSpacedNames(t *testing.T) { func TestPatchHeaderPathsUnspacedStillWorks(t *testing.T) { patch := "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n-old\n+new\n" - got := patchHeaderPaths(patch) + got := sandbox.PatchHeaderPaths(patch) if !slices.Contains(got, "x.go") { t.Fatalf("plain path not extracted: %v", got) } @@ -30,3 +32,12 @@ func TestPatchHeaderPathsUnspacedStillWorks(t *testing.T) { t.Fatalf("/dev/null should not be a header path: %v", got) } } + +func TestPatchHeaderPathsHandlesCQuotedDiffGitOperands(t *testing.T) { + patch := "diff --git \"a/bridge\\040token\" \"b/exposed\\tcopy\"\n" + got := sandbox.PatchHeaderPaths(patch) + want := []string{"bridge token", "exposed\tcopy"} + if !slices.Equal(got, want) { + t.Fatalf("C-quoted diff --git operands = %q, want %q", got, want) + } +} diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 0a1636e9a..13a81f564 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -204,6 +204,136 @@ func TestApplyPatchDeniesQuotedDaemonTokenPathWithSpaces(t *testing.T) { } } +func TestApplyPatchDeniesHeaderOnlyAndBinaryDaemonTokenPatches(t *testing.T) { + original := []byte("bridge-secret\x00original\n") + for _, tc := range []struct { + name string + tokenName string + patch string + destination string + wantControlSource []byte + wantControlTarget []byte + }{ + { + name: "header-only copy", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/exposed-token\n" + + "similarity index 100%\n" + + "copy from bridge token\n" + + "copy to exposed-token\n", + destination: "exposed-token", + wantControlSource: original, + wantControlTarget: original, + }, + { + name: "header-only rename", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/renamed-token\n" + + "similarity index 100%\n" + + "rename from bridge token\n" + + "rename to renamed-token\n", + destination: "renamed-token", + wantControlTarget: original, + }, + { + name: "header-only rename aliases", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/alias-renamed-token\n" + + "similarity index 100%\n" + + "rename old bridge token\n" + + "rename new alias-renamed-token\n", + destination: "alias-renamed-token", + wantControlTarget: original, + }, + { + name: "header-only copy preserves leading space", + tokenName: " bridge-token", + patch: "diff --git a/ bridge-token b/leading-space-copy\n" + + "similarity index 100%\n" + + "copy from bridge-token\n" + + "copy to leading-space-copy\n", + destination: "leading-space-copy", + wantControlSource: original, + wantControlTarget: original, + }, + { + name: "binary modification", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/bridge token\n" + + "index 6e4018bb778ca15e70706fe1f7a4c22e762f37b6..d3eae57ec6ad245ec7ab173e28141ac2f87cca68 100644\n" + + "GIT binary patch\n" + + "literal 23\n" + + "ecmYc+DM?JuPA$?+Ni0cZ$jwj5Ov_A7;Q|0@R|sMN\n\n" + + "literal 23\n" + + "ecmYc)%1lX5)h$j Date: Sat, 1 Aug 2026 23:27:23 +0200 Subject: [PATCH 15/17] test(sandbox): pin the ambiguous git header the parser cannot split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PatchHeaderPaths yields nothing for a `diff --git` line whose two operands both contain unquoted spaces and name different files: no split is recoverable from the line alone. That is only safe because git cannot resolve it either — real patches of that shape carry the copy/rename headers that disambiguate them, and without those git refuses the patch outright, so the token gate has nothing to miss. This asserts both halves, so a future git that accepted the form would fail here instead of silently becoming a gap. Co-Authored-By: Claude Opus 5 (1M context) --- internal/tools/daemon_token_exclusion_test.go | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 13a81f564..278f85948 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -334,6 +334,50 @@ func TestApplyPatchDeniesHeaderOnlyAndBinaryDaemonTokenPatches(t *testing.T) { } } +// TestAmbiguousGitHeaderIsNotAnApplicablePatch pins the residual assumption in +// the shared header parser: for a `diff --git` line whose two operands both +// contain unquoted spaces and name DIFFERENT files, no split is recoverable +// from the line alone, so PatchHeaderPaths yields nothing for it. That is only +// safe because git cannot resolve the line either — every real patch of that +// shape carries the copy/rename headers (parsed separately) that disambiguate +// it. Without those headers git refuses the patch outright, so there is nothing +// for the token gate to miss. +// +// If a future git ever accepted this form, this test fails and the parser needs +// a real split rule rather than the current heuristics. +func TestAmbiguousGitHeaderIsNotAnApplicablePatch(t *testing.T) { + patch := "diff --git a/bridge token b/exposed token\n" + + "GIT binary patch\n" + + "literal 5\n" + + "LcmZQzU|<4=0Rj{Q\n\n" + + "literal 0\n" + + "HcmV?d00001\n\n" + if paths := sandbox.PatchHeaderPaths(patch); len(paths) != 0 { + t.Fatalf("PatchHeaderPaths = %q; this test only means anything while the line is unresolvable", paths) + } + + workspace := t.TempDir() + const original = "bridge-secret\n" + if err := os.WriteFile(filepath.Join(workspace, "bridge token"), []byte(original), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + registry := NewRegistry() + registry.Register(NewScopedApplyPatchTool(workspace, nil)) + result := registry.RunWithOptions(context.Background(), "apply_patch", map[string]any{ + "patch": patch, + }, RunOptions{PermissionGranted: true}) + + if result.Status == StatusOK { + t.Fatalf("git applied an ambiguous header patch: %q", result.Output) + } + if contents, err := os.ReadFile(filepath.Join(workspace, "bridge token")); err != nil || string(contents) != original { + t.Fatalf("token changed: contents=%q err=%v", contents, err) + } + if _, err := os.Stat(filepath.Join(workspace, "exposed token")); !os.IsNotExist(err) { + t.Fatalf("ambiguous patch created a destination file: err=%v", err) + } +} + // TestDaemonTokenAliasesDeniedEndToEnd covers the in-process tools, which are // the layer that can close inode aliases: they see every requested path before // opening it, so a symlink or hard link to the token resolves back to the From 1642762ce57a8993a2750e82674824dd27a6a1df Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 2 Aug 2026 22:25:57 +0200 Subject: [PATCH 16/17] fix(sandbox): close remaining daemon token gaps --- internal/sandbox/manager_test.go | 45 +++++++++++++++++++ internal/sandbox/profile.go | 18 ++++---- internal/sandbox/risk.go | 15 +++++-- internal/tools/apply_patch_paths_test.go | 17 +++++++ internal/tools/daemon_token_exclusion_test.go | 44 ++++++++++++++++++ 5 files changed, 127 insertions(+), 12 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index a0075a448..1328e6df0 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "reflect" "runtime" + "slices" "strings" "testing" ) @@ -615,6 +616,50 @@ func TestPermissionProfileDeniesDaemonTokenFile(t *testing.T) { } } +// TestPermissionProfileDeniesAbsentDaemonTokenFile covers the window an external +// secret rotation opens: the token file is gone, but the pathname is still the +// one the next `serve-remote` start will read. Existence-filtering it out of the +// profile would let a sandboxed process recreate it with an attacker-chosen +// bearer, so the mandatory entry survives the file's absence for backend +// enforcement. +func TestPermissionProfileDeniesAbsentDaemonTokenFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read paths are disabled on Windows pending the ACL model") + } + workspace := t.TempDir() + tokenFile := filepath.Join(workspace, "daemon-token") + if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, tokenFile) + + // Rotation removes the file after the bridge has already loaded its token. + if err := os.Remove(tokenFile); err != nil { + t.Fatal(err) + } + + profile := PermissionProfileFromPolicy(workspace, DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{tokenFile})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want the absent token path %q still denied", profile.FileSystem.DenyRead, want) + } + + // Seatbelt must also deny writes because the workspace root is otherwise + // writable. The replacement is read by the next bridge start, not this one. + rules := credentialDenyWriteRules(profile.FileSystem, DefaultPolicy()) + if !slices.ContainsFunc(rules, func(rule string) bool { return strings.Contains(rule, want) }) { + t.Fatalf("seatbelt credential write rules = %#v, want a deny for %q", rules, want) + } + + // Bubblewrap has no target to bind for a missing path, so the fallback must + // still produce an unreadable, unwritable mount rather than nothing. + args := appendUnreadableLinuxPathArgs(nil, want) + if !slices.Contains(args, "--tmpfs") || !slices.Contains(args, want) { + t.Fatalf("bubblewrap args for the absent token = %#v, want an unreadable tmpfs at %q", args, want) + } +} + func TestCredentialDenyReadPathsForEnvironmentHonorsConfigOverrides(t *testing.T) { root := t.TempDir() configHome := filepath.Join(root, "xdg") diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index e7c1c7259..ec02974d3 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -307,14 +307,16 @@ func credentialDenyReadPathsForEnvironment(env credentialPathEnvironment, allowR out = append(out, path) } } - for _, path := range mandatory { - // Existence filtering still applies: a deny rule for a path that is not - // there protects nothing, and Bubblewrap cannot bind a missing target. - if _, err := os.Stat(path); err != nil { - continue - } - out = append(out, path) - } + // Unlike the candidates above, the token path is NOT existence-filtered. The + // entry protects a future write target as much as a current secret: after an + // external rotation deletes the file, dropping the rule would let a sandboxed + // process recreate it with an attacker-chosen bearer that the next + // `serve-remote` start would then accept. Each enforcing backend materializes + // a missing target fail-closed — bubblewrap mounts an unreadable tmpfs + // (appendUnreadableLinuxPathArgs), seatbelt names the path both literally and + // as a subpath, and Landlock refuses deny-read profiles outright — which is + // also why protectedCredentialPaths keeps the configured lexical path. + out = append(out, mandatory...) return dedupeStrings(out) } diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index b3ddcc40b..3d198bcec 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -471,15 +471,22 @@ func consumeGitPath(input string) (string, string, bool) { // patchFileHeaderPath mirrors the apply_patch parser's handling of unified-diff // file headers: paths may be C-quoted and may contain spaces, while an optional // timestamp is separated by a tab. +// +// Only the tab is header formatting. Git's own parser takes every remaining byte +// of an unquoted operand as pathname data, so a name with a leading or trailing +// space is written verbatim; trimming here would make the token gate evaluate +// `bridge-token` while git patches the live `bridge-token ` beside it. func patchFileHeaderPath(line string) string { rest := line[len("--- "):] // "--- " and "+++ " are both 4 bytes if tab := strings.IndexByte(rest, '\t'); tab >= 0 { rest = rest[:tab] } - rest = strings.TrimSpace(rest) - if len(rest) >= 2 && rest[0] == '"' && rest[len(rest)-1] == '"' { - if unquoted, err := strconv.Unquote(rest); err == nil { - return unquoted + if strings.HasPrefix(rest, `"`) { + // A quoted operand ends at its closing quote; git allows nothing but the + // (already removed) timestamp after it, so a trailing remainder means the + // line is not the quoted form and the raw bytes are the pathname. + if path, remainder, ok := consumeGitPath(rest); ok && remainder == "" { + return path } } return rest diff --git a/internal/tools/apply_patch_paths_test.go b/internal/tools/apply_patch_paths_test.go index 7244bfe14..d5b8bb766 100644 --- a/internal/tools/apply_patch_paths_test.go +++ b/internal/tools/apply_patch_paths_test.go @@ -33,6 +33,23 @@ func TestPatchHeaderPathsUnspacedStillWorks(t *testing.T) { } } +// git only C-quotes names containing characters it must escape, so a filename +// whose last byte is an ordinary space reaches the header raw. git apply reads +// it to the tab or newline and patches that exact file, so trimming the operand +// here would leave the token gate comparing against a neighbouring pathname. +func TestPatchHeaderPathsPreservesSurroundingSpacesInNames(t *testing.T) { + patch := "--- a/bridge-token \t2024-01-01 00:00:00\n" + + "+++ b/bridge-token \n" + + "@@ -1 +1 @@\n-old\n+new\n" + got := sandbox.PatchHeaderPaths(patch) + if !slices.Contains(got, "bridge-token ") { + t.Fatalf("trailing-space path not preserved: %q", got) + } + if slices.Contains(got, "bridge-token") { + t.Fatalf("trailing space trimmed off the header path: %q", got) + } +} + func TestPatchHeaderPathsHandlesCQuotedDiffGitOperands(t *testing.T) { patch := "diff --git \"a/bridge\\040token\" \"b/exposed\\tcopy\"\n" got := sandbox.PatchHeaderPaths(patch) diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 278f85948..472defb17 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "runtime" "strings" "testing" @@ -334,6 +335,49 @@ func TestApplyPatchDeniesHeaderOnlyAndBinaryDaemonTokenPatches(t *testing.T) { } } +// TestApplyPatchDeniesTrailingSpaceDaemonTokenPath pins the unified-diff form +// git leaves unquoted: a header operand whose last byte is an ordinary space. +// The parser must keep that byte rather than trim it as header formatting. +func TestApplyPatchDeniesTrailingSpaceDaemonTokenPath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows filenames cannot end in a space") + } + const original = "bridge-secret\n" + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token ") + if err := os.WriteFile(token, []byte(original), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + + t.Setenv(remote.EnvToken, "") + // CanonicalizeTokenFileEnv is the production daemon boundary and preserves + // filename whitespace while converting the selected path to an absolute one. + t.Setenv(remote.EnvTokenFile, filepath.Join(ws, ".", "bridge-token ")) + if err := remote.CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + + engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: sandbox.DefaultPolicy()}) + registry := NewRegistry() + registry.Register(NewScopedApplyPatchTool(ws, nil)) + result := registry.RunWithOptions(context.Background(), "apply_patch", map[string]any{ + "patch": "--- a/bridge-token \n" + + "+++ b/bridge-token \n" + + "@@ -1 +1 @@\n" + + "-bridge-secret\n" + + "+attacker-controlled\n", + }, RunOptions{Sandbox: engine, PermissionGranted: true}) + if result.Status == StatusOK || !strings.Contains(result.Output, "remote bridge token") { + t.Fatalf("apply_patch on the selected token: status=%s output=%q, want bridge-token denial", result.Status, result.Output) + } + if contents, err := os.ReadFile(token); err != nil || string(contents) != original { + t.Fatalf("token changed after denied patch: contents=%q err=%v", contents, err) + } +} + // TestAmbiguousGitHeaderIsNotAnApplicablePatch pins the residual assumption in // the shared header parser: for a `diff --git` line whose two operands both // contain unquoted spaces and name DIFFERENT files, no split is recoverable From cf653d2560566d3e21c3ee08a3676bc711bc797b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 2 Aug 2026 23:37:22 +0200 Subject: [PATCH 17/17] fix(daemon): treat the token-file pointer as a pathname, not a trimmed word TestApplyPatchDeniesTrailingSpaceDaemonTokenPath failed on ubuntu and macos (and took the Zero Review job's test step down with it): the test selects a token file named "bridge-token " through the production daemon boundary, but CanonicalizeTokenFileEnv trimmed the environment value before resolving it, so it looked for "bridge-token" and failed closed on a file that exists. The trim was applied to a value that names a file. Every consumer has to agree on which bytes that is: TokenFromEnv reads it with os.ReadFile, which is byte-exact, and the sandbox derives its protected-credential paths from the same variable. With the trim, a token whose filename begins or ends with a space is read by neither, yet the deny rules would protect a different, possibly attacker-controlled pathname. Take the value verbatim in all three places; a value that is entirely whitespace still counts as unset, which is what a blank variable means. Regressions: the pointer survives leading/trailing whitespace and blank values read as unset (remote), canonicalization keeps a trailing-space filename byte for byte and TokenFromEnv still reads it from another working directory (remote), and protectedCredentialPaths protects the spaced name rather than the trimmed one (sandbox). Co-Authored-By: Claude Opus 5 (1M context) --- internal/daemon/remote/auth.go | 21 +++++++- internal/daemon/remote/auth_test.go | 53 +++++++++++++++++++ internal/sandbox/pathlists.go | 12 ++++- .../sandbox/protected_credentials_test.go | 22 ++++++++ 4 files changed, 105 insertions(+), 3 deletions(-) diff --git a/internal/daemon/remote/auth.go b/internal/daemon/remote/auth.go index abc44de52..b1cedd455 100644 --- a/internal/daemon/remote/auth.go +++ b/internal/daemon/remote/auth.go @@ -67,13 +67,30 @@ func (a *TokenAuthenticator) Authenticate(token string) error { return ErrUnauthorized } +// TokenFilePathFromEnv returns the configured token-file pointer exactly as the +// operator set it, or "" when the variable is unset or holds only whitespace. +// +// The value is a PATHNAME, and every consumer of it — os.ReadFile here, the +// canonicalization below, the sandbox's protected-credential list — must agree +// on which bytes name the file. A filename may legitimately begin or end with a +// space, so trimming the value would make this boundary read one file while the +// deny rules protect another. A value that is only whitespace still reads as +// unset, which is what a blank variable means. +func TokenFilePathFromEnv() string { + configured := os.Getenv(EnvTokenFile) + if strings.TrimSpace(configured) == "" { + return "" + } + return configured +} + // TokenFromEnv resolves the bridge token from EnvToken, or a file named by // EnvTokenFile. It never logs the token. func TokenFromEnv() (string, error) { if t := strings.TrimSpace(os.Getenv(EnvToken)); t != "" { return t, nil } - if file := strings.TrimSpace(os.Getenv(EnvTokenFile)); file != "" { + if file := TokenFilePathFromEnv(); file != "" { data, err := os.ReadFile(file) if err != nil { return "", fmt.Errorf("remote: read token file: %w", err) @@ -108,7 +125,7 @@ func CanonicalizeTokenFileEnv() error { if strings.TrimSpace(os.Getenv(EnvToken)) != "" { return nil } - configured := strings.TrimSpace(os.Getenv(EnvTokenFile)) + configured := TokenFilePathFromEnv() if configured == "" { return nil } diff --git a/internal/daemon/remote/auth_test.go b/internal/daemon/remote/auth_test.go index 25c21acee..0ba660df4 100644 --- a/internal/daemon/remote/auth_test.go +++ b/internal/daemon/remote/auth_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "testing" ) @@ -57,6 +58,58 @@ func TestTokenFromEnv(t *testing.T) { } } +// TestTokenFilePathFromEnvPreservesFilenameWhitespace pins the pointer as a +// pathname rather than a trimmed word. Trimming it made this boundary select +// "/bridge-token" while the operator had named "/bridge-token " — +// which fails the daemon start at best, and at worst reads and protects a +// different file than the one holding the bearer token. +func TestTokenFilePathFromEnvPreservesFilenameWhitespace(t *testing.T) { + t.Setenv(EnvToken, "") + for _, configured := range []string{"/srv/zero/bridge-token ", " /srv/zero/bridge-token", "/srv/zero/ token "} { + t.Setenv(EnvTokenFile, configured) + if got := TokenFilePathFromEnv(); got != configured { + t.Fatalf("TokenFilePathFromEnv() = %q, want the configured pathname %q", got, configured) + } + } + for _, blank := range []string{"", " ", "\t\n"} { + t.Setenv(EnvTokenFile, blank) + if got := TokenFilePathFromEnv(); got != "" { + t.Fatalf("TokenFilePathFromEnv() = %q for a blank value, want unset", got) + } + } +} + +// TestCanonicalizeTokenFileEnvKeepsTrailingSpaceFilename covers the same rule at +// the daemon boundary: the file the bridge authenticates against must survive +// canonicalization byte for byte. +func TestCanonicalizeTokenFileEnvKeepsTrailingSpaceFilename(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows filenames cannot end in a space") + } + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "bridge-token ") + if err := os.WriteFile(token, []byte("from-file\n"), 0o600); err != nil { + t.Fatalf("write token file: %v", err) + } + t.Chdir(base) + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "bridge-token ") + + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) + } + t.Chdir(t.TempDir()) + if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { + t.Fatalf("TokenFromEnv after canonicalization = %q, %v", tok, err) + } +} + // TestCanonicalizeTokenFileEnv pins the value every child process (and the // sandbox profile derived for it) inherits to the file this process reads. func TestCanonicalizeTokenFileEnv(t *testing.T) { diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 9d6d2e748..6ab75b92d 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -53,11 +53,21 @@ const ( // selectedDaemonRemoteTokenFile returns the token-file pointer only when the // daemon would use it. TokenFromEnv gives the inline token precedence, so an // inherited file pointer is not a credential when both variables are set. +// +// The pointer is used verbatim, mirroring remote.TokenFilePathFromEnv: the +// daemon reads whatever bytes the variable names, so trimming whitespace here +// would leave a token file whose name begins or ends with a space unprotected +// while the daemon still reads it. Only a value that is entirely whitespace +// counts as unset. func selectedDaemonRemoteTokenFile() string { if strings.TrimSpace(os.Getenv(daemonRemoteTokenEnv)) != "" { return "" } - return strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) + configured := os.Getenv(daemonRemoteTokenFileEnv) + if strings.TrimSpace(configured) == "" { + return "" + } + return configured } // protectedCredentialPaths returns credential files that Zero's own in-process diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index 25d5e7cc9..5888747f2 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -80,6 +80,28 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { } }) + t.Run("filename whitespace is part of the pathname", func(t *testing.T) { + // The daemon reads exactly the bytes the variable names, so trimming the + // pointer here would protect "/token" while the bearer file is + // "/token " — leaving the real credential readable and replaceable. + if runtime.GOOS == "windows" { + t.Skip("Windows filenames cannot end in a space") + } + spaced := filepath.Join(base, "token ") + if err := os.WriteFile(spaced, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, spaced) + got := protectedCredentialPaths() + if !stringSliceContains(got, spaced) { + t.Fatalf("protected paths = %#v, want %q", got, spaced) + } + if stringSliceContains(got, token) { + t.Fatalf("protected paths = %#v, must not protect the trimmed name %q", got, token) + } + }) + t.Run("a symlinked pathname protects the link and its target", func(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink creation needs elevation on Windows")