fix(server): sanitize the retry closure's error display

The retry (_run) closure emitted the raw str(exc) to ui.on_error, so a
credential-bearing base-URL in a backend ConnectError
(https://user:pass@host) crossed into the dashboard SSE — the
confidentiality floor _record_fatal_error enforces, bypassed here.
Sanitize the display inline with the same sanitize_error_text redactor.

This is separable from the reused-session stale-flag hazard that keeps
_run off ensure_error_recorded: that hazard is about recording /
idempotency (deferred to #865); this is only the display string. The
double state emit and the pre-try no-persist remain in #865.

Adds a focused test that a retry-error's on_error is redacted.
Flagged by review on #866.
This commit is contained in:
Patrick Buckley
2026-07-17 19:48:42 -07:00
parent 3af80907c7
commit cdc360bd98
2 changed files with 52 additions and 6 deletions
@@ -776,3 +776,44 @@ class TestInitialWorkerFailureState:
assert events, "init worker never emitted"
assert all(e.get("state") != "error" for e in events)
assert all(e.get("type") != "error" for e in events)
def test_retry_closure_sanitizes_error_display(monkeypatch):
"""The retry (_run) closure sanitizes the exception text before on_error, so
a credential-bearing base-URL in a backend error can't cross into the
dashboard SSE. It deliberately does NOT route through ensure_error_recorded
(the reused-session stale-flag hazard — #865); the display-sanitize half of
that hygiene is fixed at the site. Driven via the capture pattern: patch the
dispatcher to hand back the run closure, then run it inline as the owner."""
import threading
from types import SimpleNamespace
from turnstone.core import session_worker
from turnstone.server import _interactive_dispatch_retry
ui = _FakeUI(ws_id="ws-retry")
def _boom(_msg):
raise RuntimeError("cannot reach https://user:pass@host:8000/v1 for model=x")
ws = SimpleNamespace(
id="ws-retry", session=SimpleNamespace(send=_boom), ui=ui, worker_thread=None
)
captured: dict = {}
def _capture(_ws, *, enqueue, run, thread_name=None, **_kw):
captured["run"] = run
return True
monkeypatch.setattr(session_worker, "send", _capture)
_interactive_dispatch_retry(ws, "retry this")
assert "run" in captured, "retry did not dispatch through session_worker.send"
# The owner guard reads ws.worker_thread is the executing thread; run the
# captured closure inline as that owner.
ws.worker_thread = threading.current_thread()
captured["run"]()
errors = [e["message"] for e in ui.events if e.get("type") == "error"]
assert errors, "retry closure emitted no on_error"
assert all("user:pass" not in m for m in errors), errors # credential redacted
assert any("REDACTED" in m for m in errors) # the sanitizer ran, not a no-op
+11 -6
View File
@@ -727,13 +727,18 @@ def _interactive_dispatch_retry(ws: Workstream, user_msg: str) -> None:
# REUSED session a pre-try raise after a prior errored turn finds
# _has_persisted_error stale-True (it is session-lifetime — cleared
# only by _emit_state idle/running, not per-turn), so the recorder
# would no-op and swallow the fresh error. The raw-exc UI leak, the
# double state emit, and the pre-try no-persist (the coordinator can
# then read a STALE last_error on a reused-session retry) are known,
# match the /send and coord-send sibling closures, and are tracked in
# #865's per-turn error-signal redesign.
# would no-op and swallow the fresh error. The DISPLAY string is
# sanitized inline (a credential-bearing base-URL in the exception
# text must not cross into the dashboard SSE, the confidentiality
# floor _record_fatal_error also enforces); the double state emit and
# the pre-try no-persist (a reused-session retry can then have the
# coordinator read a STALE last_error) still need the per-turn
# error-signal redesign and are tracked in #865, matching the /send
# and coord-send sibling closures.
if ws.worker_thread is me:
ui.on_error(f"Error: {exc}")
from turnstone.core.memory import sanitize_error_text
ui.on_error(f"Error: {sanitize_error_text(str(exc))}")
ui.on_stream_end()
ui.on_state_change("error")