fix(sandbox): web_search/web_fetch hard-denied under default network policy - #202
Conversation
…eny too Follow-up to the EnforceToolNetwork change: web_search/web_fetch were still hard-denied under NetworkDeny because the engine-level gate in Evaluate (netMode==NetworkDeny && risk has "network") fired BEFORE the tool's RunWithSandbox/NetworkHostAllowed ever ran. The earlier change only relaxed NetworkHostAllowed, so the user-facing path (Evaluate -> registry) still blocked the tool with "network access is blocked by sandbox policy". Fix: Evaluate now skips the network-deny for a request that declares SideEffectNetwork (the first-party in-process tools, web_search/web_fetch) when EnforceToolNetwork is off — mirroring the NetworkHostAllowed exemption so the two gates agree. A SHELL command merely classified as network (SideEffectShell) is NOT exempt, so shell egress stays blocked under deny. EnforceToolNetwork restores the strict deny for the tools. Tests: TestEvaluateExemptsNetworkToolsFromDenyByDefault covers the full path (default-exempt, granted->allow, EnforceToolNetwork->deny, shell stays denied); the two existing gate tests now opt into EnforceToolNetwork to keep testing the strict path. Gates: gofmt, vet, build, test (incl. -race), staticcheck (no new), govulncheck (0), deadcode (no new unreachable funcs).
|
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 (3)
Walkthrough
ChangesNetwork Tool Exemption from Engine-Level Deny
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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 |
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
There was a problem hiding this comment.
Reviewed locally. No blocking findings.
The change keeps Evaluate aligned with NetworkHostAllowed: first-party SideEffectNetwork tools remain exempt by default when EnforceToolNetwork is off, while shell network risk and explicit EnforceToolNetwork still deny. The added tests cover the default exemption, strict opt-in, shell non-exemption, and scoped gate behavior.
Tests:
- GOCACHE=/tmp/zero-go-cache go test ./internal/sandbox -run 'TestEvaluateExemptsNetworkToolsFromDenyByDefault|TestEngineDeniesNetworkSideEffectWhenPolicyBlocksNetwork|TestScopedNetworkGateInEvaluate': pass
- GOCACHE=/tmp/zero-go-cache go test ./internal/sandbox ./internal/tools: pass
- GOCACHE=/tmp/zero-go-cache go test ./...: fails in internal/imageinput TestLoadDocumentTruncatesLongText, outside this PR changed files.
Problem
With the default sandbox policy (
network: deny),web_search(andweb_fetch) are hard-denied before they run:#199 added
EnforceToolNetwork(off by default) to exempt the first-party in-process network tools, but it only relaxedNetworkHostAllowed— the per-host check inside the tool'sRunWithSandbox. The actual deny happens earlier, at the engine-level gate inEvaluate(run by the tool registry): a request whose risk includesnetworkunderNetworkDenyis rejected before the tool executes. So the tool never reached the relaxed check. (#199 merged at the commit just before this follow-up, so the gap shipped tomain.)Fix
Evaluatenow skips the network-deny for a request that declaresSideEffectNetwork(the first-party in-process toolsweb_search/web_fetch) whenEnforceToolNetworkis off — mirroring theNetworkHostAllowedexemption so the engine-level gate and the per-tool gate agree. A shell command merely classified as network (SideEffect=shell, e.g.curl) is not exempt, so sandboxed-shell egress stays blocked under deny. SettingEnforceToolNetwork: truerestores the strict deny for the tools.Net effect:
web_search/web_fetchare no longer hard-blocked by the sandbox; they follow the normal permission flow (prompt in ask mode → run on allow; run in auto mode).Tests
TestEvaluateExemptsNetworkToolsFromDenyByDefault— full path: default → not denied; granted →allow;EnforceToolNetwork=true→ denied; network shell command → stays denied.TestEngineDeniesNetworkSideEffectWhenPolicyBlocksNetworkandTestScopedNetworkGateInEvaluateupdated to opt intoEnforceToolNetwork(they test the strict gate).Gates (local)
gofmt -l .empty ·go vet·go build·go test ./...(incl.-raceon sandbox+tools) ·staticcheckno new findings ·govulncheck0 ·deadcode -test=falseno new unreachable funcs.Scoped to the sandbox fix only — 3 files (
engine.go,engine_test.go,runner_scoped_test.go).Summary by CodeRabbit