diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index 31973b962..bc1c52be5 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -667,7 +667,17 @@ class DiscordAdapter(BasePlatformAdapter): # human-user allowlist below (bots aren't in it). else: # Non-bot: enforce the configured user/role allowlists. - if not self._is_allowed_user(str(message.author.id), message.author): + # Pass guild + is_dm so role checks are scoped to the + # originating guild (prevents cross-guild DM bypass, see + # _is_allowed_user docstring). + _msg_guild = getattr(message, "guild", None) + _is_dm = isinstance(message.channel, discord.DMChannel) or _msg_guild is None + if not self._is_allowed_user( + str(message.author.id), + message.author, + guild=_msg_guild, + is_dm=_is_dm, + ): return # Multi-agent filtering: if the message mentions specific bots @@ -1477,8 +1487,16 @@ class DiscordAdapter(BasePlatformAdapter): pass completed = receiver.check_silence() + # Voice inputs always originate from a specific guild + # (guild_id is in scope). Pass it so role checks are + # guild-scoped and not cross-guild. + _vc_guild = self._client.get_guild(guild_id) if self._client is not None else None for user_id, pcm_data in completed: - if not self._is_allowed_user(str(user_id)): + if not self._is_allowed_user( + str(user_id), + guild=_vc_guild, + is_dm=False, + ): continue await self._process_voice_input(guild_id, user_id, pcm_data) except asyncio.CancelledError: @@ -1521,13 +1539,32 @@ class DiscordAdapter(BasePlatformAdapter): except OSError: pass - def _is_allowed_user(self, user_id: str, author=None) -> bool: + def _is_allowed_user( + self, + user_id: str, + author=None, + *, + guild=None, + is_dm: bool = False, + ) -> bool: """Check if user is allowed via DISCORD_ALLOWED_USERS or DISCORD_ALLOWED_ROLES. Uses OR semantics: if the user matches EITHER allowlist, they're allowed. If both allowlists are empty, everyone is allowed (backwards compatible). - When author is a Member, checks .roles directly; otherwise falls back - to scanning the bot's mutual guilds for a Member record. + + Role checks are **scoped to the guild the message originated from**. + For DMs (no guild context), role-based auth is disabled by default and + only user-ID allowlist applies. Set ``DISCORD_DM_ROLE_AUTH_GUILD`` + to a specific guild ID to opt-in: role membership in that one guild + will authorize DMs. This prevents cross-guild privilege escalation + where a user with the configured role in any shared public server + could DM the bot and pass the allowlist. + + Args: + user_id: Author ID as a string. + author: Optional Member/User object for in-guild role lookup. + guild: The guild the message arrived in (None for DMs). + is_dm: True if the message came from a DM channel. """ # ``getattr`` fallbacks here guard against test fixtures that build # an adapter via ``object.__new__(DiscordAdapter)`` and skip __init__ @@ -1538,31 +1575,54 @@ class DiscordAdapter(BasePlatformAdapter): has_roles = bool(allowed_roles) if not has_users and not has_roles: return True - # Check user ID allowlist + # Check user ID allowlist (works for both DMs and guild messages) if has_users and user_id in allowed_users: return True - # Check role allowlist - if has_roles: - # Try direct role check from Member object - direct_roles = getattr(author, "roles", None) if author is not None else None - if direct_roles: - if any(getattr(r, "id", None) in allowed_roles for r in direct_roles): - return True - # Fallback: scan mutual guilds for member's roles - if self._client is not None: - try: - uid_int = int(user_id) - except (TypeError, ValueError): - uid_int = None - if uid_int is not None: - for guild in self._client.guilds: - m = guild.get_member(uid_int) - if m is None: - continue - m_roles = getattr(m, "roles", None) or [] - if any(getattr(r, "id", None) in allowed_roles for r in m_roles): - return True - return False + # Role allowlist is only consulted when configured. + if not has_roles: + return False + + # DM path: roles require explicit opt-in via DISCORD_DM_ROLE_AUTH_GUILD. + # Without this, a user with the configured role in ANY mutual guild + # could DM the bot and bypass the allowlist (cross-guild leakage). + if is_dm or guild is None: + dm_guild_env = os.getenv("DISCORD_DM_ROLE_AUTH_GUILD", "").strip() + if not dm_guild_env.isdigit(): + return False + dm_guild_id = int(dm_guild_env) + if self._client is None: + return False + dm_guild = self._client.get_guild(dm_guild_id) + if dm_guild is None: + return False + try: + uid_int = int(user_id) + except (TypeError, ValueError): + return False + m = dm_guild.get_member(uid_int) + if m is None: + return False + m_roles = getattr(m, "roles", None) or [] + return any(getattr(r, "id", None) in allowed_roles for r in m_roles) + + # Guild path: role check is scoped to THIS guild only. + # 1) Prefer the direct Member object passed in (correct guild by construction). + direct_roles = getattr(author, "roles", None) if author is not None else None + author_guild = getattr(author, "guild", None) + if direct_roles and (author_guild is None or author_guild.id == guild.id): + if any(getattr(r, "id", None) in allowed_roles for r in direct_roles): + return True + # 2) Fallback: resolve the Member in the message's guild only — NEVER + # scan other mutual guilds (that is the cross-guild bypass bug). + try: + uid_int = int(user_id) + except (TypeError, ValueError): + return False + m = guild.get_member(uid_int) + if m is None: + return False + m_roles = getattr(m, "roles", None) or [] + return any(getattr(r, "id", None) in allowed_roles for r in m_roles) async def send_image_file( self, diff --git a/tests/gateway/test_discord_roles_dm_scope.py b/tests/gateway/test_discord_roles_dm_scope.py new file mode 100644 index 000000000..a8c856116 --- /dev/null +++ b/tests/gateway/test_discord_roles_dm_scope.py @@ -0,0 +1,254 @@ +"""Regression guard: DISCORD_ALLOWED_ROLES must be guild-scoped, not global. + +Prior to this fix, ``_is_allowed_user`` iterated ``self._client.guilds`` and +returned True if the user held any allowed role in ANY mutual guild. This +allowed a cross-guild DM bypass: + +1. Bot is in both a large public server A and a private trusted server B. +2. User has role ``R`` in public server A. ``DISCORD_ALLOWED_ROLES`` is + configured with ``R`` intending it to authorize server B members. +3. User DMs the bot. The role check scans every mutual guild, finds ``R`` + in public server A, and authorizes the DM. + +The fix scopes role checks to the originating guild and disables role-based +auth on DMs unless ``DISCORD_DM_ROLE_AUTH_GUILD`` explicitly opts into a +single trusted guild. +""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from gateway.platforms.discord import DiscordAdapter + + +def _make_adapter(allowed_users=None, allowed_roles=None, guilds=None): + """Build a minimal DiscordAdapter without running __init__.""" + adapter = object.__new__(DiscordAdapter) + adapter._allowed_user_ids = set(allowed_users or []) + adapter._allowed_role_ids = set(allowed_roles or []) + + client = MagicMock() + client.guilds = guilds or [] + client.get_guild = lambda gid: next( + (g for g in (guilds or []) if getattr(g, "id", None) == gid), + None, + ) + adapter._client = client + return adapter + + +def _role(role_id): + return SimpleNamespace(id=role_id) + + +def _guild_with_member(guild_id, member_id, role_ids): + """Build a fake guild that holds one member with the given roles.""" + member = SimpleNamespace( + id=member_id, + roles=[_role(rid) for rid in role_ids], + guild=None, # filled below + ) + guild = SimpleNamespace( + id=guild_id, + get_member=lambda uid: member if uid == member_id else None, + ) + member.guild = guild + return guild, member + + +# --------------------------------------------------------------------------- +# Cross-guild DM bypass — MUST be rejected +# --------------------------------------------------------------------------- + + +def test_dm_rejects_role_held_in_other_guild(monkeypatch): + """A user with an allowed role in a DIFFERENT guild must NOT pass a DM. + + Regression guard for the cross-guild DM bypass in the initial + DISCORD_ALLOWED_ROLES implementation. + """ + monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + + public_guild, _ = _guild_with_member( + guild_id=111111, + member_id=42, + role_ids=[5555], # allowed role, but in the wrong guild + ) + trusted_guild = SimpleNamespace(id=222222, get_member=lambda uid: None) + + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[public_guild, trusted_guild], + ) + + # DM from user 42: role check must NOT scan other guilds. + assert ( + adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) + is False + ) + + +def test_dm_role_auth_requires_explicit_guild_optin(monkeypatch): + """With DISCORD_DM_ROLE_AUTH_GUILD set, only that specific guild counts. + + The user has the role in the opted-in guild — allowed. + """ + trusted_guild, _ = _guild_with_member( + guild_id=222222, + member_id=42, + role_ids=[5555], + ) + other_guild = SimpleNamespace(id=333333, get_member=lambda uid: None) + + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[other_guild, trusted_guild], + ) + monkeypatch.setenv("DISCORD_DM_ROLE_AUTH_GUILD", "222222") + + assert ( + adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) + is True + ) + + +def test_dm_role_auth_optin_rejects_when_not_member(monkeypatch): + """DISCORD_DM_ROLE_AUTH_GUILD set but user isn't a member → reject.""" + trusted_guild = SimpleNamespace( + id=222222, + get_member=lambda uid: None, # user not in trusted guild + ) + public_guild, _ = _guild_with_member( + guild_id=111111, + member_id=42, + role_ids=[5555], + ) + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[public_guild, trusted_guild], + ) + monkeypatch.setenv("DISCORD_DM_ROLE_AUTH_GUILD", "222222") + + assert ( + adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) + is False + ) + + +# --------------------------------------------------------------------------- +# Guild messages — role check must be scoped to THIS guild only +# --------------------------------------------------------------------------- + + +def test_guild_message_role_check_scoped_to_originating_guild(monkeypatch): + """A user with the role in a DIFFERENT guild than the message origin + must NOT be authorized, even when both guilds are mutual. + """ + monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + + public_guild, _ = _guild_with_member( + guild_id=111111, + member_id=42, + role_ids=[5555], # allowed role in public guild only + ) + # Message arrives in trusted_guild where user 42 has NO role + trusted_guild = SimpleNamespace(id=222222, get_member=lambda uid: None) + + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[public_guild, trusted_guild], + ) + + # No author object passed → falls through to guild.get_member path + assert ( + adapter._is_allowed_user( + "42", author=None, guild=trusted_guild, is_dm=False + ) + is False + ) + + +def test_guild_message_role_check_allows_when_role_in_same_guild(monkeypatch): + """Positive path: user has the role IN the message's guild → allowed.""" + monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + + trusted_guild, _ = _guild_with_member( + guild_id=222222, + member_id=42, + role_ids=[5555], + ) + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[trusted_guild], + ) + + assert ( + adapter._is_allowed_user( + "42", author=None, guild=trusted_guild, is_dm=False + ) + is True + ) + + +def test_guild_message_rejects_author_roles_from_different_guild(monkeypatch): + """If an author Member object comes from a different guild than the + message, the cached .roles on it must NOT be trusted — rely on the + current guild's Member lookup instead. + """ + monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + + # Author is a Member of a DIFFERENT guild with the allowed role + foreign_guild = SimpleNamespace(id=999, get_member=lambda uid: None) + foreign_author = SimpleNamespace( + id=42, + roles=[_role(5555)], + guild=foreign_guild, + ) + # Message arrives in this_guild where user 42 has NO role + this_guild = SimpleNamespace(id=222222, get_member=lambda uid: None) + + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[foreign_guild, this_guild], + ) + + assert ( + adapter._is_allowed_user( + "42", author=foreign_author, guild=this_guild, is_dm=False + ) + is False + ) + + +# --------------------------------------------------------------------------- +# Backwards-compatibility — user-ID allowlist still works in both contexts +# --------------------------------------------------------------------------- + + +def test_user_id_allowlist_works_in_dm(): + adapter = _make_adapter(allowed_users=["42"]) + assert ( + adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) + is True + ) + + +def test_user_id_allowlist_works_in_guild(): + adapter = _make_adapter(allowed_users=["42"]) + some_guild = SimpleNamespace(id=111, get_member=lambda uid: None) + assert ( + adapter._is_allowed_user( + "42", author=None, guild=some_guild, is_dm=False + ) + is True + ) + + +def test_empty_allowlists_allow_everyone(): + adapter = _make_adapter() + assert ( + adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) + is True + )