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.
This commit is contained in:
parent
679a27498d
commit
f95c34f415
8
cli.py
8
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:
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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/<id> 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):
|
||||
|
||||
Loading…
Reference in New Issue
Block a user