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

@@ -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:

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