feat(sandbox): unify command execution and enforcement#781
Conversation
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. |
WalkthroughThis PR introduces shared typed execution contracts, process management, sandbox reporting, runtime isolation, protected metadata enforcement, and runner wiring across tools, hooks, plugins, MCP servers, agent permissions, and transcript rendering. ChangesExecution and sandbox integration
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)
✨ 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: 7
🧹 Nitpick comments (6)
internal/background/process_windows.go (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale comment.
The comment mentions
taskkill /T, which was an implementation detail of the oldterminateProcesslogic. Since the code now delegates toexecution.TerminateProcessTree, the comment should be updated to reflect this abstraction.♻️ Proposed refactor
-// ConfigureChildProcessGroup is a no-op on Windows: terminateProcess uses -// `taskkill /T` to kill the whole process tree, so no launch-time process-group +// ConfigureChildProcessGroup is a no-op on Windows: terminateProcess uses +// execution.TerminateProcessTree to kill the whole process tree, so no launch-time process-group // setup is required (the POSIX build sets Setpgid here instead). func ConfigureChildProcessGroup(cmd *exec.Cmd) { execution.ConfigureProcessGroup(cmd) }🤖 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/background/process_windows.go` around lines 11 - 14, Update the comment above ConfigureChildProcessGroup to reference execution.TerminateProcessTree rather than the obsolete taskkill /T implementation detail, while preserving the explanation that Windows requires no launch-time process-group setup and POSIX configures Setpgid.internal/agent/loop.go (1)
2624-2649: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a shared constant for the generic approval reason. This literal is emitted in the denial path too; centralizing it keeps the suppression and the producer aligned if the wording changes.
🤖 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/agent/loop.go` around lines 2624 - 2649, The generic approval reason is duplicated as a string literal in the suppression check within userFacingPermissionReason. Replace that literal with the shared constant already used by the denial path, preserving the existing comparison behavior and keeping the producer and suppression logic aligned.internal/execution/process_manager_test.go (1)
1-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood concurrency coverage; consider regression tests for the merge/ordering bugs.
The existing tests cover retained-identity continuation, interrupt, wait clamping, buffer capping, and pruning-vs-touch races well. Consider adding a regression test asserting
ProcessResult.Interruptedstaystrueafter a context-cancelledStart()re-collects, and one assertingChanges()correctly reports files touched by a very fast command (covering the observer-ordering issue flagged inprocess_manager.go).As per coding guidelines, "add regression tests for behavior changes" for
**/*_test.go.🤖 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/execution/process_manager_test.go` around lines 1 - 171, Add regression coverage for the two noted behaviors: verify a context-cancelled Start flow preserves ProcessResult.Interrupted as true when the process is re-collected, and verify Changes() reports files touched by a very fast command despite observer ordering. Place the tests alongside the existing process-manager tests and reuse their established setup patterns.Source: Coding guidelines
internal/sandbox/runner.go (3)
310-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
/tmpfor the execution report path.
newLinuxExecutionReportPathhardcodes"/tmp"instead ofos.TempDir(). This ignoresTMPDIRoverrides and could fail in environments where/tmpisn't writable/available even though a valid temp dir is configured elsewhere.♻️ Proposed fix
func newLinuxExecutionReportPath() (string, error) { var token [16]byte if _, err := rand.Read(token[:]); err != nil { return "", fmt.Errorf("generate sandbox execution report path: %w", err) } - return filepath.Join("/tmp", "zero-sandbox-report-"+hex.EncodeToString(token[:])+".json"), nil + return filepath.Join(os.TempDir(), "zero-sandbox-report-"+hex.EncodeToString(token[:])+".json"), nil }🤖 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/runner.go` around lines 310 - 316, Update newLinuxExecutionReportPath to use os.TempDir() as the base directory instead of the hardcoded "/tmp" string, while preserving the existing randomized filename and error handling.
204-210: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRuntime prep + cleanup run unconditionally on every command execution.
prepareSandboxRuntime(and its call intocleanupSandboxRuntimeRoots) now runs on everyBuildCommandPlancall whenever sandboxing isn't forbidden/disabled — i.e., once per tool invocation in an agent loop. It doesMkdirAll+Chmodon ~14 directories, aChtimes, and a fullos.ReadDirscan of the runtime-roots parent directory each time. See the related concern raised onruntime_state.goaboutcleanupSandboxRuntimeRootspotentially evicting another workspace's still-in-use runtime root under this call pattern.🤖 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/runner.go` around lines 204 - 210, Cache or otherwise reuse the prepared sandbox runtime across repeated BuildCommandPlan executions instead of invoking prepareSandboxRuntime on every call. Update the flow around prepareSandboxRuntime and permissionProfileWithRuntime so setup and cleanup occur only when the workspace runtime state is absent or requires refresh, while preserving the existing preference and policy guards and error propagation.
891-906: 🔒 Security & Privacy | 🔵 TrivialDocument the macOS loopback exception in
networkRuleForProfile.NetworkDenystill allowslocalhostbind/inbound/outbound here, and on macOS that reaches the host’s real loopback rather than an isolated namespace. Keep that asymmetry explicit in the policy docs/tests, and tighten the outbound allowance if it isn’t required for local test servers.🤖 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/runner.go` around lines 891 - 906, Update networkRuleForProfile and its associated policy documentation/tests to explicitly record that NetworkDeny’s localhost bind, inbound, and outbound allowances are a macOS host-loopback exception rather than an isolated namespace. Verify whether the network-outbound localhost rule is required by local test servers; if not, remove or narrow it while preserving required listener 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.
Inline comments:
In `@internal/execution/process_manager.go`:
- Around line 179-193: Update the post-cancellation result merge in the process
execution flow to propagate the interruption state from more into result.
Specifically, alongside the existing field assignments for Output, Exited,
Report, and Changes, assign result.Interrupted from more.Interrupted so Start
returns the forced-termination status.
- Around line 413-436: Update managedProcess.collect to cap the cumulative
output assembled in output using the existing maxPendingOutputBytes limit, not
just the undrained buffer. When the limit is exceeded, retain only the bounded
output, mark truncated consistently with process.output.consumeTruncated(), and
preserve the existing finish and process-completion behavior.
- Around line 121-159: Update the process setup flow around NewChangeObserver
and startProcessTransport so the observer is created from
request.WorkspaceRoots[0] before starting the process transport, ensuring its
baseline is captured first. Preserve the existing cleanup and error-return
behavior if transport startup fails, and continue using only the first workspace
root.
In `@internal/sandbox/protected_create_linux.go`:
- Around line 115-190: Update protectedCreateMonitor.wait and scanAndRemove to
parse queued inotify_event records, including each event’s mask and filename,
rather than draining them only as a wake-up. When an IN_CREATE or IN_MOVED_TO
event matches a protected target, record the violation and invoke the existing
removal/notification flow even if os.Lstat(target) later reports the path is
absent; preserve polling/fallback behavior for monitors without inotify.
In `@internal/sandbox/runtime_state.go`:
- Around line 39-99: Add an active-use lease or lock in prepareSandboxRuntime
and make cleanupSandboxRuntimeRoots skip any non-current runtime root with an
active lease before calling os.RemoveAll, preventing eviction of roots used by
other processes; update internal/sandbox/runtime_state.go lines 39-99. In
internal/sandbox/runner.go lines 204-210, optionally cache the prepared
SandboxRuntime per workspace/session and reuse it to avoid repeated directory
setup and cleanup scans; this site requires only the requested optimization if
implemented.
In `@internal/sandbox/seccomp_other.go`:
- Around line 17-18: Update the comment above ApplyLinuxIsolatedNetworkGuard to
state that non-Linux platforms return ErrSeccompUnsupported rather than
describing the function as a no-op; leave the function behavior unchanged.
In `@internal/sandbox/seccomp.go`:
- Around line 159-171: Guard architecture-dispatch jump-offset narrowing in
isolatedNetworkGuardFilter and networkDenySeccompFilter by introducing a shared
bounds-checking helper for section lengths. Make it fail loudly when a section
exceeds the uint8 BPF jump range, and use it for both x86Section and armSection
offsets instead of direct uint8 conversions.
---
Nitpick comments:
In `@internal/agent/loop.go`:
- Around line 2624-2649: The generic approval reason is duplicated as a string
literal in the suppression check within userFacingPermissionReason. Replace that
literal with the shared constant already used by the denial path, preserving the
existing comparison behavior and keeping the producer and suppression logic
aligned.
In `@internal/background/process_windows.go`:
- Around line 11-14: Update the comment above ConfigureChildProcessGroup to
reference execution.TerminateProcessTree rather than the obsolete taskkill /T
implementation detail, while preserving the explanation that Windows requires no
launch-time process-group setup and POSIX configures Setpgid.
In `@internal/execution/process_manager_test.go`:
- Around line 1-171: Add regression coverage for the two noted behaviors: verify
a context-cancelled Start flow preserves ProcessResult.Interrupted as true when
the process is re-collected, and verify Changes() reports files touched by a
very fast command despite observer ordering. Place the tests alongside the
existing process-manager tests and reuse their established setup patterns.
In `@internal/sandbox/runner.go`:
- Around line 310-316: Update newLinuxExecutionReportPath to use os.TempDir() as
the base directory instead of the hardcoded "/tmp" string, while preserving the
existing randomized filename and error handling.
- Around line 204-210: Cache or otherwise reuse the prepared sandbox runtime
across repeated BuildCommandPlan executions instead of invoking
prepareSandboxRuntime on every call. Update the flow around
prepareSandboxRuntime and permissionProfileWithRuntime so setup and cleanup
occur only when the workspace runtime state is absent or requires refresh, while
preserving the existing preference and policy guards and error propagation.
- Around line 891-906: Update networkRuleForProfile and its associated policy
documentation/tests to explicitly record that NetworkDeny’s localhost bind,
inbound, and outbound allowances are a macOS host-loopback exception rather than
an isolated namespace. Verify whether the network-outbound localhost rule is
required by local test servers; if not, remove or narrow it while preserving
required listener behavior.
🪄 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: 3da2023f-f5c5-4fdb-a503-7f11dbe54937
📒 Files selected for processing (77)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/permission_additional_perms_test.gointernal/agent/prompt_budget_test.gointernal/agent/types.gointernal/background/process_posix.gointernal/background/process_windows.gointernal/cli/app.gointernal/cli/exec.gointernal/cli/exec_spec.gointernal/cli/hook_dispatch.gointernal/cli/mcp_tools.gointernal/cli/plugin_activate.gointernal/execution/change_observer.gointernal/execution/change_observer_test.gointernal/execution/contracts.gointernal/execution/contracts_test.gointernal/execution/process_manager.gointernal/execution/process_manager_test.gointernal/execution/process_unix.gointernal/execution/process_windows.gointernal/execution/pty_fallback.gointernal/execution/pty_linux.gointernal/execution/runner.gointernal/execution/runner_test.gointernal/execution/transport.gointernal/hooks/dispatch.gointernal/hooks/dispatch_test.gointernal/mcp/client.gointernal/mcp/client_test.gointernal/mcp/registry.gointernal/plugins/activate.gointernal/plugins/activate_test.gointernal/sandbox/analyzer.gointernal/sandbox/analyzer_test.gointernal/sandbox/engine.gointernal/sandbox/execution_adapter_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_linux.gointernal/sandbox/linux_helper_linux_test.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager.gointernal/sandbox/platform_contract_test.gointernal/sandbox/profile.gointernal/sandbox/protected_create_linux.gointernal/sandbox/protected_create_linux_test.gointernal/sandbox/request_permissions.gointernal/sandbox/request_permissions_test.gointernal/sandbox/runner.gointernal/sandbox/runner_linux_integration_test.gointernal/sandbox/runner_test.gointernal/sandbox/runtime_state.gointernal/sandbox/runtime_state_test.gointernal/sandbox/seccomp.gointernal/sandbox/seccomp_linux.gointernal/sandbox/seccomp_other.gointernal/sandbox/seccomp_test.gointernal/sandbox/windows_runner.gointernal/tools/bash.gointernal/tools/bash_proc_unix.gointernal/tools/bash_proc_windows.gointernal/tools/bash_tool_test.gointernal/tools/exec_command.gointernal/tools/exec_command_test.gointernal/tools/exec_pty_fallback.gointernal/tools/exec_transport.gointernal/tools/sandbox_denial.gointernal/tools/sandbox_denial_test.gointernal/tools/types.gointernal/tui/file_view_test.gointernal/tui/files_panel.gointernal/tui/files_panel_test.gointernal/tui/model.gointernal/tui/session.gointernal/tui/transcript.go
💤 Files with no reviewable changes (2)
- internal/tools/exec_pty_fallback.go
- internal/tools/exec_transport.go
|
fix coderabbit changes @anandh8x |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
This is a big, careful piece of work, and the core security posture holds up. I went at the scariest part hard and it is sound.
The central question, whether "keep exec_command available in auto-approve mode" lets a command run outside the sandbox or the permission gate, is no. AdvertiseInAuto is visibility-only (read only by ToolAdvertised; it never sets permissionGranted), and the gate is enforced at two fail-closed layers: executeToolCall (exec_command's effectivePermission stays PermissionPrompt, so Evaluate + shouldRequestPermission still prompt) and again in registry.RunWithOptions (refuses a prompt tool when Action==ActionPrompt && !PermissionGranted). Ordinary shell only auto-allows on the shellSandboxActive branch, which is false for any unavailable/disabled/re-entrant/Windows-uninitialized backend, so auto-allow never runs an unsandboxed command; require_escalated (the only path that nulls the engine) always prompts in non-Unsafe modes, so no-prompt and bare-run cannot co-occur. write_stdin only writes bytes to an already-sandboxed session's stdin, so it cannot spawn a command past the gate. The -559/+199 rewrite kept the same engine resolution. I also ran the agent permission suite (40 tests: RejectsMissingAdditionalPermissionsBeforePrompt, DeniesPromptToolWithoutUnsafePermission, RejectsLocalWebFetchBeforePermissionRequest, and so on) and they pass. The permission-explanation rework is display-only too: it rewords the Reason string, but action/risk/block come from the typed Decision before the reword, so it cannot flip a Deny into an Allow.
Windows validation you asked for: clean. The PR does not actually touch the Windows enforcement mechanics. Restricted-token creation, the workspace ACL, the protected-metadata jail, and the WFP outbound filter all live in windows_process_windows.go / adapters.go, which are not in this PR's file set. The only Windows source change is a one-line runtime-env addition in windows_runner.go, and wrapped commands still route through the runner unchanged. On native Windows I ran a full build, vet, gofmt, and the execution/sandbox/tools packages plus the agent permission tests, all green. So the Windows path is safe as far as this diff goes. The one thing I cannot automate here is a full admin-privileged end-to-end TUI pass (real restricted-token exec + ACL write-jail), but nothing in the diff should change that behavior.
A handful of things I would want resolved before it lands: one real bug, and a couple of posture trade-offs to bless explicitly.
1. [fix] Interactive output is unbounded and can OOM the host. In execution/process_manager.go, collect() appends every drained chunk to a plain var output []byte for the whole poll window and only stops on the deadline (up to 30s interactive, 5min on an empty poll). The 2MB maxPendingOutputBytes bounds only the pending buffer, not the accumulated result: the captured path hard-drops past its 1MB cap, but the interactive path does not. A flooding process (for example yes under write_stdin polling) grows that slice at pipe throughput and can allocate multi-GB, and it busy-spins draining+appending at ~100% CPU for the window. Cap output the way capturedBuffer caps, and set truncated=true.
2. [confirm intended] macOS no-network now allows localhost. runner.go networkRuleForProfile's isolated default went from (deny network*) to deny-plus-allow-localhost (bind + inbound + outbound). macOS has no netns, so this lets a sandboxed command reach host loopback services (the Zero daemon's own port, a local DB, a localhost-bound admin API) that deny-all previously blocked. The comment says it is for test servers, which is reasonable, but it is a genuine loosening of the no-network guarantee on macOS, worth an explicit "yes, we accept localhost reachability in the isolated profile." (Linux is unaffected: --unshare-net gives its own loopback.)
3. [confirm intended] Linux nonexistent-protected-metadata went from a kernel guarantee to a userspace race. For a protected path that does not exist yet, main mounted a read-only tmpfs (--perms 555 --tmpfs --remount-ro), so creation was impossible (EROFS). Head routes the nonexistent case to a userspace inotify + 10ms-poll monitor that kills bwrap and RemoveAll's the target only after it appears. The workspace root is a real rw bind, so a command can mkdir .zero/... and it physically lands on the host and is readable during the detect-and-kill window. I assume this was to let the agent legitimately create .zero at runtime, but it trades a hard kernel guarantee for a best-effort race, so it is worth confirming the window is acceptable or gating it tighter.
4. [fix/confirm] Grant migration: a read now writes to disk, and legacy migration skips the policy re-check for non-shell allows. readState turns a persistent-grant Lookup into a mandatory backup + writeState for any v1/v2 or policy-mismatched file, so a pure read now mutates disk, which can fail or race under the new lock. It also dispatches v1/v2 to migrateLegacyState before the policyVersion comparison, so a legacy file's non-shell ALLOW grants are preserved without re-validating against the current policy (migrateLegacyState only invalidates shell allows). To be fair, the migration is otherwise more conservative than before (invalidates on migration, fails closed on corrupt, write-only 0600 backups), but the read-writes-disk and the legacy policy-skip are worth a look.
5. [minor] The PTY fallback restores a shared SysProcAttr that hardenPTYProcess already mutated, so a non-PTY fallback keeps Setsid/Setctty. And the Linux seccomp change drops the socket-block for the network dimension, so no-network isolation now rests solely on the netns (the ptrace/process_vm/io_uring denials stay). Both minor, noting for completeness.
For the record, a lot of what I checked came back clean or hardened: sandbox_denial is now typed (never inferred from stdout), analyzer network classification is broader (more restrictive), manager.go adds a fail-closed check when the native backend is unavailable but a deny is configured, and grant migration / runtime-state carry no tamperable trust.
Really solid core. Happy to re-review quickly once #1 is fixed and #2/#3 get an explicit thumbs-up.
Bound retained interactive output, preserve interruption state, and start workspace observation before process launch. Protect active runtime roots with cross-platform leases, capture transient protected-path creation events, and restore process attributes after PTY fallback.
|
@Vasanthdev2004 @coderabbitai Follow-up pushed in b9b96c6. Accepted correctness fixes:
Design confirmations:
Validated with formatting, vet, full tests, race tests for execution and sandbox, Linux build and smoke, Windows and macOS cross-compilation, govulncheck, diff hygiene, and a real termctrl TUI pass covering ordinary exec, the reasoned permission prompt, session creation, write_stdin, and clean interactive completion. Please re-review the focused follow-up. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/execution/process_manager_test.go (1)
210-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid unbounded spin loops in tests.
Using an unbounded
runtime.Gosched()loop can hang indefinitely if there is a bug that preventscollectfrom draining the buffer. This will peg a CPU core and wait for the test runner's global timeout (e.g., 10 minutes) instead of failing gracefully.Consider adding a timeout and a small
time.Sleepto fail the test immediately and cleanly on regressions.🛠️ Proposed refactor for a safer wait loop
- for { - process.output.mu.Lock() - drained := len(process.output.data) == 0 - process.output.mu.Unlock() - if drained { - break - } - runtime.Gosched() - } + deadline := time.Now().Add(5 * time.Second) + for { + process.output.mu.Lock() + drained := len(process.output.data) == 0 + process.output.mu.Unlock() + if drained { + break + } + if time.Now().After(deadline) { + panic("timeout waiting for process output to be drained") + } + time.Sleep(10 * time.Millisecond) + }🤖 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/execution/process_manager_test.go` around lines 210 - 218, Replace the unbounded wait loop around process.output draining with a bounded timeout that fails the test if the buffer is not emptied promptly. Add a short time.Sleep between checks to avoid busy-spinning, while preserving the existing mutex-protected drained check and normal completion 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.
Inline comments:
In `@internal/sandbox/protected_create_linux_test.go`:
- Around line 56-67: In the transient-event test, wait for the monitor’s
violation callback by receiving from the triggered channel before calling
monitor.stopAndCleanup(). Keep the existing violation and target assertions, and
retain a timeout or failure path so the test cannot block indefinitely if the
callback is never invoked.
In `@internal/sandbox/runtime_lease_windows.go`:
- Around line 17-46: Update acquireSharedRuntimeLease to handle
windows.ERROR_IO_PENDING by assigning an event to handle.overlapped and waiting
for the lock operation to complete before returning success; preserve cleanup
and error propagation on wait failure. In tryAcquireExclusiveRuntimeLease, treat
ERROR_IO_PENDING like ERROR_LOCK_VIOLATION for the immediate contention result,
closing the file and returning the existing busy status.
---
Nitpick comments:
In `@internal/execution/process_manager_test.go`:
- Around line 210-218: Replace the unbounded wait loop around process.output
draining with a bounded timeout that fails the test if the buffer is not emptied
promptly. Add a short time.Sleep between checks to avoid busy-spinning, while
preserving the existing mutex-protected drained check and normal completion
behavior.
🪄 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: 7992bcde-a311-44de-aa58-86f71dca453b
📒 Files selected for processing (14)
internal/background/process_windows.gointernal/execution/process_manager.gointernal/execution/process_manager_test.gointernal/execution/pty_linux_test.gointernal/execution/transport.gointernal/sandbox/protected_create_linux.gointernal/sandbox/protected_create_linux_test.gointernal/sandbox/runner.gointernal/sandbox/runtime_lease.gointernal/sandbox/runtime_lease_unix.gointernal/sandbox/runtime_lease_windows.gointernal/sandbox/runtime_state.gointernal/sandbox/runtime_state_test.gointernal/sandbox/seccomp_other.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/sandbox/seccomp_other.go
- internal/background/process_windows.go
- internal/execution/transport.go
- internal/execution/process_manager.go
- internal/sandbox/runtime_state.go
- internal/sandbox/runner.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed the follow-up (b9b96c6). This addresses everything cleanly. Approving.
Verified on the new head:
- The OOM is fixed. appendBoundedProcessOutput hard-caps the cumulative interactive output at 2 MiB, keeping the newest bytes and setting truncated, and collect() runs it on every drain, so a flooding process can no longer grow the slice unbounded.
- The Linux protected-metadata monitor is hardened the right way: it watches the parent for IN_CREATE|IN_MOVED_TO and parses the raw inotify events, so a fast create+unlink is still reported by the kernel event rather than slipping between Lstat polls. The absent-path-stays-absent contract is preserved.
- The new runtime lease is correct and fails safe: a command holds a shared flock/LockFileEx on its runtime root for its lifetime, and cleanup takes an exclusive lease and skips deletion when the root is in use or on any lease error, so cleanup can never evict an active root and a lease failure never weakens enforcement. Nice touch using the typed windows.LockFileEx wrapper on Windows rather than a raw LazyProc.Call, which sidesteps the last-error versus return-value trap entirely.
- The PTY-fallback SysProcAttr restore and the seccomp comment corrections are in, with regressions.
I built and ran it on native Windows: build, vet and gofmt clean, and the execution and sandbox packages pass. (plan9 is not a concern here: the subsystem already does not build there on main via internal/fsutil, unlike a portable package.)
On CodeRabbit's follow-up note about runtime_lease_windows.go handling ERROR_IO_PENDING: I checked it and it does not apply. os.OpenFile returns a synchronous handle, so windows.LockFileEx blocks to completion and never returns IO_PENDING. I confirmed it with a contention probe on real Windows: while an exclusive lease is held, acquireSharedRuntimeLease blocks (about 400ms) and then completes cleanly after release, never returning early. So the lease code is correct as written. The other two CodeRabbit items are test-quality nitpicks (bounded wait loops in the execution and protected-create tests) worth doing as polish but not blocking.
On the three design confirmations, I am good with all of them:
- macOS host-loopback in the isolated profile: accepted as an intentional, macOS-only trade-off for local test servers, with non-loopback still denied and the asymmetry now explicit in code. The only residual worth keeping in mind is that a sandboxed command can reach localhost-bound services (for example the daemon port) on macOS; fine given the daemon token protection, just flagging that it is in scope there now.
- Absent protected paths staying absent for workspace fidelity: reasonable, and the inotify hardening closes the evasion I was worried about.
- Legacy migration being lossless only for exact non-shell mappings while shell meanings and v3 policy mismatches invalidate, atomic and fail-closed: sound.
The one thing that can only be validated by actually running it is a full admin-privileged Windows end-to-end pass (real restricted-token exec plus ACL write-jail), and you noted you did a real termctrl TUI pass, which is good enough for me on this diff since it does not touch the Windows enforcement mechanics anyway.
Thanks for the fast, thorough turnaround. LGTM.
Remove Seatbelt host-local network allowances after native validation showed localhost filters can reach host interfaces. Track process reap completion so retained sessions cannot signal a stale numeric PID.
|
@gnanam1990 @Vasanthdev2004 @coderabbitai Native macOS review follow-up pushed in 84d0bc4. Correction to my earlier loopback confirmation: live Seatbelt validation showed that the localhost filters can reach host interface addresses, not only 127.0.0.1. That makes the exception incompatible with NetworkDeny. The macOS restricted profile is now back to a strict deny network rule, with regressions asserting that no host-local bind, inbound, or outbound IP allowances are emitted. The retained-session PID finding was also valid. Process reap completion is now recorded immediately after Wait returns, and terminate refuses to call the raw process-group kill path once the process has been reaped. A regression verifies that a retained completed session cannot signal a stale numeric PID. I did not change migration backup retention. Those files are inactive recovery artifacts, mode 0600, and never loaded by grant lookup. A host user who explicitly restores one can already edit the unsigned live grant store; deleting backups would change the documented recovery behavior rather than close an authorization path. The follow-up passes the focused regressions, full tests, execution and sandbox race tests, formatting, vet, build, smoke, Windows and macOS cross-compilation, vulnerability scan, and diff hygiene. The rebuilt TUI also completed normal exec plus session and write_stdin interaction successfully. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames 84d0bc47
printf '\n--- changed paths ---\n'
git diff-tree --no-commit-id --name-only -r 84d0bc47
printf '\n--- outlines ---\n'
for f in internal/sandbox/runner.go internal/sandbox/runner_test.go internal/execution/process_manager.go internal/execution/process_manager_test.go; do
if [ -f "$f" ]; then
echo "### $f"
ast-grep outline "$f" --items all --type function,method --view compact || true
fi
done
printf '\n--- focused diff ---\n'
git show --format=fuller --find-renames --find-copies -- \
internal/sandbox/runner.go \
internal/sandbox/runner_test.go \
internal/execution/process_manager.go \
internal/execution/process_manager_test.go \
84d0bc47Length of output: 7603 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- process manager lifecycle map ---'
ast-grep outline internal/execution/process_manager.go --items all --type function,method --view signatures
printf '%s\n' '--- managedProcess lifecycle implementation ---'
sed -n '130,215p' internal/execution/process_manager.go
sed -n '400,545p' internal/execution/process_manager.go
printf '%s\n' '--- termination / reaped references ---'
rg -n -C 5 --type go '\.terminate\s*\(|reapedClosed\s*\(|reaped\b|command\.Wait\s*\(' internal/execution
printf '%s\n' '--- changed platform contract assertions ---'
git diff 84d0bc47^ 84d0bc47 -- internal/sandbox/platform_contract_test.go
sed -n '1,150p' internal/sandbox/platform_contract_test.goLength of output: 24689
One process-safety race remains in
I agree that inactive, ✏️ Learnings added
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-approving on 84d0bc4. Both follow-up changes are correct, and I built and ran them on Windows (build, vet and gofmt clean; execution and sandbox tests pass).
- macOS isolation is fully restored: networkRuleForProfile's restricted default is back to a plain (deny network*), with the localhost bind/inbound/outbound allowances removed and regressions asserting no host-local allowance is emitted. Good call validating it live rather than trusting the earlier reasoning: a Seatbelt localhost filter reaching host interface addresses (not just 127.0.0.1) is exactly the kind of thing worth catching, and strict deny is the right resolution.
- The stale-PID fix is right: terminate() now returns once reapedClosed() (the reaped channel is closed right after command.Wait()), so a retained completed session can no longer KillProcessTree a reused numeric PID. There is an irreducible sliver between the reap inside Wait() and close(reaped), but that is inherent to PID-based signaling; the practical case is now guarded and the regression pins it.
Everything from the earlier round still holds (the 2 MiB output cap, the inotify create/moved-to hardening, the runtime leases, and the PTY/seccomp fixes). LGTM.
|
@coderabbitai The post-Wait/pre-close interval is acknowledged, but it is not removable with user-space channel or mutex synchronization: exec.Cmd.Wait performs the kernel reap internally before returning control to this code. Closing reaped on the next instruction removes the practical retained-session exposure; the remaining interval requires PID reuse plus a concurrent termination call between two adjacent instructions plus the reused PID becoming a matching process-group target. Eliminating that theoretical interval fully would require OS-specific stable process/group identity mechanisms rather than a focused synchronization change. A mocked deterministic interleaving would not exercise the real kernel boundary inside exec.Cmd.Wait. Native review reached the same conclusion and accepted this as the irreducible residual of PID-based group signaling. Given that the latest incremental review reports no actionable comments, all required checks pass, and the human re-review approves 84d0bc4, I am leaving the current focused fix unchanged. Deeper cross-platform process-identity hardening can be considered separately if the project wants to replace PID-based process-group signaling. |
|
Tip For best results, initiate chat on the files or code changes.
I’m withdrawing this finding for the current PR. If desired, would you like a follow-up issue for cross-platform process-identity hardening? ✏️ Learnings added
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
COMMENT — macOS native validation (the platform this PR most needed exercised). The PR body asked @Vasanthdev2004 to validate native Windows; nobody was assigned macOS, so I validated the seatbelt path on real Apple-silicon hardware — not by reading code, but by driving sandbox-exec through the kernel. Two of the three concerns from my earlier pass are now fixed and verified live by the latest two commits; one real issue remains, plus a doc-accuracy note.
✅ Fixed since b8eec2d — both verified live on 84d0bc47
1. macOS network isolation regression — RESOLVED. 84d0bc4 fix(sandbox): restore strict macOS isolation reverts the localhost carve-out to a full (deny network*), with the correct rationale in the comment (seatbelt has no private network namespace, so its localhost filter can't be trusted). I re-ran my live kernel test under NetworkDeny (the default):
| target | before (b8eec2d) | now (84d0bc4) |
|---|---|---|
external 1.1.1.1:53 |
denied | denied |
loopback 127.0.0.1:<svc> |
reachable ❌ | denied ✅ |
host LAN 192.168.x.x:<svc> |
reachable ❌ | denied ✅ |
The earlier (allow network-outbound (remote ip "localhost:*")) matched the host's real loopback and interface IPs (no netns on macOS), so a sandboxed command could reach a local proxy / DB / docker-tcp and escape "isolated". That hole is closed.
2. Execution lifecycle race — RESOLVED. b9b96c6 fix(sandbox): close execution lifecycle races guards terminate() with if process.reapedClosed() ... { return }, so it no longer SIGKILLs a reaped (and possibly OS-recycled) PID via KillProcessTree. The new TestManagedProcessTerminateSkipsReapedProcess passes under -race, and the injectable kill makes it deterministic. Good fix.
macOS runtime checks — all green
- Seatbelt command wrapping: every plan is
Wrappedwith backendsandbox-exec;TestSeatbeltEnforces*PASS (real kernel enforcement, 0 SKIP). - Isolated localhost vs external egress: external denied, and on macOS localhost is now denied too (fail-closed — see the note below).
- Metadata protection:
.git/hooks,.git/config,.zero,.agentsare kernel-denied for writes while ordinary workspace writes and benign.gitmetadata succeed; deny-after-allow ordering holds.TestSeatbeltProfileProtectsMetadataAndDenyOrderingandTestSeatbeltAllowsGitMetadataWritesExceptHooksAndConfigPASS. - exec_command → write_stdin: live session + stdin drive works, real content still prompts, interrupt terminates, spawned process stays sandboxed.
internal/tools/internal/executionpass under-race.
⚠️ Remaining real concern — grant-store backups defeat "safe invalidation" (Major, still present)
internal/sandbox/grants.go — grants.go is untouched by the two new commits, so this stands from my earlier read. Every migration writes the entire prior grant set verbatim to a sibling <store>.<label>.backup (writeBackup, grants.go:754 via writeLegacyBackup grants.go:750 / migrateChangedPolicy grants.go:706). Nothing ever deletes these — Clear() (grants.go:407), Revoke(), and RevokePath() only touch the live file.
Reproduced live on 84d0bc47:
- write a legacy (
schemaVersion:2) store with anallowgrant → read it (migrates, dropsg.json.v2.backup) →Clear()→ the backup survives and still contains the originalallowgrant, restorable with a singlemv.
So zero sandbox grants clear leaves the full old approval set recoverable on disk, and a grant that migration deliberately invalidated (e.g. a legacy shell allow that command-unification changed) sits one mv from resurrection. It also accumulates unbounded across migrations. This directly undercuts the PR's stated "safe invalidation with recoverable backups" guarantee. It is bounded to same-user file access (mode 0600 in a 0700 dir) — not a cross-principal escalation — hence Major, not blocker. Suggested fix: have Clear()/Revoke* also remove the sibling *.backup files, and/or expire them.
📝 Doc-accuracy note
The PR objective "isolated localhost/loopback remains usable while external egress is restricted" no longer holds on macOS after the isolation fix — localhost is now denied there too (the correct call, since seatbelt can't isolate it). The claim is still true on Linux (real netns). Worth updating the PR/description so the platform difference is explicit.
Build / validation (macOS, 84d0bc47)
gofmt clean · go vet ./internal/sandbox/... ./internal/execution/... clean · go build ./cmd/zero OK · GOOS=windows GOARCH=amd64 cross-build of sandbox+execution OK · seatbelt integration + lifecycle suites pass. The internal/tools TestRegistrySandboxGatesPathAliasKeys failure is environmental, not this PR — it fails byte-identically on the merge base (3967d49d) because the worktree lives under /private/tmp (the test's .zero-sandbox-outside-* dir lands inside the workspace). Same class as the other ~7 /private/tmp symlink test failures; not attributable to #781.
Net: the security-critical macOS path is now solid and the two regressions I flagged are genuinely fixed. The grant-backup recoverability is the one real item left; it's same-user-bounded, so I'm commenting rather than blocking. Merge is kevin's call per the program gate.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — the macOS path is now solid and two of my earlier concerns are genuinely fixed, but one unaddressed issue defeats a feature this PR explicitly claims, so I'm holding on that single item.
What's blocking — grant-store backups defeat the PR's own "safe invalidation" (Major)
internal/sandbox/grants.go (untouched by the two latest commits). Every migration writes the entire prior grant set verbatim to a sibling <store>.<label>.backup (writeBackup grants.go:754, via writeLegacyBackup grants.go:750 and migrateChangedPolicy grants.go:706). Nothing ever removes them — Clear() (grants.go:407), Revoke(), and RevokePath() only touch the live file.
Reproduced live on 84d0bc47:
- legacy (
schemaVersion:2) store with anallowgrant → first read migrates it and drops<store>.v2.backup→zero sandbox grants clear→ the backup survives and still holds the originalallowgrant, restorable with a singlemv.
Two consequences, both against the stated guarantee:
grants clearleaves the full old approval set recoverable on disk — a user who wipes approvals hasn't.- A grant migration deliberately invalidated (e.g. a legacy shell allow that command-unification changed) sits one
mvfrom resurrection.
Backups also accumulate unbounded across migrations.
It's bounded to same-user file access (0600 in a 0700 dir), so not a cross-principal escalation — but it directly undercuts "safe invalidation with recoverable backups," which is why it's the one item I'd fix before merge. Suggested fix: have Clear() / Revoke*() also delete the sibling *.backup files (glob <store>.*.backup*), and/or expire them after the migration notice is consumed.
Also please address (non-blocking, but do it in the same push)
Doc accuracy — the objective "isolated localhost/loopback remains usable while external egress is restricted" no longer holds on macOS after 84d0bc4; localhost is now denied there too (the correct call — seatbelt has no netns to isolate it). Still true on Linux. Update the description so the platform difference is explicit.
Verified good — for the record (real macOS kernel, base-compared)
The two regressions I flagged earlier are genuinely fixed:
- Network isolation (
84d0bc4): underNetworkDeny, external / loopback / host-LAN are now all kernel-denied (I re-ran the livesandbox-exectest; the earlierremote ip "localhost:*"leak to host loopback+LAN is gone). - Execution lifecycle (
b9b96c6):terminate()now guardsreapedClosed(), so it can'tSIGKILLa reaped/recycled PID;TestManagedProcessTerminateSkipsReapedProcesspasses under-race.
macOS runtime checks all green: seatbelt command wrapping (TestSeatbeltEnforces* PASS, 0 SKIP); .git/hooks / .git/config / .zero / .agents writes kernel-denied with correct deny-after-allow ordering; exec_command→write_stdin live session works, prompts on real content, stays sandboxed. gofmt/go vet clean; go build + Windows cross-build OK. The internal/tools TestRegistrySandboxGatesPathAliasKeys failure is environmental (fails identically on base 3967d49d under /private/tmp), not this PR.
Solid, security-critical work — just purge the backups on clear/revoke and this clears from my side. Merge is kevin's call per the program gate.
|
@gnanam1990 Thank you for the native macOS validation. I updated the PR description to state the platform behavior accurately: Linux retains sandbox-local loopback through its private network namespace, while macOS denies localhost together with external IP networking under NetworkDeny. On migration backups, the observed retention after Clear/Revoke is real, but it does not restore authorization state. The backup is an inactive mode-0600 recovery artifact, grant lookup never reads it, and restoring it requires an explicit same-user filesystem operation by a principal who can already replace the unsigned live grant store. Here, invalidation means that prior grants are no longer active or automatically accepted; it is intentionally not a secure-erasure guarantee. Removing all sibling backups during Revoke or RevokePath would also change the documented recovery contract and can destroy recovery data containing grants unrelated to the particular revocation. The unbounded-retention question is worth handling separately as a product decision, where expiry, explicit backup purge, and secure-clear semantics can be specified without changing this focused PR. Given that distinction, I am leaving the grant implementation unchanged and asking you to reconsider the blocking review. If the team wants Clear to guarantee deletion of every historical recovery copy, that should be captured as a separate explicit contract rather than inferred as an authorization fix here. |
|
@coderabbitai Your latest follow-up withdrew the remaining process-lifecycle finding for this PR, and the latest incremental review reported no actionable comments. All required checks are passing on head 84d0bc4. If you have no remaining actionable findings on the current head, please clear or dismiss your stale CHANGES_REQUESTED review so the formal review state matches your latest conclusion. |
|
Tip For best results, initiate chat on the files or code changes.
To replace the stale
|
|
@anandh8x You're right, and I verified it rather than take it on trust — so I'm withdrawing the block. I confirmed both of your points on
So my "recoverable via mv" framing overstated it — the backup is an inert, same-user 0600 recovery artifact, and reactivating it needs an explicit filesystem action by a principal who could already rewrite the unsigned live store directly. Your distinction is correct: invalidation here means prior grants are no longer active or auto-accepted, not secure erasure. And I agree that having I'm reclassifying the retention point to a Minor follow-up (bounded backup lifetime / explicit purge / secure-clear semantics), which as you say is a separate product decision, not something to force into this focused PR. Dismissing my CHANGES_REQUESTED now. Thanks also for updating the description with the accurate Linux-vs-macOS localhost behavior. For the record, the macOS native path is validated: strict network isolation now denies external + loopback + host-LAN under NetworkDeny (re-tested live through sandbox-exec), the lifecycle |
Withdrawing: verified anand's rebuttal — grant lookup never reads the backup and a cleared grant does not reactivate on reload, so this is not an authorization issue. Reclassified the backup-retention point as a Minor follow-up (bounded lifetime/explicit purge), a separate product decision, not a blocker for this PR.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE (head 84d0bc47) — macOS native path validated on real hardware.
Both regressions I raised earlier are fixed and re-verified live through sandbox-exec:
- Network isolation (
84d0bc4): underNetworkDeny, external + loopback + host-LAN are all kernel-denied — the earlierremote ip "localhost:*"leak to host loopback/LAN is gone. - Execution lifecycle (
b9b96c6):terminate()guardsreapedClosed(), so noSIGKILLof a reaped/recycled PID;TestManagedProcessTerminateSkipsReapedProcesspasses under-race.
macOS runtime checks green: seatbelt command wrapping (TestSeatbeltEnforces* PASS, 0 SKIP); .git/hooks / .git/config / .zero / .agents writes kernel-denied with correct deny-after-allow ordering; exec_command→write_stdin live session works, prompts on real content, stays sandboxed. gofmt/vet/build clean, Windows cross-build OK. The one internal/tools test failure is environmental (identical on base 3967d49d under /private/tmp), not this PR.
The grant-store backup-retention point is a Minor follow-up — I verified it's not an authorization issue (grant lookup never reads the backup; a cleared grant stays inactive on reload), so bounded backup lifetime / explicit purge is a separate product decision, not a blocker for this PR.
Merge is kevin's call per the program gate.
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
✅ Action performedComments resolved and changes approved. |
3 participants
Summary
exec_commandavailable in auto-approve mode so process creation andwrite_stdininteraction are both usableUser-facing verification
Tested the current build through the real TUI:
exec_commandruns successfully in auto-approve modewrite_stdinPlatform verification requested
@Vasanthdev2004, could you please validate the native Windows path? In particular:
exec_commandreturning a live session followed bywrite_stdin@gnanam1990, could you please validate the native macOS path? In particular:
.git/hooks,.git/config,.zero, and.agentsremain protectedexec_commandreturning a live session followed bywrite_stdinThe platform adapters have shared contract coverage and cross-compile successfully from Linux; these requested checks cover native runtime behavior on the actual operating systems.
Validation
make fmt-checkgo vet ./...go test ./...go test -race ./internal/agent ./internal/execution ./internal/tools ./internal/sandboxgo run ./cmd/zero-release buildgo run ./cmd/zero-release smokego run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...git diff HEAD --checkThe pinned advisory lint still reports the existing repository-wide backlog; no findings point to files or lines introduced by this change.
Summary by CodeRabbit
New Features
Bug Fixes