fix(sandbox): preserve user config and retry denied writes - #801
Conversation
Keep user identity and configuration paths discoverable while retaining managed cache and temporary storage. Classify native sandbox failures as typed recoverable denials so approved commands can retry outside the sandbox. Add regression coverage for global Git ignores, typed denial handling, and permission retry behavior.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change reduces synthetic sandbox runtime directories, preserves caller configuration, expands credential deny-read paths, and refines structured sandbox-denial retry handling with additional tests. ChangesSandbox runtime environment
Credential deny-read paths
Structured sandbox denials
Estimated code review effort: 3 (Moderate) | ~25 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 docstrings
🧪 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: 4
🧹 Nitpick comments (1)
internal/tools/sandbox_denial_test.go (1)
28-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd false-positive regression cases for the keyword heuristic.
The table covers obvious denial strings but not ordinary output containing
"sandbox","failed to write file", or unrelated permission/read-only errors. Add those cases and assert that both denial metadata and the returned denial remain unset.As per coding guidelines,
**/*_test.gofiles should add regression tests for behavior changes; this heuristic needs explicit false-positive coverage.🤖 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/tools/sandbox_denial_test.go` around lines 28 - 38, Expand the test table in the markLikelySandboxDenial test to include sandboxed commands whose output contains “sandbox”, “failed to write file”, and unrelated permission or read-only errors. Set each case to expect no denial, and assert both the SandboxLikelyDeniedMeta metadata and the returned denial remain unset.Source: Coding guidelines
🤖 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/agent/loop.go`:
- Around line 1622-1626: Update the denial predicate in the surrounding loop
logic to also require the typed platform denial’s Recoverable field to be true
before returning true. Preserve the existing source, capability, and next-action
checks so non-recoverable denials cannot enter the unrestricted retry or
host-escalation flow.
In `@internal/sandbox/runtime_state_test.go`:
- Line 27: Extend the runtime state filesystem assertions around the existing
Cache, Data, and Temp loop to verify the obsolete synthetic home, config, and
state paths are absent. Add regression coverage using the test’s established
assertion style, while preserving the existing existence checks for the active
directories.
- Around line 226-229: Update the environment setup in the runtime-state Git
test to remove inherited GIT_* override variables from os.Environ(), including
GIT_CONFIG_GLOBAL, GIT_DIR, and GIT_CONFIG_COUNT, and disable system Git
configuration before invoking Git. Add regression coverage verifying these
host-level settings cannot override the test’s HOME and XDG_CONFIG_HOME
configuration.
In `@internal/tools/sandbox_denial.go`:
- Around line 9-17: Narrow sandbox-denial detection to require platform-specific
evidence before treating generic keywords such as “sandbox” or “failed to write
file” as denials; update the heuristic in internal/tools/sandbox_denial.go lines
9-17 and only construct unrestricted host denial metadata in lines 35-54 after
stronger corroboration. Add regression cases in
internal/tools/sandbox_denial_test.go lines 28-38 covering generic application,
permission, and read-only errors, asserting neither denial metadata nor a
pointer is produced.
---
Nitpick comments:
In `@internal/tools/sandbox_denial_test.go`:
- Around line 28-38: Expand the test table in the markLikelySandboxDenial test
to include sandboxed commands whose output contains “sandbox”, “failed to write
file”, and unrelated permission or read-only errors. Set each case to expect no
denial, and assert both the SandboxLikelyDeniedMeta metadata and the returned
denial remain unset.
🪄 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 Plus
Run ID: 289f6db2-c754-4b9c-91f7-df23c5876269
📒 Files selected for processing (9)
internal/agent/loop.gointernal/agent/loop_test.gointernal/sandbox/runtime_state.gointernal/sandbox/runtime_state_test.gointernal/tools/bash.gointernal/tools/exec_command.gointernal/tools/exec_command_test.gointernal/tools/sandbox_denial.gointernal/tools/sandbox_denial_test.go
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/tools/sandbox_denial_test.go`:
- Around line 39-42: Expand the assertions in the markLikelySandboxDenial test
to validate the complete denial structure, not just whether denial is non-nil.
Check Source, unrestricted Capability, Recoverable, NextAction, and the
reason/keyword metadata used by the retry prompt, using the expected values from
each test case and preserving the existing nil/non-nil assertion.
🪄 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 Plus
Run ID: b8019c76-b4ef-4dd4-84a2-3173d8acaf94
📒 Files selected for processing (5)
internal/agent/loop.gointernal/agent/loop_test.gointernal/sandbox/runtime_state_test.gointernal/tools/sandbox_denial.gointernal/tools/sandbox_denial_test.go
💤 Files with no reviewable changes (1)
- internal/tools/sandbox_denial.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/agent/loop.go
- internal/agent/loop_test.go
- internal/sandbox/runtime_state_test.go
|
Windows Smoke is currently blocked by the existing TestLoadProviderCommandTimeout startup-timing flake in internal/config/command_test.go: the test tries to read sleep.pid before the Windows helper creates it. The sandbox, agent, and tools packages pass on Windows, and this PR does not change internal/config. The same job passed once on c2429d0, then reproduced the identical missing sleep.pid failure twice after the test-only 3b9be60 commit. The scoped fix is already in #800; that PR should land independently rather than mixing its unrelated test changes into this sandbox PR. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
The fix does what it claims. A native-sandbox read-only / permission-denied shell failure now produces an ExecutionOutcome.Denial (Source=PlatformSandbox, Capability=Unrestricted, Recoverable=true, NextAction=RequestApproval) that matches the narrowed sandboxDeniedShellResult predicate in internal/agent/loop.go, so the command re-enters the unrestricted retry path. Wiring is coherent end-to-end: markLikelySandboxDenial (internal/tools/sandbox_denial.go) → execExecutionOutcome (exec_command.go:439/554) → StateDenied → loop.go retry prompt. PR-added tests pass on the PR HEAD (TestExecCommandInfersNativeSandboxDenialFromOutput, TestLikelySandboxDenialMetadataFromCommandOutput, TestExecCommandUsesStructuredAdapterDenial, TestTypedExecutionOutcomeOverridesLegacySandboxHeuristics, and the sandbox runtime/env-preservation tests). gofmt/vet/build clean.
Two minor issues, neither blocking merge.
[Minor] Non-sandbox "permission denied" errors are misclassified as recoverable sandbox denials — PR-introduced
internal/tools/sandbox_denial.go:40
The heuristic matches the bare substring "permission denied" in stderr whenever the command ran sandboxed and exited non-zero (bash.go:184-191 / exec_command.go:440-447 gate only on sandboxed && exitCode != 0). Ordinary OS permission failures therefore get classified as recoverable sandbox denials:
git push git@github.com:foo/barwith no valid key →git@github.com: Permission denied (publickey)., exit 128cat /etc/shadow→cat: /etc/shadow: Permission denied, exit 1- any
sudo ...invocation that is denied
Each yields Denial{Kind:CapabilityUnrestricted, Source:PlatformSandbox, Recoverable:true, NextAction:RequestApproval}, so maybeRetryUnsandboxedAfterSandboxRestriction (loop.go:1453) prompts the user to re-run unsandboxed with reason "sandbox blocked command execution". Because the sandbox never caused the failure, the unsandboxed retry hits the identical OS error (SSH auth / file mode / sudo) and the user is shown a factually wrong "sandbox blocked" reason. All three sample strings were confirmed to match the keyword heuristic.
Fix: require a corroborating signal before firing on "permission denied" — e.g. only when the target is a known-restricted write root or the denial monitor/adapter also flagged a block — or drop the broad "permission denied" keyword and rely on the sandbox-specific signals (EROFS "read-only file system", seccomp, landlock) plus structured adapter denials.
[Minor] bash.go sandbox-denial inference is untested on the real shell tool path — PR-introduced
internal/tools/bash.go:184
Deleting the entire if adapterReport.Denial == nil && reportErr == nil { adapterReport.Denial = markLikelySandboxDenial(...) } block (bash.go:184-192) produces zero new failures across go test ./internal/tools/... and go test ./internal/agent/... — only the pre-existing environment failures remain. So a future edit that breaks this wiring would silently disable sandbox-denied-write retry for real bash tool commands (e.g. touch "$HOME/probe" → "Read-only file system" under native enforcement no longer produces a Denial, and loop.go's retry never fires) with the suite staying green. The existing coverage doesn't reach this path: TestExecCommandInfersNativeSandboxDenialFromOutput drives exec_command.go, and the agent-level TestRunRetriesShellUnsandboxedAfterSandboxDeniedExit uses a fake tool that hand-builds the ExecutionOutcome, so neither exercises bash.go's plan.Wrapped && plan.EnforcementLevel == EnforcementNative guard or its stdout+stderr passing.
Fix: add a bash-tool-level test analogous to the exec_command one that runs the real bash tool with a native-wrapped plan and a stderr denial keyword, asserting result.ExecutionOutcome.Denial is populated and SandboxLikelyDeniedMeta is set.
Tests
All failures in the three touched packages (3 in internal/tools, 7 in internal/sandbox, 4 in internal/agent) are pre-existing environment failures — the known sandbox/path/symlink issues from the worktree living under /private/tmp. The --- FAIL name sets are byte-for-byte identical between the PR HEAD and the clean base worktree, so none is PR-attributable. -race not run (this is not a concurrency PR).
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Good change in the main line: pointing HOME and the XDG vars back at the caller's real paths so sandboxed tooling sees normal user settings is the right call, and I checked the thing I was most worried about before anything else. It does not widen the read boundary. profile.go is untouched, the posture there is still the deliberate read-all/write-jail one, and credentialDenyReadPaths resolves from os.UserHomeDir() in the Zero process rather than the child's $HOME, so the deny masks still land on the same real paths after the env change. Writes are actually tighter than before, since the old synthetic HOME sat inside the runtime root and was therefore writable, while the real home is in no write root. So no credential regression, and #675/#677 are neither helped nor hurt.
Two things I would like fixed before this lands.
1. [blocker] The sandbox denial is inferred from the command's own stdout/stderr again, and it now drives an unsandboxed re-run.
markLikelySandboxDenial (internal/tools/sandbox_denial.go) scans the command output for operation not permitted, permission denied, read-only file system, seccomp, landlock and mints a real execution.Denial{Source: DenialSourcePlatformSandbox, Capability: {Kind: CapabilityUnrestricted, Scope: "host"}, Recoverable: true, NextAction: DenialNextActionRequestApproval}. It sits directly under the comment // Classification is never inferred from stdout or stderr. that #781 added when it deleted this exact heuristic.
That matters more than it did before #781, because loop.go now keys the unsandboxed-retry predicate on precisely that denial shape, and this function is its only producer. So the retry path fires on, and only on, stdout-inferred denials. The strings are generic enough to be produced by ordinary commands: a failing test, a linter, or cat on a root-owned file all print permission denied without the sandbox being involved at all. Since tool output is model- and content-controllable, a command can effectively ask Zero to offer to re-run it without the sandbox.
To be fair on severity: in ask and auto modes the retry is fail-closed and requires a fresh human Allow, so this is not a silent bypass there. But under PermissionModeUnsafe it is taken with no prompt, and the retry resolves to a nil engine, which means ModeDisabled, NetworkAllow, FileSystemUnrestricted and no DenyRead at all, so the credential masks that are the whole safety story above are gone on that path.
The fix I would want is to keep the classification typed: derive the denial from the sandbox adapter's own facts (which is what markStructuredSandboxDenial already does) and let the keyword scan at most annotate a denial that the sandbox layer already reported, never create one. If a heuristic is genuinely needed for a backend that cannot report denials structurally, it should produce a non-recoverable, non-retry-eligible marker rather than one that unlocks the unsandboxed path.
2. [minor] CARGO_HOME looks like collateral damage.
The PR removes data/cargo and the CARGO_HOME override as unused, but it is the only place in the tree that sets it, and cargo does not honour XDG_CACHE_HOME. Every sibling redirect survives (npm, yarn, corepack, pip, GOCACHE, GOMODCACHE), so cargo is the one toolchain left pointing at $HOME/.cargo, which is in no write root. That should make cargo builds fail inside the sandbox rather than land in the managed cache. Worth restoring unless you found it genuinely dead.
Verification note: Smart App Control on my machine is currently blocking the unsigned go toolchain, so this review is source-reading only; I could not run builds or tests locally. CI is green on all three OSes.
Happy to re-review quickly once the denial classification is back to typed facts.
Stop promoting command output keywords into recoverable unrestricted denials, so only structured adapter facts can enter the approved unsandboxed retry path. Restore the managed CARGO_HOME directory and environment override so Cargo remains writable while the caller's real home stays read-only.
7dd3d57
There was a problem hiding this comment.
Verdict
Changes requested, on a single point noted below.
Thank you for this. Preserving the caller's configuration so sandboxed tooling behaves like the user's own shell is a real improvement, and the denial-retry path is carefully constructed. I am requesting changes on a single point, phrased as a question you may be able to close by stating intent rather than by writing code.
Verification. On macOS (darwin/arm64), make fmt-check, go vet ./... and git diff --check are clean, and the full suite passes at 82 packages with no failures. Both halves of the change are covered by tests that genuinely bind to them: reverting internal/agent/loop.go to base while retaining the new tests fails TestRunRetriesShellUnsandboxedAfterSandboxDeniedExit, and reverting internal/sandbox/runtime_state.go fails four tests, including TestSandboxRuntimeEnvironmentPreservesUserConfiguration and TestSandboxRuntimeEnvironmentPreservesGitGlobalIgnore.
I also traced the retry path rather than taking the description on trust. At internal/agent/loop.go:1449-1494 the unsandboxed retry reaches runToolForUnsandboxedRetry only after requestPermission returns an Allow variant, and sandboxRestrictedShellRetryCandidate additionally requires UnsandboxedExecutionAllowed() and refuses commands already marked require_escalated. PermissionModeUnsafe bypasses the prompt, which is that mode's documented contract. Narrowing the typed-denial branch to CapabilityUnrestricted, Recoverable and RequestApproval is a reasonable reading of the original constraint.
The point I would like resolved before this lands. Configuration resolution inside the sandbox now points at the user's real credential locations, while the credential deny-list has not been extended to match. The change removes HOME, XDG_CONFIG_HOME, XDG_DATA_HOME, XDG_STATE_HOME and NPM_CONFIG_USERCONFIG from the sandbox environment (internal/sandbox/runtime_state.go:198), while credentialDenyReadPathsIn (internal/sandbox/profile.go:174) still covers only ~/.aws, ~/.config/gcloud, ~/.azure and $GOOGLE_APPLICATION_CREDENTIALS.
Locations that become the resolution target and are not deny-listed include ~/.npmrc, which holds _authToken and is precisely what the removed NPM_CONFIG_USERCONFIG previously redirected away; ~/.config/gh/hosts.yml; ~/.netrc; ~/.docker/config.json; ~/.kube/config; and ~/.config/zero/config.json, which holds Zero's own provider API keys (internal/config/paths.go:41). The .zero and .agents entries in protectedMetadataNames cover workspace metadata and do not apply to the user configuration directory.
To be fair on severity, this is not new filesystem access on Linux: internal/sandbox/linux_helper.go:242 already binds / read-only, so those files were reachable by absolute path beforehand. What changes is the default, moving exposure from "requires knowing an absolute path" to "any tool that reads its own configuration". On macOS the seatbelt read rule is an allow-list (internal/sandbox/runner.go:670), so reads remain denied there and the effect is platform-dependent.
The tension worth resolving is with the design comment at internal/sandbox/profile.go:209-213, which records that only the global git config files are granted, deliberately excluding ~/.config/git, so that "credentials and the rest of HOME stay unreadable". This change makes the real ~/.config the resolution root while that comment still stands. Extending credentialDenyReadPaths to cover the newly-resolved surface, or documenting why the wider resolution is acceptable, would close the question either way.
$HOME is no longer writable inside the sandbox. The retained overrides cover caches, temporary directories, GOCACHE, GOMODCACHE and CARGO_HOME, but writes elsewhere under $HOME — git config --global, ~/.gradle, ~/.m2, ~/.rustup — will now be denied rather than landing in a synthetic home. The final commit on this branch restores Cargo state, which suggests this was encountered once already; it would be worth confirming the enumeration is complete rather than incremental.
Stale comment at internal/agent/loop.go:1620-1621. The note that a structured narrow denial must never become the legacy unrestricted retry now sits directly above code that converts a typed denial into that retry. The new capability guard arguably honours the intent, but the wording reads as contradicting the code beneath it, and rewording it would help the next reader.
No linked issue. The description carries no Fixes # reference and the closing-issue reference is empty. This is not a ground for closure on a collaborator pull request, but it leaves the change without traceability.
Limitations of this review. I worked on macOS only. The Linux and Windows statements above rest on reading the code; I did not run the Linux TUI smoke test described in the description, and I did not observe the denial classification end to end against a native Linux sandbox. If the credential question turns on Linux behaviour I have not exercised, I would defer to a check on that platform.
Merge is kevin's call per the program gate.
Preserving caller configuration made credential-bearing tool files discoverable under Linux's read-all sandbox profile. Add default and environment-overridden npm, GitHub CLI, netrc, Docker, Kubernetes, and Zero config paths to the credential deny set, with regression coverage. Clarify the typed-denial retry and platform read-model comments without changing the approval behavior.
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 202-236: Update credentialPathEnvironment and
credentialDenyReadPathsForEnvironment to add a CloudSDKConfig override,
defaulting it to ~/.config/gcloud only when unset, and use that value when
appending the gcloud deny-read path. Ensure credentialDenyReadPaths passes
os.Getenv("CLOUDSDK_CONFIG") into the environment so the configured credential
directory is protected.
🪄 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 Plus
Run ID: 4398398c-0986-4b6a-a364-5bda62116f6e
📒 Files selected for processing (3)
internal/agent/loop.gointernal/sandbox/manager_test.gointernal/sandbox/profile.go
CLOUDSDK_CONFIG can relocate gcloud credentials outside the XDG-derived default. Include the configured directory in the credential deny profile and cover the override with a regression test.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested, on one point that is narrower than my last one.
Reviewed at 538b31ed75e5, base 9d5c82d441b7, re-confirmed against the live head before posting.
My previous blocker is resolved, and thoroughly. I asked for the credential deny-list to be extended to cover the locations that the preserved caller environment makes the resolution target. Every location I named is now covered, and the two commits go further than I asked by honouring the environment overrides rather than hard-coding paths — CLOUDSDK_CONFIG, GH_CONFIG_DIR, NETRC, DOCKER_CONFIG, NPM_CONFIG_USERCONFIG and KUBECONFIG, the last correctly split on the list separator. I verified this by driving credentialDenyReadPathsForEnvironment against a synthetic home with every credential file materialised: ~/.aws, ~/.azure, ~/.npmrc, ~/.netrc, ~/.docker/config.json, ~/.config/zero/config.json, ~/.config/gcloud, ~/.config/gh/hosts.yml and ~/.kube/config all come back denied. Nine of nine.
The two stale comments are fixed as well — internal/agent/loop.go:1620 now describes what the code beneath it does, and the profile.go comment no longer claims a property the change had altered.
I also confirmed the new tests bind. Neutering the npmUserConfig/netrc append fails TestCredentialDenyReadPathsForEnvironmentHonorsConfigOverrides, so the coverage is asserted rather than incidental.
The remaining point: Zero's own credential store is beside the file you denied, not in it.
internal/sandbox/profile.go denies ~/.config/zero/config.json. That is the one file in that directory which deliberately does not hold secrets. internal/credstore/credstore.go:1 opens by stating the package "persists provider API keys outside config.json so secrets are not written to disk in cleartext", and internal/config/credentials.go:32 resolves the store to filepath.Dir(configPath) — the same directory. So alongside the denied file sit:
credentials.encandcredentials.enc.secret(credstore.go:27,:108-109) — the encrypted provider keys and, in the same directory, the key material that decrypts themcredentials.json(credstore.go:28) — the plaintext backendoauth-tokens.json(internal/oauth/store.go:158)
None of the four is denied. Denying the file rather than the directory protects the one location the codebase moved secrets out of, and leaves both halves of the encrypted store readable together, which defeats the encryption rather than merely bypassing one file.
Changing the entry from the file to the directory looks like a one-line change, and I would not hold the pull request for anything else.
Three further gaps, non-blocking, listed so the enumeration can be judged as a whole. ~/.ssh and ~/.gnupg are not covered. Neither is ~/.git-credentials or ~/.config/git/credentials — worth noting because the comment this change rewrote previously named the XDG credential store under ~/.config/git as the specific reason for granting only the git config files. That reasoning is still sound; it simply no longer appears anywhere, and the file it was protecting against is not on the list.
The broader shape, offered as a question rather than a request. This is a deny-list on a security boundary, and the entries are being added a few at a time — cloud CLIs first, then tool configs, then a custom gcloud location. This repository has been through that cycle before on command classification, where three correct incremental patches each left the class open. I do not think it blocks this change, but it is worth deciding whether the Linux read-all posture with credential deny rules is the shape you want long term, or whether the resolution root should be narrowed instead. #808 approaches the same boundary from the other side by making credentials unreachable by construction, and the two would benefit from being sequenced deliberately.
Verification. On macOS (darwin/arm64), go build ./..., go vet ./... and gofmt -l are clean, and ./internal/sandbox/... and ./internal/agent/... both pass.
Limitations. macOS only. The deny-list is skipped on Windows by design and is a no-op on macOS, where reads are allow-listed, so everything above concerns Linux behaviour that I reasoned about from the code rather than executed on a Linux host.
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed on 538b31e. Both of my findings are resolved, so I am not holding this on my own account. I am not approving either, because I went and checked gnanam's remaining point and it is real.
My blocker is fixed properly. markLikelySandboxDenial is gone rather than softened, and there is no keyword scan of stdout or stderr left in internal/tools at all. The only producer now is markStructuredSandboxDenial, fed from the adapter's own report at three call sites. That is exactly the typed-facts shape I asked for, so the unsandboxed-retry predicate can no longer be reached by content a command prints.
CARGO_HOME is back, at internal/sandbox/runtime_state.go:211, so cargo lands in the managed cache with its siblings.
On gnanam's point about the credential store, which I would treat as blocking.
I drove credentialDenyReadPathsForEnvironment against a home with the whole store materialised, since the builder filters to paths that exist and a synthetic path returns nothing:
denied=true ~/.config/zero/config.json
denied=false ~/.config/zero/credentials.enc
denied=false ~/.config/zero/credentials.enc.secret
denied=false ~/.config/zero/credentials.json
denied=false ~/.config/zero/oauth-tokens.json
total deny entries: 1
Confirmed, and I would put it slightly more strongly than "one file was missed". credentials.enc and credentials.enc.secret are both readable, in the same directory, so the ciphertext and the key that decrypts it are available together. That is not a bypass of one entry; it makes the encryption itself not worth anything on this path. config.json is the single file in that directory the codebase deliberately moved secrets out of, and it is the only one denied.
The same probe puts ~/.ssh/id_rsa and ~/.git-credentials at denied=false, so gnanam's non-blocking list holds too.
Changing that entry from the file to the directory is the one-line fix, and I would want it before this merges rather than as a follow-up, because the failure mode is silent: nothing about the current behaviour looks wrong from the outside.
On the enumeration question gnanam raised, since it is partly aimed at #808. I agree it is worth deciding deliberately rather than drifting. My read is that a deny-list on a read-all posture will keep needing entries as long as the resolution root is the real home, and this change is what makes that root the real home. So the incremental cycle is not an accident of this change, it is a consequence of it. That is an argument for sequencing, not for blocking: #808 removes the need for the list on Windows by giving the sandbox its own identity, and it is deliberately inert by default, so the two do not collide. Ordering is kevin's call.
Verification: on Windows, so the deny-list is a no-op for me at runtime and I exercised the builder directly rather than end to end. go build, go vet and gofmt clean; internal/sandbox passes.
There was a problem hiding this comment.
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)
204-207: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve non-blank path values instead of trimming them.
These values come from the caller’s environment and may legally contain spaces. Trimming changes the path before
os.Stat, so the actual credential store can be omitted fromDenyReadand remain readable in the sandbox. Only trim to detect an empty value; retain the original path for resolution and add a regression test covering a path with spaces.Also applies to: 217-227, 238-243, 245-261
🤖 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 204 - 207, Update credentialDenyReadPathsForEnvironment to use trimmed Home and ConfigHome only for blank-value checks, while preserving the original non-blank path strings for resolution and os.Stat/DenyRead generation. Apply this consistently across the related candidate and path-resolution logic, and add a regression test verifying credential paths containing spaces are retained and denied.
🤖 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.
Outside diff comments:
In `@internal/sandbox/profile.go`:
- Around line 204-207: Update credentialDenyReadPathsForEnvironment to use
trimmed Home and ConfigHome only for blank-value checks, while preserving the
original non-blank path strings for resolution and os.Stat/DenyRead generation.
Apply this consistently across the related candidate and path-resolution logic,
and add a regression test verifying credential paths containing spaces are
retained and denied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f488b57e-d264-4dc6-9edc-66ac639aabca
📒 Files selected for processing (2)
internal/sandbox/manager_test.gointernal/sandbox/profile.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/manager_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed at 9fc61fc. The blocking finding is fixed. Approving.
I re-ran the probe that found it rather than reading the one-line diff, with the whole store materialised, since the builder filters to paths that exist:
denied=true ~/.config/zero/config.json
denied=true ~/.config/zero/credentials.enc
denied=true ~/.config/zero/credentials.enc.secret
denied=true ~/.config/zero/credentials.json
denied=true ~/.config/zero/oauth-tokens.json
denied=true ~/.config/zero/providers/anthropic/token.json
The last row is one I added this time: the deny covers nested subtrees, not just the directory's immediate children, so a store that grows a subdirectory later is covered by construction rather than by remembering to add it. That is the property that makes this the right shape of fix rather than a bigger enumeration.
It also picks up files nobody listed. mcp-oauth-tokens.json and mcp-permissions.json live in that directory too and are now covered without anyone having had to notice them, which is precisely the argument for denying the directory.
I checked the fix binds rather than trusting the tests exist: reverting the entry to the file form fails TestCredentialDenyReadPathsIn and TestCredentialDenyReadPathsForEnvironmentHonorsConfigOverrides.
I also checked the obvious risk of widening a deny to a whole directory, which is breaking something that legitimately reads from it. hooks.json, mcp-permissions.json and the dictation model root are all under there, but they are read by Zero's own process rather than by the sandboxed child, so denying the child is correct and I could not find a path that regresses.
Two things left open, both non-blocking and both gnanam's from the previous round rather than new:
~/.ssh, ~/.gnupg, ~/.git-credentials and ~/.config/git/credentials are still uncovered. I confirmed that at this head. Worth a follow-up rather than another round here.
The test proves the directory is in the deny list, but not the reason it matters. Asserting that credentials.enc specifically comes back denied would document the intent and survive a future refactor that changes how entries are built. Small, and I would not hold the change for it.
Verification: on Windows, so the deny-list is a no-op at runtime for me and I drove the builder directly rather than end to end. go vet clean, internal/sandbox passes.
Merge is kevin's call per the program gate. From my side this is done, and gnanam's outstanding request is the one on the previous commit rather than this one.
|
@gnanam1990 when you get a chance, this one needs your eyes to move: your changes-requested is on 538b31e and the credential-store finding was fixed two commits later in bb103b2. You are the only thing holding it now, and I do not think that reflects your actual position on the current head. I verified the fix independently before approving, with the store materialised, since the builder filters to paths that exist: The nested path is one I added: the deny covers subtrees, so the ciphertext and its key are no longer readable together and a store that grows a subdirectory later is covered without anyone having to notice. It also picks up Your two non-blocking items are still open and I confirmed that at this head: Good catch on this one. Denying the file that deliberately holds no secrets while both halves of the encrypted store sat beside it readable is not something I would have spotted from the diff. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at 9fc61fc43300, base 5d1869e, re-confirmed against the live head before posting.
My blocker is resolved. Changing the entry from $XDG_CONFIG_HOME/zero/config.json to the directory is the one-line fix I asked for, and I verified it rather than reading the commit message. Driving credentialDenyReadPathsForEnvironment against a synthetic home with the whole credential set materialised:
deny entries: /.aws /.npmrc /.netrc /.config/zero /.kube/config
denied .config/zero/config.json
denied .config/zero/credentials.enc
denied .config/zero/credentials.enc.secret
denied .config/zero/credentials.json
denied .config/zero/oauth-tokens.json
All five, where previously only the first was covered. The pair that mattered most — credentials.enc and the .secret beside it — are no longer readable together.
Thank you to @Vasanthdev2004 for independently probing this and reaching the same result before the fix landed; his framing that it "makes the encryption itself not worth anything on this path" is the right way to describe the severity, and sharper than mine.
The tests bind. Reverting the entry from the directory back to the file fails both TestCredentialDenyReadPathsIn and TestCredentialDenyReadPathsForEnvironmentHonorsConfigOverrides.
One consequence worth recording deliberately, because it is wider than what I asked for. $XDG_CONFIG_HOME/zero/ is not only the credential store. It also holds plugins/ (internal/plugins/plugins.go:194), commands/ (internal/usercommands/usercommands.go:51), hooks.json (internal/hooks/hooks.go:203), trust.json (internal/workspacetrust/trust.go:38), modelsdev.json (internal/modelregistry/modelsdev.go:151), background/ and the per-user context file (internal/agent/system_prompt.go:393). Denying the directory denies all of it to sandboxed commands, so a zero invoked from inside a sandboxed shell loses its user commands, plugins, hooks, cached model catalog and workspace-trust record.
I think that is the right trade and I am not asking you to narrow it. Enumerating the credential files instead would reintroduce exactly the defect this fixes: a credential file added later would default to readable, which is how credentials.enc came to be exposed in the first place. But it is a broader behaviour change than "deny the credential store", the failure mode is a nested zero behaving subtly differently rather than erroring, and it belongs in the description or a release note rather than being discovered later.
The non-blocking gaps from my previous review remain open, and @Vasanthdev2004's probe confirms them independently: ~/.ssh, ~/.gnupg, ~/.git-credentials and ~/.config/git/credentials are all still readable. The last two are worth a follow-up specifically because the comment this change rewrote previously named the XDG git credential store as the reason for granting only the git config files — that reasoning was sound and no longer appears anywhere.
On the deny-list question, I take his point and it is better than mine. I framed the incremental cycle as a risk to decide about. He is right that it is a consequence: a deny-list over a read-all posture will keep needing entries for as long as the resolution root is the real home, and this change is what makes it the real home. That reframes it from "will this drift?" to "this will require maintenance by construction, so decide who owns it" — which is a sequencing question against #808's identity model rather than a reason to hold this. Agreed that ordering is kevin's call.
Verification. On macOS (darwin/arm64): go build ./..., go vet ./..., gofmt -l and git diff --check clean; ./internal/sandbox/... and ./internal/agent/... passing. The branch is 11 commits behind main, so I merged origin/main and confirmed the merged tree builds and internal/sandbox still passes, rather than trusting the absence of a conflict — a clean textual merge in this repository has produced a broken tree before. A rebase before merge would still be worth doing so CI reflects the tree that lands.
Limitations. macOS only, and this is the part a reader should weigh: credentialDenyReadPaths returns early on Windows and is a no-op on macOS, where seatbelt reads are allow-listed. Everything above concerns Linux runtime behaviour that I exercised only through the profile builder, never against a live Linux sandbox. @Vasanthdev2004 reports the same constraint from the other direction, on Windows. Nobody on this thread has run the enforcement path this change is about.
Merge is kevin's call per the program gate.
|
@gnanam1990 the One thing that came out of writing it up: it is not a one-line addition. Denying The other thing worth recording there: I also closed #675, since every file it named is covered by this PR, verified against merged |
3 participants
Summary
Behavior
Sandboxed commands now honor user configuration such as Git global ignore rules. Writes outside approved roots remain read-only by default. When a native sandbox blocks a command, Zero surfaces the denial reason and retries only after explicit user approval.
Verification
make fmt-checkgo vet ./...go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegovulncheck ./...— no vulnerabilities foundgit diff HEAD --checkThe pinned advisory linter reports only existing repository-wide findings outside the changed files.
Summary by CodeRabbit