From a89706e4cb6c1bd580171e33fa1bb9ad9b7e1685 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Mon, 31 Aug 2026 09:44:13 +0800 Subject: [PATCH] fix(feishu): gate approval/update-prompt card clicks on operator allowlist, not group policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The synchronous card-action handlers and the update-prompt resolver authorized clicks with _allow_group_message(), which answers "may this sender chat in this group?" — with group_policy=open it returns True for everyone. The approval resolver already used the correct operator gate (_is_interactive_operator_authorized), so the three code paths disagreed: with an open group policy an out-of-allowlist click on an update-prompt card was fully executed, and approval clicks returned a resolved-looking card before being rejected asynchronously. Authorize all three paths with _is_interactive_operator_authorized(), which checks membership of admins ∪ allowed_group_users (wildcard and the empty pairing-mode allowlist keep their existing allow semantics, matching _admit's DM pairing default). A missing operator identity now fails closed on the update-prompt resolver instead of skipping the check. Fixes #96045 --- plugins/platforms/feishu/adapter.py | 14 ++- tests/gateway/test_feishu_approval_buttons.py | 86 ++++++++++++++++++- 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/plugins/platforms/feishu/adapter.py b/plugins/platforms/feishu/adapter.py index be20f1a3df..153f2d4d0c 100644 --- a/plugins/platforms/feishu/adapter.py +++ b/plugins/platforms/feishu/adapter.py @@ -2815,8 +2815,7 @@ class FeishuAdapter(BasePlatformAdapter): operator = getattr(event, "operator", None) open_id = str(getattr(operator, "open_id", "") or "") - 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): + if not self._is_interactive_operator_authorized(open_id): logger.warning("[Feishu] Unauthorized approval click by %s", open_id or "") return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None @@ -2875,8 +2874,7 @@ class FeishuAdapter(BasePlatformAdapter): operator = getattr(event, "operator", None) open_id = str(getattr(operator, "open_id", "") or "") - 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): + if not self._is_interactive_operator_authorized(open_id): logger.warning("[Feishu] Unauthorized update prompt click by %s", open_id or "") return P2CardActionTriggerResponse() if P2CardActionTriggerResponse else None @@ -2982,11 +2980,9 @@ class FeishuAdapter(BasePlatformAdapter): 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 + if not self._is_interactive_operator_authorized(open_id): + 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( diff --git a/tests/gateway/test_feishu_approval_buttons.py b/tests/gateway/test_feishu_approval_buttons.py index 6238f0dc4c..daf71c7bb6 100644 --- a/tests/gateway/test_feishu_approval_buttons.py +++ b/tests/gateway/test_feishu_approval_buttons.py @@ -378,6 +378,30 @@ class TestCardActionCallbackResponse: assert response.card is None mock_submit.assert_not_called() + def test_rejects_approval_click_when_group_policy_open(self, _patch_callback_card_types): + adapter = _make_adapter() + adapter._loop = MagicMock() + adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_allowed"} + adapter._group_policy = "open" + adapter._default_group_policy = "open" + adapter._approval_state[6] = { + "session_key": "sess-6", + "message_id": "msg-6", + "chat_id": "oc_12345", + } + data = _make_card_action_data( + {"hermes_action": "approve_once", "approval_id": 6}, + open_id="ou_attacker", + ) + + 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 + mock_submit.assert_not_called() + def test_update_prompt_unauthorized_operator_returns_no_card(self, _patch_callback_card_types): adapter = _make_adapter() @@ -401,6 +425,30 @@ class TestCardActionCallbackResponse: assert response.card is None mock_submit.assert_not_called() + def test_update_prompt_unauthorized_click_rejected_when_group_policy_open(self, _patch_callback_card_types): + adapter = _make_adapter() + adapter._loop = MagicMock() + adapter._loop.is_closed = MagicMock(return_value=False) + adapter._allowed_group_users = {"ou_allowed"} + adapter._group_policy = "open" + adapter._default_group_policy = "open" + 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 + mock_submit.assert_not_called() + def test_update_prompt_chat_mismatch_returns_no_card(self, _patch_callback_card_types): adapter = _make_adapter() @@ -441,9 +489,45 @@ class TestResolveUpdatePrompt: "chat_id": "oc_12345", } - await adapter._resolve_update_prompt(1, "y", "Alice") + await adapter._resolve_update_prompt(1, "y", "Alice", open_id="ou_user1", chat_id="oc_12345") assert (tmp_path / ".hermes" / ".update_response").read_text() == "y" assert 1 not in adapter._update_prompt_state + @pytest.mark.asyncio + async def test_unauthorized_operator_does_not_write_response(self, tmp_path, monkeypatch): + adapter = _make_adapter() + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes").mkdir() + adapter._allowed_group_users = {"ou_allowed"} + adapter._group_policy = "open" + adapter._default_group_policy = "open" + adapter._update_prompt_state[2] = { + "session_key": "sess-up-2", + "message_id": "msg_up_004", + "chat_id": "oc_12345", + } + + await adapter._resolve_update_prompt(2, "y", "Mallory", open_id="ou_intruder", chat_id="oc_12345") + + assert not (tmp_path / ".hermes" / ".update_response").exists() + assert 2 in adapter._update_prompt_state + + @pytest.mark.asyncio + async def test_missing_operator_identity_does_not_write_response(self, tmp_path, monkeypatch): + adapter = _make_adapter() + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes").mkdir() + adapter._allowed_group_users = {"ou_allowed"} + adapter._update_prompt_state[3] = { + "session_key": "sess-up-3", + "message_id": "msg_up_005", + "chat_id": "oc_12345", + } + + await adapter._resolve_update_prompt(3, "y", "Anonymous", open_id="", chat_id="oc_12345") + + assert not (tmp_path / ".hermes" / ".update_response").exists() + assert 3 in adapter._update_prompt_state +