From c44952b93bdb045ff8d38f20eca20990af66814b Mon Sep 17 00:00:00 2001 From: StepWise1234 Date: Thu, 11 Jun 2026 23:23:48 -0700 Subject: [PATCH] fix(gateway): authorize Signal group members via SIGNAL_GROUP_ALLOWED_USERS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chat-allowlist bypass in `authz_mixin._is_user_authorized` previously only enumerated Telegram and QQBOT, so Signal group messages always fell through to the per-user `SIGNAL_ALLOWED_USERS` check. Operators who set `SIGNAL_GROUP_ALLOWED_USERS` expected groups to "just work" the way they do on Telegram, but every group member not also listed in `SIGNAL_ALLOWED_USERS` was silently rejected with `WARNING gateway.run: Unauthorized user: () on signal`. Two fixes in `gateway/authz_mixin.py`: 1. Add `Platform.SIGNAL -> "SIGNAL_GROUP_ALLOWED_USERS"` to `chat_allowlist_env`. The env var name was chosen long ago and is already consumed by `platforms/signal.py` as a group-ID list — this just makes auth honor the same setting. 2. Normalize `source.chat_id` by stripping the `group:` prefix before comparison. `platforms/signal.py` builds Signal chat_ids as `f"group:{group_id}"` (line ~520), while the env value is stored unprefixed (signal.py's own filter at line ~515 compares unprefixed). Without normalization, the equality check would still fail. Adds `tests/gateway/test_signal_group_auth.py` with 10 cases mirroring the existing channel-allowlist test pattern (private helper mirrors the patched branch, stdlib only, no Hermes imports). All 376 tests across the 18 auth/allowlist/signal gateway test files pass with this change. Co-Authored-By: Claude Opus 4.7 (1M context) --- gateway/authz_mixin.py | 10 +- tests/gateway/test_signal_group_auth.py | 156 ++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 tests/gateway/test_signal_group_auth.py diff --git a/gateway/authz_mixin.py b/gateway/authz_mixin.py index 824d730871c00..2d3a46aa15f80 100644 --- a/gateway/authz_mixin.py +++ b/gateway/authz_mixin.py @@ -123,6 +123,7 @@ def _is_user_authorized(self, source: SessionSource) -> bool: chat_allowlist_env = { Platform.TELEGRAM: "TELEGRAM_GROUP_ALLOWED_CHATS", Platform.QQBOT: "QQ_GROUP_ALLOWED_USERS", + Platform.SIGNAL: "SIGNAL_GROUP_ALLOWED_USERS", }.get(source.platform, "") if chat_allowlist_env: raw_chat_allowlist = os.getenv(chat_allowlist_env, "").strip() @@ -132,7 +133,14 @@ def _is_user_authorized(self, source: SessionSource) -> bool: for cid in raw_chat_allowlist.split(",") if cid.strip() } - if "*" in allowed_group_ids or source.chat_id in allowed_group_ids: + # Signal's adapter builds source.chat_id as f"group:{id}" + # while SIGNAL_GROUP_ALLOWED_USERS stores the bare group + # IDs (signal.py's own filter compares unprefixed). Strip + # the prefix so the same env value works in both checks. + chat_id_norm = source.chat_id + if chat_id_norm.startswith("group:"): + chat_id_norm = chat_id_norm[len("group:"):] + if "*" in allowed_group_ids or chat_id_norm in allowed_group_ids: return True if not user_id: diff --git a/tests/gateway/test_signal_group_auth.py b/tests/gateway/test_signal_group_auth.py new file mode 100644 index 0000000000000..fcb89f2968556 --- /dev/null +++ b/tests/gateway/test_signal_group_auth.py @@ -0,0 +1,156 @@ +"""Regression guard for SIGNAL_GROUP_ALLOWED_USERS group-member auth bypass. + +Before this fix, ``authz_mixin._is_user_authorized`` only enumerated Telegram +and QQBOT in its group-chat-allowlist bypass: + + chat_allowlist_env = { + Platform.TELEGRAM: "TELEGRAM_GROUP_ALLOWED_CHATS", + Platform.QQBOT: "QQ_GROUP_ALLOWED_USERS", + }.get(source.platform, "") + +Signal was missing, so every Signal group message fell through to the +per-user ``SIGNAL_ALLOWED_USERS`` check — meaning operators who set +``SIGNAL_GROUP_ALLOWED_USERS`` (the group-ID allowlist consumed by +``platforms/signal.py``) expected groups to "just work" the way they do on +Telegram, but in practice every group member other than those explicitly +listed in ``SIGNAL_ALLOWED_USERS`` was silently rejected with +``WARNING gateway.run: Unauthorized user: () on signal``. + +There is also a chat_id format gotcha: ``platforms/signal.py`` builds +``source.chat_id`` as ``f"group:{group_id}"`` (line ~520), while the env +value ``SIGNAL_GROUP_ALLOWED_USERS`` is stored as bare group IDs (signal.py +itself compares unprefixed at line ~515). A naive bypass patch would still +fail equality because ``"group:abc..." not in {"abc..."}``. + +This test mirrors the patched bypass logic with both fixes and exercises +representative cases. +""" + +import unittest + + +def _is_group_member_authorized( + chat_type: str, chat_id: str, allowed_groups_raw: str +) -> bool: + """Mirror the Signal-relevant branch of ``_is_user_authorized``. + + Replicates: chat-type gate, env parse, prefix normalization, equality / + wildcard check. Returns ``True`` when the group bypass applies. + """ + if chat_type not in {"group", "forum", "channel"} or not chat_id: + return False + if not allowed_groups_raw: + return False + allowed_group_ids = { + cid.strip() for cid in allowed_groups_raw.split(",") if cid.strip() + } + if not allowed_group_ids: + return False + chat_id_norm = chat_id + if chat_id_norm.startswith("group:"): + chat_id_norm = chat_id_norm[len("group:") :] + return "*" in allowed_group_ids or chat_id_norm in allowed_group_ids + + +# Realistic-shaped sample IDs (random UUIDs, base64-shaped group IDs). +GROUP_A = "RTkH42KVKvN5BrfL82d69UvbXZtXfwZs8xNlcQoANmw=" +GROUP_B = "XyZAbCdeFghi0123456789==" +USER_RANDOM = "f1355565-f2c6-4649-ae7d-f4cbf3160f63" +USER_OTHER = "82ed6ad1-84e7-40ed-8d15-7073c189ecd4" + + +class TestSignalGroupBypass(unittest.TestCase): + """Group members in an allowed Signal group are authorized regardless of user ID.""" + + def test_member_in_allowed_group_with_prefixed_chat_id(self): + """The Signal adapter passes chat_id with a 'group:' prefix; bypass + must still match against the unprefixed env value.""" + self.assertTrue( + _is_group_member_authorized("group", f"group:{GROUP_A}", GROUP_A) + ) + + def test_member_in_allowed_group_without_prefix(self): + """If the chat_id happens to be unprefixed it must also match.""" + self.assertTrue( + _is_group_member_authorized("group", GROUP_A, GROUP_A) + ) + + def test_wildcard_allows_any_group(self): + """'*' in SIGNAL_GROUP_ALLOWED_USERS authorizes any group.""" + self.assertTrue( + _is_group_member_authorized("group", f"group:{GROUP_A}", "*") + ) + self.assertTrue( + _is_group_member_authorized("group", f"group:{GROUP_B}", "*") + ) + + def test_member_in_different_group_is_denied(self): + """Bypass scoped to allow-listed groups — wrong group ≠ bypass.""" + self.assertFalse( + _is_group_member_authorized("group", f"group:{GROUP_B}", GROUP_A) + ) + + def test_dm_chat_type_skips_group_bypass(self): + """DMs must not be authorized by a group-list match.""" + self.assertFalse( + _is_group_member_authorized("dm", USER_RANDOM, GROUP_A) + ) + + def test_empty_allowlist_denies(self): + """No SIGNAL_GROUP_ALLOWED_USERS set ⇒ bypass off, denies.""" + self.assertFalse( + _is_group_member_authorized("group", f"group:{GROUP_A}", "") + ) + + def test_whitespace_only_allowlist_denies(self): + """Whitespace-only env value parses to empty set ⇒ denies.""" + self.assertFalse( + _is_group_member_authorized("group", f"group:{GROUP_A}", " , ") + ) + + def test_multiple_groups_in_allowlist(self): + """Comma-separated allowlist authorizes each listed group.""" + raw = f"{GROUP_A},{GROUP_B}" + self.assertTrue( + _is_group_member_authorized("group", f"group:{GROUP_A}", raw) + ) + self.assertTrue( + _is_group_member_authorized("group", f"group:{GROUP_B}", raw) + ) + self.assertFalse( + _is_group_member_authorized("group", "group:OTHER", raw) + ) + + def test_forum_and_channel_chat_types_also_bypass(self): + """Existing bypass already supports forum/channel; keep that intact.""" + self.assertTrue( + _is_group_member_authorized("forum", f"group:{GROUP_A}", GROUP_A) + ) + self.assertTrue( + _is_group_member_authorized("channel", f"group:{GROUP_A}", GROUP_A) + ) + + +class TestSignalGroupBypassMimicsTelegram(unittest.TestCase): + """Behavioural parity with the Telegram/QQBOT branch. + + Telegram's TELEGRAM_GROUP_ALLOWED_CHATS already authorizes any user + posting in a listed chat. Signal users with SIGNAL_GROUP_ALLOWED_USERS + set should get the same UX — adding Signal to the dict closes the gap. + """ + + def test_any_random_user_in_allowed_group_is_authorized(self): + """Bypass is chat-scoped; the user_id of the sender is irrelevant.""" + # Both these "users" should be authorized by virtue of group membership. + for random_user in (USER_RANDOM, USER_OTHER, "+1-anonymous-phone"): + with self.subTest(user=random_user): + self.assertTrue( + _is_group_member_authorized( + "group", f"group:{GROUP_A}", GROUP_A + ), + f"user {random_user!r} should be authorized by group bypass", + ) + + +if __name__ == "__main__": + unittest.main()