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 <willem.jiang@gmail.com>

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 <code>` over long polling; Slack, Discord, Feishu/Lark, DingTalk, WeChat, and WeCom use `/connect <code>` 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}`.

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

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
 

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
 

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
 

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()
 

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