From f95c34f41510b494d0fceeb97a844675a2635423 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 28 Apr 2026 23:13:29 -0500 Subject: [PATCH] fix(browser): address Copilot round-4 on /browser connect * Reject unsupported schemes (anything outside http/https/ws/wss) in cli.py /browser connect before probing or persisting, matching the gateway's existing 4015 path. * Defend gateway browser.manage against `{"url": null}` and non-string urls: empty/null falls back to DEFAULT_BROWSER_CDP_URL, non-string returns a 4015 instead of slipping into the generic 5031 catch via TypeError on `"://" in url`. * Add regression tests for both null-url fallback and non-string rejection. --- cli.py | 8 +++++++ tests/test_tui_gateway_server.py | 38 ++++++++++++++++++++++++++++++++ tui_gateway/server.py | 18 +++++++++++---- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/cli.py b/cli.py index d0bf6e79..4f938789 100644 --- a/cli.py +++ b/cli.py @@ -6569,6 +6569,14 @@ class HermesCLI: connect_parts = cmd.strip().split(None, 2) # ["/browser", "connect", "ws://..."] cdp_url = connect_parts[2].strip() if len(connect_parts) > 2 else _DEFAULT_CDP parsed_cdp = urlparse(cdp_url if "://" in cdp_url else f"http://{cdp_url}") + if parsed_cdp.scheme not in {"http", "https", "ws", "wss"}: + print() + print( + f" ⚠ Unsupported browser url scheme: {parsed_cdp.scheme or '(missing)'} " + "(expected one of: http, https, ws, wss)" + ) + print() + return try: _port = parsed_cdp.port or (443 if parsed_cdp.scheme in {"https", "wss"} else 80) except ValueError: diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 25e4d410..1c8eecf0 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -2995,6 +2995,44 @@ def test_browser_manage_connect_no_session_skips_progress_events(monkeypatch): assert [evt for evt, _ in emitted if evt == "browser.progress"] == [] +def test_browser_manage_connect_handles_null_url(monkeypatch): + """Explicit ``{"url": null}`` (or empty string) must fall back to the + default loopback URL instead of raising a TypeError that gets swallowed + by the outer 5031 catch.""" + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + fake = types.SimpleNamespace( + cleanup_all_browsers=lambda: None, + _get_cdp_override=lambda: os.environ.get("BROWSER_CDP_URL", ""), + ) + with patch.dict(sys.modules, {"tools.browser_tool": fake}): + _stub_urlopen(monkeypatch, ok=True) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": None}, + } + ) + + assert resp["result"]["connected"] is True + assert resp["result"]["url"] == "http://127.0.0.1:9222" + + +def test_browser_manage_connect_rejects_non_string_url(monkeypatch): + monkeypatch.delenv("BROWSER_CDP_URL", raising=False) + resp = server.handle_request( + { + "id": "1", + "method": "browser.manage", + "params": {"action": "connect", "url": 9222}, + } + ) + + assert resp["error"]["code"] == 4015 + assert "must be a string" in resp["error"]["message"] + assert "BROWSER_CDP_URL" not in os.environ + + def test_browser_manage_connect_default_local_retries_after_launch(monkeypatch): monkeypatch.delenv("BROWSER_CDP_URL", raising=False) monkeypatch.setattr(server.time, "sleep", lambda _seconds: None) diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 8ee81bdd..e5fda15a 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -4843,7 +4843,11 @@ def _browser_connect(rid, params: dict) -> dict: from tools.browser_tool import cleanup_all_browsers from urllib.parse import urlparse - url = params.get("url", DEFAULT_BROWSER_CDP_URL) + raw_url = params.get("url") + if raw_url is not None and not isinstance(raw_url, str): + return _err(rid, 4015, f"browser url must be a string, got {type(raw_url).__name__}") + url = (raw_url or "").strip() or DEFAULT_BROWSER_CDP_URL + sid = params.get("session_id") or "" system = platform.system() messages: list[str] = [] @@ -4877,7 +4881,9 @@ def _browser_connect(rid, params: dict) -> dict: # ws[s]://.../devtools/browser/ endpoints (hosted CDP # providers) don't serve the HTTP discovery path; just check # TCP-level reachability and let browser_navigate handshake. - if parsed.scheme in {"ws", "wss"} and parsed.path.startswith("/devtools/browser/"): + if parsed.scheme in {"ws", "wss"} and parsed.path.startswith( + "/devtools/browser/" + ): import socket try: @@ -4892,7 +4898,9 @@ def _browser_connect(rid, params: dict) -> dict: if not ok and _is_default_local_cdp(parsed): from hermes_cli.browser_connect import try_launch_chrome_debug - announce("Chrome isn't running with remote debugging — attempting to launch...") + announce( + "Chrome isn't running with remote debugging — attempting to launch..." + ) if try_launch_chrome_debug(port, system): for _ in range(20): @@ -4906,7 +4914,9 @@ def _browser_connect(rid, params: dict) -> dict: else: for line in _failure_messages(url, port, system)[1:]: announce(line, level="error") - return _ok(rid, {"connected": False, "url": url, "messages": messages}) + return _ok( + rid, {"connected": False, "url": url, "messages": messages} + ) elif not ok: return _err(rid, 5031, f"could not reach browser CDP at {url}") elif _is_default_local_cdp(parsed):