From 8f6f92d901c72da0baebec3689ce33d3408bcf08 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:26:14 -0700 Subject: [PATCH] fix(browser): one loopback proxy-bypass helper covers child envs and in-process CDP dials Move the loopback NO_PROXY merge from browser_tool into agent/proxy_bypass.py (the module that already owns NO_PROXY semantics) and reuse no_proxy_entries() so comma- and whitespace-separated operator values are both preserved. Add loopback_connect_kwargs() and pass proxy=None on the two in-process websockets dials to loopback CDP endpoints (browser_cdp_tool._cdp_call, BrowserSupervisor._run): those never see the child env, so the env merge alone left them routed through a macOS system proxy. Remote CDP URLs keep the default proxy behaviour. Tests trimmed to two invariants: the built child env appends loopback to an operator NO_PROXY in both casings, and only loopback URLs get proxy=None. Sibling helper in tools/browser_use_cli (#110570) is redundant once the shared env carries the entries. --- agent/proxy_bypass.py | 33 ++++++ .../test_browser_env_loopback_no_proxy.py | 109 +++++------------- tools/browser_cdp_tool.py | 3 +- tools/browser_supervisor.py | 4 +- tools/browser_tool.py | 25 +--- 5 files changed, 71 insertions(+), 103 deletions(-) diff --git a/agent/proxy_bypass.py b/agent/proxy_bypass.py index 64cc22bd22..4ab7c56632 100644 --- a/agent/proxy_bypass.py +++ b/agent/proxy_bypass.py @@ -58,6 +58,39 @@ def no_proxy_entries(no_proxy_value: str | None = None) -> list[str]: return [part for part in re.split(r"[\s,]+", no_proxy_value.strip()) if part] +# Loopback must never be dialed through a proxy. ``websockets>=14`` connects with +# ``proxy=True`` and resolves it via ``urllib.request.getproxies()`` — on macOS that reads the +# *system* proxy (``_scproxy``) even with no ``*_proxy`` env vars — so a local CDP endpoint +# (``ws://127.0.0.1:/devtools/...``) is dialed through the proxy and the handshake dies +# with "did not receive a valid HTTP response" (#110565). ``urllib``'s bypass check honours +# NO_PROXY in both casings, so children get the entries appended; in-process dials pass +# ``proxy=None`` when the host is loopback. +LOOPBACK_HOSTS = ("127.0.0.1", "localhost", "::1") + + +def is_loopback_host(host: str | None) -> bool: + """True for a host that must always bypass a proxy (loopback literal or ``localhost``).""" + return str(host or "").strip().lower().strip("[]") in LOOPBACK_HOSTS + + +def loopback_connect_kwargs(url: str) -> dict: + """``websockets.connect`` kwargs for an in-process dial: ``{"proxy": None}`` when ``url`` + targets loopback (skip the library's system-proxy auto-detection), else ``{}`` so remote + endpoints keep the default proxy behaviour.""" + return {"proxy": None} if is_loopback_host(split_host_port(url)[0]) else {} + + +def add_loopback_no_proxy(env: dict) -> dict: + """Append the loopback hosts to ``NO_PROXY`` / ``no_proxy`` in ``env`` (both casings), + keeping every operator-provided entry; returns ``env``.""" + for key in ("NO_PROXY", "no_proxy"): + entries = no_proxy_entries(env.get(key) or "") + missing = [host for host in LOOPBACK_HOSTS if host not in entries] + if missing: + env[key] = ",".join(entries + missing) + return env + + def _ip_or_none(value: str, parse=ipaddress.ip_address): """``parse(value)`` or None on ``ValueError`` (``parse`` is ip_address / ip_network).""" try: diff --git a/tests/tools/test_browser_env_loopback_no_proxy.py b/tests/tools/test_browser_env_loopback_no_proxy.py index a05777c0da..6a7710051d 100644 --- a/tests/tools/test_browser_env_loopback_no_proxy.py +++ b/tests/tools/test_browser_env_loopback_no_proxy.py @@ -1,93 +1,40 @@ -"""Browser child envs must bypass proxies for loopback hosts (#110565). +"""Loopback CDP dials must never go through a proxy (#110565). -websockets>=14 defaults to ``proxy=True`` and resolves proxies via -``urllib.request.getproxies()``, which reads the macOS/Windows *system* proxy -config even with no ``*_proxy`` env vars set — so a local CDP WebSocket -(``ws://127.0.0.1:/devtools/...``) gets routed into the system proxy and -the handshake fails with "did not receive a valid HTTP response". -``_build_browser_env`` therefore appends the loopback hosts to NO_PROXY/no_proxy -for every browser subprocess (append, never overwrite operator entries). +``websockets>=14`` connects with ``proxy=True`` and resolves the proxy via +``urllib.request.getproxies()``, which on macOS reads the *system* proxy even with no +``*_proxy`` env vars. The browser child env therefore carries loopback ``NO_PROXY`` entries +(both casings, operator entries kept), and the in-process CDP dials pass ``proxy=None`` +for loopback URLs only. """ import pytest import tools.browser_tool as bt +from agent.proxy_bypass import loopback_connect_kwargs -class TestEnsureLoopbackNoProxy: - def test_empty_env_sets_both_casings(self): - env = {} - bt._ensure_loopback_no_proxy(env) - assert env["NO_PROXY"] == "127.0.0.1,localhost,::1" - assert env["no_proxy"] == "127.0.0.1,localhost,::1" - - def test_appends_without_dropping_operator_entries(self): - env = {"NO_PROXY": "corp.example.com,.internal"} - bt._ensure_loopback_no_proxy(env) - assert env["NO_PROXY"] == "corp.example.com,.internal,127.0.0.1,localhost,::1" - - def test_lowercase_value_appended_too(self): - env = {"no_proxy": "10.0.0.0/8"} - bt._ensure_loopback_no_proxy(env) - assert env["no_proxy"] == "10.0.0.0/8,127.0.0.1,localhost,::1" - - def test_idempotent_when_loopback_already_present(self): - env = {"NO_PROXY": "127.0.0.1,localhost,::1"} - bt._ensure_loopback_no_proxy(env) - assert env["NO_PROXY"] == "127.0.0.1,localhost,::1" - - def test_partial_overlap_only_adds_missing(self): - env = {"NO_PROXY": "localhost"} - bt._ensure_loopback_no_proxy(env) - assert env["NO_PROXY"] == "localhost,127.0.0.1,::1" - - def test_unrelated_keys_untouched(self): - env = {"PATH": "/usr/bin", "NO_PROXY": "x.example"} - bt._ensure_loopback_no_proxy(env) - assert env["PATH"] == "/usr/bin" - assert set(env) == {"PATH", "NO_PROXY", "no_proxy"} +@pytest.fixture +def stub_sanitized_env(monkeypatch): + """Replace the credential-scrub layer with a fixed dict so the test sees exactly what + ``_build_browser_env`` adds on top.""" + import tools.environments.local as local + holder = {} + monkeypatch.setattr(local, "hermes_subprocess_env", lambda inherit_credentials=False: dict(holder)) + return holder -class TestBuildBrowserEnvLoopback: - @pytest.fixture - def stub_sanitized_env(self, monkeypatch): - """Replace the credential-scrub layer with a fixed dict so the test sees - exactly what _build_browser_env adds on top.""" - import tools.environments.local as local - holder = {} +def test_browser_env_appends_loopback_to_operator_no_proxy(stub_sanitized_env): + stub_sanitized_env["NO_PROXY"] = "git.internal, 10.0.0.0/8" + env = bt._build_browser_env() + assert env["NO_PROXY"] == "git.internal,10.0.0.0/8,127.0.0.1,localhost,::1" + assert env["no_proxy"] == "127.0.0.1,localhost,::1" - def _fake(inherit_credentials=False): - return dict(holder) - monkeypatch.setattr(local, "hermes_subprocess_env", _fake) - return holder - - def test_sets_loopback_no_proxy_when_scrubbed_env_has_none(self, stub_sanitized_env, monkeypatch): - for key in ("NO_PROXY", "no_proxy"): - monkeypatch.delenv(key, raising=False) - monkeypatch.delenv("BROWSER_USE_API_KEY", raising=False) - - env = bt._build_browser_env() - - assert env["NO_PROXY"] == "127.0.0.1,localhost,::1" - assert env["no_proxy"] == "127.0.0.1,localhost,::1" - - def test_keeps_operator_loopback_entries(self, stub_sanitized_env, monkeypatch): - monkeypatch.delenv("NO_PROXY", raising=False) - monkeypatch.delenv("no_proxy", raising=False) - stub_sanitized_env["NO_PROXY"] = "git.internal" - monkeypatch.delenv("BROWSER_USE_API_KEY", raising=False) - - env = bt._build_browser_env() - - assert env["NO_PROXY"] == "git.internal,127.0.0.1,localhost,::1" - - def test_passthrough_keys_still_readded(self, stub_sanitized_env, monkeypatch): - monkeypatch.delenv("NO_PROXY", raising=False) - monkeypatch.delenv("no_proxy", raising=False) - monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") - - env = bt._build_browser_env() - - assert env["BROWSER_USE_API_KEY"] == "test-key" - assert env["NO_PROXY"] == "127.0.0.1,localhost,::1" +@pytest.mark.parametrize("url, expected", [ + ("ws://127.0.0.1:9222/devtools/browser/abc", {"proxy": None}), + ("ws://localhost:9222/devtools/browser/abc", {"proxy": None}), + ("ws://[::1]:9222/devtools/browser/abc", {"proxy": None}), + ("wss://connect.browserbase.com/cdp?apiKey=x", {}), +]) +def test_in_process_cdp_dial_disables_proxy_only_for_loopback(url, expected): + assert loopback_connect_kwargs(url) == expected diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index 8b4efb8bed..0def5a16dd 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -169,10 +169,11 @@ async def _cdp_call(ws_url: str, method: str, params: Dict[str, Any], target_id: """Make a single CDP call. With ``target_id``, ``Target.attachToTarget(flatten=True)`` multiplexes a page-level session over the browser-level WebSocket; without it ``method`` runs at browser level.""" assert websockets is not None # guarded by _WS_AVAILABLE at call-site + from agent.proxy_bypass import loopback_connect_kwargs # max_size=None: CDP responses (e.g. DOM.getDocument) can be large; ping_interval=None: CDP # servers don't expect pings. async with websockets.connect(ws_url, max_size=None, open_timeout=timeout, close_timeout=5, - ping_interval=None) as ws: + ping_interval=None, **loopback_connect_kwargs(ws_url)) as ws: next_id = 1 async def _send(req: Dict[str, Any], what: str) -> Dict[str, Any]: diff --git a/tools/browser_supervisor.py b/tools/browser_supervisor.py index db157ef0a1..5a35cf59b6 100644 --- a/tools/browser_supervisor.py +++ b/tools/browser_supervisor.py @@ -353,9 +353,11 @@ class CDPSupervisor(DialogSupervisionMixin, FrameTrackingMixin): A failure before the first successful attach is fatal for ``start()``.""" attempt, last_success_at, backoff = 0, 0.0, 0.5 import websockets # deferred: only supervisors that connect pay the import + from agent.proxy_bypass import loopback_connect_kwargs + connect_kwargs = {"max_size": 50 * 1024 * 1024, **loopback_connect_kwargs(self.cdp_url)} while not self._stop_requested: try: - self._ws = await asyncio.wait_for(websockets.connect(self.cdp_url, max_size=50 * 1024 * 1024), timeout=10.0) + self._ws = await asyncio.wait_for(websockets.connect(self.cdp_url, **connect_kwargs), timeout=10.0) except Exception as e: attempt += 1 if self._fail_start(e): diff --git a/tools/browser_tool.py b/tools/browser_tool.py index cc9c639343..d6a7cb4a15 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -34,24 +34,6 @@ _BROWSER_PASSTHROUGH_KEYS: tuple[str, ...] = ( "FIRECRAWL_API_KEY", "FIRECRAWL_API_URL", "FIRECRAWL_BROWSER_TTL", ) -# Loopback hosts that must never be proxied: the browser backends dial local CDP -# endpoints (ws://127.0.0.1:/devtools/...), and websockets>=14 defaults to -# proxy=True with proxies resolved via urllib.request.getproxies() — which reads the -# macOS/Windows *system* proxy config even with no *_proxy env vars set. Without an -# explicit NO_PROXY the CDP handshake is routed into the system proxy and fails with -# "did not receive a valid HTTP response" (#110565). See #14372 for the env-var flavor. -_LOOPBACK_NO_PROXY_ENTRIES: tuple[str, ...] = ("127.0.0.1", "localhost", "::1") - - -def _ensure_loopback_no_proxy(env: dict) -> None: - """Append the loopback hosts to NO_PROXY/no_proxy (both casings) without dropping - operator-provided entries.""" - for key in ("NO_PROXY", "no_proxy"): - entries = [part.strip() for part in env.get(key, "").split(",") if part.strip()] - missing = [host for host in _LOOPBACK_NO_PROXY_ENTRIES if host not in entries] - if missing: - env[key] = ",".join(entries + missing) - def _build_browser_env() -> dict: """Credential-scrubbed env for an agent-browser subprocess (deferred import: test @@ -61,6 +43,8 @@ def _build_browser_env() -> dict: from agent.secret_scope import UnscopedSecretError, get_secret from tools.environments.local import served_profile_child_env + from agent.proxy_bypass import add_loopback_no_proxy + env = served_profile_child_env(inherit_credentials=False) for key in _BROWSER_PASSTHROUGH_KEYS: try: @@ -69,8 +53,9 @@ def _build_browser_env() -> dict: value = None # multiplex, no scope bound: no key rather than a sibling profile's if value is not None: env[key] = value - _ensure_loopback_no_proxy(env) - return env + # The Browser Use harness dials the resolved local CDP URL over ``websockets``; without a + # loopback NO_PROXY a macOS system proxy captures that dial (#110565). + return add_loopback_no_proxy(env) try: