fix(sandbox): address macOS deny-path test and config parity (#681 review)

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

internal/sandbox/manager_test.go

@@ -378,43 +378,161 @@ 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")
+	// 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"),
+		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"),
+		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 {
+			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 := []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.
-	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)
 	}
+	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)
 	}
 
-	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)
 	}
 }
+
+// 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")
+	}
+	// 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")
+
+	// 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)
+	}
+	secret := filepath.Join(zeroDir, "oauth-tokens.json")
+	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)
+	}
+
+	// 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)
+	}
+}

internal/sandbox/profile.go

@@ -148,16 +148,23 @@ 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/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
 //     WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit
 //     once the Windows deny-read model is settled.
 //   - A candidate nested under a user-configured AllowRead entry is dropped,
 //     so `allowRead: ["~/.aws"]` remains an explicit opt-out.
+//   - 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
@@ -166,33 +173,59 @@ 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 are still submitted as candidates 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 != "" {
+		// 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-<pid>-<nanos>,
+		// 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 _, 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) {
@@ -207,6 +240,50 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead
 	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 <cwd>/~/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
+// 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
+}
+
 // 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

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)
+	}
+}