From 70e8f8974f37098e443321daf6c4cb27536cbabd Mon Sep 17 00:00:00 2001 From: "Zhengxu (Joshua) Jin" <33077399+Jinzhengxu@users.noreply.github.com> Date: Wed, 14 Oct 2026 10:56:31 +0800 Subject: [PATCH] fix(channels): parse Slack and DingTalk allowed_users fail-closed (#6742) * fix(channels): parse Slack and DingTalk allowed_users fail-closed Slack and DingTalk still built their allowlists with their own _normalize_allowed_users: every entry went through str() without stripping, only list/tuple/set counted as collections, and the gate was `if self._allowed_users and ...`. A configured list whose entries are all blank, such as `allowed_users: [$SLACK_USER_ID]` with the variable set to an empty string, became an empty set and let every sender through. " U123 " never matched U123, and a frozenset was stringified into one ID. Parse both with the shared parse_allowed_users helper that Telegram, WeChat and QQ use, and gate on `is not None`: unset, null, empty collections and a blank string allow everyone; a scalar is one entry; strings are stripped and integers keep matching their string form, as before; booleans, floats, None and mappings are rejected (a mapping as a whole); a configured value with no valid ID denies every sender and logs an error. A scalar containing commas or whitespace warns that it stays one literal ID. * refactor(channels): share the Slack and DingTalk allowlist coercer and separator warning Slack and DingTalk had byte-identical ID coercers and their own copies of the scalar-separator warning. Move the coercer into allowed_users.py as parse_string_or_int_user_id and opt both channels into warn_on_scalar_separators, as QQ and WeChat do. Log messages are unchanged. Pin that a separator-only string such as "," stays one literal ID (denying everyone else) and still warns. --------- Co-authored-by: Willem Jiang --- backend/app/channels/AGENTS.md | 2 +- backend/app/channels/allowed_users.py | 16 +++++ backend/app/channels/dingtalk.py | 33 +++++----- backend/app/channels/slack.py | 33 +++++----- backend/tests/test_channels.py | 66 +++++++++++++++++++- backend/tests/test_dingtalk_channel.py | 85 ++++++++++++++++++++------ config.example.yaml | 4 +- 7 files changed, 180 insertions(+), 59 deletions(-) diff --git a/backend/app/channels/AGENTS.md b/backend/app/channels/AGENTS.md index d49f9b9af9..ab5ab973ad 100644 --- a/backend/app/channels/AGENTS.md +++ b/backend/app/channels/AGENTS.md @@ -68,7 +68,7 @@ The cached value is reused for both the blocking (`runs.wait`) and streaming (`_ - No public IP, OAuth callback URL, or provider webhook route is required by the current implementation. - Telegram binds via deep-link `/start ` over long polling; Slack, Discord, Feishu/Lark, DingTalk, WeChat, and WeCom use `/connect ` through their outbound workers. - WeChat `polling_timeout`, `polling_retry_delay`, `qrcode_poll_interval`, and `qrcode_poll_timeout` require positive finite seconds; invalid values use defaults to prevent hot loops or endless sleeps. -- `allowed_users.py`: QQ/WeChat/Telegram empty allows all; invalid warns; all-invalid denies all. WeChat coerces integral IDs and gates downloads. Buzz/Discord `parse_channel_ids`: trims IDs; empty = no exemption; no splitting/mapping keys. Buzz/QQ/WeChat warn on scalar separators. +- `allowed_users.py`: QQ/WeChat/Telegram/Slack/DingTalk empty allows all; invalid warns; all-invalid denies. WeChat: int IDs, gate downloads. Buzz/Discord `parse_channel_ids`: trim; no splitting/mapping keys; empty = no exemption. Separator warnings: Buzz/QQ/WeChat/Slack/DingTalk. - WeCom serializes `start()` and `stop()` for each channel instance. The SDK `connect()` task covers connection setup only; after the handshake, the SDK owns a separate receive task. Shutdown cancels an in-progress connection attempt and awaits the SDK's actual asynchronous receive-task/socket cleanup before releasing lifecycle state or allowing a restart. Cancellation of `stop()` still propagates, but only after owned cleanup finishes and lifecycle references are cleared; real connection failures remain reported by `_on_ws_task_done`. - WeCom outbound content is capped at the protocol's 20480 UTF-8 bytes: stream replies clip on a character boundary with a truncation marker (one stream carries the whole reply and cannot split mid-way), while proactive pushes split into at most 10 sequential markdown messages per push, with the remaining tail clipped and marker-terminated. Both paths measure bytes, not characters. A per-chat send lock serializes each split batch end to end, because manager workers run concurrently and two long pushes to the same chat would otherwise interleave chunks; locks are reference-counted and reclaimed once no sender holds or waits on them, so the registry does not grow for the life of the Gateway. - Frontend APIs: `GET /api/channels/providers`, `GET /api/channels/connections`, `POST /api/channels/{provider}/connect`, and `DELETE /api/channels/connections/{connection_id}`. diff --git a/backend/app/channels/allowed_users.py b/backend/app/channels/allowed_users.py index 6d7655a6da..fd2029d777 100644 --- a/backend/app/channels/allowed_users.py +++ b/backend/app/channels/allowed_users.py @@ -93,3 +93,19 @@ def parse_allowed_users[UserId: Hashable]( if not user_ids: logger.error("[%s] allowed_users has no valid %s; denying every user until it is fixed", channel_name, valid_id_name) return frozenset(user_ids) + + +def parse_string_or_int_user_id(entry: Any) -> str | None: + """A user ID from a non-blank string or an integer, else ``None``. + + An unquoted YAML number keeps matching its string form. Quote IDs with + leading zeros. + """ + if isinstance(entry, bool): + # bool is an int subclass: ``true`` must not become the user "True". + return None + if isinstance(entry, int): + return str(entry) + if isinstance(entry, str): + return entry.strip() or None + return None diff --git a/backend/app/channels/dingtalk.py b/backend/app/channels/dingtalk.py index ec33ab1ffa..901af7108f 100644 --- a/backend/app/channels/dingtalk.py +++ b/backend/app/channels/dingtalk.py @@ -16,6 +16,7 @@ from urllib.parse import quote_plus import httpx +from app.channels.allowed_users import parse_allowed_users, parse_string_or_int_user_id from app.channels.base import Channel, ChannelStopTimeout, abort_connection_transport, shutdown_private_loop from app.channels.commands import is_known_channel_command, strip_leading_mentions from app.channels.connection_identity import attach_connection_identity @@ -72,20 +73,20 @@ def _normalize_conversation_type(raw: Any) -> str: return _CONVERSATION_TYPE_P2P -def _normalize_allowed_users(allowed_users: Any) -> set[str]: - if allowed_users is None: - return set() - if isinstance(allowed_users, str): - values = [allowed_users] - elif isinstance(allowed_users, (list, tuple, set)): - values = allowed_users - else: - logger.warning( - "DingTalk allowed_users should be a list of user IDs; treating %s as one string value", - type(allowed_users).__name__, - ) - values = [allowed_users] - return {str(uid) for uid in values if str(uid)} +def _parse_allowed_users(allowed_users: Any) -> frozenset[str] | None: + """Parse DingTalk staff IDs with the shared fail-closed allowlist semantics. + + Staff IDs are often all digits, so integer entries are accepted as text. + """ + return parse_allowed_users( + allowed_users, + parse_user_id=parse_string_or_int_user_id, + logger=logger, + channel_name="DingTalk", + expected_id="a DingTalk staff ID (senderStaffId)", + valid_id_name="staff ID", + warn_on_scalar_separators=True, + ) def _is_dingtalk_command(text: str) -> bool: @@ -187,7 +188,7 @@ class DingTalkChannel(Channel): self._main_loop: asyncio.AbstractEventLoop | None = None self._client_id: str = "" self._client_secret: str = "" - self._allowed_users: set[str] = _normalize_allowed_users(config.get("allowed_users")) + self._allowed_users: frozenset[str] | None = _parse_allowed_users(config.get("allowed_users")) self._cached_token: str = "" self._token_expires_at: float = 0.0 self._token_lock = asyncio.Lock() @@ -603,7 +604,7 @@ class DingTalkChannel(Channel): logger.warning("[DingTalk] main loop not running, cannot bind channel connection") return - if self._allowed_users and sender_staff_id not in self._allowed_users: + if self._allowed_users is not None and sender_staff_id not in self._allowed_users: logger.debug("[DingTalk] ignoring message from non-allowed user: %s", sender_staff_id) return diff --git a/backend/app/channels/slack.py b/backend/app/channels/slack.py index 51a78c1db8..7b8522c0e7 100644 --- a/backend/app/channels/slack.py +++ b/backend/app/channels/slack.py @@ -10,6 +10,7 @@ from typing import Any from markdown_to_mrkdwn import SlackMarkdownConverter +from app.channels.allowed_users import parse_allowed_users, parse_string_or_int_user_id from app.channels.base import Channel, ChannelStopTimeout from app.channels.commands import is_known_channel_command from app.channels.connection_identity import attach_connection_identity @@ -50,20 +51,16 @@ def _escape_slack_text(text: str) -> str: return re.sub(r"(?m)^>", ">", escaped) -def _normalize_allowed_users(allowed_users: Any) -> set[str]: - if allowed_users is None: - return set() - if isinstance(allowed_users, str): - values = [allowed_users] - elif isinstance(allowed_users, list | tuple | set): - values = allowed_users - else: - logger.warning( - "Slack allowed_users should be a list of Slack user IDs or a single Slack user ID string; treating %s as one string value", - type(allowed_users).__name__, - ) - values = [allowed_users] - return {str(user_id) for user_id in values if str(user_id)} +def _parse_allowed_users(allowed_users: Any) -> frozenset[str] | None: + """Parse Slack user IDs with the shared fail-closed allowlist semantics.""" + return parse_allowed_users( + allowed_users, + parse_user_id=parse_string_or_int_user_id, + logger=logger, + channel_name="Slack", + expected_id="a Slack user ID such as U123456", + warn_on_scalar_separators=True, + ) def _strip_leading_slack_bot_mention(text: str, bot_user_id: str | None) -> str: @@ -97,8 +94,8 @@ class SlackChannel(Channel): - ``bot_token``: Slack Bot User OAuth Token (xoxb-...). - ``app_token``: Slack App-Level Token (xapp-...) for Socket Mode. - ``allowed_users``: (optional) List of allowed Slack user IDs, or a - single Slack user ID string as shorthand. Empty = allow all. Other - scalar values are treated as a single string with a warning. + single Slack user ID as shorthand. Unset, empty or blank = allow + all. A non-empty value without a valid ID denies everyone. """ def __init__(self, bus: MessageBus, config: dict[str, Any]) -> None: @@ -108,7 +105,7 @@ class SlackChannel(Channel): self._socket_close_task: asyncio.Task[None] | None = None self._web_client = None self._loop: asyncio.AbstractEventLoop | None = None - self._allowed_users = _normalize_allowed_users(config.get("allowed_users", [])) + self._allowed_users = _parse_allowed_users(config.get("allowed_users")) self._web_client_factory = config.get("web_client_factory") self._connection_web_clients: dict[str, tuple[str, Any]] = {} configured_bot_user_id = config.get("bot_user_id") @@ -414,7 +411,7 @@ class SlackChannel(Channel): # Check allowed users after connect-code handling so browser-initiated # binding can bootstrap a new external identity. - if self._allowed_users and user_id not in self._allowed_users: + if self._allowed_users is not None and user_id not in self._allowed_users: logger.debug("Ignoring message from non-allowed user: %s", user_id) return diff --git a/backend/tests/test_channels.py b/backend/tests/test_channels.py index 67cbf3e996..e48b4bd1db 100644 --- a/backend/tests/test_channels.py +++ b/backend/tests/test_channels.py @@ -10670,7 +10670,7 @@ class TestSlackAllowedUsers: assert inbound.text == "/help" assert inbound.msg_type == InboundMessageType.COMMAND - def test_scalar_allowed_users_warns_and_matches_stringified_event_user_id(self, caplog): + def test_numeric_scalar_allowed_users_matches_stringified_event_user_id(self, caplog): from app.channels.slack import SlackChannel bus = MessageBus() @@ -10697,11 +10697,73 @@ class TestSlackAllowedUsers: ): channel._handle_message_event(event) - assert "Slack allowed_users should be a list" in caplog.text + # A scalar is one entry under the shared allowlist contract, not a misconfiguration. + assert caplog.records == [] channel._loop.call_soon_threadsafe.assert_called_once() inbound = bus.get_inbound_nowait() assert inbound.user_id == "123456" + @pytest.mark.parametrize( + ("allowed_users", "admitted"), + [([""], False), ({"U123456": True}, False), ([" U123456 "], True), (frozenset({"U123456"}), True)], + ids=["blank-entry", "mapping", "padded-entry", "frozenset"], + ) + def test_allowlist_gate_uses_parsed_user_ids(self, allowed_users, admitted): + from app.channels.slack import SlackChannel + + bus = MessageBus() + channel = SlackChannel(bus=bus, config={"allowed_users": allowed_users}) + channel._loop = self._immediate_loop() + channel._add_reaction = MagicMock() + channel._send_running_reply = MagicMock() + + event = {"user": "U123456", "text": "hello from slack", "channel": "C123", "ts": "1710000000.000100"} + with patch("app.channels.slack.asyncio.run_coroutine_threadsafe", side_effect=self._submit_coro): + channel._handle_message_event(event) + + assert (not bus.inbound_queue.empty()) is admitted + assert channel._add_reaction.called is admitted + + @pytest.mark.parametrize("allowed_users", [None, [], (), set(), frozenset(), "", " "]) + def test_unset_or_empty_allowlist_allows_everyone_quietly(self, allowed_users, caplog): + from app.channels.slack import _parse_allowed_users + + with caplog.at_level(logging.WARNING, logger="app.channels.slack"): + assert _parse_allowed_users(allowed_users) is None + assert caplog.records == [] + + @pytest.mark.parametrize("allowed_users", ["U123456,U654321", ","]) + def test_scalar_with_separators_stays_one_literal_id_and_warns(self, allowed_users, caplog): + from app.channels.slack import _parse_allowed_users + + with caplog.at_level(logging.WARNING, logger="app.channels.slack"): + assert _parse_allowed_users(allowed_users) == {allowed_users} + assert "YAML list" in caplog.text + + def test_invalid_entries_are_dropped_with_a_warning(self, caplog): + from app.channels.slack import _parse_allowed_users + + with caplog.at_level(logging.WARNING, logger="app.channels.slack"): + assert _parse_allowed_users(["U123456", True, 4.2, None, " "]) == {"U123456"} + assert "True" in caplog.text + assert "4.2" in caplog.text + assert not any(record.levelno == logging.ERROR for record in caplog.records) + + @pytest.mark.parametrize( + "allowed_users", + [[""], ["", " "], [None], [True], {"U123456": True}, {}, True, 4.2], + ids=["blank-entry", "blank-entries", "null-entry", "bool-entry", "mapping", "empty-mapping", "bool-scalar", "float-scalar"], + ) + def test_allowlist_without_a_valid_user_id_denies_everyone(self, allowed_users, caplog): + # A mapping must not authorize its keys, and a list whose entries are + # all blank (e.g. ``[$SLACK_USER_ID]`` with the variable set to an + # empty string) must not turn into "allow all". + from app.channels.slack import _parse_allowed_users + + with caplog.at_level(logging.ERROR, logger="app.channels.slack"): + assert _parse_allowed_users(allowed_users) == frozenset() + assert any(record.levelno == logging.ERROR for record in caplog.records) + def test_raises_after_all_retries_exhausted(self): from app.channels.slack import SlackChannel diff --git a/backend/tests/test_dingtalk_channel.py b/backend/tests/test_dingtalk_channel.py index 7ce5640dcf..24392c56d7 100644 --- a/backend/tests/test_dingtalk_channel.py +++ b/backend/tests/test_dingtalk_channel.py @@ -22,8 +22,8 @@ from app.channels.dingtalk import ( _DingTalkMessageHandler, _extract_text_from_rich_text, _is_dingtalk_command, - _normalize_allowed_users, _normalize_conversation_type, + _parse_allowed_users, ) from app.channels.message_bus import InboundMessageType, MessageBus, OutboundMessage from deerflow.config.paths import VIRTUAL_PATH_PREFIX @@ -120,32 +120,55 @@ class TestDingTalkMessageHandlerSdkContract: # --------------------------------------------------------------------------- -# _normalize_allowed_users tests +# _parse_allowed_users tests # --------------------------------------------------------------------------- -class TestNormalizeAllowedUsers: - def test_none_returns_empty(self): - assert _normalize_allowed_users(None) == set() +class TestParseAllowedUsers: + @pytest.mark.parametrize("allowed_users", [None, [], (), set(), frozenset(), "", " "]) + def test_unset_or_empty_allows_everyone_quietly(self, allowed_users, caplog): + with caplog.at_level(logging.WARNING, logger="app.channels.dingtalk"): + assert _parse_allowed_users(allowed_users) is None + assert caplog.records == [] - def test_empty_list_returns_empty(self): - assert _normalize_allowed_users([]) == set() + @pytest.mark.parametrize("collection", [list, tuple, set, frozenset]) + def test_listed_staff_ids(self, collection): + assert _parse_allowed_users(collection(["user1", " user2 "])) == {"user1", "user2"} - def test_list_of_strings(self): - result = _normalize_allowed_users(["user1", "user2"]) - assert result == {"user1", "user2"} - - def test_single_string(self): - result = _normalize_allowed_users("user1") - assert result == {"user1"} + def test_single_string_is_one_entry(self): + assert _parse_allowed_users("user1") == {"user1"} def test_numeric_values_converted_to_string(self): - result = _normalize_allowed_users([123, 456]) - assert result == {"123", "456"} + assert _parse_allowed_users([123, 456]) == {"123", "456"} - def test_scalar_treated_as_single_value(self): - result = _normalize_allowed_users(12345) - assert result == {"12345"} + def test_numeric_scalar_is_one_entry(self): + assert _parse_allowed_users(12345) == {"12345"} + + @pytest.mark.parametrize("allowed_users", ["user1,user2", ","]) + def test_scalar_with_separators_stays_one_literal_id_and_warns(self, allowed_users, caplog): + with caplog.at_level(logging.WARNING, logger="app.channels.dingtalk"): + assert _parse_allowed_users(allowed_users) == {allowed_users} + assert "YAML list" in caplog.text + + def test_invalid_entries_are_dropped_with_a_warning(self, caplog): + with caplog.at_level(logging.WARNING, logger="app.channels.dingtalk"): + assert _parse_allowed_users(["user1", True, 4.2, None, " "]) == {"user1"} + assert "True" in caplog.text + assert "4.2" in caplog.text + assert not any(record.levelno == logging.ERROR for record in caplog.records) + + @pytest.mark.parametrize( + "allowed_users", + [[""], ["", " "], [None], [True], {"user1": True}, {}, True, 4.2], + ids=["blank-entry", "blank-entries", "null-entry", "bool-entry", "mapping", "empty-mapping", "bool-scalar", "float-scalar"], + ) + def test_allowlist_without_a_valid_staff_id_denies_everyone(self, allowed_users, caplog): + # A mapping must not authorize its keys, and a list whose entries are + # all blank (e.g. ``[$DINGTALK_STAFF_ID]`` with the variable set to an + # empty string) must not turn into "allow all". + with caplog.at_level(logging.ERROR, logger="app.channels.dingtalk"): + assert _parse_allowed_users(allowed_users) == frozenset() + assert any(record.levelno == logging.ERROR for record in caplog.records) # --------------------------------------------------------------------------- @@ -648,6 +671,28 @@ class TestAllowedUsersFiltering: _run(go()) + @pytest.mark.parametrize( + ("allowed_users", "admitted"), + [([""], False), ({"user_001": True}, False), ([" user_001 "], True), (frozenset({"user_001"}), True)], + ids=["blank-entry", "mapping", "padded-entry", "frozenset"], + ) + def test_allowlist_gate_uses_parsed_staff_ids(self, allowed_users, admitted): + async def go(): + bus = MessageBus() + bus.publish_inbound = AsyncMock() + channel = DingTalkChannel(bus, config={"allowed_users": allowed_users}) + channel._client_id = "test_key" + channel._main_loop = asyncio.get_event_loop() + channel._running = True + channel._send_running_reply = AsyncMock() + + channel._on_chatbot_message(_make_chatbot_message(sender_staff_id="user_001")) + + await asyncio.sleep(0.1) + assert (not bus.inbound_queue.empty()) is admitted + + _run(go()) + def test_empty_allowed_users_allows_all(self): async def go(): bus = MessageBus() @@ -1687,7 +1732,7 @@ class TestCardMode: mock_message.rich_text_content = None channel._main_loop = asyncio.get_running_loop() - channel._allowed_users = set() + channel._allowed_users = None channel._running = True channel._send_running_reply = AsyncMock() diff --git a/config.example.yaml b/config.example.yaml index b9997703d0..61f8f81d1c 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -3108,7 +3108,7 @@ run_ownership: # enabled: false # bot_token: $SLACK_BOT_TOKEN # xoxb-... # app_token: $SLACK_APP_TOKEN # xapp-... (Socket Mode) -# allowed_users: [] # empty = allow all; can also be a single Slack user ID string, e.g. U123456, but list form is recommended +# allowed_users: [] # Slack user IDs, e.g. U123456; empty or blank allows all; a single ID is one entry; non-empty lists with no valid ID fail closed # # telegram: # enabled: false @@ -3180,7 +3180,7 @@ run_ownership: # enabled: false # client_id: $DINGTALK_CLIENT_ID # client_secret: $DINGTALK_CLIENT_SECRET -# allowed_users: [] # empty = allow all +# allowed_users: [] # staff IDs (senderStaffId); empty or blank allows all; quote IDs with leading zeros; non-empty lists with no valid ID fail closed # card_template_id: "" # Optional: AI Card template ID for streaming updates # # # Discord joins as a bot; requires the `discord` dependency extra