fix(channels): reject unauthorized inbound before side effects
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
committed by
Xubin Ren
co-authored by
Cursor
parent
1813fc5021
commit
4db50f2e32
@@ -1,14 +1,13 @@
|
||||
from email.message import EmailMessage
|
||||
from datetime import date
|
||||
from pathlib import Path
|
||||
import imaplib
|
||||
from datetime import date
|
||||
from email.message import EmailMessage
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from nanobot.bus.events import OutboundMessage
|
||||
from nanobot.bus.queue import MessageBus
|
||||
from nanobot.channels.email import EmailChannel
|
||||
from nanobot.channels.email import EmailConfig
|
||||
from nanobot.channels.email import EmailChannel, EmailConfig
|
||||
|
||||
|
||||
def _make_config(**overrides) -> EmailConfig:
|
||||
@@ -24,6 +23,7 @@ def _make_config(**overrides) -> EmailConfig:
|
||||
smtp_username="bot@example.com",
|
||||
smtp_password="secret",
|
||||
mark_seen=True,
|
||||
allow_from=["*"],
|
||||
# Disable auth verification by default so existing tests are unaffected
|
||||
verify_dkim=False,
|
||||
verify_spf=False,
|
||||
@@ -707,8 +707,8 @@ def test_email_content_tagged_with_email_context(monkeypatch) -> None:
|
||||
|
||||
def test_check_authentication_results_method() -> None:
|
||||
"""Unit test for the _check_authentication_results static method."""
|
||||
from email.parser import BytesParser
|
||||
from email import policy
|
||||
from email.parser import BytesParser
|
||||
|
||||
# No Authentication-Results header
|
||||
msg_no_auth = EmailMessage()
|
||||
@@ -788,6 +788,32 @@ def _make_raw_email_with_attachment(
|
||||
return msg.as_bytes()
|
||||
|
||||
|
||||
def test_fetch_new_messages_ignores_unauthorized_sender_before_attachments(monkeypatch) -> None:
|
||||
raw = _make_raw_email_with_attachment(from_addr="blocked@example.com")
|
||||
fake = _make_fake_imap(raw)
|
||||
monkeypatch.setattr("nanobot.channels.email.imaplib.IMAP4_SSL", lambda _h, _p: fake)
|
||||
|
||||
called = {"attachments": False}
|
||||
|
||||
def _extract_attachments(*_args, **_kwargs):
|
||||
called["attachments"] = True
|
||||
return []
|
||||
|
||||
monkeypatch.setattr(EmailChannel, "_extract_attachments", _extract_attachments)
|
||||
|
||||
cfg = _make_config(
|
||||
allow_from=["allowed@example.com"],
|
||||
allowed_attachment_types=["application/pdf"],
|
||||
verify_dkim=False,
|
||||
verify_spf=False,
|
||||
)
|
||||
channel = EmailChannel(cfg, MessageBus())
|
||||
|
||||
assert channel._fetch_new_messages() == []
|
||||
assert called["attachments"] is False
|
||||
assert fake.store_calls == [(b"1", "+FLAGS", "\\Seen")]
|
||||
|
||||
|
||||
def test_extract_attachments_saves_pdf(tmp_path, monkeypatch) -> None:
|
||||
"""PDF attachment is saved to media dir and path returned in media list."""
|
||||
monkeypatch.setattr("nanobot.channels.email.get_media_dir", lambda ch: tmp_path)
|
||||
|
||||
@@ -806,3 +806,26 @@ def test_on_background_task_done_removes_from_set() -> None:
|
||||
loop.close()
|
||||
|
||||
assert task not in channel._background_tasks
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_on_message_ignores_unauthorized_sender_before_side_effects() -> None:
|
||||
channel = _make_feishu_channel(group_policy="open")
|
||||
channel.config.allow_from = ["ou_allowed"]
|
||||
channel._add_reaction = AsyncMock()
|
||||
channel._download_and_save_media = AsyncMock(return_value=("/tmp/audio.ogg", "[audio]"))
|
||||
channel.transcribe_audio = AsyncMock(return_value="transcript")
|
||||
channel._handle_message = AsyncMock()
|
||||
|
||||
event = _make_feishu_event(
|
||||
msg_type="audio",
|
||||
content='{"file_key": "file_1"}',
|
||||
sender_open_id="ou_blocked",
|
||||
)
|
||||
|
||||
await channel._on_message(event)
|
||||
|
||||
channel._add_reaction.assert_not_awaited()
|
||||
channel._download_and_save_media.assert_not_awaited()
|
||||
channel.transcribe_audio.assert_not_awaited()
|
||||
channel._handle_message.assert_not_awaited()
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
"""Tests for QQ channel media support: helpers, send, inbound, and upload."""
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -182,6 +182,35 @@ async def test_send_media_failure_falls_back_to_text() -> None:
|
||||
assert "bad.png" in failure_calls[0]["content"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_on_message_ignores_unauthorized_sender_before_attachments_and_ack() -> None:
|
||||
channel = QQChannel(
|
||||
QQConfig(
|
||||
app_id="app",
|
||||
secret="secret",
|
||||
allow_from=["allowed-user"],
|
||||
ack_message="Processing...",
|
||||
),
|
||||
MessageBus(),
|
||||
)
|
||||
channel._client = _FakeClient()
|
||||
channel._handle_attachments = AsyncMock(return_value=(["/tmp/a.png"], ["file"], []))
|
||||
channel._handle_message = AsyncMock()
|
||||
|
||||
data = SimpleNamespace(
|
||||
id="msg-blocked",
|
||||
content="hello",
|
||||
author=SimpleNamespace(user_openid="blocked-user"),
|
||||
attachments=[SimpleNamespace(filename="a.png")],
|
||||
)
|
||||
|
||||
await channel._on_message(data, is_group=False)
|
||||
|
||||
channel._handle_attachments.assert_not_awaited()
|
||||
channel._handle_message.assert_not_awaited()
|
||||
assert channel._client.api.c2c_calls == []
|
||||
|
||||
|
||||
# ── _on_message() exception handling ────────────────────────────────
|
||||
|
||||
|
||||
|
||||
@@ -1802,3 +1802,32 @@ async def test_send_uses_native_keyboard_when_flag_on() -> None:
|
||||
sent = channel._app.bot.sent_messages[0]
|
||||
assert isinstance(sent.get("reply_markup"), InlineKeyboardMarkup)
|
||||
assert "[Yes]" not in sent["text"] # native keyboard owns the rendering
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_callback_query_ignores_unauthorized_user_before_side_effects() -> None:
|
||||
channel = TelegramChannel(
|
||||
TelegramConfig(enabled=True, token="123:abc", allow_from=["999"], inline_keyboards=True),
|
||||
MessageBus(),
|
||||
)
|
||||
channel._handle_message = AsyncMock()
|
||||
|
||||
query = SimpleNamespace(
|
||||
id="cb_1",
|
||||
data="Yes",
|
||||
answer=AsyncMock(),
|
||||
message=SimpleNamespace(
|
||||
chat_id=123,
|
||||
edit_reply_markup=AsyncMock(),
|
||||
),
|
||||
)
|
||||
update = SimpleNamespace(
|
||||
callback_query=query,
|
||||
effective_user=SimpleNamespace(id=12345, username="alice", first_name="Alice"),
|
||||
)
|
||||
|
||||
await channel._on_callback_query(update, None)
|
||||
|
||||
query.answer.assert_not_awaited()
|
||||
query.message.edit_reply_markup.assert_not_awaited()
|
||||
channel._handle_message.assert_not_awaited()
|
||||
|
||||
@@ -3,7 +3,6 @@
|
||||
import os
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
@@ -451,6 +450,39 @@ async def test_process_text_message() -> None:
|
||||
assert msg.metadata["msg_type"] == "text"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_enter_chat_ignores_unauthorized_user_before_welcome() -> None:
|
||||
channel = WecomChannel(WecomConfig(bot_id="b", secret="s", allow_from=["allowed"]), MessageBus())
|
||||
client = _FakeWeComClient()
|
||||
channel._client = client
|
||||
channel.config.welcome_message = "hello"
|
||||
|
||||
await channel._on_enter_chat(_FakeFrame(body={"chatid": "blocked"}))
|
||||
|
||||
client.reply_welcome.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_message_ignores_unauthorized_sender_before_download() -> None:
|
||||
channel = WecomChannel(WecomConfig(bot_id="b", secret="s", allow_from=["allowed"]), MessageBus())
|
||||
client = _FakeWeComClient()
|
||||
channel._client = client
|
||||
channel._handle_message = AsyncMock()
|
||||
|
||||
frame = _FakeFrame(body={
|
||||
"msgid": "msg_blocked",
|
||||
"chatid": "chat1",
|
||||
"from": {"userid": "blocked"},
|
||||
"image": {"url": "https://example.com/img.png", "aeskey": "key123"},
|
||||
})
|
||||
|
||||
await channel._process_message(frame, "image")
|
||||
|
||||
client.download_file.assert_not_awaited()
|
||||
channel._handle_message.assert_not_awaited()
|
||||
assert channel.bus.inbound_size == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_image_message() -> None:
|
||||
"""Image message: download success → media_paths non-empty."""
|
||||
|
||||
@@ -5,8 +5,8 @@ from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
import nanobot.channels.weixin as weixin_mod
|
||||
from nanobot.bus.queue import MessageBus
|
||||
@@ -15,10 +15,10 @@ from nanobot.channels.weixin import (
|
||||
ITEM_TEXT,
|
||||
MESSAGE_TYPE_BOT,
|
||||
WEIXIN_CHANNEL_VERSION,
|
||||
_decrypt_aes_ecb,
|
||||
_encrypt_aes_ecb,
|
||||
WeixinChannel,
|
||||
WeixinConfig,
|
||||
_decrypt_aes_ecb,
|
||||
_encrypt_aes_ecb,
|
||||
)
|
||||
|
||||
|
||||
@@ -128,6 +128,34 @@ async def test_process_message_caches_context_token_and_send_uses_it() -> None:
|
||||
channel._send_text.assert_awaited_once_with("wx-user", "pong", "ctx-2")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_message_ignores_unauthorized_sender_before_side_effects(tmp_path) -> None:
|
||||
bus = MessageBus()
|
||||
channel = WeixinChannel(
|
||||
WeixinConfig(enabled=True, allow_from=["allowed-user"], state_dir=str(tmp_path)),
|
||||
bus,
|
||||
)
|
||||
channel._download_media_item = AsyncMock(return_value="/tmp/test.jpg")
|
||||
channel._start_typing = AsyncMock()
|
||||
|
||||
await channel._process_message(
|
||||
{
|
||||
"message_type": 1,
|
||||
"message_id": "m-unauthorized",
|
||||
"from_user_id": "blocked-user",
|
||||
"context_token": "ctx-blocked",
|
||||
"item_list": [
|
||||
{"type": ITEM_IMAGE, "image_item": {"media": {"encrypt_query_param": "x"}}},
|
||||
],
|
||||
}
|
||||
)
|
||||
|
||||
assert channel._context_tokens == {}
|
||||
channel._download_media_item.assert_not_awaited()
|
||||
channel._start_typing.assert_not_awaited()
|
||||
assert bus.inbound_size == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_message_persists_context_token_to_state_file(tmp_path) -> None:
|
||||
bus = MessageBus()
|
||||
|
||||
@@ -116,7 +116,7 @@ async def test_send_when_disconnected_is_noop():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_group_policy_mention_skips_unmentioned_group_message():
|
||||
ch = WhatsAppChannel({"enabled": True, "groupPolicy": "mention"}, MagicMock())
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["*"], "groupPolicy": "mention"}, MagicMock())
|
||||
ch._handle_message = AsyncMock()
|
||||
|
||||
await ch._handle_bridge_message(
|
||||
@@ -139,7 +139,7 @@ async def test_group_policy_mention_skips_unmentioned_group_message():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_group_policy_mention_accepts_mentioned_group_message():
|
||||
ch = WhatsAppChannel({"enabled": True, "groupPolicy": "mention"}, MagicMock())
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["*"], "groupPolicy": "mention"}, MagicMock())
|
||||
ch._handle_message = AsyncMock()
|
||||
|
||||
await ch._handle_bridge_message(
|
||||
@@ -166,7 +166,7 @@ async def test_group_policy_mention_accepts_mentioned_group_message():
|
||||
@pytest.mark.asyncio
|
||||
async def test_sender_id_prefers_phone_jid_over_lid():
|
||||
"""sender_id should resolve to phone number when @s.whatsapp.net JID is present."""
|
||||
ch = WhatsAppChannel({"enabled": True}, MagicMock())
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["*"]}, MagicMock())
|
||||
ch._handle_message = AsyncMock()
|
||||
|
||||
await ch._handle_bridge_message(
|
||||
@@ -187,7 +187,7 @@ async def test_sender_id_prefers_phone_jid_over_lid():
|
||||
@pytest.mark.asyncio
|
||||
async def test_lid_to_phone_cache_resolves_lid_only_messages():
|
||||
"""When only LID is present, a cached LID→phone mapping should be used."""
|
||||
ch = WhatsAppChannel({"enabled": True}, MagicMock())
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["*"]}, MagicMock())
|
||||
ch._handle_message = AsyncMock()
|
||||
|
||||
# First message: both phone and LID → builds cache
|
||||
@@ -220,7 +220,7 @@ async def test_lid_to_phone_cache_resolves_lid_only_messages():
|
||||
@pytest.mark.asyncio
|
||||
async def test_voice_message_transcription_uses_media_path():
|
||||
"""Voice messages are transcribed when media path is available."""
|
||||
ch = WhatsAppChannel({"enabled": True}, MagicMock())
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["*"]}, MagicMock())
|
||||
ch.transcription_provider = "openai"
|
||||
ch.transcription_api_key = "sk-test"
|
||||
ch._handle_message = AsyncMock()
|
||||
@@ -243,10 +243,32 @@ async def test_voice_message_transcription_uses_media_path():
|
||||
assert kwargs["content"].startswith("Hello world")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unauthorized_voice_message_does_not_transcribe() -> None:
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["allowed"]}, MagicMock())
|
||||
ch._handle_message = AsyncMock()
|
||||
ch.transcribe_audio = AsyncMock(return_value="Hello world")
|
||||
|
||||
await ch._handle_bridge_message(
|
||||
json.dumps({
|
||||
"type": "message",
|
||||
"id": "v-blocked",
|
||||
"sender": "blocked@s.whatsapp.net",
|
||||
"pn": "",
|
||||
"content": "[Voice Message]",
|
||||
"timestamp": 1,
|
||||
"media": ["/tmp/voice.ogg"],
|
||||
})
|
||||
)
|
||||
|
||||
ch.transcribe_audio.assert_not_awaited()
|
||||
ch._handle_message.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_voice_message_no_media_shows_not_available():
|
||||
"""Voice messages without media produce a fallback placeholder."""
|
||||
ch = WhatsAppChannel({"enabled": True}, MagicMock())
|
||||
ch = WhatsAppChannel({"enabled": True, "allowFrom": ["*"]}, MagicMock())
|
||||
ch._handle_message = AsyncMock()
|
||||
|
||||
await ch._handle_bridge_message(
|
||||
|
||||
Reference in New Issue
Block a user