fix(sandbox): deny reads of Zero credential stores#681
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCredential deny-read computation now uses command-specific environment and directory context, covers Zero and token stores before creation, and resolves overrides literally. Linux bwrap filesystem generation now fails closed for missing mount targets, while OAuth writes use fixed protected temporary siblings. ChangesCredential deny-read generation
Missing sandbox mount targets
Atomic OAuth store writes
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
- 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.
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 |
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 <PierrunoYT@users.noreply.github.com>
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/zeroconfigdir_parity_test.go`:
- Line 1: Rename the test file from zeroconfigdir_parity_test.go to
profile_test.go so the tests for zeroUserConfigDir in profile.go reside
alongside the source file they cover.
🪄 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: 584d1c8c-ed4f-4446-88c8-e9f7a52829c6
📒 Files selected for processing (4)
internal/sandbox/export_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/zeroconfigdir_parity_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/manager_test.go
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] Keep absent deny paths from aborting the Linux sandbox
internal/sandbox/profile.go:228
This now emits every missing default and override candidate (including~/.azureand a fresh~/.config/zero). The Linux backend translates a missingDenyReadtarget into--tmpfs <path>only after it has read-only-bound/; bubblewrap cannot create that target or its missing parents in the read-only tree, so startup fails before the requested command executes. For example,bwrap --ro-bind / / --tmpfs <existing-parent>/missing -- trueexits with “Can't mkdir … Read-only file system.” Preserve backend-safe handling for nonexistent paths (or materialize them safely before the read-only bind) and add a real Linux launch regression test. -
[P1] Cover all credential-bearing siblings of explicit token-store overrides
internal/sandbox/profile.go:221
An override outside the default config directory receives rules only for<path>and<path>.secret. OAuth publishes its token JSON through<path>.tmp-<pid>-<nanos>, encrypted storage creates<path>.secret.*.tmp, and MCP migration preserves the token-bearing legacy file at<ZERO_MCP_OAUTH_TOKENS_PATH>.migrated; none of those siblings is denied. A sandboxed command can poll the override directory during a save or read the durable migration backup and recover the token/key. Protect the required sibling set or use a safe directory-level containment approach for overrides, with publication and migration tests. -
[P2] Resolve the config-root deny path with the store's literal XDG semantics
internal/sandbox/profile.go:211
zeroUserConfigDirreturns a nonemptyXDG_CONFIG_HOMEverbatim, matching the config and token stores, but the laternormalizeProfilePathsexpands a leading~. WithXDG_CONFIG_HOME=~/zero-config, the stores use the literal cwd-relative~/zero-config/zero/...path while this profile denies$HOME/zero-config/zero; the real config, provider credentials, and OAuth files remain readable in the sandbox. Resolve this candidate without tilde expansion (or through the same resolver used by the stores) and cover a literal-tilde XDG value.
Open inline review threads
The following inline threads remain open on GitHub:
- Copilot —
internal/sandbox/profile.go:170: the comment about existence filtering is stale; the filter was removed in the current head. - Copilot —
internal/sandbox/profile.go:244: the macOS/XDG wording is now aligned in the current helper comment. - gnanam1990 —
internal/sandbox/manager_test.go:511: the macOS path-normalization test issue is addressed by resolving the temp base and recomputing the expected path after creation. - gnanam1990 —
internal/sandbox/profile.go:188: the resolver-drift concern is addressed by the new parity test againstconfig.UserConfigDir. - CodeRabbit —
internal/sandbox/zeroconfigdir_parity_test.go:1: the requested rename toprofile_test.gohas not been made; this is a non-blocking test-file organization nit.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-approving. gnanam's blocker was the darwin/arm64 sandbox test failing, and the commit that addresses the macOS deny-path test plus config parity fixes it: the full smoke matrix is green now, macOS included. Denying the whole config dir rather than itemized filenames is the approach I already backed last round, since the stores publish random-named .tmp/.secret/.migrated siblings, so this is consistent with what I approved. gnanam will want to clear his own changes-requested once he re-runs, but from my side this is good.
|
Addressed the current review feedback in
Validation:
One limitation remains for randomly named atomic-write siblings beside arbitrary override paths. The current permission model supports exact paths/subtrees, but not filename patterns. Denying the whole override parent could unintentionally hide |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/linux_helper_test.go (1)
208-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
errors.Is(err, os.ErrNotExist)instead ofos.IsNotExist.
os.IsNotExistdoes not support error unwrapping and is discouraged in modern Go versions. Usingerrors.Isaligns with best practices and the rest of the codebase (e.g., your newly added assertions inrunner_linux_integration_test.go).♻️ Proposed refactor
- if _, err := os.Stat(filepath.Dir(missing)); !os.IsNotExist(err) { + if _, err := os.Stat(filepath.Dir(missing)); !errors.Is(err, os.ErrNotExist) {🤖 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/linux_helper_test.go` at line 208, Update the missing-path assertion in the test around os.Stat to use errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), adding the errors import if needed while preserving the existing condition.
🤖 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/linux_helper_test.go`:
- Line 208: Update the missing-path assertion in the test around os.Stat to use
errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), adding the errors
import if needed while preserving the existing condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4eeca769-0c14-4119-937b-c4b627e08071
📒 Files selected for processing (6)
internal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/profile_test.gointernal/sandbox/runner_linux_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/profile.go
- internal/sandbox/manager_test.go
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] Fail closed when a protected Bubblewrap path does not exist
internal/sandbox/linux_helper.go:308
Both carveout helpers now return no mount arguments whenos.Statfails. The new profile deliberately includes absent credential directories so stores created later remain hidden, but the standard Linux plan has already--ro-binded/; a long-lived sandbox launched before$XDG_CONFIG_HOME/zeroexists can read a token created by a concurrent login. The same change also drops missing.git/hooks/.git/configread-only carveouts beneath a writable workspace, allowing a sandboxed command to create a hook that runs during the user's later unsandboxed commit. Do not silently omit a missing protected path: enforce it safely in the namespace or fail closed. -
[P1] Protect the atomic-publication siblings of custom token overrides
internal/sandbox/profile.go:220
ExplicitZERO_OAUTH_TOKENS_PATHandZERO_MCP_OAUTH_TOKENS_PATHoverrides receive rules only for the final name (plus a few fixed siblings), whilefileBlob.writepublishes the plaintext token through<path>.tmp-<pid>-<nanos>and encrypted storage creates<path>.secret.*.tmp. For an override outside the protected Zero config directory, its readable parent lets a sandboxed command poll those temporary files during login or refresh and recover the token or its encryption key. Cover the publication siblings or use a safe directory-level containment rule for override paths. -
[P1] Use the token stores' HOME/USERPROFILE fallback when deriving the default deny root
internal/sandbox/profile.go:177
On Unix withHOMEunset andUSERPROFILE=/custom/home, the OAuth and MCP resolvers store tokens under/custom/home/.config/zero, but this code usesos.UserHomeDir/os.UserConfigDir, which do not implement that fallback. The resulting profile omits the actual token directory and leaves it readable to a sandboxed command. Derive the default path with the same resolver semantics as the stores and add this environment combination to the regression tests. -
[P1] Resolve relative credential paths relative to the command that consumes them
internal/sandbox/profile.go:220
The deny paths for a relative override orXDG_CONFIG_HOMEare resolved while the parent process builds the profile, butBuildCommandPlanlater changes the child toCommandSpec.Dir. A child Zero process resolves the inherited relative value against that command directory, so a command run from a workspace subdirectory reads a different token location than the one denied by the profile. Resolve these candidates against the effective command directory, or make the child receive an unambiguous absolute value.
|
Addressed the latest review findings in commit 63adeb4:
Validation:
The full go test ./... run reaches one unrelated existing TUI copy assertion failure in TestProviderWizardAdvancesProviderAPIKeyAndModelSteps. The requested golangci-lint command also reports 35 pre-existing repository findings; none are introduced by this patch. A real Bubblewrap execution could not be run locally because bwrap is not installed in WSL, so the pushed CI Ubuntu smoke remains the authoritative integration run. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sandbox/profile.go (1)
255-263: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid making fresh Linux environments unsandboxable.
This unconditionally emits
<config>/zeroeven when it does not exist, while the Bubblewrap backend rejects every missing deny target. A user without that directory therefore cannot launch any sandboxed command. The fixture creation ininternal/sandbox/runner_test.golines 622-628 masks this production path.Support a safe representation for absent deny targets before emitting them unconditionally, and add a regression test using an empty credential home.
🤖 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 255 - 263, Update the candidate-deny handling around the configDir branch in the profile-building flow so absent paths such as filepath.Join(configDir, "zero") are represented safely before reaching the Bubblewrap backend, which rejects missing deny targets. Preserve denial of the directory when it exists, add the required regression coverage in the runner tests using an empty credential home, and ensure fresh Linux environments can launch sandboxed commands without precreating the directory.
🤖 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/linux_helper.go`:
- Around line 234-248: Guard each appendReadOnlyLinuxPathArgs call in
internal/sandbox/linux_helper.go lines 234-248 with pathExists checks, skipping
missing ReadOnlySubpaths and ProtectedMetadataNames entries while preserving
errors for existing paths. Remove the os.Mkdir workaround for .git in
internal/sandbox/linux_helper_test.go lines 259-261 and the os.MkdirAll
workarounds for .git/hooks, .zero, and .agents in
internal/sandbox/runner_linux_integration_test.go lines 26-32.
---
Outside diff comments:
In `@internal/sandbox/profile.go`:
- Around line 255-263: Update the candidate-deny handling around the configDir
branch in the profile-building flow so absent paths such as
filepath.Join(configDir, "zero") are represented safely before reaching the
Bubblewrap backend, which rejects missing deny targets. Preserve denial of the
directory when it exists, add the required regression coverage in the runner
tests using an empty credential home, and ensure fresh Linux environments can
launch sandboxed commands without precreating the directory.
🪄 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: 007ead2c-bd5f-4bb4-8a11-5c85598c86e1
📒 Files selected for processing (9)
internal/cli/sandbox_test.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/profile_test.gointernal/sandbox/runner.gointernal/sandbox/runner_linux_integration_test.gointernal/sandbox/runner_test.go
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] Keep absent default credential paths from aborting Linux sandbox startup
internal/sandbox/linux_helper.go:339
The default profile now deliberately emits~/.aws,~/.config/gcloud,~/.azure, and$XDG_CONFIG_HOME/zeroeven before they exist, but the Linux helper returns an error for any missingDenyReadtarget. On a normal fresh Linux account at least one of those paths is absent, so every sandboxed command exits before Bubblewrap can execute it. The new smoke setup creates all of those directories, which hides the default failure. Represent absent default paths in a way Bubblewrap can enforce without making ordinary sandbox launches fail. -
[P1] Do not mask arbitrary token-override parent directories
internal/sandbox/profile.go:269
An override is allowed to name any token file, but this converts it into a deny rule for its entire parent directory. For example,ZERO_OAUTH_TOKENS_PATH=.zero-tokenswith a workspace CWD adds that workspace toDenyRead; Bubblewrap bind-mounts it writable, then overlays it with an unreadable tmpfs and cannotchdirinto it. Overrides under/tmpor a home directory similarly hide those whole usable roots. Either constrain/reject unsafe override locations or protect the required sibling files without replacing a granted working root.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/sandbox/runner_linux_integration_test.go (1)
94-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a temporary directory instead of
/etcfor the missing path.Hardcoding
/etc/...relies on the host's root filesystem state. On locked-down test runners or systems with strict AppArmor/SELinux profiles, accessing/etcmight yield permission errors rather thanos.ErrNotExist, causing the test to flake or fail for the wrong reason.Additionally, because
/etcis typically read-only for non-root users, the assertion on line 113 (verifying the sandbox didn't materialize the path) is a false positive—it would fail to create it regardless of whether there's a bug in the sandboxing logic. Using a subdirectory withint.TempDir()guarantees a writable location, genuinely validating that the logic doesn't attempt to create the missing path.♻️ Proposed refactor
- 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) }Note: Depending on the other imports in this file, removing
fmt.Sprintfmight make thefmtimport unused. Ensure to rungoimportsorgo mod tidyafterwards.🤖 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/runner_linux_integration_test.go` around lines 94 - 97, Update the missingDenied setup in the test to create the missing path beneath t.TempDir() rather than constructing it under /etc. Preserve the nested missing-directory structure and precondition check, and ensure the assertion that the sandbox does not materialize the path now exercises a writable location; remove any imports made unused by this change.internal/oauth/store.go (1)
420-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify cleanup with
defer.You can clean up the error-handling paths and ensure cleanup even in the event of a panic by using a single
defer os.Remove(tempPath)immediately after successful file creation. This nicely aligns with the robust pattern you are already using inencrypt.go.♻️ Proposed refactor
- 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 + defer os.Remove(tempPath) + + if _, err := temp.Write(data); err != nil { + _ = temp.Close() + return err + } + if err := temp.Close(); err != nil { + return err + } + return os.Rename(tempPath, b.path)🤖 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/oauth/store.go` around lines 420 - 433, In the temporary-file write flow, add a single deferred os.Remove(tempPath) immediately after successful file creation, then remove the repeated cleanup calls from the Write, Close, and Rename error paths. Preserve the existing error returns and successful rename behavior while ensuring the temporary path is cleaned up even if execution panics.
🤖 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 277-282: Update the MCPOAuthTokens candidate construction in the
profile setup to include the override path’s adjacent .secret, .secret.tmp, and
.tmp files alongside the existing tokenPath and migrated entries. Ensure these
sibling paths are added only when the trimmed tokenPath is non-empty so all MCP
token-store artifacts are denied.
In `@internal/sandbox/runner.go`:
- Around line 357-358: Update the credentialDenyReadPaths call in the
compatibility profile to pass the current process environment via os.Environ()
instead of nil, so environment-based credential path overrides such as
ZERO_OAUTH_TOKENS_PATH are included and protected.
---
Nitpick comments:
In `@internal/oauth/store.go`:
- Around line 420-433: In the temporary-file write flow, add a single deferred
os.Remove(tempPath) immediately after successful file creation, then remove the
repeated cleanup calls from the Write, Close, and Rename error paths. Preserve
the existing error returns and successful rename behavior while ensuring the
temporary path is cleaned up even if execution panics.
In `@internal/sandbox/runner_linux_integration_test.go`:
- Around line 94-97: Update the missingDenied setup in the test to create the
missing path beneath t.TempDir() rather than constructing it under /etc.
Preserve the nested missing-directory structure and precondition check, and
ensure the assertion that the sandbox does not materialize the path now
exercises a writable location; remove any imports made unused by this change.
🪄 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: fc9c6b41-56aa-44b3-966f-71d5b19c98b9
📒 Files selected for processing (12)
internal/cli/sandbox_test.gointernal/oauth/encrypt.gointernal/oauth/store.gointernal/oauth/store_test.gointernal/sandbox/landlock_linux.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/profile_test.gointernal/sandbox/runner.gointernal/sandbox/runner_linux_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/sandbox/profile_test.go
- internal/cli/sandbox_test.go
- internal/sandbox/linux_helper.go
- internal/sandbox/linux_helper_test.go
- internal/sandbox/manager_test.go
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] Preserve protected metadata carveouts when the path is absent
internal/sandbox/linux_helper.go:235
Skipping a missing.git/config,.git/hooks,.zero, or.agentscarveout leaves the writable-root bind unmasked. A sandboxed command in a fresh workspace can create those paths and then write them, bypassing the OS-level protections for hooks, Git configuration, and Zero state. The same regression also affects a workspace that is a default temp root, where the profile now removes these carveouts entirely. Keep an enforcement mechanism that prevents creation without making an ordinary fresh workspace fail to launch. -
[P1] Do not silently omit absent credential deny targets on Linux
internal/sandbox/linux_helper.go:270
DenyReadIfExistsis used for all of the new default and override credential paths, but bwrap emits no masking mount when a target is absent at startup. An already-running sandbox has a read-only bind of/; after a concurrent login or refresh creates$XDG_CONFIG_HOME/zero(or an override path), that file becomes visible and readable. A relative override beneath the writable workspace can also be created and read by the sandbox itself. That also leaves the new fixed.tmppublication sibling writable, so it can be replaced between close and rename. If the intended contract is to protect stores created during a long-lived sandboxed session, this needs a future-path mask or fail-closed behavior; otherwise the Linux limitation needs an explicit maintainer decision and a narrower security claim. -
[P2] Deny the OAuth lock-file siblings for token overrides
internal/sandbox/profile.go:270
The override deny set protects the data, secret, temp, and migrated siblings but omits<token>.lockfileand<token>.secret.lock. Those files are created byfileBlob.withLockandcreateSecretFile; a sandboxed command with an override below a writable workspace or temp root can keep either lock present, making later OAuth saves or encrypted-store initialization time out. Include the lock siblings in the protected set (and cover the behavior with an override-path test).
|
Addressed the latest review in
For the two future-path findings, I am retaining the existing Linux behavior as an explicit backend limitation. Bubblewrap cannot mount over a nonexistent child beneath an already bound writable host root without creating the mount point on the host. The alternatives are to mutate the user's workspace/home or mask/remount an existing parent, which can hide the workspace, HOME, or Validation:
All pass. Please re-review when convenient. |
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