mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-15 04:12:25 +00:00
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=<guild_id> 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)
This commit is contained in:
parent
8308d18339
commit
ef1e565570
2 changed files with 342 additions and 28 deletions
|
|
@ -701,7 +701,17 @@ class DiscordAdapter(BasePlatformAdapter):
|
||||||
# human-user allowlist below (bots aren't in it).
|
# human-user allowlist below (bots aren't in it).
|
||||||
else:
|
else:
|
||||||
# Non-bot: enforce the configured user/role allowlists.
|
# 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
|
return
|
||||||
|
|
||||||
# Multi-agent filtering: if the message mentions specific bots
|
# Multi-agent filtering: if the message mentions specific bots
|
||||||
|
|
@ -2063,8 +2073,16 @@ class DiscordAdapter(BasePlatformAdapter):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
completed = receiver.check_silence()
|
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:
|
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
|
continue
|
||||||
await self._process_voice_input(guild_id, user_id, pcm_data)
|
await self._process_voice_input(guild_id, user_id, pcm_data)
|
||||||
except asyncio.CancelledError:
|
except asyncio.CancelledError:
|
||||||
|
|
@ -2107,13 +2125,32 @@ class DiscordAdapter(BasePlatformAdapter):
|
||||||
except OSError:
|
except OSError:
|
||||||
pass
|
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.
|
"""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.
|
Uses OR semantics: if the user matches EITHER allowlist, they're allowed.
|
||||||
If both allowlists are empty, everyone is allowed (backwards compatible).
|
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
|
# ``getattr`` fallbacks here guard against test fixtures that build
|
||||||
# an adapter via ``object.__new__(DiscordAdapter)`` and skip __init__
|
# an adapter via ``object.__new__(DiscordAdapter)`` and skip __init__
|
||||||
|
|
@ -2124,31 +2161,54 @@ class DiscordAdapter(BasePlatformAdapter):
|
||||||
has_roles = bool(allowed_roles)
|
has_roles = bool(allowed_roles)
|
||||||
if not has_users and not has_roles:
|
if not has_users and not has_roles:
|
||||||
return True
|
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:
|
if has_users and user_id in allowed_users:
|
||||||
return True
|
return True
|
||||||
# Check role allowlist
|
# Role allowlist is only consulted when configured.
|
||||||
if has_roles:
|
if not has_roles:
|
||||||
# Try direct role check from Member object
|
return False
|
||||||
direct_roles = getattr(author, "roles", None) if author is not None else None
|
|
||||||
if direct_roles:
|
# DM path: roles require explicit opt-in via DISCORD_DM_ROLE_AUTH_GUILD.
|
||||||
if any(getattr(r, "id", None) in allowed_roles for r in direct_roles):
|
# Without this, a user with the configured role in ANY mutual guild
|
||||||
return True
|
# could DM the bot and bypass the allowlist (cross-guild leakage).
|
||||||
# Fallback: scan mutual guilds for member's roles
|
if is_dm or guild is None:
|
||||||
if self._client is not None:
|
dm_guild_env = os.getenv("DISCORD_DM_ROLE_AUTH_GUILD", "").strip()
|
||||||
try:
|
if not dm_guild_env.isdigit():
|
||||||
uid_int = int(user_id)
|
return False
|
||||||
except (TypeError, ValueError):
|
dm_guild_id = int(dm_guild_env)
|
||||||
uid_int = None
|
if self._client is None:
|
||||||
if uid_int is not None:
|
return False
|
||||||
for guild in self._client.guilds:
|
dm_guild = self._client.get_guild(dm_guild_id)
|
||||||
m = guild.get_member(uid_int)
|
if dm_guild is None:
|
||||||
if m is None:
|
return False
|
||||||
continue
|
try:
|
||||||
m_roles = getattr(m, "roles", None) or []
|
uid_int = int(user_id)
|
||||||
if any(getattr(r, "id", None) in allowed_roles for r in m_roles):
|
except (TypeError, ValueError):
|
||||||
return True
|
return False
|
||||||
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 command authorization ─────────────────────────────────────
|
||||||
# Slash commands (``_run_simple_slash`` and ``_handle_thread_create_slash``)
|
# Slash commands (``_run_simple_slash`` and ``_handle_thread_create_slash``)
|
||||||
|
|
|
||||||
254
tests/gateway/test_discord_roles_dm_scope.py
Normal file
254
tests/gateway/test_discord_roles_dm_scope.py
Normal file
|
|
@ -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
|
||||||
|
)
|
||||||
Loading…
Add table
Add a link
Reference in a new issue