# NousResearch/hermes-agent · Slack approval auth fails open without allowlist ## Open now - Nothing open on GitHub. A security issue where Slack (and related) approval/confirm button handlers grant access to any user when no allowlist is configured, plus associated fixes and cleanup of related dead code. ## TL;DR - 6 member(s): 0 open issue(s), 2 closed issue(s), 0 open PR(s), 1 merged PR(s), 3 PR(s) closed without merging. - A fix was merged to main in PR #41226. Verify it addresses your case. - **Recommended:** read PR #41226 (and any competing PRs below) before filing. Confirm the merged change covers your symptom. ## Maintainer resolution verdict - Internal-tracker verdict over this cluster's issue members: `unresolved`. This is distinct from the fix status above. Weigh it yourself. ## How this cluster was grouped - Link evidence (`STRUCTURAL_SPANNING`): every member is connected by structural evidence: shared changed files, or a closing/superseded reference. - Pairwise link density `0.36`: the share of member pairs that carry a link, counting a structural link in full and a relevance-only link at 0.36. `1.00` means every pair is linked. ## Closed / duplicate issues - #36848: Security: Slack Block-Kit approval handler fails open when SLACK_ALLOWED_USERS is unset (any channel member can Approve-always) - #38068: fix(slack): approval and confirm buttons ignore configured authorization ## Prior & competing PRs - **#41226: fix(gateway): fail closed for approval-button auth on Slack, Feishu, Discord when no allowlist set**. merged to main · fix tier `verified` - maintainer direction: category `merged`; implication `superseded` - files: `gateway/platforms/feishu.py`, `gateway/platforms/slack.py`, `plugins/platforms/discord/adapter.py`, `scripts/release.py`, `tests/gateway/test_discord_component_auth.py`, `tests/gateway/test_feishu_approval_buttons.py`, `tests/gateway/test_slack_approval_buttons.py` - fix hunks: - `gateway/platforms/feishu.py` ```diff @@ -2644,7 +2644,8 @@ def _handle_update_prompt_card_action(self, *, event: Any, action_value: Dict[st if prompt_id is None: logger.debug("[Feishu] Card action missing update_prompt_id, ignoring") return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None - if prompt_id not in self._update_prompt_state: + state = self._update_prompt_state.get(prompt_id) + if not state: logger.debug("[Feishu] Update prompt %s already resolved or unknown", prompt_id) return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None ``` ```diff @@ -2655,12 +2656,33 @@ def _handle_update_prompt_card_action(self, *, event: Any, action_value: Dict[st operator = getattr(event, "operator", None) open_id = str(getattr(operator, "open_id", "") or "") - if not self._is_interactive_operator_authorized(open_id): + sender_id = SimpleNamespace(open_id=open_id, user_id=str(getattr(operator, "user_id", "") or "")) + if not self._allow_group_message(sender_id, state.get("chat_id", ""), is_bot=False): logger.warning("[Feishu] Unauthorized update prompt click by %s", open_id or "") return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None + callback_chat_id = str(getattr(getattr(event, "context", None), "open_chat_id", "") or "") + expected_chat_id = str(state.get("chat_id", "") or "") + if callback_chat_id and expected_chat_id and callback_chat_id != expected_chat_id: + logger.warning( + "[Feishu] Update prompt callback chat mismatch for %s (expected=%s, got=%s)", + prompt_id, + expected_chat_id, + callback_chat_id, + ) + return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None + user_name = self._get_cached_sender_name(open_id) or open_id - if not self._submit_on_loop(loop, self._resolve_update_prompt(prompt_id, answer, user_name)): + if not self._submit_on_loop( + loop, + self._resolve_update_prompt( + prompt_id, + answer, + user_name, + open_id=open_id, + chat_id=callback_chat_id, + ), + ): return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None if P2CardActionTriggerResponse is None: ``` ```diff @@ -2711,12 +2733,38 @@ async def _resolve_approval( except Exception as exc: logger.error("Failed to resolve gateway approval from Feishu button: %s", exc) - async def _resolve_update_prompt(self, prompt_id: Any, answer: str, user_name: str) -> None: + async def _resolve_update_prompt( + self, + prompt_id: Any, + answer: str, + user_name: str, + *, + open_id: str = "", + chat_id: str = "", + ) -> None: """Persist an update prompt answer for the detached update process.""" - state = self._update_prompt_state.pop(prompt_id, None) + state = self._update_prompt_state.get(prompt_id) if not state: logger.debug("[Feishu] Update prompt %s already resolved or unknown", prompt_id) return + if open_id: + sender_id = SimpleNamespace(open_id=open_id, user_id="") + if not self._allow_group_message(sender_id, state.get("chat_id", ""), is_bot=False): + logger.warning("[Feishu] Unauthorized update prompt click by %s for prompt %s", open_id, prompt_id) + return + expected_chat_id = str(state.get("chat_id", "") or "") + if expected_chat_id and chat_id and expected_chat_id != chat_id: + logger.warning( + "[Feishu] Update prompt %s chat mismatch (expected=%s, got=%s)", + prompt_id, + expected_chat_id, + chat_id, + ) + return + state = self._update_prompt_state.pop(prompt_id, None) + if not state: + logger.debug("[Feishu] Update prompt %s already resolved while validating callback", prompt_id) + return try: self._write_update_prompt_response(answer) logger.info( ``` - `gateway/platforms/slack.py` ```diff @@ -2797,6 +2797,55 @@ async def send_slash_confirm( logger.error("[Slack] send_slash_confirm failed: %s", e, exc_info=True) return SendResult(success=False, error=str(e)) + def _is_interactive_user_authorized( + self, + user_id: str, + *, + channel_id: str = "", + user_name: Optional[str] = None, + ) -> bool: + """Return whether a Slack interactive caller may perform gated actions.""" + normalized_user_id = str(user_id or "").strip() + if not normalized_user_id: + return False + + runner = getattr(getattr(self, "_message_handler", None), "__self__", None) + auth_fn = getattr(runner, "_is_user_authorized", None) + if callable(auth_fn): + try: + from gateway.session import SessionSource + + source = SessionSource( + platform=Platform.SLACK, + chat_id=str(channel_id or normalized_user_id), + chat_type="dm" if str(channel_id or "").startswith("D") else "group", + user_id=normalized_user_id, + user_name=str(user_name).strip() if user_name else None, + ) + return bool(auth_fn(source)) + except Exception: + logger.debug( + "[Slack] Falling back to env-only interactive auth for user %s", + normalized_user_id, + exc_info=True, + ) + + if os.getenv("SLACK_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"}: + return True + + allowed_ids = set() + platform_allowlist = os.getenv("SLACK_ALLOWED_USERS", "").strip() + if platform_allowlist: + allowed_ids.update(uid.strip() for uid in platform_allowlist.split(",") if uid.strip()) + global_allowlist = os.getenv("GATEWAY_ALLOWED_USERS", "").strip() + if global_allowlist: + allowed_ids.update(uid.strip() for uid in global_allowlist.split(",") if uid.strip()) + + if allowed_ids: + return "*" in allowed_ids or normalized_user_id in allowed_ids + + return os.getenv("GATEWAY_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"} + async def _handle_slash_confirm_action(self, ack, body, action) -> None: """Handle a slash-confirm button click from Block Kit.""" await ack() ``` ```diff @@ -2808,9 +2857,19 @@ async def _handle_slash_confirm_action(self, ack, body, action) -> None: channel_id = body.get("channel", {}).get("id", "") user_name = body.get("user", {}).get("name", "unknown") user_id = body.get("user", {}).get("id", "") + if not self._is_interactive_user_authorized( + user_id, + channel_id=channel_id, + user_name=user_name, + ): + logger.warning( + "[Slack] Unauthorized slash-confirm click by %s (%s) - ignoring", + user_name, user_id, + ) + return # Authorization — reuse the exec-approval allowlist. - allowed_csv = os.getenv("SLACK_ALLOWED_USERS", "").strip() + allowed_csv = "" # Interactive auth already ran above. if allowed_csv: allowed_ids = {uid.strip() for uid in allowed_csv.split(",") if uid.strip()} if "*" not in allowed_ids and user_id not in allowed_ids: ``` ```diff @@ -2917,10 +2976,21 @@ async def _handle_approval_action(self, ack, body, action) -> None: user_name = body.get("user", {}).get("name", "unknown") user_id = body.get("user", {}).get("id", "") + if not self._is_interactive_user_authorized( + user_id, + channel_id=channel_id, + user_name=user_name, + ): + logger.warning( + "[Slack] Unauthorized approval click by %s (%s) - ignoring", + user_name, user_id, + ) + return + # Only authorized users may click approval buttons. Button clicks # bypass the normal message auth flow in gateway/run.py, so we must # check here as well. - allowed_csv = os.getenv("SLACK_ALLOWED_USERS", "").strip() + allowed_csv = "" # Interactive auth already ran above. if allowed_csv: allowed_ids = {uid.strip() for uid in allowed_csv.split(",") if uid.strip()} if "*" not in allowed_ids and user_id not in allowed_ids: ``` - `plugins/platforms/discord/adapter.py` ```diff @@ -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 ``` ```diff @@ -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: ``` - `scripts/release.py` ```diff @@ -45,6 +45,8 @@ # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { + "[redacted:email]": "Dusk1e", + "[redacted:email]": "LaPhilosophie", "[redacted:email]": "bmoore210", "[redacted:email]": "TheGardenGallery", "[redacted:email]": "randomsnowflake", ``` - `tests/gateway/test_discord_component_auth.py` ```diff @@ -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 ``` ```diff @@ -19,6 +19,7 @@ # 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, ``` ```diff @@ -27,9 +28,19 @@ ) [redacted:email](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. # --------------------------------------------------------------------------- ``` ```diff @@ -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_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 -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).""" [redacted:email]( + ("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 ───────────────────────────────────────────────────────── ``` ```diff @@ -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 ──────────────────────────────────────────── ``` ```diff @@ -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.""" ``` ```diff @@ -196,8 +243,19 @@ async def _noop(*_a, **_k): 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. # --------------------------------------------------------------------------- ``` ```diff @@ -207,14 +265,26 @@ async def _noop(*_a, **_k): 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 "" ``` ```diff @@ -227,4 +297,10 @@ async def _noop(*_a, **_k): 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 ``` - `tests/gateway/test_feishu_approval_buttons.py` ```diff @@ -642,6 +642,7 @@ def test_returns_card_for_update_prompt_yes(self, _patch_callback_card_types): adapter = _make_adapter() adapter._loop = MagicMock() adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_bob"} adapter._update_prompt_state[1] = { "session_key": "sess-up-1", "message_id": "msg_up_003", ``` ```diff @@ -667,6 +668,7 @@ def test_returns_card_for_update_prompt_no(self, _patch_callback_card_types): adapter = _make_adapter() adapter._loop = MagicMock() adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_user1"} adapter._update_prompt_state[2] = { "session_key": "sess-up-2", "message_id": "msg_up_004", ``` ```diff @@ -717,6 +719,7 @@ def test_update_prompt_schedule_failure_returns_no_card(self, _patch_callback_ca adapter = _make_adapter() adapter._loop = MagicMock() adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_user1"} adapter._update_prompt_state[1] = { "session_key": "sess-up-1", "message_id": "msg_up_005", ``` ```diff @@ -754,6 +757,52 @@ def test_update_prompt_unauthorized_operator_returns_no_card(self, _patch_callba assert response.card is None mock_submit.assert_not_called() + def test_update_prompt_empty_allowlists_fail_closed(self, _patch_callback_card_types): + adapter = _make_adapter() + adapter._loop = MagicMock() + adapter._loop.is_closed = MagicMock(return_value=False) + adapter._update_prompt_state[7] = { + "session_key": "sess-up-7", + "message_id": "msg_up_007", + "chat_id": "oc_12345", + } + data = _make_card_action_data( + {"hermes_update_prompt_action": "y", "update_prompt_id": 7}, + open_id="ou_intruder", + ) + + with patch("asyncio.run_coroutine_threadsafe") as mock_submit: + response = adapter._on_card_action_trigger(data) + + assert response is not None + assert response.card is None + assert 7 in adapter._update_prompt_state + mock_submit.assert_not_called() + + def test_update_prompt_chat_mismatch_returns_no_card(self, _patch_callback_card_types): + adapter = _make_adapter() + adapter._loop = MagicMock() + adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_bob"} + adapter._update_prompt_state[8] = { + "session_key": "sess-up-8", + "message_id": "msg_up_008", + "chat_id": "oc_expected", + } + data = _make_card_action_data( + {"hermes_update_prompt_action": "y", "update_prompt_id": 8}, + chat_id="oc_mismatch", + open_id="ou_bob", + ) + + with patch("asyncio.run_coroutine_threadsafe") as mock_submit: + response = adapter._on_card_action_trigger(data) + + assert response is not None + assert response.card is None + assert 8 in adapter._update_prompt_state + mock_submit.assert_not_called() + class TestResolveUpdatePrompt: """Test update prompt resolution persists the response file.""" ``` ```diff @@ -800,3 +849,26 @@ async def test_unknown_prompt_id_drops_silently(self, tmp_path, monkeypatch): await adapter._resolve_update_prompt(99, "n", "Nobody") assert not (tmp_path / ".hermes" / ".update_response").exists() + + @pytest.mark.asyncio + async def test_chat_mismatch_does_not_write_response_file(self, tmp_path, monkeypatch): + adapter = _make_adapter() + adapter._allowed_group_users = {"ou_bob"} + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes").mkdir() + adapter._update_prompt_state[10] = { + "session_key": "sess-up-10", + "message_id": "msg_up_010", + "chat_id": "oc_expected", + } + + await adapter._resolve_update_prompt( + 10, + "y", + "Bob", + open_id="ou_bob", + chat_id="oc_wrong", + ) + + assert not (tmp_path / ".hermes" / ".update_response").exists() + assert 10 in adapter._update_prompt_state ``` - `tests/gateway/test_slack_approval_buttons.py` ```diff @@ -1,5 +1,6 @@ """Tests for Slack Block Kit approval buttons and thread context fetching.""" +import asyncio import sys from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch ``` ```diff @@ -42,7 +43,7 @@ def _ensure_slack_mock(): _ensure_slack_mock() from gateway.platforms.slack import SlackAdapter -from gateway.config import PlatformConfig +from gateway.config import PlatformConfig, Platform def _make_adapter(): ``` ```diff @@ -57,6 +58,25 @@ def _make_adapter(): return adapter +class _AuthRunner: + def __init__(self, auth_fn=None): + self._auth_fn = auth_fn or (lambda _source: True) + self.seen_sources = [] + + async def handle(self, event): + return None + + def _is_user_authorized(self, source): + self.seen_sources.append(source) + return self._auth_fn(source) + + +def _attach_auth_runner(adapter, auth_fn=None): + runner = _AuthRunner(auth_fn=auth_fn) + adapter.set_message_handler(runner.handle) + return runner + + # =========================================================================== # send_exec_approval — Block Kit buttons # =========================================================================== ``` ```diff @@ -153,6 +173,7 @@ class TestSlackApprovalAction: @pytest.mark.asyncio async def test_resolves_approval(self): adapter = _make_adapter() + _attach_auth_runner(adapter) adapter._approval_resolved["1234.5678"] = False ack = AsyncMock() ``` ```diff @@ -165,7 +186,7 @@ async def test_resolves_approval(self): ], }, "channel": {"id": "C1"}, - "user": {"name": "norbert"}, + "user": {"name": "norbert", "id": "U_NORBERT"}, } action = { "action_id": "hermes_approve_once", ``` ```diff @@ -189,13 +210,14 @@ async def test_resolves_approval(self): @pytest.mark.asyncio async def test_prevents_double_click(self): adapter = _make_adapter() + _attach_auth_runner(adapter) adapter._approval_resolved["1234.5678"] = True # Already resolved ack = AsyncMock() body = { "message": {"ts": "1234.5678", "blocks": []}, "channel": {"id": "C1"}, - "user": {"name": "norbert"}, + "user": {"name": "norbert", "id": "U_NORBERT"}, } action = { "action_id": "hermes_approve_once", ``` ```diff @@ -212,6 +234,7 @@ async def test_prevents_double_click(self): @pytest.mark.asyncio async def test_deny_action(self): adapter = _make_adapter() + _attach_auth_runner(adapter) adapter._approval_resolved["1.2"] = False ack = AsyncMock() ``` ```diff @@ -220,7 +243,7 @@ async def test_deny_action(self): {"type": "section", "text": {"type": "mrkdwn", "text": "cmd"}}, ]}, "channel": {"id": "C1"}, - "user": {"name": "alice"}, + "user": {"name": "alice", "id": "U_ALICE"}, } action = {"action_id": "hermes_deny", "value": "session-key"} ``` ```diff @@ -234,6 +257,95 @@ async def test_deny_action(self): update_kwargs = mock_client.chat_update.call_args[1] assert "Denied by alice" in update_kwargs["text"] + @pytest.mark.asyncio + async def test_global_allowlist_blocks_unauthorized_click(self, monkeypatch): + adapter = _make_adapter() + adapter._approval_resolved["1234.5678"] = False + monkeypatch.delenv("SLACK_ALLOWED_USERS", raising=False) + monkeypatch.delenv("SLACK_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOW_ALL_USERS", raising=False) + monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "U_OWNER") + + ack = AsyncMock() + body = { + "message": {"ts": "1234.5678", "blocks": []}, + "channel": {"id": "C1"}, + "user": {"name": "mallory", "id": "U_ATTACKER"}, + } + action = { + "action_id": "hermes_approve_once", + "value": "agent:main:slack:group:C1:1111", + } + + with patch("tools.approval.resolve_gateway_approval") as mock_resolve: + await adapter._handle_approval_action(ack, body, action) + + ack.assert_called_once() + mock_resolve.assert_not_called() + + +class TestSlackInteractiveAuth: + def test_delegates_to_gateway_runner_auth(self): + adapter = _make_adapter() + runner = _attach_auth_runner(adapter, auth_fn=lambda source: source.user_id == "U_OK") + + assert adapter._is_interactive_user_authorized( + "U_OK", + channel_id="C1", + user_name="operator", + ) is True + assert adapter._is_interactive_user_authorized( + "U_BAD", + channel_id="C1", + user_name="intruder", + ) is False + + assert len(runner.seen_sources) == 2 + assert runner.seen_sources[0].platform == Platform.SLACK + assert runner.seen_sources[0].chat_id == "C1" + assert runner.seen_sources[0].chat_type == "group" + + +class TestSlackSlashConfirmAction: + @pytest.mark.asyncio + async def test_global_allowlist_allows_authorized_click(self, monkeypatch): + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.chat_update = AsyncMock() + mock_client.chat_postMessage = AsyncMock() + monkeypatch.delenv("SLACK_ALLOWED_USERS", raising=False) + monkeypatch.delenv("SLACK_ALLOW_ALL_USERS", raising=False) + monkeypatch.delenv("GATEWAY_ALLOW_ALL_USERS", raising=False) + monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "U_OWNER") + + ack = AsyncMock() + body = { + "message": { + "ts": "2222.3333", + "blocks": [ + {"type": "section", "text": {"type": "mrkdwn", "text": "Original prompt"}}, + ], + }, + "channel": {"id": "C1"}, + "user": {"name": "owner", "id": "U_OWNER"}, + } + action = { + "action_id": "hermes_confirm_once", + "value": "agent:main:slack:group:C1:1111|confirm-1", + } + + with patch("tools.slash_confirm.resolve", new=AsyncMock(return_value="follow-up")) as mock_resolve: + await adapter._handle_slash_confirm_action(ack, body, action) + + ack.assert_called_once() + mock_resolve.assert_awaited_once_with( + "agent:main:slack:group:C1:1111", + "confirm-1", + "once", + ) + mock_client.chat_update.assert_called_once() + mock_client.chat_postMessage.assert_called_once() + # =========================================================================== # _fetch_thread_context ``` - **#36861: fix(gateway): Slack approval handler must fail closed when SLACK_ALLOWED_USERS unset**. closed without merge - maintainer direction: category `implemented_elsewhere`; implication `superseded`; refs `a317e5493`, `410cb743b`, `3fa15b33d`, `f6f363662`, `#41226`, `#33844`, `#41226` - **#38069: fix(slack): approval and confirm buttons ignore configured authorization**. closed without merge - maintainer direction: category `superseded_by_pr`; implication `superseded`; refs `#41226` - **#43597: refactor(gateway): remove dead Slack approval allowlist code (#36848)**. closed without merge - maintainer direction: category `uncategorized`; implication `needs_llm` ## Fix status - Merged to main in PR #41226 (cited above). No release data is tracked. Verify downstream. - Fix last verified (merged) `2026-06-07`. - Subsystem: `gateway`