Slack approval auth fails open without allowlist

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #41226 fix(gateway): fail closed for approval-button auth on Slack, Feishu, Discord when no allowlist set

  merged to main

  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

  Show the diff

  • gateway/platforms/feishu.py

    @@ -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
     
    @@ -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 "<unknown>")
                 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:
    @@ -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

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

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

  • scripts/release.py

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

    @@ -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 @@
     # 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 @@
     )
     
     
    [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.
     # ---------------------------------------------------------------------------
     
     
    @@ -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 ─────────────────────────────────────────────────────────
    @@ -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 @@ 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.
     # ---------------------------------------------------------------------------
     
     
    @@ -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 ""
     
    @@ -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

    @@ -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",
    @@ -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",
    @@ -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",
    @@ -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."""
    @@ -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

    @@ -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
    @@ -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():
    @@ -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
     # ===========================================================================
    @@ -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()
    @@ -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",
    @@ -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",
    @@ -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()
    @@ -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"}
     
    @@ -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