From da15b70535ce3fdc5705cd27ed6c16977fd05353 Mon Sep 17 00:00:00 2001 From: houren Antony <2212222@mail.nankai.edu.cn> Date: Sat, 15 Aug 2026 00:25:57 +0800 Subject: [PATCH] fix(channels): reject unsigned webhook POSTs on encryption-configured channels (#401) Closes #392 (uncontroversial part). WeChat (`_handle_message`) and Feishu (`_handle_event`) gated their signature/decryption checks behind a condition the REQUEST controls: - WeChat: `if encrypt and self._crypto:` -- a POST with no `` element took the false branch and reached `_safe_process_message` without any verification, even when `encoding_aes_key` + `token` were configured. - Feishu: `if self.config.encrypt_key and "encrypt" in body:` -- a plaintext body skipped decryption entirely and was processed directly. Since the webhook port is the channel's only inbound boundary, an attacker could POST forged plaintext and reach the agent, spoofing `sender_id` / `FromUserName` (and, with an empty allowlist, passing the sender gate). Fix: when encryption is configured, an inbound POST MUST carry the encrypted field (`` / `encrypt`) -- otherwise it is rejected with 403 and never reaches the agent. Plaintext mode (no encryption configured) is unchanged, so existing plaintext deployments are not affected. The remaining fail-closed question (what to do when credentials are entirely unset) is left for the maintainers to decide as the policy part of the issue. Regression tests (9 new): - WeChat: plaintext rejected / missing Encrypt rejected / bad signature rejected / valid signature decrypts and processes / plaintext still accepted when no crypto. - Feishu: plaintext rejected / non-dict body rejected / encrypted body decrypts and processes / plaintext still accepted when no encrypt_key. 93 tests in the two channel files pass; full suite 3045 passed, 13 skipped; ruff clean. Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com> --- EvoScientist/channels/feishu/channel.py | 14 ++- EvoScientist/channels/wechat/channel.py | 16 +++- tests/test_feishu_channel.py | 92 ++++++++++++++++++ tests/test_wechat_channel.py | 121 ++++++++++++++++++++++++ 4 files changed, 239 insertions(+), 4 deletions(-) diff --git a/EvoScientist/channels/feishu/channel.py b/EvoScientist/channels/feishu/channel.py index 28e3e50..6901488 100644 --- a/EvoScientist/channels/feishu/channel.py +++ b/EvoScientist/channels/feishu/channel.py @@ -828,8 +828,18 @@ class FeishuChannel(Channel, WebhookMixin, TokenMixin): except Exception: return web.Response(status=400) - # ── Decrypt if encrypt_key is configured ── - if self.config.encrypt_key and "encrypt" in body: + # When encryption is configured the inbound POST MUST carry an + # ``encrypt`` field. A plaintext body used to skip decryption and + # reach the agent directly, defeating the encryption setup (issue + # #392). Treat a missing ``encrypt`` field on an + # encryption-configured channel as an authentication failure. + if self.config.encrypt_key: + if not isinstance(body, dict) or "encrypt" not in body: + logger.warning( + "Feishu event rejected: encrypt_key is configured but the " + "body has no 'encrypt' field (possible signature bypass)" + ) + return web.Response(status=403) try: body = self._decrypt_event(body["encrypt"]) except Exception: diff --git a/EvoScientist/channels/wechat/channel.py b/EvoScientist/channels/wechat/channel.py index a39ed4c..a986072 100644 --- a/EvoScientist/channels/wechat/channel.py +++ b/EvoScientist/channels/wechat/channel.py @@ -337,9 +337,21 @@ class WeChatChannel(Channel, WebhookMixin, TokenMixin): logger.info(f"WeChat callback POST received, body length={len(body)}") xml_data = parse_xml(body) - # If encrypted, decrypt first + # If encryption is configured, the inbound POST MUST carry an + # element and a matching msg_signature. An unsigned body + # used to fall through to _safe_process_message and reach the agent + # regardless of credentials, which made the encryption setup + # ineffective (issue #392). Treat a missing on an + # encryption-configured channel as an authentication failure. encrypt = xml_data.get("Encrypt", "") - if encrypt and self._crypto: + if self._crypto: + if not encrypt: + logger.warning( + "WeChat POST rejected: encryption is configured but the " + "body has no element (possible signature bypass)" + ) + return web.Response(status=403) + signature = request.query.get("msg_signature", "") timestamp = request.query.get("timestamp", "") nonce = request.query.get("nonce", "") diff --git a/tests/test_feishu_channel.py b/tests/test_feishu_channel.py index f5f409c..53f6e26 100644 --- a/tests/test_feishu_channel.py +++ b/tests/test_feishu_channel.py @@ -2,6 +2,7 @@ import json import sys +from typing import ClassVar from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -624,3 +625,94 @@ class TestFeishuWebSocketMode: assert channel._main_loop is None assert channel._ws_event_queue is None assert channel._access_token is None + + +# ── Webhook signature bypass regression (issue #392) ────────────── + + +class _FakeFeishuRequest: + """Minimal stand-in for aiohttp.web.Request for _handle_event tests.""" + + def __init__(self, json_body): + self._json = json_body + + async def json(self): + return self._json + + +class TestFeishuWebhookSignatureBypass: + """Regression tests for issue #392: when ``encrypt_key`` is configured, + a plaintext POST (no ``encrypt`` field) must NOT reach the agent.""" + + FORGED_V2_EVENT: ClassVar[dict] = { + "schema": "2.0", + "header": {"event_type": "im.message.receive_v1", "token": ""}, + "event": { + "sender": {"sender_id": {"open_id": "attacker"}, "sender_type": "user"}, + "message": { + "chat_id": "oc_chat", + "message_type": "text", + "message_id": "om_msg", + "content": json.dumps({"text": "forged"}), + }, + }, + } + + def _make_channel_with_encrypt_key(self) -> FeishuChannel: + config = FeishuConfig( + app_id="id", + app_secret="secret", + encrypt_key="my-encrypt-key", + ) + channel = FeishuChannel(config) + channel._running = True + channel._http_client = MagicMock() + channel._access_token = "fake-token" + channel._token_expires = 9999999999 + channel._on_message = AsyncMock() # type: ignore[assignment] + return channel + + def _make_channel_without_encrypt_key(self) -> FeishuChannel: + config = FeishuConfig(app_id="id", app_secret="secret") + channel = FeishuChannel(config) + channel._running = True + channel._http_client = MagicMock() + channel._access_token = "fake-token" + channel._token_expires = 9999999999 + channel._on_message = AsyncMock() # type: ignore[assignment] + return channel + + async def test_plaintext_rejected_when_encrypt_key_configured(self): + """Plaintext POST with no `encrypt` field → 403, agent not reached.""" + channel = self._make_channel_with_encrypt_key() + resp = await channel._handle_event(_FakeFeishuRequest(self.FORGED_V2_EVENT)) + assert resp.status == 403 + channel._on_message.assert_not_called() + + async def test_non_dict_body_rejected_when_encrypt_key_configured(self): + """Defensive: a non-dict JSON body (list/str/etc.) → 403.""" + channel = self._make_channel_with_encrypt_key() + for junk in ([1, 2, 3], "string-body", 42): + resp = await channel._handle_event(_FakeFeishuRequest(junk)) + assert resp.status == 403, f"body={junk!r} should be rejected" + channel._on_message.assert_not_called() + + async def test_encrypted_body_decrypts_and_processes(self): + """A valid encrypted body → 200, agent reached (no behavior change).""" + channel = self._make_channel_with_encrypt_key() + decrypted_event = self.FORGED_V2_EVENT + with patch.object( + FeishuChannel, "_decrypt_event", return_value=decrypted_event + ): + resp = await channel._handle_event( + _FakeFeishuRequest({"encrypt": "encrypted-blob"}) + ) + assert resp.status == 200 + channel._on_message.assert_called_once() + + async def test_plaintext_accepted_when_encrypt_key_not_configured(self): + """No-regression: plaintext mode keeps working when no encrypt_key.""" + channel = self._make_channel_without_encrypt_key() + resp = await channel._handle_event(_FakeFeishuRequest(self.FORGED_V2_EVENT)) + assert resp.status == 200 + channel._on_message.assert_called_once() diff --git a/tests/test_wechat_channel.py b/tests/test_wechat_channel.py index 772c38e..9360a4f 100644 --- a/tests/test_wechat_channel.py +++ b/tests/test_wechat_channel.py @@ -4,6 +4,7 @@ import asyncio import hashlib import time import xml.etree.ElementTree as ET +from unittest.mock import AsyncMock, MagicMock import pytest @@ -442,6 +443,126 @@ class TestMessageProcessing: assert channel._queue.empty() +# ── Webhook signature bypass regression (issue #392) ────────────── + + +class _FakeWeChatRequest: + """Minimal stand-in for aiohttp.web.Request for _handle_message tests.""" + + def __init__(self, text_body: str, query: dict | None = None): + self._text = text_body + self.query = query or {} + + async def text(self) -> str: + return self._text + + +class TestWebhookSignatureBypass: + """Regression tests for issue #392: when encryption is configured, an + unsigned POST must NOT reach the agent — verify before branching, not + inside the branch the request controls.""" + + PLAINTEXT_FORGED_XML = ( + "" + "" + "" + ) + + def _make_channel_with_crypto(self) -> WeChatChannel: + """Channel whose `_crypto` is set, mimicking what start() does when + encoding_aes_key + token are configured. We set _crypto directly to + avoid the network roundtrip in start().""" + config = WeComConfig( + corp_id="corp", + agent_id="1", + secret="s", + token="t", + encoding_aes_key="a" * 43, + ) + channel = WeChatChannel(config, backend="wecom") + channel._crypto = MagicMock() + channel._crypto.verify_signature.return_value = ( + False # default: signature won't match + ) + channel._safe_process_message = AsyncMock() # type: ignore[assignment] + return channel + + def _make_channel_without_crypto(self) -> WeChatChannel: + config = WeComConfig(corp_id="corp", agent_id="1", secret="s") + channel = WeChatChannel(config, backend="wecom") + # _crypto stays None (plaintext mode) + channel._safe_process_message = AsyncMock() # type: ignore[assignment] + return channel + + async def test_plaintext_rejected_when_crypto_configured(self): + """An unsigned POST on an encryption-configured channel must 403.""" + channel = self._make_channel_with_crypto() + resp = await channel._handle_message( + _FakeWeChatRequest(self.PLAINTEXT_FORGED_XML) + ) + assert resp.status == 403 + channel._safe_process_message.assert_not_called() + + async def test_missing_encrypt_rejected_even_with_crypto_present(self): + """Even if the body has other XML, no + crypto set → 403.""" + channel = self._make_channel_with_crypto() + body = "" + resp = await channel._handle_message(_FakeWeChatRequest(body)) + assert resp.status == 403 + channel._safe_process_message.assert_not_called() + + async def test_invalid_signature_rejected(self): + """Encrypted body with wrong signature → 403 (no behavior change).""" + channel = self._make_channel_with_crypto() + body = "" + # crypto.verify_signature returns False by default in _make_channel_with_crypto + resp = await channel._handle_message( + _FakeWeChatRequest( + body, + query={ + "msg_signature": "wrong", + "timestamp": "1", + "nonce": "n", + }, + ) + ) + assert resp.status == 403 + channel._safe_process_message.assert_not_called() + + async def test_valid_signature_decrypts_and_processes(self): + """Encrypted body with valid signature → 200, agent reached.""" + channel = self._make_channel_with_crypto() + channel._crypto.verify_signature.return_value = True + channel._crypto.decrypt.return_value = ( + "" + "" + "", + "user1", + ) + body = "" + resp = await channel._handle_message( + _FakeWeChatRequest( + body, + query={ + "msg_signature": "right", + "timestamp": "1", + "nonce": "n", + }, + ) + ) + assert resp.status == 200 + channel._safe_process_message.assert_called_once() + + async def test_plaintext_accepted_when_crypto_not_configured(self): + """No-regression: plaintext mode (no crypto) keeps working.""" + channel = self._make_channel_without_crypto() + resp = await channel._handle_message( + _FakeWeChatRequest(self.PLAINTEXT_FORGED_XML) + ) + assert resp.status == 200 + channel._safe_process_message.assert_called_once() + + # ── Registration test ─────────────────────────────────────────────