From f6f363662e91ee1636a0eb67568dc26d2d7831b3 Mon Sep 17 00:00:00 2001 From: LaPhilosophie <804436395@qq.com> Date: Sun, 7 Jun 2026 05:01:39 -0700 Subject: [PATCH] fix(discord): fail closed for component button auth when no allowlist set Salvage of the Discord half of PR #30964 by @LaPhilosophie. Discord component button callbacks (ExecApprovalView, SlashConfirmView, UpdatePromptView, ModelPickerView) bypass the normal message dispatch authorization path. _component_check_auth previously returned True when both the user and role allowlists were empty, so any guild member who could see an approval prompt could click Approve on a dangerous command. Fail closed instead: require DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES / GATEWAY_ALLOWED_USERS membership, or an explicit DISCORD_ALLOW_ALL_USERS / GATEWAY_ALLOW_ALL_USERS opt-in for deliberately-open deployments. Mirrors the Telegram (#24457) and Matrix fail-closed precedent. The Slack half of #30964 is superseded by PR #33844's helper. Reported via GHSA-mc26-p6fw-7pp6 (@whyiug). Co-authored-by: LaPhilosophie <804436395@qq.com> --- plugins/platforms/discord/adapter.py | 37 ++++--- tests/gateway/test_discord_component_auth.py | 110 ++++++++++++++++--- 2 files changed, 112 insertions(+), 35 deletions(-) diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index fa0f81c9b2e..3d97274ea48 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -5188,34 +5188,35 @@ def _component_check_auth( ) -> bool: """Shared user-or-role OR semantics for component view button clicks. - Mirrors ``DiscordAdapter._is_allowed_user`` / the slash and on_message - gates so every Discord interaction surface honors the same trust - boundary. Component views (ExecApprovalView, SlashConfirmView, - UpdatePromptView, ModelPickerView) used to receive only - ``allowed_user_ids``: in role-only deployments - (DISCORD_ALLOWED_ROLES set, DISCORD_ALLOWED_USERS empty) the user - set was empty and the legacy "no allowlist = allow everyone" branch - let any guild member click the buttons -- approving exec commands, - cancelling slash confirmations, switching the model. + Mirrors the gateway's external-surface authorization model: component + button clicks must be explicitly authorized by a Discord user/role + allowlist, a global user allowlist, or an explicit allow-all flag. Behavior: - - both allowlists empty -> allow (preserves existing no-allowlist - deployments, no regression) - - user is in user allowlist -> allow + - DISCORD_ALLOW_ALL_USERS or GATEWAY_ALLOW_ALL_USERS -> allow + - user is in DISCORD_ALLOWED_USERS or GATEWAY_ALLOWED_USERS -> allow - role allowlist set + user has a role in it -> allow - role allowlist set + interaction.user has no resolvable ``roles`` attribute (e.g. DM context with a role policy active) -> reject (fail closed) - otherwise -> reject """ - user_set = allowed_user_ids or set() - role_set = allowed_role_ids or set() - has_users = bool(user_set) - has_roles = bool(role_set) - if not has_users and not has_roles: + if os.getenv("DISCORD_ALLOW_ALL_USERS", "").strip().lower() in {"true", "1", "yes"}: + return True + if os.getenv("GATEWAY_ALLOW_ALL_USERS", "").strip().lower() in {"true", "1", "yes"}: return True + user_set = {str(uid).strip() for uid in (allowed_user_ids or set()) if str(uid).strip()} + global_allowed = { + uid.strip() + for uid in os.getenv("GATEWAY_ALLOWED_USERS", "").split(",") + if uid.strip() + } + user_set.update(global_allowed) + role_set = set(allowed_role_ids or set()) + has_users = bool(user_set) + has_roles = bool(role_set) user = getattr(interaction, "user", None) if user is None: return False @@ -5225,7 +5226,7 @@ def _component_check_auth( uid = str(user.id) except AttributeError: uid = "" - if uid and uid in user_set: + if "*" in user_set or (uid and uid in user_set): return True if has_roles: diff --git a/tests/gateway/test_discord_component_auth.py b/tests/gateway/test_discord_component_auth.py index 95d746b80ee..74d06308618 100644 --- a/tests/gateway/test_discord_component_auth.py +++ b/tests/gateway/test_discord_component_auth.py @@ -1,15 +1,15 @@ -"""Security regression tests: Discord component views honor role allowlists. +"""Security regression tests: Discord component views honor allowlists. -The four interactive component views (ExecApprovalView, SlashConfirmView, -UpdatePromptView, ModelPickerView) historically accepted only +The interactive component views (ExecApprovalView, SlashConfirmView, +UpdatePromptView, ModelPickerView, ClarifyChoiceView) historically accepted only ``allowed_user_ids``. Deployments that configure DISCORD_ALLOWED_ROLES without DISCORD_ALLOWED_USERS therefore had a wide-open component surface: any guild member who could see the prompt could approve exec commands, cancel slash confirmations, or switch the model -- even when the same user would be rejected at the slash and on_message gates. -These tests pin the user-or-role OR semantics and the fail-closed -behavior on missing role data so the parity cannot regress. +These tests pin user/role/global allowlist semantics, explicit allow-all +handling, and fail-closed behavior so the parity cannot regress. """ from types import SimpleNamespace @@ -19,6 +19,7 @@ import pytest # Trigger the shared discord mock from tests/gateway/conftest.py before # importing the production module. from plugins.platforms.discord.adapter import ( # noqa: E402 + ClarifyChoiceView, ExecApprovalView, ModelPickerView, SlashConfirmView, @@ -27,9 +28,19 @@ from plugins.platforms.discord.adapter import ( # noqa: E402 ) +@pytest.fixture(autouse=True) +def _clear_component_auth_env(monkeypatch): + for name in ( + "DISCORD_ALLOW_ALL_USERS", + "GATEWAY_ALLOW_ALL_USERS", + "GATEWAY_ALLOWED_USERS", + ): + monkeypatch.delenv(name, raising=False) + + # --------------------------------------------------------------------------- -# Direct helper coverage -- the four views all delegate to this helper, so -# pinning the helper's contract pins all four call sites. +# Direct helper coverage -- the views all delegate to this helper, so +# pinning the helper's contract pins all call sites. # --------------------------------------------------------------------------- @@ -49,16 +60,30 @@ def _interaction(user_id, role_ids=None, *, drop_user=False, drop_roles=False): return SimpleNamespace(user=SimpleNamespace(**user_kwargs)) -# ── back-compat: empty allowlists -> allow everyone ──────────────────────── +# ── no policy configured -> deny unless allow-all is explicit ────────────── -def test_component_check_empty_allowlists_allows_everyone(): - """SECURITY-CRITICAL backwards-compat: deployments without any - DISCORD_ALLOWED_* env vars set must continue to allow component - interactions from anyone (no regression for unconfigured setups).""" +def test_component_check_empty_allowlists_rejects_by_default(monkeypatch): + """Button interactions must fail closed without an allowlist or allow-all.""" + monkeypatch.delenv("DISCORD_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOWED_USERS", raising=False) + interaction = _interaction(11111) + assert _component_check_auth(interaction, set(), set()) is False + assert _component_check_auth(interaction, None, None) is False + + +@pytest.mark.parametrize( + ("env_name", "env_value"), + [ + ("DISCORD_ALLOW_ALL_USERS", "true"), + ("GATEWAY_ALLOW_ALL_USERS", "yes"), + ], +) +def test_component_check_explicit_allow_all_passes(monkeypatch, env_name, env_value): + monkeypatch.setenv(env_name, env_value) interaction = _interaction(11111) assert _component_check_auth(interaction, set(), set()) is True - assert _component_check_auth(interaction, None, None) is True # ── user allowlist ───────────────────────────────────────────────────────── @@ -74,6 +99,23 @@ def test_component_check_user_not_in_user_allowlist_rejected(): assert _component_check_auth(interaction, {"11111"}, set()) is False +def test_component_check_user_in_global_allowlist_passes(monkeypatch): + monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "11111,22222") + interaction = _interaction(11111) + assert _component_check_auth(interaction, set(), set()) is True + + +def test_component_check_global_allowlist_without_match_rejects(monkeypatch): + monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "22222") + interaction = _interaction(11111) + assert _component_check_auth(interaction, set(), set()) is False + + +def test_component_check_wildcard_user_allowlist_passes(): + interaction = _interaction(99999) + assert _component_check_auth(interaction, {"*"}, set()) is True + + # ── role allowlist OR semantics ──────────────────────────────────────────── @@ -87,6 +129,11 @@ def test_component_check_role_only_user_with_matching_role_passes(): assert _component_check_auth(interaction, set(), {42}) is True +def test_component_check_accepts_role_allowlist_sequences(): + interaction = _interaction(99999, role_ids=[42]) + assert _component_check_auth(interaction, set(), [42]) is True + + def test_component_check_role_only_user_without_matching_role_rejected(): """Role-only deployment where the user has no matching role: reject. Previously this allowed everyone because allowed_user_ids was empty.""" @@ -196,8 +243,19 @@ def test_model_picker_view_accepts_role_allowlist(): assert view._check_auth(_interaction(99999, role_ids=[7])) is False +def test_clarify_choice_view_accepts_role_allowlist(): + view = ClarifyChoiceView( + choices=["one", "two"], + clarify_id="clarify-1", + allowed_user_ids=set(), + allowed_role_ids={42}, + ) + assert view._check_auth(_interaction(99999, role_ids=[42])) is True + assert view._check_auth(_interaction(99999, role_ids=[7])) is False + + # --------------------------------------------------------------------------- -# Empty allowlists across views: legacy "allow everyone" must hold. +# Empty allowlists across views: fail closed unless allow-all is explicit. # --------------------------------------------------------------------------- @@ -207,14 +265,26 @@ def test_model_picker_view_accepts_role_allowlist(): lambda: ExecApprovalView(session_key="s", allowed_user_ids=set()), lambda: SlashConfirmView(session_key="s", confirm_id="c", allowed_user_ids=set()), lambda: UpdatePromptView(session_key="s", allowed_user_ids=set()), + lambda: ClarifyChoiceView( + choices=["one"], + clarify_id="c", + allowed_user_ids=set(), + ), ], ) -def test_views_empty_allowlists_allow_everyone(view_factory): +def test_views_empty_allowlists_reject_by_default(view_factory, monkeypatch): + monkeypatch.delenv("DISCORD_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOWED_USERS", raising=False) view = view_factory() - assert view._check_auth(_interaction(99999)) is True + assert view._check_auth(_interaction(99999)) is False -def test_model_picker_view_empty_allowlists_allow_everyone(): +def test_model_picker_view_empty_allowlists_reject_by_default(monkeypatch): + monkeypatch.delenv("DISCORD_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOWED_USERS", raising=False) + async def _noop(*_a, **_k): return "" @@ -227,4 +297,10 @@ def test_model_picker_view_empty_allowlists_allow_everyone(): allowed_user_ids=set(), ) assert view.allowed_role_ids == set() + assert view._check_auth(_interaction(99999)) is False + + +def test_view_empty_allowlists_allow_with_explicit_allow_all(monkeypatch): + monkeypatch.setenv("DISCORD_ALLOW_ALL_USERS", "true") + view = ExecApprovalView(session_key="s", allowed_user_ids=set()) assert view._check_auth(_interaction(99999)) is True