From 410cb743bf6f3a7dd0b581f927bff719338a06fa Mon Sep 17 00:00:00 2001 From: Dusk1e Date: Thu, 28 May 2026 14:07:26 +0300 Subject: [PATCH] fix(slack): re-check gateway auth on approval and slash-confirm buttons --- gateway/platforms/slack.py | 74 +++++++++++- tests/gateway/test_slack_approval_buttons.py | 120 ++++++++++++++++++- 2 files changed, 188 insertions(+), 6 deletions(-) diff --git a/gateway/platforms/slack.py b/gateway/platforms/slack.py index 13564f1e6e2..46068ca20ea 100644 --- a/gateway/platforms/slack.py +++ b/gateway/platforms/slack.py @@ -2797,6 +2797,55 @@ class SlackAdapter(BasePlatformAdapter): 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 @@ class SlackAdapter(BasePlatformAdapter): 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 @@ class SlackAdapter(BasePlatformAdapter): 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: diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index 16f991118b8..e09b3406c6d 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/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 @@ class TestSlackApprovalAction: ], }, "channel": {"id": "C1"}, - "user": {"name": "norbert"}, + "user": {"name": "norbert", "id": "U_NORBERT"}, } action = { "action_id": "hermes_approve_once", @@ -189,13 +210,14 @@ class TestSlackApprovalAction: @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 @@ class TestSlackApprovalAction: @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 @@ class TestSlackApprovalAction: {"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 @@ class TestSlackApprovalAction: 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