fix(browser): extend private-network guard to browser_vision

[liuhao1024]

What does this PR do?

Extends the private-network SSRF guard (from #44755) to browser_vision() in addition to browser_snapshot(). After browser_console() navigates to a private/internal URL via JavaScript eval, browser_vision() would capture and forward the private page content to the vision model without checking the current URL. This PR adds the same re-check of window.location.href before any screenshot is taken.

This addresses the CHANGES_REQUESTED review by @egilewski on #44755, which found that browser_vision() remained as a bypass vector for the eval-navigation SSRF attack.

Related Issue

Fixes #44731 (extends #44755)

Supersedes #44755

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• tools/browser_tool.py: Add private-network URL safety guard to browser_vision() before any screenshot is captured. Checks window.location.href via CDP eval and blocks if the URL targets a private/internal address. Guard is skipped for local backends and when allow_private_urls is enabled. Fails open if the eval check itself fails.
• tests/tools/test_browser_snapshot_ssrf.py: Add 7 tests for browser_vision() SSRF guard — blocks private URL, allows public URL, skips in local backend, skips when private URLs allowed, handles eval failure/empty/exception gracefully.

How to Test

1. python -B -m pytest -q tests/tools/test_browser_snapshot_ssrf.py -p no:cacheprovider
2. Verify all 15 tests pass (8 existing snapshot tests + 7 new vision tests)
3. The blocking test confirms browser_vision() returns {"success": false, "error": "Blocked: page URL targets a private or internal address..."} when the page has been navigated to http://127.0.0.1:8080/secret via eval.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

⚠️ GitNexus unavailable — grep-based fallback used.

• Checked: _is_safe_url, _is_local_backend, _allow_private_urls, _run_browser_command (all defined in tools/browser_tool.py, 21 references)
• Blast radius: LOW — guard only adds a pre-check; no existing behavior changed
• Related patterns: Same guard pattern as browser_snapshot() SSRF fix (line 2533)

[egilewski]

Recommendation: request changes

I reviewed this in security mode against current GitHub main 46d758bb3e0709bef51b7e3416cfb25da95d2335, PR base 46d758bb3e0709bef51b7e3416cfb25da95d2335, and PR head d1ffb138e354d71536e02551b55b290a7ec62826.

Validation:

• git rev-list --left-right --count upstream/main...refs/remotes/upstream/pr/45101: 30 2.
• git merge-tree --write-tree upstream/main refs/remotes/upstream/pr/45101: passed, tree 741221bb668648177721fdc3604abc1a3d9e2a49.
• git diff --check upstream/main...refs/remotes/upstream/pr/45101: passed.
• python -B -m pytest -q tests/tools/test_browser_snapshot_ssrf.py tests/tools/test_browser_ssrf_local.py tests/tools/test_browser_eval_supervisor_path.py -p no:cacheprovider: passed, 52 passed.
• python -B -m py_compile tools/browser_tool.py tests/tools/test_browser_snapshot_ssrf.py: passed.

Finding:
This still needs rework because the new current-URL guards block the existing hybrid local-sidecar path for ordinary private URLs. Current browser_navigate() intentionally skips the full private-IP block when the navigation is served by a local sidecar, and the existing test says hybrid routing for ordinary private URLs “must be narrow enough to not break the PR #16136 feature” (tests/tools/test_browser_ssrf_local.py). With hybrid routing forced to review::local, current main allows browser_navigate("http://192.168.1.1/admin") and a follow-up browser_snapshot() returns the local sidecar page. On this PR head, the same navigation still succeeds, but the follow-up browser_snapshot() is rejected by the new guard:

{"success": false, "error": "Blocked: page URL targets a private or internal address (http://192.168.1.1/admin)..."}.

The same issue affects browser_vision(): with _last_active_session_key["review"] = "review::local", the PR-head guard calls eval on the sidecar session and blocks before screenshot capture. The guard needs to distinguish the eval-mutated cloud session from the legitimate _is_local_sidecar_key(effective_task_id) case, while preserving the existing always-blocked cloud-metadata floor.

I stopped after this blocker.

Signed: GPT-5.5-xhigh in Codex

[liuhao1024]

Superseded by #45133 which addresses the review feedback from @egilewski.

The fix adds _is_local_sidecar_key(effective_task_id) check to both browser_snapshot() and browser_vision() guards, matching the existing pattern in browser_navigate(). This allows local sidecar sessions (hybrid routing) to access private URLs without being blocked.

All 17 SSRF tests pass, including the new test_skips_check_for_local_sidecar_session tests.

[teknium1]

Superseded by #45133, which was merged into current main via PR #54132 (your authorship preserved). The final fix also closes the eval return-value path. Thanks!