fix(web): share SSRF guard and isolate web.run sessions

[Hmbown]

Summary

• route fetch_url and web.run page fetches through one DNS-pinned SSRF guard
• disable ambient HTTP/HTTPS/SOCKS proxies on guarded transports so a proxy cannot re-resolve a validated hostname
• keep loopback, private, link-local, metadata, CGNAT, and IPv6 ULA fail-closed; trusted proxy hosts are useful only when the resolved address is also inside the explicit 198.18.0.0/15 fake-IP placeholder range
• isolate cached web.run pages by state namespace and reject exact foreign-session refs for both open and click
• guard real click targets and every redirect hop, including public-to-private redirects
• share DuckDuckGo/Bing scraping between web_search and web.run, filtering the known spam family without dropping legitimate same-domain, .co.uk, github.io, or small result sets

This is the repaired W1 slice. It closes the proxy-rebinding, trusted-host private-address, cache namespace, redirect/click, and broad same-root heuristic findings from security review.

Security verification after merging current main

• shared guard: 26 passed, 1 subprocess helper ignored
• web.run: 22 passed
• shared scraper: 16 passed
• fetch_url: 10 passed
• network policy: 23 passed
• config: 414 passed, 1 doc test passed
• cargo fmt --all -- --check
• cargo clippy -p codewhale-config -p codewhale-tui --all-targets --all-features --locked -- -D warnings
• git diff --check origin/main...HEAD

The complete locked workspace/all-features suite also passed before the initial push (TUI 7480 passed / 3 ignored; PTY 16 passed / 1 ignored; integration and doc tests green). Exact-head CI is required before merge.

Current exact head: ab87b0eb645973f161356f37dcd41d12bc9a36d4.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 2m 56s —— View job

---

Claude finished the review

• Gather context (PR body, diff stat, files changed)
• Read CLAUDE.md / AGENTS.md guidance
• Review crates/tui/src/tools/web/guard.rs (new SSRF guard)
• Review crates/tui/src/tools/web/scrape.rs (shared scraper)
• Review crates/tui/src/tools/web_run.rs changes
• Review crates/tui/src/tools/fetch_url.rs + web_search.rs changes
• Review network_policy.rs / config changes
• Check CI status
• Post final review

Summary

This is a solid, well-tested security fix. I read the full diff against origin/main (not just the description) and didn't find a correctness bug or a bypass of the stated invariants. Highlights:

• crates/tui/src/tools/web/guard.rs: validate_fetch_target correctly normalizes bracketed IPv6 literals before the restricted-IP check (closing the GHSA-88gh-2526-gfrr class of bug), validates every resolved address (not just the first) before pinning, and guarded_reqwest_client_builder() calls .no_proxy() so an ambient HTTP_PROXY/HTTPS_PROXY/ALL_PROXY can't re-resolve a validated hostname outside this process — verified with a genuine subprocess probe test (guarded_transport_bypasses_ambient_http_https_and_remote_dns_socks_proxies), not just a mock.
• Trusted-proxy fake-IP exemption (network_policy.rs:288-295, guard.rs:172-178): correctly requires both an explicitly trusted host (proxy) and an explicitly trusted CIDR (proxy_fake_ip_cidrs), and parse_trusted_fakeip_cidr hard-restricts configured CIDRs to 198.18.0.0/15 regardless of what an operator puts in config — so a misconfigured proxy_fake_ip_cidrs = ["10.0.0.0/8"] can never punch a hole in the private-IP block (configured_fakeip_cidrs_are_loaded_but_unsafe_ranges_are_ignored covers this).
• Redirect handling (fetch_url.rs, web_run.rs::fetch_page_with_initial_pin): every redirect hop re-runs validate_fetch_target and gets a fresh pinned client — open_refuses_redirect_from_public_host_to_private_ip exercises a real public→private redirect through a mock server, not just a unit assertion.
• Session/namespace isolation (web_run.rs): get_page rejects cross-namespace ref lookups, and execute_open_rejects_exact_foreign_session_ref / execute_click_rejects_exact_foreign_session_ref cover both open and click. Click targets are routed through the same guarded fetch as open (execute_click_routes_target_through_shared_ssrf_guard).
• Scraper consolidation (tools/web/scrape.rs): replacing the old "≥60% same-root-domain ⇒ spam" heuristic with an explicit known-spam-domain list is the right call — the old heuristic had real false-positive risk against legitimate site-scoped/docs searches, which the new legitimate_same_domain_results_are_preserved / public_suffix_and_private_suffix_hosts_are_not_misgrouped tests now pin down. I confirmed no leftover references to the removed normalize_url/is_likely_spam_results/root_domain functions in web_search.rs.
• Config plumbing (NetworkPolicyToml in both crates/config and crates/tui, config.example.toml) is consistent and mechanically threaded through with matching tests.

