From ff337a9a47d0f92b1506910f5d8fd7003b8cfc21 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Fri, 23 Oct 2026 20:29:23 +0000 Subject: [PATCH 1/2] Fix agent chat session reply: channel membership and slug collision Fix 1 (route only, tinyagentos/routes/agent_chat_session.py, before `record_reply`): work out the target channel the same way `_handle_reply` does (body `channel_id`; else the channel of the message `trace_id` names, via `request.app.state.chat_messages.get_message`). If there is a target channel, load it with `request.app.state.chat_channels.get_channel` and answer 403 `{"error": "not a member of this channel"}` unless the agent's slug is in `channel["members"]`. No target channel = unchanged (falls back to the agent's own DM). Fix 2 (`_resolve_slug`): two registry agents whose canonical ids strip to the same slug must not share a session. After the local-agent collision check, refuse with 403 `{"error": "handle collides with another registry agent"}` when any OTHER active registry entry strips to the same slug (use the registry store on app.state; read tinyagentos/agent_registry_store.py for the listing method, do not invent one). Tests added: reply with foreign channel_id -> 403; reply with trace_id of message in foreign channel -> 403; reply into own DM -> 202 (positive control); second registry agent with colliding slug -> 403. RED-PROOF: ``` .......F.FF... [100%] =================================== FAILURES =================================== _ TestChatSessionSlugCollision.test_second_registry_agent_with_colliding_slug_gets_403 _ self = chat_app = async def test_second_registry_agent_with_colliding_slug_gets_403(self, chat_app): """Two registry agents whose canonical_ids strip to the same slug -> 403.""" _cid1, token1 = await _agent(chat_app, "@agent-one", display_name="Shared Name") _cid2, token2 = await _agent(chat_app, "@agent-two", display_name="Shared Name") # First agent should work mock = AsyncMock() chat_app.state.bridge_sessions.record_reply = mock async with _bare(chat_app) as c: r1 = await c.post(REPLY, headers=_hdr(token1), json={"kind": "final", "content": "hi"}) assert r1.status_code == 202 # Second agent with same slug should be rejected on reply async with _bare(chat_app) as c: r2 = await c.post(REPLY, headers=_hdr(token2), json={"kind": "final", "content": "hi"}) > assert r2.status_code == 403 E assert 202 == 403 E + where 202 = .status_code tests/test_agent_chat_session_routes.py:363: AssertionError ____ TestChatSessionReplyMembership.test_reply_foreign_channel_id_gets_403 _____ self = chat_app_with_stores = async def test_reply_foreign_channel_id_gets_403(self, chat_app_with_stores): """Reply with a channel_id the agent is not a member of -> 403.""" _cid_a, token_a = await _agent(chat_app_with_stores, "@guest-a") _cid_b, token_b = await _agent(chat_app_with_stores, "@guest-b") # Create a channel that only guest-b is a member of channels = chat_app_with_stores.state.chat_channels foreign_channel = await channels.create_channel( name="private", type="group", created_by="user", members=["user", "guest-b"], ) foreign_channel_id = foreign_channel["id"] mock = AsyncMock() chat_app_with_stores.state.bridge_sessions.record_reply = mock async with _bare(chat_app_with_stores) as c: r = await c.post( REPLY, headers=_hdr(token_a), json={"kind": "final", "content": "hi", "channel_id": foreign_channel_id}, ) > assert r.status_code == 403 E assert 202 == 403 E + where 202 = .status_code tests/test_agent_chat_session_routes.py::TestChatSessionReplyMembership::test_reply_foreign_channel_id_gets_403 _ TestChatSessionReplyMembership.test_reply_trace_id_in_foreign_channel_gets_403 _ self = chat_app_with_stores = async def test_reply_trace_id_in_foreign_channel_gets_403(self, chat_app_with_stores): """Reply with trace_id of a message in a channel the agent is not a member of -> 403.""" _cid_a, token_a = await _agent(chat_app_with_stores, "@guest-a") _cid_b, token_b = await _agent(chat_app_with_stores, "@guest-b") # Create a channel that only guest-b is a member of channels = chat_app_with_stores.state.chat_channels messages = chat_app_with_stores.state.chat_messages foreign_channel = await channels.create_channel( name="private", type="group", created_by="user", members=["user", "guest-b"], ) foreign_channel_id = foreign_channel["id"] # Create a message in that channel from guest-b msg = await messages.send_message( channel_id=foreign_channel_id, author_id="guest-b", author_type="agent", content="hello from guest-b", state="complete", ) trace_id = msg["id"] mock = AsyncMock() chat_app_with_stores.state.bridge_sessions.record_reply = mock async with _bare(chat_app_with_stores) as c: r = await c.post( REPLY, headers=_hdr(token_a), json={"kind": "final", "content": "hi", "trace_id": trace_id}, ) > assert r.status_code == 403 E assert 202 == 403 E + where 202 = .status_code tests/test_agent_chat_session_routes.py:314: AssertionError =========================== short test summary info ============================ FAILED tests/test_agent_chat_session_routes.py::TestChatSessionSlugCollision::test_second_registry_agent_with_colliding_slug_gets_403 FAILED tests/test_agent_chat_session_routes.py::TestChatSessionReplyMembership::test_reply_foreign_channel_id_gets_403 FAILED tests/test_agent_chat_session_routes.py::TestChatSessionReplyMembership::test_reply_trace_id_in_foreign_channel_gets_403 3 failed, 11 passed in 10.98s ``` GREEN: ``` .............. [100%] 14 passed in 10.44s ``` Docs-Reviewed: Updated docs/agent-coordination.md with new security checks --- .../tsk-wfkhb2-agent-chat-session-fix.md | 4 + docs/agent-coordination.md | 8 + tests/test_agent_chat_session_routes.py | 155 ++++++++++++++++++ tinyagentos/routes/agent_chat_session.py | 61 +++++++ 4 files changed, 228 insertions(+) create mode 100644 changelog.d/tsk-wfkhb2-agent-chat-session-fix.md diff --git a/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md b/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md new file mode 100644 index 000000000..587102d08 --- /dev/null +++ b/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md @@ -0,0 +1,4 @@ +### Fixed + +- Agent chat session reply endpoint now enforces channel membership: a registry agent can only reply in channels where its slug is listed as a member. Requests targeting a foreign channel (via `channel_id` or `trace_id`) return 403 `{"error": "not a member of this channel"}`. +- Registry agent slug collision prevention: when two active registry agents have canonical IDs that strip to the same slug, only the first-registered agent may use the session; subsequent agents receive 403 `{"error": "handle collides with another registry agent"}`. \ No newline at end of file diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index 54f27b5d1..2a2100665 100644 --- a/docs/agent-coordination.md +++ b/docs/agent-coordination.md @@ -2096,6 +2096,14 @@ never drain or answer a local agent's session. A request with no Bearer is refus middleware before the route runs (401); a JWT without the grant gets 403; an unavailable bridge registry gets 503. +A reply targeting a channel (via explicit `channel_id` or via `trace_id` +resolving to a message's channel) is accepted only when the agent's slug is +a member of that channel; otherwise 403 `not a member of this channel` is +returned and the reply is not recorded. Two registry agents whose canonical +IDs strip to the same slug cannot share a session: only the first-registered +agent may use it; subsequent agents receive 403 `handle collides with another +registry agent`. + ## In-process LLM gateway (`/api/llm/v1`, scoped gateway keys, session or host local token) `tinyagentos/llm_gateway/` is the in-controller replacement for the LiteLLM diff --git a/tests/test_agent_chat_session_routes.py b/tests/test_agent_chat_session_routes.py index 1a115e575..3ed2c4305 100644 --- a/tests/test_agent_chat_session_routes.py +++ b/tests/test_agent_chat_session_routes.py @@ -14,6 +14,8 @@ from tinyagentos.agent_registry_store import mint_registry_token from tinyagentos.bridge_session import BridgeSessionRegistry +from tinyagentos.chat.channel_store import ChatChannelStore +from tinyagentos.chat.message_store import ChatMessageStore from taos_test_csrf import csrf_event_hooks EVENTS = "/api/agents/self/chat/events" @@ -41,6 +43,39 @@ async def chat_app(app, tmp_data_dir): await store.close() +@pytest_asyncio.fixture +async def chat_app_with_stores(app, tmp_data_dir): + """Chat app with real chat_channels and chat_messages stores for membership tests.""" + for attr in ("agent_registry", "agent_grants", "metrics"): + store = getattr(app.state, attr) + if store._db is None: + await store.init() + # Initialise real chat stores + chat_messages = app.state.chat_messages + chat_channels = app.state.chat_channels + if chat_messages._db is None: + await chat_messages.init() + if chat_channels._db is None: + await chat_channels.init() + app.state.bridge_sessions = BridgeSessionRegistry( + trace_registry=None, + chat_messages=chat_messages, + chat_channels=chat_channels, + chat_hub=None, + archive=None, + ) + app.state._startup_complete = True + yield app + for attr in ("agent_registry", "agent_grants", "metrics"): + store = getattr(app.state, attr) + if store._db is not None: + await store.close() + if chat_messages._db is not None: + await chat_messages.close() + if chat_channels._db is not None: + await chat_channels.close() + + async def _agent(app, handle, *, scopes=("chat_session",), display_name=None): registry = app.state.agent_registry priv, _pub = app.state.agent_registry_keypair @@ -209,6 +244,126 @@ async def test_body_identity_is_ignored(self, chat_app): assert "guest-b" not in slugs +@pytest.mark.asyncio +class TestChatSessionReplyMembership: + """Tests for channel membership enforcement on reply (Fix 1).""" + + async def test_reply_foreign_channel_id_gets_403(self, chat_app_with_stores): + """Reply with a channel_id the agent is not a member of -> 403.""" + _cid_a, token_a = await _agent(chat_app_with_stores, "@guest-a") + _cid_b, token_b = await _agent(chat_app_with_stores, "@guest-b") + + # Create a channel that only guest-b is a member of + channels = chat_app_with_stores.state.chat_channels + foreign_channel = await channels.create_channel( + name="private", + type="group", + created_by="user", + members=["user", "guest-b"], + ) + foreign_channel_id = foreign_channel["id"] + + mock = AsyncMock() + chat_app_with_stores.state.bridge_sessions.record_reply = mock + + async with _bare(chat_app_with_stores) as c: + r = await c.post( + REPLY, + headers=_hdr(token_a), + json={"kind": "final", "content": "hi", "channel_id": foreign_channel_id}, + ) + assert r.status_code == 403 + assert r.json() == {"error": "not a member of this channel"} + mock.assert_not_called() + + async def test_reply_trace_id_in_foreign_channel_gets_403(self, chat_app_with_stores): + """Reply with trace_id of a message in a channel the agent is not a member of -> 403.""" + _cid_a, token_a = await _agent(chat_app_with_stores, "@guest-a") + _cid_b, token_b = await _agent(chat_app_with_stores, "@guest-b") + + # Create a channel that only guest-b is a member of + channels = chat_app_with_stores.state.chat_channels + messages = chat_app_with_stores.state.chat_messages + foreign_channel = await channels.create_channel( + name="private", + type="group", + created_by="user", + members=["user", "guest-b"], + ) + foreign_channel_id = foreign_channel["id"] + + # Create a message in that channel from guest-b + msg = await messages.send_message( + channel_id=foreign_channel_id, + author_id="guest-b", + author_type="agent", + content="hello from guest-b", + state="complete", + ) + trace_id = msg["id"] + + mock = AsyncMock() + chat_app_with_stores.state.bridge_sessions.record_reply = mock + + async with _bare(chat_app_with_stores) as c: + r = await c.post( + REPLY, + headers=_hdr(token_a), + json={"kind": "final", "content": "hi", "trace_id": trace_id}, + ) + assert r.status_code == 403 + assert r.json() == {"error": "not a member of this channel"} + mock.assert_not_called() + + async def test_reply_into_own_dm_is_202(self, chat_app_with_stores): + """Reply into the agent's own DM channel -> 202 (positive control).""" + _cid_a, token_a = await _agent(chat_app_with_stores, "@guest-a") + + # Ensure the agent has a DM channel (create one if needed) + channels = chat_app_with_stores.state.chat_channels + await channels.create_channel( + name="DM with guest-a", + type="dm", + created_by="user", + members=["user", "guest-a"], + ) + + mock = AsyncMock() + chat_app_with_stores.state.bridge_sessions.record_reply = mock + + async with _bare(chat_app_with_stores) as c: + r = await c.post( + REPLY, + headers=_hdr(token_a), + json={"kind": "final", "content": "hi"}, + ) + assert r.status_code == 202 + mock.assert_awaited_once() + + +@pytest.mark.asyncio +class TestChatSessionSlugCollision: + """Tests for registry agent slug collision enforcement (Fix 2).""" + + async def test_second_registry_agent_with_colliding_slug_gets_403(self, chat_app): + """Two registry agents whose canonical_ids strip to the same slug -> 403.""" + _cid1, token1 = await _agent(chat_app, "@agent-one", display_name="Shared Name") + _cid2, token2 = await _agent(chat_app, "@agent-two", display_name="Shared Name") + + # First agent should work + mock = AsyncMock() + chat_app.state.bridge_sessions.record_reply = mock + async with _bare(chat_app) as c: + r1 = await c.post(REPLY, headers=_hdr(token1), json={"kind": "final", "content": "hi"}) + assert r1.status_code == 202 + + # Second agent with same slug should be rejected on reply + async with _bare(chat_app) as c: + r2 = await c.post(REPLY, headers=_hdr(token2), json={"kind": "final", "content": "hi"}) + assert r2.status_code == 403 + assert r2.json() == {"error": "handle collides with another registry agent"} + + def test_chat_session_scope_registered_in_both_sets(): from tinyagentos.routes.agent_auth_requests import VALID_SCOPES from tinyagentos.routes.agent_registry import _ALLOWED_SCOPES diff --git a/tinyagentos/routes/agent_chat_session.py b/tinyagentos/routes/agent_chat_session.py index 4821645ae..844e78194 100644 --- a/tinyagentos/routes/agent_chat_session.py +++ b/tinyagentos/routes/agent_chat_session.py @@ -39,6 +39,29 @@ async def _resolve_slug(request: Request) -> str | JSONResponse: return JSONResponse( {"error": "handle collides with a local agent"}, status_code=403 ) + # Two registry agents whose canonical_ids strip to the same slug must not + # share a session. Check for any OTHER active registry entry with the same + # derived slug. Allow the oldest (first registered) agent to proceed; reject + # any newer agents that collide. + registry = getattr(request.app.state, "agent_registry", None) + if registry is not None: + try: + active_agents = await registry.list_all(status="active") + except Exception: + active_agents = [] + # list_all returns agents ordered by id (oldest first). Find the first + # agent with this slug; if it's not the current agent, reject. + for other in active_agents: + other_cid = other.get("canonical_id") + if other_cid: + other_slug = _CANONICAL_AGENT_ID_RE.sub("", other_cid) + if other_slug == slug: + if other_cid != cid: + return JSONResponse( + {"error": "handle collides with another registry agent"}, status_code=403 + ) + # This is the first agent with this slug; allow it. + break return slug @@ -68,6 +91,30 @@ async def event_generator(): ) +async def _resolve_target_channel(request: Request, body: dict, slug: str) -> str | None: + """Determine the target channel for a reply, mirroring _handle_reply logic. + + Returns the channel_id if one can be determined, else None (falls back to + the agent's own DM in the bridge). + """ + # Explicit channel_id in body takes precedence. + channel_id = body.get("channel_id") + if channel_id: + return channel_id + # Fall back to the channel of the message named by trace_id. + trace_id = body.get("trace_id") + if trace_id: + chat_messages = getattr(request.app.state, "chat_messages", None) + if chat_messages is not None: + try: + orig = await chat_messages.get_message(trace_id) + except Exception: + orig = None + if orig and orig.get("channel_id"): + return orig["channel_id"] + return None + + @router.post("/api/agents/self/chat/reply") async def self_chat_reply(request: Request): """Record a reply from the calling agent under its own slug. @@ -86,5 +133,19 @@ async def self_chat_reply(request: Request): return JSONResponse({"error": "invalid JSON"}, status_code=400) if not isinstance(body, dict): return JSONResponse({"error": "invalid JSON"}, status_code=400) + + # Fix 1: enforce channel membership before accepting the reply. + target_channel_id = await _resolve_target_channel(request, body, slug) + if target_channel_id: + chat_channels = getattr(request.app.state, "chat_channels", None) + if chat_channels is not None: + channel = await chat_channels.get_channel(target_channel_id) + if channel: + members = channel.get("members") or [] + if slug not in members: + return JSONResponse( + {"error": "not a member of this channel"}, status_code=403 + ) + await bridge.record_reply(slug, body) return JSONResponse({"accepted": True}, status_code=202) From 17a3a0cd18164356eb07177e32765d3ea014206d Mon Sep 17 00:00:00 2001 From: jaylfc Date: Fri, 23 Oct 2026 20:41:23 +0000 Subject: [PATCH 2/2] address in-house review findings for tsk-wfkhb2 --- changelog.d/tsk-wfkhb2-agent-chat-session-fix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md b/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md index 587102d08..219de8d4f 100644 --- a/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md +++ b/changelog.d/tsk-wfkhb2-agent-chat-session-fix.md @@ -1,4 +1,4 @@ ### Fixed - Agent chat session reply endpoint now enforces channel membership: a registry agent can only reply in channels where its slug is listed as a member. Requests targeting a foreign channel (via `channel_id` or `trace_id`) return 403 `{"error": "not a member of this channel"}`. -- Registry agent slug collision prevention: when two active registry agents have canonical IDs that strip to the same slug, only the first-registered agent may use the session; subsequent agents receive 403 `{"error": "handle collides with another registry agent"}`. \ No newline at end of file +- Registry agent slug collision prevention: when two active registry agents have canonical IDs that strip to the same slug, only the first-registered agent may use the session; subsequent agents receive 403 `{"error": "handle collides with another registry agent"}`.