From 5248877c61f178bde89dcdec6b487ca83d3a661d Mon Sep 17 00:00:00 2001 From: claudlos Date: Sun, 28 Jun 2026 19:27:02 -0500 Subject: [PATCH] security(gateway): prove chat/thread origin for persisted /resume; tighten DM scoping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the egilewski/CodeRabbit and teknium1 reviews on PR #52355. 1) Persisted-row chat scope (egilewski/CodeRabbit). The sessions table stored only source + user_id, so an identity-bearing caller could resume/list an INACTIVE persisted row that matched source+user_id but belonged to a DIFFERENT chat (probe: same user moves `same_user_chat_b` into chat-a). Persist the messaging origin and compare it: - schema: sessions gains origin_chat_id / origin_thread_id (declarative auto-migration via the existing column reconciler). - SessionDB._insert_session_row accepts + writes the two columns. - the gateway records them at every origin-bearing creation: both SessionStore create paths (get_or_create_session + reset/switch) and the /title path that materializes a store-only session into the DB. - _resume_target_allowed's identity branch now also requires origin_chat_id AND origin_thread_id to match the caller. Legacy rows with NULL origin (created before this change) cannot prove chat origin and fail closed — resume them via a live session or an admin --all override. The /sessions listing inherits the fix (non-Matrix rows route through the same helper). 2) DM key-contract mirror (teknium1). _same_origin_chat's DM branch only compared user_id and allowed when either side was missing, diverging from build_session_key (no-chat_id DM keys are built from user_id_alt or user_id). It now: treats an equal non-blank chat_id as sufficient (the DM key IS the chat_id when present), and otherwise compares the effective participant id (user_id_alt or user_id), failing closed on a missing/different participant so two no-chat_id DM origins are never conflated. Tests: add same-user/different-chat (e2e + unit) and chat-scope unit cases; add DM no-chat_id / user_id_alt / no-identity / same-chat_id cases; update existing fixtures to record origin_chat_id like the gateway does; make the cross-room `/resume --all` listing test run as admin (cross-room listing is admin-gated) and give the boundary-state resume runner a live same-origin so its post-resume clearing assertions exercise an authorized resume. Co-Authored-By: Claude Opus 4.8 --- gateway/slash_commands.py | 57 +++++-- hermes_state.py | 6 + .../test_matrix_project_context_isolation.py | 3 + tests/gateway/test_resume_command.py | 144 +++++++++++++----- .../test_session_boundary_security_state.py | 4 + 5 files changed, 166 insertions(+), 48 deletions(-) diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index ca3bd518de9..483b614fc27 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -710,9 +710,19 @@ class GatewaySlashCommandsMixin: chat_type = (getattr(current, "chat_type", "") or "").lower() # DM-like chats are always per-user. if chat_type in {"dm", "direct", "private", ""}: - if origin.user_id and current.user_id: - return origin.user_id == current.user_id - return True + # chat_id was already required equal above and, when present, IS the + # DM session key — so an equal non-empty chat_id is sufficient. + # build_session_key only falls back to the participant id + # (``user_id_alt or user_id`` — Signal/Feishu key on user_id_alt) + # when there is NO chat_id; mirror that and fail closed on a + # missing/different participant so two no-chat_id DM origins are + # never conflated (was: compared user_id only and allowed when + # either side was missing). + if str(getattr(current, "chat_id", "") or ""): + return True + cur_pid = str(current.user_id_alt or current.user_id or "") + org_pid = str(origin.user_id_alt or origin.user_id or "") + return bool(cur_pid) and cur_pid == org_pid # Non-DM: scope by participant whenever the session key for this source # is per-user. is_shared_multi_user_session mirrors build_session_key's # isolation rules exactly, so the guard stays in lock-step with the key. @@ -787,22 +797,37 @@ class GatewaySlashCommandsMixin: return False # different platform / source caller_uid = str(getattr(source, "user_id", "") or "") row_uid = str(row.get("user_id") or "") + # Chat/thread origin recorded at session creation (see + # SessionDB._insert_session_row). The sessions table historically stored + # only source + user_id, so a same-user row could belong to a DIFFERENT + # chat; comparing the persisted origin closes that gap. Legacy rows + # created before origin capture have NULL here and therefore fail closed + # (they cannot prove the caller's chat) — resume them via a live session + # or an admin override. + caller_chat = str(getattr(source, "chat_id", "") or "") + row_chat = str(row.get("chat_id") or "") + caller_thread = str(getattr(source, "thread_id", "") or "") + row_thread = str(row.get("thread_id") or "") if caller_uid: # Identity-bearing caller: allow only when the row PROVES the same - # owner AND the same platform/origin. A row with no/blank user_id - # cannot be proven to belong to this caller; a row with no/blank - # source cannot be proven to share the caller's platform (the - # row_src check above only rejects a *mismatching* non-blank source, - # so a blank/legacy source would otherwise slip through on user_id - # equality alone). Either gap fails closed — an identified user must - # not bind to an unowned, other-owned, or unproven-origin persisted - # session by id/title. (Legacy NULL-owner or blank-source rows are - # intentionally not resumable this way; use a live session or an - # explicit admin override.) + # owner AND the same platform/origin AND the same chat/thread. A row + # with no/blank user_id cannot be proven to belong to this caller; a + # row with no/blank source cannot be proven to share the caller's + # platform (the row_src check above only rejects a *mismatching* + # non-blank source, so a blank/legacy source would otherwise slip + # through on user_id equality alone); and a row whose origin chat + # (or thread) differs from the caller's belongs to a different + # conversation. Any gap fails closed — an identified user must not + # bind to an unowned, other-owned, other-chat, or unproven-origin + # 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 ( 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 ) # 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 @@ -3254,6 +3279,12 @@ class GatewaySlashCommandsMixin: session_id=session_id, source=source.platform.value if source.platform else "unknown", user_id=source.user_id, + # Persist the messaging origin so a later /resume of this + # titled-but-now-inactive session can prove it belongs to the + # caller's chat/thread (IDOR scoping). + chat_id=source.chat_id, + chat_type=source.chat_type, + thread_id=source.thread_id, ) except Exception: pass # Session might already exist, ignore errors diff --git a/hermes_state.py b/hermes_state.py index 275b2f45f72..ac1a1266753 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -1590,6 +1590,12 @@ class SessionDB: only filling columns that are still NULL, never overwriting values an earlier writer already set (so a later bare call with source="unknown" can't clobber a real source/model). + + ``chat_id``/``thread_id`` record the messaging origin (the chat/room and + thread the session was started in) so that gateway ``/resume`` can prove + a persisted, now-inactive row belongs to the caller's chat/thread before + switching to it (IDOR scoping — without them the ``sessions`` table has + no chat/thread to compare). """ def _do(conn): conn.execute( diff --git a/tests/gateway/test_matrix_project_context_isolation.py b/tests/gateway/test_matrix_project_context_isolation.py index 00341a8036c..c7ed758a1de 100644 --- a/tests/gateway/test_matrix_project_context_isolation.py +++ b/tests/gateway/test_matrix_project_context_isolation.py @@ -506,6 +506,9 @@ async def test_matrix_resume_all_lists_room_names(): source_b, [_entry(source_a, "session-a", "Project A Plan"), _entry(source_b, "session-b", "Project B Plan")], ) + # Cross-room `/resume --all` listing is admin-gated (IDOR scoping), so this + # cross-room listing test must run as a configured admin. + runner._resume_caller_is_admin = lambda _src: True result = await runner._handle_resume_command(_event("/resume --all", source_b)) diff --git a/tests/gateway/test_resume_command.py b/tests/gateway/test_resume_command.py index 2f425ba4ed0..078c75740f9 100644 --- a/tests/gateway/test_resume_command.py +++ b/tests/gateway/test_resume_command.py @@ -84,8 +84,8 @@ class TestHandleResumeCommand: """With no argument, lists recently titled sessions.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("sess_001", "telegram", user_id="12345") - db.create_session("sess_002", "telegram", user_id="12345") + db.create_session("sess_001", "telegram", user_id="12345", chat_id="67890") + db.create_session("sess_002", "telegram", user_id="12345", chat_id="67890") db.set_session_title("sess_001", "Research") db.set_session_title("sess_002", "Coding") @@ -105,7 +105,7 @@ class TestHandleResumeCommand: """With no arg and no titled sessions, shows instructions.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("sess_001", "telegram", user_id="12345") # No title + db.create_session("sess_001", "telegram", user_id="12345", chat_id="67890") # No title event = _make_event(text="/resume") runner = _make_runner(session_db=db, event=event) @@ -119,11 +119,11 @@ class TestHandleResumeCommand: """Numeric argument resumes the indexed titled session from the list.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("sess_001", "telegram", user_id="12345") - db.create_session("sess_002", "telegram", user_id="12345") + db.create_session("sess_001", "telegram", user_id="12345", chat_id="67890") + db.create_session("sess_002", "telegram", user_id="12345", chat_id="67890") db.set_session_title("sess_001", "Research") db.set_session_title("sess_002", "Coding") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume 2") runner = _make_runner(session_db=db, current_session_id="current_session_001", @@ -141,9 +141,9 @@ class TestHandleResumeCommand: """Out-of-range numeric arguments show a helpful error.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("sess_001", "telegram", user_id="12345") + db.create_session("sess_001", "telegram", user_id="12345", chat_id="67890") db.set_session_title("sess_001", "Research") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume 9") runner = _make_runner(session_db=db, current_session_id="current_session_001", @@ -160,9 +160,9 @@ class TestHandleResumeCommand: """Resolves a title and switches to that session.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("old_session_abc", "telegram", user_id="12345") + 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", user_id="12345") + 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", @@ -216,7 +216,7 @@ class TestHandleResumeCommand: """Returns error for unknown session name.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume Nonexistent Session") runner = _make_runner(session_db=db, event=event) @@ -229,7 +229,7 @@ class TestHandleResumeCommand: """Returns friendly message when already on the requested session.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") db.set_session_title("current_session_001", "Active Project") event = _make_event(text="/resume Active Project") @@ -244,11 +244,11 @@ class TestHandleResumeCommand: """Asking for 'My Project' when 'My Project #2' exists gets the latest.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("sess_v1", "telegram", user_id="12345") + db.create_session("sess_v1", "telegram", user_id="12345", chat_id="67890") db.set_session_title("sess_v1", "My Project") - db.create_session("sess_v2", "telegram", user_id="12345") + db.create_session("sess_v2", "telegram", user_id="12345", chat_id="67890") db.set_session_title("sess_v2", "My Project #2") - db.create_session("current_session_001", "telegram", user_id="12345") + 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", @@ -267,12 +267,12 @@ class TestHandleResumeCommand: from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("compressed_root", "telegram", user_id="12345") + db.create_session("compressed_root", "telegram", user_id="12345", chat_id="67890") db.set_session_title("compressed_root", "Compressed Work") db.end_session("compressed_root", "compression") - db.create_session("compressed_child", "telegram", user_id="12345", parent_session_id="compressed_root") + db.create_session("compressed_child", "telegram", user_id="12345", chat_id="67890", parent_session_id="compressed_root") db.append_message("compressed_child", "user", "hello from continuation") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume Compressed Work") runner = _make_runner( @@ -300,9 +300,9 @@ class TestHandleResumeCommand: """Switching sessions clears any cached running agent.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("old_session", "telegram", user_id="12345") + db.create_session("old_session", "telegram", user_id="12345", chat_id="67890") db.set_session_title("old_session", "Old Work") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume Old Work") runner = _make_runner(session_db=db, current_session_id="current_session_001", @@ -326,9 +326,9 @@ class TestHandleResumeCommand: import threading from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("old_session", "telegram", user_id="12345") + db.create_session("old_session", "telegram", user_id="12345", chat_id="67890") db.set_session_title("old_session", "Old Work") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume Old Work") runner = _make_runner(session_db=db, current_session_id="current_session_001", @@ -353,9 +353,9 @@ class TestHandleResumeCommand: """ from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("abc123", "telegram", user_id="12345") + db.create_session("abc123", "telegram", user_id="12345", chat_id="67890") db.set_session_title("abc123", "Bracketed") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") for raw in ("", "[abc123]", '"abc123"', "'abc123'"): event = _make_event(text=f"/resume {raw}") @@ -382,9 +382,9 @@ class TestHandleResumeCommand: """ from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("unnamed_session_xyz", "telegram", user_id="12345") + db.create_session("unnamed_session_xyz", "telegram", user_id="12345", chat_id="67890") # Deliberately no title set — this session can ONLY be resolved by ID. - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") event = _make_event(text="/resume unnamed_session_xyz") runner = _make_runner( @@ -409,7 +409,7 @@ class TestHandleSessionsCommand: async def test_sessions_command_lists_current_platform_sessions(self, tmp_path): from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("tg_session", "telegram", user_id="12345") + db.create_session("tg_session", "telegram", user_id="12345", chat_id="67890") db.set_session_title("tg_session", "Telegram Work") db.create_session("discord_session", "discord") db.set_session_title("discord_session", "Discord Work") @@ -434,7 +434,7 @@ class TestHandleSessionsCommand: config is not.""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("tg_named", "telegram", user_id="12345") + db.create_session("tg_named", "telegram", user_id="12345", chat_id="67890") db.set_session_title("tg_named", "Telegram Work") db.create_session("discord_unnamed", "discord") # other origin db.append_message("discord_unnamed", "user", "discord first prompt") @@ -462,7 +462,7 @@ class TestHandleSessionsCommand: db.set_session_title("victim_other_uid", "Other User") db.create_session("victim_missing_uid", "telegram") # NULL owner db.set_session_title("victim_missing_uid", "Unowned") - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") for name in ("Other User", "victim_other_uid", "Unowned", "victim_missing_uid"): event = _make_event(text=f"/resume {name}") @@ -482,14 +482,14 @@ class TestHandleSessionsCommand: unproven-origin transcript).""" from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("blank_source_same_uid", "telegram", user_id="12345") + db.create_session("blank_source_same_uid", "telegram", user_id="12345", chat_id="67890") db.set_session_title("blank_source_same_uid", "Blank Source Same UID") # Simulate a malformed/legacy row that does not record its origin. db._conn.execute( "UPDATE sessions SET source = '' WHERE id = ?", ("blank_source_same_uid",) ) db._conn.commit() - db.create_session("current_session_001", "telegram", user_id="12345") + db.create_session("current_session_001", "telegram", user_id="12345", chat_id="67890") for name in ("Blank Source Same UID", "blank_source_same_uid"): event = _make_event(text=f"/resume {name}") @@ -537,11 +537,56 @@ class TestHandleSessionsCommand: allow_override=False) is False db.close() + @pytest.mark.asyncio + async def test_resume_blocks_same_user_different_chat(self, tmp_path): + """egilewski/CodeRabbit probe: the SAME user must not move a persisted + transcript from another chat into the current one. The row records its + records origin chat_id, so a chat-a caller cannot resume a chat-b row even with + a matching user_id (persisted-row chat-scope proof).""" + from hermes_state import SessionDB + db = SessionDB(db_path=tmp_path / "state.db") + db.create_session("same_user_chat_b", "telegram", user_id="12345", + chat_id="chat-b") + db.set_session_title("same_user_chat_b", "Same User Chat B") + db.create_session("current_session_001", "telegram", user_id="12345", + chat_id="chat-a") + + for name in ("Same User Chat B", "same_user_chat_b"): + event = _make_event(text=f"/resume {name}", user_id="12345", + chat_id="chat-a") + event.source.chat_type = "group" + runner = _make_runner(session_db=db, current_session_id="current_session_001", + event=event) + result = await runner._handle_resume_command(event) + runner.session_store.switch_session.assert_not_called() + assert "Resumed" not in result, name + db.close() + + 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 + db = SessionDB(db_path=tmp_path / "state.db") + db.create_session("row_chat_a", "telegram", user_id="12345", + chat_id="chat-a") + db.create_session("row_chat_b", "telegram", user_id="12345", + chat_id="chat-b") + db.create_session("row_legacy_nochat", "telegram", user_id="12345") # NULL chat + runner = _make_runner(session_db=db) + runner._gateway_session_origin_for_id = lambda sid: None # persisted-only + 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 + db.close() + @pytest.mark.asyncio async def test_gateway_dispatches_sessions_command(self, tmp_path): from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") - db.create_session("tg_session", "telegram", user_id="12345") + db.create_session("tg_session", "telegram", user_id="12345", chat_id="67890") db.set_session_title("tg_session", "Telegram Work") event = _make_event(text="/sessions") @@ -580,11 +625,40 @@ class TestSameOriginChatGroupScoping: runner.config.group_sessions_per_user = False assert runner._same_origin_chat(self._src("alice"), self._src("bob")) is True - def test_dm_cross_user_still_blocked(self): + def test_dm_cross_user_blocked_without_chat_id(self): + # No-chat_id DM: build_session_key falls back to the participant id + # (user_id_alt or user_id), so two different participants are different + # origins and must not match. (With a chat_id present the DM key IS the + # chat_id — see test_dm_same_chat_id_is_same_origin.) + runner = _make_runner() + a = self._src("alice", chat_type="dm", chat_id=None) + b = self._src("bob", chat_type="dm", chat_id=None) + assert runner._same_origin_chat(a, b) is False + + def test_dm_no_identity_no_chat_id_fails_closed(self): + # teknium1 review: an identity-less no-chat_id DM must fail closed rather + # than be treated as a shared origin. + runner = _make_runner() + a = self._src(None, chat_type="dm", chat_id=None) + b = self._src(None, chat_type="dm", chat_id=None) + assert runner._same_origin_chat(a, b) is False + + def test_dm_user_id_alt_mismatch_without_chat_id_blocked(self): + # No-chat_id DM keyed on user_id_alt (Signal/Feishu): different alt ids + # are different sessions even if user_id is absent/equal. + runner = _make_runner() + a = self._src(None, chat_type="dm", chat_id=None, user_id_alt="alice-alt") + b = self._src(None, chat_type="dm", chat_id=None, user_id_alt="bob-alt") + assert runner._same_origin_chat(a, b) is False + + def test_dm_same_chat_id_is_same_origin(self): + # With a chat_id present, the DM session key is chat_id-only (no + # participant), so an equal chat_id is a same-origin match — mirrors + # build_session_key. runner = _make_runner() a = self._src("alice", chat_type="dm", chat_id="dm-1") - b = self._src("bob", chat_type="dm", chat_id="dm-1") - assert runner._same_origin_chat(a, b) is False + 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): """End-to-end via the live-origin branch: Alice cannot resume Bob's diff --git a/tests/gateway/test_session_boundary_security_state.py b/tests/gateway/test_session_boundary_security_state.py index f3862aac6a7..b00ae1d96c9 100644 --- a/tests/gateway/test_session_boundary_security_state.py +++ b/tests/gateway/test_session_boundary_security_state.py @@ -90,6 +90,10 @@ def _make_resume_runner(): runner._session_db = AsyncSessionDB(MagicMock()) runner._session_db._db.resolve_session_by_title.return_value = "resumed-session" runner._session_db._db.get_session_title.return_value = "Resumed Work" + # The resumed session is live and shares the caller's origin, so the + # /resume IDOR guard authorizes it (this test covers the post-resume + # security-state clearing, not the ownership check). + runner._gateway_session_origin_for_id = lambda sid: source return runner, session_key