From 51b2ea25dbd0c78ff4e29a98682b1f3a8e8b8c10 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 21:13:10 +0200 Subject: [PATCH 01/18] fix(sandbox): deny reads of zero credential stores --- internal/sandbox/manager_test.go | 65 +++++++++++++++++++++++++++---- internal/sandbox/profile.go | 67 +++++++++++++++++++++++++++----- 2 files changed, 115 insertions(+), 17 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index a2b8550f2..32d67fc1e 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -378,16 +378,38 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { home := t.TempDir() awsDir := filepath.Join(home, ".aws") gcloudDir := filepath.Join(home, ".config", "gcloud") - if err := mkdirAll(awsDir, gcloudDir); err != nil { + zeroDir := filepath.Join(home, "config", "zero") + if err := mkdirAll(awsDir, gcloudDir, zeroDir); err != nil { t.Fatal(err) } keyFile := filepath.Join(home, "sa-key.json") - if err := os.WriteFile(keyFile, []byte("{}"), 0o600); err != nil { - t.Fatal(err) + oauthOverride := filepath.Join(home, "custom-oauth.json") + mcpOverride := filepath.Join(home, "custom-mcp-oauth.json") + zeroFiles := []string{ + filepath.Join(zeroDir, "config.json"), + filepath.Join(zeroDir, "credentials.json"), + filepath.Join(zeroDir, "credentials.enc"), + filepath.Join(zeroDir, "credentials.enc.secret"), + filepath.Join(zeroDir, "oauth-tokens.json"), + filepath.Join(zeroDir, "oauth-tokens.json.secret"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json"), + } + for _, path := range append([]string{keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) { + if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } } - paths := credentialDenyReadPathsIn(home, keyFile, nil) - for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile}) { + options := credentialPathOptions{ + Home: home, + GoogleCredentials: keyFile, + ZeroConfigDir: filepath.Join(home, "config"), + OAuthTokens: oauthOverride, + MCPOAuthTokens: mcpOverride, + } + paths := credentialDenyReadPathsIn(options, nil) + wantPaths := append([]string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) + for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) } @@ -399,22 +421,49 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } // An explicit AllowRead entry covering a store is an opt-out. - optedOut := credentialDenyReadPathsIn(home, keyFile, []string{awsDir}) + optedOut := credentialDenyReadPathsIn(options, []string{awsDir, zeroDir}) if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) } + for _, zeroFile := range normalizeProfilePaths(zeroFiles) { + if stringSliceContains(optedOut, zeroFile) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop %q", optedOut, zeroFile) + } + } if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } - if got := credentialDenyReadPathsIn(" ", "", nil); len(got) != 0 { + if got := credentialDenyReadPathsIn(credentialPathOptions{}, 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(credentialPathOptions{GoogleCredentials: keyFile}, nil) if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths without home = %#v, want key file included", homeless) } } + +func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + configHome := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configHome) + zeroDir := filepath.Join(configHome, "zero") + if err := os.MkdirAll(zeroDir, 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(zeroDir, "oauth-tokens.json") + if err := os.WriteFile(secret, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{secret})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want Zero credential file %q", profile.FileSystem.DenyRead, want) + } +} diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 194736667..c699fd817 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -148,9 +148,9 @@ 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: +// cloud credential stores, the file GOOGLE_APPLICATION_CREDENTIALS points to, +// and Zero's own config/token/key files 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 @@ -166,26 +166,58 @@ func credentialDenyReadPaths(policy Policy) []string { if runtime.GOOS == "windows" { return nil } - // A failed home lookup only drops the home-based candidates; the - // GOOGLE_APPLICATION_CREDENTIALS target must be protected regardless. + // Failed home/config lookups only drop their derived candidates; explicit + // credential-file overrides must be protected regardless. home, _ := os.UserHomeDir() - return credentialDenyReadPathsIn(home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), policy.AllowRead) + configDir, _ := zeroUserConfigDir() + return credentialDenyReadPathsIn(credentialPathOptions{ + Home: home, + GoogleCredentials: os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), + ZeroConfigDir: configDir, + OAuthTokens: os.Getenv("ZERO_OAUTH_TOKENS_PATH"), + MCPOAuthTokens: os.Getenv("ZERO_MCP_OAUTH_TOKENS_PATH"), + }, policy.AllowRead) +} + +type credentialPathOptions struct { + Home string + GoogleCredentials string + ZeroConfigDir string + OAuthTokens string + MCPOAuthTokens string } // 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(options credentialPathOptions, allowRead []string) []string { var candidates []string - if home = strings.TrimSpace(home); home != "" { + if home := strings.TrimSpace(options.Home); home != "" { candidates = append(candidates, filepath.Join(home, ".aws"), filepath.Join(home, ".config", "gcloud"), filepath.Join(home, ".azure"), ) } - if target := strings.TrimSpace(googleCredentials); target != "" { + if target := strings.TrimSpace(options.GoogleCredentials); target != "" { candidates = append(candidates, target) } + if configDir := strings.TrimSpace(options.ZeroConfigDir); configDir != "" { + zeroDir := filepath.Join(configDir, "zero") + candidates = append(candidates, + filepath.Join(zeroDir, "config.json"), + filepath.Join(zeroDir, "credentials.json"), + filepath.Join(zeroDir, "credentials.enc"), + filepath.Join(zeroDir, "credentials.enc.secret"), + filepath.Join(zeroDir, "oauth-tokens.json"), + filepath.Join(zeroDir, "oauth-tokens.json.secret"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json"), + ) + } + for _, tokenPath := range []string{options.OAuthTokens, options.MCPOAuthTokens} { + if tokenPath = strings.TrimSpace(tokenPath); tokenPath != "" { + candidates = append(candidates, tokenPath, tokenPath+".secret") + } + } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) for _, path := range normalizeProfilePaths(candidates) { @@ -207,6 +239,23 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead return out } +// zeroUserConfigDir mirrors config.UserConfigDir without importing config +// (config depends on sandbox). Zero deliberately uses ~/.config on macOS and +// os.UserConfigDir everywhere else. +func zeroUserConfigDir() (string, error) { + if runtime.GOOS != "darwin" { + return os.UserConfigDir() + } + if xdg := strings.TrimSpace(os.Getenv("XDG_CONFIG_HOME")); xdg != "" { + return xdg, nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, ".config"), nil +} + // userGitConfigReadPaths returns the user's global git config FILES so a // sandboxed git can read identity and config (user.name/email, aliases) instead // of failing with "unable to access ~/.gitconfig". It is deliberately the config From 054704609fdb72fd373adc1d1bf95240e3029860 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 23:11:59 +0200 Subject: [PATCH 02/18] fix(sandbox): address PR review comments - Deny reads of mcp-oauth-tokens.json.secret in the default Zero config candidates: file-backed oauth stores keep their encryption secret in a sibling .secret file, so the default MCP token store needs the same protection as the override paths (CodeRabbit). - Clarify that explicit credential-file overrides are still filtered by on-disk existence like every other candidate (Copilot). - Align the zeroUserConfigDir doc comment with config.UserConfigDir: macOS honors XDG_CONFIG_HOME before falling back to ~/.config (Copilot). Co-Authored-By: Claude Fable 5 --- internal/sandbox/manager_test.go | 1 + internal/sandbox/profile.go | 10 +++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 32d67fc1e..420f89c2f 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -393,6 +393,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { filepath.Join(zeroDir, "oauth-tokens.json"), filepath.Join(zeroDir, "oauth-tokens.json.secret"), filepath.Join(zeroDir, "mcp-oauth-tokens.json"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json.secret"), } for _, path := range append([]string{keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) { if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index c699fd817..3528f4546 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -167,7 +167,8 @@ func credentialDenyReadPaths(policy Policy) []string { return nil } // Failed home/config lookups only drop their derived candidates; explicit - // credential-file overrides must be protected regardless. + // credential-file overrides are still submitted as candidates (and, like + // every candidate, filtered by on-disk existence below). home, _ := os.UserHomeDir() configDir, _ := zeroUserConfigDir() return credentialDenyReadPathsIn(credentialPathOptions{ @@ -211,6 +212,7 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string filepath.Join(zeroDir, "oauth-tokens.json"), filepath.Join(zeroDir, "oauth-tokens.json.secret"), filepath.Join(zeroDir, "mcp-oauth-tokens.json"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json.secret"), ) } for _, tokenPath := range []string{options.OAuthTokens, options.MCPOAuthTokens} { @@ -240,8 +242,10 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string } // zeroUserConfigDir mirrors config.UserConfigDir without importing config -// (config depends on sandbox). Zero deliberately uses ~/.config on macOS and -// os.UserConfigDir everywhere else. +// (config depends on sandbox). On macOS Zero deliberately honors +// XDG_CONFIG_HOME when set and falls back to ~/.config, instead of the +// os.UserConfigDir default (~/Library/Application Support); everywhere else +// it uses os.UserConfigDir. func zeroUserConfigDir() (string, error) { if runtime.GOOS != "darwin" { return os.UserConfigDir() From 43e81fdd453c849684bc67d8970d58bbcdd0ed16 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 15 Jul 2026 04:30:08 +0200 Subject: [PATCH 03/18] fix(sandbox): deny the whole Zero config dir, not itemized filenames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code review on PR #681: - Deny credentialDenyReadPaths' Zero-config candidate as the containing directory instead of an itemized filename list. The token/credential/ config stores each publish through a randomly-named .tmp-- sibling before their atomic rename, and the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated backup after importing it — none of those names were covered by the old itemized list, so a sandboxed command could read them under the read-all posture. - Stop dropping default candidates that don't exist yet at profile-build time. A store created later in a long-lived sandboxed session (e.g. a concurrent ) previously got no deny rule at all; every backend already treats a deny rule over a not-yet-existing path as a harmless no-op that still takes effect once the path appears. - Resolve ZERO_OAUTH_TOKENS_PATH / ZERO_MCP_OAUTH_TOKENS_PATH overrides the same way the token stores resolve them (relative-to-cwd, no ~ expansion) instead of through normalizeProfilePath, which tilde-expands and so could derive a deny path different from where the store actually writes. Adds regression coverage for the directory-wide deny (including the migrated backup and synthetic temp-file siblings), for building the profile before the store directory exists, and for the tilde-override resolution mismatch. Co-Authored-By: Claude Sonnet 5 --- internal/sandbox/manager_test.go | 83 +++++++++++++++++++++++++++----- internal/sandbox/profile.go | 63 ++++++++++++++++-------- 2 files changed, 116 insertions(+), 30 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 420f89c2f..43fcde5a6 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -385,6 +385,9 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { keyFile := filepath.Join(home, "sa-key.json") oauthOverride := filepath.Join(home, "custom-oauth.json") mcpOverride := filepath.Join(home, "custom-mcp-oauth.json") + // The migrated legacy MCP token backup and the atomic-write temp siblings + // every store publishes before its rename; none of these are itemized by + // name, so they only stay protected if the whole zeroDir is denied. zeroFiles := []string{ filepath.Join(zeroDir, "config.json"), filepath.Join(zeroDir, "credentials.json"), @@ -394,6 +397,10 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { filepath.Join(zeroDir, "oauth-tokens.json.secret"), filepath.Join(zeroDir, "mcp-oauth-tokens.json"), filepath.Join(zeroDir, "mcp-oauth-tokens.json.secret"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json.migrated"), + filepath.Join(zeroDir, "oauth-tokens.json.tmp-1234-5678"), + filepath.Join(zeroDir, "credentials.enc.9-1.tmp"), + filepath.Join(zeroDir, ".zero-config-1.tmp"), } for _, path := range append([]string{keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) { if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { @@ -409,16 +416,27 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { MCPOAuthTokens: mcpOverride, } paths := credentialDenyReadPathsIn(options, nil) - wantPaths := append([]string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) + wantPaths := []string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret", zeroDir} for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) } } + // zeroFiles is covered by the zeroDir subpath deny above, not by an + // itemized entry — including the never-enumerated migrated backup and + // temp-write siblings. + for _, zeroFile := range zeroFiles { + if stringSliceContains(paths, normalizeProfilePaths([]string{zeroFile})[0]) { + t.Errorf("credential deny paths = %#v, want itemized %q dropped in favor of the zeroDir subpath rule", paths, zeroFile) + } + } - // A path the host does not have is dropped, not emitted blind. - if stringSliceContains(paths, filepath.Join(home, ".azure")) { - t.Errorf("credential deny paths = %#v, must not include the absent ~/.azure", paths) + // A default candidate absent from disk at profile-build time is still + // emitted: a rule installed only for what exists now would miss a store + // created later in a long-lived sandboxed session (e.g. a concurrent + // `zero auth login`, or a token file appearing mid-session). + if !stringSliceContains(paths, filepath.Join(home, ".azure")) { + t.Errorf("credential deny paths = %#v, want the not-yet-existing ~/.azure included", paths) } // An explicit AllowRead entry covering a store is an opt-out. @@ -426,10 +444,8 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) } - for _, zeroFile := range normalizeProfilePaths(zeroFiles) { - if stringSliceContains(optedOut, zeroFile) { - t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop %q", optedOut, zeroFile) - } + if stringSliceContains(optedOut, normalizeProfilePaths([]string{zeroDir})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop %q", optedOut, zeroDir) } if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) @@ -447,6 +463,36 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } } +// TestCredentialDenyReadPathsInOverrideMatchesStoreResolution reproduces the +// audit finding that a relative-and-tilde ZERO_OAUTH_TOKENS_PATH / +// ZERO_MCP_OAUTH_TOKENS_PATH override produced a deny rule for a DIFFERENT +// path than the one the token stores actually resolve (oauth.ResolveStorePath +// / mcp.ResolveTokenStorePath never expand "~"; they resolve a relative +// override literally against the working directory), leaving the real file +// unprotected. +func TestCredentialDenyReadPathsInOverrideMatchesStoreResolution(t *testing.T) { + override := "~/relative-tilde-tokens.json" + options := credentialPathOptions{OAuthTokens: override, MCPOAuthTokens: override} + paths := credentialDenyReadPathsIn(options, nil) + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + storeResolved := filepath.Clean(filepath.Join(cwd, override)) + if !stringSliceContains(paths, storeResolved) { + t.Errorf("credential deny paths = %#v, want the store's literal resolution %q included", paths, storeResolved) + } + + home, err := os.UserHomeDir() + if err == nil { + tildeExpanded := normalizeProfilePaths([]string{override})[0] + if tildeExpanded != storeResolved && stringSliceContains(paths, tildeExpanded) { + t.Errorf("credential deny paths = %#v, must not deny the tilde-expanded %q instead of what the store resolves to (home %q)", paths, tildeExpanded, home) + } + } +} + func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") @@ -454,6 +500,16 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { configHome := t.TempDir() t.Setenv("XDG_CONFIG_HOME", configHome) zeroDir := filepath.Join(configHome, "zero") + + // Build the profile BEFORE the store directory exists on disk: a + // sandboxed command launched early in a session must still deny reads of + // credentials created later, not just ones present at profile-build time. + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{zeroDir})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want Zero config directory %q even before it exists", profile.FileSystem.DenyRead, want) + } + if err := os.MkdirAll(zeroDir, 0o700); err != nil { t.Fatal(err) } @@ -461,10 +517,15 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { if err := os.WriteFile(secret, []byte("{}"), 0o600); err != nil { t.Fatal(err) } + migrated := filepath.Join(zeroDir, "mcp-oauth-tokens.json.migrated") + if err := os.WriteFile(migrated, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } - profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) - want := normalizeProfilePaths([]string{secret})[0] + // Re-derive after the files exist: the same directory rule covers both + // the known store filename and the never-itemized migrated backup. + profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) if !stringSliceContains(profile.FileSystem.DenyRead, want) { - t.Fatalf("DenyRead = %#v, want Zero credential file %q", profile.FileSystem.DenyRead, want) + t.Fatalf("DenyRead = %#v, want Zero config directory %q", profile.FileSystem.DenyRead, want) } } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 3528f4546..08eacbf6e 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -149,8 +149,9 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // credentialDenyReadPaths returns default deny-read entries for well-known // cloud credential stores, the file GOOGLE_APPLICATION_CREDENTIALS points to, -// and Zero's own config/token/key files so sandboxed commands cannot read -// secrets under the read-all workspace posture. Two deliberate limits: +// and Zero's own config/credential/token directory so sandboxed commands +// cannot read secrets under the read-all workspace posture. Three 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 @@ -158,6 +159,12 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // 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. +// - Candidates are emitted whether or not they currently exist on disk: a +// rule installed only for stores present at profile-build time would miss +// a store created later in a long-lived sandboxed session (e.g. `zero +// auth login` run concurrently), and every backend already treats a deny +// rule over a not-yet-existing path as a harmless no-op that still takes +// effect once the path appears. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -203,30 +210,23 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string candidates = append(candidates, target) } if configDir := strings.TrimSpace(options.ZeroConfigDir); configDir != "" { - zeroDir := filepath.Join(configDir, "zero") - candidates = append(candidates, - filepath.Join(zeroDir, "config.json"), - filepath.Join(zeroDir, "credentials.json"), - filepath.Join(zeroDir, "credentials.enc"), - filepath.Join(zeroDir, "credentials.enc.secret"), - filepath.Join(zeroDir, "oauth-tokens.json"), - filepath.Join(zeroDir, "oauth-tokens.json.secret"), - filepath.Join(zeroDir, "mcp-oauth-tokens.json"), - filepath.Join(zeroDir, "mcp-oauth-tokens.json.secret"), - ) + // Deny the whole directory rather than an itemized file list. Zero's + // credential/token/config stores each publish through a randomly-named + // sibling before an atomic rename (oauth-tokens.json.tmp--, + // credentials.{enc,json}.*.tmp, *.secret.*.tmp, .zero-config-*.tmp), and + // the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated + // backup behind after importing it — an itemized list can never keep up + // with those names. Nothing else has a legitimate reason to live here. + candidates = append(candidates, filepath.Join(configDir, "zero")) } - for _, tokenPath := range []string{options.OAuthTokens, options.MCPOAuthTokens} { - if tokenPath = strings.TrimSpace(tokenPath); tokenPath != "" { + for _, override := range []string{options.OAuthTokens, options.MCPOAuthTokens} { + if tokenPath := resolveCredentialOverridePath(override); tokenPath != "" { candidates = append(candidates, tokenPath, tokenPath+".secret") } } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) for _, path := range normalizeProfilePaths(candidates) { - // Only stores that actually exist on this host need a deny rule. - if _, err := os.Stat(path); err != nil { - continue - } reincluded := false for _, allow := range allowRoots { if pathWithinRoot(allow, path) { @@ -241,6 +241,31 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string return out } +// resolveCredentialOverridePath mirrors the token stores' own override +// resolution (oauth.ResolveStorePath, mcp.ResolveTokenStorePath — duplicated +// here rather than imported, the same tradeoff zeroUserConfigDir makes, +// because internal/mcp depends on this package): a relative override is +// resolved literally against the process working directory, NOT tilde- +// expanded the way normalizeProfilePath expands other candidates. Using +// normalizeProfilePath here would derive a deny path that doesn't match +// where the store actually writes — e.g. ZERO_OAUTH_TOKENS_PATH=~/x resolves +// to /~/x on disk (the store never expands "~"), but normalizeProfilePath +// would deny $HOME/x instead, leaving the real file unprotected. +func resolveCredentialOverridePath(override string) string { + override = strings.TrimSpace(override) + if override == "" { + return "" + } + if filepath.IsAbs(override) { + return filepath.Clean(override) + } + abs, err := filepath.Abs(override) + if err != nil { + return "" + } + return filepath.Clean(abs) +} + // zeroUserConfigDir mirrors config.UserConfigDir without importing config // (config depends on sandbox). On macOS Zero deliberately honors // XDG_CONFIG_HOME when set and falls back to ~/.config, instead of the From 5b4f7438a912343352dd038720284f11b1befe04 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 13:09:45 +0000 Subject: [PATCH 04/18] fix(sandbox): address macOS deny-path test and config parity Recompute the expected Zero config deny path after MkdirAll and resolve the temp base with EvalSymlinks so macOS /var -> /private/var does not flake TestPermissionProfileDeniesZeroCredentialFiles. Add a parity test that sandbox.zeroUserConfigDir stays aligned with config.UserConfigDir. Co-authored-by: PierrunoYT --- internal/sandbox/export_test.go | 6 ++ internal/sandbox/manager_test.go | 9 ++- internal/sandbox/profile.go | 3 +- internal/sandbox/zeroconfigdir_parity_test.go | 62 +++++++++++++++++++ 4 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 internal/sandbox/export_test.go create mode 100644 internal/sandbox/zeroconfigdir_parity_test.go diff --git a/internal/sandbox/export_test.go b/internal/sandbox/export_test.go new file mode 100644 index 000000000..05b5c88fd --- /dev/null +++ b/internal/sandbox/export_test.go @@ -0,0 +1,6 @@ +package sandbox + +// ZeroUserConfigDir exports zeroUserConfigDir for parity tests against +// config.UserConfigDir. Production callers stay on the unexported helper to +// avoid growing the sandbox public surface. +var ZeroUserConfigDir = zeroUserConfigDir diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 43fcde5a6..ab788b368 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -497,7 +497,10 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") } - configHome := t.TempDir() + // Resolve the temp base up front so macOS /var -> /private/var does not + // diverge between the pre-mkdir Clean fallback and the post-mkdir + // EvalSymlinks success path inside normalizeProfilePath. + configHome := resolvedTempDir(t) t.Setenv("XDG_CONFIG_HOME", configHome) zeroDir := filepath.Join(configHome, "zero") @@ -524,7 +527,11 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { // Re-derive after the files exist: the same directory rule covers both // the known store filename and the never-itemized migrated backup. + // Recompute want once the directory exists so EvalSymlinks can resolve + // the full path (macOS would otherwise compare a pre-mkdir Clean path + // against a post-mkdir /private/var form). profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want = normalizeProfilePaths([]string{zeroDir})[0] if !stringSliceContains(profile.FileSystem.DenyRead, want) { t.Fatalf("DenyRead = %#v, want Zero config directory %q", profile.FileSystem.DenyRead, want) } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 08eacbf6e..7753b731d 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -174,8 +174,7 @@ func credentialDenyReadPaths(policy Policy) []string { return nil } // Failed home/config lookups only drop their derived candidates; explicit - // credential-file overrides are still submitted as candidates (and, like - // every candidate, filtered by on-disk existence below). + // credential-file overrides are still submitted as candidates regardless. home, _ := os.UserHomeDir() configDir, _ := zeroUserConfigDir() return credentialDenyReadPathsIn(credentialPathOptions{ diff --git a/internal/sandbox/zeroconfigdir_parity_test.go b/internal/sandbox/zeroconfigdir_parity_test.go new file mode 100644 index 000000000..4d95d570c --- /dev/null +++ b/internal/sandbox/zeroconfigdir_parity_test.go @@ -0,0 +1,62 @@ +package sandbox_test + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/sandbox" +) + +// TestZeroUserConfigDirMatchesConfigUserConfigDir guards against silent drift +// between sandbox.zeroUserConfigDir and config.UserConfigDir. The sandbox copy +// exists only to avoid an import cycle (config already depends on sandbox); if +// the two diverge, deny rules would target a different directory than the +// stores write to. +func TestZeroUserConfigDirMatchesConfigUserConfigDir(t *testing.T) { + t.Run("default", func(t *testing.T) { + assertUserConfigDirParity(t) + }) + + t.Run("xdg_override", func(t *testing.T) { + xdg := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", xdg) + assertUserConfigDirParity(t) + }) + + t.Run("xdg_cleared", func(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", "") + assertUserConfigDirParity(t) + if runtime.GOOS == "darwin" { + home, err := os.UserHomeDir() + if err != nil { + t.Fatal(err) + } + want := filepath.Join(home, ".config") + got, err := sandbox.ZeroUserConfigDir() + if err != nil { + t.Fatal(err) + } + if got != want { + t.Fatalf("zeroUserConfigDir() = %q, want macOS ~/.config fallback %q", got, want) + } + } + }) +} + +func assertUserConfigDirParity(t *testing.T) { + t.Helper() + want, err := config.UserConfigDir() + if err != nil { + t.Fatalf("config.UserConfigDir: %v", err) + } + got, err := sandbox.ZeroUserConfigDir() + if err != nil { + t.Fatalf("sandbox.ZeroUserConfigDir: %v", err) + } + if got != want { + t.Fatalf("sandbox.ZeroUserConfigDir() = %q, config.UserConfigDir() = %q", got, want) + } +} From 750328c6d27ec55cd276971c659c98766c651ff8 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 12:36:43 +0200 Subject: [PATCH 05/18] fix(sandbox): harden credential deny paths --- internal/sandbox/linux_helper.go | 12 ++++-- internal/sandbox/linux_helper_test.go | 27 ++++++++++++ internal/sandbox/manager_test.go | 27 +++++++++++- internal/sandbox/profile.go | 42 ++++++++++++------- ...nfigdir_parity_test.go => profile_test.go} | 2 +- .../sandbox/runner_linux_integration_test.go | 12 +++++- 6 files changed, 99 insertions(+), 23 deletions(-) rename internal/sandbox/{zeroconfigdir_parity_test.go => profile_test.go} (95%) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index abf05b7e6..7405356c4 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -305,10 +305,10 @@ func appendReadOnlyLinuxPathArgs(args []string, path string) []string { if path == "" { return args } - if pathExists(path) { - return append(args, "--ro-bind", path, path) + if _, err := os.Stat(path); err != nil { + return args } - return append(args, "--perms", "555", "--tmpfs", path, "--remount-ro", path) + return append(args, "--ro-bind", path, path) } func appendUnreadableLinuxPathArgs(args []string, path string) []string { @@ -316,7 +316,11 @@ func appendUnreadableLinuxPathArgs(args []string, path string) []string { if path == "" { return args } - if info, err := os.Stat(path); err == nil && !info.IsDir() { + info, err := os.Stat(path) + if err != nil { + return args + } + if !info.IsDir() { return append(args, "--ro-bind", "/dev/null", path) } return append(args, "--perms", "000", "--tmpfs", path, "--remount-ro", path) diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index 4b6602657..1049a9446 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -196,6 +196,33 @@ func TestLinuxBwrapRootReadUsesReadOnlyHostRoot(t *testing.T) { } } +func TestLinuxBwrapPathCarveoutsSkipMissingMountTargets(t *testing.T) { + root := t.TempDir() + missing := filepath.Join(root, "missing", "nested") + if got := appendReadOnlyLinuxPathArgs(nil, missing); len(got) != 0 { + t.Fatalf("missing read-only target args = %#v, want none", got) + } + if got := appendUnreadableLinuxPathArgs(nil, missing); len(got) != 0 { + t.Fatalf("missing deny-read target args = %#v, want none", got) + } + if _, err := os.Stat(filepath.Dir(missing)); !os.IsNotExist(err) { + t.Fatalf("building carveouts materialized a host path: %v", err) + } + + deniedDir := filepath.Join(root, "denied") + if err := os.Mkdir(deniedDir, 0o755); err != nil { + t.Fatal(err) + } + assertArgsContainSequence(t, appendUnreadableLinuxPathArgs(nil, deniedDir), "--perms", "000", "--tmpfs", deniedDir, "--remount-ro", deniedDir) + + deniedFile := filepath.Join(root, "secret.json") + if err := os.WriteFile(deniedFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + assertArgsContainSequence(t, appendUnreadableLinuxPathArgs(nil, deniedFile), "--ro-bind", "/dev/null", deniedFile) + assertArgsContainSequence(t, appendReadOnlyLinuxPathArgs(nil, deniedFile), "--ro-bind", deniedFile, deniedFile) +} + func TestLinuxBwrapTempUsesHostWriteRoots(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Linux bwrap temp root assertions use Unix paths") diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index ab788b368..9cc19db9d 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -416,7 +416,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { MCPOAuthTokens: mcpOverride, } paths := credentialDenyReadPathsIn(options, nil) - wantPaths := []string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret", zeroDir} + wantPaths := []string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret", mcpOverride + ".migrated", zeroDir} for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) @@ -493,6 +493,31 @@ func TestCredentialDenyReadPathsInOverrideMatchesStoreResolution(t *testing.T) { } } +func TestCredentialDenyReadPathsInConfigDirMatchesLiteralXDGResolution(t *testing.T) { + configDir := "~/literal-xdg" + t.Setenv("XDG_CONFIG_HOME", configDir) + resolvedConfigDir, err := zeroCredentialConfigDir() + if err != nil { + t.Fatal(err) + } + paths := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDir: resolvedConfigDir}, nil) + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + want := filepath.Join(cwd, configDir, "zero") + if resolvedConfigDir != filepath.Dir(want) { + t.Fatalf("zero credential config dir = %q, want literal XDG resolution %q", resolvedConfigDir, filepath.Dir(want)) + } + if !stringSliceContains(paths, want) { + t.Fatalf("credential deny paths = %#v, want literal XDG resolution %q", paths, want) + } + if expanded := normalizeProfilePaths([]string{filepath.Join(configDir, "zero")})[0]; expanded != want && stringSliceContains(paths, expanded) { + t.Fatalf("credential deny paths = %#v, must not use tilde-expanded XDG path %q", paths, expanded) + } +} + func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 7753b731d..c883c06e2 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -1,6 +1,7 @@ package sandbox import ( + "fmt" "os" "path/filepath" "runtime" @@ -159,12 +160,10 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // 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. -// - Candidates are emitted whether or not they currently exist on disk: a -// rule installed only for stores present at profile-build time would miss -// a store created later in a long-lived sandboxed session (e.g. `zero -// auth login` run concurrently), and every backend already treats a deny -// rule over a not-yet-existing path as a harmless no-op that still takes -// effect once the path appears. +// - Candidates are emitted whether or not they currently exist on disk so +// backends that support future-path rules can protect stores created later. +// The Linux bwrap backend drops missing mount targets because it cannot +// safely create them beneath its read-only root. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -176,7 +175,7 @@ func credentialDenyReadPaths(policy Policy) []string { // Failed home/config lookups only drop their derived candidates; explicit // credential-file overrides are still submitted as candidates regardless. home, _ := os.UserHomeDir() - configDir, _ := zeroUserConfigDir() + configDir, _ := zeroCredentialConfigDir() return credentialDenyReadPathsIn(credentialPathOptions{ Home: home, GoogleCredentials: os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), @@ -208,7 +207,7 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string if target := strings.TrimSpace(options.GoogleCredentials); target != "" { candidates = append(candidates, target) } - if configDir := strings.TrimSpace(options.ZeroConfigDir); configDir != "" { + if configDir := resolveCredentialOverridePath(options.ZeroConfigDir); configDir != "" { // Deny the whole directory rather than an itemized file list. Zero's // credential/token/config stores each publish through a randomly-named // sibling before an atomic rename (oauth-tokens.json.tmp--, @@ -218,10 +217,11 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string // with those names. Nothing else has a legitimate reason to live here. candidates = append(candidates, filepath.Join(configDir, "zero")) } - for _, override := range []string{options.OAuthTokens, options.MCPOAuthTokens} { - if tokenPath := resolveCredentialOverridePath(override); tokenPath != "" { - candidates = append(candidates, tokenPath, tokenPath+".secret") - } + if tokenPath := resolveCredentialOverridePath(options.OAuthTokens); tokenPath != "" { + candidates = append(candidates, tokenPath, tokenPath+".secret") + } + if tokenPath := resolveCredentialOverridePath(options.MCPOAuthTokens); tokenPath != "" { + candidates = append(candidates, tokenPath, tokenPath+".secret", tokenPath+".migrated") } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) @@ -255,14 +255,24 @@ func resolveCredentialOverridePath(override string) string { if override == "" { return "" } - if filepath.IsAbs(override) { - return filepath.Clean(override) - } abs, err := filepath.Abs(override) if err != nil { return "" } - return filepath.Clean(abs) + return abs +} + +// zeroCredentialConfigDir follows the OAuth stores' literal XDG resolution. +// In particular, "~/config" is relative syntax to those stores, not a request +// for shell-style home expansion. +func zeroCredentialConfigDir() (string, error) { + if xdg := strings.TrimSpace(os.Getenv("XDG_CONFIG_HOME")); xdg != "" { + if resolved := resolveCredentialOverridePath(xdg); resolved != "" { + return resolved, nil + } + return "", fmt.Errorf("resolve XDG_CONFIG_HOME %q", xdg) + } + return zeroUserConfigDir() } // zeroUserConfigDir mirrors config.UserConfigDir without importing config diff --git a/internal/sandbox/zeroconfigdir_parity_test.go b/internal/sandbox/profile_test.go similarity index 95% rename from internal/sandbox/zeroconfigdir_parity_test.go rename to internal/sandbox/profile_test.go index 4d95d570c..ffa3ba19a 100644 --- a/internal/sandbox/zeroconfigdir_parity_test.go +++ b/internal/sandbox/profile_test.go @@ -10,7 +10,7 @@ import ( "github.com/Gitlawb/zero/internal/sandbox" ) -// TestZeroUserConfigDirMatchesConfigUserConfigDir guards against silent drift +// TestZeroUserConfigDirMatchesConfigUserConfigDir prevents silent drift // between sandbox.zeroUserConfigDir and config.UserConfigDir. The sandbox copy // exists only to avoid an import cycle (config already depends on sandbox); if // the two diverge, deny rules would target a different directory than the diff --git a/internal/sandbox/runner_linux_integration_test.go b/internal/sandbox/runner_linux_integration_test.go index 699fdbaea..1bf5ccab5 100644 --- a/internal/sandbox/runner_linux_integration_test.go +++ b/internal/sandbox/runner_linux_integration_test.go @@ -4,6 +4,8 @@ package sandbox import ( "context" + "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -36,8 +38,13 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { t.Fatalf("Mkdir blocked: %v", err) } + missingDenied := fmt.Sprintf("/etc/zero-sandbox-missing-%d-%d/nested", os.Getpid(), time.Now().UnixNano()) + if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing deny-path precondition failed: %v", err) + } + policy := DefaultPolicy() - policy.DenyRead = []string{secretDir} + policy.DenyRead = []string{secretDir, missingDenied} policy.DenyWrite = []string{blockedDir} engine := NewEngine(EngineOptions{WorkspaceRoot: root, Policy: policy, Backend: backend}) output, runErr := runLinuxSandboxSmokeCommand(t, engine, CommandSpec{ @@ -58,6 +65,9 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { } t.Fatalf("allowed smoke command failed: %v\n%s", runErr, output) } + if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("sandbox launch materialized missing deny path on host: %v", err) + } for _, tc := range []struct { name string From c92e0ed22b92bd1bf70392648e4e8e5a44e9f048 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 12:51:08 +0200 Subject: [PATCH 06/18] test(cli): normalize credential deny baseline --- internal/cli/sandbox_test.go | 44 +++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/internal/cli/sandbox_test.go b/internal/cli/sandbox_test.go index 8145fe514..c65c1015e 100644 --- a/internal/cli/sandbox_test.go +++ b/internal/cli/sandbox_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "reflect" + "runtime" "strings" "testing" @@ -483,14 +484,16 @@ func TestTUISandboxSetupCommandGatedToWindowsNativeBackend(t *testing.T) { } func TestRunSandboxPolicyJSONGoldenIncludesManagerBaselineFields(t *testing.T) { - // Point HOME at an empty directory so the default credential-store - // deny-read entries (which depend on what exists in the real home, e.g. - // ~/.aws on the macOS CI image) cannot leak host paths into the golden - // comparison. + // Point all user-config roots at an empty directory so the platform-specific + // credential deny baseline can be asserted without leaking host paths into + // the platform-neutral golden comparison. emptyHome := t.TempDir() t.Setenv("HOME", emptyHome) t.Setenv("USERPROFILE", emptyHome) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(emptyHome, ".config")) t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "") + t.Setenv("ZERO_OAUTH_TOKENS_PATH", "") + t.Setenv("ZERO_MCP_OAUTH_TOKENS_PATH", "") store := newSandboxTestStore(t) workspace := t.TempDir() deps := appDeps{ @@ -518,7 +521,7 @@ func TestRunSandboxPolicyJSONGoldenIncludesManagerBaselineFields(t *testing.T) { got := stdout.String() got = replacePathToken(got, workspace, "$WORKSPACE") got = replacePathToken(got, store.FilePath(), "$GRANTS") - gotBytes := normalizeSandboxPolicyGoldenTempRoots(t, []byte(got), workspace) + gotBytes := normalizeSandboxPolicyGoldenTempRoots(t, []byte(got), workspace, emptyHome) wantBytes, err := os.ReadFile(filepath.Join("testdata", "sandbox_policy_windows_unavailable.golden.json")) if err != nil { t.Fatalf("read golden: %v", err) @@ -528,7 +531,7 @@ func TestRunSandboxPolicyJSONGoldenIncludesManagerBaselineFields(t *testing.T) { } } -func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, workspace string) []byte { +func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, workspace string, emptyHome string) []byte { t.Helper() scope, err := sandbox.NewScope(workspace, nil) if err != nil { @@ -549,6 +552,19 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp plan, _ := value.(map[string]any)["plan"].(map[string]any) profile, _ := plan["permissionProfile"].(map[string]any) fileSystem, _ := profile["fileSystem"].(map[string]any) + wantDenyRead := []string(nil) + if runtime.GOOS != "windows" { + wantDenyRead = []string{ + filepath.Join(emptyHome, ".aws"), + filepath.Join(emptyHome, ".config", "gcloud"), + filepath.Join(emptyHome, ".azure"), + filepath.Join(emptyHome, ".config", "zero"), + } + } + if gotDenyRead := jsonStringSlice(fileSystem["denyRead"]); !reflect.DeepEqual(gotDenyRead, wantDenyRead) { + t.Fatalf("manager credential deny baseline = %#v, want %#v", gotDenyRead, wantDenyRead) + } + delete(fileSystem, "denyRead") fileSystem["readRoots"] = filterJSONStringRoots(fileSystem["readRoots"], tempRoots) fileSystem["writeRoots"] = filterJSONWriteRoots(fileSystem["writeRoots"], tempRoots) normalized, err := json.MarshalIndent(value, "", " ") @@ -558,6 +574,22 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp return append(normalized, '\n') } +func jsonStringSlice(value any) []string { + values, ok := value.([]any) + if !ok { + return nil + } + out := make([]string, 0, len(values)) + for _, value := range values { + text, ok := value.(string) + if !ok { + return nil + } + out = append(out, text) + } + return out +} + func filterJSONStringRoots(value any, excluded map[string]struct{}) any { roots, ok := value.([]any) if !ok { From 63adeb4f683071fb6eb8a0ce0eb69eb194b0e63c Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 23:48:43 +0200 Subject: [PATCH 07/18] fix(sandbox): close credential path gaps --- internal/sandbox/export_test.go | 6 - internal/sandbox/linux_helper.go | 54 +++++-- internal/sandbox/linux_helper_test.go | 62 ++++++-- internal/sandbox/manager_test.go | 91 +++++++---- internal/sandbox/profile.go | 145 ++++++++++-------- internal/sandbox/profile_test.go | 132 +++++++++++----- internal/sandbox/runner.go | 5 +- .../sandbox/runner_linux_integration_test.go | 58 +++++-- internal/sandbox/runner_test.go | 19 ++- 9 files changed, 392 insertions(+), 180 deletions(-) delete mode 100644 internal/sandbox/export_test.go diff --git a/internal/sandbox/export_test.go b/internal/sandbox/export_test.go deleted file mode 100644 index 05b5c88fd..000000000 --- a/internal/sandbox/export_test.go +++ /dev/null @@ -1,6 +0,0 @@ -package sandbox - -// ZeroUserConfigDir exports zeroUserConfigDir for parity tests against -// config.UserConfigDir. Production callers stay on the unexported helper to -// avoid growing the sandbox public surface. -var ZeroUserConfigDir = zeroUserConfigDir diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 7405356c4..92c6d1af3 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -160,7 +160,11 @@ func BuildLinuxSandboxBwrapArgs(options LinuxSandboxBwrapOptions) ([]string, err "--new-session", "--die-with-parent", } - args = append(args, linuxBwrapFilesystemArgs(config.PermissionProfile)...) + filesystemArgs, err := linuxBwrapFilesystemArgs(config.PermissionProfile) + if err != nil { + return nil, err + } + args = append(args, filesystemArgs...) if pathExists(helperPath) { args = append(args, "--ro-bind", helperPath, helperPath) } @@ -188,7 +192,7 @@ func BuildLinuxSandboxBwrapArgs(options LinuxSandboxBwrapOptions) ([]string, err return args, nil } -func linuxBwrapFilesystemArgs(profile PermissionProfile) []string { +func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { fs := profile.FileSystem if fs.Kind == FileSystemUnrestricted { // Disabled filesystem policy means no write jail: expose the host root @@ -200,7 +204,7 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) []string { args = append(args, "--bind", root.Root, root.Root) } } - return args + return args, nil } args := []string{} @@ -228,19 +232,35 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) []string { } args = append(args, "--bind", root.Root, root.Root) for _, subpath := range root.ReadOnlySubpaths { - args = appendReadOnlyLinuxPathArgs(args, subpath) + var err error + args, err = appendReadOnlyLinuxPathArgs(args, subpath) + if err != nil { + return nil, err + } } for _, name := range root.ProtectedMetadataNames { - args = appendReadOnlyLinuxPathArgs(args, filepath.Join(root.Root, name)) + var err error + args, err = appendReadOnlyLinuxPathArgs(args, filepath.Join(root.Root, name)) + if err != nil { + return nil, err + } } } for _, path := range fs.DenyWrite { - args = appendReadOnlyLinuxPathArgs(args, path) + var err error + args, err = appendReadOnlyLinuxPathArgs(args, path) + if err != nil { + return nil, err + } } for _, path := range fs.DenyRead { - args = appendUnreadableLinuxPathArgs(args, path) + var err error + args, err = appendUnreadableLinuxPathArgs(args, path) + if err != nil { + return nil, err + } } - return args + return args, nil } func linuxWriteRootsWithTemp(fs FileSystemPolicy) []WritableRoot { @@ -300,30 +320,30 @@ func linuxPlatformReadRoots() []string { return roots } -func appendReadOnlyLinuxPathArgs(args []string, path string) []string { +func appendReadOnlyLinuxPathArgs(args []string, path string) ([]string, error) { path = strings.TrimSpace(path) if path == "" { - return args + return args, nil } if _, err := os.Stat(path); err != nil { - return args + return nil, fmt.Errorf("cannot enforce Linux read-only path %q: %w", path, err) } - return append(args, "--ro-bind", path, path) + return append(args, "--ro-bind", path, path), nil } -func appendUnreadableLinuxPathArgs(args []string, path string) []string { +func appendUnreadableLinuxPathArgs(args []string, path string) ([]string, error) { path = strings.TrimSpace(path) if path == "" { - return args + return args, nil } info, err := os.Stat(path) if err != nil { - return args + return nil, fmt.Errorf("cannot enforce Linux deny-read path %q: %w", path, err) } if !info.IsDir() { - return append(args, "--ro-bind", "/dev/null", path) + return append(args, "--ro-bind", "/dev/null", path), nil } - return append(args, "--perms", "000", "--tmpfs", path, "--remount-ro", path) + return append(args, "--perms", "000", "--tmpfs", path, "--remount-ro", path), nil } func shouldUnshareLinuxNetwork(policy NetworkPolicy) bool { diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index 1049a9446..95261d870 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -2,10 +2,12 @@ package sandbox import ( "encoding/json" + "errors" "os" "path/filepath" "reflect" "runtime" + "strings" "testing" ) @@ -91,9 +93,13 @@ func TestBuildLinuxSandboxBwrapArgsWrapsInnerSeccompStage(t *testing.T) { if err := os.WriteFile(helperPath, []byte("helper"), 0o755); err != nil { t.Fatalf("WriteFile helper: %v", err) } + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{Kind: FileSystemRestricted, ReadRoots: []string{string(filepath.Separator)}, IncludePlatformRoots: true}, + Network: NetworkPolicy{Mode: NetworkDeny}, + } args, err := BuildLinuxSandboxCommandArgs(LinuxSandboxCommandArgsOptions{ SandboxPolicyCWD: "/workspace", - PermissionProfile: DefaultPermissionProfile("/workspace"), + PermissionProfile: profile, BlockUnixSockets: true, Command: []string{"true"}, }) @@ -149,8 +155,10 @@ func TestBuildLinuxSandboxBwrapArgsKeepsHostNetworkWhenAllowed(t *testing.T) { if err := os.WriteFile(helperPath, []byte("helper"), 0o755); err != nil { t.Fatalf("WriteFile helper: %v", err) } - profile := DefaultPermissionProfile("/workspace") - profile.Network = NetworkPolicy{Mode: NetworkAllow} + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{Kind: FileSystemRestricted, ReadRoots: []string{string(filepath.Separator)}, IncludePlatformRoots: true}, + Network: NetworkPolicy{Mode: NetworkAllow}, + } args, err := BuildLinuxSandboxCommandArgs(LinuxSandboxCommandArgsOptions{ SandboxPolicyCWD: "/workspace", PermissionProfile: profile, @@ -189,23 +197,26 @@ func TestLinuxBwrapRootReadUsesReadOnlyHostRoot(t *testing.T) { Network: NetworkPolicy{Mode: NetworkAllow}, } - args := linuxBwrapFilesystemArgs(profile) + args, err := linuxBwrapFilesystemArgs(profile) + if err != nil { + t.Fatal(err) + } assertArgsContainSequence(t, args, "--ro-bind", "/", "/") if argsContainSequence(args, "--tmpfs", "/") { t.Fatalf("root-read profile must not start from an empty root: %#v", args) } } -func TestLinuxBwrapPathCarveoutsSkipMissingMountTargets(t *testing.T) { +func TestLinuxBwrapPathCarveoutsFailClosedForMissingMountTargets(t *testing.T) { root := t.TempDir() missing := filepath.Join(root, "missing", "nested") - if got := appendReadOnlyLinuxPathArgs(nil, missing); len(got) != 0 { - t.Fatalf("missing read-only target args = %#v, want none", got) + if _, err := appendReadOnlyLinuxPathArgs(nil, missing); err == nil || !strings.Contains(err.Error(), missing) { + t.Fatalf("missing read-only target error = %v, want path-specific failure", err) } - if got := appendUnreadableLinuxPathArgs(nil, missing); len(got) != 0 { - t.Fatalf("missing deny-read target args = %#v, want none", got) + if _, err := appendUnreadableLinuxPathArgs(nil, missing); err == nil || !strings.Contains(err.Error(), missing) { + t.Fatalf("missing deny-read target error = %v, want path-specific failure", err) } - if _, err := os.Stat(filepath.Dir(missing)); !os.IsNotExist(err) { + if _, err := os.Stat(filepath.Dir(missing)); !errors.Is(err, os.ErrNotExist) { t.Fatalf("building carveouts materialized a host path: %v", err) } @@ -213,14 +224,26 @@ func TestLinuxBwrapPathCarveoutsSkipMissingMountTargets(t *testing.T) { if err := os.Mkdir(deniedDir, 0o755); err != nil { t.Fatal(err) } - assertArgsContainSequence(t, appendUnreadableLinuxPathArgs(nil, deniedDir), "--perms", "000", "--tmpfs", deniedDir, "--remount-ro", deniedDir) + args, err := appendUnreadableLinuxPathArgs(nil, deniedDir) + if err != nil { + t.Fatal(err) + } + assertArgsContainSequence(t, args, "--perms", "000", "--tmpfs", deniedDir, "--remount-ro", deniedDir) deniedFile := filepath.Join(root, "secret.json") if err := os.WriteFile(deniedFile, []byte("secret"), 0o600); err != nil { t.Fatal(err) } - assertArgsContainSequence(t, appendUnreadableLinuxPathArgs(nil, deniedFile), "--ro-bind", "/dev/null", deniedFile) - assertArgsContainSequence(t, appendReadOnlyLinuxPathArgs(nil, deniedFile), "--ro-bind", deniedFile, deniedFile) + args, err = appendUnreadableLinuxPathArgs(nil, deniedFile) + if err != nil { + t.Fatal(err) + } + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", deniedFile) + args, err = appendReadOnlyLinuxPathArgs(nil, deniedFile) + if err != nil { + t.Fatal(err) + } + assertArgsContainSequence(t, args, "--ro-bind", deniedFile, deniedFile) } func TestLinuxBwrapTempUsesHostWriteRoots(t *testing.T) { @@ -233,6 +256,9 @@ func TestLinuxBwrapTempUsesHostWriteRoots(t *testing.T) { if err := os.MkdirAll(workspace, 0o755); err != nil { t.Fatalf("MkdirAll workspace: %v", err) } + if err := os.Mkdir(filepath.Join(workspace, ".git"), 0o755); err != nil { + t.Fatalf("Mkdir .git: %v", err) + } profile := PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, @@ -243,7 +269,10 @@ func TestLinuxBwrapTempUsesHostWriteRoots(t *testing.T) { Network: NetworkPolicy{Mode: NetworkAllow}, } - args := linuxBwrapFilesystemArgs(profile) + args, err := linuxBwrapFilesystemArgs(profile) + if err != nil { + t.Fatal(err) + } if argsContainSequence(args, "--tmpfs", "/tmp") { t.Fatalf("workspace-write temp access must bind host /tmp, not create private tmpfs: %#v", args) } @@ -272,7 +301,10 @@ func TestLinuxBwrapUnrestrictedFilesystemUsesWritableHostRoot(t *testing.T) { Network: NetworkPolicy{Mode: NetworkDeny}, } - args := linuxBwrapFilesystemArgs(profile) + args, err := linuxBwrapFilesystemArgs(profile) + if err != nil { + t.Fatal(err) + } assertArgsContainSequence(t, args, "--bind", "/", "/") if argsContainSequence(args, "--ro-bind", "/", "/") { t.Fatalf("unrestricted filesystem profile must not make host root read-only: %#v", args) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 9cc19db9d..a76be7c6f 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -383,8 +383,13 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { t.Fatal(err) } keyFile := filepath.Join(home, "sa-key.json") - oauthOverride := filepath.Join(home, "custom-oauth.json") - mcpOverride := filepath.Join(home, "custom-mcp-oauth.json") + oauthDir := filepath.Join(home, "oauth-store") + mcpDir := filepath.Join(home, "mcp-store") + if err := mkdirAll(oauthDir, mcpDir); err != nil { + t.Fatal(err) + } + oauthOverride := filepath.Join(oauthDir, "tokens.json") + mcpOverride := filepath.Join(mcpDir, "tokens.json") // The migrated legacy MCP token backup and the atomic-write temp siblings // every store publishes before its rename; none of these are itemized by // name, so they only stay protected if the whole zeroDir is denied. @@ -416,7 +421,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { MCPOAuthTokens: mcpOverride, } paths := credentialDenyReadPathsIn(options, nil) - wantPaths := []string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret", mcpOverride + ".migrated", zeroDir} + wantPaths := []string{awsDir, gcloudDir, keyFile, oauthDir, mcpDir, zeroDir} for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) @@ -470,43 +475,44 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { // / mcp.ResolveTokenStorePath never expand "~"; they resolve a relative // override literally against the working directory), leaving the real file // unprotected. -func TestCredentialDenyReadPathsInOverrideMatchesStoreResolution(t *testing.T) { +func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { + commandDir := t.TempDir() override := "~/relative-tilde-tokens.json" - options := credentialPathOptions{OAuthTokens: override, MCPOAuthTokens: override} + options := credentialPathOptionsFromEnvironment(commandDir, []string{ + "HOME=", + "USERPROFILE=" + filepath.Join(commandDir, "profile-home"), + "XDG_CONFIG_HOME=~/literal-xdg", + "ZERO_OAUTH_TOKENS_PATH=" + override, + "ZERO_MCP_OAUTH_TOKENS_PATH=mcp/tokens.json", + }) paths := credentialDenyReadPathsIn(options, nil) - cwd, err := os.Getwd() - if err != nil { - t.Fatal(err) + wantHome := filepath.Join(commandDir, "profile-home") + if options.Home != wantHome { + t.Fatalf("home = %q, want USERPROFILE fallback %q", options.Home, wantHome) } - storeResolved := filepath.Clean(filepath.Join(cwd, override)) - if !stringSliceContains(paths, storeResolved) { - t.Errorf("credential deny paths = %#v, want the store's literal resolution %q included", paths, storeResolved) + wantConfig := filepath.Join(commandDir, "~", "literal-xdg") + if options.ZeroConfigDir != wantConfig { + t.Fatalf("config dir = %q, want command-relative literal XDG path %q", options.ZeroConfigDir, wantConfig) } - - home, err := os.UserHomeDir() - if err == nil { - tildeExpanded := normalizeProfilePaths([]string{override})[0] - if tildeExpanded != storeResolved && stringSliceContains(paths, tildeExpanded) { - t.Errorf("credential deny paths = %#v, must not deny the tilde-expanded %q instead of what the store resolves to (home %q)", paths, tildeExpanded, home) + for _, want := range []string{ + filepath.Join(wantConfig, "zero"), + filepath.Join(commandDir, "~"), + filepath.Join(commandDir, "mcp"), + } { + if !stringSliceContains(paths, want) { + t.Errorf("credential deny paths = %#v, want command-relative root %q", paths, want) } } } func TestCredentialDenyReadPathsInConfigDirMatchesLiteralXDGResolution(t *testing.T) { configDir := "~/literal-xdg" - t.Setenv("XDG_CONFIG_HOME", configDir) - resolvedConfigDir, err := zeroCredentialConfigDir() - if err != nil { - t.Fatal(err) - } + commandDir := t.TempDir() + resolvedConfigDir := credentialPathOptionsFromEnvironment(commandDir, []string{"XDG_CONFIG_HOME=" + configDir}).ZeroConfigDir paths := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDir: resolvedConfigDir}, nil) - cwd, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - want := filepath.Join(cwd, configDir, "zero") + want := filepath.Join(commandDir, configDir, "zero") if resolvedConfigDir != filepath.Dir(want) { t.Fatalf("zero credential config dir = %q, want literal XDG resolution %q", resolvedConfigDir, filepath.Dir(want)) } @@ -518,6 +524,37 @@ func TestCredentialDenyReadPathsInConfigDirMatchesLiteralXDGResolution(t *testin } } +func TestBuildCommandPlanUsesCommandCredentialContext(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + workspace := t.TempDir() + commandDir := filepath.Join(workspace, "nested") + if err := os.MkdirAll(commandDir, 0o755); err != nil { + t.Fatal(err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: workspace, + Policy: DefaultPolicy(), + Backend: Backend{Name: BackendUnavailable, Platform: runtime.GOOS}, + }) + plan, err := engine.BuildCommandPlan(CommandSpec{ + Name: "true", + Dir: commandDir, + Env: []string{ + "HOME=" + filepath.Join(workspace, "home"), + "ZERO_OAUTH_TOKENS_PATH=credentials/tokens.json", + }, + }) + if err != nil { + t.Fatal(err) + } + want := filepath.Join(commandDir, "credentials") + if !stringSliceContains(plan.PermissionProfile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want command-relative override parent %q", plan.PermissionProfile.FileSystem.DenyRead, want) + } +} + func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index c883c06e2..d71161965 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -1,7 +1,6 @@ package sandbox import ( - "fmt" "os" "path/filepath" "runtime" @@ -60,8 +59,8 @@ var sandboxFullyProtectedMetadataNames = []string{".zero", ".agents"} // gitMetadataWriteCarveouts returns the .git subpaths that stay write-denied // under the OS-level sandbox even though the rest of .git is writable to git -// subprocesses. Nonexistent paths are harmless no-ops in every backend's -// enforcement (seatbelt regex, bwrap ro-bind, Windows ACL deny entry). +// subprocesses. Backends must either enforce these paths or fail closed when a +// missing target cannot be represented safely. func gitMetadataWriteCarveouts(root string) []string { return []string{ filepath.Join(root, ".git", "hooks"), @@ -74,6 +73,11 @@ func DefaultPermissionProfile(workspaceRoot string) PermissionProfile { } func PermissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Scope) PermissionProfile { + baseDir, _ := os.Getwd() + return permissionProfileFromPolicy(workspaceRoot, policy, scope, baseDir, nil) +} + +func permissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Scope, credentialBaseDir string, credentialEnv []string) PermissionProfile { if policy.Mode == "" { policy = DefaultPolicy() } @@ -90,19 +94,21 @@ func PermissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Sco } readRoots := permissionProfileReadRoots(workspaceRoot, policy, scope, roots) writeRoots := make([]WritableRoot, 0, len(roots)) + tempRoots := defaultTempWriteRoots() for _, root := range roots { - writeRoots = append(writeRoots, WritableRoot{ - Root: root, - ReadOnlySubpaths: gitMetadataWriteCarveouts(root), - ProtectedMetadataNames: append([]string{}, sandboxFullyProtectedMetadataNames...), - }) + writable := WritableRoot{Root: root} + if !profilePathInList(tempRoots, root) { + writable.ReadOnlySubpaths = gitMetadataWriteCarveouts(root) + writable.ProtectedMetadataNames = append([]string{}, sandboxFullyProtectedMetadataNames...) + } + writeRoots = append(writeRoots, writable) } return PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, ReadRoots: readRoots, WriteRoots: writeRoots, - DenyRead: dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy)...)), + DenyRead: dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy, credentialBaseDir, credentialEnv)...)), DenyWrite: normalizeProfilePaths(policy.DenyWrite), IncludePlatformRoots: true, AllowTemp: true, @@ -162,27 +168,66 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // so `allowRead: ["~/.aws"]` remains an explicit opt-out. // - Candidates are emitted whether or not they currently exist on disk so // backends that support future-path rules can protect stores created later. -// The Linux bwrap backend drops missing mount targets because it cannot -// safely create them beneath its read-only root. +// Linux fails closed before launching when a missing path cannot be mounted. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and // must keep reflecting user configuration alone. -func credentialDenyReadPaths(policy Policy) []string { +func credentialDenyReadPaths(policy Policy, baseDir string, commandEnv []string) []string { if runtime.GOOS == "windows" { return nil } - // Failed home/config lookups only drop their derived candidates; explicit - // credential-file overrides are still submitted as candidates regardless. - home, _ := os.UserHomeDir() - configDir, _ := zeroCredentialConfigDir() - return credentialDenyReadPathsIn(credentialPathOptions{ + options := credentialPathOptionsFromEnvironment(baseDir, commandEnv) + return credentialDenyReadPathsIn(options, policy.AllowRead) +} + +func profilePathInList(paths []string, want string) bool { + want = filepath.Clean(want) + for _, path := range paths { + if filepath.Clean(path) == want { + return true + } + } + return false +} + +func credentialPathOptionsFromEnvironment(baseDir string, commandEnv []string) credentialPathOptions { + env := commandEnv + if env == nil { + env = os.Environ() + } + home := strings.TrimSpace(credentialEnvValue(env, "HOME")) + if home == "" { + home = strings.TrimSpace(credentialEnvValue(env, "USERPROFILE")) + } + if home == "" { + home, _ = os.UserHomeDir() + } + home = resolveCredentialOverridePath(home, baseDir) + configDir := strings.TrimSpace(credentialEnvValue(env, "XDG_CONFIG_HOME")) + if configDir == "" && home != "" { + configDir = filepath.Join(home, ".config") + } else { + configDir = resolveCredentialOverridePath(configDir, baseDir) + } + return credentialPathOptions{ Home: home, - GoogleCredentials: os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), + GoogleCredentials: resolveCredentialOverridePath(credentialEnvValue(env, "GOOGLE_APPLICATION_CREDENTIALS"), baseDir), ZeroConfigDir: configDir, - OAuthTokens: os.Getenv("ZERO_OAUTH_TOKENS_PATH"), - MCPOAuthTokens: os.Getenv("ZERO_MCP_OAUTH_TOKENS_PATH"), - }, policy.AllowRead) + OAuthTokens: resolveCredentialOverridePath(credentialEnvValue(env, "ZERO_OAUTH_TOKENS_PATH"), baseDir), + MCPOAuthTokens: resolveCredentialOverridePath(credentialEnvValue(env, "ZERO_MCP_OAUTH_TOKENS_PATH"), baseDir), + } +} + +func credentialEnvValue(env []string, key string) string { + value := "" + for _, entry := range env { + name, candidate, ok := strings.Cut(entry, "=") + if ok && name == key { + value = candidate + } + } + return value } type credentialPathOptions struct { @@ -207,7 +252,7 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string if target := strings.TrimSpace(options.GoogleCredentials); target != "" { candidates = append(candidates, target) } - if configDir := resolveCredentialOverridePath(options.ZeroConfigDir); configDir != "" { + if configDir := strings.TrimSpace(options.ZeroConfigDir); configDir != "" { // Deny the whole directory rather than an itemized file list. Zero's // credential/token/config stores each publish through a randomly-named // sibling before an atomic rename (oauth-tokens.json.tmp--, @@ -217,11 +262,14 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string // with those names. Nothing else has a legitimate reason to live here. candidates = append(candidates, filepath.Join(configDir, "zero")) } - if tokenPath := resolveCredentialOverridePath(options.OAuthTokens); tokenPath != "" { - candidates = append(candidates, tokenPath, tokenPath+".secret") + if tokenPath := strings.TrimSpace(options.OAuthTokens); tokenPath != "" { + // File and key writes use random atomic-publication siblings. Protecting + // the configured parent is the only exact-path rule that covers every + // sibling without a race; callers should use a credential-dedicated dir. + candidates = append(candidates, filepath.Dir(tokenPath)) } - if tokenPath := resolveCredentialOverridePath(options.MCPOAuthTokens); tokenPath != "" { - candidates = append(candidates, tokenPath, tokenPath+".secret", tokenPath+".migrated") + if tokenPath := strings.TrimSpace(options.MCPOAuthTokens); tokenPath != "" { + candidates = append(candidates, filepath.Dir(tokenPath)) } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) @@ -250,48 +298,23 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string // where the store actually writes — e.g. ZERO_OAUTH_TOKENS_PATH=~/x resolves // to /~/x on disk (the store never expands "~"), but normalizeProfilePath // would deny $HOME/x instead, leaving the real file unprotected. -func resolveCredentialOverridePath(override string) string { +func resolveCredentialOverridePath(override string, baseDir string) string { override = strings.TrimSpace(override) if override == "" { return "" } - abs, err := filepath.Abs(override) - if err != nil { - return "" + if filepath.IsAbs(override) { + return filepath.Clean(override) } - return abs -} - -// zeroCredentialConfigDir follows the OAuth stores' literal XDG resolution. -// In particular, "~/config" is relative syntax to those stores, not a request -// for shell-style home expansion. -func zeroCredentialConfigDir() (string, error) { - if xdg := strings.TrimSpace(os.Getenv("XDG_CONFIG_HOME")); xdg != "" { - if resolved := resolveCredentialOverridePath(xdg); resolved != "" { - return resolved, nil + baseDir = strings.TrimSpace(baseDir) + if baseDir == "" { + var err error + baseDir, err = os.Getwd() + if err != nil { + return "" } - return "", fmt.Errorf("resolve XDG_CONFIG_HOME %q", xdg) - } - return zeroUserConfigDir() -} - -// zeroUserConfigDir mirrors config.UserConfigDir without importing config -// (config depends on sandbox). On macOS Zero deliberately honors -// XDG_CONFIG_HOME when set and falls back to ~/.config, instead of the -// os.UserConfigDir default (~/Library/Application Support); everywhere else -// it uses os.UserConfigDir. -func zeroUserConfigDir() (string, error) { - if runtime.GOOS != "darwin" { - return os.UserConfigDir() - } - if xdg := strings.TrimSpace(os.Getenv("XDG_CONFIG_HOME")); xdg != "" { - return xdg, nil - } - home, err := os.UserHomeDir() - if err != nil { - return "", err } - return filepath.Join(home, ".config"), nil + return filepath.Clean(filepath.Join(baseDir, override)) } // userGitConfigReadPaths returns the user's global git config FILES so a diff --git a/internal/sandbox/profile_test.go b/internal/sandbox/profile_test.go index ffa3ba19a..04243992c 100644 --- a/internal/sandbox/profile_test.go +++ b/internal/sandbox/profile_test.go @@ -6,57 +6,109 @@ import ( "runtime" "testing" - "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/mcp" + "github.com/Gitlawb/zero/internal/oauth" "github.com/Gitlawb/zero/internal/sandbox" ) -// TestZeroUserConfigDirMatchesConfigUserConfigDir prevents silent drift -// between sandbox.zeroUserConfigDir and config.UserConfigDir. The sandbox copy -// exists only to avoid an import cycle (config already depends on sandbox); if -// the two diverge, deny rules would target a different directory than the -// stores write to. -func TestZeroUserConfigDirMatchesConfigUserConfigDir(t *testing.T) { - t.Run("default", func(t *testing.T) { - assertUserConfigDirParity(t) +func TestCredentialDeniesMatchTokenStoreFallbacks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + workspace := t.TempDir() + profileHome := filepath.Join(workspace, "profile-home") + envMap := map[string]string{"HOME": "", "USERPROFILE": profileHome, "XDG_CONFIG_HOME": ""} + oauthPath, err := oauth.ResolveStorePath(envMap) + if err != nil { + t.Fatal(err) + } + mcpPath, err := mcp.ResolveTokenStorePath(envMap) + if err != nil { + t.Fatal(err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: workspace, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Platform: runtime.GOOS}, }) - - t.Run("xdg_override", func(t *testing.T) { - xdg := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", xdg) - assertUserConfigDirParity(t) + plan, err := engine.BuildCommandPlan(sandbox.CommandSpec{ + Name: "true", + Dir: workspace, + Env: []string{"HOME=", "USERPROFILE=" + profileHome, "XDG_CONFIG_HOME="}, }) - - t.Run("xdg_cleared", func(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", "") - assertUserConfigDirParity(t) - if runtime.GOOS == "darwin" { - home, err := os.UserHomeDir() - if err != nil { - t.Fatal(err) - } - want := filepath.Join(home, ".config") - got, err := sandbox.ZeroUserConfigDir() - if err != nil { - t.Fatal(err) - } - if got != want { - t.Fatalf("zeroUserConfigDir() = %q, want macOS ~/.config fallback %q", got, want) - } + if err != nil { + t.Fatal(err) + } + for _, storePath := range []string{oauthPath, mcpPath} { + want := filepath.Dir(storePath) + if !containsPath(plan.PermissionProfile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want token-store root %q", plan.PermissionProfile.FileSystem.DenyRead, want) } - }) + } } -func assertUserConfigDirParity(t *testing.T) { - t.Helper() - want, err := config.UserConfigDir() +func TestCredentialDeniesMatchRelativeTokenOverridesAtCommandDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + workspace := t.TempDir() + commandDir := filepath.Join(workspace, "nested") + if err := os.MkdirAll(commandDir, 0o755); err != nil { + t.Fatal(err) + } + originalDir, err := os.Getwd() if err != nil { - t.Fatalf("config.UserConfigDir: %v", err) + t.Fatal(err) + } + if err := os.Chdir(commandDir); err != nil { + t.Fatal(err) } - got, err := sandbox.ZeroUserConfigDir() + defer func() { _ = os.Chdir(originalDir) }() + + envMap := map[string]string{ + "HOME": filepath.Join(workspace, "home"), + "ZERO_OAUTH_TOKENS_PATH": "oauth/tokens.json", + "ZERO_MCP_OAUTH_TOKENS_PATH": "mcp/tokens.json", + } + oauthPath, err := oauth.ResolveStorePath(envMap) + if err != nil { + t.Fatal(err) + } + mcpPath, err := mcp.ResolveTokenStorePath(envMap) if err != nil { - t.Fatalf("sandbox.ZeroUserConfigDir: %v", err) + t.Fatal(err) } - if got != want { - t.Fatalf("sandbox.ZeroUserConfigDir() = %q, config.UserConfigDir() = %q", got, want) + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: workspace, + Policy: sandbox.DefaultPolicy(), + Backend: sandbox.Backend{Name: sandbox.BackendUnavailable, Platform: runtime.GOOS}, + }) + plan, err := engine.BuildCommandPlan(sandbox.CommandSpec{ + Name: "true", + Dir: commandDir, + Env: []string{ + "HOME=" + envMap["HOME"], + "ZERO_OAUTH_TOKENS_PATH=" + envMap["ZERO_OAUTH_TOKENS_PATH"], + "ZERO_MCP_OAUTH_TOKENS_PATH=" + envMap["ZERO_MCP_OAUTH_TOKENS_PATH"], + }, + }) + if err != nil { + t.Fatal(err) + } + for _, storePath := range []string{oauthPath, mcpPath} { + want := filepath.Dir(storePath) + if !containsPath(plan.PermissionProfile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want override publication root %q", plan.PermissionProfile.FileSystem.DenyRead, want) + } + } +} + +func containsPath(paths []string, want string) bool { + want = filepath.Clean(want) + for _, path := range paths { + if filepath.Clean(path) == want { + return true + } } + return false } diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 4bcf83808..c20f2d32e 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -139,7 +139,7 @@ func (engine *Engine) BuildCommandPlan(spec CommandSpec) (CommandPlan, error) { if policy.Mode == ModeDisabled { preference = SandboxPreferenceForbid } - profile := PermissionProfileFromPolicy(workspaceRoot, policy, engine.scope) + profile := permissionProfileFromPolicy(workspaceRoot, policy, engine.scope, spec.Dir, spec.Env) manager := NewSandboxManager(SandboxManagerOptions{ GOOS: backend.Platform, Backend: backend, @@ -340,6 +340,7 @@ func seatbeltCommandPlanWithProfile(spec CommandSpec, workspaceRoot string, prof } func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) PermissionProfile { + credentialBaseDir, _ := os.Getwd() fs := FileSystemPolicy{ Kind: FileSystemUnrestricted, ReadRoots: []string{string(filepath.Separator)}, @@ -353,7 +354,7 @@ func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) fs.WriteRoots = append(fs.WriteRoots, WritableRoot{Root: root}) } } - fs.DenyRead = dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy)...)) + fs.DenyRead = dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy, credentialBaseDir, nil)...)) fs.DenyWrite = normalizeProfilePaths(policy.DenyWrite) return PermissionProfile{ FileSystem: fs, diff --git a/internal/sandbox/runner_linux_integration_test.go b/internal/sandbox/runner_linux_integration_test.go index 1bf5ccab5..76fea601a 100644 --- a/internal/sandbox/runner_linux_integration_test.go +++ b/internal/sandbox/runner_linux_integration_test.go @@ -23,12 +23,31 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { t.Skipf("Linux sandbox backend unavailable: %s", backend.Message) } root := t.TempDir() - if err := os.Mkdir(filepath.Join(root, ".git"), 0o755); err != nil { - t.Fatalf("Mkdir .git: %v", err) + if err := os.MkdirAll(filepath.Join(root, ".git", "hooks"), 0o755); err != nil { + t.Fatalf("Mkdir .git/hooks: %v", err) + } + for _, name := range []string{".zero", ".agents"} { + if err := os.Mkdir(filepath.Join(root, name), 0o755); err != nil { + t.Fatalf("Mkdir %s: %v", name, err) + } } if err := os.WriteFile(filepath.Join(root, ".git", "config"), []byte("[core]\n"), 0o644); err != nil { t.Fatalf("WriteFile .git/config: %v", err) } + credentialHome := t.TempDir() + configHome := filepath.Join(credentialHome, "config") + for _, path := range []string{ + filepath.Join(credentialHome, ".aws"), + filepath.Join(credentialHome, ".config", "gcloud"), + filepath.Join(credentialHome, ".azure"), + filepath.Join(configHome, "zero"), + } { + if err := os.MkdirAll(path, 0o700); err != nil { + t.Fatalf("Mkdir credential path: %v", err) + } + } + t.Setenv("HOME", credentialHome) + t.Setenv("XDG_CONFIG_HOME", configHome) secretDir := t.TempDir() if err := os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("hidden\n"), 0o644); err != nil { t.Fatalf("WriteFile secret: %v", err) @@ -38,13 +57,8 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { t.Fatalf("Mkdir blocked: %v", err) } - missingDenied := fmt.Sprintf("/etc/zero-sandbox-missing-%d-%d/nested", os.Getpid(), time.Now().UnixNano()) - if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("missing deny-path precondition failed: %v", err) - } - policy := DefaultPolicy() - policy.DenyRead = []string{secretDir, missingDenied} + policy.DenyRead = []string{secretDir} policy.DenyWrite = []string{blockedDir} engine := NewEngine(EngineOptions{WorkspaceRoot: root, Policy: policy, Backend: backend}) output, runErr := runLinuxSandboxSmokeCommand(t, engine, CommandSpec{ @@ -65,9 +79,31 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { } t.Fatalf("allowed smoke command failed: %v\n%s", runErr, output) } - if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("sandbox launch materialized missing deny path on host: %v", err) - } + + t.Run("missing protected path fails closed", func(t *testing.T) { + missingDenied := fmt.Sprintf("/etc/zero-sandbox-missing-%d-%d/nested", os.Getpid(), time.Now().UnixNano()) + if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing deny-path precondition failed: %v", err) + } + marker := filepath.Join(root, "missing-path-command-ran") + failPolicy := DefaultPolicy() + failPolicy.DenyRead = []string{missingDenied} + failEngine := NewEngine(EngineOptions{WorkspaceRoot: root, Policy: failPolicy, Backend: backend}) + output, runErr := runLinuxSandboxSmokeCommand(t, failEngine, CommandSpec{ + Name: "/bin/sh", + Args: []string{"-c", "touch " + shellQuote(marker)}, + Dir: root, + }) + if runErr == nil || !strings.Contains(string(output), missingDenied) { + t.Fatalf("missing protected path did not fail closed: err=%v output=%s", runErr, output) + } + if _, err := os.Stat(marker); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("target command ran despite unenforceable path: %v", err) + } + if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("sandbox launch materialized missing deny path on host: %v", err) + } + }) for _, tc := range []struct { name string diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 5e7a5b256..c65f45630 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -609,6 +609,23 @@ func TestResolveCommandDirAllowsExtraRootCwd(t *testing.T) { func TestLinuxHelperPlanPreservesRealExtraRootCwd(t *testing.T) { workspace := t.TempDir() extra := tempDirOutsideDefaultTemp(t) + for _, root := range []string{workspace, extra} { + for _, dir := range []string{filepath.Join(root, ".git", "hooks"), filepath.Join(root, ".zero"), filepath.Join(root, ".agents")} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(root, ".git", "config"), nil, 0o644); err != nil { + t.Fatal(err) + } + } + credentialHome := filepath.Join(workspace, "credential-home") + configHome := filepath.Join(credentialHome, "config") + for _, dir := range []string{filepath.Join(credentialHome, ".aws"), filepath.Join(credentialHome, ".config", "gcloud"), filepath.Join(credentialHome, ".azure"), filepath.Join(configHome, "zero")} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } scope, err := NewScope(workspace, []string{extra}) if err != nil { t.Fatalf("NewScope: %v", err) @@ -620,7 +637,7 @@ func TestLinuxHelperPlanPreservesRealExtraRootCwd(t *testing.T) { Backend: Backend{Name: BackendLinuxBwrap, Available: true, Executable: "/usr/bin/zero-linux-sandbox"}, }) resolvedExtra := scope.Roots()[1] - plan, err := engine.BuildCommandPlan(CommandSpec{Name: "true", Dir: extra}) + plan, err := engine.BuildCommandPlan(CommandSpec{Name: "true", Dir: extra, Env: []string{"HOME=" + credentialHome, "XDG_CONFIG_HOME=" + configHome}}) if err != nil { t.Fatalf("BuildCommandPlan: %v", err) } From 5b4c21602dd1c2538ddfe6207ae6f7cc9f04b667 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 18 Jul 2026 23:55:11 +0200 Subject: [PATCH 08/18] test(sandbox): normalize command credential path --- internal/sandbox/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index a76be7c6f..142d3afb7 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -549,7 +549,7 @@ func TestBuildCommandPlanUsesCommandCredentialContext(t *testing.T) { if err != nil { t.Fatal(err) } - want := filepath.Join(commandDir, "credentials") + want := filepath.Join(plan.Dir, "credentials") if !stringSliceContains(plan.PermissionProfile.FileSystem.DenyRead, want) { t.Fatalf("DenyRead = %#v, want command-relative override parent %q", plan.PermissionProfile.FileSystem.DenyRead, want) } From 9f8147e2c0f60f2074fa39533288adb820e05355 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 19 Jul 2026 11:04:27 +0200 Subject: [PATCH 09/18] fix(sandbox): handle absent credential paths safely --- internal/cli/sandbox_test.go | 4 +-- internal/oauth/encrypt.go | 6 ++-- internal/oauth/store.go | 17 +++++++-- internal/oauth/store_test.go | 33 +++++++++++++++++ internal/sandbox/landlock_linux.go | 2 +- internal/sandbox/linux_helper.go | 19 +++++++++- internal/sandbox/linux_helper_test.go | 30 ++++++++++++++-- internal/sandbox/manager_test.go | 35 +++++++++++++------ internal/sandbox/profile.go | 31 ++++++++++------ internal/sandbox/profile_test.go | 9 +++-- internal/sandbox/runner.go | 5 +-- .../sandbox/runner_linux_integration_test.go | 20 ++++++++--- 12 files changed, 165 insertions(+), 46 deletions(-) diff --git a/internal/cli/sandbox_test.go b/internal/cli/sandbox_test.go index c65c1015e..1c58b878d 100644 --- a/internal/cli/sandbox_test.go +++ b/internal/cli/sandbox_test.go @@ -561,10 +561,10 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp filepath.Join(emptyHome, ".config", "zero"), } } - if gotDenyRead := jsonStringSlice(fileSystem["denyRead"]); !reflect.DeepEqual(gotDenyRead, wantDenyRead) { + if gotDenyRead := jsonStringSlice(fileSystem["denyReadIfExists"]); !reflect.DeepEqual(gotDenyRead, wantDenyRead) { t.Fatalf("manager credential deny baseline = %#v, want %#v", gotDenyRead, wantDenyRead) } - delete(fileSystem, "denyRead") + delete(fileSystem, "denyReadIfExists") fileSystem["readRoots"] = filterJSONStringRoots(fileSystem["readRoots"], tempRoots) fileSystem["writeRoots"] = filterJSONWriteRoots(fileSystem["writeRoots"], tempRoots) normalized, err := json.MarshalIndent(value, "", " ") diff --git a/internal/oauth/encrypt.go b/internal/oauth/encrypt.go index ad1a6ddee..3e9502dcb 100644 --- a/internal/oauth/encrypt.go +++ b/internal/oauth/encrypt.go @@ -140,12 +140,12 @@ func writeNewSecretFile(path string) ([]byte, error) { if _, err := io.ReadFull(rand.Reader, secret); err != nil { return nil, fmt.Errorf("oauth: generate token secret: %w", err) } - dir := filepath.Dir(path) - tmp, err := os.CreateTemp(dir, filepath.Base(path)+".*.tmp") + tmpPath := path + ".tmp" + _ = os.Remove(tmpPath) + tmp, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) if err != nil { return nil, fmt.Errorf("oauth: create token secret temp file: %w", err) } - tmpPath := tmp.Name() defer os.Remove(tmpPath) if err := tmp.Chmod(0o600); err != nil { _ = tmp.Close() diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 951e9616f..a02fb24ba 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -409,8 +409,21 @@ func (b fileBlob) write(data []byte) error { if err := os.MkdirAll(filepath.Dir(b.path), 0o700); err != nil { return err } - tempPath := fmt.Sprintf("%s.tmp-%d-%d", b.path, os.Getpid(), time.Now().UnixNano()) - if err := os.WriteFile(tempPath, data, 0o600); err != nil { + // Saves are serialized by withLock, so a fixed sibling remains collision-free + // while allowing sandbox profiles to protect it without masking the parent. + tempPath := b.path + ".tmp" + _ = os.Remove(tempPath) + temp, err := os.OpenFile(tempPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + return err + } + if _, err := temp.Write(data); err != nil { + _ = temp.Close() + _ = os.Remove(tempPath) + return err + } + if err := temp.Close(); err != nil { + _ = os.Remove(tempPath) return err } if err := os.Rename(tempPath, b.path); err != nil { diff --git a/internal/oauth/store_test.go b/internal/oauth/store_test.go index d160f5a7e..bc79d5bb9 100644 --- a/internal/oauth/store_test.go +++ b/internal/oauth/store_test.go @@ -1,6 +1,7 @@ package oauth import ( + "errors" "os" "path/filepath" "runtime" @@ -91,6 +92,38 @@ func TestStoreFileMode0600(t *testing.T) { } } +func TestStoreUsesFixedProtectedPublicationSibling(t *testing.T) { + s, path := newTestStore(t) + tempPath := path + ".tmp" + if err := os.WriteFile(tempPath, []byte("stale"), 0o600); err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("x"), Token{AccessToken: "secret"}); err != nil { + t.Fatalf("Save: %v", err) + } + if _, err := os.Stat(tempPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("publication sibling remains after save: %v", err) + } +} + +func TestEncryptedStoreUsesFixedProtectedSecretSibling(t *testing.T) { + path := filepath.Join(t.TempDir(), "tokens.json") + secretTempPath := path + ".secret.tmp" + if err := os.WriteFile(secretTempPath, []byte("stale"), 0o600); err != nil { + t.Fatal(err) + } + s, err := NewStore(StoreOptions{FilePath: path, Storage: "encrypted-file"}) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + if err := s.Save(ProviderKey("x"), Token{AccessToken: "secret"}); err != nil { + t.Fatalf("Save: %v", err) + } + if _, err := os.Stat(secretTempPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("secret publication sibling remains after save: %v", err) + } +} + func TestStoreMalformedFailsClosed(t *testing.T) { s, path := newTestStore(t) if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil { diff --git a/internal/sandbox/landlock_linux.go b/internal/sandbox/landlock_linux.go index 1d51d6a35..1796373c8 100644 --- a/internal/sandbox/landlock_linux.go +++ b/internal/sandbox/landlock_linux.go @@ -48,7 +48,7 @@ func validateLandlockProfile(profile PermissionProfile) error { if fs.Kind != FileSystemRestricted { return nil } - if len(fs.DenyRead) > 0 { + if len(fs.DenyRead) > 0 || len(fs.DenyReadIfExists) > 0 { return errors.New("deny-read paths require the bubblewrap helper mode") } if len(fs.DenyWrite) > 0 { diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 92c6d1af3..7655a97c3 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -232,6 +232,9 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { } args = append(args, "--bind", root.Root, root.Root) for _, subpath := range root.ReadOnlySubpaths { + if !pathExists(subpath) { + continue + } var err error args, err = appendReadOnlyLinuxPathArgs(args, subpath) if err != nil { @@ -239,8 +242,12 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { } } for _, name := range root.ProtectedMetadataNames { + path := filepath.Join(root.Root, name) + if !pathExists(path) { + continue + } var err error - args, err = appendReadOnlyLinuxPathArgs(args, filepath.Join(root.Root, name)) + args, err = appendReadOnlyLinuxPathArgs(args, path) if err != nil { return nil, err } @@ -260,6 +267,16 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { return nil, err } } + for _, path := range fs.DenyReadIfExists { + if !pathExists(path) { + continue + } + var err error + args, err = appendUnreadableLinuxPathArgs(args, path) + if err != nil { + return nil, err + } + } return args, nil } diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index 95261d870..17108a8a9 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -246,6 +246,33 @@ func TestLinuxBwrapPathCarveoutsFailClosedForMissingMountTargets(t *testing.T) { assertArgsContainSequence(t, args, "--ro-bind", deniedFile, deniedFile) } +func TestLinuxBwrapSkipsMissingAutomaticBaselines(t *testing.T) { + root := t.TempDir() + missingCredential := filepath.Join(root, "home", ".config", "zero") + profile := PermissionProfile{FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: root, ReadOnlySubpaths: []string{filepath.Join(root, ".git", "hooks")}, ProtectedMetadataNames: []string{".zero"}}}, + DenyReadIfExists: []string{missingCredential}, + }} + + args, err := linuxBwrapFilesystemArgs(profile) + if err != nil { + t.Fatalf("missing automatic baselines must not abort launch: %v", err) + } + if stringSliceContains(args, missingCredential) { + t.Fatalf("missing automatic baseline unexpectedly emitted: %#v", args) + } + if err := os.MkdirAll(missingCredential, 0o700); err != nil { + t.Fatal(err) + } + args, err = linuxBwrapFilesystemArgs(profile) + if err != nil { + t.Fatal(err) + } + assertArgsContainSequence(t, args, "--perms", "000", "--tmpfs", missingCredential, "--remount-ro", missingCredential) +} + func TestLinuxBwrapTempUsesHostWriteRoots(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Linux bwrap temp root assertions use Unix paths") @@ -256,9 +283,6 @@ func TestLinuxBwrapTempUsesHostWriteRoots(t *testing.T) { if err := os.MkdirAll(workspace, 0o755); err != nil { t.Fatalf("MkdirAll workspace: %v", err) } - if err := os.Mkdir(filepath.Join(workspace, ".git"), 0o755); err != nil { - t.Fatalf("Mkdir .git: %v", err) - } profile := PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 142d3afb7..05048284b 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -390,6 +390,14 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } oauthOverride := filepath.Join(oauthDir, "tokens.json") mcpOverride := filepath.Join(mcpDir, "tokens.json") + overrideFiles := []string{ + oauthOverride, + oauthOverride + ".tmp", + oauthOverride + ".secret", + oauthOverride + ".secret.tmp", + mcpOverride, + mcpOverride + ".migrated", + } // The migrated legacy MCP token backup and the atomic-write temp siblings // every store publishes before its rename; none of these are itemized by // name, so they only stay protected if the whole zeroDir is denied. @@ -407,7 +415,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { filepath.Join(zeroDir, "credentials.enc.9-1.tmp"), filepath.Join(zeroDir, ".zero-config-1.tmp"), } - for _, path := range append([]string{keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) { + for _, path := range append(append([]string{keyFile}, overrideFiles...), zeroFiles...) { if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { t.Fatal(err) } @@ -421,7 +429,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { MCPOAuthTokens: mcpOverride, } paths := credentialDenyReadPathsIn(options, nil) - wantPaths := []string{awsDir, gcloudDir, keyFile, oauthDir, mcpDir, zeroDir} + wantPaths := append([]string{awsDir, gcloudDir, keyFile, zeroDir}, overrideFiles...) for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) @@ -497,8 +505,10 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { } for _, want := range []string{ filepath.Join(wantConfig, "zero"), - filepath.Join(commandDir, "~"), - filepath.Join(commandDir, "mcp"), + filepath.Join(commandDir, override), + filepath.Join(commandDir, override) + ".tmp", + filepath.Join(commandDir, "mcp", "tokens.json"), + filepath.Join(commandDir, "mcp", "tokens.json.migrated"), } { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want command-relative root %q", paths, want) @@ -549,9 +559,12 @@ func TestBuildCommandPlanUsesCommandCredentialContext(t *testing.T) { if err != nil { t.Fatal(err) } - want := filepath.Join(plan.Dir, "credentials") - if !stringSliceContains(plan.PermissionProfile.FileSystem.DenyRead, want) { - t.Fatalf("DenyRead = %#v, want command-relative override parent %q", plan.PermissionProfile.FileSystem.DenyRead, want) + want := filepath.Join(plan.Dir, "credentials", "tokens.json") + if !stringSliceContains(plan.PermissionProfile.FileSystem.DenyReadIfExists, want) { + t.Fatalf("DenyReadIfExists = %#v, want command-relative override %q", plan.PermissionProfile.FileSystem.DenyReadIfExists, want) + } + if stringSliceContains(plan.PermissionProfile.FileSystem.DenyReadIfExists, filepath.Dir(want)) { + t.Fatalf("DenyReadIfExists = %#v, must not mask override parent %q", plan.PermissionProfile.FileSystem.DenyReadIfExists, filepath.Dir(want)) } } @@ -571,8 +584,8 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { // credentials created later, not just ones present at profile-build time. profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) want := normalizeProfilePaths([]string{zeroDir})[0] - if !stringSliceContains(profile.FileSystem.DenyRead, want) { - t.Fatalf("DenyRead = %#v, want Zero config directory %q even before it exists", profile.FileSystem.DenyRead, want) + if !stringSliceContains(profile.FileSystem.DenyReadIfExists, want) { + t.Fatalf("DenyReadIfExists = %#v, want Zero config directory %q even before it exists", profile.FileSystem.DenyReadIfExists, want) } if err := os.MkdirAll(zeroDir, 0o700); err != nil { @@ -594,7 +607,7 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { // against a post-mkdir /private/var form). profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) want = normalizeProfilePaths([]string{zeroDir})[0] - if !stringSliceContains(profile.FileSystem.DenyRead, want) { - t.Fatalf("DenyRead = %#v, want Zero config directory %q", profile.FileSystem.DenyRead, want) + if !stringSliceContains(profile.FileSystem.DenyReadIfExists, want) { + t.Fatalf("DenyReadIfExists = %#v, want Zero config directory %q", profile.FileSystem.DenyReadIfExists, want) } } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index d71161965..14b5815f3 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -25,6 +25,7 @@ type FileSystemPolicy struct { ReadRoots []string `json:"readRoots,omitempty"` WriteRoots []WritableRoot `json:"writeRoots,omitempty"` DenyRead []string `json:"denyRead,omitempty"` + DenyReadIfExists []string `json:"denyReadIfExists,omitempty"` DenyWrite []string `json:"denyWrite,omitempty"` IncludePlatformRoots bool `json:"includePlatformRoots,omitempty"` AllowTemp bool `json:"allowTemp,omitempty"` @@ -59,8 +60,8 @@ var sandboxFullyProtectedMetadataNames = []string{".zero", ".agents"} // gitMetadataWriteCarveouts returns the .git subpaths that stay write-denied // under the OS-level sandbox even though the rest of .git is writable to git -// subprocesses. Backends must either enforce these paths or fail closed when a -// missing target cannot be represented safely. +// subprocesses. Backends enforce paths that exist when the sandbox starts; +// unlike explicit deny rules, absent baseline metadata must not prevent launch. func gitMetadataWriteCarveouts(root string) []string { return []string{ filepath.Join(root, ".git", "hooks"), @@ -108,7 +109,8 @@ func permissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Sco Kind: FileSystemRestricted, ReadRoots: readRoots, WriteRoots: writeRoots, - DenyRead: dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy, credentialBaseDir, credentialEnv)...)), + DenyRead: normalizeProfilePaths(policy.DenyRead), + DenyReadIfExists: credentialDenyReadPaths(policy, credentialBaseDir, credentialEnv), DenyWrite: normalizeProfilePaths(policy.DenyWrite), IncludePlatformRoots: true, AllowTemp: true, @@ -166,9 +168,9 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // 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. -// - Candidates are emitted whether or not they currently exist on disk so -// backends that support future-path rules can protect stores created later. -// Linux fails closed before launching when a missing path cannot be mounted. +// - Candidates are emitted whether or not they currently exist on disk. +// Backends that support future-path rules enforce them immediately; Linux +// enforces the baseline paths that exist without making fresh homes unusable. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -263,13 +265,20 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string candidates = append(candidates, filepath.Join(configDir, "zero")) } if tokenPath := strings.TrimSpace(options.OAuthTokens); tokenPath != "" { - // File and key writes use random atomic-publication siblings. Protecting - // the configured parent is the only exact-path rule that covers every - // sibling without a race; callers should use a credential-dedicated dir. - candidates = append(candidates, filepath.Dir(tokenPath)) + // OAuth uses fixed same-directory publication paths so exact denies do not + // have to hide an arbitrary parent such as the workspace or /tmp. + candidates = append(candidates, + tokenPath, + tokenPath+".tmp", + tokenPath+".secret", + tokenPath+".secret.tmp", + ) } if tokenPath := strings.TrimSpace(options.MCPOAuthTokens); tokenPath != "" { - candidates = append(candidates, filepath.Dir(tokenPath)) + candidates = append(candidates, + tokenPath, + tokenPath+".migrated", + ) } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) diff --git a/internal/sandbox/profile_test.go b/internal/sandbox/profile_test.go index 04243992c..12b2d67c2 100644 --- a/internal/sandbox/profile_test.go +++ b/internal/sandbox/profile_test.go @@ -41,8 +41,8 @@ func TestCredentialDeniesMatchTokenStoreFallbacks(t *testing.T) { } for _, storePath := range []string{oauthPath, mcpPath} { want := filepath.Dir(storePath) - if !containsPath(plan.PermissionProfile.FileSystem.DenyRead, want) { - t.Fatalf("DenyRead = %#v, want token-store root %q", plan.PermissionProfile.FileSystem.DenyRead, want) + if !containsPath(plan.PermissionProfile.FileSystem.DenyReadIfExists, want) { + t.Fatalf("DenyReadIfExists = %#v, want token-store root %q", plan.PermissionProfile.FileSystem.DenyReadIfExists, want) } } } @@ -96,9 +96,8 @@ func TestCredentialDeniesMatchRelativeTokenOverridesAtCommandDir(t *testing.T) { t.Fatal(err) } for _, storePath := range []string{oauthPath, mcpPath} { - want := filepath.Dir(storePath) - if !containsPath(plan.PermissionProfile.FileSystem.DenyRead, want) { - t.Fatalf("DenyRead = %#v, want override publication root %q", plan.PermissionProfile.FileSystem.DenyRead, want) + if !containsPath(plan.PermissionProfile.FileSystem.DenyReadIfExists, storePath) { + t.Fatalf("DenyReadIfExists = %#v, want override path %q", plan.PermissionProfile.FileSystem.DenyReadIfExists, storePath) } } } diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index c20f2d32e..d3e4310b8 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -354,7 +354,8 @@ func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) fs.WriteRoots = append(fs.WriteRoots, WritableRoot{Root: root}) } } - fs.DenyRead = dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy, credentialBaseDir, nil)...)) + fs.DenyRead = normalizeProfilePaths(policy.DenyRead) + fs.DenyReadIfExists = credentialDenyReadPaths(policy, credentialBaseDir, nil) fs.DenyWrite = normalizeProfilePaths(policy.DenyWrite) return PermissionProfile{ FileSystem: fs, @@ -734,7 +735,7 @@ func seatbeltProtectedMetadataRegex(root string, name string) string { } func denyReadRules(fs FileSystemPolicy) []string { - return denySeatbeltPathRules("file-read*", fs.DenyRead) + return denySeatbeltPathRules("file-read*", dedupeStrings(append(append([]string{}, fs.DenyRead...), fs.DenyReadIfExists...))) } func writeRootCarveoutDenyRules(fs FileSystemPolicy) []string { diff --git a/internal/sandbox/runner_linux_integration_test.go b/internal/sandbox/runner_linux_integration_test.go index 76fea601a..69af36af8 100644 --- a/internal/sandbox/runner_linux_integration_test.go +++ b/internal/sandbox/runner_linux_integration_test.go @@ -26,11 +26,6 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { if err := os.MkdirAll(filepath.Join(root, ".git", "hooks"), 0o755); err != nil { t.Fatalf("Mkdir .git/hooks: %v", err) } - for _, name := range []string{".zero", ".agents"} { - if err := os.Mkdir(filepath.Join(root, name), 0o755); err != nil { - t.Fatalf("Mkdir %s: %v", name, err) - } - } if err := os.WriteFile(filepath.Join(root, ".git", "config"), []byte("[core]\n"), 0o644); err != nil { t.Fatalf("WriteFile .git/config: %v", err) } @@ -80,6 +75,21 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { t.Fatalf("allowed smoke command failed: %v\n%s", runErr, output) } + t.Run("fresh home and non-git workspace launch", func(t *testing.T) { + freshRoot := t.TempDir() + freshHome := t.TempDir() + freshEngine := NewEngine(EngineOptions{WorkspaceRoot: freshRoot, Policy: DefaultPolicy(), Backend: backend}) + output, runErr := runLinuxSandboxSmokeCommand(t, freshEngine, CommandSpec{ + Name: "/bin/sh", + Args: []string{"-c", "echo ok > launched"}, + Dir: freshRoot, + Env: []string{"HOME=" + freshHome, "XDG_CONFIG_HOME=" + filepath.Join(freshHome, ".config")}, + }) + if runErr != nil { + t.Fatalf("fresh environment failed to launch: %v\n%s", runErr, output) + } + }) + t.Run("missing protected path fails closed", func(t *testing.T) { missingDenied := fmt.Sprintf("/etc/zero-sandbox-missing-%d-%d/nested", os.Getpid(), time.Now().UnixNano()) if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { From b8e7a994db9ca94d9ced818b83d569e09585c1a7 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 19 Jul 2026 16:08:59 +0200 Subject: [PATCH 10/18] fix(sandbox): protect MCP OAuth siblings --- internal/oauth/store.go | 9 ++------ internal/sandbox/manager_test.go | 6 ++++++ internal/sandbox/profile.go | 3 +++ internal/sandbox/runner.go | 2 +- .../sandbox/runner_linux_integration_test.go | 3 +-- internal/sandbox/runner_test.go | 21 +++++++++++++++++++ 6 files changed, 34 insertions(+), 10 deletions(-) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index a02fb24ba..503363a5a 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -417,20 +417,15 @@ func (b fileBlob) write(data []byte) error { if err != nil { return err } + defer os.Remove(tempPath) if _, err := temp.Write(data); err != nil { _ = temp.Close() - _ = os.Remove(tempPath) return err } if err := temp.Close(); err != nil { - _ = os.Remove(tempPath) return err } - if err := os.Rename(tempPath, b.path); err != nil { - _ = os.Remove(tempPath) - return err - } - return nil + return os.Rename(tempPath, b.path) } func (b fileBlob) withLock(now func() time.Time, fn func() error) error { diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 05048284b..0db68ca3f 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -396,6 +396,9 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { oauthOverride + ".secret", oauthOverride + ".secret.tmp", mcpOverride, + mcpOverride + ".tmp", + mcpOverride + ".secret", + mcpOverride + ".secret.tmp", mcpOverride + ".migrated", } // The migrated legacy MCP token backup and the atomic-write temp siblings @@ -508,6 +511,9 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { filepath.Join(commandDir, override), filepath.Join(commandDir, override) + ".tmp", filepath.Join(commandDir, "mcp", "tokens.json"), + filepath.Join(commandDir, "mcp", "tokens.json.tmp"), + filepath.Join(commandDir, "mcp", "tokens.json.secret"), + filepath.Join(commandDir, "mcp", "tokens.json.secret.tmp"), filepath.Join(commandDir, "mcp", "tokens.json.migrated"), } { if !stringSliceContains(paths, want) { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 14b5815f3..8c3ff01d0 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -277,6 +277,9 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string if tokenPath := strings.TrimSpace(options.MCPOAuthTokens); tokenPath != "" { candidates = append(candidates, tokenPath, + tokenPath+".tmp", + tokenPath+".secret", + tokenPath+".secret.tmp", tokenPath+".migrated", ) } diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index d3e4310b8..b0676a8ea 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -355,7 +355,7 @@ func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) } } fs.DenyRead = normalizeProfilePaths(policy.DenyRead) - fs.DenyReadIfExists = credentialDenyReadPaths(policy, credentialBaseDir, nil) + fs.DenyReadIfExists = credentialDenyReadPaths(policy, credentialBaseDir, os.Environ()) fs.DenyWrite = normalizeProfilePaths(policy.DenyWrite) return PermissionProfile{ FileSystem: fs, diff --git a/internal/sandbox/runner_linux_integration_test.go b/internal/sandbox/runner_linux_integration_test.go index 69af36af8..dcf856e63 100644 --- a/internal/sandbox/runner_linux_integration_test.go +++ b/internal/sandbox/runner_linux_integration_test.go @@ -5,7 +5,6 @@ package sandbox import ( "context" "errors" - "fmt" "os" "os/exec" "path/filepath" @@ -91,7 +90,7 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { }) t.Run("missing protected path fails closed", func(t *testing.T) { - missingDenied := fmt.Sprintf("/etc/zero-sandbox-missing-%d-%d/nested", os.Getpid(), time.Now().UnixNano()) + missingDenied := filepath.Join(t.TempDir(), "missing-parent", "nested") if _, err := os.Stat(filepath.Dir(missingDenied)); !errors.Is(err, os.ErrNotExist) { t.Fatalf("missing deny-path precondition failed: %v", err) } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index c65f45630..cf299aef7 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -533,6 +533,27 @@ func TestSandboxExecCommandPlanUsesUniquePerPlanDenialTag(t *testing.T) { } } +func TestSeatbeltCompatibilityProfileUsesCredentialEnvironment(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + override := filepath.Join(t.TempDir(), "mcp-tokens.json") + t.Setenv("ZERO_MCP_OAUTH_TOKENS_PATH", override) + + profile := seatbeltCompatibilityPermissionProfile([]string{"/ws"}, DefaultPolicy()) + for _, want := range []string{ + override, + override + ".tmp", + override + ".secret", + override + ".secret.tmp", + override + ".migrated", + } { + if !stringSliceContains(profile.FileSystem.DenyReadIfExists, normalizeProfilePath(want)) { + t.Fatalf("DenyReadIfExists = %#v, want environment override sibling %q", profile.FileSystem.DenyReadIfExists, want) + } + } +} + func TestSandboxExecProfileGrantsSignalAndMachLookup(t *testing.T) { profile := sandboxExecProfile([]string{"/ws"}, Policy{Mode: ModeEnforce, EnforceWorkspace: true}, "") From 800de1202996a990ef1d9319377d97bfb694655a Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 18:05:29 +0200 Subject: [PATCH 11/18] fix(sandbox): protect OAuth lock siblings --- internal/sandbox/linux_helper.go | 8 ++++++++ internal/sandbox/manager_test.go | 8 ++++++++ internal/sandbox/profile.go | 28 +++++++++++++++++++--------- internal/sandbox/runner_test.go | 2 ++ 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 7655a97c3..2c0917eb8 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -233,6 +233,9 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { args = append(args, "--bind", root.Root, root.Root) for _, subpath := range root.ReadOnlySubpaths { if !pathExists(subpath) { + // Bubblewrap cannot mount over an absent child after binding a + // writable host root without creating that child on the host. Do not + // mutate the workspace merely to prepare the mount namespace. continue } var err error @@ -244,6 +247,8 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { for _, name := range root.ProtectedMetadataNames { path := filepath.Join(root.Root, name) if !pathExists(path) { + // As above, an absent mount point cannot be represented without + // either changing the host or making the writable root read-only. continue } var err error @@ -269,6 +274,9 @@ func linuxBwrapFilesystemArgs(profile PermissionProfile) ([]string, error) { } for _, path := range fs.DenyReadIfExists { if !pathExists(path) { + // The read-all profile begins with a read-only host-root bind. Bwrap + // cannot create a missing mount destination in that tree, and masking + // its nearest existing parent could hide a workspace, HOME, or /tmp. continue } var err error diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 0db68ca3f..7e433f1ce 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -393,12 +393,16 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { overrideFiles := []string{ oauthOverride, oauthOverride + ".tmp", + oauthOverride + ".lockfile", oauthOverride + ".secret", oauthOverride + ".secret.tmp", + oauthOverride + ".secret.lock", mcpOverride, mcpOverride + ".tmp", + mcpOverride + ".lockfile", mcpOverride + ".secret", mcpOverride + ".secret.tmp", + mcpOverride + ".secret.lock", mcpOverride + ".migrated", } // The migrated legacy MCP token backup and the atomic-write temp siblings @@ -510,10 +514,14 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { filepath.Join(wantConfig, "zero"), filepath.Join(commandDir, override), filepath.Join(commandDir, override) + ".tmp", + filepath.Join(commandDir, override) + ".lockfile", + filepath.Join(commandDir, override) + ".secret.lock", filepath.Join(commandDir, "mcp", "tokens.json"), filepath.Join(commandDir, "mcp", "tokens.json.tmp"), + filepath.Join(commandDir, "mcp", "tokens.json.lockfile"), filepath.Join(commandDir, "mcp", "tokens.json.secret"), filepath.Join(commandDir, "mcp", "tokens.json.secret.tmp"), + filepath.Join(commandDir, "mcp", "tokens.json.secret.lock"), filepath.Join(commandDir, "mcp", "tokens.json.migrated"), } { if !stringSliceContains(paths, want) { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 8c3ff01d0..0caf5063f 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -21,14 +21,17 @@ type PermissionProfile struct { } type FileSystemPolicy struct { - Kind FileSystemPolicyKind `json:"kind"` - ReadRoots []string `json:"readRoots,omitempty"` - WriteRoots []WritableRoot `json:"writeRoots,omitempty"` - DenyRead []string `json:"denyRead,omitempty"` - DenyReadIfExists []string `json:"denyReadIfExists,omitempty"` - DenyWrite []string `json:"denyWrite,omitempty"` - IncludePlatformRoots bool `json:"includePlatformRoots,omitempty"` - AllowTemp bool `json:"allowTemp,omitempty"` + Kind FileSystemPolicyKind `json:"kind"` + ReadRoots []string `json:"readRoots,omitempty"` + WriteRoots []WritableRoot `json:"writeRoots,omitempty"` + DenyRead []string `json:"denyRead,omitempty"` + // DenyReadIfExists contains best-effort baseline paths. Backends with + // path-based policies can protect future paths; mount-based Linux only + // masks entries that exist when the namespace is assembled. + DenyReadIfExists []string `json:"denyReadIfExists,omitempty"` + DenyWrite []string `json:"denyWrite,omitempty"` + IncludePlatformRoots bool `json:"includePlatformRoots,omitempty"` + AllowTemp bool `json:"allowTemp,omitempty"` } type WritableRoot struct { @@ -61,7 +64,10 @@ var sandboxFullyProtectedMetadataNames = []string{".zero", ".agents"} // gitMetadataWriteCarveouts returns the .git subpaths that stay write-denied // under the OS-level sandbox even though the rest of .git is writable to git // subprocesses. Backends enforce paths that exist when the sandbox starts; -// unlike explicit deny rules, absent baseline metadata must not prevent launch. +// mount-based Linux cannot protect an absent child beneath a writable bind +// without either creating it on the host or making its parent read-only, so an +// absent baseline remains an acknowledged backend limitation rather than +// preventing ordinary non-Git workspaces from launching. func gitMetadataWriteCarveouts(root string) []string { return []string{ filepath.Join(root, ".git", "hooks"), @@ -270,16 +276,20 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string candidates = append(candidates, tokenPath, tokenPath+".tmp", + tokenPath+".lockfile", tokenPath+".secret", tokenPath+".secret.tmp", + tokenPath+".secret.lock", ) } if tokenPath := strings.TrimSpace(options.MCPOAuthTokens); tokenPath != "" { candidates = append(candidates, tokenPath, tokenPath+".tmp", + tokenPath+".lockfile", tokenPath+".secret", tokenPath+".secret.tmp", + tokenPath+".secret.lock", tokenPath+".migrated", ) } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index cf299aef7..f66d7ffd9 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -544,8 +544,10 @@ func TestSeatbeltCompatibilityProfileUsesCredentialEnvironment(t *testing.T) { for _, want := range []string{ override, override + ".tmp", + override + ".lockfile", override + ".secret", override + ".secret.tmp", + override + ".secret.lock", override + ".migrated", } { if !stringSliceContains(profile.FileSystem.DenyReadIfExists, normalizeProfilePath(want)) { From 662770c2a304effbca1a3b581c9cd33646350189 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 18:28:50 +0200 Subject: [PATCH 12/18] docs(sandbox): clarify Linux future-path limits --- internal/sandbox/manager_test.go | 15 ++++++++------- internal/sandbox/profile.go | 5 +++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 7e433f1ce..481967f0e 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -452,9 +452,9 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } // A default candidate absent from disk at profile-build time is still - // emitted: a rule installed only for what exists now would miss a store - // created later in a long-lived sandboxed session (e.g. a concurrent - // `zero auth login`, or a token file appearing mid-session). + // emitted so pathname-policy backends can reserve it. Mount-based Linux + // retains the same profile baseline but can mask only paths that exist when + // Bubblewrap assembles the namespace. if !stringSliceContains(paths, filepath.Join(home, ".azure")) { t.Errorf("credential deny paths = %#v, want the not-yet-existing ~/.azure included", paths) } @@ -582,7 +582,7 @@ func TestBuildCommandPlanUsesCommandCredentialContext(t *testing.T) { } } -func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { +func TestPermissionProfileIncludesZeroCredentialPaths(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") } @@ -593,9 +593,10 @@ func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", configHome) zeroDir := filepath.Join(configHome, "zero") - // Build the profile BEFORE the store directory exists on disk: a - // sandboxed command launched early in a session must still deny reads of - // credentials created later, not just ones present at profile-build time. + // Build the profile before the store directory exists to verify that profile + // derivation retains the baseline candidate. Pathname-policy backends can + // reserve it immediately; mount-based Linux applies it only if the path + // exists when Bubblewrap assembles the namespace. profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) want := normalizeProfilePaths([]string{zeroDir})[0] if !stringSliceContains(profile.FileSystem.DenyReadIfExists, want) { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 0caf5063f..2bab390d8 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -175,8 +175,9 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // - A candidate nested under a user-configured AllowRead entry is dropped, // so `allowRead: ["~/.aws"]` remains an explicit opt-out. // - Candidates are emitted whether or not they currently exist on disk. -// Backends that support future-path rules enforce them immediately; Linux -// enforces the baseline paths that exist without making fresh homes unusable. +// Pathname-policy backends such as Seatbelt can enforce future paths; +// mount-based Linux masks only baseline paths that exist when the namespace +// is assembled so fresh homes remain usable. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and From 46d071da01d0c21280ee450799bf6846144787a1 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 13:14:24 +0200 Subject: [PATCH 13/18] test(sandbox): probe credential store reads in the Linux smoke test The smoke test wired HOME/XDG_CONFIG_HOME at credential directories but never attempted a read through the sandbox, so it could not catch a real-bwrap regression for the paths this change protects. Write a secret into ~/.aws and $XDG_CONFIG_HOME/zero and add both to the deny-read table. --- .../sandbox/runner_linux_integration_test.go | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/sandbox/runner_linux_integration_test.go b/internal/sandbox/runner_linux_integration_test.go index ade5c9673..f542d2f6b 100644 --- a/internal/sandbox/runner_linux_integration_test.go +++ b/internal/sandbox/runner_linux_integration_test.go @@ -41,6 +41,16 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { t.Fatalf("Mkdir credential path: %v", err) } } + // Secrets inside those stores, so the deny-read table below probes a real read + // rather than only the directory's presence. + awsCredentials := filepath.Join(credentialHome, ".aws", "credentials") + if err := os.WriteFile(awsCredentials, []byte("[default]\naws_secret_access_key = leaked\n"), 0o600); err != nil { + t.Fatalf("WriteFile aws credentials: %v", err) + } + zeroTokens := filepath.Join(configHome, "zero", "oauth-tokens.json") + if err := os.WriteFile(zeroTokens, []byte(`{"access_token":"leaked"}`+"\n"), 0o600); err != nil { + t.Fatalf("WriteFile zero tokens: %v", err) + } t.Setenv("HOME", credentialHome) t.Setenv("XDG_CONFIG_HOME", configHome) secretDir := t.TempDir() @@ -142,6 +152,16 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { script: "if echo leak > .git/config 2>/dev/null; then echo METADATA_WRITE_SUCCEEDED; exit 42; fi", marker: "METADATA_WRITE_SUCCEEDED", }, + { + name: "cloud credential store read", + script: "if cat " + shellQuote(awsCredentials) + " 2>/dev/null | grep -q leaked; then echo CLOUD_CREDENTIAL_READ_SUCCEEDED; exit 42; fi", + marker: "CLOUD_CREDENTIAL_READ_SUCCEEDED", + }, + { + name: "zero credential store read", + script: "if cat " + shellQuote(zeroTokens) + " 2>/dev/null | grep -q leaked; then echo ZERO_CREDENTIAL_READ_SUCCEEDED; exit 42; fi", + marker: "ZERO_CREDENTIAL_READ_SUCCEEDED", + }, } { t.Run(tc.name, func(t *testing.T) { output, _ := runLinuxSandboxSmokeCommand(t, engine, CommandSpec{ From 0f6c87a8f05b454815c30128fe2132127125de11 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 21:52:51 +0200 Subject: [PATCH 14/18] fix(sandbox): keep the credential deny usable, current, and unguessable Four problems with the credential deny baseline, all in the same mechanism. The whole-directory deny over $XDG_CONFIG_HOME/zero also hid the supported user plugin, specialist, and command roots, whose commands and scripts are executed through this sandbox, so an installed user plugin could no longer start. Narrowing the deny to itemized files was not an option: the stores publish through temporary names and a concurrent login can add a store that did not exist when the profile was built, and only a directory rule covers both. The directory deny therefore stays, and the non-secret subtrees are carved back out through a new DenyReadCarveouts field: seatbelt re-allows them after the deny (SBPL is last-match-wins) plus ancestor metadata so path resolution can reach them, and bubblewrap masks the directory with --perms 111 instead of 000 -- unlistable but traversable -- and re-binds the carveout read-only before the tmpfs is remounted read-only. A relative ZERO_OAUTH_TOKENS_PATH, ZERO_MCP_OAUTH_TOKENS_PATH, or XDG_CONFIG_HOME was resolved against the command directory only, while oauth.ResolveStorePath and mcp.ResolveTokenStorePath call filepath.Abs, i.e. resolve against the Zero process working directory. A command run from a subdirectory therefore had the wrong path denied and the real store stayed readable under the read-all posture. Relative candidates now resolve against both directories, since the process that resolves the value is not necessarily the one that writes the store. The regression test runs the command from a different directory than the process, which the previous tests did not. bubblewrap cannot mount over a path that is absent when the namespace is assembled, so a store created mid-session stayed readable through the live read-only host-root bind for the rest of a long-lived sandbox session. The directories Zero owns -- its config directory and the per-store publication directories -- are now reported as EnsureDenyReadDirs and created (0700) before the namespace is assembled, so their mask always exists. Third-party stores such as ~/.aws are still skipped when absent: Zero must not create them, and that limitation stays documented on the field. The fixed .tmp publication sibling made the plaintext token, and via the same path its encryption key, observable to a same-user sandboxed process with write access to an override parent: it could wait for the known name and rename the opened inode into an allowed path. Both stores now publish through a per-store .publish directory with a randomly-named file inside. The directory name is derived from the store path, so the profile can deny it up front, while the random name inside is what cannot be waited for or renamed away. A test keeps the duplicated suffix in step with oauth.PublicationDirSuffix. Also adds the seatbelt render coverage that was missing: dropping DenyReadIfExists from denyReadRules made every credential store readable again on macOS without failing a single test. --- internal/oauth/encrypt.go | 11 +- internal/oauth/store.go | 40 +- internal/oauth/store_test.go | 43 ++- internal/sandbox/export_test.go | 6 +- internal/sandbox/linux_helper.go | 77 +++- internal/sandbox/linux_helper_test.go | 68 ++++ internal/sandbox/manager_test.go | 156 ++++++-- internal/sandbox/profile.go | 362 +++++++++++++----- internal/sandbox/profile_test.go | 11 +- internal/sandbox/runner.go | 33 ++ .../sandbox/runner_linux_integration_test.go | 87 +++++ internal/sandbox/runner_test.go | 39 ++ 12 files changed, 772 insertions(+), 161 deletions(-) diff --git a/internal/oauth/encrypt.go b/internal/oauth/encrypt.go index 3e9502dcb..ed821924c 100644 --- a/internal/oauth/encrypt.go +++ b/internal/oauth/encrypt.go @@ -140,17 +140,14 @@ func writeNewSecretFile(path string) ([]byte, error) { if _, err := io.ReadFull(rand.Reader, secret); err != nil { return nil, fmt.Errorf("oauth: generate token secret: %w", err) } - tmpPath := path + ".tmp" - _ = os.Remove(tmpPath) - tmp, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + // The encryption key is published exactly like the token blob: through the + // protected per-store directory, under a random name a sandboxed process + // cannot guess, wait for, or rename away. + tmp, tmpPath, err := createPublicationFile(path) if err != nil { return nil, fmt.Errorf("oauth: create token secret temp file: %w", err) } defer os.Remove(tmpPath) - if err := tmp.Chmod(0o600); err != nil { - _ = tmp.Close() - return nil, fmt.Errorf("oauth: chmod token secret temp file: %w", err) - } if _, werr := tmp.Write(secret); werr != nil { _ = tmp.Close() return nil, fmt.Errorf("oauth: write token secret: %w", werr) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 503363a5a..1bc7f1dc8 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -409,11 +409,7 @@ func (b fileBlob) write(data []byte) error { if err := os.MkdirAll(filepath.Dir(b.path), 0o700); err != nil { return err } - // Saves are serialized by withLock, so a fixed sibling remains collision-free - // while allowing sandbox profiles to protect it without masking the parent. - tempPath := b.path + ".tmp" - _ = os.Remove(tempPath) - temp, err := os.OpenFile(tempPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + temp, tempPath, err := createPublicationFile(b.path) if err != nil { return err } @@ -428,6 +424,40 @@ func (b fileBlob) write(data []byte) error { return os.Rename(tempPath, b.path) } +// PublicationDirSuffix names the per-store directory a token store publishes new +// contents through. Sandbox profiles deny it by name (see +// internal/sandbox.credentialPublicationDir), which is why the directory name is +// derived from the store path while the file inside it is randomly named: the +// deterministic part is what a deny rule can reference, and the random part is +// what stops a same-user process from waiting for the plaintext to appear at a +// path it can open or rename away. +const PublicationDirSuffix = ".publish" + +// PublicationDir returns the publication directory for a store path. +func PublicationDir(path string) string { return path + PublicationDirSuffix } + +// createPublicationFile creates the randomly-named 0600 file that path's next +// contents are written to before being renamed into place. It lives in +// PublicationDir(path) — same filesystem as path, so the rename stays atomic — +// and the directory is created 0700 if it does not exist yet. +func createPublicationFile(path string) (*os.File, string, error) { + dir := PublicationDir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, "", err + } + temp, err := os.CreateTemp(dir, "publish-*") + if err != nil { + return nil, "", err + } + if err := temp.Chmod(0o600); err != nil { + name := temp.Name() + _ = temp.Close() + _ = os.Remove(name) + return nil, "", err + } + return temp, temp.Name(), nil +} + func (b fileBlob) withLock(now func() time.Time, fn func() error) error { unlock, err := acquireFileLock(b.path+".lockfile", now) if err != nil { diff --git a/internal/oauth/store_test.go b/internal/oauth/store_test.go index bc79d5bb9..494f35a02 100644 --- a/internal/oauth/store_test.go +++ b/internal/oauth/store_test.go @@ -1,7 +1,6 @@ package oauth import ( - "errors" "os" "path/filepath" "runtime" @@ -92,24 +91,29 @@ func TestStoreFileMode0600(t *testing.T) { } } -func TestStoreUsesFixedProtectedPublicationSibling(t *testing.T) { +// TestStorePublishesThroughProtectedDirectory pins the publication contract the +// sandbox profile relies on: the plaintext blob is never written to a path a +// same-user process can predict (a fixed `.tmp` sibling), only to a random name +// inside the directory the profile denies by name, and nothing is left behind. +func TestStorePublishesThroughProtectedDirectory(t *testing.T) { s, path := newTestStore(t) - tempPath := path + ".tmp" - if err := os.WriteFile(tempPath, []byte("stale"), 0o600); err != nil { + if err := os.WriteFile(path+".tmp", []byte("stale"), 0o600); err != nil { t.Fatal(err) } if err := s.Save(ProviderKey("x"), Token{AccessToken: "secret"}); err != nil { t.Fatalf("Save: %v", err) } - if _, err := os.Stat(tempPath); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("publication sibling remains after save: %v", err) + // A predictable sibling is never used, so a pre-existing one is untouched + // rather than becoming the file the plaintext passes through. + if data, err := os.ReadFile(path + ".tmp"); err != nil || string(data) != "stale" { + t.Fatalf("fixed sibling data = %q, err = %v, want the untouched placeholder", data, err) } + assertEmptyPublicationDir(t, path) } -func TestEncryptedStoreUsesFixedProtectedSecretSibling(t *testing.T) { +func TestEncryptedStorePublishesSecretThroughProtectedDirectory(t *testing.T) { path := filepath.Join(t.TempDir(), "tokens.json") - secretTempPath := path + ".secret.tmp" - if err := os.WriteFile(secretTempPath, []byte("stale"), 0o600); err != nil { + if err := os.WriteFile(path+".secret.tmp", []byte("stale"), 0o600); err != nil { t.Fatal(err) } s, err := NewStore(StoreOptions{FilePath: path, Storage: "encrypted-file"}) @@ -119,8 +123,25 @@ func TestEncryptedStoreUsesFixedProtectedSecretSibling(t *testing.T) { if err := s.Save(ProviderKey("x"), Token{AccessToken: "secret"}); err != nil { t.Fatalf("Save: %v", err) } - if _, err := os.Stat(secretTempPath); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("secret publication sibling remains after save: %v", err) + if data, err := os.ReadFile(path + ".secret.tmp"); err != nil || string(data) != "stale" { + t.Fatalf("fixed secret sibling data = %q, err = %v, want the untouched placeholder", data, err) + } + assertEmptyPublicationDir(t, path) + assertEmptyPublicationDir(t, path+".secret") +} + +// assertEmptyPublicationDir asserts the store's publication directory exists +// (so the sandbox has a mount target to mask on Linux) and holds no leftover +// copy of the secret it published. +func assertEmptyPublicationDir(t *testing.T, storePath string) { + t.Helper() + dir := PublicationDir(storePath) + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read publication dir %s: %v", dir, err) + } + if len(entries) != 0 { + t.Fatalf("publication dir %s = %v, want empty after publish", dir, entries) } } diff --git a/internal/sandbox/export_test.go b/internal/sandbox/export_test.go index e610960e4..88fba2a99 100644 --- a/internal/sandbox/export_test.go +++ b/internal/sandbox/export_test.go @@ -46,7 +46,6 @@ func sandboxExecProfile(writeRoots []string, policy Policy, denialTag string) st } func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) PermissionProfile { - credentialBaseDir, _ := os.Getwd() fs := FileSystemPolicy{ Kind: FileSystemUnrestricted, ReadRoots: []string{string(filepath.Separator)}, @@ -61,7 +60,10 @@ func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) } } fs.DenyRead = normalizeProfilePaths(policy.DenyRead) - fs.DenyReadIfExists = credentialDenyReadPaths(policy, credentialBaseDir, os.Environ()) + credentials := credentialDenyReadPaths(policy, "", os.Environ()) + fs.DenyReadIfExists = credentials.Paths + fs.DenyReadCarveouts = pathsOutsideRoots(credentials.Carveouts, fs.DenyRead) + fs.EnsureDenyReadDirs = credentials.EnsureDirs fs.DenyWrite = normalizeProfilePaths(policy.DenyWrite) return PermissionProfile{ FileSystem: fs, diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index ad6a32a4b..a33f1974b 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -277,19 +277,26 @@ func buildLinuxBwrapFilesystemPlan(profile PermissionProfile) linuxBwrapFilesyst args = appendReadOnlyLinuxPathArgs(args, path) } for _, path := range fs.DenyRead { - args = appendUnreadableLinuxPathArgs(args, path) - } + args = appendUnreadableLinuxPathArgs(args, path, fs.DenyReadCarveouts) + } + // Zero owns these directories, so create the ones that are missing before the + // namespace is assembled: bubblewrap cannot mount over an absent path, and a + // directory that appears afterwards (a concurrent login writing a token + // store) would otherwise be readable through the live read-only host bind for + // the rest of a long-lived sandbox session. + ensureLinuxDenyReadDirs(fs.EnsureDenyReadDirs) for _, path := range fs.DenyReadIfExists { if !pathExists(path) { - // Baseline credential paths are emitted for every run, so an absent - // entry is the common case on a fresh machine. The read-all profile - // starts from a read-only host-root bind where bubblewrap cannot - // create a missing mount destination, and masking the nearest - // existing parent could hide HOME, /tmp, or the workspace. Path-based - // backends (seatbelt) still deny these paths before they exist. + // A baseline credential path is emitted for every run, so an absent + // entry is the common case on a fresh machine — a third-party store + // such as ~/.aws that Zero must not create. The read-all profile starts + // from a read-only host-root bind where bubblewrap cannot create a + // missing mount destination, and masking the nearest existing parent + // could hide HOME, /tmp, or the workspace. Path-based backends + // (seatbelt) still deny these paths before they exist. continue } - args = appendUnreadableLinuxPathArgs(args, path) + args = appendUnreadableLinuxPathArgs(args, path, fs.DenyReadCarveouts) } return linuxBwrapFilesystemPlan{ Args: args, @@ -365,7 +372,7 @@ func appendReadOnlyLinuxPathArgs(args []string, path string) []string { return append(args, "--perms", "555", "--tmpfs", path, "--remount-ro", path) } -func appendUnreadableLinuxPathArgs(args []string, path string) []string { +func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []string) []string { path = strings.TrimSpace(path) if path == "" { return args @@ -373,7 +380,55 @@ func appendUnreadableLinuxPathArgs(args []string, path string) []string { if info, err := os.Stat(path); err == nil && !info.IsDir() { return append(args, "--ro-bind", "/dev/null", path) } - return append(args, "--perms", "000", "--tmpfs", path, "--remount-ro", path) + nested := nestedCarveoutPaths(path, carveouts) + if len(nested) == 0 { + return append(args, "--perms", "000", "--tmpfs", path, "--remount-ro", path) + } + // A carveout has to stay reachable, and traversing into a directory needs the + // execute bit, so the mask is 111 (--x--x--x) instead of 000: the directory's + // contents remain unlistable and unreadable, while an explicitly re-bound + // subpath below it can still be resolved. The binds must precede the + // --remount-ro, which is what freezes the tmpfs. + args = append(args, "--perms", "111", "--tmpfs", path) + for _, carveout := range nested { + if pathExists(carveout) { + args = append(args, "--ro-bind", carveout, carveout) + } + } + return append(args, "--remount-ro", path) +} + +// nestedCarveoutPaths returns the carveouts that sit strictly inside root, +// shallowest first so a parent bind is created before a nested one. +func nestedCarveoutPaths(root string, carveouts []string) []string { + if len(carveouts) == 0 { + return nil + } + out := make([]string, 0, len(carveouts)) + for _, carveout := range carveouts { + carveout = strings.TrimSpace(carveout) + if carveout == "" || carveout == root { + continue + } + if pathWithinRoot(root, carveout) { + out = append(out, carveout) + } + } + sort.SliceStable(out, func(i, j int) bool { return pathDepth(out[i]) < pathDepth(out[j]) }) + return dedupeStrings(out) +} + +// ensureLinuxDenyReadDirs creates the Zero-owned directories a deny mask needs +// to exist for. Best effort: a failure just leaves the path unmasked, exactly as +// before, and never blocks the command. +func ensureLinuxDenyReadDirs(dirs []string) { + for _, dir := range dirs { + dir = strings.TrimSpace(dir) + if dir == "" || pathExists(dir) { + continue + } + _ = os.MkdirAll(dir, 0o700) + } } func shouldUnshareLinuxNetwork(policy NetworkPolicy) bool { diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index abe89b2ca..0cab99c53 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -297,6 +297,74 @@ func TestLinuxBwrapSkipsMissingCredentialBaselines(t *testing.T) { assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", missingCredential, "--remount-ro", missingCredential) } +// TestLinuxBwrapCreatesOwnedCredentialDirsBeforeMasking covers the long-lived +// session race: bubblewrap cannot mount over a path that does not exist, so a +// store written after the namespace was assembled would stay readable through +// the live read-only host-root bind. Zero's own directories are therefore +// created up front and masked, unlike third-party stores it must not create. +func TestLinuxBwrapCreatesOwnedCredentialDirsBeforeMasking(t *testing.T) { + root := t.TempDir() + ownedDir := filepath.Join(root, "config", "zero") + thirdParty := filepath.Join(root, "home", ".aws") + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: root}}, + DenyReadIfExists: []string{ownedDir, thirdParty}, + EnsureDenyReadDirs: []string{ownedDir}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + + plan := buildLinuxBwrapFilesystemPlan(profile) + if info, err := os.Stat(ownedDir); err != nil || !info.IsDir() { + t.Fatalf("owned credential dir was not created: err=%v", err) + } + assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", ownedDir, "--remount-ro", ownedDir) + if stringSliceContains(plan.Args, thirdParty) { + t.Fatalf("absent third-party store must stay unmounted and uncreated: %#v", plan.Args) + } + if _, err := os.Stat(thirdParty); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("third-party store must not be created by the sandbox: %v", err) + } +} + +// TestLinuxBwrapKeepsCarveoutsReachableInsideMaskedDir covers the user plugin +// root inside the denied Zero config directory: the mask has to keep the +// traverse bit and re-bind the carveout, otherwise an installed user plugin +// cannot be executed through the sandbox at all. +func TestLinuxBwrapKeepsCarveoutsReachableInsideMaskedDir(t *testing.T) { + root := t.TempDir() + credentialDir := filepath.Join(root, "config", "zero") + pluginRoot := filepath.Join(credentialDir, "plugins") + if err := os.MkdirAll(pluginRoot, 0o700); err != nil { + t.Fatal(err) + } + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: root}}, + DenyReadIfExists: []string{credentialDir}, + DenyReadCarveouts: []string{pluginRoot}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + + plan := buildLinuxBwrapFilesystemPlan(profile) + // 111 rather than 000: a 000 directory cannot be traversed, so the re-bound + // subpath below it would be unreachable. Contents stay unlistable either way. + assertArgsContainSequence(t, plan.Args, "--perms", "111", "--tmpfs", credentialDir) + assertArgsContainSequence(t, plan.Args, "--ro-bind", pluginRoot, pluginRoot) + assertArgsContainSequence(t, plan.Args, "--remount-ro", credentialDir) + bindIdx := argsSequenceIndex(plan.Args, "--ro-bind", pluginRoot, pluginRoot) + remountIdx := argsSequenceIndex(plan.Args, "--remount-ro", credentialDir) + if bindIdx < 0 || remountIdx < 0 || bindIdx > remountIdx { + t.Fatalf("carveout bind (%d) must precede the tmpfs remount-ro (%d): %#v", bindIdx, remountIdx, plan.Args) + } +} + func TestLinuxBwrapUnrestrictedFilesystemUsesWritableHostRoot(t *testing.T) { profile := PermissionProfile{ FileSystem: FileSystemPolicy{ diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 481967f0e..7b85a6c74 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -7,8 +7,20 @@ import ( "reflect" "runtime" "testing" + + "github.com/Gitlawb/zero/internal/oauth" ) +// TestCredentialPublicationDirSuffixMatchesStore keeps the duplicated suffix in +// step with the store that creates the directory: if they ever diverge, the +// profile denies a directory the store does not publish through, and the +// plaintext token passes through an unprotected path instead. +func TestCredentialPublicationDirSuffixMatchesStore(t *testing.T) { + if credentialPublicationDirSuffix != oauth.PublicationDirSuffix { + t.Fatalf("publication dir suffix = %q, want oauth.PublicationDirSuffix %q", credentialPublicationDirSuffix, oauth.PublicationDirSuffix) + } +} + func TestPermissionProfileFromPolicyBuildsWorkspaceWriteProfile(t *testing.T) { workspace := t.TempDir() extra := t.TempDir() @@ -429,19 +441,46 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } options := credentialPathOptions{ - Home: home, - GoogleCredentials: keyFile, - ZeroConfigDir: filepath.Join(home, "config"), - OAuthTokens: oauthOverride, - MCPOAuthTokens: mcpOverride, - } - paths := credentialDenyReadPathsIn(options, nil) + Homes: []string{home}, + GoogleCredentials: []string{keyFile}, + ZeroConfigDirs: []string{filepath.Join(home, "config")}, + OAuthTokens: []string{oauthOverride}, + MCPOAuthTokens: []string{mcpOverride}, + } + credentials := credentialDenyReadPathsIn(options, nil) + paths := credentials.Paths wantPaths := append([]string{awsDir, gcloudDir, keyFile, zeroDir}, overrideFiles...) for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) } } + // The store publishes through a per-store directory whose name is derived + // from the store path, so it can be denied even though the random file name + // inside it cannot be. + for _, want := range normalizeProfilePaths([]string{oauthOverride + ".publish", mcpOverride + ".publish"}) { + if !stringSliceContains(paths, want) { + t.Errorf("credential deny paths = %#v, want publication directory %q included", paths, want) + } + } + // Zero owns its config directory and the publication directories, so the + // mount-based backend may create them to guarantee a mask exists. + for _, want := range normalizeProfilePaths([]string{zeroDir, oauthOverride + ".publish", mcpOverride + ".publish"}) { + if !stringSliceContains(credentials.EnsureDirs, want) { + t.Errorf("credential ensure dirs = %#v, want %q", credentials.EnsureDirs, want) + } + } + if stringSliceContains(credentials.EnsureDirs, normalizeProfilePaths([]string{awsDir})[0]) { + t.Errorf("credential ensure dirs = %#v, must not create third-party stores", credentials.EnsureDirs) + } + // The user plugin/specialist/command roots live in the denied config + // directory and are executed through the sandbox, so they stay readable. + for _, name := range []string{"plugins", "specialists", "commands"} { + want := filepath.Join(zeroDir, name) + if !stringSliceContains(credentials.Carveouts, normalizeProfilePaths([]string{want})[0]) { + t.Errorf("credential carveouts = %#v, want %q", credentials.Carveouts, want) + } + } // zeroFiles is covered by the zeroDir subpath deny above, not by an // itemized entry — including the never-enumerated migrated backup and // temp-write siblings. @@ -461,25 +500,32 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { // An explicit AllowRead entry covering a store is an opt-out. optedOut := credentialDenyReadPathsIn(options, []string{awsDir, zeroDir}) - if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { - t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) + if stringSliceContains(optedOut.Paths, normalizeProfilePaths([]string{awsDir})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut.Paths) + } + if stringSliceContains(optedOut.Paths, normalizeProfilePaths([]string{zeroDir})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop %q", optedOut.Paths, zeroDir) } - if stringSliceContains(optedOut, normalizeProfilePaths([]string{zeroDir})[0]) { - t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop %q", optedOut, zeroDir) + if !stringSliceContains(optedOut.Paths, normalizeProfilePaths([]string{keyFile})[0]) { + t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut.Paths) } - if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { - t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) + // Nothing is created or carved out for a directory that is no longer denied. + if stringSliceContains(optedOut.EnsureDirs, normalizeProfilePaths([]string{zeroDir})[0]) { + t.Errorf("credential ensure dirs = %#v, want the opted-out config dir dropped", optedOut.EnsureDirs) + } + if stringSliceContains(optedOut.Carveouts, normalizeProfilePaths([]string{filepath.Join(zeroDir, "plugins")})[0]) { + t.Errorf("credential carveouts = %#v, want no allow-back inside an opted-out deny", optedOut.Carveouts) } - if got := credentialDenyReadPathsIn(credentialPathOptions{}, nil); len(got) != 0 { - t.Errorf("credential deny paths for blank home = %#v, want none", got) + if got := credentialDenyReadPathsIn(credentialPathOptions{}, nil); len(got.Paths) != 0 { + t.Errorf("credential deny paths for blank home = %#v, want none", got.Paths) } // The GOOGLE_APPLICATION_CREDENTIALS target stays protected even when no // home directory is resolvable. - homeless := credentialDenyReadPathsIn(credentialPathOptions{GoogleCredentials: keyFile}, nil) - if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { - t.Errorf("credential deny paths without home = %#v, want key file included", homeless) + homeless := credentialDenyReadPathsIn(credentialPathOptions{GoogleCredentials: []string{keyFile}}, nil) + if !stringSliceContains(homeless.Paths, normalizeProfilePaths([]string{keyFile})[0]) { + t.Errorf("credential deny paths without home = %#v, want key file included", homeless.Paths) } } @@ -500,15 +546,15 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { "ZERO_OAUTH_TOKENS_PATH=" + override, "ZERO_MCP_OAUTH_TOKENS_PATH=mcp/tokens.json", }) - paths := credentialDenyReadPathsIn(options, nil) + paths := credentialDenyReadPathsIn(options, nil).Paths wantHome := filepath.Join(commandDir, "profile-home") - if options.Home != wantHome { - t.Fatalf("home = %q, want USERPROFILE fallback %q", options.Home, wantHome) + if !stringSliceContains(options.Homes, wantHome) { + t.Fatalf("homes = %#v, want USERPROFILE fallback %q", options.Homes, wantHome) } wantConfig := filepath.Join(commandDir, "~", "literal-xdg") - if options.ZeroConfigDir != wantConfig { - t.Fatalf("config dir = %q, want command-relative literal XDG path %q", options.ZeroConfigDir, wantConfig) + if !stringSliceContains(options.ZeroConfigDirs, wantConfig) { + t.Fatalf("config dirs = %#v, want command-relative literal XDG path %q", options.ZeroConfigDirs, wantConfig) } for _, want := range []string{ filepath.Join(wantConfig, "zero"), @@ -533,12 +579,12 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { func TestCredentialDenyReadPathsInConfigDirMatchesLiteralXDGResolution(t *testing.T) { configDir := "~/literal-xdg" commandDir := t.TempDir() - resolvedConfigDir := credentialPathOptionsFromEnvironment(commandDir, []string{"XDG_CONFIG_HOME=" + configDir}).ZeroConfigDir - paths := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDir: resolvedConfigDir}, nil) + resolvedConfigDirs := credentialPathOptionsFromEnvironment(commandDir, []string{"XDG_CONFIG_HOME=" + configDir}).ZeroConfigDirs + paths := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDirs: resolvedConfigDirs}, nil).Paths want := filepath.Join(commandDir, configDir, "zero") - if resolvedConfigDir != filepath.Dir(want) { - t.Fatalf("zero credential config dir = %q, want literal XDG resolution %q", resolvedConfigDir, filepath.Dir(want)) + if !stringSliceContains(resolvedConfigDirs, filepath.Dir(want)) { + t.Fatalf("zero credential config dirs = %#v, want literal XDG resolution %q", resolvedConfigDirs, filepath.Dir(want)) } if !stringSliceContains(paths, want) { t.Fatalf("credential deny paths = %#v, want literal XDG resolution %q", paths, want) @@ -582,6 +628,62 @@ func TestBuildCommandPlanUsesCommandCredentialContext(t *testing.T) { } } +// TestBuildCommandPlanDeniesRelativeOverrideAtProcessAndCommandDir covers the +// bypass a command-directory-only resolution left open: oauth.ResolveStorePath +// and mcp.ResolveTokenStorePath call filepath.Abs, so a relative override names +// a file under the ZERO PROCESS working directory, while a sandboxed command +// runs with its own cwd. Denying only the command-relative path left the real +// store readable under the read-all posture. +func TestBuildCommandPlanDeniesRelativeOverrideAtProcessAndCommandDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + workspace := resolvedTempDir(t) + processDir := filepath.Join(workspace, "process") + commandDir := filepath.Join(workspace, "process", "nested") + if err := mkdirAll(commandDir); err != nil { + t.Fatal(err) + } + originalDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(processDir); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(originalDir) }) + + engine := NewEngine(EngineOptions{ + WorkspaceRoot: workspace, + Policy: DefaultPolicy(), + Backend: Backend{Name: BackendUnavailable, Platform: runtime.GOOS}, + }) + plan, err := engine.BuildCommandPlan(CommandSpec{ + Name: "true", + Dir: commandDir, + Env: []string{"HOME=" + filepath.Join(workspace, "home"), "ZERO_OAUTH_TOKENS_PATH=tokens.json"}, + }) + if err != nil { + t.Fatal(err) + } + // The store the Zero process actually writes. + processStore := filepath.Join(processDir, "tokens.json") + // The path a sandboxed child (e.g. a nested Zero) would resolve instead. + commandStore := filepath.Join(commandDir, "tokens.json") + for _, want := range []string{processStore, commandStore} { + if !stringSliceContains(plan.PermissionProfile.FileSystem.DenyReadIfExists, normalizeProfilePath(want)) { + t.Fatalf("DenyReadIfExists = %#v, want relative override resolved to %q", plan.PermissionProfile.FileSystem.DenyReadIfExists, want) + } + } + // Neither resolution may mask the parent directory itself: that would hide the + // workspace or a temp root from every sandboxed command. + for _, unwanted := range []string{processDir, commandDir} { + if stringSliceContains(plan.PermissionProfile.FileSystem.DenyReadIfExists, normalizeProfilePath(unwanted)) { + t.Fatalf("DenyReadIfExists = %#v, must not mask override parent %q", plan.PermissionProfile.FileSystem.DenyReadIfExists, unwanted) + } + } +} + func TestPermissionProfileIncludesZeroCredentialPaths(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index f412e8b0b..c1b0e418a 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -29,7 +29,19 @@ type FileSystemPolicy struct { // DenyReadIfExists contains best-effort baseline paths. Backends with // path-based policies can protect future paths; mount-based Linux only // masks entries that exist when the namespace is assembled. - DenyReadIfExists []string `json:"denyReadIfExists,omitempty"` + DenyReadIfExists []string `json:"denyReadIfExists,omitempty"` + // DenyReadCarveouts are subtrees that stay readable INSIDE a denied root. + // They exist so a directory-level credential deny can also cover the files + // a store publishes (arbitrary temporary names, files created later in the + // session) without hiding the supported non-secret subtrees that live in the + // same directory — Zero's user plugin/specialist/command roots, whose + // commands and scripts are themselves executed through the sandbox. + DenyReadCarveouts []string `json:"denyReadCarveouts,omitempty"` + // EnsureDenyReadDirs are directories Zero owns that a mount-based backend + // may create (0700) so a mask exists for them. bubblewrap cannot mount over + // a path that is absent when the namespace is assembled, so without this a + // store created mid-session would be readable by an already-running sandbox. + EnsureDenyReadDirs []string `json:"ensureDenyReadDirs,omitempty"` DenyWrite []string `json:"denyWrite,omitempty"` IncludePlatformRoots bool `json:"includePlatformRoots,omitempty"` AllowTemp bool `json:"allowTemp,omitempty"` @@ -74,11 +86,10 @@ func gitMetadataWriteCarveouts(root string) []string { } func PermissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Scope) PermissionProfile { - baseDir, _ := os.Getwd() - return permissionProfileFromPolicy(workspaceRoot, policy, scope, baseDir, nil) + return permissionProfileFromPolicy(workspaceRoot, policy, scope, "", nil) } -func permissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Scope, credentialBaseDir string, credentialEnv []string) PermissionProfile { +func permissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Scope, credentialCommandDir string, credentialEnv []string) PermissionProfile { if policy.Mode == "" { policy = DefaultPolicy() } @@ -102,13 +113,20 @@ func permissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Sco ProtectedMetadataNames: append([]string{}, sandboxFullyProtectedMetadataNames...), }) } + userDenyRead := normalizeProfilePaths(policy.DenyRead) + credentials := credentialDenyReadPaths(policy, credentialCommandDir, credentialEnv) + // A carveout re-includes reads, so it must never reach inside a path the USER + // denied: their configuration outranks Zero's automatic baseline. + credentials.Carveouts = pathsOutsideRoots(credentials.Carveouts, userDenyRead) return PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, ReadRoots: readRoots, WriteRoots: writeRoots, - DenyRead: normalizeProfilePaths(policy.DenyRead), - DenyReadIfExists: credentialDenyReadPaths(policy, credentialBaseDir, credentialEnv), + DenyRead: userDenyRead, + DenyReadIfExists: credentials.Paths, + DenyReadCarveouts: credentials.Carveouts, + EnsureDenyReadDirs: credentials.EnsureDirs, DenyWrite: normalizeProfilePaths(policy.DenyWrite), IncludePlatformRoots: true, AllowTemp: true, @@ -154,10 +172,20 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop return dedupeStrings(readRoots) } +// credentialDenyPaths is the credential baseline a profile derives from the +// environment: the paths to deny reads on, the non-secret subtrees that stay +// readable inside them, and the Zero-owned directories a mount-based backend +// may create so its mask actually exists. +type credentialDenyPaths struct { + Paths []string + Carveouts []string + EnsureDirs []string +} + // credentialDenyReadPaths returns default deny-read entries for well-known // cloud credential stores, the file GOOGLE_APPLICATION_CREDENTIALS points to, // and Zero's own config/credential/token directory so sandboxed commands -// cannot read secrets under the read-all workspace posture. Three deliberate +// cannot read secrets under the read-all workspace posture. Four deliberate // limits: // // - Windows is skipped: a non-empty profile DenyRead switches the Windows @@ -168,45 +196,84 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // so `allowRead: ["~/.aws"]` remains an explicit opt-out. // - Candidates are emitted whether or not they currently exist on disk. // Pathname-policy backends such as Seatbelt can enforce future paths; -// mount-based Linux masks only baseline paths that exist when the namespace -// is assembled so fresh homes remain usable. +// mount-based Linux masks a path only if it exists when the namespace is +// assembled, which is why the directories Zero owns are also reported as +// EnsureDirs (the backend creates them, so the mask is always present) and +// why third-party stores such as ~/.aws stay best-effort there. +// - Zero's own config directory is denied WHOLE, with the supported +// non-secret subtrees carved back out. Only a directory-level rule covers +// the temporary names its stores publish through and the files a concurrent +// login creates mid-session; the carveouts keep the user plugin, +// specialist, and command roots readable, since those are executed through +// the sandbox. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and // must keep reflecting user configuration alone. -func credentialDenyReadPaths(policy Policy, baseDir string, commandEnv []string) []string { +func credentialDenyReadPaths(policy Policy, commandDir string, commandEnv []string) credentialDenyPaths { if runtime.GOOS == "windows" { - return nil + return credentialDenyPaths{} } - options := credentialPathOptionsFromEnvironment(baseDir, commandEnv) + options := credentialPathOptionsFromEnvironment(commandDir, commandEnv) return credentialDenyReadPathsIn(options, policy.AllowRead) } -func credentialPathOptionsFromEnvironment(baseDir string, commandEnv []string) credentialPathOptions { +// zeroConfigReadCarveoutNames are the supported non-secret subtrees of +// /zero. Their contents are extension code and prompts that a +// sandboxed command legitimately executes or reads (a user plugin's tool +// command lives below the plugin root), so the credential deny must not hide +// them. Nothing here holds a secret: credentials, tokens, and config live in +// files directly under /zero, not in these subtrees. +var zeroConfigReadCarveoutNames = []string{"plugins", "specialists", "commands"} + +// credentialOverrideBaseDirs returns the directories a RELATIVE credential +// override is resolved against, most authoritative first. +// +// The stores themselves call filepath.Abs (oauth.ResolveStorePath, +// mcp.ResolveTokenStorePath), i.e. they resolve against the Zero PROCESS +// working directory, so that is the path that must be denied. The command +// directory is kept as a second candidate because a sandboxed child (including +// a nested Zero) resolves the inherited value against its own cwd instead. +// Denying both costs two rules and closes the mismatch in either direction. +func credentialOverrideBaseDirs(commandDir string) []string { + var dirs []string + if cwd, err := os.Getwd(); err == nil && strings.TrimSpace(cwd) != "" { + dirs = append(dirs, filepath.Clean(cwd)) + } + if dir := strings.TrimSpace(commandDir); dir != "" { + dirs = append(dirs, filepath.Clean(dir)) + } + return dedupeStrings(dirs) +} + +func credentialPathOptionsFromEnvironment(commandDir string, commandEnv []string) credentialPathOptions { env := commandEnv if env == nil { env = os.Environ() } - home := strings.TrimSpace(credentialEnvValue(env, "HOME")) - if home == "" { - home = strings.TrimSpace(credentialEnvValue(env, "USERPROFILE")) + baseDirs := credentialOverrideBaseDirs(commandDir) + homes := resolveCredentialOverridePaths(credentialEnvValue(env, "HOME"), baseDirs) + if len(homes) == 0 { + homes = resolveCredentialOverridePaths(credentialEnvValue(env, "USERPROFILE"), baseDirs) } - if home == "" { - home, _ = os.UserHomeDir() + if len(homes) == 0 { + home, err := os.UserHomeDir() + if err == nil { + homes = resolveCredentialOverridePaths(home, baseDirs) + } } - home = resolveCredentialOverridePath(home, baseDir) - configDir := strings.TrimSpace(credentialEnvValue(env, "XDG_CONFIG_HOME")) - if configDir == "" && home != "" { - configDir = filepath.Join(home, ".config") - } else { - configDir = resolveCredentialOverridePath(configDir, baseDir) + configDirs := resolveCredentialOverridePaths(credentialEnvValue(env, "XDG_CONFIG_HOME"), baseDirs) + if len(configDirs) == 0 { + for _, home := range homes { + configDirs = append(configDirs, filepath.Join(home, ".config")) + } } return credentialPathOptions{ - Home: home, - GoogleCredentials: resolveCredentialOverridePath(credentialEnvValue(env, "GOOGLE_APPLICATION_CREDENTIALS"), baseDir), - ZeroConfigDir: configDir, - OAuthTokens: resolveCredentialOverridePath(credentialEnvValue(env, "ZERO_OAUTH_TOKENS_PATH"), baseDir), - MCPOAuthTokens: resolveCredentialOverridePath(credentialEnvValue(env, "ZERO_MCP_OAUTH_TOKENS_PATH"), baseDir), + Homes: homes, + GoogleCredentials: resolveCredentialOverridePaths(credentialEnvValue(env, "GOOGLE_APPLICATION_CREDENTIALS"), baseDirs), + ZeroConfigDirs: dedupeStrings(configDirs), + OAuthTokens: resolveCredentialOverridePaths(credentialEnvValue(env, "ZERO_OAUTH_TOKENS_PATH"), baseDirs), + MCPOAuthTokens: resolveCredentialOverridePaths(credentialEnvValue(env, "ZERO_MCP_OAUTH_TOKENS_PATH"), baseDirs), } } @@ -222,104 +289,207 @@ func credentialEnvValue(env []string, key string) string { } type credentialPathOptions struct { - Home string - GoogleCredentials string - ZeroConfigDir string - OAuthTokens string - MCPOAuthTokens string + Homes []string + GoogleCredentials []string + ZeroConfigDirs []string + OAuthTokens []string + MCPOAuthTokens []string } // credentialDenyReadPathsIn is the pure core of credentialDenyReadPaths, // separated so tests can exercise it against a synthetic home directory. -func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string) []string { +func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string) credentialDenyPaths { var candidates []string - if home := strings.TrimSpace(options.Home); home != "" { + var carveouts []string + var ensureDirs []string + for _, home := range options.Homes { + if strings.TrimSpace(home) == "" { + continue + } candidates = append(candidates, filepath.Join(home, ".aws"), filepath.Join(home, ".config", "gcloud"), filepath.Join(home, ".azure"), ) } - if target := strings.TrimSpace(options.GoogleCredentials); target != "" { - candidates = append(candidates, target) - } - if configDir := strings.TrimSpace(options.ZeroConfigDir); configDir != "" { - // Deny the whole directory rather than an itemized file list. Zero's - // credential/token/config stores each publish through a randomly-named - // sibling before an atomic rename (oauth-tokens.json.tmp--, - // credentials.{enc,json}.*.tmp, *.secret.*.tmp, .zero-config-*.tmp), and - // the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated - // backup behind after importing it — an itemized list can never keep up - // with those names. Nothing else has a legitimate reason to live here. - candidates = append(candidates, filepath.Join(configDir, "zero")) - } - if tokenPath := strings.TrimSpace(options.OAuthTokens); tokenPath != "" { - // OAuth uses fixed same-directory publication paths so exact denies do not - // have to hide an arbitrary parent such as the workspace or /tmp. - candidates = append(candidates, - tokenPath, - tokenPath+".tmp", - tokenPath+".lockfile", - tokenPath+".secret", - tokenPath+".secret.tmp", - tokenPath+".secret.lock", - ) + candidates = append(candidates, options.GoogleCredentials...) + for _, configDir := range options.ZeroConfigDirs { + if strings.TrimSpace(configDir) == "" { + continue + } + // Deny the whole directory rather than an itemized file list: Zero's + // credential, token, and config stores publish through temporary siblings + // before an atomic rename, the legacy MCP store leaves a + // mcp-oauth-tokens.json.migrated backup behind, and a concurrent login can + // add a store that did not exist when this profile was built. Only a + // directory rule covers all three. Zero owns this directory, so it is also + // an EnsureDir: bubblewrap cannot mask a path that is absent when the + // namespace is assembled. + zeroDir := filepath.Join(configDir, "zero") + candidates = append(candidates, zeroDir) + ensureDirs = append(ensureDirs, zeroDir) + for _, name := range zeroConfigReadCarveoutNames { + carveouts = append(carveouts, filepath.Join(zeroDir, name)) + } } - if tokenPath := strings.TrimSpace(options.MCPOAuthTokens); tokenPath != "" { - candidates = append(candidates, - tokenPath, - tokenPath+".tmp", - tokenPath+".lockfile", - tokenPath+".secret", - tokenPath+".secret.tmp", - tokenPath+".secret.lock", - tokenPath+".migrated", - ) + for _, tokenPath := range options.OAuthTokens { + candidates = append(candidates, credentialTokenStorePaths(tokenPath)...) + ensureDirs = append(ensureDirs, credentialPublicationDirs(tokenPath)...) + } + for _, tokenPath := range options.MCPOAuthTokens { + candidates = append(candidates, credentialTokenStorePaths(tokenPath)...) + // The legacy store renames itself aside after importing into the unified + // store, leaving a readable copy of the same tokens behind. + candidates = append(candidates, tokenPath+".migrated") + ensureDirs = append(ensureDirs, credentialPublicationDirs(tokenPath)...) } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) for _, path := range normalizeProfilePaths(candidates) { - reincluded := false - for _, allow := range allowRoots { - if pathWithinRoot(allow, path) { - reincluded = true + if credentialPathReincluded(allowRoots, path) { + continue + } + out = append(out, path) + } + return credentialDenyPaths{ + Paths: out, + Carveouts: credentialCarveoutPaths(out, carveouts), + EnsureDirs: credentialEnsureDirs(out, ensureDirs), + } +} + +// credentialTokenStorePaths returns the deny entries for one token-store path: +// the store, its lock siblings, its encryption-key sibling, and the directory +// it publishes new contents through. The names are fixed so an override outside +// Zero's config directory is protected by exact rules instead of hiding an +// arbitrary parent such as the workspace or /tmp. +func credentialTokenStorePaths(tokenPath string) []string { + if strings.TrimSpace(tokenPath) == "" { + return nil + } + paths := []string{ + tokenPath, + tokenPath + ".lockfile", + tokenPath + ".secret", + tokenPath + ".secret.lock", + // Left behind by a Zero older than the publication directories below. + tokenPath + ".tmp", + tokenPath + ".secret.tmp", + } + return append(paths, credentialPublicationDirs(tokenPath)...) +} + +// credentialPublicationDirs are the per-store directories the OAuth stores +// create their randomly-named temporary file in (see oauth.PublicationDir) — +// one for the token blob, one for its encryption key. The directory NAME is +// derived from the store path so the profile can deny it up front, while the +// random name inside it is what keeps a sandboxed same-user process from +// opening, or renaming away, the file that briefly holds the plaintext. +func credentialPublicationDirs(tokenPath string) []string { + if strings.TrimSpace(tokenPath) == "" { + return nil + } + return []string{ + tokenPath + credentialPublicationDirSuffix, + tokenPath + ".secret" + credentialPublicationDirSuffix, + } +} + +// credentialPublicationDirSuffix mirrors oauth.PublicationDirSuffix, duplicated +// because internal/mcp depends on this package and internal/oauth must stay +// importable from both. +const credentialPublicationDirSuffix = ".publish" + +// pathsOutsideRoots drops every path that lies within one of roots. +func pathsOutsideRoots(paths []string, roots []string) []string { + if len(paths) == 0 || len(roots) == 0 { + return paths + } + out := make([]string, 0, len(paths)) + for _, path := range paths { + if credentialPathReincluded(roots, path) { + continue + } + out = append(out, path) + } + return out +} + +func credentialPathReincluded(allowRoots []string, path string) bool { + for _, allow := range allowRoots { + if pathWithinRoot(allow, path) { + return true + } + } + return false +} + +// credentialCarveoutPaths keeps only the carveouts that sit inside a path that +// is actually denied, so an AllowRead opt-out that removed the deny does not +// leave a stray allow-back rule behind. +func credentialCarveoutPaths(denied []string, carveouts []string) []string { + if len(carveouts) == 0 { + return nil + } + out := make([]string, 0, len(carveouts)) + for _, carveout := range normalizeProfilePaths(carveouts) { + for _, deny := range denied { + if carveout != deny && pathWithinRoot(deny, carveout) { + out = append(out, carveout) break } } - if !reincluded { - out = append(out, path) + } + return dedupeStrings(out) +} + +// credentialEnsureDirs keeps only the directories that are still denied, so the +// sandbox never creates a directory it is not going to mask. +func credentialEnsureDirs(denied []string, ensureDirs []string) []string { + if len(ensureDirs) == 0 { + return nil + } + out := make([]string, 0, len(ensureDirs)) + for _, dir := range normalizeProfilePaths(ensureDirs) { + for _, deny := range denied { + if dir == deny { + out = append(out, dir) + break + } } } - return out + return dedupeStrings(out) } -// resolveCredentialOverridePath mirrors the token stores' own override +// resolveCredentialOverridePaths mirrors the token stores' own override // resolution (oauth.ResolveStorePath, mcp.ResolveTokenStorePath — reimplemented -// here rather than imported because internal/mcp depends on this package): a -// relative override is resolved literally against the command directory that -// the child inherits, NOT tilde- -// expanded the way normalizeProfilePath expands other candidates. Using -// normalizeProfilePath here would derive a deny path that doesn't match -// where the store actually writes — e.g. ZERO_OAUTH_TOKENS_PATH=~/x resolves -// to /~/x on disk (the store never expands "~"), but normalizeProfilePath -// would deny $HOME/x instead, leaving the real file unprotected. -func resolveCredentialOverridePath(override string, baseDir string) string { +// here rather than imported because internal/mcp depends on this package): the +// value is used literally, NOT tilde-expanded the way normalizeProfilePath +// expands other candidates. Using normalizeProfilePath here would derive a deny +// path that doesn't match where the store actually writes — e.g. +// ZERO_OAUTH_TOKENS_PATH=~/x resolves to /~/x on disk (the store never +// expands "~"), but normalizeProfilePath would deny $HOME/x instead, leaving +// the real file unprotected. +// +// A relative value yields one candidate per base dir (see +// credentialOverrideBaseDirs) because the process that resolves it is not +// necessarily the one that writes the store. +func resolveCredentialOverridePaths(override string, baseDirs []string) []string { override = strings.TrimSpace(override) if override == "" { - return "" + return nil } if filepath.IsAbs(override) { - return filepath.Clean(override) + return []string{filepath.Clean(override)} } - baseDir = strings.TrimSpace(baseDir) - if baseDir == "" { - var err error - baseDir, err = os.Getwd() - if err != nil { - return "" + out := make([]string, 0, len(baseDirs)) + for _, baseDir := range baseDirs { + if strings.TrimSpace(baseDir) == "" { + continue } + out = append(out, filepath.Clean(filepath.Join(baseDir, override))) } - return filepath.Clean(filepath.Join(baseDir, override)) + return dedupeStrings(out) } // userGitConfigReadPaths returns the user's global git config FILES so a diff --git a/internal/sandbox/profile_test.go b/internal/sandbox/profile_test.go index 12b2d67c2..38a7edee8 100644 --- a/internal/sandbox/profile_test.go +++ b/internal/sandbox/profile_test.go @@ -47,7 +47,12 @@ func TestCredentialDeniesMatchTokenStoreFallbacks(t *testing.T) { } } -func TestCredentialDeniesMatchRelativeTokenOverridesAtCommandDir(t *testing.T) { +// TestCredentialDeniesMatchRelativeTokenOverridesFromStoreResolution runs the +// sandboxed command from a DIFFERENT directory than the Zero process, which is +// what makes this meaningful: the stores resolve a relative override with +// filepath.Abs against the process working directory, so that is the path the +// profile has to deny. +func TestCredentialDeniesMatchRelativeTokenOverridesFromStoreResolution(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") } @@ -60,7 +65,9 @@ func TestCredentialDeniesMatchRelativeTokenOverridesAtCommandDir(t *testing.T) { if err != nil { t.Fatal(err) } - if err := os.Chdir(commandDir); err != nil { + // The Zero process stays in the workspace root while the command runs in the + // nested directory, so the two resolutions differ. + if err := os.Chdir(workspace); err != nil { t.Fatal(err) } defer func() { _ = os.Chdir(originalDir) }() diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 38a5eda57..3c77088e2 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -661,6 +661,10 @@ func seatbeltProfileFromPermissionProfile(profile PermissionProfile, policy Poli writeRule, } rules = append(rules, denyReadRules(profile.FileSystem)...) + // SBPL is last-match-wins, so the carveouts MUST follow the deny rules above: + // they re-include the supported non-secret subtrees of a directory-level + // credential deny (Zero's user plugin/specialist/command roots). + rules = append(rules, denyReadCarveoutRules(profile.FileSystem)...) rules = append(rules, writeRootCarveoutDenyRules(profile.FileSystem)...) rules = append(rules, denyWriteRulesFromPaths(profile.FileSystem.DenyWrite)...) rules = append(rules, networkRule) @@ -791,6 +795,35 @@ func denyReadRules(fs FileSystemPolicy) []string { return denySeatbeltPathRules("file-read*", dedupeStrings(append(append([]string{}, fs.DenyRead...), fs.DenyReadIfExists...))) } +// denyReadCarveoutRules re-allows reads for the non-secret subtrees of a denied +// credential directory. Writes are untouched: the credential directory is not a +// write root, so the profile's write rule keeps denying them. Only +// DenyReadCarveouts entries are emitted, and the profile builder derives those +// exclusively from Zero's own config directory (never from a user-configured +// DenyRead root), so no user deny is weakened here. +func denyReadCarveoutRules(fs FileSystemPolicy) []string { + resolved := normalizeProfilePaths(fs.DenyReadCarveouts) + if len(resolved) == 0 { + return nil + } + out := make([]string, 0, len(resolved)+1) + for _, path := range resolved { + escaped := sandboxProfileString(path) + out = append(out, + `(allow file-read* file-test-existence (subpath "`+escaped+`"))`, + `(allow file-read* file-test-existence (literal "`+escaped+`"))`, + ) + } + // Resolving a path into the carveout also needs stat on its ancestors, and the + // deny rule above covers the denied directory itself. This grants metadata + // only, so the denied directory stays unreadable and unlistable — the same + // split seatbeltReadRule already relies on for deeply nested read roots. + if ancestors := seatbeltAncestorMetadataRule(resolved); ancestors != "" { + out = append(out, ancestors) + } + return out +} + func writeRootCarveoutDenyRules(fs FileSystemPolicy) []string { if fs.Kind != FileSystemRestricted { return nil diff --git a/internal/sandbox/runner_linux_integration_test.go b/internal/sandbox/runner_linux_integration_test.go index f542d2f6b..560c9c2f8 100644 --- a/internal/sandbox/runner_linux_integration_test.go +++ b/internal/sandbox/runner_linux_integration_test.go @@ -105,6 +105,93 @@ func TestLinuxHelperRealSandboxSmoke(t *testing.T) { } }) + // The mid-session race the EnsureDenyReadDirs contract exists for: nothing + // under $XDG_CONFIG_HOME exists when the plan is built, so bubblewrap would + // have had no mount destination to mask. The sandbox creates Zero's own + // directory first, and the token written by this test WHILE the sandboxed + // command is already running must stay invisible to it. + t.Run("credential store created during the session stays hidden", func(t *testing.T) { + raceHome := t.TempDir() + raceConfig := filepath.Join(raceHome, "config") + raceStore := filepath.Join(raceConfig, "zero") + tokenPath := filepath.Join(raceStore, "oauth-tokens.json") + raceRoot := t.TempDir() + started := filepath.Join(raceRoot, "started") + ready := filepath.Join(raceRoot, "ready") + done := make(chan struct{}) + go func() { + defer close(done) + deadline := time.Now().Add(20 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(started); err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + // The namespace is assembled by now, so this write is the concurrent + // login the sandbox must not be able to read. + if err := os.MkdirAll(raceStore, 0o700); err == nil { + _ = os.WriteFile(tokenPath, []byte("racyleak\n"), 0o600) + } + _ = os.WriteFile(ready, []byte("1\n"), 0o600) + }() + raceEngine := NewEngine(EngineOptions{WorkspaceRoot: raceRoot, Policy: DefaultPolicy(), Backend: backend}) + output, _ := runLinuxSandboxSmokeCommand(t, raceEngine, CommandSpec{ + Name: "/bin/sh", + Args: []string{"-c", strings.Join([]string{ + "echo 1 > " + shellQuote(started), + "i=0", + "while [ ! -e " + shellQuote(ready) + " ] && [ \"$i\" -lt 2000 ]; do i=$((i+1)); sleep 0.01; done", + "if cat " + shellQuote(tokenPath) + " 2>/dev/null | grep -q racyleak; then echo MIDSESSION_CREDENTIAL_READ_SUCCEEDED; fi", + }, "\n")}, + Dir: raceRoot, + Env: []string{"HOME=" + raceHome, "XDG_CONFIG_HOME=" + raceConfig}, + }) + <-done + if strings.Contains(string(output), "MIDSESSION_CREDENTIAL_READ_SUCCEEDED") { + t.Fatalf("token created during the session was readable: %s", output) + } + if _, err := os.Stat(tokenPath); err != nil { + t.Fatalf("test did not create the token it probes for: %v", err) + } + }) + + // The user plugin root shares the denied credential directory, and its + // commands are executed through this sandbox, so it must stay readable. + t.Run("user plugin root inside the denied config dir stays readable", func(t *testing.T) { + pluginHome := t.TempDir() + pluginConfig := filepath.Join(pluginHome, "config") + pluginRoot := filepath.Join(pluginConfig, "zero", "plugins", "demo") + if err := os.MkdirAll(pluginRoot, 0o700); err != nil { + t.Fatalf("MkdirAll plugin root: %v", err) + } + manifest := filepath.Join(pluginRoot, "plugin.json") + if err := os.WriteFile(manifest, []byte(`{"name":"demo"}`+"\n"), 0o600); err != nil { + t.Fatalf("WriteFile plugin manifest: %v", err) + } + secret := filepath.Join(pluginConfig, "zero", "oauth-tokens.json") + if err := os.WriteFile(secret, []byte(`{"access_token":"leaked"}`+"\n"), 0o600); err != nil { + t.Fatalf("WriteFile zero tokens: %v", err) + } + pluginEngine := NewEngine(EngineOptions{WorkspaceRoot: root, Policy: DefaultPolicy(), Backend: backend}) + output, runErr := runLinuxSandboxSmokeCommand(t, pluginEngine, CommandSpec{ + Name: "/bin/sh", + Args: []string{"-c", strings.Join([]string{ + "set -eu", + "grep -q demo " + shellQuote(manifest), + "if cat " + shellQuote(secret) + " 2>/dev/null | grep -q leaked; then echo CARVEOUT_LEAKED_SIBLING; exit 42; fi", + }, "\n")}, + Dir: root, + Env: []string{"HOME=" + pluginHome, "XDG_CONFIG_HOME=" + pluginConfig}, + }) + if runErr != nil { + t.Fatalf("plugin root inside the denied config dir was not readable: %v\n%s", runErr, output) + } + if strings.Contains(string(output), "CARVEOUT_LEAKED_SIBLING") { + t.Fatalf("carveout exposed a credential sibling: %s", output) + } + }) + t.Run("credential store created after launch stays hidden on the next run", func(t *testing.T) { lateHome := t.TempDir() lateConfig := filepath.Join(lateHome, "config") diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 8cad62b2c..26df8f815 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -508,6 +508,45 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { } } +// TestSeatbeltProfileRendersCredentialBaselineAndCarveouts pins the rendering of +// the credential baseline, which the struct-level tests do not cover: a +// DenyReadIfExists entry must reach the profile as a real deny rule (dropping +// the field from denyReadRules would silently make every credential store +// readable again on macOS), and a DenyReadCarveouts entry must be re-allowed +// AFTER it, since SBPL is last-match-wins. +func TestSeatbeltProfileRendersCredentialBaselineAndCarveouts(t *testing.T) { + credentialDir := filepath.Join(t.TempDir(), "zero") + pluginRoot := filepath.Join(credentialDir, "plugins") + if err := os.MkdirAll(pluginRoot, 0o700); err != nil { + t.Fatal(err) + } + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{"/"}, + WriteRoots: []WritableRoot{{Root: "/repo"}}, + DenyReadIfExists: []string{credentialDir}, + DenyReadCarveouts: []string{pluginRoot}, + AllowTemp: true, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + denyRule := `(deny file-read* (subpath "` + sandboxProfileString(normalizeProfilePath(credentialDir)) + `"))` + allowRule := `(allow file-read* file-test-existence (subpath "` + sandboxProfileString(normalizeProfilePath(pluginRoot)) + `"))` + denyIdx := strings.Index(sbpl, denyRule) + allowIdx := strings.Index(sbpl, allowRule) + if denyIdx < 0 { + t.Fatalf("Seatbelt profile missing credential baseline deny %q:\n%s", denyRule, sbpl) + } + if allowIdx < 0 { + t.Fatalf("Seatbelt profile missing carveout allow %q:\n%s", allowRule, sbpl) + } + if allowIdx < denyIdx { + t.Fatalf("carveout allow (%d) must follow the deny (%d) to win under last-match-wins:\n%s", allowIdx, denyIdx, sbpl) + } +} + // 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 a62bd8ca5e01c269c02f8bdad11ce9868eba94e8 Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 21:05:53 +0000 Subject: [PATCH 15/18] fix(sandbox): close credential deny review gaps Preserve process credential roots alongside command overrides while limiting host precreation and read carveouts to trusted process-derived paths. Canonicalize future credential roots through existing path aliases, reject symlink carveouts, and collapse automatic masks already covered by user or automatic directory denies. Normalize the policy golden and add regression coverage across profile, Seatbelt, and bubblewrap rendering. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno --- internal/cli/sandbox_test.go | 31 +++- internal/sandbox/export_test.go | 4 +- internal/sandbox/linux_helper.go | 27 +-- internal/sandbox/linux_helper_test.go | 35 ++++ internal/sandbox/manager_test.go | 183 +++++++++++++++++++- internal/sandbox/profile.go | 229 ++++++++++++++++++++------ internal/sandbox/runner.go | 3 +- internal/sandbox/runner_test.go | 35 ++++ 8 files changed, 469 insertions(+), 78 deletions(-) diff --git a/internal/cli/sandbox_test.go b/internal/cli/sandbox_test.go index 5c5500021..3415f3ae5 100644 --- a/internal/cli/sandbox_test.go +++ b/internal/cli/sandbox_test.go @@ -554,17 +554,40 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp fileSystem, _ := profile["fileSystem"].(map[string]any) wantDenyRead := []string(nil) if runtime.GOOS != "windows" { + credentialHome := emptyHome + if resolved, err := filepath.EvalSymlinks(emptyHome); err == nil { + credentialHome = resolved + } wantDenyRead = []string{ - filepath.Join(emptyHome, ".aws"), - filepath.Join(emptyHome, ".config", "gcloud"), - filepath.Join(emptyHome, ".azure"), - filepath.Join(emptyHome, ".config", "zero"), + filepath.Join(credentialHome, ".aws"), + filepath.Join(credentialHome, ".config", "gcloud"), + filepath.Join(credentialHome, ".azure"), + filepath.Join(credentialHome, ".config", "zero"), } } if gotDenyRead := jsonStringSlice(fileSystem["denyReadIfExists"]); !reflect.DeepEqual(gotDenyRead, wantDenyRead) { t.Fatalf("manager credential deny baseline = %#v, want %#v", gotDenyRead, wantDenyRead) } + wantCarveouts := []string(nil) + wantEnsureDirs := []string(nil) + if runtime.GOOS != "windows" { + zeroDir := wantDenyRead[len(wantDenyRead)-1] + wantCarveouts = []string{ + filepath.Join(zeroDir, "plugins"), + filepath.Join(zeroDir, "specialists"), + filepath.Join(zeroDir, "commands"), + } + wantEnsureDirs = []string{zeroDir} + } + if gotCarveouts := jsonStringSlice(fileSystem["denyReadCarveouts"]); !reflect.DeepEqual(gotCarveouts, wantCarveouts) { + t.Fatalf("manager credential carveouts = %#v, want %#v", gotCarveouts, wantCarveouts) + } + if gotEnsureDirs := jsonStringSlice(fileSystem["ensureDenyReadDirs"]); !reflect.DeepEqual(gotEnsureDirs, wantEnsureDirs) { + t.Fatalf("manager credential ensure dirs = %#v, want %#v", gotEnsureDirs, wantEnsureDirs) + } delete(fileSystem, "denyReadIfExists") + delete(fileSystem, "denyReadCarveouts") + delete(fileSystem, "ensureDenyReadDirs") fileSystem["readRoots"] = filterJSONStringRoots(fileSystem["readRoots"], tempRoots) fileSystem["writeRoots"] = filterJSONWriteRoots(fileSystem["writeRoots"], tempRoots) normalized, err := json.MarshalIndent(value, "", " ") diff --git a/internal/sandbox/export_test.go b/internal/sandbox/export_test.go index 88fba2a99..62ed5a2ef 100644 --- a/internal/sandbox/export_test.go +++ b/internal/sandbox/export_test.go @@ -60,9 +60,9 @@ func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) } } fs.DenyRead = normalizeProfilePaths(policy.DenyRead) - credentials := credentialDenyReadPaths(policy, "", os.Environ()) + credentials := finalizeCredentialDenyPaths(credentialDenyReadPaths(policy, "", os.Environ()), fs.DenyRead) fs.DenyReadIfExists = credentials.Paths - fs.DenyReadCarveouts = pathsOutsideRoots(credentials.Carveouts, fs.DenyRead) + fs.DenyReadCarveouts = credentials.Carveouts fs.EnsureDenyReadDirs = credentials.EnsureDirs fs.DenyWrite = normalizeProfilePaths(policy.DenyWrite) return PermissionProfile{ diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index a33f1974b..9edf1558e 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -279,11 +279,9 @@ func buildLinuxBwrapFilesystemPlan(profile PermissionProfile) linuxBwrapFilesyst for _, path := range fs.DenyRead { args = appendUnreadableLinuxPathArgs(args, path, fs.DenyReadCarveouts) } - // Zero owns these directories, so create the ones that are missing before the - // namespace is assembled: bubblewrap cannot mount over an absent path, and a - // directory that appears afterwards (a concurrent login writing a token - // store) would otherwise be readable through the live read-only host bind for - // the rest of a long-lived sandbox session. + // The profile includes only trusted, process-environment-derived directories + // here. Command-controlled credential roots remain deny-if-present and must + // never cause host filesystem mutations before sandbox launch. ensureLinuxDenyReadDirs(fs.EnsureDenyReadDirs) for _, path := range fs.DenyReadIfExists { if !pathExists(path) { @@ -391,7 +389,7 @@ func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []strin // --remount-ro, which is what freezes the tmpfs. args = append(args, "--perms", "111", "--tmpfs", path) for _, carveout := range nested { - if pathExists(carveout) { + if info, err := os.Lstat(carveout); err == nil && info.IsDir() { args = append(args, "--ro-bind", carveout, carveout) } } @@ -404,23 +402,14 @@ func nestedCarveoutPaths(root string, carveouts []string) []string { if len(carveouts) == 0 { return nil } - out := make([]string, 0, len(carveouts)) - for _, carveout := range carveouts { - carveout = strings.TrimSpace(carveout) - if carveout == "" || carveout == root { - continue - } - if pathWithinRoot(root, carveout) { - out = append(out, carveout) - } - } + out := credentialCarveoutPaths([]string{root}, carveouts) sort.SliceStable(out, func(i, j int) bool { return pathDepth(out[i]) < pathDepth(out[j]) }) return dedupeStrings(out) } -// ensureLinuxDenyReadDirs creates the Zero-owned directories a deny mask needs -// to exist for. Best effort: a failure just leaves the path unmasked, exactly as -// before, and never blocks the command. +// ensureLinuxDenyReadDirs creates trusted Zero-process directories a deny mask +// needs to exist for. Best effort: a failure leaves the path unmasked and never +// blocks the command. func ensureLinuxDenyReadDirs(dirs []string) { for _, dir := range dirs { dir = strings.TrimSpace(dir) diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index 0cab99c53..4d04c3a21 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -365,6 +365,41 @@ func TestLinuxBwrapKeepsCarveoutsReachableInsideMaskedDir(t *testing.T) { } } +func TestLinuxBwrapDoesNotBindSymlinkCarveout(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + root := t.TempDir() + credentialDir := filepath.Join(root, "config", "zero") + if err := os.MkdirAll(credentialDir, 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(credentialDir, "oauth-tokens.json") + if err := os.WriteFile(secret, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + pluginRoot := filepath.Join(credentialDir, "plugins") + if err := os.Symlink(secret, pluginRoot); err != nil { + t.Fatal(err) + } + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: root}}, + DenyReadIfExists: []string{credentialDir}, + DenyReadCarveouts: []string{pluginRoot}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + + plan := buildLinuxBwrapFilesystemPlan(profile) + assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", credentialDir, "--remount-ro", credentialDir) + if argsContainSequence(plan.Args, "--ro-bind", pluginRoot, pluginRoot) || argsContainSequence(plan.Args, "--ro-bind", secret, secret) { + t.Fatalf("symlink carveout was rebound into credential mask: %#v", plan.Args) + } +} + func TestLinuxBwrapUnrestrictedFilesystemUsesWritableHostRoot(t *testing.T) { profile := PermissionProfile{ FileSystem: FileSystemPolicy{ diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 7b85a6c74..072ebc59b 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -539,7 +539,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { commandDir := t.TempDir() override := "~/relative-tilde-tokens.json" - options := credentialPathOptionsFromEnvironment(commandDir, []string{ + options := credentialPathOptionsFromEnvironment(credentialCommandBaseDirs(commandDir), []string{ "HOME=", "USERPROFILE=" + filepath.Join(commandDir, "profile-home"), "XDG_CONFIG_HOME=~/literal-xdg", @@ -579,7 +579,7 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { func TestCredentialDenyReadPathsInConfigDirMatchesLiteralXDGResolution(t *testing.T) { configDir := "~/literal-xdg" commandDir := t.TempDir() - resolvedConfigDirs := credentialPathOptionsFromEnvironment(commandDir, []string{"XDG_CONFIG_HOME=" + configDir}).ZeroConfigDirs + resolvedConfigDirs := credentialPathOptionsFromEnvironment(credentialCommandBaseDirs(commandDir), []string{"XDG_CONFIG_HOME=" + configDir}).ZeroConfigDirs paths := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDirs: resolvedConfigDirs}, nil).Paths want := filepath.Join(commandDir, configDir, "zero") @@ -652,6 +652,7 @@ func TestBuildCommandPlanDeniesRelativeOverrideAtProcessAndCommandDir(t *testing t.Fatal(err) } t.Cleanup(func() { _ = os.Chdir(originalDir) }) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", "tokens.json") engine := NewEngine(EngineOptions{ WorkspaceRoot: workspace, @@ -684,6 +685,184 @@ func TestBuildCommandPlanDeniesRelativeOverrideAtProcessAndCommandDir(t *testing } } +func TestCredentialCarveoutsUseNormalizedParentForMissingChildren(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + realConfig := filepath.Join(t.TempDir(), "real-config") + if err := os.MkdirAll(realConfig, 0o700); err != nil { + t.Fatal(err) + } + alias := filepath.Join(t.TempDir(), "config-link") + if err := os.Symlink(realConfig, alias); err != nil { + t.Fatal(err) + } + + credentials := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDirs: []string{alias}}, nil) + wantZeroDir := normalizeProfilePath(filepath.Join(realConfig, "zero")) + if !stringSliceContains(credentials.Paths, wantZeroDir) { + t.Fatalf("credential deny paths = %#v, want canonical missing Zero dir %q", credentials.Paths, wantZeroDir) + } + if _, err := os.Stat(wantZeroDir); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("test requires the Zero dir to remain absent, got %v", err) + } + for _, name := range zeroConfigReadCarveoutNames { + want := filepath.Join(wantZeroDir, name) + if !stringSliceContains(credentials.Carveouts, want) { + t.Errorf("credential carveouts = %#v, want lexical child of canonical root %q", credentials.Carveouts, want) + } + } +} + +func TestCredentialCarveoutsRejectSymlinks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + configDir := t.TempDir() + zeroDir := filepath.Join(configDir, "zero") + if err := os.MkdirAll(zeroDir, 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(zeroDir, "oauth-tokens.json") + if err := os.WriteFile(secret, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + pluginRoot := filepath.Join(zeroDir, "plugins") + if err := os.Symlink(secret, pluginRoot); err != nil { + t.Fatal(err) + } + + credentials := credentialDenyReadPathsIn(credentialPathOptions{ZeroConfigDirs: []string{configDir}}, nil) + normalizedZeroDir := normalizeProfilePath(zeroDir) + if stringSliceContains(credentials.Carveouts, filepath.Join(normalizedZeroDir, "plugins")) || stringSliceContains(credentials.Carveouts, normalizeProfilePath(secret)) { + t.Fatalf("credential carveouts = %#v, must not re-allow a symlink or its credential target", credentials.Carveouts) + } + if want := filepath.Join(normalizedZeroDir, "specialists"); !stringSliceContains(credentials.Carveouts, want) { + t.Fatalf("credential carveouts = %#v, want unrelated fixed subtree %q", credentials.Carveouts, want) + } +} + +func TestPermissionProfileUnionsProcessAndCommandCredentialRootsWithoutCreatingCommandDirs(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + parentHome := t.TempDir() + parentConfig := filepath.Join(parentHome, "config") + parentToken := filepath.Join(parentHome, "trusted-store", "tokens.json") + t.Setenv("HOME", parentHome) + t.Setenv("USERPROFILE", parentHome) + t.Setenv("XDG_CONFIG_HOME", parentConfig) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", parentToken) + t.Setenv("ZERO_MCP_OAUTH_TOKENS_PATH", "") + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "") + + workspace := t.TempDir() + childHome := filepath.Join(workspace, "child-home") + childConfig := filepath.Join(childHome, "config") + childToken := filepath.Join(workspace, "child-store", "tokens.json") + engine := NewEngine(EngineOptions{ + WorkspaceRoot: workspace, + Policy: DefaultPolicy(), + Backend: Backend{Name: BackendUnavailable, Platform: runtime.GOOS}, + }) + plan, err := engine.BuildCommandPlan(CommandSpec{ + Name: "true", + Dir: workspace, + Env: []string{ + "HOME=" + childHome, + "XDG_CONFIG_HOME=" + childConfig, + "ZERO_OAUTH_TOKENS_PATH=" + childToken, + }, + }) + if err != nil { + t.Fatal(err) + } + fs := plan.PermissionProfile.FileSystem + parentZero := filepath.Join(parentConfig, "zero") + childZero := filepath.Join(childConfig, "zero") + for _, want := range []string{parentZero, childZero, parentToken, childToken} { + if !stringSliceContains(fs.DenyReadIfExists, normalizeProfilePath(want)) { + t.Errorf("DenyReadIfExists = %#v, want process/command root %q", fs.DenyReadIfExists, want) + } + } + for _, want := range []string{parentZero, parentToken + credentialPublicationDirSuffix} { + if !stringSliceContains(fs.EnsureDenyReadDirs, normalizeProfilePath(want)) { + t.Errorf("EnsureDenyReadDirs = %#v, want trusted process dir %q", fs.EnsureDenyReadDirs, want) + } + } + for _, unwanted := range []string{childZero, childToken + credentialPublicationDirSuffix} { + if stringSliceContains(fs.EnsureDenyReadDirs, normalizeProfilePath(unwanted)) { + t.Errorf("EnsureDenyReadDirs = %#v, must not include command-controlled dir %q", fs.EnsureDenyReadDirs, unwanted) + } + } + + _ = buildLinuxBwrapFilesystemPlan(plan.PermissionProfile) + if info, err := os.Stat(parentZero); err != nil || !info.IsDir() { + t.Fatalf("trusted process credential dir was not created: info=%v err=%v", info, err) + } + for _, unwanted := range []string{childZero, childToken + credentialPublicationDirSuffix} { + if _, err := os.Stat(unwanted); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("command-controlled credential dir %q was created on the host: %v", unwanted, err) + } + } +} + +func TestPermissionProfileDropsAutomaticMasksCoveredByUserDeny(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", "") + t.Setenv("ZERO_MCP_OAUTH_TOKENS_PATH", "") + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "") + policy := DefaultPolicy() + policy.DenyRead = []string{home} + + profile := PermissionProfileFromPolicy(t.TempDir(), policy, nil) + fs := profile.FileSystem + if len(fs.DenyReadIfExists) != 0 || len(fs.DenyReadCarveouts) != 0 || len(fs.EnsureDenyReadDirs) != 0 { + t.Fatalf("automatic credential fields beneath user deny were retained: paths=%#v carveouts=%#v ensure=%#v", fs.DenyReadIfExists, fs.DenyReadCarveouts, fs.EnsureDenyReadDirs) + } + zeroDir := filepath.Join(home, ".config", "zero") + plan := buildLinuxBwrapFilesystemPlan(profile) + if stringSliceContains(plan.Args, zeroDir) { + t.Fatalf("bwrap args contain a nested automatic mount beneath user deny %q: %#v", home, plan.Args) + } +} + +func TestPermissionProfileDropsRedundantAutomaticMasksInsideZeroDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + home := t.TempDir() + configHome := filepath.Join(home, ".config") + zeroDir := filepath.Join(configHome, "zero") + tokenPath := filepath.Join(zeroDir, "oauth-tokens.json") + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("XDG_CONFIG_HOME", configHome) + t.Setenv("ZERO_OAUTH_TOKENS_PATH", tokenPath) + t.Setenv("ZERO_MCP_OAUTH_TOKENS_PATH", "") + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "") + + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + fs := profile.FileSystem + if !stringSliceContains(fs.DenyReadIfExists, normalizeProfilePath(zeroDir)) { + t.Fatalf("DenyReadIfExists = %#v, want Zero directory mask", fs.DenyReadIfExists) + } + for _, path := range credentialTokenStorePaths(tokenPath) { + if stringSliceContains(fs.DenyReadIfExists, normalizeProfilePath(path)) { + t.Fatalf("DenyReadIfExists = %#v, redundant nested mask %q must be covered by Zero directory", fs.DenyReadIfExists, path) + } + } + if stringSliceContains(fs.EnsureDenyReadDirs, normalizeProfilePath(tokenPath+credentialPublicationDirSuffix)) { + t.Fatalf("EnsureDenyReadDirs = %#v, redundant nested publication dir must not be created", fs.EnsureDenyReadDirs) + } +} + func TestPermissionProfileIncludesZeroCredentialPaths(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index c1b0e418a..f60072108 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -115,9 +115,7 @@ func permissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Sco } userDenyRead := normalizeProfilePaths(policy.DenyRead) credentials := credentialDenyReadPaths(policy, credentialCommandDir, credentialEnv) - // A carveout re-includes reads, so it must never reach inside a path the USER - // denied: their configuration outranks Zero's automatic baseline. - credentials.Carveouts = pathsOutsideRoots(credentials.Carveouts, userDenyRead) + credentials = finalizeCredentialDenyPaths(credentials, userDenyRead) return PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, @@ -173,13 +171,14 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop } // credentialDenyPaths is the credential baseline a profile derives from the -// environment: the paths to deny reads on, the non-secret subtrees that stay -// readable inside them, and the Zero-owned directories a mount-based backend -// may create so its mask actually exists. +// environment: the paths to deny reads on, the known directory paths among +// them, the trusted non-secret subtrees that stay readable, and the trusted +// Zero-owned directories a mount-based backend may create so its mask exists. type credentialDenyPaths struct { Paths []string Carveouts []string EnsureDirs []string + Dirs []string } // credentialDenyReadPaths returns default deny-read entries for well-known @@ -197,9 +196,9 @@ type credentialDenyPaths struct { // - Candidates are emitted whether or not they currently exist on disk. // Pathname-policy backends such as Seatbelt can enforce future paths; // mount-based Linux masks a path only if it exists when the namespace is -// assembled, which is why the directories Zero owns are also reported as -// EnsureDirs (the backend creates them, so the mask is always present) and -// why third-party stores such as ~/.aws stay best-effort there. +// assembled, which is why directories derived from Zero's own process +// environment are also reported as EnsureDirs (command-controlled roots +// never are) and why third-party stores such as ~/.aws stay best-effort. // - Zero's own config directory is denied WHOLE, with the supported // non-secret subtrees carved back out. Only a directory-level rule covers // the temporary names its stores publish through and the files a concurrent @@ -214,8 +213,37 @@ func credentialDenyReadPaths(policy Policy, commandDir string, commandEnv []stri if runtime.GOOS == "windows" { return credentialDenyPaths{} } - options := credentialPathOptionsFromEnvironment(commandDir, commandEnv) - return credentialDenyReadPathsIn(options, policy.AllowRead) + + // Only roots derived from Zero's own process environment may produce + // carveouts or host directories. CommandSpec.Env can contain project-controlled + // MCP environment overrides, so it contributes deny-if-present paths only. + processBaseDirs := credentialProcessBaseDirs() + trusted := credentialDenyReadPathsIn( + credentialPathOptionsFromEnvironment(processBaseDirs, os.Environ()), + policy.AllowRead, + ) + appendUntrusted := func(options credentialPathOptions) { + paths := credentialDenyReadPathsIn(options, policy.AllowRead) + trusted.Paths = append(trusted.Paths, paths.Paths...) + trusted.Dirs = append(trusted.Dirs, paths.Dirs...) + } + commandBaseDirs := credentialCommandBaseDirs(commandDir) + if len(commandBaseDirs) > 0 { + // A nested Zero inherits the process environment but resolves relative + // values from the sandboxed command's cwd. + appendUntrusted(credentialPathOptionsFromEnvironment(commandBaseDirs, os.Environ())) + } + if commandEnv != nil { + // A supplied environment may also be consumed by code in the running Zero + // process before exec, so conservatively deny both process- and + // command-relative resolutions. These remain untrusted: neither resolution + // contributes carveouts or host directory creation. + baseDirs := dedupeStrings(append(append([]string{}, processBaseDirs...), commandBaseDirs...)) + appendUntrusted(credentialPathOptionsFromEnvironment(baseDirs, commandEnv)) + } + trusted.Paths = dedupeStrings(trusted.Paths) + trusted.Dirs = dedupeStrings(trusted.Dirs) + return trusted } // zeroConfigReadCarveoutNames are the supported non-secret subtrees of @@ -226,32 +254,25 @@ func credentialDenyReadPaths(policy Policy, commandDir string, commandEnv []stri // files directly under /zero, not in these subtrees. var zeroConfigReadCarveoutNames = []string{"plugins", "specialists", "commands"} -// credentialOverrideBaseDirs returns the directories a RELATIVE credential -// override is resolved against, most authoritative first. -// -// The stores themselves call filepath.Abs (oauth.ResolveStorePath, -// mcp.ResolveTokenStorePath), i.e. they resolve against the Zero PROCESS -// working directory, so that is the path that must be denied. The command -// directory is kept as a second candidate because a sandboxed child (including -// a nested Zero) resolves the inherited value against its own cwd instead. -// Denying both costs two rules and closes the mismatch in either direction. -func credentialOverrideBaseDirs(commandDir string) []string { - var dirs []string +// credentialProcessBaseDirs returns the directory the running Zero process uses +// to resolve relative credential overrides. +func credentialProcessBaseDirs() []string { if cwd, err := os.Getwd(); err == nil && strings.TrimSpace(cwd) != "" { - dirs = append(dirs, filepath.Clean(cwd)) + return []string{filepath.Clean(cwd)} } + return nil +} + +// credentialCommandBaseDirs returns the directory a sandboxed child (including +// a nested Zero) uses to resolve inherited relative credential overrides. +func credentialCommandBaseDirs(commandDir string) []string { if dir := strings.TrimSpace(commandDir); dir != "" { - dirs = append(dirs, filepath.Clean(dir)) + return []string{filepath.Clean(dir)} } - return dedupeStrings(dirs) + return nil } -func credentialPathOptionsFromEnvironment(commandDir string, commandEnv []string) credentialPathOptions { - env := commandEnv - if env == nil { - env = os.Environ() - } - baseDirs := credentialOverrideBaseDirs(commandDir) +func credentialPathOptionsFromEnvironment(baseDirs []string, env []string) credentialPathOptions { homes := resolveCredentialOverridePaths(credentialEnvValue(env, "HOME"), baseDirs) if len(homes) == 0 { homes = resolveCredentialOverridePaths(credentialEnvValue(env, "USERPROFILE"), baseDirs) @@ -302,15 +323,18 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string var candidates []string var carveouts []string var ensureDirs []string + var dirs []string for _, home := range options.Homes { if strings.TrimSpace(home) == "" { continue } - candidates = append(candidates, + homeDirs := []string{ filepath.Join(home, ".aws"), filepath.Join(home, ".config", "gcloud"), filepath.Join(home, ".azure"), - ) + } + candidates = append(candidates, homeDirs...) + dirs = append(dirs, homeDirs...) } candidates = append(candidates, options.GoogleCredentials...) for _, configDir := range options.ZeroConfigDirs { @@ -325,8 +349,16 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string // directory rule covers all three. Zero owns this directory, so it is also // an EnsureDir: bubblewrap cannot mask a path that is absent when the // namespace is assembled. - zeroDir := filepath.Join(configDir, "zero") + // Normalize the denied root first, then derive its fixed children + // lexically. This keeps nonexistent carveouts under the same canonical + // parent (for example macOS /var -> /private/var) without following a + // plugins/specialists/commands symlink into a credential file. + zeroDir := normalizeProfilePath(filepath.Join(configDir, "zero")) + if zeroDir == "" { + continue + } candidates = append(candidates, zeroDir) + dirs = append(dirs, zeroDir) ensureDirs = append(ensureDirs, zeroDir) for _, name := range zeroConfigReadCarveoutNames { carveouts = append(carveouts, filepath.Join(zeroDir, name)) @@ -334,14 +366,18 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string } for _, tokenPath := range options.OAuthTokens { candidates = append(candidates, credentialTokenStorePaths(tokenPath)...) - ensureDirs = append(ensureDirs, credentialPublicationDirs(tokenPath)...) + publicationDirs := credentialPublicationDirs(tokenPath) + dirs = append(dirs, publicationDirs...) + ensureDirs = append(ensureDirs, publicationDirs...) } for _, tokenPath := range options.MCPOAuthTokens { candidates = append(candidates, credentialTokenStorePaths(tokenPath)...) // The legacy store renames itself aside after importing into the unified // store, leaving a readable copy of the same tokens behind. candidates = append(candidates, tokenPath+".migrated") - ensureDirs = append(ensureDirs, credentialPublicationDirs(tokenPath)...) + publicationDirs := credentialPublicationDirs(tokenPath) + dirs = append(dirs, publicationDirs...) + ensureDirs = append(ensureDirs, publicationDirs...) } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) @@ -354,7 +390,8 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string return credentialDenyPaths{ Paths: out, Carveouts: credentialCarveoutPaths(out, carveouts), - EnsureDirs: credentialEnsureDirs(out, ensureDirs), + EnsureDirs: credentialRetainedDirs(out, ensureDirs), + Dirs: credentialRetainedDirs(out, normalizeProfilePaths(dirs)), } } @@ -415,6 +452,29 @@ func pathsOutsideRoots(paths []string, roots []string) []string { return out } +// pathsOutsideOverlappingRoots drops paths that contain, or are contained by, a +// root. A credential carveout is an allow-back rule, so either overlap could +// weaken a user deny after Seatbelt's last-match-wins evaluation. +func pathsOutsideOverlappingRoots(paths []string, roots []string) []string { + if len(paths) == 0 || len(roots) == 0 { + return paths + } + out := make([]string, 0, len(paths)) + for _, path := range paths { + overlaps := false + for _, root := range roots { + if pathWithinRoot(root, path) || pathWithinRoot(path, root) { + overlaps = true + break + } + } + if !overlaps { + out = append(out, path) + } + } + return out +} + func credentialPathReincluded(allowRoots []string, path string) bool { for _, allow := range allowRoots { if pathWithinRoot(allow, path) { @@ -432,7 +492,21 @@ func credentialCarveoutPaths(denied []string, carveouts []string) []string { return nil } out := make([]string, 0, len(carveouts)) - for _, carveout := range normalizeProfilePaths(carveouts) { + for _, entry := range carveouts { + carveout := normalizeProfilePathLexically(entry) + if carveout == "" { + continue + } + // A missing fixed subtree may be installed later by trusted host code, but + // an existing entry must be a real directory. In particular, never turn a + // plugins symlink into an allow rule for its credential-file target. + if info, err := os.Lstat(carveout); err == nil { + if !info.IsDir() { + continue + } + } else if !os.IsNotExist(err) { + continue + } for _, deny := range denied { if carveout != deny && pathWithinRoot(deny, carveout) { out = append(out, carveout) @@ -443,14 +517,13 @@ func credentialCarveoutPaths(denied []string, carveouts []string) []string { return dedupeStrings(out) } -// credentialEnsureDirs keeps only the directories that are still denied, so the -// sandbox never creates a directory it is not going to mask. -func credentialEnsureDirs(denied []string, ensureDirs []string) []string { - if len(ensureDirs) == 0 { +// credentialRetainedDirs keeps only directories that remain exact deny entries. +func credentialRetainedDirs(denied []string, dirs []string) []string { + if len(dirs) == 0 { return nil } - out := make([]string, 0, len(ensureDirs)) - for _, dir := range normalizeProfilePaths(ensureDirs) { + out := make([]string, 0, len(dirs)) + for _, dir := range dirs { for _, deny := range denied { if dir == deny { out = append(out, dir) @@ -461,6 +534,36 @@ func credentialEnsureDirs(denied []string, ensureDirs []string) []string { return dedupeStrings(out) } +// finalizeCredentialDenyPaths gives user denies precedence and removes nested +// automatic mounts that an outer automatic directory mask already covers. +// Children inside a retained carveout stay explicit denies, because the +// carveout re-allows that subtree. +func finalizeCredentialDenyPaths(credentials credentialDenyPaths, userDenyRead []string) credentialDenyPaths { + credentials.Paths = pathsOutsideRoots(credentials.Paths, userDenyRead) + credentials.Carveouts = pathsOutsideOverlappingRoots(credentials.Carveouts, userDenyRead) + credentials.Dirs = credentialRetainedDirs(credentials.Paths, credentials.Dirs) + credentials.Carveouts = credentialCarveoutPaths(credentials.Paths, credentials.Carveouts) + + paths := make([]string, 0, len(credentials.Paths)) + for _, path := range credentials.Paths { + covered := false + for _, dir := range credentials.Dirs { + if path != dir && pathWithinRoot(dir, path) && !credentialPathReincluded(credentials.Carveouts, path) { + covered = true + break + } + } + if !covered { + paths = append(paths, path) + } + } + credentials.Paths = dedupeStrings(paths) + credentials.Dirs = credentialRetainedDirs(credentials.Paths, credentials.Dirs) + credentials.Carveouts = credentialCarveoutPaths(credentials.Paths, credentials.Carveouts) + credentials.EnsureDirs = credentialRetainedDirs(credentials.Paths, credentials.EnsureDirs) + return credentials +} + // resolveCredentialOverridePaths mirrors the token stores' own override // resolution (oauth.ResolveStorePath, mcp.ResolveTokenStorePath — reimplemented // here rather than imported because internal/mcp depends on this package): the @@ -471,9 +574,8 @@ func credentialEnsureDirs(denied []string, ensureDirs []string) []string { // expands "~"), but normalizeProfilePath would deny $HOME/x instead, leaving // the real file unprotected. // -// A relative value yields one candidate per base dir (see -// credentialOverrideBaseDirs) because the process that resolves it is not -// necessarily the one that writes the store. +// A relative value yields one candidate per supplied base dir because the +// process that resolves it is not necessarily the one that writes the store. func resolveCredentialOverridePaths(override string, baseDirs []string) []string { override = strings.TrimSpace(override) if override == "" { @@ -549,6 +651,36 @@ func normalizeProfilePaths(entries []string) []string { } func normalizeProfilePath(entry string) string { + absolute := normalizeProfilePathLexically(entry) + if absolute == "" { + return "" + } + if resolved, err := filepath.EvalSymlinks(absolute); err == nil { + return resolved + } + + // EvalSymlinks requires the whole path to exist. Resolve the deepest existing + // ancestor and append the missing tail lexically so future deny paths still + // match canonical aliases such as macOS /var -> /private/var. + current := absolute + var tail []string + for { + parent := filepath.Dir(current) + if parent == current { + return filepath.Clean(absolute) + } + tail = append([]string{filepath.Base(current)}, tail...) + current = parent + if resolved, err := filepath.EvalSymlinks(current); err == nil { + return filepath.Join(append([]string{resolved}, tail...)...) + } + } +} + +// normalizeProfilePathLexically expands and absolutizes a profile path without +// resolving symlinks. Credential carveouts use it so their fixed lexical name +// can never become an allow rule for a symlink target. +func normalizeProfilePathLexically(entry string) string { trimmed := strings.TrimSpace(entry) if trimmed == "" { return "" @@ -564,8 +696,5 @@ func normalizeProfilePath(entry string) string { if err != nil { return "" } - if resolved, err := filepath.EvalSymlinks(absolute); err == nil { - return resolved - } return filepath.Clean(absolute) } diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 3c77088e2..23714e174 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -802,7 +802,8 @@ func denyReadRules(fs FileSystemPolicy) []string { // exclusively from Zero's own config directory (never from a user-configured // DenyRead root), so no user deny is weakened here. func denyReadCarveoutRules(fs FileSystemPolicy) []string { - resolved := normalizeProfilePaths(fs.DenyReadCarveouts) + denied := dedupeStrings(append(append([]string{}, fs.DenyRead...), fs.DenyReadIfExists...)) + resolved := credentialCarveoutPaths(denied, fs.DenyReadCarveouts) if len(resolved) == 0 { return nil } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 26df8f815..6b65e9809 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -547,6 +547,41 @@ func TestSeatbeltProfileRendersCredentialBaselineAndCarveouts(t *testing.T) { } } +func TestSeatbeltProfileDoesNotRenderSymlinkCarveout(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + credentialDir := filepath.Join(t.TempDir(), "zero") + if err := os.MkdirAll(credentialDir, 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(credentialDir, "oauth-tokens.json") + if err := os.WriteFile(secret, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + pluginRoot := filepath.Join(credentialDir, "plugins") + if err := os.Symlink(secret, pluginRoot); err != nil { + t.Fatal(err) + } + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{"/"}, + DenyReadIfExists: []string{credentialDir}, + DenyReadCarveouts: []string{pluginRoot}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + for _, path := range []string{pluginRoot, secret, normalizeProfilePath(pluginRoot), normalizeProfilePath(secret)} { + allow := `(allow file-read* file-test-existence (subpath "` + sandboxProfileString(path) + `"))` + if strings.Contains(sbpl, allow) { + t.Fatalf("Seatbelt profile re-allowed symlink carveout path %q:\n%s", path, sbpl) + } + } +} + // 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 e79b46ba6f32d56e2baa0554267654fadfa69ec7 Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 21:25:49 +0000 Subject: [PATCH 16/18] fix(sandbox): normalize credential carveout paths Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno --- internal/sandbox/manager_test.go | 6 ++--- internal/sandbox/profile.go | 42 ++++++++++++++++++++++---------- internal/sandbox/profile_test.go | 5 +++- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 072ebc59b..7a658ab9a 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -494,7 +494,7 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { // emitted so pathname-policy backends can reserve it. Mount-based Linux // retains the same profile baseline but can mask only paths that exist when // Bubblewrap assembles the namespace. - if !stringSliceContains(paths, filepath.Join(home, ".azure")) { + if !stringSliceContains(paths, normalizeProfilePath(filepath.Join(home, ".azure"))) { t.Errorf("credential deny paths = %#v, want the not-yet-existing ~/.azure included", paths) } @@ -570,7 +570,7 @@ func TestCredentialPathOptionsResolveAgainstCommandDirectory(t *testing.T) { filepath.Join(commandDir, "mcp", "tokens.json.secret.lock"), filepath.Join(commandDir, "mcp", "tokens.json.migrated"), } { - if !stringSliceContains(paths, want) { + if !stringSliceContains(paths, normalizeProfilePath(want)) { t.Errorf("credential deny paths = %#v, want command-relative root %q", paths, want) } } @@ -586,7 +586,7 @@ func TestCredentialDenyReadPathsInConfigDirMatchesLiteralXDGResolution(t *testin if !stringSliceContains(resolvedConfigDirs, filepath.Dir(want)) { t.Fatalf("zero credential config dirs = %#v, want literal XDG resolution %q", resolvedConfigDirs, filepath.Dir(want)) } - if !stringSliceContains(paths, want) { + if !stringSliceContains(paths, normalizeProfilePath(want)) { t.Fatalf("credential deny paths = %#v, want literal XDG resolution %q", paths, want) } if expanded := normalizeProfilePaths([]string{filepath.Join(configDir, "zero")})[0]; expanded != want && stringSliceContains(paths, expanded) { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index f60072108..e99bfe142 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -390,7 +390,7 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string return credentialDenyPaths{ Paths: out, Carveouts: credentialCarveoutPaths(out, carveouts), - EnsureDirs: credentialRetainedDirs(out, ensureDirs), + EnsureDirs: credentialRetainedDirs(out, normalizeProfilePaths(ensureDirs)), Dirs: credentialRetainedDirs(out, normalizeProfilePaths(dirs)), } } @@ -493,20 +493,10 @@ func credentialCarveoutPaths(denied []string, carveouts []string) []string { } out := make([]string, 0, len(carveouts)) for _, entry := range carveouts { - carveout := normalizeProfilePathLexically(entry) + carveout := normalizeCredentialCarveoutPath(entry) if carveout == "" { continue } - // A missing fixed subtree may be installed later by trusted host code, but - // an existing entry must be a real directory. In particular, never turn a - // plugins symlink into an allow rule for its credential-file target. - if info, err := os.Lstat(carveout); err == nil { - if !info.IsDir() { - continue - } - } else if !os.IsNotExist(err) { - continue - } for _, deny := range denied { if carveout != deny && pathWithinRoot(deny, carveout) { out = append(out, carveout) @@ -517,6 +507,32 @@ func credentialCarveoutPaths(denied []string, carveouts []string) []string { return dedupeStrings(out) } +// normalizeCredentialCarveoutPath canonicalizes the parent while preserving +// the fixed terminal name. That keeps the allow under the same canonical root +// as its deny (for example macOS /var -> /private/var) without ever following a +// plugins/specialists/commands symlink to a credential target. +func normalizeCredentialCarveoutPath(entry string) string { + carveout := normalizeProfilePathLexically(entry) + if carveout == "" { + return "" + } + // A missing fixed subtree may be installed later by trusted host code, but + // an existing entry must be a real directory. In particular, never turn a + // plugins symlink into an allow rule for its credential-file target. + if info, err := os.Lstat(carveout); err == nil { + if !info.IsDir() { + return "" + } + } else if !os.IsNotExist(err) { + return "" + } + parent := normalizeProfilePath(filepath.Dir(carveout)) + if parent == "" { + return "" + } + return filepath.Join(parent, filepath.Base(carveout)) +} + // credentialRetainedDirs keeps only directories that remain exact deny entries. func credentialRetainedDirs(denied []string, dirs []string) []string { if len(dirs) == 0 { @@ -544,7 +560,7 @@ func finalizeCredentialDenyPaths(credentials credentialDenyPaths, userDenyRead [ credentials.Dirs = credentialRetainedDirs(credentials.Paths, credentials.Dirs) credentials.Carveouts = credentialCarveoutPaths(credentials.Paths, credentials.Carveouts) - paths := make([]string, 0, len(credentials.Paths)) + var paths []string for _, path := range credentials.Paths { covered := false for _, dir := range credentials.Dirs { diff --git a/internal/sandbox/profile_test.go b/internal/sandbox/profile_test.go index 38a7edee8..5decbe9af 100644 --- a/internal/sandbox/profile_test.go +++ b/internal/sandbox/profile_test.go @@ -15,7 +15,10 @@ func TestCredentialDeniesMatchTokenStoreFallbacks(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Windows credential deny-read is tracked separately") } - workspace := t.TempDir() + workspace, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } profileHome := filepath.Join(workspace, "profile-home") envMap := map[string]string{"HOME": "", "USERPROFILE": profileHome, "XDG_CONFIG_HOME": ""} oauthPath, err := oauth.ResolveStorePath(envMap) From 083d6f0fb7a523ac411e40b460c0ab3ba968eeba Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 21:36:57 +0000 Subject: [PATCH 17/18] fix(sandbox): canonicalize backend credential masks Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno --- internal/sandbox/linux_helper.go | 2 +- internal/sandbox/linux_helper_test.go | 15 +++++++++------ internal/sandbox/profile.go | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 9edf1558e..227c7d602 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -371,7 +371,7 @@ func appendReadOnlyLinuxPathArgs(args []string, path string) []string { } func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []string) []string { - path = strings.TrimSpace(path) + path = normalizeProfilePath(path) if path == "" { return args } diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index 4d04c3a21..22f974fb0 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -353,13 +353,15 @@ func TestLinuxBwrapKeepsCarveoutsReachableInsideMaskedDir(t *testing.T) { } plan := buildLinuxBwrapFilesystemPlan(profile) + normalizedCredentialDir := normalizeProfilePath(credentialDir) + normalizedPluginRoot := normalizeCredentialCarveoutPath(pluginRoot) // 111 rather than 000: a 000 directory cannot be traversed, so the re-bound // subpath below it would be unreachable. Contents stay unlistable either way. - assertArgsContainSequence(t, plan.Args, "--perms", "111", "--tmpfs", credentialDir) - assertArgsContainSequence(t, plan.Args, "--ro-bind", pluginRoot, pluginRoot) - assertArgsContainSequence(t, plan.Args, "--remount-ro", credentialDir) - bindIdx := argsSequenceIndex(plan.Args, "--ro-bind", pluginRoot, pluginRoot) - remountIdx := argsSequenceIndex(plan.Args, "--remount-ro", credentialDir) + assertArgsContainSequence(t, plan.Args, "--perms", "111", "--tmpfs", normalizedCredentialDir) + assertArgsContainSequence(t, plan.Args, "--ro-bind", normalizedPluginRoot, normalizedPluginRoot) + assertArgsContainSequence(t, plan.Args, "--remount-ro", normalizedCredentialDir) + bindIdx := argsSequenceIndex(plan.Args, "--ro-bind", normalizedPluginRoot, normalizedPluginRoot) + remountIdx := argsSequenceIndex(plan.Args, "--remount-ro", normalizedCredentialDir) if bindIdx < 0 || remountIdx < 0 || bindIdx > remountIdx { t.Fatalf("carveout bind (%d) must precede the tmpfs remount-ro (%d): %#v", bindIdx, remountIdx, plan.Args) } @@ -394,7 +396,8 @@ func TestLinuxBwrapDoesNotBindSymlinkCarveout(t *testing.T) { } plan := buildLinuxBwrapFilesystemPlan(profile) - assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", credentialDir, "--remount-ro", credentialDir) + normalizedCredentialDir := normalizeProfilePath(credentialDir) + assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", normalizedCredentialDir, "--remount-ro", normalizedCredentialDir) if argsContainSequence(plan.Args, "--ro-bind", pluginRoot, pluginRoot) || argsContainSequence(plan.Args, "--ro-bind", secret, secret) { t.Fatalf("symlink carveout was rebound into credential mask: %#v", plan.Args) } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index e99bfe142..2a326ab22 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -491,6 +491,7 @@ func credentialCarveoutPaths(denied []string, carveouts []string) []string { if len(carveouts) == 0 { return nil } + denied = normalizeProfilePaths(denied) out := make([]string, 0, len(carveouts)) for _, entry := range carveouts { carveout := normalizeCredentialCarveoutPath(entry) From 497aeb5d777ec1a5cd43191437e2a82f076a1557 Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 21:43:00 +0000 Subject: [PATCH 18/18] test(sandbox): normalize backend mask expectations Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno --- internal/sandbox/linux_helper_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/sandbox/linux_helper_test.go b/internal/sandbox/linux_helper_test.go index 22f974fb0..f3edd61eb 100644 --- a/internal/sandbox/linux_helper_test.go +++ b/internal/sandbox/linux_helper_test.go @@ -294,7 +294,8 @@ func TestLinuxBwrapSkipsMissingCredentialBaselines(t *testing.T) { t.Fatalf("MkdirAll credential dir: %v", err) } plan = buildLinuxBwrapFilesystemPlan(profile) - assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", missingCredential, "--remount-ro", missingCredential) + normalizedCredential := normalizeProfilePath(missingCredential) + assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", normalizedCredential, "--remount-ro", normalizedCredential) } // TestLinuxBwrapCreatesOwnedCredentialDirsBeforeMasking covers the long-lived @@ -321,7 +322,8 @@ func TestLinuxBwrapCreatesOwnedCredentialDirsBeforeMasking(t *testing.T) { if info, err := os.Stat(ownedDir); err != nil || !info.IsDir() { t.Fatalf("owned credential dir was not created: err=%v", err) } - assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", ownedDir, "--remount-ro", ownedDir) + normalizedOwnedDir := normalizeProfilePath(ownedDir) + assertArgsContainSequence(t, plan.Args, "--perms", "000", "--tmpfs", normalizedOwnedDir, "--remount-ro", normalizedOwnedDir) if stringSliceContains(plan.Args, thirdParty) { t.Fatalf("absent third-party store must stay unmounted and uncreated: %#v", plan.Args) }