feat(sandbox): disable the sandbox via config (#687)#746
Conversation
Add a sandbox.enabled field (`"sandbox": {"enabled": false}`). ModeDisabled
already short-circuits the engine but had no config surface.
- SandboxConfig.Enabled *bool (pointer distinguishes an explicit false from an
omitted key).
- mergeConfig honors it (global config / CLI); applyConfiguredSandboxPolicy maps
enabled:false to ModeDisabled.
- mergeProjectConfig intentionally does NOT merge it: a cloned repo must not be
able to disable the sandbox that constrains it — same posture as
AdditionalWriteRoots and the Network tighten-only rule.
WalkthroughSandbox configuration now supports an explicit ChangesSandbox disablement
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigFile
participant Resolve
participant CLI
participant SandboxPolicy
participant Command
ConfigFile->>Resolve: provide sandbox.enabled
Resolve->>CLI: return trusted resolved configuration
CLI->>SandboxPolicy: disable policy when enabled is false
SandboxPolicy-->>Command: allow unwrapped or sandboxed execution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Please merge sandbox.enabled through the CLI override path before this lands.
mergeConfig now copies Sandbox.Enabled, but ResolveOptions.Overrides is applied later by applyOverrides, which does not copy overrides.Sandbox.Enabled. As a result, an explicit CLI/programmatic override such as Overrides{Sandbox: SandboxConfig{Enabled: &disabled}} is ignored, contradicting the PR description and the new comment that global config / CLI can disable the sandbox.
Please update applyOverrides to preserve the tri-state pointer and add a resolver regression test showing that an explicit false override disables the sandbox (and, ideally, that explicit true can override a lower-precedence false).
…eview)
applyOverrides copied BlockUnixSockets/MonitorDenials but not the new Enabled
tri-state pointer, so a CLI/programmatic Overrides{Sandbox:{Enabled:&false}} was
dropped — contradicting the 'global config / CLI can disable' contract. Preserve
the pointer in applyOverrides (runs after all config sources, so an explicit
override wins) and add a resolver regression test: an explicit false override
disables, and an explicit true override wins over a lower-precedence config false.
|
Good catch — fixed in 2a68d9c. |
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/config/resolver.go (1)
219-221: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftBlock provider-command output from toggling sandbox state.
SandboxConfig.Enabledis documented as user config/CLI only, butResolvestill appliesLoadProviderCommandthroughmergeConfig, so a provider command can clear the sandbox too. Keepenabledout of that merge path or gate it behind an explicit trusted-source flag, and add a regression test.🤖 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/config/resolver.go` around lines 219 - 221, Prevent provider-command data from changing sandbox state during Resolve: update mergeConfig and the surrounding SandboxConfig handling so SandboxConfig.Enabled is excluded from provider-command merges or applied only when an explicit trusted-source flag is set, while preserving user config/CLI behavior. Add a regression test confirming LoadProviderCommand cannot enable or clear sandbox state.
🤖 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/config/resolver.go`:
- Around line 219-221: Prevent provider-command data from changing sandbox state
during Resolve: update mergeConfig and the surrounding SandboxConfig handling so
SandboxConfig.Enabled is excluded from provider-command merges or applied only
when an explicit trusted-source flag is set, while preserving user config/CLI
behavior. Add a regression test confirming LoadProviderCommand cannot enable or
clear sandbox state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2e43d08-36e0-4b48-8bf6-19ea4946a019
📒 Files selected for processing (2)
internal/config/resolver.gointernal/config/resolver_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving on the current head (2a68d9c4). anand's requested change is addressed here: applyOverrides now threads Sandbox.Enabled, and TestResolveSandboxEnabledCLIOverride covers both an explicit false override disabling the sandbox and an explicit true winning over a lower-precedence config false. @anandh8x this postdates your review, worth a re-look.
I verified the security posture first-hand (tests pass locally):
- Off by default:
Enabled *boolnil (omitted) keeps enforcement; only an explicitfalsedisables. - A cloned repo CANNOT disable the sandbox that constrains it:
mergeProjectConfigdeliberately skipsSandbox.Enabled, andTestResolveSandboxDisableIgnoredFromProjectConfiglocks that in. That is the property that matters most for a disable knob, and it is correct. - Precedence is right: global config and CLI can disable, project config cannot, and an explicit true beats a config false. The
applyConfiguredSandboxPolicypolicy test and all three resolver tests pass.
One non-blocking thought: a disabled sandbox surfaces as "sandbox disabled" in status, which is good, but a more prominent session-start notice (so someone who set enabled:false globally and forgot does not run unsandboxed unaware) might be worth a follow-up. Not blocking.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Request changes: provider-command output still reaches mergeConfig after LoadProviderCommand, and mergeConfig now copies Sandbox.Enabled. A provider command that returns a valid provider plus "sandbox": {"enabled": false} can therefore disable the sandbox, despite this setting being documented as global-config/CLI only. Keep Enabled out of the provider-command merge path (or gate it on an explicit trusted source) and add a regression test that provider-command output cannot enable or disable the sandbox.
review) mergeConfig now copies Sandbox.Enabled, and it is shared by two callers: the global-config merge (trusted) and the provider-command merge (not). A provider command is an arbitrary executable named in config, and LoadProviderCommand parses its stdout into a full FileConfig -- so one returning a valid provider plus {"sandbox":{"enabled":false}} could disable the sandbox meant to constrain it, despite the setting being documented as global-config/CLI only. Confirmed reproducible in both directions before the fix. Clear Sandbox.Enabled from the provider-command config before merging, mirroring the carve-out mergeProjectConfig already applies for the same reason. Done at the call site rather than inside mergeConfig because that helper must keep honouring the setting for the trusted global-config merge. Adds TestResolveSandboxEnabledIgnoredFromProviderCommand covering both directions (cannot disable, cannot enable) and asserting the provider itself is still applied, so the guard cannot be satisfied by dropping the command wholesale.
76ef636
|
@coderabbitai review Pushed A provider command returning the full-config shape ( Fix clears
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 76ef636
git diff 76ef636^ 76ef636 -- internal/config
printf '\n=== Relevant declarations and functions ===\n'
ast-grep outline internal/config --items all --type function,struct --match 'Resolve|mergeConfig|mergeProjectConfig|parseProviderCommandJSON|Provider|Sandbox|applyOverrides'
printf '\n=== Resolver references with context ===\n'
rg -n -C 5 'parseProviderCommandJSON|mergeConfig\(|mergeProjectConfig\(|Sandbox\.Enabled|provider command|ProviderCommand' internal/config
printf '\n=== Changed test sections ===\n'
git diff 76ef636^ 76ef636 --unified=60 -- internal/config/resolver_test.goLength of output: 51743
Clearing ✏️ Learnings added
✅ Action performedReview finished.
|
|
@anandh8x — ready for another look. Your review was on What you reported: Confirmed reproducible before the fix, in both directions. One detail worth having: only the full-config shape reaches it — the shorthand single-provider form is already safe, because What changed:
|
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Approved on the latest head. Provider-command output now has sandbox.enabled cleared before the shared config merge, while global config and CLI overrides retain the intended behavior. Regression coverage and focused tests pass.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-approving on 76ef636. My earlier approve was auto-dismissed by this push, so I re-checked the whole thing on the current head rather than standing on the old verdict.
The new commit closes a real hole. mergeConfig copies Sandbox.Enabled, and it is shared by two callers: the trusted global-config merge and the provider-command merge. Since LoadProviderCommand runs an arbitrary executable and parses its full stdout into a FileConfig, a command returning a valid provider plus {"sandbox":{"enabled":false}} could have turned off the sandbox meant to constrain it, despite the setting being documented as global-config/CLI only. Clearing commandConfig.Sandbox.Enabled = nil at the call site (not inside the shared helper, which the global-config merge still needs) is the right fix, and it mirrors the existing mergeProjectConfig carve-out.
Verified on 76ef636:
mergeConfigonly assignsEnabledwhen the source is non-nil (resolver.go:229), so nil-ing it drops just the toggle while the provider itself still merges.- Full posture is green: global config CAN disable, project config CANNOT, provider command CANNOT (both directions), CLI override CAN. Ran the four
TestResolveSandbox*cases plus the CLI policy test locally, all pass. - The new test also asserts the provider is still applied, so the guard cannot be satisfied by silently dropping the whole command config. Good touch.
One non-blocking heads-up, separate from this PR: the sibling sandbox fields (Network, AdditionalWriteRoots) still flow through the provider-command merge. That only matters if you later decide a provider command should not be able to widen write roots or open network either. Out of scope here; the enabled toggle was the documented contract, and that is what this closes.
LGTM.
3 participants
Summary
Closes #687.
Adds a
sandbox.enabledfield to config, so"sandbox": {"enabled": false}turns the sandbox off. The engine already had a fully-wiredModeDisabled(it short-circuits evaluation and returns unwrapped command plans), but no config surface reached it.Changes
SandboxConfig.Enabled *bool— a pointer so an explicitfalse(disable) is distinct from an omitted key (keep the default: enabled).true/omitted keep enforcement.mergeConfighonors it, so global (user) config and CLI can turn the sandbox off;applyConfiguredSandboxPolicymapsenabled:falsetosandbox.ModeDisabled.mergeProjectConfigintentionally does NOT merge it — a cloned repo's.zero/config.jsonmust not be able to disable the sandbox that constrains it. Same posture the file already takes forAdditionalWriteRoots(never from project) andNetwork(project may only tighten todeny).Scope
internal/config+internal/clionly. Additive field, no behavior change when the key is absent. No performance change expected.Testing
TestResolveSandboxEnabledUserConfigDisables— global configenabled:falseresolves to a false pointer.TestResolveSandboxDisableIgnoredFromProjectConfig— security: a project configenabled:falseis ignored (resolves to nil), so a repo cannot disable the sandbox.TestApplyConfiguredSandboxPolicyEnabledFalseDisables—enabled:false→ModeDisabled; omitted/truekeep enforcing.make fmt-check,go vet ./...,go test -race ./internal/config/... ./internal/cli/...,go run ./cmd/zero-release build,go run ./cmd/zero-release smoke,govulncheck,git diff HEAD --checkall clean.zero sandbox check, macos-seatbelt backend): a global configenabled:falsereportspolicy: mode=disabled/ "sandbox disabled"; a project.zero/config.jsonenabled:falsestill reportspolicy: mode=enforce/ "sandbox is active" — the repo's attempt to disable is ignored.Summary by CodeRabbit
sandbox.enabled: false).