From f44c272f96c002d5577cdbe3b44a9a87e56961b7 Mon Sep 17 00:00:00 2001 From: 0xyg3n Date: Sat, 18 Apr 2026 11:51:49 +0000 Subject: [PATCH 1/3] fix(discord): scope DISCORD_ALLOWED_ROLES to originating guild (CVSS 8.1) The initial DISCORD_ALLOWED_ROLES implementation (#11608, merged from #9873) scans every mutual guild when resolving a user's roles. This allows a cross-guild DM bypass: 1. Bot is in both public server A and private server B. 2. User holds the allowed role in server A only. 3. User DMs the bot. The role check finds the role in A and authorizes the DM, granting access as if the user were trusted in server B. Fix: - DMs (no guild context) disable role-based auth by default. Opt-in via DISCORD_DM_ROLE_AUTH_GUILD= restricts role lookup to one explicitly-trusted guild. - Guild messages check roles only in the originating guild (message.guild), never in other mutual guilds. - Reject cached author.roles when the Member came from a different guild than the current message. Backwards compatibility: - DISCORD_ALLOWED_USERS behavior is unchanged (still works in both DMs and guild messages). - Deployments that rely on roles in guild channels continue to work; role checks are now strictly scoped to that guild. - Deployments that intentionally want role-based DM auth can opt into a single trusted guild via DISCORD_DM_ROLE_AUTH_GUILD. Tests: 9 new regression guards in tests/gateway/test_discord_roles_dm_scope.py covering the bypass path, the opt-in path, cross-guild guild-message bypass, and backwards-compat user-ID paths. 47/47 discord-auth tests pass. Refs: #11608 (initial implementation), #7871 (feature request), #9873 (PR author credit @0xyg3n) --- gateway/platforms/discord.py | 116 +++++++-- tests/gateway/test_discord_roles_dm_scope.py | 254 +++++++++++++++++++ 2 files changed, 342 insertions(+), 28 deletions(-) create mode 100644 tests/gateway/test_discord_roles_dm_scope.py diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index ecce8b8fc0f9d..0f2b0bbad6ed0 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -701,7 +701,17 @@ async def on_message(message: DiscordMessage): # 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 @@ -2063,8 +2073,16 @@ async def _voice_listen_loop(self, guild_id: int): 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: @@ -2107,13 +2125,32 @@ async def _process_voice_input(self, guild_id: int, user_id: int, pcm_data: byte 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__ @@ -2124,31 +2161,54 @@ def _is_allowed_user(self, user_id: str, author=None) -> bool: 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) # ── Slash command authorization ───────────────────────────────────── # Slash commands (``_run_simple_slash`` and ``_handle_thread_create_slash``) 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 0000000000000..a8c8561164ac1 --- /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 + ) From d5bea736453226c561f7e26099fc75fff055cc3d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 7 May 2026 05:43:55 -0700 Subject: [PATCH 2/3] fix(discord): extend role-scope fix to slash surface + fixture update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sibling-site fix: _evaluate_slash_authorization was the fourth _is_allowed_user caller and didn't pass guild/is_dm through, so slash interactions would take the DM branch regardless of whether they came from a guild channel. Now reads interaction.guild + in_dm and forwards. Also updates test_discord_slash_auth fixture (_make_interaction) so the SimpleNamespace guild mock has a get_member(uid)->None method — required by the new guild-scoped fallback path in _is_allowed_user. Tests exercising positive role paths still work via user.roles. Three new regression tests in test_discord_roles_dm_scope: - Slash DM + role in mutual public guild → rejected - Slash in guild B + role only in guild A → rejected - Slash in guild B + role in guild B → allowed (positive control) 368 Discord tests pass. test_discord_free_channel_skips_auto_thread also fails on clean main (pre-existing, unrelated to this fix). --- gateway/platforms/discord.py | 11 ++- tests/gateway/test_discord_roles_dm_scope.py | 90 ++++++++++++++++++++ tests/gateway/test_discord_slash_auth.py | 6 +- 3 files changed, 105 insertions(+), 2 deletions(-) diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index 0f2b0bbad6ed0..c5b12e09c1357 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -2305,7 +2305,16 @@ def _evaluate_slash_authorization( return (True, None) user_id = str(user.id) - if not self._is_allowed_user(user_id, author=user): + # Pass guild + is_dm so role check is scoped to the originating + # guild and cross-guild DM bypass (#12136) can't land via the + # slash surface either. + interaction_guild = getattr(interaction, "guild", None) + if not self._is_allowed_user( + user_id, + author=user, + guild=interaction_guild, + is_dm=in_dm, + ): return ( False, "user not in DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES", diff --git a/tests/gateway/test_discord_roles_dm_scope.py b/tests/gateway/test_discord_roles_dm_scope.py index a8c8561164ac1..604b4e0aab50f 100644 --- a/tests/gateway/test_discord_roles_dm_scope.py +++ b/tests/gateway/test_discord_roles_dm_scope.py @@ -252,3 +252,93 @@ def test_empty_allowlists_allow_everyone(): adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) is True ) + + +# --------------------------------------------------------------------------- +# Slash-surface sibling site: _evaluate_slash_authorization must pass +# guild/is_dm through so the cross-guild bypass can't land via slash either. +# --------------------------------------------------------------------------- + + +def test_slash_authorization_rejects_cross_guild_role_dm(monkeypatch): + """Slash interaction in a DM must not be authorized by a role held in + any mutual guild (parallel to the on_message cross-guild bypass).""" + import discord as _discord # type: ignore + monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + + public_guild, _ = _guild_with_member( + guild_id=111111, + member_id=42, + role_ids=[5555], + ) + adapter = _make_adapter( + allowed_roles=[5555], + guilds=[public_guild], + ) + + # Fake a DM interaction: user is Member-like, channel is DMChannel, + # interaction.guild is None. + interaction = SimpleNamespace( + user=SimpleNamespace(id=42), + channel=MagicMock(spec=_discord.DMChannel), + channel_id=None, + guild=None, + ) + + allowed, reason = adapter._evaluate_slash_authorization(interaction) + assert allowed is False + assert "ALLOWED" in (reason or "") + + +def test_slash_authorization_rejects_cross_guild_role_in_guild(monkeypatch): + """Slash in guild B must not be authorized by a role held in guild A.""" + monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + + public_guild, _ = _guild_with_member( + guild_id=111111, + member_id=42, + role_ids=[5555], + ) + # Interaction 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], + ) + + interaction = SimpleNamespace( + user=SimpleNamespace(id=42), + channel=SimpleNamespace(id=9999), # not a DMChannel instance + channel_id=9999, + guild=trusted_guild, + ) + + allowed, reason = adapter._evaluate_slash_authorization(interaction) + assert allowed is False + assert "ALLOWED" in (reason or "") + + +def test_slash_authorization_allows_in_scope_guild_role(monkeypatch): + """Positive control: slash in guild B, user has role in guild B → 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], + ) + + interaction = SimpleNamespace( + user=SimpleNamespace(id=42), + channel=SimpleNamespace(id=9999), + channel_id=9999, + guild=trusted_guild, + ) + + allowed, reason = adapter._evaluate_slash_authorization(interaction) + assert allowed is True + assert reason is None diff --git a/tests/gateway/test_discord_slash_auth.py b/tests/gateway/test_discord_slash_auth.py index a52ee1fd7e6a8..e51f240e3aa57 100644 --- a/tests/gateway/test_discord_slash_auth.py +++ b/tests/gateway/test_discord_slash_auth.py @@ -158,7 +158,11 @@ def _make_interaction( return SimpleNamespace( user=user_obj, - guild=SimpleNamespace(owner_id=999), + # `get_member` needed for the guild-scoped role fallback path in + # _is_allowed_user after the #12136 cross-guild fix. Fixture guild + # has no members by default — tests exercising positive role paths + # assign their own Member via user.roles + matching allowed_role_ids. + guild=SimpleNamespace(owner_id=999, id=guild_id, get_member=lambda uid: None), guild_id=guild_id, channel_id=channel_id, channel=channel, From 18cd6ffef139442d689627602f6448882ac6c510 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 7 May 2026 05:51:18 -0700 Subject: [PATCH 3/3] fix(discord): route DM role-auth opt-in through config.yaml (not env var) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per repo policy, ~/.hermes/.env is for secrets only. Guild IDs are behavioral configuration, not secrets. Replacing the DISCORD_DM_ROLE_AUTH_GUILD env var from the original fix with discord.dm_role_auth_guild in config.yaml. - New module-level _read_dm_role_auth_guild() helper reads hermes_cli.config.read_raw_config()['discord']['dm_role_auth_guild']. Fails closed on any parse error (safe default = DM role-auth off). - DEFAULT_CONFIG['discord'] gains dm_role_auth_guild: '' with a comment documenting the opt-in. - Tests patch hermes_cli.config.read_raw_config directly (via the _set_dm_role_auth_guild helper) instead of setenv/delenv. 12 tests in test_discord_roles_dm_scope pass; no env var involvement. - Docstring + module docstring + comments updated to reference discord.dm_role_auth_guild. - E2E verified with real imports across 6 scenarios: unset, int, string, garbage, zero, and (crucially) env-var-only-no-config all return None except the valid int/string cases. Env var has zero effect — policy compliance confirmed. --- gateway/platforms/discord.py | 50 +++++++++++++++----- hermes_cli/config.py | 6 +++ tests/gateway/test_discord_roles_dm_scope.py | 37 ++++++++++----- 3 files changed, 69 insertions(+), 24 deletions(-) diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index c5b12e09c1357..ae107cdfb2b11 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -477,6 +477,34 @@ def pcm_to_wav(pcm_data: bytes, output_path: str, pass +def _read_dm_role_auth_guild() -> Optional[int]: + """Return the guild ID opted-in for DM role-based auth, or None. + + Reads ``discord.dm_role_auth_guild`` from config.yaml. This is + deliberately a config.yaml-only setting (not an env var): per repo + policy, ``~/.hermes/.env`` is for secrets only, and this is a + behavioral setting. Guild IDs aren't secrets. + + Accepts ints or numeric strings in the config. Anything else + (empty, malformed, None) returns None, which keeps the secure + default (DM role-auth disabled). + """ + try: + from hermes_cli.config import read_raw_config + cfg = read_raw_config() or {} + discord_cfg = cfg.get("discord", {}) or {} + raw = discord_cfg.get("dm_role_auth_guild") + except Exception: + return None + if raw is None or raw == "": + return None + try: + guild_id = int(raw) + except (TypeError, ValueError): + return None + return guild_id if guild_id > 0 else None + + class DiscordAdapter(BasePlatformAdapter): """ Discord bot adapter. @@ -2140,11 +2168,11 @@ def _is_allowed_user( 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. + only user-ID allowlist applies. Set ``discord.dm_role_auth_guild`` + in config.yaml 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. @@ -2168,14 +2196,14 @@ def _is_allowed_user( 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). + # DM path: roles require explicit opt-in via + # ``discord.dm_role_auth_guild`` in config.yaml. 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(): + dm_guild_id = _read_dm_role_auth_guild() + if dm_guild_id is None: 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) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 6753ae3de0da6..9db661a27e5ed 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1108,6 +1108,12 @@ def _ensure_hermes_home_managed(home: Path): "auto_thread": True, # Auto-create threads on @mention in channels (like Slack) "reactions": True, # Add 👀/✅/❌ reactions to messages during processing "channel_prompts": {}, # Per-channel ephemeral system prompts (forum parents apply to child threads) + # Opt-in DM role-based auth (#12136). By default, DISCORD_ALLOWED_ROLES + # authorizes only guild messages in the role's own guild — DMs require + # DISCORD_ALLOWED_USERS. Set dm_role_auth_guild to a guild ID to also + # authorize DMs from members of that one trusted guild holding the + # allowed role. Unset / empty / 0 = secure default (DM role-auth off). + "dm_role_auth_guild": "", # discord / discord_admin tools: restrict which actions the agent may call. # Default (empty) = all actions allowed (subject to bot privileged intents). # Accepts comma-separated string ("list_guilds,list_channels,fetch_messages") diff --git a/tests/gateway/test_discord_roles_dm_scope.py b/tests/gateway/test_discord_roles_dm_scope.py index 604b4e0aab50f..0f10ba79ae1fa 100644 --- a/tests/gateway/test_discord_roles_dm_scope.py +++ b/tests/gateway/test_discord_roles_dm_scope.py @@ -11,8 +11,8 @@ 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. +auth on DMs unless ``discord.dm_role_auth_guild`` in config.yaml explicitly +opts into a single trusted guild. """ from types import SimpleNamespace @@ -23,6 +23,17 @@ from gateway.platforms.discord import DiscordAdapter +def _set_dm_role_auth_guild(monkeypatch, guild_id=None): + """Stub ``hermes_cli.config.read_raw_config`` so ``_read_dm_role_auth_guild`` + resolves to ``guild_id`` (or None for the opt-out default). + """ + cfg = {"discord": {"dm_role_auth_guild": guild_id if guild_id is not None else ""}} + # Patch the attribute ``hermes_cli.config.read_raw_config`` — that's + # what ``_read_dm_role_auth_guild`` imports at call time. + import hermes_cli.config as _cfg_mod + monkeypatch.setattr(_cfg_mod, "read_raw_config", lambda: cfg, raising=True) + + def _make_adapter(allowed_users=None, allowed_roles=None, guilds=None): """Build a minimal DiscordAdapter without running __init__.""" adapter = object.__new__(DiscordAdapter) @@ -69,7 +80,7 @@ def test_dm_rejects_role_held_in_other_guild(monkeypatch): Regression guard for the cross-guild DM bypass in the initial DISCORD_ALLOWED_ROLES implementation. """ - monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + _set_dm_role_auth_guild(monkeypatch) public_guild, _ = _guild_with_member( guild_id=111111, @@ -91,7 +102,7 @@ def test_dm_rejects_role_held_in_other_guild(monkeypatch): def test_dm_role_auth_requires_explicit_guild_optin(monkeypatch): - """With DISCORD_DM_ROLE_AUTH_GUILD set, only that specific guild counts. + """With dm_role_auth_guild set, only that specific guild counts. The user has the role in the opted-in guild — allowed. """ @@ -106,7 +117,7 @@ def test_dm_role_auth_requires_explicit_guild_optin(monkeypatch): allowed_roles=[5555], guilds=[other_guild, trusted_guild], ) - monkeypatch.setenv("DISCORD_DM_ROLE_AUTH_GUILD", "222222") + _set_dm_role_auth_guild(monkeypatch, 222222) assert ( adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) @@ -115,7 +126,7 @@ def test_dm_role_auth_requires_explicit_guild_optin(monkeypatch): def test_dm_role_auth_optin_rejects_when_not_member(monkeypatch): - """DISCORD_DM_ROLE_AUTH_GUILD set but user isn't a member → reject.""" + """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 @@ -129,7 +140,7 @@ def test_dm_role_auth_optin_rejects_when_not_member(monkeypatch): allowed_roles=[5555], guilds=[public_guild, trusted_guild], ) - monkeypatch.setenv("DISCORD_DM_ROLE_AUTH_GUILD", "222222") + _set_dm_role_auth_guild(monkeypatch, 222222) assert ( adapter._is_allowed_user("42", author=None, guild=None, is_dm=True) @@ -146,7 +157,7 @@ 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) + _set_dm_role_auth_guild(monkeypatch) public_guild, _ = _guild_with_member( guild_id=111111, @@ -172,7 +183,7 @@ def test_guild_message_role_check_scoped_to_originating_guild(monkeypatch): 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) + _set_dm_role_auth_guild(monkeypatch) trusted_guild, _ = _guild_with_member( guild_id=222222, @@ -197,7 +208,7 @@ def test_guild_message_rejects_author_roles_from_different_guild(monkeypatch): 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) + _set_dm_role_auth_guild(monkeypatch) # Author is a Member of a DIFFERENT guild with the allowed role foreign_guild = SimpleNamespace(id=999, get_member=lambda uid: None) @@ -264,7 +275,7 @@ def test_slash_authorization_rejects_cross_guild_role_dm(monkeypatch): """Slash interaction in a DM must not be authorized by a role held in any mutual guild (parallel to the on_message cross-guild bypass).""" import discord as _discord # type: ignore - monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + _set_dm_role_auth_guild(monkeypatch) public_guild, _ = _guild_with_member( guild_id=111111, @@ -292,7 +303,7 @@ def test_slash_authorization_rejects_cross_guild_role_dm(monkeypatch): def test_slash_authorization_rejects_cross_guild_role_in_guild(monkeypatch): """Slash in guild B must not be authorized by a role held in guild A.""" - monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + _set_dm_role_auth_guild(monkeypatch) public_guild, _ = _guild_with_member( guild_id=111111, @@ -320,7 +331,7 @@ def test_slash_authorization_rejects_cross_guild_role_in_guild(monkeypatch): def test_slash_authorization_allows_in_scope_guild_role(monkeypatch): """Positive control: slash in guild B, user has role in guild B → allowed.""" - monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False) + _set_dm_role_auth_guild(monkeypatch) trusted_guild, _ = _guild_with_member( guild_id=222222,