fix(sandbox): deny reads of Zero credential stores#681
Conversation
WalkthroughThe sandbox credential deny-read logic now includes Zero config and credential files, OAuth and MCP token overrides, and their secret files. Path resolution uses structured options and platform-aware config lookup. Tests cover filtering, opt-outs, Google credentials, override resolution, and permission-profile integration. ChangesCredential deny-read coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/profile.go`:
- Around line 206-214: The default Zero deny-read candidates in
internal/sandbox/profile.go:206-214 must include mcp-oauth-tokens.json.secret
alongside the other token files. Update the zeroFiles test slice in
internal/sandbox/manager_test.go:388-396 with the same path to enforce coverage;
both changes belong near the existing mcp-oauth-tokens.json entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c715035-5186-49df-889d-1fac687bba1c
📒 Files selected for processing (2)
internal/sandbox/manager_test.gointernal/sandbox/profile.go
There was a problem hiding this comment.
Pull request overview
This PR hardens the sandbox’s default deny-read profile so sandboxed commands cannot read Zero’s own on-disk secrets (user config, cred stores, OAuth token stores), in addition to existing cloud-provider credential locations—closing a gap where environment scrubbing alone was insufficient.
Changes:
- Expands
credentialDenyReadPathsto include Zero’s config/credential/token files and to honorZERO_OAUTH_TOKENS_PATH/ZERO_MCP_OAUTH_TOKENS_PATH(including adjacent.secretfiles). - Refactors
credentialDenyReadPathsInto take a structured options object, improving testability and supporting new inputs. - Adds/extends tests to validate the expanded deny list and AllowRead opt-outs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/sandbox/profile.go | Extends default deny-read candidates to cover Zero credential/config/token files and env overrides; adds zeroUserConfigDir helper. |
| internal/sandbox/manager_test.go | Updates deny-path unit tests for new candidates/options and adds a profile-level test for denying Zero token reads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Failed home/config lookups only drop their derived candidates; explicit | ||
| // credential-file overrides must be protected regardless. |
| // zeroUserConfigDir mirrors config.UserConfigDir without importing config | ||
| // (config depends on sandbox). Zero deliberately uses ~/.config on macOS and | ||
| // os.UserConfigDir everywhere else. |
- 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 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Deny the migrated legacy MCP token backup
internal/sandbox/profile.go:214
NewTokenStoreintentionally preserves the pre-unificationmcp-oauth-tokens.jsonasmcp-oauth-tokens.json.migratedafter importing it. That backup still contains the access and refresh tokens, but this list denies only the original filename; the override branch has the same omission. A sandboxed command can therefore read the persistent migrated backup under the read-all profile. Include the migrated filename for both default and override paths (and cover the migration path in the regression test) or remove/redact the backup before claiming the legacy store is protected. -
[P1] Cover credential-store publication files, not just their final names
internal/sandbox/profile.go:208
The protected stores publish secret-bearing temporary siblings before their atomic rename: OAuth usesoauth-tokens.json.tmp-*, the credential store usescredentials.{enc,json}.*.tmp, encrypted stores create*.secret.*.tmp, and config writes.zero-config-*.tmp. Those files live in the same readable directory but do not match any added exact deny entry, so a sandboxed process can enumerate/poll the directory during a login or key/config update and read the token, API key, encryption key, or inline config secret. Deny the appropriate credential-file patterns or otherwise make the write paths unreadable for the lifetime of the sandbox, with a concurrent-publication regression test. -
[P2] Do not drop default credential paths merely because they are absent when the sandbox starts
internal/sandbox/profile.go:225
The new rules are omitted wheneveros.Statinitially returns an error. If a sandboxed command is already running whilezero author a credential rotation creates one of these files, no later filesystem rule is installed and that command can read the newly created final store. The Linux helper already has a missing-path unreadable mount mode, and seatbelt can express a deny before a file exists, so retain the fixed default candidates (subject to the explicitAllowReadopt-out) and test creation after profile construction. -
[P2] Resolve token overrides using the token stores' path semantics
internal/sandbox/profile.go:218
oauth.ResolveStorePathandmcp.ResolveTokenStorePathtreat any non-absolute override as a literal path relative to the process working directory.normalizeProfilePath, however, expands a~/...override before making it absolute. WithZERO_OAUTH_TOKENS_PATH=~/tokens.json, the real store is<cwd>/~/tokens.jsonwhile the deny entry targets$HOME/tokens.json; the actual token file remains readable. Normalize these overrides through the same resolver used by their stores (and add a relative-tilde case) before derivingDenyRead.
Address code review on PR Gitlawb#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-<pid>-<nanos> 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 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/profile.go (1)
244-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify path resolution by relying on
filepath.Abs.
filepath.Absinherently handles both relative and absolute paths, and automatically invokesfilepath.Cleanon its output. You can streamline this helper by removing the redundantfilepath.IsAbscondition and the explicitfilepath.Cleancalls.♻️ Proposed refactor
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) + return abs }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/profile.go` around lines 244 - 267, Update resolveCredentialOverridePath to trim and reject empty overrides, then rely solely on filepath.Abs for both absolute and relative paths. Remove the filepath.IsAbs branch and explicit filepath.Clean calls, while preserving the existing empty-string and Abs-error return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/sandbox/profile.go`:
- Around line 244-267: Update resolveCredentialOverridePath to trim and reject
empty overrides, then rely solely on filepath.Abs for both absolute and relative
paths. Remove the filepath.IsAbs branch and explicit filepath.Clean calls, while
preserving the existing empty-string and Abs-error return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20cdc36c-d46b-4198-9a7d-066526d292f3
📒 Files selected for processing (2)
internal/sandbox/manager_test.gointernal/sandbox/profile.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewing against commit 43e81fdd (head). The directory-wide deny on ~/.config/zero (patch 3/3) closes the atomic-rename temp-file and .migrated backup leak that the prior itemized list missed — the directory is the only stable primitive. The "emit whether or not it exists on disk" rule is correct (backends treat missing paths as no-ops, but the rule takes effect once the store appears mid-session, which the prior behavior would have missed). The resolveCredentialOverridePath fix aligns with the token stores' literal-CWD resolution instead of normalizeProfilePath's tilde expansion, closing the real mismatch the new test covers.
LGTM. The zeroUserConfigDir duplication is intentional (import cycle: config depends on sandbox); a follow-up to break the cycle is worth filing but not a blocker.
Cross-PR note: #681 / #682 / #685 are coordinated. Recommend rebasing #682 and #685 on top of #681 in that order to resolve the credentialDenyReadPaths signature and the scrubSensitiveEnv plumbing cleanly.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Local review: built and ran go test ./internal/sandbox on darwin/arm64 — one test fails (blocking). Plus one drift note.
| // 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] |
There was a problem hiding this comment.
[P1] TestPermissionProfileDeniesZeroCredentialFiles fails on macOS (darwin) — blocks CI
want is computed here, before os.MkdirAll(zeroDir) creates the dir (line 513), then reused for the second profile build after the dir exists. normalizeProfilePath calls filepath.EvalSymlinks, which on macOS:
- fails for a non-existent tail (
/var/.../001/zerowhenzerodoesn't exist) and falls back tofilepath.Clean, leaving/varunresolved; - succeeds once the dir exists and resolves
/var->/private/var.
I confirmed this with a standalone probe: EvalSymlinks("/var/.../zero") returns lstat /private/var/.../zero: no such file or directory before mkdir, and /private/var/.../zero after. Result: first want is /var/.../zero, second profile's DenyRead is /private/var/.../zero -> mismatch at the second assertion. Fix: recompute want after MkdirAll, or EvalSymlinks the base temp dir, or build the expected path from a non-symlinked temp base.
| MCPOAuthTokens: os.Getenv("ZERO_MCP_OAUTH_TOKENS_PATH"), | ||
| }, policy.AllowRead) | ||
| } | ||
|
|
There was a problem hiding this comment.
[P3] zeroUserConfigDir() duplicates config.UserConfigDir() to avoid an import cycle. If the real resolver ever changes (different XDG fallback, etc.), the deny rule targets a different directory than the store writes to -> silent bypass. A parity test against config.UserConfigDir() would harden it.
There was a problem hiding this comment.
Approving. I checked the new helpers against the real store resolution: zeroUserConfigDir matches config.UserConfigDir exactly (including the macOS ~/.config/XDG behavior), and resolveCredentialOverridePath mirrors oauth.ResolveStorePath / mcp.ResolveTokenStorePath relative overrides resolve literally against cwd with no tilde expansion, so the deny rule lands on the path the store actually writes to. Denying the whole /zero directory is the right call given the random-named .tmp/.secret/.migrated siblings the stores publish, and emitting rules for not-yet-existing paths is safe (seatbelt treats them as no-ops, bwrap mounts a perms-000 tmpfs, Windows is skipped) so stores created mid-session stay covered. One minor residual worth a follow-up: for an explicit ZERO_OAUTH_TOKENS_PATH override, the atomic-write .tmp-- sibling isn't denied (only the file and .secret are), leaving a brief read window during a token write not a regression since overrides had no coverage before. Build, vet, gofmt, and the new tests pass here; the six sandbox-suite failures reproduce on clean main.
|
fix smoke tests and stuffs seems all good then |
Summary
Root cause
The sandbox scrubbed credential environment variables but its read-deny profile only covered cloud-provider paths. Under a read-all posture, an agent could still read Zero's own credential and OAuth stores from disk.
Windows read denial remains tracked separately in #662.
Fixes #675
Validation
go test ./internal/sandbox -count=1go test ./... -count=1with ANTHROPIC_API_KEY unsetgo vet ./...go run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...(Go 1.26.5): no vulnerabilities foundgo fmt ./...Known repository/environment limitations
make lintuses a Unix shell assignment that does not run under this Windows shell.make testpath cannot run because GCC is not installed; the complete non-race suite passed.Summary by CodeRabbit