Minor observations (non-blocking)

• is_restricted_ip (guard.rs:29) doesn't special-case 0.0.0.0/8 (other than the exact 0.0.0.0 unspecified address) or the deprecated IPv4-compatible IPv6 form (::a.b.c.d, distinct from the handled IPv4-mapped ::ffff:a.b.c.d). Neither is a realistic SSRF vector on typical stacks/hyper today, so this is a hardening nit, not a finding.
• run_search/run_bing_search/run_image_search in web_run.rs build their client via the plain crate::tls::reqwest_client_builder() rather than the guarded builder — that's fine since the target URLs are hardcoded DuckDuckGo/Bing endpoints, not attacker-influenced, but worth a one-line comment noting that's intentional (why these three don't need validate_fetch_target) so a future refactor doesn't "fix" it into a redundant guard call or, worse, assume it's already guarded.

Verification note

I don't have permission to run cargo build/cargo test/cargo clippy in this sandboxed review environment (Bash tool calls to cargo require approval that isn't available here) — if you want me to actually execute the security-lane suites the PR body claims (26/1/22/16/10/23/409 passed), please update my --allowedTools to permit cargo invocations. I did verify by static review that the removed functions in web_search.rs have no dangling references and that struct field additions (proxy_fake_ip_cidrs) are threaded consistently everywhere they're constructed.

CI on this PR was still running at review time (CI and web.run review-check in progress; DCO, Web Frontend, Contribution gate already green, 0 failures so far).

[claude]

Claude finished @Hmbown's task in 1m 8s —— View job

---

Claude re-review (synchronize event)

• Gather context (PR body, diff, prior review)
• Diff origin/main...HEAD to see what changed since the last review
• Re-review guard.rs, web_run.rs, scrape.rs, network_policy.rs for new issues
• Check CI status
• Post final review

Summary

No new review needed on the security-relevant code — this push is a merge of origin/main only, not a content change to the W1 SSRF slice.

I diffed the previously-reviewed head (00f9a4c) against the current head (ab87b0e) directly:

git diff 00f9a4c...ab87b0e --stat

The only files touched are unrelated route/model-routing files pulled in from main (PR #4562's route-candidate immutability work: crates/config/src/route/*, crates/tui/src/route_runtime.rs, model_routing.rs, client.rs, fleet/worker_runtime.rs, etc.). None of guard.rs, scrape.rs, web_run.rs, fetch_url.rs, web_search.rs, or network_policy.rs appear in that diff — the SSRF guard content is byte-identical to what I already reviewed in the prior pass.

The one touch point worth double-checking was the merge conflict resolution in crates/tui/src/main.rs, since both PRs modified nearby code:

• active_route_limits = crate::route_budget::known_route_limits(validated_route.candidate.limits()) — updated from a field access to the new RouteCandidate::limits() accessor method introduced by feat(route): make ReadyRouteCandidate immutable with sourced limit overrides #4562 (crates/config/src/route/candidate.rs:294). Confirmed the method exists with the expected signature — this is a correct, mechanical merge resolution, not new logic.
• The proxy_fake_ip_cidrs: Vec::new() test-fixture field from this PR is still present and correctly threaded into the merged main.rs test struct literal.

My prior full review (posted earlier on this PR) still stands — no correctness bugs, no bypass of the SSRF invariants, redirect/session isolation and trusted-proxy fake-IP restriction all hold. Only the two non-blocking nits noted previously remain (missing 0.0.0.0/8 / deprecated IPv4-compatible IPv6 special-casing in is_restricted_ip, and the unguarded-but-hardcoded-URL DuckDuckGo/Bing client builders in web_run.rs search helpers).

CI status

CI was still in progress at review time: CI and this review check in progress; DCO and Web Frontend already green, 0 failures so far.
·