fix(tui_gateway): check steer authority by transport membership under fan-out
subagent.steer resolves authority by comparing the request's context-bound transport with the session's transport slot. Once a session mirrors to more than one client that slot holds a FanoutTransport, so the comparison fails for every client, the peer that commissioned the subagent included, and every steer is rejected. Fan-out without this check ships that regression, and no existing test catches it because the suite only exercises single-client sessions. Authority now asks whether the request's transport is attached to the session, directly or through the fan-out. The single-client case is unchanged: a bare slot still compares by identity. This widens authority. Any client attached to a mirrored session can steer that session's subagents, not only the peer that commissioned them. Narrowing it back to the commissioning peer requires recording that peer per subagent, which this change does not do. tests/tui_gateway/test_multi_client_fanout.py pins the commissioning peer's authority inside a fan-out, pins the widened case, and keeps a single-client control in which an unattached client is still refused. The four browser.controller.* handlers gate on the session transport slot exactly as subagent.steer did, through one shared gate in the _controller_method decorator, and the conversion missed them. Once a session mirrors to more than one client the slot holds a FanoutTransport, which is identical to no peer's WSTransport, so browser.controller.register, .result, .heartbeat and .detach all answer "session is not owned by this transport" for every client, the peer that registered the controller included. Browser control is therefore unusable on any mirrored session. No existing test catches it because the browser-control suite only exercises single-client sessions. All four now ask the same question steer asks: is the request's transport attached to this session, directly or through the fan-out. The single-client case is unchanged, because a bare slot still compares by identity. Only registration widens. On .result, .heartbeat and .detach the broker's is_owner check sits below the session gate and compares controller.owner is owner against the transport recorded at attach time (gateway/browser_control_broker.py), so a mirrored peer that did not register the controller is still refused there, now with "controller is not owned by this transport" instead of the session message. browser.controller.register has no such check, so any client attached to a mirrored session may register a controller for it; the broker's principal lane keeps that inside one authenticated identity, and a second identity in the same lane hard-replaces the first. tests/tui_gateway/test_multi_client_fanout.py pins the registering peer's access inside a fan-out, pins the widened and broker-refused cases, and keeps a single-client control in which an unattached client is still refused on all three scope-gated handlers.
This commit is contained in:
Ryan Tucker
@@ -327,6 +327,197 @@ def test_a_fanout_with_no_live_peer_is_dead():
|
||||
assert server._transport_is_dead(FanoutTransport()) is True
|
||||
|
||||
|
||||
def test_steer_authority_recognizes_the_exact_client_inside_a_fanout():
|
||||
"""Wrapping attached peers must not revoke the commissioning peer's authority."""
|
||||
owner, watcher, stranger = (
|
||||
_FakeClient("owner"),
|
||||
_FakeClient("watcher"),
|
||||
_FakeClient("stranger"),
|
||||
)
|
||||
session = _session(transport=FanoutTransport(owner, watcher))
|
||||
server._sessions["sid"] = session
|
||||
try:
|
||||
token = server.bind_transport(owner)
|
||||
try:
|
||||
assert server._current_session_steer_authority("sid") == (owner, session)
|
||||
finally:
|
||||
server.reset_transport(token)
|
||||
|
||||
token = server.bind_transport(stranger)
|
||||
try:
|
||||
assert server._current_session_steer_authority("sid") == (None, None)
|
||||
finally:
|
||||
server.reset_transport(token)
|
||||
finally:
|
||||
server._sessions.pop("sid", None)
|
||||
|
||||
|
||||
def test_steer_authority_is_granted_to_an_attached_watcher():
|
||||
"""A client that attached to watch a session may also steer its subagents.
|
||||
|
||||
This is INTENTIONAL and wider than the pre-fan-out rule, which admitted only
|
||||
whichever client happened to hold the transport slot. A mirrored session has
|
||||
no single owner in that slot, so authority is membership in it. Narrowing
|
||||
this back to the commissioning peer would need a per-subagent record of who
|
||||
commissioned it, which this change does not add; the widening is stated in
|
||||
the pull request description, and this test pins it so it cannot be changed
|
||||
silently in either direction.
|
||||
"""
|
||||
owner, watcher, stranger = (
|
||||
_FakeClient("owner"),
|
||||
_FakeClient("watcher"),
|
||||
_FakeClient("stranger"),
|
||||
)
|
||||
session = _session(transport=owner)
|
||||
server._attach_session_transport(session, watcher)
|
||||
solo = _session(transport=owner)
|
||||
server._sessions["sid"] = session
|
||||
server._sessions["solo"] = solo
|
||||
try:
|
||||
token = server.bind_transport(watcher)
|
||||
try:
|
||||
assert server._current_session_steer_authority("sid") == (watcher, session)
|
||||
# Control: on a single-client session the same watcher is a stranger,
|
||||
# so the widening reaches attached clients and nobody else.
|
||||
assert server._current_session_steer_authority("solo") == (None, None)
|
||||
finally:
|
||||
server.reset_transport(token)
|
||||
|
||||
token = server.bind_transport(stranger)
|
||||
try:
|
||||
assert server._current_session_steer_authority("sid") == (None, None)
|
||||
finally:
|
||||
server.reset_transport(token)
|
||||
finally:
|
||||
server._sessions.pop("sid", None)
|
||||
server._sessions.pop("solo", None)
|
||||
|
||||
|
||||
# ── browser-control session ownership ──────────────────────────────────────
|
||||
#
|
||||
# The four browser.controller.* handlers gate on the session slot exactly as
|
||||
# subagent.steer did, so fan-out breaks them the same way and the fix is the
|
||||
# same predicate. These tests pin the gate itself: they assert on the ownership
|
||||
# error message and stop at the NEXT gate ("no controller registered for this
|
||||
# session"), so a later broker change cannot make them pass vacuously.
|
||||
|
||||
_CONTROLLER_IDENTITY = {"user_id": "user-fixture", "provider": "provider-fixture"}
|
||||
_NOT_OWNED = "session is not owned by this transport"
|
||||
_NO_CONTROLLER = "no controller registered for this session"
|
||||
|
||||
|
||||
def _controller_client(name: str) -> _FakeClient:
|
||||
"""A fan-out client that also carries a server-authenticated identity."""
|
||||
client = _FakeClient(name)
|
||||
client.auth_identity = dict(_CONTROLLER_IDENTITY)
|
||||
return client
|
||||
|
||||
|
||||
def _controller_rpc(transport, method_name: str, **params) -> dict:
|
||||
return server.dispatch(
|
||||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": method_name,
|
||||
"params": params,
|
||||
},
|
||||
transport,
|
||||
)
|
||||
|
||||
|
||||
def _error_message(response: dict) -> str | None:
|
||||
return (response.get("error") or {}).get("message")
|
||||
|
||||
|
||||
def test_browser_control_ownership_gate_admits_a_peer_inside_a_fanout():
|
||||
"""Wrapping attached peers must not revoke browser control for all of them."""
|
||||
owner, watcher, stranger = (
|
||||
_controller_client("owner"),
|
||||
_controller_client("watcher"),
|
||||
_controller_client("stranger"),
|
||||
)
|
||||
session = _session(transport=FanoutTransport(owner, watcher), profile="default")
|
||||
server._sessions["sid"] = session
|
||||
try:
|
||||
# The peer that would have registered the controller clears the
|
||||
# ownership gate and stops at the next one.
|
||||
assert _error_message(_controller_rpc(owner, "browser.controller.heartbeat", session_id="sid")) == _NO_CONTROLLER
|
||||
# Widened, exactly as the steer conversion widened steer: any attached
|
||||
# peer clears the session gate. The broker's is_owner check below it is
|
||||
# what still refuses a peer that did not register the controller.
|
||||
assert _error_message(_controller_rpc(watcher, "browser.controller.heartbeat", session_id="sid")) == _NO_CONTROLLER
|
||||
# An unattached client is still refused at the ownership gate.
|
||||
assert _error_message(_controller_rpc(stranger, "browser.controller.heartbeat", session_id="sid")) == _NOT_OWNED
|
||||
finally:
|
||||
server._sessions.pop("sid", None)
|
||||
|
||||
|
||||
def test_browser_control_ownership_gate_is_unchanged_for_a_single_client():
|
||||
"""Control: a bare slot still compares by identity on all four handlers."""
|
||||
owner, stranger = _controller_client("owner"), _controller_client("stranger")
|
||||
session = _session(transport=owner, profile="default")
|
||||
server._sessions["solo"] = session
|
||||
try:
|
||||
for method_name in (
|
||||
"browser.controller.heartbeat",
|
||||
"browser.controller.result",
|
||||
"browser.controller.detach",
|
||||
):
|
||||
assert _error_message(_controller_rpc(stranger, method_name, session_id="solo")) == _NOT_OWNED
|
||||
assert _error_message(_controller_rpc(owner, "browser.controller.heartbeat", session_id="solo")) == _NO_CONTROLLER
|
||||
finally:
|
||||
server._sessions.pop("solo", None)
|
||||
|
||||
|
||||
def test_browser_controller_registers_and_detaches_inside_a_fanout(monkeypatch):
|
||||
"""End to end: registration on a mirrored session, then a clean detach."""
|
||||
from gateway import browser_control_broker
|
||||
|
||||
monkeypatch.setattr(
|
||||
"gateway.browser_control_broker.browser_control_enabled", lambda: True
|
||||
)
|
||||
owner, watcher, stranger = (
|
||||
_controller_client("owner"),
|
||||
_controller_client("watcher"),
|
||||
_controller_client("stranger"),
|
||||
)
|
||||
session = _session(transport=FanoutTransport(owner, watcher), profile="default")
|
||||
server._sessions["sid"] = session
|
||||
registration = {}
|
||||
try:
|
||||
registration = _controller_rpc(
|
||||
owner,
|
||||
"browser.controller.register",
|
||||
session_id="sid",
|
||||
controller_id="controller-fixture",
|
||||
browser_profile_id="browser-profile-fixture",
|
||||
capabilities=["controller.noop"],
|
||||
protocol_version=browser_control_broker.BROWSER_CONTROL_PROTOCOL_VERSION,
|
||||
)
|
||||
assert registration.get("error") is None, registration
|
||||
assert registration["result"]["scope"]["session_id"] == "sid"
|
||||
|
||||
# The registering peer owns the controller; the fan-out peer that did
|
||||
# not register clears the session gate and is refused by the broker.
|
||||
assert _controller_rpc(owner, "browser.controller.heartbeat", session_id="sid")["result"] == {"ok": True}
|
||||
assert _error_message(_controller_rpc(watcher, "browser.controller.heartbeat", session_id="sid")) == "controller is not owned by this transport"
|
||||
assert _error_message(_controller_rpc(stranger, "browser.controller.heartbeat", session_id="sid")) == _NOT_OWNED
|
||||
|
||||
detached = _controller_rpc(owner, "browser.controller.detach", session_id="sid")
|
||||
assert detached.get("error") is None, detached
|
||||
finally:
|
||||
if registration.get("result"):
|
||||
broker = browser_control_broker.get_browser_control_broker()
|
||||
scope = broker.scope_for_session(
|
||||
session_id="sid",
|
||||
principal_id=registration["result"]["scope"]["principal_id"],
|
||||
transport_family=registration["result"]["scope"]["transport_family"],
|
||||
)
|
||||
if scope is not None:
|
||||
broker.detach(scope, notify_controller=False)
|
||||
server._sessions.pop("sid", None)
|
||||
|
||||
|
||||
# ── event delivery ─────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
|
||||
@@ -3,10 +3,12 @@
|
||||
The controller extension registers over the authenticated ``/api/ws`` gateway. Everything
|
||||
binds to the SERVER-MINTED identity (``WSTransport.auth_identity``, stamped from the single-use
|
||||
ticket); a client-supplied ``principal_id`` is ignored and replaced by a digest of it. Broker
|
||||
frames are re-enveloped as Gateway ``event`` frames; ``result`` resolves a command only on the
|
||||
owning transport for the exact attached scope (the broker's exact-scope ``complete`` is the
|
||||
backstop). Capabilities come from the broker's explicit allowlist (no raw CDP/eval/uploads).
|
||||
Bodies are rebound onto server.py's globals (bind_module publishes this module's helpers too).
|
||||
frames are re-enveloped as Gateway ``event`` frames; ``result`` resolves a command only when the
|
||||
request arrives on a transport ATTACHED to the session and that transport is the broker-recorded
|
||||
owner of the exact attached scope (the broker's exact-scope ``complete`` is the backstop).
|
||||
Capabilities come from the broker's explicit allowlist (no raw CDP/eval/uploads). Bodies are
|
||||
rebound onto server.py's globals (bind_module publishes this module's helpers too), which is how
|
||||
the session gate reaches ``_session_transport_contains`` with no import of its own.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -81,9 +83,10 @@ def _controller_method(
|
||||
"""Register a handler behind the shared fail-closed (4403) controller gates.
|
||||
|
||||
Order: ``precheck(rid, params)`` (may return an error envelope) → caller holds a
|
||||
server-authenticated, non-internal identity → the named session exists and its
|
||||
``transport`` is exactly the caller → when ``lookup_scope``, a scope is attached for
|
||||
this session/principal/family and the caller owns it. Then
|
||||
server-authenticated, non-internal identity → the named session exists and the caller is
|
||||
ATTACHED to it, directly or through the ``FanoutTransport`` a mirrored session holds in its
|
||||
slot → when ``lookup_scope``, a scope is attached for this session/principal/family and the
|
||||
caller owns it. Then
|
||||
``fn(rid, params, transport, identity, session_id, broker, scope, session)`` runs.
|
||||
"""
|
||||
|
||||
@@ -102,7 +105,11 @@ def _controller_method(
|
||||
session_id = str(params.get("session_id") or "")
|
||||
with _sessions_lock:
|
||||
session = _sessions.get(session_id)
|
||||
if session is None or session.get("transport") is not transport:
|
||||
# Membership, not slot identity: a mirrored session holds a FanoutTransport, which is
|
||||
# identical to no peer's transport, so slot identity would refuse every client here — the
|
||||
# peer that registered the controller included. The broker's is_owner check below still
|
||||
# keys on the transport that attached the scope.
|
||||
if not _session_transport_contains(session, transport):
|
||||
return _err(rid, _ERR_FORBIDDEN, "session is not owned by this transport")
|
||||
broker = browser_control_broker.get_browser_control_broker()
|
||||
scope = None
|
||||
@@ -190,7 +197,11 @@ def _(rid, params: dict, _transport, _identity, _session_id, broker, scope, _ses
|
||||
|
||||
@_controller_method("browser.controller.heartbeat")
|
||||
def _(rid, params: dict, *_gate) -> dict:
|
||||
"""Acknowledge a heartbeat only for this transport's attached controller."""
|
||||
"""Acknowledge a heartbeat only for this transport's own attached controller.
|
||||
|
||||
The session gate admits any client attached to the session, including a fan-out peer; the
|
||||
broker's ``is_owner`` check then narrows the answer to the transport that actually registered
|
||||
the controller."""
|
||||
return _ok(rid, {"ok": True})
|
||||
|
||||
|
||||
|
||||
@@ -915,16 +915,20 @@ def handle_request(req: dict) -> dict | None:
|
||||
|
||||
def _current_session_steer_authority(session_id: str) -> tuple[Transport | None, dict | None]:
|
||||
"""Unforgeable steering authority for this RPC context: the public session id is only a lookup
|
||||
hint; authority is the identity of BOTH the ContextVar-bound transport and the live in-memory
|
||||
record under that id, so transport rebinding, removal or id reuse invalidates an earlier generation."""
|
||||
hint; authority requires the ContextVar-bound transport to be ATTACHED to the live in-memory record
|
||||
under that id, so transport detachment, session removal or id reuse invalidates an earlier generation."""
|
||||
transport = current_transport()
|
||||
if transport is None or not session_id:
|
||||
return None, None
|
||||
expected_session = _current_runtime_session_record.get()
|
||||
with _sessions_lock:
|
||||
session = _sessions.get(session_id)
|
||||
# Membership, not slot identity: a mirrored session stores a FanoutTransport in the slot, so slot
|
||||
# identity alone would reject every client, the peer that commissioned the subagent included.
|
||||
# Authority is membership in the slot, which also grants it to any client attached to mirror the
|
||||
# session (see tests/tui_gateway/test_multi_client_fanout.py).
|
||||
if (session is None or (expected_session is not None and session is not expected_session)
|
||||
or session.get("transport") is not transport):
|
||||
or not _session_transport_contains(session, transport)):
|
||||
return None, None
|
||||
return transport, session
|
||||
|
||||
|
||||
Reference in New Issue
Block a user