From 599a6391d4089802e68e5b244881e369ac7765a0 Mon Sep 17 00:00:00 2001 From: claudlos Date: Mon, 29 Jun 2026 08:32:14 -0500 Subject: [PATCH] security(gateway): fail closed on no-provenance persisted /resume for non-DM callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses egilewski/CodeRabbit follow-up on PR #52355: the identity-bearing persisted fallback compared row_chat == caller_chat, which SUCCEEDS when both normalize to "" — so a legacy row with no stored chat provenance could still be resumed by a caller that also has no chat_id (probe: a group caller with chat_id=None resuming a NULL-chat telegram row on matching user_id). A non-DM session (group/channel/forum/thread) is keyed by chat_id in build_session_key, so a blank chat on either side is NOT proof of same-chat. Require both row and caller chat_id to be non-blank and equal for non-DM callers; a legacy NULL-chat row (or a caller missing its chat_id) now fails closed. DMs are unchanged: they are keyed on user_id, so a no-chat_id DM row stays resumable by the same user (and a mismatching chat_id, when present, is still rejected). Adds the blank-caller-chat group probe and a DM no-chat_id same-user/other-user regression. Co-Authored-By: Claude Opus 4.8 --- gateway/slash_commands.py | 18 ++++++- tests/gateway/test_resume_command.py | 72 ++++++++++++++++++++-------- 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 483b614fc27..54c69aa6457 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -808,6 +808,8 @@ class GatewaySlashCommandsMixin: row_chat = str(row.get("chat_id") or "") caller_thread = str(getattr(source, "thread_id", "") or "") row_thread = str(row.get("thread_id") or "") + chat_type = (getattr(source, "chat_type", "") or "").lower() + caller_is_dm = chat_type in {"dm", "direct", "private", ""} if caller_uid: # Identity-bearing caller: allow only when the row PROVES the same # owner AND the same platform/origin AND the same chat/thread. A row @@ -822,13 +824,25 @@ class GatewaySlashCommandsMixin: # persisted session by id/title. (Legacy NULL-owner/blank-source/ # NULL-chat rows are intentionally not resumable this way; use a # live session or an explicit admin override.) - return ( + base_ok = ( bool(row_uid) and row_uid == caller_uid and bool(row_src) and bool(caller_src) and str(row_src) == str(caller_src) - and row_chat == caller_chat and row_thread == caller_thread ) + if not base_ok: + return False + if caller_is_dm: + # DMs are keyed on user_id; chat_id is legitimately absent on + # both sides for a no-chat_id DM (already scoped by user_id + # above). Still reject a mismatching chat_id when present. + return row_chat == caller_chat + # Non-DM (group/channel/forum/thread): build_session_key includes + # chat_id, so a row (or caller) with NO chat provenance cannot prove + # same-chat. Require both sides non-blank and equal — a legacy + # NULL-chat row (or a caller missing its chat_id) fails closed even + # when both normalize to "". (CWE-639) + return bool(row_chat) and bool(caller_chat) and row_chat == caller_chat # No caller identity: the persisted row carries only source + user_id # (the sessions table has no chat_id), so a same-platform row can belong # to a DIFFERENT chat or user. Same-platform alone is therefore NOT diff --git a/tests/gateway/test_resume_command.py b/tests/gateway/test_resume_command.py index 078c75740f9..aa39af9cfac 100644 --- a/tests/gateway/test_resume_command.py +++ b/tests/gateway/test_resume_command.py @@ -183,9 +183,9 @@ class TestHandleResumeCommand: restored conversation, while leaving other chats' overrides intact (#10702).""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("old_session_abc", "telegram") + db.create_session("old_session_abc", "telegram", user_id="12345", chat_id="67890") db.set_session_title("old_session_abc", "My Project") - db.create_session("current_session_001", "telegram") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume My Project") runner = _make_runner(session_db=db, current_session_id="current_session_001", @@ -523,7 +523,8 @@ class TestHandleSessionsCommand: assert "Resumed" not in result, name db.close() - def test_resume_target_allowed_blocks_no_identity_persisted(self, tmp_path): + @pytest.mark.asyncio + async def test_resume_target_allowed_blocks_no_identity_persisted(self, tmp_path): """Unit-level: the persisted-row fallback fails closed for an identity-less caller (no live origin resolvable).""" from hermes_state import SessionDB @@ -533,7 +534,7 @@ class TestHandleSessionsCommand: runner._gateway_session_origin_for_id = lambda sid: None # inactive/persisted-only caller = SessionSource(platform=Platform.TELEGRAM, chat_id="chat-a", chat_type="group", user_id=None) - assert runner._resume_target_allowed(caller, "victim_chat_b_uid", + assert await runner._resume_target_allowed(caller, "victim_chat_b_uid", allow_override=False) is False db.close() @@ -562,7 +563,8 @@ class TestHandleSessionsCommand: assert "Resumed" not in result, name db.close() - def test_resume_target_allowed_chat_scope(self, tmp_path): + @pytest.mark.asyncio + async def test_resume_target_allowed_chat_scope(self, tmp_path): """Unit-level: identity-bearing persisted fallback requires the row's origin chat (and thread) to match the caller's.""" from hermes_state import SessionDB @@ -577,9 +579,33 @@ class TestHandleSessionsCommand: caller = SessionSource(platform=Platform.TELEGRAM, chat_id="chat-a", chat_type="group", user_id="12345") # Same chat → allowed; different chat → blocked; legacy NULL-chat → blocked. - assert runner._resume_target_allowed(caller, "row_chat_a", allow_override=False) is True - assert runner._resume_target_allowed(caller, "row_chat_b", allow_override=False) is False - assert runner._resume_target_allowed(caller, "row_legacy_nochat", allow_override=False) is False + assert await runner._resume_target_allowed(caller, "row_chat_a", allow_override=False) is True + assert await runner._resume_target_allowed(caller, "row_chat_b", allow_override=False) is False + assert await runner._resume_target_allowed(caller, "row_legacy_nochat", allow_override=False) is False + # egilewski/CodeRabbit probe: a GROUP caller that itself has no chat_id + # must NOT resume a legacy NULL-chat row just because both normalize to + # "" — a non-DM session is keyed by chat_id, so blank == no provenance. + blank_caller = SessionSource(platform=Platform.TELEGRAM, chat_id=None, + chat_type="group", user_id="12345") + assert await runner._resume_target_allowed(blank_caller, "row_legacy_nochat", + allow_override=False) is False + db.close() + + @pytest.mark.asyncio + async def test_resume_target_allowed_dm_no_chat_id_scopes_by_user(self, tmp_path): + """A DM is keyed on user_id; a no-chat_id DM row is resumable by the same + user (chat_id legitimately absent on both sides), unlike a group row.""" + from hermes_state import SessionDB + db = SessionDB(db_path=tmp_path / "state.db") + db.create_session("dm_row", "telegram", user_id="12345") # DM, no chat_id + runner = _make_runner(session_db=db) + runner._gateway_session_origin_for_id = lambda sid: None # persisted-only + same = SessionSource(platform=Platform.TELEGRAM, chat_id=None, + chat_type="dm", user_id="12345") + other = SessionSource(platform=Platform.TELEGRAM, chat_id=None, + chat_type="dm", user_id="99999") + assert await runner._resume_target_allowed(same, "dm_row", allow_override=False) is True + assert await runner._resume_target_allowed(other, "dm_row", allow_override=False) is False db.close() @pytest.mark.asyncio @@ -660,13 +686,14 @@ class TestSameOriginChatGroupScoping: b = self._src("alice", chat_type="dm", chat_id="dm-1") assert runner._same_origin_chat(a, b) is True - def test_resume_target_allowed_blocks_cross_user_live_group(self): + @pytest.mark.asyncio + async def test_resume_target_allowed_blocks_cross_user_live_group(self): """End-to-end via the live-origin branch: Alice cannot resume Bob's active group session in the same chat.""" runner = _make_runner() bob = self._src("bob") runner._gateway_session_origin_for_id = lambda sid: bob - assert runner._resume_target_allowed( + assert await runner._resume_target_allowed( self._src("alice"), "bobs_live_sid", allow_override=False ) is False @@ -717,7 +744,8 @@ class TestResumeRowVisibleMatrixAllScoping: return SessionSource(platform=Platform.MATRIX, chat_id=chat_id, chat_type="group", user_id=user_id) - def test_non_admin_all_does_not_expose_other_room(self): + @pytest.mark.asyncio + async def test_non_admin_all_does_not_expose_other_room(self): runner = _make_runner() runner._resume_caller_is_admin = lambda src: False # Titled row whose live origin is a DIFFERENT Matrix room. @@ -725,32 +753,35 @@ class TestResumeRowVisibleMatrixAllScoping: chat_type="group", user_id="@bob:hs") runner._gateway_session_origin_for_id = lambda sid: other_room row = {"id": "sid_other_room"} - assert runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is False + assert await runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is False - def test_non_admin_all_still_shows_same_room(self): + @pytest.mark.asyncio + async def test_non_admin_all_still_shows_same_room(self): runner = _make_runner() runner._resume_caller_is_admin = lambda src: False same_room = SessionSource(platform=Platform.MATRIX, chat_id="!room-a:hs", chat_type="group", user_id="@bob:hs") runner._gateway_session_origin_for_id = lambda sid: same_room row = {"id": "sid_same_room"} - assert runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is True + assert await runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is True - def test_admin_all_exposes_cross_room(self): + @pytest.mark.asyncio + async def test_admin_all_exposes_cross_room(self): runner = _make_runner() runner._resume_caller_is_admin = lambda src: True other_room = SessionSource(platform=Platform.MATRIX, chat_id="!room-b:hs", chat_type="group", user_id="@bob:hs") runner._gateway_session_origin_for_id = lambda sid: other_room row = {"id": "sid_other_room"} - assert runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is True + assert await runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is True - def test_non_admin_all_fails_closed_on_unknown_origin(self): + @pytest.mark.asyncio + async def test_non_admin_all_fails_closed_on_unknown_origin(self): runner = _make_runner() runner._resume_caller_is_admin = lambda src: False runner._gateway_session_origin_for_id = lambda sid: None row = {"id": "sid_unknown"} - assert runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is False + assert await runner._resume_row_visible(self._matrix_src(), row, allow_all=True) is False class TestSameMatrixRoomThreadScoping: @@ -792,7 +823,8 @@ class TestSameMatrixRoomThreadScoping: assert runner._same_matrix_room(threaded, room_level) is False assert runner._same_matrix_room(room_level, threaded) is False - def test_resume_row_visible_blocks_cross_thread(self): + @pytest.mark.asyncio + async def test_resume_row_visible_blocks_cross_thread(self): """End-to-end through the Matrix listing guard.""" runner = _make_runner() runner._resume_caller_is_admin = lambda src: False @@ -800,4 +832,4 @@ class TestSameMatrixRoomThreadScoping: runner._gateway_session_origin_for_id = lambda sid: origin_thread_b row = {"id": "sid_thread_b"} caller_thread_a = self._msrc(thread_id="thread-a") - assert runner._resume_row_visible(caller_thread_a, row, allow_all=False) is False + assert await runner._resume_row_visible(caller_thread_a, row, allow_all=False) is False