security(gateway): allow shared-group resume in persisted /resume fallback
Addresses egilewski follow-up on PR #52355: the persisted-row fallback required row_uid == caller_uid for every identity-bearing caller, which wrongly blocked a legitimately SHARED non-DM group session. With group_sessions_per_user=False, build_session_key resolves every participant of a chat to one session key, so a co-member (different user_id) in the same chat shares Bob's session — but the guard returned "/resume blocked". Mirror is_shared_multi_user_session() in the fallback, exactly as the live-origin branch (_same_origin_chat) already does: for a non-DM caller, first require the same platform + chat + thread provenance (unchanged — blank/mismatching chat still fails closed), then allow without user-id equality when the session is shared, and keep requiring the same owner for per-user group/thread sessions. DM scoping is unchanged (always per-user). Adds a regression: shared group → co-member allowed; per-user group → blocked; different chat → blocked even when shared. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
599a6391d4
commit
f1e58d8c1a
|
|
@ -824,25 +824,48 @@ 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.)
|
||||
base_ok = (
|
||||
bool(row_uid) and row_uid == caller_uid
|
||||
and bool(row_src) and bool(caller_src)
|
||||
# Common origin proof for any identity-bearing caller: a non-blank
|
||||
# source that matches the caller's platform, and the same thread. A
|
||||
# blank/legacy source can't prove the platform; a different thread is
|
||||
# a different session (build_session_key appends thread_id).
|
||||
origin_ok = (
|
||||
bool(row_src) and bool(caller_src)
|
||||
and str(row_src) == str(caller_src)
|
||||
and row_thread == caller_thread
|
||||
)
|
||||
if not base_ok:
|
||||
if not origin_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
|
||||
# DMs are keyed on user_id; require the same owner. chat_id is
|
||||
# legitimately absent on both sides for a no-chat_id DM (scoped
|
||||
# by user_id), but a mismatching chat_id (when present) is still
|
||||
# rejected.
|
||||
return (
|
||||
bool(row_uid) and row_uid == caller_uid
|
||||
and 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
|
||||
if not (bool(row_chat) and bool(caller_chat) and row_chat == caller_chat):
|
||||
return False
|
||||
# Within the same non-DM chat/thread, mirror build_session_key's
|
||||
# participant scoping: a SHARED group/thread session
|
||||
# (group_sessions_per_user=False, or a shared thread) is one session
|
||||
# for every participant, so the same-chat proof above is sufficient —
|
||||
# do NOT also require user-id equality (otherwise a co-member is
|
||||
# wrongly blocked from their own shared session). A per-user session
|
||||
# still requires the same owner.
|
||||
shared = is_shared_multi_user_session(
|
||||
source,
|
||||
group_sessions_per_user=getattr(self.config, "group_sessions_per_user", True),
|
||||
thread_sessions_per_user=getattr(self.config, "thread_sessions_per_user", False),
|
||||
)
|
||||
if shared:
|
||||
return True
|
||||
return bool(row_uid) and row_uid == caller_uid
|
||||
# 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
|
||||
|
|
|
|||
|
|
@ -608,6 +608,37 @@ class TestHandleSessionsCommand:
|
|||
assert await runner._resume_target_allowed(other, "dm_row", allow_override=False) is False
|
||||
db.close()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resume_target_allowed_shared_group_no_user_match(self, tmp_path):
|
||||
"""egilewski probe: with group_sessions_per_user=False a non-DM group
|
||||
session is shared, so a co-member (different user_id) in the SAME chat
|
||||
may resume it — same-chat/thread proof is sufficient, user equality is
|
||||
not required. Per-user groups (default) still require the same owner."""
|
||||
from hermes_state import SessionDB
|
||||
db = SessionDB(db_path=tmp_path / "state.db")
|
||||
db.create_session("shared_group_row", "telegram", user_id="bob",
|
||||
chat_id="shared-chat", chat_type="group")
|
||||
runner = _make_runner(session_db=db)
|
||||
runner._gateway_session_origin_for_id = lambda sid: None # persisted-only
|
||||
alice = SessionSource(platform=Platform.TELEGRAM, chat_id="shared-chat",
|
||||
chat_type="group", user_id="alice")
|
||||
|
||||
# Shared group → Alice may resume Bob's row in the same chat.
|
||||
runner.config.group_sessions_per_user = False
|
||||
assert await runner._resume_target_allowed(alice, "shared_group_row",
|
||||
allow_override=False) is True
|
||||
# Per-user group → Alice must NOT resume Bob's row (IDOR preserved).
|
||||
runner.config.group_sessions_per_user = True
|
||||
assert await runner._resume_target_allowed(alice, "shared_group_row",
|
||||
allow_override=False) is False
|
||||
# A different chat is still blocked even when shared.
|
||||
runner.config.group_sessions_per_user = False
|
||||
other_chat = SessionSource(platform=Platform.TELEGRAM, chat_id="other-chat",
|
||||
chat_type="group", user_id="alice")
|
||||
assert await runner._resume_target_allowed(other_chat, "shared_group_row",
|
||||
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
|
||||
|
|
|
|||
Loading…
Reference in New Issue