diff --git a/tests/tools/test_browser_cdp_override.py b/tests/tools/test_browser_cdp_override.py index 79b379af241..6f8893b1629 100644 --- a/tests/tools/test_browser_cdp_override.py +++ b/tests/tools/test_browser_cdp_override.py @@ -162,6 +162,30 @@ class TestGetCdpOverride: assert resolved == WS_URL mock_get.assert_called_once_with(VERSION_URL, timeout=10) + def test_camofox_yields_to_config_cdp_override(self, monkeypatch): + """CAMOFOX_URL + a persistent browser.cdp_url config override must NOT + report camofox mode: the CDP browser takes precedence so navigation is + not routed through Camofox, and the CDP backend stays non-local for SSRF + checks. Regression for the env-only suppression gap (config CDP was + ignored, so CAMOFOX_URL + config CDP still dispatched to Camofox).""" + import tools.browser_camofox as bc + + monkeypatch.setenv("CAMOFOX_URL", "http://localhost:9377") + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + + # No CDP anywhere -> camofox mode is on. + with patch("hermes_cli.config.read_raw_config", return_value={}): + assert bc.is_camofox_mode() is True + + # A config-only CDP override suppresses camofox. + with patch("hermes_cli.config.read_raw_config", + return_value={"browser": {"cdp_url": HTTP_URL}}): + assert bc.is_camofox_mode() is False + + # The env override still suppresses camofox. + monkeypatch.setenv("BROWSER_CDP_URL", HTTP_URL) + with patch("hermes_cli.config.read_raw_config", return_value={}): + assert bc.is_camofox_mode() is False class TestCreateCdpSession: """_create_cdp_session() must sanitize the CDP URL before logging. diff --git a/tools/browser_camofox.py b/tools/browser_camofox.py index 006c5249a50..9d4c8c9bc96 100644 --- a/tools/browser_camofox.py +++ b/tools/browser_camofox.py @@ -93,16 +93,40 @@ def get_camofox_url() -> str: return os.getenv("CAMOFOX_URL", "").rstrip("/") +def _config_cdp_url() -> str: + """Persistent ``browser.cdp_url`` from config.yaml, or empty string. + + Read here (instead of importing ``browser_tool._get_cdp_override`` to avoid + a circular import) so Camofox can yield to a config-based CDP override the + same way it already yields to the ``BROWSER_CDP_URL`` env override. + """ + try: + from hermes_cli.config import read_raw_config + + browser_cfg = read_raw_config().get("browser", {}) + if isinstance(browser_cfg, dict): + return str(browser_cfg.get("cdp_url", "") or "").strip() + except Exception: + pass + return "" + + def is_camofox_mode() -> bool: """True when Camofox backend is configured and no CDP override is active. - When the user has explicitly connected to a live Chromium-family browser via - ``/browser connect`` (which sets ``BROWSER_CDP_URL``), the CDP connection - takes priority over Camofox so the browser tools operate on the real - browser instead of being silently routed to the Camofox backend. + A CDP override takes priority over Camofox so the browser tools operate on + the real CDP browser (and a CDP backend is treated as non-local for SSRF + checks) instead of being silently routed to Camofox. The override may come + from the ``BROWSER_CDP_URL`` env var (set by ``/browser connect``) OR a + persistent ``browser.cdp_url`` in config.yaml — both are honored, matching + ``browser_tool._get_cdp_override()``'s precedence. (Previously only the env + var suppressed Camofox, so ``CAMOFOX_URL`` + a config CDP override still + routed navigation through Camofox.) """ if os.getenv("BROWSER_CDP_URL", "").strip(): return False + if _config_cdp_url(): + return False return bool(get_camofox_url()) diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 33854687de1..512716299b4 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -800,6 +800,20 @@ def _is_local_backend() -> bool: that the terminal cannot. In this case, SSRF protection should be enabled even though the browser is technically "local". """ + # A CDP override points the browser at a separate Chrome process whose + # network position is not guaranteed to match the terminal (it may live + # off-host). Don't treat it as a trusted local backend — otherwise a + # model-driven navigate could reach internal/metadata services reachable + # from the CDP host but not the terminal. This MUST be checked before the + # camofox short-circuit below so a Camofox backend combined with a CDP + # override still fails the local check instead of returning local and + # skipping the private/internal SSRF gate. The override is honored from + # either the BROWSER_CDP_URL env var or a persistent `browser.cdp_url` + # config (both via _get_cdp_override(), and both now suppress camofox in + # browser_camofox.py). _is_local_mode() already treats any CDP override as + # non-local; keep the two helpers in agreement. + if _get_cdp_override(): + return False if _is_camofox_mode(): return True if _get_cloud_provider() is not None: @@ -2740,7 +2754,10 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: # 169.254.169.254 / metadata.google.internal / ECS task metadata # via a browser, and routing those to a local Chromium sidecar # on an EC2/GCP/Azure host exfiltrates IAM credentials (#16234). - if not _is_local_backend() and _is_always_blocked_url(url): + # The floor is UNCONDITIONAL — it must fire for every backend, + # including the pure-local headless Chromium and off-host CDP cases + # (a local Chromium on a cloud VM still reaches the host IMDS). + if _is_always_blocked_url(url): return json.dumps({ "success": False, "error": "Blocked: URL targets a cloud metadata endpoint", @@ -2808,12 +2825,11 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: # Skipped for local backends (same rationale as the pre-nav check), # and for the hybrid local sidecar (we're already on a local browser # hitting a private URL by design). - # Always-blocked floor (cloud metadata / IMDS) is enforced even - # when auto_local_this_nav is true — see pre-nav check for - # rationale (#16234). + # Always-blocked floor (cloud metadata / IMDS) is enforced for every + # backend and even when auto_local_this_nav is true — see pre-nav + # check for rationale (#16234). if ( - not _is_local_backend() - and final_url + final_url and final_url != url and _is_always_blocked_url(final_url) ):