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
This commit is contained in:
@@ -16,7 +16,8 @@ from nanobot.bus.queue import MessageBus
|
|||||||
from nanobot.channels.base import BaseChannel
|
from nanobot.channels.base import BaseChannel
|
||||||
from nanobot.config.paths import get_media_dir
|
from nanobot.config.paths import get_media_dir
|
||||||
from nanobot.config_base import Base
|
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_MAX_MESSAGE_LEN = 16383
|
||||||
MATTERMOST_WS_RECONNECT_BASE_DELAY = 1
|
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 not await self._is_allowed(sender_id, channel_id, channel_type):
|
||||||
if is_dm and self.config.dm.enabled:
|
if is_dm and self.config.dm.enabled:
|
||||||
await self._handle_message(
|
code = generate_code(self.name, str(sender_id))
|
||||||
sender_id=sender_id,
|
await self.send(
|
||||||
chat_id=channel_id,
|
OutboundMessage(
|
||||||
content="",
|
channel=self.name,
|
||||||
is_dm=True,
|
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
|
return
|
||||||
|
|
||||||
@@ -287,7 +295,7 @@ class MattermostChannel(BaseChannel):
|
|||||||
if not sender_id or not channel_id or not value:
|
if not sender_id or not channel_id or not value:
|
||||||
return
|
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):
|
if not await self._is_allowed(sender_id, channel_id, channel_type):
|
||||||
return
|
return
|
||||||
|
|
||||||
@@ -343,10 +351,15 @@ class MattermostChannel(BaseChannel):
|
|||||||
return chat_id in self.config.group_allow_from
|
return chat_id in self.config.group_allow_from
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
_BOT_MENTION_RE: re.Pattern | None = None
|
||||||
|
|
||||||
def _is_mentioned(self, text: str) -> bool:
|
def _is_mentioned(self, text: str) -> bool:
|
||||||
if not self._self_username:
|
if not self._self_username:
|
||||||
return False
|
return False
|
||||||
return f"@{self._self_username}" in text
|
if self._BOT_MENTION_RE is None:
|
||||||
|
pat = r"(?<![@\w])@" + re.escape(self._self_username) + r"(?![@\w])"
|
||||||
|
self._BOT_MENTION_RE = re.compile(pat)
|
||||||
|
return bool(self._BOT_MENTION_RE.search(text))
|
||||||
|
|
||||||
def _strip_bot_mention(self, text: str) -> str:
|
def _strip_bot_mention(self, text: str) -> str:
|
||||||
if not text or not self._self_username:
|
if not text or not self._self_username:
|
||||||
@@ -618,7 +631,7 @@ class MattermostChannel(BaseChannel):
|
|||||||
info_resp.raise_for_status()
|
info_resp.raise_for_status()
|
||||||
info = info_resp.json()
|
info = info_resp.json()
|
||||||
name = Path(info.get("name", file_id)).name
|
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)
|
out.parent.mkdir(parents=True, exist_ok=True)
|
||||||
|
|
||||||
dl = await self._http_client.get(f"/api/v4/files/{file_id}/download")
|
dl = await self._http_client.get(f"/api/v4/files/{file_id}/download")
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ import pytest
|
|||||||
|
|
||||||
from nanobot.bus.events import OutboundMessage
|
from nanobot.bus.events import OutboundMessage
|
||||||
from nanobot.bus.queue import MessageBus
|
from nanobot.bus.queue import MessageBus
|
||||||
|
from nanobot.pairing import PAIRING_CODE_META_KEY
|
||||||
from nanobot.channels.mattermost import (
|
from nanobot.channels.mattermost import (
|
||||||
MATTERMOST_MAX_MESSAGE_LEN,
|
MATTERMOST_MAX_MESSAGE_LEN,
|
||||||
MattermostChannel,
|
MattermostChannel,
|
||||||
@@ -669,6 +670,7 @@ async def test_thread_session_key():
|
|||||||
async def test_action_event():
|
async def test_action_event():
|
||||||
channel, fake = _make_channel()
|
channel, fake = _make_channel()
|
||||||
channel._self_id = "bot_id"
|
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:
|
with patch.object(channel, "_handle_message", AsyncMock()) as mock_handle:
|
||||||
ws_msg = {
|
ws_msg = {
|
||||||
"event": "action",
|
"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
|
# 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
|
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
|
# split_message helper
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user