From 51b2ea25dbd0c78ff4e29a98682b1f3a8e8b8c10 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 21:13:10 +0200 Subject: [PATCH 1/4] 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 a2b8550f..32d67fc1 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 19473666..c699fd81 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 2/4] 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 32d67fc1..420f89c2 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 c699fd81..3528f454 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 3/4] 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 420f89c2..43fcde5a 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 3528f454..08eacbf6 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 4/4] 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 00000000..05b5c88f --- /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 43fcde5a..ab788b36 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 08eacbf6..7753b731 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 00000000..4d95d570 --- /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) + } +}