Align web tool permission prompts - #242
Conversation
Treat hosted web_search as an allowed read-only network discovery tool so normal searches do not stop on a permission prompt, while retaining sandbox network enforcement when tool-network policy is enabled. Add host-scoped persistent grants for web_fetch URLs so an always-allow decision is bounded to the requested host instead of becoming a blanket tool-wide fetch grant. Update permission prompt and transcript rendering to show network targets, scoped always labels, and preserved scope metadata across live rows, render caching, and session hydration. Tested: GOCACHE=/tmp/zero-go-cache go test ./...
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (19)
WalkthroughAdds a ChangesHost-scoped web_fetch grants, web_search allow model, and TUI scope rendering
Sequence Diagram(s)sequenceDiagram
participant Agent
participant Engine
participant GrantScope as grant_scope.go
participant GrantStore as grants.go
participant TUI
Agent->>Engine: Evaluate(web_fetch, {url: "https://example.com/..."})
Engine->>GrantScope: DeriveScope("web_fetch", args)
GrantScope-->>Engine: ("example.com", ScopeHost)
Engine->>GrantScope: resolveScopeForKind("example.com", ScopeHost, root)
GrantScope-->>Engine: "example.com"
Engine->>GrantStore: Lookup(reqScope="example.com")
GrantStore-->>Engine: ActionAllow (if host grant exists)
Engine-->>Agent: ActionAllow / ActionPrompt
Agent->>TUI: PermissionEvent{Scope: "example.com", SideEffect: "network"}
TUI->>TUI: permissionEventScopeLabel → "target"
TUI->>TUI: permissionOptionLabel → "always for this host"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. 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. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review: web tool permission alignment
Reviewed the security-sensitive parts closely (grant scoping, the web_search auto-allow, scope-through-cache/hydration) and ran build/test/staticcheck on the branch. On net this tightens the sandbox, and both headline changes hold up. Approving.
Security — verified sound
web_search auto-allow does not bypass the network policy. web_search.go flips to Permission: PermissionAllow, but in-process network tools were never subject to the sandboxed-shell's Network: deny gate in the first place — that gate is for the shell (engine.go effectiveNetworkMode; see the NetworkHostAllowed comment). The opt-in EnforceToolNetwork path still holds web_search to the policy: RunWithSandbox still calls enforceScopedNetworkPolicy(engine, host), and sameHostRedirectPolicy still fails closed on cross-host redirects. The egress target is operator-fixed — web_search's schema exposes only query/limit/domains (no host/URL), so the model can't redirect it. Net effect: removes a prompt, keeps enforcement.
Host-scoped web_fetch grants are matched exactly — and this narrows the previously tool-wide grant. grantCovers's ScopeHost case is normalizeHostScope(reqScope) == normalizeHostScope(grant.Scope) — exact equality, no prefix/suffix. I exercised the confusion vectors: a grant for example.com correctly refuses api.example.com (subdomain), example.com.attacker.net (sibling suffix), and the userinfo trick https://example.com@evil.com (url.Hostname() → evil.com, not covered). Case-/port-/trailing-dot-/IPv6-bracket-normalized. No over-broad match.
No scope confusion through render cache or session hydration. The render-cache fingerprint keys on ToolCallID + the new Scope + SideEffect, and the session round-trips Scope/SideEffect, so a cached or rehydrated card can't apply host A's grant to host B.
Nits (optional, non-blocking)
- Hostless/malformed URL falls back to a tool-wide
web_fetchgrant. Ifweb_fetchgets a URL that parses but yieldsHost == ""(e.g.file:///x),DeriveScopereturnsScopeToolWide, so "always allow" persists a tool-wide grant. This matches pre-PR behavior (and the "always for this host" label won't render without a host), so it's not a regression — but for a network tool, fail-closed (re-prompt) would be safer than persisting tool-wide. Worth a follow-up. - A couple of negative tests would lock in the security crux. Host scoping is well-covered for subdomains/case; I'd add the userinfo-confusion URL (
https://a@evil.comresolves toevil.comand is refused), the hostless-URL → tool-wide fallback (documents the intent), and an IPv6/punycode negative match.
Overlap with another in-flight branch
This modifies internal/agent/loop.go, internal/tools/types.go, internal/agent/types.go, and internal/tui/model.go, which a separate open branch (specialist delegation, #243) also touches. #242's edits to those files are localized (mostly doc comments + reuse), so a textual conflict is unlikely, but types.go/loop.go will want a 3-way merge — whichever lands second should re-run the sandbox/agent/tui -race suite. No semantic conflict; the two touch different concerns.
Tooling
go build ./..., go vet ./..., and go test ./internal/sandbox/... ./internal/tools/... ./internal/agent/... ./internal/tui/... -race all pass; staticcheck is clean for the files this PR touches (the few findings are pre-existing in untouched files).
Careful, well-tested change — approving.
3 participants
Summary
Tests
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation