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 conditionally denies existing credential targets, while OAuth writes use fixed protected temporary siblings. ChangesCredential deny-read generation
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.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Final security follow-up on current head
The PR is mergeable. PierrunoYT requests a fresh human review to supersede the previous |
Preserve the expanded upstream credential baseline while retaining Zero store masking, trusted directory creation, and cross-platform carveout safety. Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-85ad-76ef-ac88-f7b9c4d27cc0 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT pushed Conflict resolution and security follow-up
ValidationLocal (Go 1.26.5):
GitHub checks on The PR is |
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] Mask the committed OAuth/MCP store file for process-trusted overrides outside
~/.config/zero, not only the.publishdirectory
internal/sandbox/linux_helper.go:285
internal/sandbox/profile.go:446
internal/oauth/store.go:412
The default…/zerocase is covered correctly: the whole directory is ensured and tmpfs-masked, so a rename into that tree stays hidden (including the mid-session integration case). A process-trustedZERO_OAUTH_TOKENS_PATH/ZERO_MCP_OAUTH_TOKENS_PATHoutside that directory is different. The profile ensures and masks<path>.publish, but only lists the final<path>inDenyReadIfExists. Bubblewrap skips absent file targets, so a sandbox launched before the first save has no mount on the store file itself.fileBlob.writepublishes plaintext under the masked.publishtree and thenRenames it into the unmasked parent; under the read-all--ro-bind /posture the committed token file becomes visible to the already-running sandbox as soon as the host save completes.TestPermissionProfileUnionsProcessAndCommandCredentialRootsWithoutCreatingCommandDirsencodes the.publishhalf of this contract forparentTokenbut never checks post-rename visibility. Please keep directory-level containment for default…/zerostores, but for out-of-tree process-trusted overrides also make the committed store path unreadable on Linux after first save (for example by masking a safe parent, pre-materializing a maskable placeholder, or another approach that does not reintroduce workspace-parent masking). Command-controlled overrides that deliberately omitEnsureDenyReadDirsare out of scope here.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the out-of-tree token-store masking finding in
Validation: focused sandbox tests pass with |
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.
The first three items are rebase drift against current main, not defects in the new credential-deny logic itself. The remaining items are gaps in this PR's own design on head f25b3e7.
Findings
-
[P1] Rebase onto current
mainand resolve the sandbox conflicts before merge
The live PR remains not mergeable (mergeable: false, headf25b3e7, base81a5e6c, currentmain8acd42a).git merge-treeshows conflicting edits ininternal/sandbox/profile.go,internal/sandbox/profile_test.go, andinternal/sandbox/runner_test.go. Please rebase, resolve those conflicts deliberately, and push so the resolved diff can be reviewed. -
[P1] On rebase, restore git credential-store deny paths from #816
internal/sandbox/profile.go:417
maindenies git's cleartext stores at~/.git-credentialsand~/.config/git/credentials. This head's refactoredcredentialDenyReadPathsInnever had those paths — they are missing from the checkout, not removed by this patch. Port them into the new helper (deny the credentials file under~/.config/git, not the wholegitdirectory), bring over the #816 tests, and extendinternal/cli/sandbox_test.gowantDenyReadso the policy JSON golden catches a regression. -
[P1] On rebase, restore
ZERO_DAEMON_REMOTE_TOKEN_FILEscrub from #677
internal/sandbox/runner.go:1080
mainscrubs bothZERO_DAEMON_REMOTE_TOKENandZERO_DAEMON_REMOTE_TOKEN_FILEbecauseTokenFromEnvaccepts either form. This head only scrubs the inline variable. Restore the file-pointer scrub andTestScrubSensitiveEnvcoverage frommainwhen rebasingrunner.go. -
[P1] Give command-controlled token overrides durable Linux protection
internal/sandbox/profile.go:254
internal/sandbox/linux_helper.go:296
f25b3e7closes the process-trusted out-of-tree rename leak via fail-closed bwrap validation. Command-supplied overrides (CommandSpec.Env, including MCP-injected env) are still weaker by design:appendUntrustedcopies onlyPathsandDirs, neverEnsureDirs, and tests require those paths to stay out ofProcessTrustedDenyReadFiles. On bubblewrap, absent token files or.publishdirectories are skipped, existing files get a/dev/nullbind that storeRenamecan bypass, and plaintext in an unmasked.publishtree during the run stays readable. Please give command-env token paths the same durability as process-trusted stores without creating command-controlled host directories outside the token tree. -
[P1] Pin relative process token denies to the store path, not rolling
Getwd
internal/sandbox/profile.go:288
internal/oauth/store.go:138
The stores resolve a relativeZERO_OAUTH_TOKENS_PATHwithfilepath.Absonce at open and write to that fixed path thereafter.credentialDenyReadPathsre-resolves the same override against currentos.Getwd()on everyBuildCommandPlan. After a cwd change, the profile can deny the wrong path while the real store stays readable. Derive process-trusted token denies from the same resolved location the writers use. -
[P2] Make
zero sandbox policy --jsonshow command-scoped credential denies
internal/cli/sandbox.go:71
internal/sandbox/adapters.go:139
Execution usespermissionProfileFromPolicy(..., spec.Dir, spec.Env);zero sandbox policy --jsonstill usesPermissionProfileFromPolicywith no command context, so exportedpermissionProfileunder-reportsdenyReadIfExistsversus nested commands. Thread command context into introspection or document that the JSON view is process-env-only.
…fixes Resolves the two sandbox conflicts deliberately rather than by side: - internal/sandbox/profile.go keeps this branch's multi-home credentialDenyReadPathsIn and ports Gitlawb#816's git credential-store denies into it. ~/.git-credentials joins candidates only, not dirs, because it is a file and dirs drives directory-shaped handling; the XDG credentials file is added before the zero-directory handling so a normalization failure there cannot drop it. Gitlawb#816's tests come with it, rewritten against the new helper, and the policy JSON baseline in internal/cli/sandbox_test.go now lists both paths so the exported contract catches a regression. - internal/sandbox/profile_test.go stays this branch's external-package file; the ported Gitlawb#816 tests need unexported symbols and live in their own internal test file. runner.go's ZERO_DAEMON_REMOTE_TOKEN_FILE scrub from Gitlawb#677 merged cleanly and is verified present. internal/config/command.go and the rest of main come through untouched. Also carries the review's two design fixes and the introspection note; see the following commit message trailers in the PR discussion for what each one does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All six findings addressed in One thing up front, since it affects how you read the diff: the merge commit carries both the conflict resolution and the two design fixes. I resolved and validated them together and did not want to push a merge whose tree I had not finished testing. If you would rather review them apart, say so and I will split it. [P1] Merge with
[P1] #816 git credential denies. Ported, with two placement decisions worth flagging: [P1] #677 token-file scrub. Merged cleanly from [P1] Command-supplied token overrides. New Two things keep this from firing on every command: entries already covered by a user deny or by a directory Zero actually masks are filtered out (so the default store inside [P1] Relative token denies. The process base directory is now captured once during package initialization instead of re-read per This changed the contract two existing tests relied on, so they get a seam rather than a rewrite: [P2] Tests. Two new regressions beyond the ported #816 pair, both verified on Linux:
Validation
One pre-existing Linux failure I did not introduce and left alone: |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable merge-blocking issues from my review of the #675 credential-deny surface at head 64ca22e5.
The default-path fix is implemented and tested: ~/.config/zero is denied whole with plugin/specialist/command carveouts, EnsureDenyReadDirs plus bubblewrap masking covers the mid-session race for Zero-owned stores, OAuth/MCP publication writes pair with .publish denies, and parent/command credential roots are unioned without command-controlled host directory creation. Required CI is green. Windows read denial remains correctly deferred to #662.
Rechecking the prior draft against current code: out-of-tree process-trusted overrides do not leave a live bubblewrap bypass at head — validateLinuxBwrapPermissionProfile refuses launch when ProcessTrustedDenyReadFiles is populated (TestEngineBuildCommandPlanValidatesBwrapBeforeCreatingRuntime). Git credential paths and absent third-party stores follow the same documented best-effort model as ~/.aws. AWS/Azure env override gaps predate this PR.
Follow-up (not blocking merge) @Vasanthdev2004
-
[Needs maintainer decision] Linux policy for out-of-tree
ZERO_OAUTH_TOKENS_PATH/ZERO_MCP_OAUTH_TOKENS_PATH
internal/sandbox/linux_helper.go:223
internal/sandbox/profile.go:250
Bubblewrap cannot durably deny a replaceable file pathname; the current answer is fail-closed (sandboxed commands refuse to start when an out-of-tree process-trusted override is set). That is secure at head but breaks users who rely on a custom absolute token path outside~/.config/zero. The alternative is committed-path masking after first save without masking workspace parents. Pick the product tradeoff explicitly rather than treating fail-closed as a temporary gap. -
[P2] Deny the filesystem path behind
ZERO_DAEMON_REMOTE_TOKEN_FILE(#677 follow-up)
internal/sandbox/runner.go:1084
internal/sandbox/profile.go:356
Scrubbing the variable in this PR closes half of the #677 leak class. The on-disk bearer token at a known path (for example under~/.zero, which is outside the~/.config/zerodeny tree) remains readable under the read-all posture. That gap largely predates this PR; extend the credential baseline the same wayGOOGLE_APPLICATION_CREDENTIALSis handled. -
[P3] Include
permissionProfileNoteonzero sandbox policy --effective --json
internal/cli/sandbox.go:250
internal/cli/sandbox.go:112
Plain--jsondocuments that the embedded profile is process-only and that command context can add denies or trigger bubblewrap fail-closed rejection.--effective --jsonserializes the same process-derived profile without that note, even though doctor/TUI steer operators to--effectivefor inspection.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting one change, then I'm happy to take this.
I re-verified from scratch on the current head (64ca22e) rather than standing on my July 25 approval — the design has moved a long way since then, and the fail-closed markers and publication directories are new. The default path is right and the tests around it are good: TestLinuxBwrapKeepsCarveoutsReachableInsideMaskedDir actually pins --perms 111 and the bind/remount ordering, and TestCommandSuppliedTokenOverrideFailsClosedOnBubblewrap asserts the host wasn't touched on refusal, which is the part that usually gets skipped. I drove the real profile builder with the Windows early-return at profile.go:237 temporarily lifted so I could see what Linux and macOS actually get; with no override, ~/.config/zero lands whole in DenyReadIfExists, plugins/specialists/commands come back as carveouts, EnsureDenyReadDirs has the config dir, and bwrap validation passes. That's the shape you documented and it holds.
The one blocker: ZERO_OAUTH_TOKENS_PATH now makes the Linux sandbox unusable.
internal/cli/auth.go:609 documents that variable in zero auth --help as the supported way to relocate the token store. Point it anywhere outside $XDG_CONFIG_HOME/zero and the profile puts the store and its .secret sibling in ProcessTrustedDenyReadFiles, validateLinuxBwrapPermissionProfile (linux_helper.go:223) returns an error, and Engine.BuildCommandPlan (runner.go:186) hands that straight back — so PrepareExecution never launches anything. Every sandboxed command in the session fails with:
bubblewrap cannot securely deny process-trusted credential file across atomic replacement: /elsewhere/tokens.json, /elsewhere/tokens.json.secret
which names an internal concept and no way out. On main this couldn't happen at all — credentialDenyReadPaths(policy) had no marker list.
Worse, it fires when the file provably cannot exist. I ran the same case with ZERO_OAUTH_STORAGE=keyring, where oauth.NewStore takes the keyring branch and never writes that path: ProcessTrustedDenyReadFiles was still populated and bwrap still refused. credentialPathOptionsFromEnvironment reads the path variable without ever consulting the storage backend.
Two asks, either is enough to unblock:
- Skip the fail-closed markers when the resolved storage backend isn't file-based.
- Make the message actionable — it should say the store must live under
$XDG_CONFIG_HOME/zero, or be listed inallowRead. I confirmedallowReadis a real escape hatch (credentialPathReincludeddrops the marker and validation passes), it's just undiscoverable from the error.
Masking the store's parent the way the default case does would be the nicer fix, but I'm not asking for that here.
Worth fixing while you're in there, not blocking. Command-supplied env values become deny targets with no check that they overlap the sandbox's own roots. Building a profile with CLOUDSDK_CONFIG=<workspace> in the command env puts the workspace in DenyReadIfExists, and the bwrap args then read --bind <ws> <ws> … --perms 000 --tmpfs <ws> --remount-ro <ws> — bound, then masked unreadable, because the deny loop at linux_helper.go:307 runs after the write-root binds. Seatbelt emits four deny rules naming the workspace. Five keys take an arbitrary path that turns into a whole-directory mask: CLOUDSDK_CONFIG (profile.go:373), NPM_CONFIG_USERCONFIG (386), NETRC (400), KUBECONFIG (413), GOOGLE_APPLICATION_CREDENTIALS (426); HOME, XDG_CONFIG_HOME, DOCKER_CONFIG and GH_CONFIG_DIR add fixed-name children under a command-named root. It reaches there from internal/mcp/client.go:152, internal/hooks/dispatch.go:116 and internal/plugins/activate.go:711.
To be straight about the severity: the profile is per-command, so a command can only degrade its own view — I tried and could not turn this into cross-command harm, and no host mutation happens because command-derived roots never reach EnsureDenyReadDirs. But pathsOutsideOverlappingRoots at profile.go:635 is already exactly the right helper; running command-derived candidates through it against the profile's read/write roots would close it, with a regression test alongside TestCommandSuppliedTokenOverrideFailsClosedOnBubblewrap.
The same gap has a second face: with CLOUDSDK_CONFIG=<configHome> in a command env, the pruning in finalizeCredentialDenyPaths collapses ~/.config/gh/hosts.yml, ~/.config/gcloud, ~/.config/git/credentials and ~/.config/zero into one ~/.config deny and empties EnsureDenyReadDirs. Everything stays denied on both backends, so it's not a hole, but it also masks ~/.config/git/config, which userGitConfigReadPaths grants deliberately.
Three scope notes, no code change needed.
credentialDenyReadPaths returns empty on Windows at profile.go:237, so none of this reaches a Windows user. "Fixes #675" will auto-close the issue with the Windows half fully exposed — please drop the closing keyword or carve out the Windows half in the body, so the tracker doesn't show the credential-read gap as done. Same request I made last time; it matters more now that the rest is landing.
On macOS, the default provider-credential backend isn't the file this protects. internal/credstore/credstore.go:90 selects keyring when goos == "darwin" && kr.Available(), and internal/keyring/keyring.go:108 reads it back with security find-generic-password -s <service> -a <account> -w — which a sandboxed command can still execute. The directory deny is still worth having on a Mac, because oauth.NewStore defaults to a plaintext oauth-tokens.json and that is now covered. Just don't let the body read as "the credential store is denied on macOS"; it isn't, by default. I've been wrong about exactly this before on #662, so I checked the per-GOOS default rather than assuming.
And unchanged, correctly: engine.go:158 still builds ReadExclusions from policy.DenyRead alone, so read_file/grep/glob don't see the credential baseline. That's the right call for #675 and consistent with keeping Policy.DenyRead reflecting user config only — worth its own issue rather than scope creep here.
Minor, take it or leave it: nothing prunes the new <store>.publish directories, so a crash between createPublicationFile and the rename leaves plaintext token or key bytes under publish-* indefinitely. Same exposure class as the old .tmp-<pid>-<nano> file, and it's inside the denied directory in the default layout, so it isn't a regression — there's just no reaper.
Test-wise I ran ./internal/sandbox, ./internal/oauth and ./internal/cli on Windows; sandbox and oauth are green, and the only cli failure is the pre-existing TestBuildServeScopeKeepsLexicalPaths symlink-privilege one, which isn't in this diff. CI is green on all three platforms.
Thanks for grinding through this one — the carveout normalization and the symlink rejection in normalizeCredentialCarveoutPath are the right answers to the earlier rounds, and they're properly pinned by tests.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfa-8469-731e-87c0-b26b7641cf59 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Addressed the latest review blocker in e106d06. What changed:
Validation completed with Go 1.26.5:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked the current head and found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve the deny mask for command-scoped token roots created after launch
internal/sandbox/profile.go:268
The profile builder treats everyCommandSpec.Envroot as untrusted and therefore copies onlyPaths/Dirsout ofcredentialDenyReadPathsIn; it intentionally discardsEnsureDirs. The bwrap planner then skips a missingDenyReadIfExistsmount target. A command with a freshHOME/XDG_CONFIG_HOMEconsequently starts with no mount over<XDG_CONFIG_HOME>/zero; a concurrent hostzero auth(or another host Zero process) can createoauth-tokens.jsonafter the read-only host-root bind is established, and the running command can read it. The new mid-session smoke case supplies exactly this command-scoped environment but does not materialize its directory before launch.The root decision needs to be made at the command-env trust boundary, rather than by adding another best-effort deny. Either reject/fail closed on command-controlled credential roots that require future-path protection on bwrap, or introduce a durable isolation mechanism that does not create arbitrary command-selected host directories. Preserve the existing process-environment
EnsureDirsbehavior only for roots Zero itself owns, and add a real bwrap regression that writes the token from a host goroutine after the child has started. -
[P2] Do not create publication directories for keyring OAuth overrides
internal/sandbox/profile.go:556
The new keyring exception only changescredentialFinalTokenFiles, which removes the atomic-replacement fail-closed marker.credentialDenyReadPathsInstill unconditionally treats every OAuth override as a file store, adding<override>.publishand<override>.secret.publishtoEnsureDenyReadDirs. Linux planning callsMkdirAllfor those trusted ensure paths before launch, butoauth.NewStore's keyring branch never callsfileBloborcreatePublicationFile. A supported configuration such asZERO_OAUTH_STORAGE=keyringwithZERO_OAUTH_TOKENS_PATH=/outside/tokens.jsontherefore creates unused host directories below/outsidewhenever a sandboxed command runs.Make storage selection part of the candidate derivation: keep any intentional pathname-level deny for the override if it is useful to the general credential baseline, but do not add file siblings, publication directories,
Dirs, orEnsureDirsfor OAuth when the effective backend iskeyring. Keep MCP's legacy file-store handling independent. Extend the new keyring override test to assert that bwrap validation and planning leave both publication paths absent. -
[P2] Prevent command credential settings from masking the command's allowed roots
internal/sandbox/profile.go:268
Command-provided credential variables are appended without being compared with the resolvedReadRootsandWriteRoots. For example, a plugin, hook, or MCP command withCLOUDSDK_CONFIG=<workspace>adds the whole workspace toDenyReadIfExists; bwrap first bind-mounts that workspace writable and later overlays it with the unreadable tmpfs mask, while Seatbelt emits the corresponding read deny. The command can no longer access its own working tree. The same class affects directory-valued settings such asXDG_CONFIG_HOMEandDOCKER_CONFIGwhen they overlap an allowed root.Finalize command-derived candidates against the resolved profile roots before rendering either backend. The existing overlap helper is close to the needed rule: discard a command-derived candidate when it contains, or is contained by, a root deliberately granted to that command; do not apply that relaxation to process-derived credential roots. Add backend-level tests for a workspace-valued
CLOUDSDK_CONFIGand for a nested credential file that should remain denied when it does not overlap a permitted root. -
[P3] Include the permission-profile scope warning in effective JSON
internal/cli/sandbox.go:253
Plainzero sandbox policy --jsonaddspermissionProfileNotebecause its plan is derived only from the current process environment and cwd.zero sandbox policy --effective --jsonserializes that same process-derivedBackendPlan, but its separate payload omits the note. Execution can add command cwd/environment credential denies, and the effective view is the operator-facing inspection path, so JSON consumers can incorrectly treat it as the complete enforcement profile.Put the same scope note in the effective JSON payload (or refactor both JSON paths to use one common profile-description field), and extend the effective JSON test/golden alongside the existing plain-JSON coverage. The text view should communicate the same limitation if it is intended to be a complete inspection surface.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-c723-74d9-a323-026040b94436 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Addressed the current-head review findings in
Regressions cover missing/file/existing command directory behavior, host non-mutation, Bubblewrap and Seatbelt allowed-root rendering, retention of non-overlapping denies, keyring planning/publication paths, effective JSON/text output, and the Linux integration fail-closed path. Vasanthdev2004's older keyring blocker is superseded by this change: keyring overrides neither trigger atomic-replacement fail-closed markers nor create unused publication directories. Validation run exactly:
@jatmn @Vasanthdev2004 please re-review the updated head when convenient. |
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] Reapply credential denies after the extension-root carveouts
internal/sandbox/runner.go:675
A token override may legitimately resolve beneath one of the required extension carveouts, for exampleZERO_OAUTH_TOKENS_PATH=$XDG_CONFIG_HOME/zero/plugins/tokens.json.finalizeCredentialDenyPathsintentionally retains that token and its.secretas explicit children of the denied Zero config directory, because thepluginscarveout makes the enclosing directory deny insufficient. However,seatbeltProfileFromPermissionProfileemits every deny rule first and then appends(allow file-read* ... (subpath ".../zero/plugins")). The code documents Seatbelt as last-match-wins, so the broad, later allow overrides both retained child denies and lets a sandboxed macOS command read the OAuth credentials.The root cause is treating a carveout as a simple trailing allow even when the profile contains a more-specific denial inside it. Preserve the intended precedence in the rendered Seatbelt policy: emit the nested credential denies after their enclosing carveout (or split the carveout into non-credential portions / omit it when it contains a protected credential). Add a macOS rendering regression that sets an override below each of
plugins,specialists, andcommands, and asserts the final SBPL ordering keeps the token and.secretdenied while ordinary extension files remain readable. -
[P2] Do not classify the legacy MCP migration source as an atomically replaced token file
internal/sandbox/profile.go:488
ZERO_MCP_OAUTH_TOKENS_PATHidentifies only the legacy source consumed bymcp.NewTokenStore. That code reads the file for one-time migration and then renames it to.migrated; it writes the unified store through the separately resolved OAuth store (ZERO_OAUTH_TOKENS_PATHor its default). The PR adds the legacy source toProcessTrustedDenyReadFiles/CommandDenyReadFinalFiles, sovalidateLinuxBwrapPermissionProfilerejects every Bubblewrap-sandboxed command when that legacy path sits outside the Zero config directory. This rejection is based on atomic replacement, but the legacy source is not atomically published during normal operation: an ordinary read-deny mount remains durable for it. A user retaining an external legacy MCP store is therefore unable to run unrelated sandboxed commands.The root cause is conflating the migration input with the writable unified token-store outputs. Model these paths according to their lifecycle rather than their common credential content: retain the legacy path and
.migratedbackup in the ordinary deny candidates, but excludeZERO_MCP_OAUTH_TOKENS_PATHfrom the atomic-replacement final-file markers. Keep fail-closed handling for the actual writable OAuth store and its encryption-key sibling. Add a Bubblewrap planning regression with an external legacy MCP path that verifies the source is denied without rejecting the command, alongside the existing tests that verify true atomic writers fail closed.
Reapply credential denies after Seatbelt extension carveouts and model the legacy MCP token path as a migration input rather than an atomic writer. Retain Bubblewrap fail-closed markers for OAuth outputs exposed through readable carveouts, and add regressions for both platform policies. Amp-Thread-ID: https://ampcode.com/threads/T-019fbf55-f6b0-71fc-b384-652ba32b9645 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Addressed the latest review findings in [P1] Reapply credential denies after the extension-root carveouts —
[P2] Do not classify the legacy MCP migration source as an atomically replaced token file — Validation: |
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.
Related to #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