From f9806cc60f6888843df729c395bf1e2e0a6da08d Mon Sep 17 00:00:00 2001 From: Kenneth Zhao Date: Tue, 23 Jun 2026 10:23:54 +0000 Subject: [PATCH] fix(mattermost): address second round of review feedback - Send pairing code response (not empty message) for denied DMs - Resolve actual channel type in action events before permission check - Use word-boundary regex in _is_mentioned to avoid partial matches - Use safe_filename for download path sanitization - Add tests: denied DM pairing, denied action event, is_mentioned boundary --- nanobot/channels/mattermost.py | 31 ++++++--- tests/channels/test_mattermost_channel.py | 79 +++++++++++++++++++++++ 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/nanobot/channels/mattermost.py b/nanobot/channels/mattermost.py index aed1370a..e2cbc25f 100644 --- a/nanobot/channels/mattermost.py +++ b/nanobot/channels/mattermost.py @@ -16,7 +16,8 @@ from nanobot.bus.queue import MessageBus from nanobot.channels.base import BaseChannel from nanobot.config.paths import get_media_dir from nanobot.config_base import Base -from nanobot.utils.helpers import split_message +from nanobot.pairing import PAIRING_CODE_META_KEY, format_pairing_reply, generate_code +from nanobot.utils.helpers import safe_filename, split_message MATTERMOST_MAX_MESSAGE_LEN = 16383 MATTERMOST_WS_RECONNECT_BASE_DELAY = 1 @@ -217,11 +218,18 @@ class MattermostChannel(BaseChannel): if not await self._is_allowed(sender_id, channel_id, channel_type): if is_dm and self.config.dm.enabled: - await self._handle_message( - sender_id=sender_id, - chat_id=channel_id, - content="", - is_dm=True, + code = generate_code(self.name, str(sender_id)) + await self.send( + OutboundMessage( + channel=self.name, + chat_id=str(channel_id), + content=format_pairing_reply(code), + metadata={PAIRING_CODE_META_KEY: code}, + ) + ) + self.logger.info( + "Sent pairing code {} to sender {} in chat {}", + code, sender_id, channel_id, ) return @@ -287,7 +295,7 @@ class MattermostChannel(BaseChannel): if not sender_id or not channel_id or not value: return - channel_type = self._channel_types.get(channel_id, "public") + channel_type = await self.resolve_channel_type(channel_id) if not await self._is_allowed(sender_id, channel_id, channel_type): return @@ -343,10 +351,15 @@ class MattermostChannel(BaseChannel): return chat_id in self.config.group_allow_from return False + _BOT_MENTION_RE: re.Pattern | None = None + def _is_mentioned(self, text: str) -> bool: if not self._self_username: return False - return f"@{self._self_username}" in text + if self._BOT_MENTION_RE is None: + pat = r"(? str: if not text or not self._self_username: @@ -618,7 +631,7 @@ class MattermostChannel(BaseChannel): info_resp.raise_for_status() info = info_resp.json() name = Path(info.get("name", file_id)).name - out = Path(get_media_dir("mattermost")) / f"{file_id}_{name}" + out = Path(get_media_dir("mattermost")) / safe_filename(f"{file_id}_{name}") out.parent.mkdir(parents=True, exist_ok=True) dl = await self._http_client.get(f"/api/v4/files/{file_id}/download") diff --git a/tests/channels/test_mattermost_channel.py b/tests/channels/test_mattermost_channel.py index c96ee0c3..15c3817b 100644 --- a/tests/channels/test_mattermost_channel.py +++ b/tests/channels/test_mattermost_channel.py @@ -11,6 +11,7 @@ import pytest from nanobot.bus.events import OutboundMessage from nanobot.bus.queue import MessageBus +from nanobot.pairing import PAIRING_CODE_META_KEY from nanobot.channels.mattermost import ( MATTERMOST_MAX_MESSAGE_LEN, MattermostChannel, @@ -669,6 +670,7 @@ async def test_thread_session_key(): async def test_action_event(): channel, fake = _make_channel() channel._self_id = "bot_id" + fake.set_get_response("/api/v4/channels/c1", {"id": "c1", "type": "O"}) with patch.object(channel, "_handle_message", AsyncMock()) as mock_handle: ws_msg = { "event": "action", @@ -688,6 +690,25 @@ async def test_action_event(): ) +@pytest.mark.asyncio +async def test_action_event_denied_dm(): + channel, fake = _make_channel({"dm": {"policy": "allowlist", "allowFrom": ["u_other"]}}) + channel._self_id = "bot_id" + fake.set_get_response("/api/v4/channels/c1", {"id": "c1", "type": "D"}) + with patch.object(channel, "_handle_message", AsyncMock()) as mock_handle: + ws_msg = { + "event": "action", + "data": { + "user_id": "u1", + "channel_id": "c1", + "context": {"selected_option": "Approve"}, + }, + "broadcast": {}, + } + await channel._handle_ws_message(ws_msg) + mock_handle.assert_not_awaited() + + # --------------------------------------------------------------------------- # Post deleted event # --------------------------------------------------------------------------- @@ -742,6 +763,64 @@ async def test_dm_allowlist_with_username_match(): assert await channel._is_allowed("u2", "dm_chan", "dm") is False +# --------------------------------------------------------------------------- +# Denied DM sends pairing code (not empty message) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_denied_dm_sends_pairing_not_empty_inbound(): + channel, fake = _make_channel({"dm": {"policy": "allowlist", "allowFrom": ["u_allowed"]}}) + channel._self_id = "botuserid123" + channel._self_username = "nanobot" + + ws_msg = { + "event": "posted", + "data": { + "channel_type": "D", + "post": json.dumps({ + "id": "p1", "user_id": "u_denied", + "channel_id": "dm_chan", "message": "hello", "root_id": "", + }), + }, + "broadcast": {"channel_id": "dm_chan", "team_id": ""}, + } + + inbound_events = [] + channel.bus.publish_inbound = AsyncMock(side_effect=lambda e: inbound_events.append(e)) + + with patch.object(channel, "send", AsyncMock()) as mock_send: + await channel._handle_ws_message(ws_msg) + mock_send.assert_awaited_once() + sent = mock_send.call_args[0][0] + assert sent.channel == "mattermost" + assert sent.chat_id == "dm_chan" + assert "pairing" in sent.content.lower() or "code" in sent.content.lower() + assert PAIRING_CODE_META_KEY in (sent.metadata or {}) + + assert len(inbound_events) == 0 + + +# --------------------------------------------------------------------------- +# Bot mention boundary +# --------------------------------------------------------------------------- + + +def test_is_mentioned_exact(): + channel, fake = _make_channel({"groupPolicy": "mention"}) + channel._self_username = "nanobot" + assert channel._is_mentioned("hello @nanobot how are you") is True + assert channel._is_mentioned("hello @nanobotty") is False + assert channel._is_mentioned("@nanobot_extra") is False + assert channel._is_mentioned("plain text") is False + + +def test_is_mentioned_no_username(): + channel, fake = _make_channel({"groupPolicy": "mention"}) + channel._self_username = None + assert channel._is_mentioned("hello @nanobot") is False + + # --------------------------------------------------------------------------- # split_message helper # ---------------------------------------------------------------------------