KrustyPlanet/hermes-agent
6
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(browser): close remaining CDP-URL leak paths in supervisor (review)

Browse source 
Review of the salvage found the timeout-message redaction left the more
common failure mode unguarded: when the first websockets.connect(cdp_url)
fails (bad URI / refused / TLS), the raw websockets exception -- which
embeds the full cdp_url incl. ?token= and user:pass@ -- is stashed as
_start_error and re-raised verbatim by start(), and two reconnect
logger.warning sites log the same raw exception.

Add a module-level _redact_cdp_error_text() chokepoint (delegating to
agent.redact.redact_cdp_url) and route all four supervisor egress points
through it:
- start() TimeoutError message (already covered; kept)
- start() _start_error re-raise -> now raises a redacted RuntimeError with
  'from None' so no secret leaks via message OR traceback cause chain
- connect-failed and session-dropped reconnect warnings

Guard tests assert the re-raised message is redacted for both token and
userinfo, the raw cause is suppressed, and the helper preserves non-secret
context (host/reason). Verified with a mutation check: reverting to the raw
'raise err' fails the new tests. Correct the redact_cdp_url docstring to
scope its guarantee to direct-URL redaction and point exception callers at
the supervisor helper.
This commit is contained in:
kshitijk4poor 2026-07-01 13:37:25 +05:30 • committed by kshitij
parent c626dded13
commit e09ff88d02
3 changed files with 126 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

9
agent/redact.py
View file

@ -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:

92
tests/tools/test_browser_cdp_override.py
View file

@ -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

32
tools/browser_supervisor.py
View file

@ -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 "<error redacted>"
# ── 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: