From 7120f1decc4cfe951572df2ecd596133b91440a7 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 22 May 2026 21:23:17 -0700 Subject: [PATCH] fix(feishu): enforce auth and chat binding for approval buttons --- gateway/platforms/feishu.py | 39 ++++++++++++++++-- tests/gateway/test_feishu_approval_buttons.py | 41 ++++++++++++++++--- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/gateway/platforms/feishu.py b/gateway/platforms/feishu.py index a9b0447080de..bf41b5d930f9 100644 --- a/gateway/platforms/feishu.py +++ b/gateway/platforms/feishu.py @@ -2569,7 +2569,18 @@ def _handle_approval_card_action(self, *, event: Any, action_value: Dict[str, An open_id = str(getattr(operator, "open_id", "") or "") user_name = self._get_cached_sender_name(open_id) or open_id - if not self._submit_on_loop(loop, self._resolve_approval(approval_id, choice, user_name)): + chat_context = getattr(event, "context", None) + chat_id = str(getattr(chat_context, "open_chat_id", "") or "") + if not self._submit_on_loop( + loop, + self._resolve_approval( + approval_id=approval_id, + choice=choice, + user_name=user_name, + open_id=open_id, + chat_id=chat_id, + ), + ): return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None if P2CardActionTriggerResponse is None: @@ -2617,12 +2628,34 @@ def _handle_update_prompt_card_action(self, *, event: Any, action_value: Dict[st response.card = card return response - async def _resolve_approval(self, approval_id: Any, choice: str, user_name: str) -> None: + async def _resolve_approval( + self, + approval_id: Any, + choice: str, + user_name: str, + *, + open_id: str = "", + chat_id: str = "", + ) -> None: """Pop approval state and unblock the waiting agent thread.""" - state = self._approval_state.pop(approval_id, None) + state = self._approval_state.get(approval_id) if not state: logger.debug("[Feishu] Approval %s already resolved or unknown", approval_id) return + if not self._is_interactive_operator_authorized(open_id): + logger.warning("[Feishu] Unauthorized approval click by %s for approval %s", open_id or "", approval_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] Approval %s chat mismatch (expected=%s, got=%s)", + approval_id, expected_chat_id, chat_id, + ) + return + state = self._approval_state.pop(approval_id, None) + if not state: + logger.debug("[Feishu] Approval %s already resolved while validating callback", approval_id) + return try: from tools.approval import resolve_gateway_approval count = resolve_gateway_approval(state["session_key"], choice) diff --git a/tests/gateway/test_feishu_approval_buttons.py b/tests/gateway/test_feishu_approval_buttons.py index 8af56913c10c..673eac5d370f 100644 --- a/tests/gateway/test_feishu_approval_buttons.py +++ b/tests/gateway/test_feishu_approval_buttons.py @@ -320,7 +320,7 @@ async def test_resolves_once(self): } with patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve: - await adapter._resolve_approval(1, "once", "Norbert") + await adapter._resolve_approval(1, "once", "Norbert", open_id="ou_user1", chat_id="oc_12345") mock_resolve.assert_called_once_with("agent:main:feishu:group:oc_12345", "once") assert 1 not in adapter._approval_state @@ -335,7 +335,7 @@ async def test_resolves_deny(self): } with patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve: - await adapter._resolve_approval(2, "deny", "Alice") + await adapter._resolve_approval(2, "deny", "Alice", open_id="ou_user1", chat_id="oc_12345") mock_resolve.assert_called_once_with("some-session", "deny") @@ -349,7 +349,7 @@ async def test_resolves_session(self): } with patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve: - await adapter._resolve_approval(3, "session", "Bob") + await adapter._resolve_approval(3, "session", "Bob", open_id="ou_user1", chat_id="oc_99") mock_resolve.assert_called_once_with("sess-3", "session") @@ -363,7 +363,7 @@ async def test_resolves_always(self): } with patch("tools.approval.resolve_gateway_approval", return_value=1) as mock_resolve: - await adapter._resolve_approval(4, "always", "Carol") + await adapter._resolve_approval(4, "always", "Carol", open_id="ou_user1", chat_id="oc_55") mock_resolve.assert_called_once_with("sess-4", "always") @@ -372,10 +372,41 @@ async def test_already_resolved_drops_silently(self): adapter = _make_adapter() with patch("tools.approval.resolve_gateway_approval") as mock_resolve: - await adapter._resolve_approval(99, "once", "Nobody") + await adapter._resolve_approval(99, "once", "Nobody", open_id="ou_user1", chat_id="oc_12345") mock_resolve.assert_not_called() + @pytest.mark.asyncio + async def test_unauthorized_click_does_not_resolve(self): + adapter = _make_adapter() + adapter._admins = {"ou_admin"} + adapter._approval_state[5] = { + "session_key": "sess-5", + "message_id": "msg_005", + "chat_id": "oc_12345", + } + + with patch("tools.approval.resolve_gateway_approval") as mock_resolve: + await adapter._resolve_approval(5, "once", "Mallory", open_id="ou_intruder", chat_id="oc_12345") + + mock_resolve.assert_not_called() + assert 5 in adapter._approval_state + + @pytest.mark.asyncio + async def test_chat_mismatch_does_not_resolve(self): + adapter = _make_adapter() + adapter._approval_state[6] = { + "session_key": "sess-6", + "message_id": "msg_006", + "chat_id": "oc_expected", + } + + with patch("tools.approval.resolve_gateway_approval") as mock_resolve: + await adapter._resolve_approval(6, "session", "Norbert", open_id="ou_user1", chat_id="oc_wrong") + + mock_resolve.assert_not_called() + assert 6 in adapter._approval_state + # =========================================================================== # _handle_card_action_event — non-approval card actions # ===========================================================================