From 2523be658b9f41b4e4100231cfddd2f849dbedf8 Mon Sep 17 00:00:00 2001 From: "Grapette.L" <90967079+Lesereingrape@users.noreply.github.com> Date: Tue, 6 Oct 2026 07:56:10 +0800 Subject: [PATCH] fix(discord): read allowed_guilds as IDs and deny every guild on an unreadable allowlist (#6338) * test(discord): expose allowlist parsing failures instead of opening the bot config.example.yaml documents channels.discord.allowed_guilds as a value that "can also be a single guild ID", but the constructor iterates it: a scalar string becomes one entry per character, and an unquoted YAML int raises TypeError. Whatever remains is gated by truthiness, so an allowlist with no parseable entry leaves the set empty and admits every guild. * fix(discord): read allowed_guilds as IDs, and fail closed when unreadable The allowlist was built by iterating the configured value, so the documented single-guild shorthand ("can also be a single guild ID" in config.example.yaml) became one entry per character: allowed_guilds: "321" admitted guilds 1, 2 and 3 while blocking 321, and an unquoted YAML int raised TypeError in the constructor. The empty result then read as "no allowlist", so a restriction the operator configured could open the bot to every guild the bot joined. Parse both ID lists through helpers that treat a scalar as a one-entry list and drop unreadable entries with a warning, following the telegram channel: an allowlist with no parseable entry now denies every guild and logs at ERROR. Mention exemptions keep their IDs verbatim, where an unreadable entry only leaves mention_only in force. * docs(discord): explain allowlist parsing and deny-all behavior --------- Co-authored-by: Willem Jiang --- README.md | 8 ++ backend/app/channels/discord.py | 82 ++++++++++++++---- backend/tests/test_discord_channel.py | 114 +++++++++++++++++++++++++- config.example.yaml | 10 ++- 4 files changed, 196 insertions(+), 18 deletions(-) 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.