diff --git a/agent/redact.py b/agent/redact.py index dc7c1957e..81512b054 100644 --- a/agent/redact.py +++ b/agent/redact.py @@ -411,10 +411,11 @@ def redact_cdp_url(value: object) -> str: that must never reach the logs. So for CDP URLs we opt INTO the two URL redactors that the global pass leaves off. - This is the single source of truth for CDP-URL log redaction. Every site - that emits a resolved CDP URL to a log or exception message -- the browser - tool's session/discovery logs and the supervisor's attach-timeout error -- - routes through here so the policy can never drift between call sites. + This is the single source of truth for redacting a CDP URL that is passed + *directly* to a log or error message. Callers that instead need to redact an + exception whose text embeds the URL (e.g. a ``websockets`` connect error) + should route that through their own error-text helper, which delegates here + -- see ``tools.browser_supervisor._redact_cdp_error_text``. """ text = redact_sensitive_text("" if value is None else str(value)) if not text: diff --git a/tests/tools/test_browser_cdp_override.py b/tests/tools/test_browser_cdp_override.py index 8787a858b..79b379af2 100644 --- a/tests/tools/test_browser_cdp_override.py +++ b/tests/tools/test_browser_cdp_override.py @@ -256,3 +256,95 @@ class TestCDPSupervisorTimeoutRedaction: assert "127.0.0.1:9222" in str(exc) else: raise AssertionError("TimeoutError was not raised") + + +class TestCDPSupervisorStartErrorRedaction: + """CDPSupervisor.start() must not leak the CDP URL via the connect-error path. + + The more common failure mode than attach-timeout: the first + websockets.connect(self.cdp_url) raises (bad URI, refused, TLS), the raw + exception is stashed as self._start_error, and start() re-raises it. Those + websockets exceptions embed the full raw cdp_url -- token and userinfo -- + in their message. start() must re-raise a REDACTED error and must not leak + the secret via the exception message or the traceback cause chain. + """ + + def _run_start_hitting_error(self, cdp_url: str, start_error: BaseException): + """Invoke start() so it takes the _start_error re-raise branch. + + start() clears _ready_event / _start_error and launches a thread, so we + can't pre-seed them. Instead we stub threading.Thread: the fake thread's + start() synchronously populates _start_error and sets the ready event, + exactly as the real supervisor loop does on a first-connect failure. + """ + import threading + from tools.browser_supervisor import CDPSupervisor + + sup = CDPSupervisor.__new__(CDPSupervisor) + sup.task_id = "test-task" + sup.cdp_url = cdp_url + sup._start_error = None + sup._stop_requested = False + sup._loop = None + sup._thread = None + sup._ready_event = threading.Event() + + def _fake_thread(*args, **kwargs): + fake = Mock() + + def _start(): + sup._start_error = start_error + sup._ready_event.set() + + fake.start.side_effect = _start + fake.is_alive.return_value = False + return fake + + with patch("threading.Thread", side_effect=_fake_thread), patch.object(sup, "stop"): + sup.start(timeout=5.0) + + def test_start_error_redacts_query_token(self): + # A realistic websockets-style error embedding the raw URL + token. + raw = "wss://cdp.example/devtools/browser/abc?token=super-secret-999" + err = ValueError(f"{raw} isn't a valid URI: hostname isn't provided") + try: + self._run_start_hitting_error(raw, err) + except Exception as exc: # noqa: BLE001 - asserting on the surface + msg = str(exc) + assert "super-secret-999" not in msg, ( + "raw token must not appear in the re-raised error message" + ) + # The raw cause must be suppressed so it can't leak via traceback. + assert exc.__cause__ is None + assert getattr(exc, "__suppress_context__", False) is True + else: + raise AssertionError("start() did not re-raise the start error") + + def test_start_error_redacts_userinfo_password(self): + raw = "wss://user:p4ssw0rd@cdp.example/devtools/browser/x" + err = ValueError(f"{raw} isn't a valid URI: hostname isn't provided") + try: + self._run_start_hitting_error(raw, err) + except Exception as exc: # noqa: BLE001 + assert "p4ssw0rd" not in str(exc) + else: + raise AssertionError("start() did not re-raise the start error") + + +class TestRedactCdpErrorText: + """The supervisor's error-text chokepoint masks credentials, keeps context.""" + + def test_masks_query_token_in_exception(self): + from tools.browser_supervisor import _redact_cdp_error_text + + err = ConnectionError("connect wss://h/x?token=leak-me failed") + out = _redact_cdp_error_text(err) + assert "leak-me" not in out + + def test_preserves_non_secret_context(self): + from tools.browser_supervisor import _redact_cdp_error_text + + err = ConnectionError("connect ws://127.0.0.1:9222/x failed: refused") + out = _redact_cdp_error_text(err) + assert "127.0.0.1:9222" in out + assert "refused" in out diff --git a/tools/browser_supervisor.py b/tools/browser_supervisor.py index 746e79cdb..bea4ef7a0 100644 --- a/tools/browser_supervisor.py +++ b/tools/browser_supervisor.py @@ -34,6 +34,25 @@ from websockets.asyncio.client import ClientConnection logger = logging.getLogger(__name__) +def _redact_cdp_error_text(exc: object) -> str: + """Redact any CDP endpoint credentials from an error's string form. + + ``websockets`` bakes the raw target URL into its exception messages + (``InvalidURI``, connection errors, TLS failures all embed the full + ``self.cdp_url`` — including a ``?token=`` query credential or + ``user:pass@`` userinfo). Every supervisor egress point that turns such an + exception into log text or a re-raised message MUST route through here so + those credentials never reach Hermes logs or tracebacks. Falls back to a + fixed sentinel if redaction itself raises, erring toward masking. + """ + try: + from agent.redact import redact_cdp_url + + return redact_cdp_url(str(exc)) + except Exception: + return "" + + # ── Config defaults ─────────────────────────────────────────────────────────── DIALOG_POLICY_MUST_RESPOND = "must_respond" @@ -353,7 +372,14 @@ class CDPSupervisor: if self._start_error is not None: err = self._start_error self.stop() - raise err + # ``err`` is a raw ``websockets`` exception whose message embeds the + # full cdp_url (token / userinfo). Re-raise a redacted RuntimeError + # and suppress the raw cause (``from None``) so no credential leaks + # via the message OR the traceback chain. Type is not load-bearing: + # the sole caller (_ensure_cdp_supervisor) only logs it. + raise RuntimeError( + f"CDP supervisor failed to start: {_redact_cdp_error_text(err)}" + ) from None def stop(self, timeout: float = 5.0) -> None: """Cancel the supervisor task and join the thread.""" @@ -631,7 +657,7 @@ class CDPSupervisor: return logger.warning( "CDP supervisor %s: connect failed (attempt %s): %s", - self.task_id, attempt, e, + self.task_id, attempt, _redact_cdp_error_text(e), ) await asyncio.sleep(min(backoff, 10.0)) backoff = min(backoff * 2, 10.0) @@ -668,7 +694,7 @@ class CDPSupervisor: "CDP supervisor %s: session dropped after %.1fs: %s", self.task_id, time.time() - last_success_at, - e, + _redact_cdp_error_text(e), ) finally: with self._state_lock: