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.
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] Do not deny the whole user Zero config directory
internal/sandbox/profile.go:254
This makes the supported user-plugin root ($XDG_CONFIG_HOME/zero/plugins) unreadable. Plugin tools are executed through the sandbox and their commands/scripts live below that root, so an installed user plugin fails to start on both bwrap and Seatbelt. Narrow the deny to credential-bearing paths (while retaining coverage for publication artifacts), or explicitly preserve the supported non-secret subtrees. -
[P1] Resolve relative token overrides from the same directory as the stores
internal/sandbox/runner.go:181
The profile now resolvesZERO_OAUTH_TOKENS_PATH,ZERO_MCP_OAUTH_TOKENS_PATH, and relative XDG paths fromspec.Dir; the OAuth and MCP store resolvers usefilepath.Abs, which resolves from the Zero process's current working directory. For example, when Zero starts in/repowithZERO_OAUTH_TOKENS_PATH=tokens.jsonand runs a sandboxed command in/repo/subdir, the profile masks/repo/subdir/tokens.jsonwhile the real/repo/tokens.jsonremains readable through the read-all root. The new tests make both directories the same and therefore miss this bypass. -
[P1] Do not leave newly created credential paths visible to an existing Linux sandbox
internal/sandbox/linux_helper.go:282
DenyReadIfExistsentries that are absent while bwrap assembles the namespace are skipped. The root is then live-mounted read-only without a mask, so a durable sandboxed MCP server started before~/.config/zeroor a token file exists can read that credential as soon as another process creates it. The added smoke case only creates the store before constructing the later sandbox run; it does not cover the long-lived-session race this change claims to protect. -
[P1] Keep publication files unguessable or protect their inode after creation
internal/oauth/store.go:414
Replacing randomizedCreateTempnames with a predictable<token>.tmpmakes the plaintext token (and, via the analogous secret path, the encryption key) observable to a sandboxed same-user process with write access to an override parent such as a workspace or/tmp. Such a process can wait for the known file, rename the opened inode into an allowed path, and read the data written through the still-open descriptor; the lock only coordinates cooperating Zero processes. On Linux, an absent sibling is also skipped byDenyReadIfExistsat sandbox setup, which makes this practical for a long-lived sandbox. Use a private protected directory/randomized file strategy that cannot be moved into an allowed path, or enforce the required write and read protections for the entire publication lifecycle.
Four problems with the credential deny baseline, all in the same mechanism. The whole-directory deny over $XDG_CONFIG_HOME/zero also hid the supported user plugin, specialist, and command roots, whose commands and scripts are executed through this sandbox, so an installed user plugin could no longer start. Narrowing the deny to itemized files was not an option: the stores publish through temporary names and a concurrent login can add a store that did not exist when the profile was built, and only a directory rule covers both. The directory deny therefore stays, and the non-secret subtrees are carved back out through a new DenyReadCarveouts field: seatbelt re-allows them after the deny (SBPL is last-match-wins) plus ancestor metadata so path resolution can reach them, and bubblewrap masks the directory with --perms 111 instead of 000 -- unlistable but traversable -- and re-binds the carveout read-only before the tmpfs is remounted read-only. A relative ZERO_OAUTH_TOKENS_PATH, ZERO_MCP_OAUTH_TOKENS_PATH, or XDG_CONFIG_HOME was resolved against the command directory only, while oauth.ResolveStorePath and mcp.ResolveTokenStorePath call filepath.Abs, i.e. resolve against the Zero process working directory. A command run from a subdirectory therefore had the wrong path denied and the real store stayed readable under the read-all posture. Relative candidates now resolve against both directories, since the process that resolves the value is not necessarily the one that writes the store. The regression test runs the command from a different directory than the process, which the previous tests did not. bubblewrap cannot mount over a path that is absent when the namespace is assembled, so a store created mid-session stayed readable through the live read-only host-root bind for the rest of a long-lived sandbox session. The directories Zero owns -- its config directory and the per-store publication directories -- are now reported as EnsureDenyReadDirs and created (0700) before the namespace is assembled, so their mask always exists. Third-party stores such as ~/.aws are still skipped when absent: Zero must not create them, and that limitation stays documented on the field. The fixed <store>.tmp publication sibling made the plaintext token, and via the same path its encryption key, observable to a same-user sandboxed process with write access to an override parent: it could wait for the known name and rename the opened inode into an allowed path. Both stores now publish through a per-store <store>.publish directory with a randomly-named file inside. The directory name is derived from the store path, so the profile can deny it up front, while the random name inside is what cannot be waited for or renamed away. A test keeps the duplicated suffix in step with oauth.PublicationDirSuffix. Also adds the seatbelt render coverage that was missing: dropping DenyReadIfExists from denyReadRules made every credential store readable again on macOS without failing a single test.
|
Pushed [P1] Whole-directory deny made the user plugin root unreadable — real, and the fix could not be "itemize the credential files". The reason the deny was directory-level in the first place is the other two findings: the stores publish through temporary names, and a concurrent login can create a store that did not exist when the profile was built. Only a directory rule covers both. So the directory deny stays and the supported non-secret subtrees are carved back out through a new Both backends had to learn it:
Carveouts are dropped when an [P1] Relative overrides resolved from the wrong directory — confirmed, and gnanam1990 flagged the same thing. [P1] Paths created mid-session stayed visible on Linux — bubblewrap genuinely cannot mount over an absent path, so the previous answer was to skip it; the fix is to make the path exist. [P1] Predictable gnanam1990 [Minor] no test asserted Vasanthdev2004's residuals — (1) in-process read tools still resolve exclusions from Validation: |
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] Fix the failing policy JSON golden test
internal/cli/sandbox_test.go:567
The test removesdenyReadIfExistsbefore comparing the golden output, but it leaves the newly addeddenyReadCarveoutsandensureDenyReadDirsfields intact. Both contain the test's temporary-home path, soTestRunSandboxPolicyJSONGoldenIncludesManagerBaselineFieldsnow fails on every platform and all three required Smoke jobs are red. Assert and normalize/remove the new fields as part of the golden-test setup. -
[P1] Normalize carveouts against the denied Zero config directory
internal/sandbox/profile.go:435
zeroDiris normalized as a candidate, while nonexistentplugins,specialists, andcommandscarveouts are normalized independently. On macOS, an existing/tmp/.../zeroresolves to/private/tmp/.../zero, but its nonexistent children remain/tmp/...;pathWithinRootthen drops every carveout. The resulting profile denies the entire Zero config directory, preventing installed plugin/specialist/command content from being read. This is also the current macOS Smoke failure inTestCredentialDenyReadPathsIn. -
[P1] Do not allow credential carveouts to resolve through symlinks
internal/sandbox/profile.go:435
normalizeProfilePathsresolves aplugins,specialists, orcommandssymlink before it is accepted as a carveout. If (for example)~/.config/zero/pluginspoints atoauth-tokens.json, the resolved token path is re-allowed after the directory-level credential deny (and is re-bound by bwrap), so a sandboxed command can read the token. Only permit the intended real directory subtrees, without following a carveout symlink outside that subtree. -
[P1] Preserve the parent process's credential roots when a child overrides its environment
internal/sandbox/profile.go:249
Supplying a non-nilCommandSpec.Envmakes the deny baseline use only that environment. MCP stdio servers pass project-configured environment overrides throughmergeProcessEnv, so a project can setHOME/XDG_CONFIG_HOMEto an empty directory. The profile then omits the running Zero process's real config and token paths, while retaining the read-all root bind; the server can read the host OAuth store. Union the parent-process credential roots with child-environment roots rather than replacing the former. -
[P1] Avoid mounting automatic credential masks beneath a user deny-read mask
internal/sandbox/linux_helper.go:279
A policy that denies$XDG_CONFIG_HOME(or$HOME) still retains the nested automatic$XDG_CONFIG_HOME/zeroentry inDenyReadIfExists. Bubblewrap masks the parent first, then cannot create the nested mount destination in that read-only tmpfs, so every sandboxed command fails before execution. Drop automatic paths already covered by a user deny, or construct the mounts so nested targets remain valid. -
[P1] Do not create command-controlled credential directories on the host before sandboxing
internal/sandbox/linux_helper.go:287
EnsureDenyReadDirsincludes paths derived fromXDG_CONFIG_HOMEand both OAuth override variables, andensureLinuxDenyReadDirsunconditionally callsos.MkdirAllbefore bwrap starts. A sandboxed MCP command can therefore cause host directories such as/home/user/outside/zeroor/home/user/outside/tokens.publishto be created outside its workspace/write roots merely by setting its environment. Restrict any precreation to trusted, permitted roots or keep missing paths best-effort.
Preserve process credential roots alongside command overrides while limiting host precreation and read carveouts to trusted process-derived paths. Canonicalize future credential roots through existing path aliases, reject symlink carveouts, and collapse automatic masks already covered by user or automatic directory denies. Normalize the policy golden and add regression coverage across profile, Seatbelt, and bubblewrap rendering. 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>
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.
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