From e5b880264bc5487282ea83983abbb62507c21959 Mon Sep 17 00:00:00 2001 From: Teknium Date: Fri, 17 Apr 2026 05:47:39 -0700 Subject: [PATCH] fix(discord): harden DISCORD_ALLOWED_ROLES and cover gateway layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the cherry-picked PR #9873 (`e3bcc819`): 1. `_is_allowed_user` now uses `getattr(self, '_allowed_*_ids', set())` so test fixtures that build the adapter via `object.__new__` (skipping __init__) don't crash with AttributeError. See AGENTS.md pitfall #17 — same pattern as gateway.run. 2. New 3-case regression coverage in test_discord_bot_auth_bypass.py: - role-only config bypasses the gateway 'no allowlists' branch - roles + users combined still authorizes user-allowlist matches - the role bypass does NOT leak to other platforms (Telegram, etc.) 3. Autouse fixture in test_discord_bot_auth_bypass.py clears all Discord auth env vars before each test so DISCORD_ALLOWED_ROLES leakage from a previous test in the session can't flip later 'should-reject' tests into false-pass. Required because the bare cherry-pick of #9873 only added the adapter- level role check — it didn't cover the gateway-level _is_user_authorized, which still rejected role-only setups via the 'no allowlists configured' branch. --- gateway/platforms/discord.py | 15 ++-- tests/gateway/test_discord_bot_auth_bypass.py | 72 +++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index 640da125f..79e70592e 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -1386,19 +1386,24 @@ class DiscordAdapter(BasePlatformAdapter): When author is a Member, checks .roles directly; otherwise falls back to scanning the bot's mutual guilds for a Member record. """ - has_users = bool(self._allowed_user_ids) - has_roles = bool(self._allowed_role_ids) + # ``getattr`` fallbacks here guard against test fixtures that build + # an adapter via ``object.__new__(DiscordAdapter)`` and skip __init__ + # (see AGENTS.md pitfall #17 — same pattern as gateway.run). + allowed_users = getattr(self, "_allowed_user_ids", set()) + allowed_roles = getattr(self, "_allowed_role_ids", set()) + has_users = bool(allowed_users) + has_roles = bool(allowed_roles) if not has_users and not has_roles: return True # Check user ID allowlist - if has_users and user_id in self._allowed_user_ids: + 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 self._allowed_role_ids for r in 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: @@ -1412,7 +1417,7 @@ class DiscordAdapter(BasePlatformAdapter): if m is None: continue m_roles = getattr(m, "roles", None) or [] - if any(getattr(r, "id", None) in self._allowed_role_ids for r in m_roles): + if any(getattr(r, "id", None) in allowed_roles for r in m_roles): return True return False diff --git a/tests/gateway/test_discord_bot_auth_bypass.py b/tests/gateway/test_discord_bot_auth_bypass.py index 29e6a8899..8ff39a1bf 100644 --- a/tests/gateway/test_discord_bot_auth_bypass.py +++ b/tests/gateway/test_discord_bot_auth_bypass.py @@ -22,6 +22,23 @@ import pytest from gateway.session import Platform, SessionSource +@pytest.fixture(autouse=True) +def _isolate_discord_env(monkeypatch): + """Make every test start with a clean Discord env so prior tests in the + session (or CI setups) can't leak DISCORD_ALLOWED_ROLES / DISCORD_ALLOWED_USERS + / DISCORD_ALLOW_BOTS and silently flip the auth result. + """ + for var in ( + "DISCORD_ALLOW_BOTS", + "DISCORD_ALLOWED_USERS", + "DISCORD_ALLOWED_ROLES", + "DISCORD_ALLOW_ALL_USERS", + "GATEWAY_ALLOW_ALL_USERS", + "GATEWAY_ALLOWED_USERS", + ): + monkeypatch.delenv(var, raising=False) + + # ----------------------------------------------------------------------------- # Gate 2: _is_user_authorized bypasses allowlist for permitted bots # ----------------------------------------------------------------------------- @@ -152,3 +169,58 @@ def test_bot_bypass_does_not_leak_to_other_platforms(monkeypatch): is_bot=True, ) assert runner._is_user_authorized(telegram_bot) is False + + +# ----------------------------------------------------------------------------- +# DISCORD_ALLOWED_ROLES gateway-layer bypass (#7871) +# ----------------------------------------------------------------------------- + + +def test_discord_role_config_bypasses_gateway_allowlist(monkeypatch): + """When DISCORD_ALLOWED_ROLES is set, _is_user_authorized must trust + the adapter's pre-filter and authorize. Without this, role-only setups + (DISCORD_ALLOWED_ROLES populated, DISCORD_ALLOWED_USERS empty) would + hit the 'no allowlists configured' branch and get rejected. + """ + runner = _make_bare_runner() + + monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674") + # Note: DISCORD_ALLOWED_USERS is NOT set — the entire point. + + source = _make_discord_human_source(user_id="999888777") + assert runner._is_user_authorized(source) is True + + +def test_discord_role_config_still_authorizes_alongside_users(monkeypatch): + """Sanity: setting both DISCORD_ALLOWED_ROLES and DISCORD_ALLOWED_USERS + doesn't break the user-id path. Users in the allowlist should still be + authorized even if they don't have a role. (OR semantics.) + """ + runner = _make_bare_runner() + + monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674") + monkeypatch.setenv("DISCORD_ALLOWED_USERS", "100200300") + + # User on the user allowlist, no role → still authorized at gateway + # level via the role bypass (adapter already approved them). + source = _make_discord_human_source(user_id="100200300") + assert runner._is_user_authorized(source) is True + + +def test_discord_role_bypass_does_not_leak_to_other_platforms(monkeypatch): + """DISCORD_ALLOWED_ROLES must only affect Discord. Setting it should + not suddenly start authorizing Telegram users whose platform has its + own empty allowlist. + """ + runner = _make_bare_runner() + + monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674") + # Telegram has its own empty allowlist and no allow-all flag. + + telegram_user = SessionSource( + platform=Platform.TELEGRAM, + chat_id="123", + chat_type="channel", + user_id="999888777", + ) + assert runner._is_user_authorized(telegram_user) is False