feat(sandbox): harden internal/sandbox — re-entrancy guard, SOCKS5 egress, sandbox-aware search, fine-grained path lists#199
Conversation
All four are off by default and fail closed; existing behavior is unchanged
when the new policy fields are empty / ZERO_SANDBOXED is unset.
1. Re-entrancy guard — ZERO_SANDBOXED=1 marks every wrapped command; a nested
BuildCommandPlan returns a pass-through plan (no double bwrap/sandbox-exec,
no second egress proxy). New: EnvSandboxed, IsAlreadySandboxed().
2. SOCKS5 egress — the scoped-egress proxy also serves SOCKS5 CONNECT on a
second loopback port through the SAME authorize()/domainAllowed() gate.
ProxyEnvWithSocks points ALL_PROXY at socks5://<addr>; HTTP(S)_PROXY stay on
the HTTP listener; the sandbox-exec profile allows both proxy ports.
Bubblewrap scoped still collapses to deny.
3. Sandbox-aware search — ReadExclusionGlobs(policy, scope) returns ripgrep
--glob '!<pat>' for DenyRead subtrees; grep/glob skip read-denied subtrees
at walk time via Engine.ReadPathExcluded/ReadDirExcluded when sandboxed.
4. Fine-grained path lists — Policy.AllowRead/DenyRead/AllowWrite/DenyWrite.
Read = deny-then-allow (more-specific AllowRead re-includes); Write =
DenyWrite > workspace/scope > AllowWrite > deny. AllowWrite is reflected in
the OS write binds; DenyWrite is emitted as sandbox-exec deny rules. Each
entry is home-expanded, made absolute, and symlink-resolved.
Tests: internal/sandbox/{reentrancy,egress_socks,pathlists}_test.go and
internal/tools/read_exclusions_test.go.
Gates: gofmt, go vet, go build, go test (incl. -race), staticcheck (no new
findings), govulncheck (0), deadcode -test=false (no new unreachable funcs).
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)
WalkthroughAdds SOCKS5 front-end to scoped egress proxy with same auth gate as HTTP. Introduces path policy with AllowRead/DenyRead/AllowWrite/DenyWrite and full precedence logic wired through Engine.Evaluate. Adds re-entrancy guard to BuildCommandPlan and propagates SOCKS port and write-root policy into sandbox-exec profile. Integrates DenyRead exclusions into grep and glob tools via RunWithSandbox. Adds optional EnforceToolNetwork gate for first-party tool network access. ChangesSandbox SOCKS5 egress, path policy, re-entrancy, and tool exclusions
Sequence Diagram(s)sequenceDiagram
participant Client
participant serveSocks as serveSocks loop
participant socksNegotiate
participant authGate as auth gate
participant Upstream
Client->>serveSocks: TCP to socksListener
serveSocks->>socksNegotiate: negotiate(conn)
socksNegotiate->>Client: select no-auth method
Client->>socksNegotiate: CONNECT host:port
socksNegotiate-->>serveSocks: (host, port)
alt authorization denied
serveSocks->>authGate: proxy.allowed(host, port)?
authGate-->>serveSocks: false
serveSocks->>Client: SOCKS REP 0x02 (not allowed)
else authorization allowed
authGate-->>serveSocks: true
serveSocks->>Upstream: dial(host:port)
serveSocks->>Client: SOCKS REP 0x00 (success)
note over serveSocks: bidirectional relay
end
sequenceDiagram
participant BuildCommandPlan
participant IsAlreadySandboxed
participant sandboxExecCommandPlan
participant sandboxExecProfile
participant ProxyEnvWithSocks
BuildCommandPlan->>IsAlreadySandboxed: check environment markers
alt process already sandboxed
IsAlreadySandboxed-->>BuildCommandPlan: true
BuildCommandPlan-->>BuildCommandPlan: return pass-through plan
else process not sandboxed
IsAlreadySandboxed-->>BuildCommandPlan: false
BuildCommandPlan->>sandboxExecCommandPlan: build wrapped plan
sandboxExecCommandPlan->>sandboxExecProfile: (proxyPort, socksPort, writeRoots, policy)
sandboxExecProfile-->>sandboxExecCommandPlan: seatbelt profile (allow both ports, DenyWrite rules)
sandboxExecCommandPlan->>ProxyEnvWithSocks: (httpAddr, socksAddr)
ProxyEnvWithSocks-->>sandboxExecCommandPlan: HTTP_PROXY=http, ALL_PROXY=socks5
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 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/egress_socks.go`:
- Around line 86-92: The SOCKS5 method negotiation violates the protocol by
always selecting socksNoAuth without checking if the client actually offered it.
After reading the greeting and the list of methods offered by the client (the
greeting[1] bytes), check if socksNoAuth (method 0x00) is present in the methods
list. If it is, send back the selection response with socksNoAuth; if not, send
back 0xFF ("no acceptable methods") instead, as required by the SOCKS5 protocol
specification.
In `@internal/sandbox/pathlists.go`:
- Around line 113-119: The readDenied and hasNestedAllowRead functions
repeatedly call resolvePolicyPaths on each invocation, causing expensive
operations like Abs and EvalSymlinks to run multiple times for the same policy
entries. Cache the resolved DenyRead and AllowRead roots once at engine
construction or per tool run, then pass these cached roots to both readDenied
and hasNestedAllowRead instead of resolving them each time. This eliminates
redundant path resolution operations in the walk predicates.
In `@internal/sandbox/runner.go`:
- Around line 187-189: The IsAlreadySandboxed() check at line 187 in the
directCommandPlan call relies only on the ZERO_SANDBOXED environment variable as
proof of nesting, which is insufficient because the variable can be inherited or
manually set by users in non-nested environments, allowing commands to execute
unsandboxed and bypass scoped-egress protections. Replace the single ambient
environment variable check with a stronger re-entrancy detection mechanism that
has provenance beyond just an inherited or user-settable flag, such as verifying
a unique identifier or state that can only be present in an actual nested
execution context created by the sandbox wrapper itself.
🪄 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: 0bc0b613-b8ec-4481-8c69-49547ba8f18a
📒 Files selected for processing (14)
internal/sandbox/egress.gointernal/sandbox/egress_socks.gointernal/sandbox/egress_socks_test.gointernal/sandbox/engine.gointernal/sandbox/pathlists.gointernal/sandbox/pathlists_test.gointernal/sandbox/reentrancy_test.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/sandbox/types.gointernal/tools/glob.gointernal/tools/grep.gointernal/tools/read_exclusions.gointernal/tools/read_exclusions_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I found 3 issues before this should merge:
-
Windows CI is failing in
internal/sandbox/pathlists_test.go(TestSandboxExecProfileEmitsDenyWriteRule). The profile escapes backslashes throughsandboxProfileString, but the test expects the raw Windows path. Please fix the assertion or emitted expected path so Windows smoke is green. -
internal/sandbox/runner.gore-entrancy guard trusts onlyZERO_SANDBOXED=1. Since that env var can be user-set before launching Zero, it can force command plans into direct/pass-through mode and bypass wrapping/scoped egress. Please add stronger provenance than a single ambient env var. -
internal/sandbox/egress_socks.goalways selects SOCKS no-auth even if the client did not offer method0x00. It should reply0xffwhen no acceptable method is offered, and have a regression test.
I ran go test ./internal/sandbox ./internal/tools -count=1; internal/sandbox fails on the Windows-path DenyWrite profile test, internal/tools passes.
…usion roots
- egress_socks: respect SOCKS5 method negotiation (RFC 1928). The proxy now
inspects the client's offered methods and replies 0xFF ("no acceptable
methods") when no-auth was not offered, instead of forcing no-auth.
- pathlists/engine: resolve DenyRead/AllowRead roots ONCE per search walk via a
new ReadExclusions matcher (Engine.ReadExclusions), replacing the per-path
ReadPathExcluded/ReadDirExcluded that re-ran Abs/EvalSymlinks for every visited
path during a grep/glob walk.
Re-entrancy guard left as-is by design: ZERO_SANDBOXED is an internal marker set
only on commands zero wraps; cross-process re-entrancy detection legitimately
relies on the inherited env, and a manually-exported flag is a self-opt-out, not
an external bypass.
Tests: SOCKS no-acceptable-methods rejection; nested-AllowRead descent via the
matcher. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new
findings), govulncheck (0), deadcode (no new unreachable funcs).
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/pathlists.go`:
- Around line 182-186: In the DirExcluded function, the abs path is constructed
but not symlink-resolved before being passed to hasNestedAllowReadResolved,
while allowRoots are already resolved. This mismatch causes prefix comparison in
pathWithinRoot to fail for symlinked paths. After constructing abs by joining
with rx.workspaceRoot if needed, resolve it using NormalizePrefixForRoot (or the
equivalent symlink resolution method used for allowRoots) to ensure consistent
path comparison before passing it to hasNestedAllowReadResolved.
🪄 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: 43817e7d-3029-4b1d-b49a-744c74ec8aaf
📒 Files selected for processing (6)
internal/sandbox/egress_socks.gointernal/sandbox/egress_socks_test.gointernal/sandbox/engine.gointernal/sandbox/pathlists.gointernal/sandbox/pathlists_test.gointernal/tools/read_exclusions.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sandbox/egress_socks_test.go
- internal/sandbox/egress_socks.go
- internal/sandbox/pathlists_test.go
- pathlists_test: TestSandboxExecProfileEmitsDenyWriteRule now builds the expected deny clause via sandboxProfileString(secret), so it matches the profile's own escaping (Windows doubles backslashes). Fixes the windows-latest smoke failure; macOS/Linux unaffected (slash paths, no-op escaping). - pathlists: ReadExclusions.DirExcluded now EvalSymlinks-resolves the candidate path before the nested-AllowRead prefix check, matching the resolution applied to allowRoots — otherwise a denied dir reached THROUGH a symlink fails to match a nested AllowRead root and is wrongly skipped, dropping a re-included subtree. Added TestDirExcludedResolvesSymlinkPrefix (skips where symlinks are unavailable). Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new findings), govulncheck (0), deadcode (no new unreachable funcs).
…rker Addresses review feedback that the re-entrancy guard trusted a single ambient env var. IsAlreadySandboxed now requires BOTH correlated markers that sandboxEnvironment sets together on every wrapped command — ZERO_SANDBOXED=1 AND a non-empty ZERO_SANDBOX_BACKEND (new EnvSandboxBackend const) — so a lone stray/hand-exported ZERO_SANDBOXED=1 no longer forces an unsandboxed pass-through. Pass-through (direct) plans set neither marker, so genuine nesting still detected and double-wrapping still avoided. Tests updated: TestIsAlreadySandboxed now asserts each marker alone is insufficient and both together are required; the re-entrancy and wrapping plan tests set/clear both markers explicitly. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new findings), govulncheck (0), deadcode (no new unreachable funcs).
|
@Vasanthdev2004 thanks for the review — all three addressed:
All gates green locally and in CI (gofmt, vet, build, test incl. -race, staticcheck, govulncheck, deadcode). Could you take another look when you have a moment? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/reentrancy_test.go (1)
30-39: ⚡ Quick winConsider also asserting
EnvSandboxBackendis set.This test only verifies
EnvSandboxed=1is present. SinceIsAlreadySandboxed()now requires both markers, a regression that removesEnvSandboxBackendfromsandboxEnvironmentwould pass this test but break re-entrancy detection.🔧 Proposed fix
func TestSandboxEnvironmentMarksSandboxed(t *testing.T) { env := sandboxEnvironment(DefaultPolicy(), BackendBubblewrap, "/home/agent") want := EnvSandboxed + "=1" + wantBackend := EnvSandboxBackend + "=" + string(BackendBubblewrap) + foundSandboxed, foundBackend := false, false for _, entry := range env { if entry == want { - return + foundSandboxed = true + } + if entry == wantBackend { + foundBackend = true } } - t.Fatalf("sandboxEnvironment must set %s so a wrapped command is detectable; got %#v", want, env) + if !foundSandboxed { + t.Fatalf("sandboxEnvironment must set %s; got %#v", want, env) + } + if !foundBackend { + t.Fatalf("sandboxEnvironment must set %s for dual-marker detection; got %#v", wantBackend, env) + } }🤖 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/reentrancy_test.go` around lines 30 - 39, The TestSandboxEnvironmentMarksSandboxed test only verifies that EnvSandboxed is set but does not check for EnvSandboxBackend, which is also required by IsAlreadySandboxed() for proper re-entrancy detection. Add an additional assertion after the loop that checks for the presence of EnvSandboxBackend in the env slice returned by sandboxEnvironment(), similar to the existing check for EnvSandboxed, to prevent regressions where EnvSandboxBackend is accidentally removed from the environment while the test still passes.
🤖 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/reentrancy_test.go`:
- Around line 30-39: The TestSandboxEnvironmentMarksSandboxed test only verifies
that EnvSandboxed is set but does not check for EnvSandboxBackend, which is also
required by IsAlreadySandboxed() for proper re-entrancy detection. Add an
additional assertion after the loop that checks for the presence of
EnvSandboxBackend in the env slice returned by sandboxEnvironment(), similar to
the existing check for EnvSandboxed, to prevent regressions where
EnvSandboxBackend is accidentally removed from the environment while the test
still passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf56244e-5846-4ead-82db-c67a6374adf3
📒 Files selected for processing (3)
internal/sandbox/reentrancy_test.gointernal/sandbox/runner.gointernal/sandbox/types.go
… default The sandbox was reported as too strict: web_search (and web_fetch) were blocked with "network access is disabled by the sandbox policy" because the default policy is network=deny and the in-process web tools were gated by the same NetworkHostAllowed check as the sandboxed shell's egress. That network policy exists to confine the sandboxed SHELL's egress, which these in-process tools don't use. New Policy.EnforceToolNetwork (off by default) makes NetworkHostAllowed exempt web_search/web_fetch unless an operator opts in. The sandboxed-shell egress decision (Evaluate / effectiveNetworkMode) is unchanged, and the tools keep their own SSRF/private-IP, port, redirect, and redaction safeguards. Set EnforceToolNetwork to hold the tools to the allow/scoped/deny policy as before. Tests: engine default-exempt + enforced-table cases; web_search default-allows and enforced-when-flag; web_fetch enforced cases set the flag. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new findings), govulncheck (0), deadcode (no new unreachable funcs).
|
Added Context: with the default Change: new Tests updated/added: engine default-exempt + enforced-table cases; |
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 (2)
internal/sandbox/engine.go (2)
73-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisabled policy should not keep filtering grep/glob paths.
ReadExclusions()andReadExclusionGlobs()ignorepolicy.Mode. WithModeDisabled,Evaluate()allows the request, but sandbox-aware grep/glob can still skipDenyReadsubtrees because they consume these helpers directly. That makes “policy disabled” still hide files.Suggested fix
func (engine *Engine) ReadExclusions() *ReadExclusions { - if engine == nil { + if engine == nil || engine.policy.Mode == ModeDisabled { return nil } return &ReadExclusions{ workspaceRoot: engine.workspaceRoot, denyRoots: resolvePolicyPaths(engine.policy.DenyRead), allowRoots: resolvePolicyPaths(engine.policy.AllowRead), } } @@ func (engine *Engine) ReadExclusionGlobs() []string { - if engine == nil { + if engine == nil || engine.policy.Mode == ModeDisabled { return nil } return ReadExclusionGlobs(engine.policy, engine.scope) }🤖 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/engine.go` around lines 73 - 92, The ReadExclusions() and ReadExclusionGlobs() methods do not check engine.policy.Mode and therefore return filtering rules even when the policy is disabled, causing files to still be hidden when they should all be accessible. Add a check in both ReadExclusions() and ReadExclusionGlobs() to return nil and an empty slice respectively when engine.policy.Mode is set to ModeDisabled, ensuring that disabled policies do not filter any paths.
261-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPath-list policy is accidentally disabled when
EnforceWorkspaceis false.This keeps
DenyRead/DenyWrite/AllowWriteout ofEvaluate()unless workspace enforcement is also enabled. That undercuts the new explicit path-policy knobs and diverges from the grep/glob path, which already consumesDenyReaddirectly.Suggested fix
- if policy.EnforceWorkspace && request.WorkspaceRoot != "" { + if request.WorkspaceRoot != "" && ( + policy.EnforceWorkspace || + len(policy.AllowRead) > 0 || + len(policy.DenyRead) > 0 || + len(policy.AllowWrite) > 0 || + len(policy.DenyWrite) > 0) { for _, requested := range requestPaths(request) { if violation := validatePathWithPolicy(scope, policy, request.SideEffect, request.WorkspaceRoot, requested); violation != nil { return deny(request, risk, violation.Code, violation.Path, violation.Reason, false) } } }🤖 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/engine.go` around lines 261 - 267, The path-list policy validation using validatePathWithPolicy is incorrectly gated behind the condition checking both policy.EnforceWorkspace and request.WorkspaceRoot, which disables DenyRead, DenyWrite, and AllowWrite policies when EnforceWorkspace is false. Remove or restructure the workspace enforcement condition so that path validation occurs independently and always checks the path policies against the requested paths, similar to how the grep/glob path handles DenyRead directly. The workspace root validation should remain as a separate workspace-specific check that does not block the general path policy evaluation.
🤖 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/engine.go`:
- Around line 73-92: The ReadExclusions() and ReadExclusionGlobs() methods do
not check engine.policy.Mode and therefore return filtering rules even when the
policy is disabled, causing files to still be hidden when they should all be
accessible. Add a check in both ReadExclusions() and ReadExclusionGlobs() to
return nil and an empty slice respectively when engine.policy.Mode is set to
ModeDisabled, ensuring that disabled policies do not filter any paths.
- Around line 261-267: The path-list policy validation using
validatePathWithPolicy is incorrectly gated behind the condition checking both
policy.EnforceWorkspace and request.WorkspaceRoot, which disables DenyRead,
DenyWrite, and AllowWrite policies when EnforceWorkspace is false. Remove or
restructure the workspace enforcement condition so that path validation occurs
independently and always checks the path policies against the requested paths,
similar to how the grep/glob path handles DenyRead directly. The workspace root
validation should remain as a separate workspace-specific check that does not
block the general path policy evaluation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b48ce849-a84a-4807-9415-8f24b42f5f51
📒 Files selected for processing (5)
internal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/types.gointernal/tools/web_fetch_test.gointernal/tools/web_search_test.go
…ent of EnforceWorkspace - engine: ReadExclusions() / ReadExclusionGlobs() now return inert results under ModeDisabled, so a disabled policy never filters grep/glob (parity with Evaluate, which already allows everything when disabled). - engine/pathlists: the fine-grained path lists (DenyRead/DenyWrite, with AllowRead/AllowWrite) now apply in Evaluate whenever the sandbox is enforcing, independent of EnforceWorkspace — matching the grep/glob path that already honors DenyRead directly. The workspace boundary (scope.validate) stays gated by EnforceWorkspace via a param threaded through validatePathWithPolicy / validateWritePath; DenyWrite wins regardless. - reentrancy_test: TestSandboxEnvironmentMarksSandboxed now asserts BOTH correlated markers (ZERO_SANDBOXED=1 and ZERO_SANDBOX_BACKEND) so dropping either would fail the test. Tests added: path lists enforced with EnforceWorkspace off; ReadExclusions inert under ModeDisabled. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new findings), govulncheck (0), deadcode (no new unreachable funcs).
|
Addressed the latest re-review in
All local gates green (gofmt, vet, build, test incl. -race, staticcheck no-new, govulncheck 0, deadcode no-new). |
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/engine.go`:
- Around line 270-273: The path validation loop starting at line 270 is guarded
by a condition checking if request.WorkspaceRoot is not empty, which causes the
entire validatePathWithPolicy validation to be skipped when WorkspaceRoot is
empty. This allows requests with absolute paths to bypass policy enforcement on
engines without a workspace root. Remove the if request.WorkspaceRoot != ""
condition so that the loop over requestPaths(request) executes unconditionally,
validating every requested path. The validatePathWithPolicy function call should
be invoked for all paths, and the enforcement logic within should handle both
cases appropriately: processing absolute paths normally when WorkspaceRoot is
empty, while failing validation for relative paths that cannot be anchored
without a workspace root.
🪄 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: ada038c3-1bf1-4da4-aba8-39746f4fa3af
📒 Files selected for processing (4)
internal/sandbox/engine.gointernal/sandbox/pathlists.gointernal/sandbox/pathlists_test.gointernal/sandbox/reentrancy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sandbox/reentrancy_test.go
- internal/sandbox/pathlists.go
- internal/sandbox/pathlists_test.go
Re-review follow-up: the Evaluate path-validation loop was still guarded by request.WorkspaceRoot != "", so an engine built without a workspace root skipped DenyRead/DenyWrite/AllowRead/AllowWrite even for absolute paths. The loop now runs unconditionally; the workspace boundary (scope.validate) stays gated on having a root (enforceWorkspace = EnforceWorkspace && WorkspaceRoot != ""), while the path lists match absolute paths regardless. An unanchorable relative path (no workspace root) fails closed when anything is configured to enforce, and is a no-op otherwise (unchanged from prior behavior). Test: TestEvaluatePathListsWithoutWorkspaceRoot. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new findings), govulncheck (0), deadcode (no new unreachable funcs).
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: approve on current head 995a231.
I re-reviewed this against the earlier blockers and against D's sandbox shape. The previous merge blockers look resolved:
- Windows path assertion now compares the sandbox-exec escaped path via
sandboxProfileString, so the DenyWrite profile test is platform-safe. - SOCKS5 negotiation now only selects no-auth when the client offered
0x00, otherwise replies0xff; there is a regression test for that path. - The re-entrancy guard no longer trusts only
ZERO_SANDBOXED=1; it now requires the correlated backend marker that Zero sets on wrapped commands. This is good enough for a re-entrancy marker. It is not a cryptographic boundary against a user intentionally launching Zero with spoofed env, but that is not the threat model here. - Path-list enforcement now runs even without a workspace root, with workspace-boundary enforcement gated separately. That fixes the absolute-path bypass concern while keeping
ModeDisabledinert.
Cross-checking with D: D is mostly a pre-check + prompt + persisted-grant service. It is intentionally product/UX-led: detect violations, show a clear prompt, and let the user allow/deny. Zero's Go runner is more protective here because it also wires command planning, OS backend profiles, read exclusions for search, and fail-closed scoped egress. That extra strictness is appropriate for Zero's Go-native terminal-agent backend as long as we keep the UX understandable.
One product note, not a blocker for this PR: the backend is now stronger than the visible permissions UI. We should still follow up with the /permissions manager/card work so users can see and edit these grants/policies without reading raw state.
Validation I ran locally:
git diff --check origin/main...HEADgo test ./internal/sandbox ./internal/tools -count=1go test ./...go vet ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
At review time, GitHub CI had most jobs green, with Windows smoke and CodeRabbit still pending. Merge after the remaining required checks finish green.
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/pathlists.go`:
- Around line 347-350: The policyHasPathLists function currently checks only
whether the raw policy slices are non-empty, but this disagrees with how the
rest of the file normalizes path lists by dropping blank, missing, and unusable
entries. Modify policyHasPathLists to check the effective normalized path lists
instead of the raw config slices. This ensures that when workspaceRoot is empty,
the fail-closed check in validatePathWithPolicy accurately reflects which path
restrictions are actually enforceable after normalization, preventing false
positives from typos or invalid paths that would be dropped during resolution.
🪄 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: 7b88c824-c463-4c65-b356-0dbbe10b8ea7
📒 Files selected for processing (3)
internal/sandbox/engine.gointernal/sandbox/pathlists.gointernal/sandbox/pathlists_test.go
|
Follow-up after the latest CodeRabbit pass: I agree with the new This is not a security regression and it does not change my D-vs-Zero read: Zero being more protective at the backend is still the right direction. But this specific helper should check the effective normalized path lists, not raw non-empty slices, otherwise a blank/missing typoed entry can make no-workspace-root evaluation fail closed even though no enforceable rule remains. So my verdict is: backend/product direction approved, but please take this tiny correctness cleanup before merge (or explicitly document that raw configured-but-unresolvable entries are intended to fail closed). |
…th lists policyHasPathLists now resolves the lists (resolvePolicyPaths/ resolveWriteRootPaths) instead of counting raw config, matching how the rest of the file normalizes them. A typo or non-existent entry (dropped during resolution) therefore no longer makes validatePathWithPolicy fail-close every relative request when there is no workspace root. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new findings), govulncheck (0), deadcode (no new unreachable funcs).
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: APPROVE.
The earlier confusing review/comment from me should be read as superseded by this one.
Current head f00e28d fixes the remaining correctness issue: policyHasPathLists now checks normalized/effective path-list entries instead of raw config slices, so blank/missing typo paths do not create a false fail-closed decision when there is no workspace root.
Cross-check vs D: D handles sandbox mostly as a pre-check + prompt + persisted-grant product layer. Zero is intentionally more protective in the Go backend: command wrapping, OS sandbox profiles, scoped egress proxying, read-denied search exclusions, and fail-closed behavior. That is the right direction for Zero as a terminal coding agent backend. The product/UI follow-up is still /permissions manager polish, but that should be a separate PR and should not block this backend hardening.
Local validation on latest head:
go test ./internal/sandbox ./internal/tools -count=1
Merge once the remaining required GitHub checks/CodeRabbit finish green.
2 participants
Summary
Four opt-in, fail-closed capabilities added to
internal/sandbox(Go only — no new deps,bin/zero.jsuntouched). Every addition is gated by aPolicyfield with a safe default, so an unconfigured policy behaves exactly as before.1. Re-entrancy guard
EnvSandboxed = "ZERO_SANDBOXED"is set on every wrapped command's env.BuildCommandPlanwhileZERO_SANDBOXED=1returns a pass-through plan — no doublebwrap/sandbox-execwrap, no second egress proxy.IsAlreadySandboxed() bool.2. SOCKS5 egress
127.0.0.1:0listener, routed through the sameauthorize()/domainAllowed()gate (deny-wins, empty-allowlist → deny, fail-closed).ProxyEnvWithSocks(httpAddr, socksAddr)pointsALL_PROXY/all_proxyatsocks5://<addr>whileHTTP_PROXY/HTTPS_PROXYstay on the HTTP listener;NO_PROXY=localhost,127.0.0.1preserved. Empty SOCKS addr falls back to the legacyProxyEnvoutput.sandbox-execprofile allows outbound to both proxy ports. Bubblewrap (isolated netns) scoped policy still collapses to deny — backend-awareness preserved.3. Sandbox-aware search
ReadExclusionGlobs(policy, scope) []stringreturns ripgrep--glob '!<pat>'exclusions forDenyReadsubtrees inside the workspace.grep/globtools become sandbox-aware (RunWithSandbox) and skip read-denied subtrees during their walk viaEngine.ReadPathExcluded/ReadDirExcluded(the predicate honors more-specificAllowReadre-inclusion). No sandbox / noDenyRead→ unchanged.4. Fine-grained allow/deny path lists
Policyfields:AllowRead,DenyRead,AllowWrite,DenyWrite []string.DenyReadsubtree is blocked unless a more-specificAllowReadre-includes it.DenyWritewins > workspace/scope-writable >AllowWrite> deny.AllowWriteroots are reflected in the OS write binds;DenyWriteis also emitted as explicitsandbox-execdeny rules (last-match-wins) so the precedence holds for shell commands.EvalSymlinks-resolved (non-existent entries dropped).Defaults / safety
No default weakened: all lists default empty,
ZERO_SANDBOXEDdefaults unset, SOCKS reuses the same allowlist, every gate fails closed.Tests
internal/sandbox/{reentrancy,egress_socks,pathlists}_test.go,internal/sandbox/runner_test.go(ProxyEnv variants), andinternal/tools/read_exclusions_test.go— covering the re-entrancy plan, SOCKS allow/deny + deny-wins, the read/write precedence matrix, glob generation, and grep/glob subtree exclusion.Gate results (local)
gofmt -l .→ emptygo vet ./...→ cleango build ./...→ cleango test ./...→ all pass (incl.-raceoninternal/sandbox+internal/tools)staticcheck ./...→ no new findings (none in changed files)govulncheck ./...→ 0 vulnerabilitiesdeadcode -test=false ./...→ no new unreachable funcsSummary by CodeRabbit
ALL_PROXY/all_proxyto SOCKS when available and restricts outbound proxy traffic to local loopback proxy ports.AllowRead/DenyReadandAllowWrite/DenyWrite, with deny precedence and nested allow re-inclusion.DenyReadduring traversal (skipping denied subtrees).EnforceToolNetworkto apply sandbox network rules to first-party tools.