diff --git a/README.md b/README.md index 03baafab01..698640fcd7 100644 --- a/README.md +++ b/README.md @@ -782,6 +782,14 @@ DeerFlow can also expose user-owned IM channel connections in the workspace UI. **Configuration in `config.yaml`:** +Discord's `channels.discord.allowed_guilds` accepts one positive numeric guild +ID (quoted or unquoted) or a YAML list. Unset, `null`, `[]`, or a blank string +allows all guilds. Invalid entries are ignored with a warning; any other +configured value yielding no valid ID denies every guild and logs an error. +`allowed_channels` accepts one channel ID (quoted or unquoted) or a YAML list +of IDs exempt from `mention_only`, within allowed guilds. An empty value gives +no exemptions, so `mention_only` applies everywhere when enabled. + ```yaml channels: # LangGraph-compatible Gateway API base URL (default: http://localhost:8001/api) diff --git a/backend/app/channels/discord.py b/backend/app/channels/discord.py index 4c23f5b01b..17c8a72f26 100644 --- a/backend/app/channels/discord.py +++ b/backend/app/channels/discord.py @@ -34,16 +34,71 @@ DISCORD_OUTBOUND_TIMEOUT_SECONDS = 30.0 DISCORD_UPLOAD_TIMEOUT_SECONDS = 120.0 +def _parse_discord_guild_id(entry: Any) -> int | None: + """Return a Discord guild (snowflake) ID, or ``None`` if ``entry`` is not one.""" + if isinstance(entry, int) and not isinstance(entry, bool) and entry > 0: + return entry + if isinstance(entry, str): + text = entry.strip() + if text.isascii() and text.isdigit(): + return int(text) or None + return None + + +def _parse_allowed_guilds(allowed_guilds: Any) -> frozenset[int] | None: + """Parse ``channels.discord.allowed_guilds``; ``None`` means no allowlist. + + A single ID is shorthand for a one-entry list, the form + ``config.example.yaml`` advertises. Iterating a scalar instead turns + ``"321"`` into guilds 1, 2 and 3 and raises ``TypeError`` on an unquoted + YAML int. Entries that are not positive numeric snowflakes are dropped with + a warning. If none remain, the result is an empty set that denies every + guild: the operator asked for a restriction, so an unreadable one must not + open the bot to every server it joined. + """ + if allowed_guilds is None or (isinstance(allowed_guilds, str) and not allowed_guilds.strip()): + return None + entries = list(allowed_guilds) if isinstance(allowed_guilds, list | tuple | set) else [allowed_guilds] + if not entries: + return None + guild_ids: set[int] = set() + for entry in entries: + guild_id = _parse_discord_guild_id(entry) + if guild_id is None: + logger.warning("[Discord] Ignoring allowed_guilds entry %r: expected a positive numeric guild ID; list several IDs as a YAML list", entry) + else: + guild_ids.add(guild_id) + if not guild_ids: + logger.error("[Discord] allowed_guilds has no valid numeric guild ID; denying every guild until it is fixed") + return frozenset(guild_ids) + + +def _parse_allowed_channels(allowed_channels: Any) -> frozenset[str]: + """Parse ``channels.discord.allowed_channels``; a single ID is one entry. + + Channel IDs are snowflakes that only ever get compared as strings here, so + the entries are kept verbatim rather than validated — an unreadable ID just + never matches, which leaves ``mention_only`` in force (fail-closed). + """ + if allowed_channels is None or (isinstance(allowed_channels, str) and not allowed_channels.strip()): + return frozenset() + entries = list(allowed_channels) if isinstance(allowed_channels, list | tuple | set) else [allowed_channels] + return frozenset(text for text in (str(entry).strip() for entry in entries) if text) + + class DiscordChannel(Channel): """Discord bot channel. Configuration keys (in ``config.yaml`` under ``channels.discord``): - ``bot_token``: Discord Bot token. - - ``allowed_guilds``: (optional) List of allowed Discord guild IDs. Empty = allow all. + - ``allowed_guilds``: (optional) List of allowed Discord guild IDs, or a + single ID. Empty = allow all; a non-empty list with no valid ID denies + every guild. - ``mention_only``: (optional) If true, only respond when the bot is mentioned. - ``allowed_channels``: (optional) List of channel IDs where messages are always accepted - (even when mention_only is true). Use for channels where you want the bot to respond - without mentions. Empty = mention_only applies everywhere. + (even when mention_only is true), or a single ID. Use for channels where + you want the bot to respond without mentions. Empty = mention_only applies + everywhere. - ``thread_mode``: (optional) If true, group a channel conversation into a thread. Default: same as ``mention_only``. """ @@ -51,17 +106,10 @@ class DiscordChannel(Channel): def __init__(self, bus: MessageBus, config: dict[str, Any]) -> None: super().__init__(name="discord", bus=bus, config=config) self._bot_token = str(config.get("bot_token", "")).strip() - self._allowed_guilds: set[int] = set() - for guild_id in config.get("allowed_guilds", []): - try: - self._allowed_guilds.add(int(guild_id)) - except (TypeError, ValueError): - continue + self._allowed_guilds: frozenset[int] | None = _parse_allowed_guilds(config.get("allowed_guilds")) self._mention_only: bool = bool(config.get("mention_only", False)) self._thread_mode: bool = config.get("thread_mode", self._mention_only) - self._allowed_channels: set[str] = set() - for channel_id in config.get("allowed_channels", []): - self._allowed_channels.add(str(channel_id)) + self._allowed_channels: frozenset[str] = _parse_allowed_channels(config.get("allowed_channels")) # Session tracking: channel_id -> Discord thread_id (in-memory, persisted to JSON). # Uses a dedicated JSON file separate from ChannelStore, which maps IM @@ -487,6 +535,11 @@ class DiscordChannel(Channel): task.add_done_callback(self._on_ack_reaction_task_done) return task + def _check_guild(self, guild) -> bool: + if self._allowed_guilds is None: + return True + return guild is not None and guild.id in self._allowed_guilds + async def _on_message(self, message) -> None: if not self._running or not self._client: return @@ -498,9 +551,8 @@ class DiscordChannel(Channel): return guild = message.guild - if self._allowed_guilds: - if guild is None or guild.id not in self._allowed_guilds: - return + if not self._check_guild(guild): + return text = (message.content or "").strip() if not text: diff --git a/backend/tests/test_discord_channel.py b/backend/tests/test_discord_channel.py index 3a6869ef74..d75f91dd6f 100644 --- a/backend/tests/test_discord_channel.py +++ b/backend/tests/test_discord_channel.py @@ -11,7 +11,7 @@ import sys import threading import weakref from types import SimpleNamespace -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -787,3 +787,115 @@ async def test_send_control_calls_keep_the_default_outbound_bound() -> None: assert len(run_mock.await_args_list) == 2 # stop_typing + one text chunk for call in run_mock.await_args_list: assert "timeout" not in call.kwargs + + +# --------------------------------------------------------------------------- +# allowed_guilds / allowed_channels parsing +# --------------------------------------------------------------------------- + + +class TestDiscordAllowedGuilds: + """A restriction the operator configured must never silently open the bot.""" + + @staticmethod + def _channel(config_extra: dict) -> DiscordChannel: + return DiscordChannel(bus=MessageBus(), config={"bot_token": "token", **config_extra}) + + @pytest.mark.parametrize("config_extra", [{}, {"allowed_guilds": None}, {"allowed_guilds": []}, {"allowed_guilds": " "}]) + def test_unset_or_empty_allowlist_allows_every_guild_without_warning(self, config_extra, caplog): + with caplog.at_level(logging.WARNING, logger="app.channels.discord"): + channel = self._channel(config_extra) + + assert channel._check_guild(SimpleNamespace(id=321)) + assert channel._check_guild(None) + assert caplog.records == [] + + def test_listed_guild_ids_are_allowed_and_others_denied(self): + channel = self._channel({"allowed_guilds": [321, "654"]}) + + assert channel._check_guild(SimpleNamespace(id=321)) + assert channel._check_guild(SimpleNamespace(id=654)) + assert not channel._check_guild(SimpleNamespace(id=1)) + assert not channel._check_guild(None) + + @pytest.mark.parametrize("allowed_guilds", ["321", 321], ids=["str", "int"]) + def test_scalar_guild_id_is_one_entry_not_its_digits(self, allowed_guilds): + # config.example.yaml sells this shorthand ("can also be a single guild + # ID"). Iterating the string "321" admitted guilds 1, 2 and 3 while + # blocking 321; an unquoted YAML int raised TypeError in the constructor. + channel = self._channel({"allowed_guilds": allowed_guilds}) + + assert channel._check_guild(SimpleNamespace(id=321)) + assert not channel._check_guild(SimpleNamespace(id=3)) + + @pytest.mark.parametrize("bad_entry", ["@acme", "321,654", 0, -5, None, 4.2, True]) + def test_unparseable_entry_is_dropped_with_warning(self, bad_entry, caplog): + with caplog.at_level(logging.WARNING, logger="app.channels.discord"): + channel = self._channel({"allowed_guilds": [321, bad_entry]}) + + assert channel._check_guild(SimpleNamespace(id=321)) + assert not channel._check_guild(SimpleNamespace(id=1)) + assert repr(bad_entry) in caplog.text + # 0, -5 and True are numeric, so the hint has to say what they lack. + assert "positive numeric guild ID" in caplog.text + + @pytest.mark.parametrize( + "allowed_guilds", + # "321,654" is what a $ENV reference to a comma-separated value resolves + # to; the dict reaches the constructor from the runtime channel config. + [["@acme", "server"], "@acme", "321,654", [True], [4.2], {"id": 321}], + ) + def test_allowlist_without_a_parseable_entry_denies_every_guild(self, allowed_guilds, caplog): + with caplog.at_level(logging.WARNING, logger="app.channels.discord"): + channel = self._channel({"allowed_guilds": allowed_guilds}) + + assert not channel._check_guild(SimpleNamespace(id=321)) + assert not channel._check_guild(SimpleNamespace(id=3)) + assert any(record.levelno == logging.ERROR for record in caplog.records) + + @pytest.mark.asyncio + @pytest.mark.parametrize(("allowed_guilds", "admitted"), [("321", True), ("322", False)]) + async def test_on_message_applies_the_allowlist(self, allowed_guilds, admitted): + # _make_discord_message reports guild ID 321 — the documented scalar. + channel = self._channel({"allowed_guilds": allowed_guilds}) + channel._running = True + channel._client = SimpleNamespace(user=SimpleNamespace(id=999, mention="<@999>")) + channel._discord_module = SimpleNamespace(Thread=type("FakeThread", (), {})) + channel._main_loop = asyncio.get_running_loop() + # A refused capacity slot ends the hand-off here, so nothing is stubbed + # beyond it: the assertion is purely "did the gate let the message in". + channel._reserve_inbound = MagicMock(return_value=None) + + await channel._on_message(_make_discord_message("hello")) + + assert channel._reserve_inbound.called is admitted + + +class TestDiscordAllowedChannels: + """``allowed_channels`` is parsed by the same lines, so it breaks the same way.""" + + @staticmethod + def _channel(config_extra: dict) -> DiscordChannel: + return DiscordChannel(bus=MessageBus(), config={"bot_token": "token", "mention_only": True, "thread_mode": False, **config_extra}) + + @pytest.mark.parametrize("allowed_channels", ["456", 456], ids=["str", "int"]) + def test_scalar_channel_id_is_one_entry_not_its_digits(self, allowed_channels): + channel = self._channel({"allowed_channels": allowed_channels}) + + assert "456" in channel._allowed_channels + assert "4" not in channel._allowed_channels + + @pytest.mark.asyncio + @pytest.mark.parametrize(("allowed_channels", "admitted"), [("456", True), ("457", False)]) + async def test_exempt_channel_needs_no_mention(self, allowed_channels, admitted): + channel = self._channel({"allowed_channels": allowed_channels}) + channel._running = True + channel._client = SimpleNamespace(user=SimpleNamespace(id=999, mention="<@999>")) + channel._discord_module = SimpleNamespace(Thread=type("FakeThread", (), {})) + channel._main_loop = asyncio.get_running_loop() + channel._reserve_inbound = MagicMock(return_value=MagicMock()) + channel._publish_reserved = MagicMock(return_value=None) + + await channel._on_message(_make_discord_message("hello")) + + assert channel._publish_reserved.called is admitted diff --git a/config.example.yaml b/config.example.yaml index dc3fcc0184..d288a1d5af 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -3039,9 +3039,15 @@ run_ownership: # discord: # enabled: false # bot_token: $DISCORD_BOT_TOKEN -# allowed_guilds: [] # empty = allow all guilds; can also be a single guild ID +# # Allowed guild IDs: one positive numeric ID (quoted or unquoted) or a YAML list. +# # Unset, null, [], or a blank string allows all guilds. +# # Invalid entries are ignored with a warning. Any other configured value +# # with no valid ID denies every guild and logs an error. +# allowed_guilds: [] # mention_only: false # If true, only respond when the bot is mentioned -# allowed_channels: [] # Optional: channel IDs exempt from mention_only (bot responds without mention) +# # Channel IDs exempt from mention_only: one ID (quoted or unquoted) or a YAML list. +# # Empty = no exemptions; mention_only applies to every channel when enabled. +# allowed_channels: [] # thread_mode: false # If true, group a channel conversation into a thread # # # Buzz (https://github.com/block/buzz) — Nostr-relay team workspace.