From 7ff48a6291f532474ad30ff2635ccb995451b2ec Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Mon, 22 Jun 2026 13:36:41 +0800 Subject: [PATCH] fix(discord): check pairing store for component button auth Component button interactions (approve/deny, slash confirm, model picker, clarify) were not checking the pairing store for authorization. Users approved via `hermes pairing approve` could send messages and use slash commands (which go through the gateway authz_mixin), but button clicks were rejected because `_component_check_auth` only checked env-var allowlists (DISCORD_ALLOWED_USERS, GATEWAY_ALLOW_ALL_USERS, etc.) and not the pairing store. This was a regression from commit f6f363662 which intentionally made component auth fail-closed when no allowlist is set (security fix for GHSA-mc26-p6fw-7pp6), but did not account for pairing-based auth. Fix: add a `PairingStore.is_approved("discord", uid)` check to `_component_check_auth`, mirroring `authz_mixin._check_authorization`. The pairing store check runs after all allowlist checks, preserving the fail-closed behavior for non-paired, non-allowed users. Fixes #50627 --- plugins/platforms/discord/adapter.py | 26 ++++++++--- tests/gateway/test_discord_component_auth.py | 45 ++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index ca31426cc18..3c07f513b43 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -5709,7 +5709,8 @@ def _component_check_auth( 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. + allowlist, a global user allowlist, an explicit allow-all flag, or + the pairing store (``hermes pairing approve``). Behavior: @@ -5719,6 +5720,7 @@ def _component_check_auth( - role allowlist set + interaction.user has no resolvable ``roles`` attribute (e.g. DM context with a role policy active) -> reject (fail closed) + - user is approved in the pairing store -> allow - otherwise -> reject """ if os.getenv("DISCORD_ALLOW_ALL_USERS", "").strip().lower() in {"true", "1", "yes"}: @@ -5740,11 +5742,13 @@ def _component_check_auth( if user is None: return False + # Resolve user ID once for both allowlist and pairing checks. + try: + uid = str(user.id) + except AttributeError: + uid = "" + if has_users: - try: - uid = str(user.id) - except AttributeError: - uid = "" if "*" in user_set or (uid and uid in user_set): return True @@ -5763,6 +5767,18 @@ def _component_check_auth( if user_role_ids & role_set: return True + # Check pairing store — mirrors ``authz_mixin._check_authorization`` + # so users approved via ``hermes pairing approve`` can interact with + # component buttons even without DISCORD_ALLOWED_USERS set. + if uid: + try: + from gateway.pairing import PairingStore + store = PairingStore() + if store.is_approved("discord", uid): + return True + except Exception: + pass + return False diff --git a/tests/gateway/test_discord_component_auth.py b/tests/gateway/test_discord_component_auth.py index 74d06308618..b82378dcc81 100644 --- a/tests/gateway/test_discord_component_auth.py +++ b/tests/gateway/test_discord_component_auth.py @@ -30,6 +30,8 @@ from plugins.platforms.discord.adapter import ( # noqa: E402 @pytest.fixture(autouse=True) def _clear_component_auth_env(monkeypatch): + from unittest.mock import MagicMock, patch + for name in ( "DISCORD_ALLOW_ALL_USERS", "GATEWAY_ALLOW_ALL_USERS", @@ -37,6 +39,13 @@ def _clear_component_auth_env(monkeypatch): ): monkeypatch.delenv(name, raising=False) + # Default-mock PairingStore so tests don't hit the filesystem. + # Pairing-specific tests override this with explicit mock values. + mock_store = MagicMock() + mock_store.is_approved.return_value = False + with patch("gateway.pairing.PairingStore", return_value=mock_store): + yield + # --------------------------------------------------------------------------- # Direct helper coverage -- the views all delegate to this helper, so @@ -304,3 +313,39 @@ 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 + + +# --------------------------------------------------------------------------- +# Pairing store: users approved via ``hermes pairing approve`` must be +# authorized even without DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES. +# --------------------------------------------------------------------------- + + +def test_component_check_pairing_approved_user_passes(monkeypatch): + """User approved in pairing store passes even without allowlists.""" + from unittest.mock import MagicMock, patch + + mock_store = MagicMock() + mock_store.is_approved.return_value = True + # Override the autouse fixture's mock with approved=True + with patch("gateway.pairing.PairingStore", return_value=mock_store): + interaction = _interaction(11111) + assert _component_check_auth(interaction, set(), set()) is True + mock_store.is_approved.assert_called_once_with("discord", "11111") + + +def test_component_check_pairing_not_approved_user_rejected(monkeypatch): + """User NOT in pairing store is still rejected (fail-closed).""" + # The autouse fixture already mocks is_approved=False, so this + # just verifies the fail-closed path still works. + interaction = _interaction(99999) + assert _component_check_auth(interaction, set(), set()) is False + + +def test_component_check_pairing_import_error_graceful(monkeypatch): + """If PairingStore import fails, fall through to fail-closed.""" + from unittest.mock import patch + + with patch("gateway.pairing.PairingStore", side_effect=ImportError("simulated")): + interaction = _interaction(11111) + assert _component_check_auth(interaction, set(), set()) is False