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.
This commit is contained in:
@@ -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:<port>/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:
|
||||
|
||||
@@ -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:<port>/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
|
||||
|
||||
@@ -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]:
|
||||
|
||||
@@ -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):
|
||||
|
||||
+5
-20
@@ -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:<port>/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:
|
||||
|
||||
Reference in New Issue
Block a user