fix(qq): parse allowed_users with the shared fail-closed allowlist (#6730)

* fix(qq): parse allowed_users with the shared fail-closed allowlist

QQChannel built its allowlist with set(config.get("allowed_users") or []).
A single OpenID, which is what an `allowed_users: $QQ_ALLOWED_USERS`
reference resolves to, became a set of its characters, so the configured
user was silently dropped. A mapping authorized every key, including one
mapped to false, and a numeric scalar raised TypeError while the channel
was being constructed.

Parse it with the shared parse_allowed_users helper that Telegram and
WeChat already use: unset, null, empty collections and a blank string
allow everyone; a scalar OpenID is one entry; entries that are not
non-blank strings (such as numeric QQ account numbers) are dropped with a
warning, and a mapping is rejected as a whole; a configured value with no
valid OpenID denies every sender and logs an error. A scalar containing commas or whitespace warns
that it stays one literal ID. Binding codes still precede the allowlist.

* test(qq): gate group messages by member OpenID in allowlist tests

Every allowlist test sent a C2C frame, and the shared event() helper puts
the sender in both user_openid and member_openid, so a group-path
regression in sender resolution would pass unnoticed. admits() now takes
a kind; group frames carry only member_openid and a group_openid that
differs from the sender. The listed-OpenID test runs for both kinds, and a
new test shows that listing the group's OpenID does not admit its members.

Document that a blank or whitespace-only allowed_users string allows every
sender, like an unset value or an empty list.

* refactor(channels): share scalar allowlist separator warnings

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>

README.md

@@ -969,7 +969,7 @@ channels:
     enabled: true
     app_id: $QQ_APP_ID
     client_secret: $QQ_CLIENT_SECRET
-    allowed_users: []  # QQ OpenIDs, not QQ account numbers
+    allowed_users: []  # QQ OpenIDs, not QQ account numbers; empty allows all; one ID is one entry
 
   wecom:
     enabled: true

backend/app/channels/AGENTS.md

@@ -64,7 +64,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` uses WeChat/Telegram ID coercers: unset/null/empty collections/blank strings allow all; invalid entries warn; all-invalid input denies all with an error. WeChat stores integer-valued numbers as integer text, warns without splitting scalar separators, and gates users before media downloads.
+- `allowed_users.py` shares QQ/WeChat/Telegram parsing: unset/null/empty lists/blank strings allow all; invalid entries warn; all-invalid denies all with an error. QQ/WeChat opt into `warn_on_scalar_separators` without splitting IDs. WeChat converts integral numbers to text and gates before media downloads.
 - 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

@@ -15,6 +15,7 @@ def parse_allowed_users[UserId: Hashable](
     channel_name: str,
     expected_id: str,
     valid_id_name: str = "user ID",
+    warn_on_scalar_separators: bool = False,
 ) -> frozenset[UserId] | None:
     """Return ``None`` for an unset/empty allowlist, or its valid user IDs.
 
@@ -22,9 +23,17 @@ def parse_allowed_users[UserId: Hashable](
     channel's coercer validates each entry. Invalid entries warn and are dropped,
     but a configured value with no valid entry returns an empty set and logs an
     error, so callers deny everyone instead of silently removing the restriction.
+    String-ID channels can opt into a scalar-separator warning without splitting
+    the ID; surrounding whitespace alone does not trigger it.
     """
     if allowed_users is None or (isinstance(allowed_users, str) and not allowed_users.strip()):
         return None
+    if warn_on_scalar_separators and isinstance(allowed_users, str) and ("," in allowed_users or any(char.isspace() for char in allowed_users.strip())):
+        logger.warning(
+            "[%s] allowed_users is a scalar string containing separators; treating it as one literal %s. Use a YAML list for multiple IDs",
+            channel_name,
+            valid_id_name,
+        )
     entries = list(allowed_users) if isinstance(allowed_users, (list, tuple, set, frozenset)) else [allowed_users]
     if not entries:
         return None

backend/app/channels/qq.py

@@ -18,6 +18,7 @@ import httpx
 from websockets.asyncio.client import connect
 from websockets.exceptions import ConnectionClosed
 
+from app.channels.allowed_users import parse_allowed_users
 from app.channels.base import Channel
 from app.channels.commands import extract_connect_code, is_known_channel_command
 from app.channels.connection_identity import attach_connection_identity
@@ -58,6 +59,30 @@ def validate_gateway_url(url: str) -> str:
     return url
 
 
+def _parse_open_id(entry: Any) -> str | None:
+    """A QQ OpenID from a non-blank string, else ``None``.
+
+    OpenIDs are opaque strings. A YAML number is at best a QQ account number,
+    which never equals an OpenID, so it is rejected rather than stringified.
+    """
+    if isinstance(entry, str):
+        return entry.strip() or None
+    return None
+
+
+def _parse_allowed_users(allowed_users: Any) -> frozenset[str] | None:
+    """Parse OpenIDs without splitting a scalar string into its characters."""
+    return parse_allowed_users(
+        allowed_users,
+        parse_user_id=_parse_open_id,
+        logger=logger,
+        channel_name="qq",
+        expected_id="a QQ OpenID string, not a numeric QQ account number",
+        valid_id_name="OpenID",
+        warn_on_scalar_separators=True,
+    )
+
+
 class _GatewayConnect(connect):
     def process_redirect(self, exc: Exception) -> Exception:
         # Never forward Identify credentials through a redirected socket.
@@ -78,7 +103,7 @@ class QQChannel(Channel):
         super().__init__("qq", bus, config)
         self._app_id = str(config.get("app_id") or "").strip()
         self._client_secret = str(config.get("client_secret") or "").strip()
-        self._allowed_users = set(config.get("allowed_users") or [])
+        self._allowed_users = _parse_allowed_users(config.get("allowed_users"))
         self._api_url = "https://sandbox.api.sgroup.qq.com" if config.get("sandbox", False) else API_URL
         self._http: httpx.AsyncClient | None = None
         self._ssl: ssl.SSLContext | None = None
@@ -392,7 +417,7 @@ class QQChannel(Channel):
         # Binding codes are always control traffic, even without a repository.
         if extract_connect_code(text) and code is None:
             return
-        if not code and self._allowed_users and sender not in self._allowed_users:
+        if not code and self._allowed_users is not None and sender not in self._allowed_users:
             return
         key = (chat_id, message_id)
         self._replies.setdefault(

backend/app/channels/wechat.py

@@ -193,14 +193,13 @@ def _parse_wechat_user_id(entry: Any) -> str | None:
 
 def _parse_allowed_users(allowed_users: Any) -> frozenset[str] | None:
     """Parse iLink IDs without splitting a scalar string into multiple IDs."""
-    if isinstance(allowed_users, str) and ("," in allowed_users or any(char.isspace() for char in allowed_users.strip())):
-        logger.warning("[WeChat] allowed_users is a scalar string containing separators; treating it as one literal user ID. Use a YAML list for multiple IDs")
     return parse_allowed_users(
         allowed_users,
         parse_user_id=_parse_wechat_user_id,
         logger=logger,
         channel_name="WeChat",
         expected_id="a valid iLink user ID",
+        warn_on_scalar_separators=True,
     )
 
 

backend/docs/IM_CHANNEL_CONNECTIONS.md

@@ -523,6 +523,15 @@ OpenIDs represent the same identity. Bind each context separately. Group
 conversation histories are isolated by sender, but replies remain visible to
 everyone in the group.
 
+List several OpenIDs as a YAML list. A single OpenID, such as an
+`allowed_users: $QQ_ALLOWED_USERS` reference, is one entry rather than a
+sequence of characters. An unset or null value, an empty list, and a blank or
+whitespace-only string such as `" "` all allow every sender; they do not
+restrict access. Entries that are not OpenID strings, such as numeric
+QQ account numbers, are ignored with a warning, and a mapping is rejected as a
+whole, so none of its keys is authorized. A configured value with no valid
+OpenID denies every sender and logs an error.
+
 The MVP supports text-only `C2C_MESSAGE_CREATE` and `GROUP_AT_MESSAGE_CREATE`
 events and final text replies. It does not download/upload attachments, stream
 partial answers, handle guild/channel messages, or proactively push scheduled

backend/tests/test_allowed_users.py

@@ -0,0 +1,83 @@
+"""Shared allowlist warnings preserve literal IDs and channel opt-in."""
+
+import logging
+from typing import Any
+
+import pytest
+
+from app.channels.allowed_users import parse_allowed_users
+
+logger = logging.getLogger(__name__)
+
+
+def parse_string_id(entry: Any) -> str | None:
+    if isinstance(entry, str):
+        return entry.strip() or None
+    return None
+
+
+@pytest.mark.parametrize("channel_name, valid_id_name", [("qq", "OpenID"), ("WeChat", "user ID")])
+@pytest.mark.parametrize("separator", [",", ", ", " ", "\t", "\n", "\u2003"])
+def test_opted_in_scalar_separator_warning_preserves_literal_id(channel_name, valid_id_name, separator, caplog):
+    literal_id = f"alice{separator}bob"
+    with caplog.at_level(logging.WARNING, logger=logger.name):
+        result = parse_allowed_users(
+            literal_id,
+            parse_user_id=parse_string_id,
+            logger=logger,
+            channel_name=channel_name,
+            expected_id="a string ID",
+            valid_id_name=valid_id_name,
+            warn_on_scalar_separators=True,
+        )
+
+    assert result == frozenset({literal_id})
+    assert len(caplog.records) == 1
+    warning = caplog.records[0]
+    assert warning.levelno == logging.WARNING
+    assert f"[{channel_name}]" in warning.message
+    assert f"one literal {valid_id_name}" in warning.message
+    assert "YAML list" in warning.message
+    assert "alice" not in warning.message
+    assert "bob" not in warning.message
+
+
+def test_scalar_separator_warning_is_opt_in(caplog):
+    result = parse_allowed_users(
+        "alice,bob",
+        parse_user_id=parse_string_id,
+        logger=logger,
+        channel_name="other",
+        expected_id="a string ID",
+    )
+
+    assert result == frozenset({"alice,bob"})
+    assert caplog.records == []
+
+
+@pytest.mark.parametrize(
+    "allowed_users, expected",
+    [
+        ("alice", frozenset({"alice"})),
+        (" \talice\n ", frozenset({"alice"})),
+        (" \t\n ", None),
+        (None, None),
+        ([], None),
+        (["alice,bob"], frozenset({"alice,bob"})),
+        (("alice bob",), frozenset({"alice bob"})),
+        ({"alice\tbob"}, frozenset({"alice\tbob"})),
+        (frozenset({"alice\nbob"}), frozenset({"alice\nbob"})),
+    ],
+)
+def test_opted_in_warning_only_checks_scalar_separators(allowed_users, expected, caplog):
+    result = parse_allowed_users(
+        allowed_users,
+        parse_user_id=parse_string_id,
+        logger=logger,
+        channel_name="other",
+        expected_id="a string ID",
+        warn_on_scalar_separators=True,
+    )
+
+    assert result == expected
+    assert caplog.records == []

backend/tests/test_qq_channel.py

@@ -4,6 +4,7 @@ from __future__ import annotations
 
 import asyncio
 import json
+import logging
 from datetime import UTC, datetime, timedelta
 from unittest.mock import AsyncMock
 
@@ -97,6 +98,94 @@ async def test_allowed_users_gate_precedes_identity_query(channel):
     channel._connection_repo.find_connection_by_external_identity.assert_not_awaited()
 
 
+OPEN_ID = "E4F4AEA33253A2797FB897C50B81D7ED"
+GROUP_OPEN_ID = "0A1B2C3D4E5F60718293A4B5C6D7E8F9"
+
+
+async def admits(allowed_users, sender=OPEN_ID, kind="c2c"):
+    """Whether an ordinary message from *sender* passes ``allowed_users``."""
+    frame = event(kind, sender=sender, target=GROUP_OPEN_ID)
+    if kind == "group":
+        # Group frames identify the sender by member_openid only.
+        frame["d"]["author"] = {"member_openid": sender}
+    instance = qq.QQChannel(
+        bus=MessageBus(),
+        config={"app_id": "app1", "client_secret": "fixture-secret", "allowed_users": allowed_users},
+    )
+    try:
+        await instance._handle_inbound(frame)
+        return not instance.bus.inbound_queue.empty()
+    finally:
+        await instance.stop()
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    "allowed_users",
+    [None, [], (), set(), frozenset(), " "],
+    ids=["null", "list", "tuple", "set", "frozenset", "blank-string"],
+)
+async def test_unset_or_empty_allowlist_admits_everyone_quietly(allowed_users, caplog):
+    with caplog.at_level(logging.WARNING, logger="app.channels.qq"):
+        assert await admits(allowed_users)
+        assert await admits(allowed_users, sender="another-open-id")
+    assert caplog.records == []
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("kind", ["c2c", "group"])
+@pytest.mark.parametrize("collection", [list, tuple, set, frozenset])
+async def test_listed_open_ids_are_admitted_and_others_denied(collection, kind):
+    allowed = collection([OPEN_ID, "another-open-id"])
+    assert await admits(allowed, kind=kind)
+    assert await admits(allowed, sender="another-open-id", kind=kind)
+    assert not await admits(allowed, sender="stranger", kind=kind)
+
+
+@pytest.mark.asyncio
+async def test_group_messages_are_gated_by_member_not_group_open_id():
+    assert not await admits([GROUP_OPEN_ID], kind="group")
+    assert await admits([OPEN_ID], kind="group")
+
+
+@pytest.mark.asyncio
+async def test_scalar_open_id_is_one_entry_not_its_characters():
+    # ``allowed_users: $QQ_ALLOWED_USERS`` resolves to one string, not a list.
+    assert await admits(OPEN_ID)
+    assert await admits(f"  {OPEN_ID}  ")
+    assert not await admits(OPEN_ID, sender=OPEN_ID[0])
+
+
+@pytest.mark.asyncio
+async def test_scalar_with_separators_stays_one_literal_id_and_warns(caplog):
+    with caplog.at_level(logging.WARNING, logger="app.channels.qq"):
+        assert not await admits(f"{OPEN_ID},another-open-id")
+    assert "YAML list" in caplog.text
+
+
+@pytest.mark.asyncio
+async def test_unparseable_entry_is_dropped_with_a_warning(caplog):
+    with caplog.at_level(logging.WARNING, logger="app.channels.qq"):
+        assert await admits([OPEN_ID, 123456789, "  "])
+    assert "123456789" in caplog.text
+    assert not await admits([OPEN_ID, 123456789], sender="123456789")
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    "allowed_users",
+    [{OPEN_ID: True}, {OPEN_ID: False}, {}, 123456789, [123456789], [""], [None], [True], 1.5],
+    ids=["mapping-true", "mapping-false", "empty-mapping", "int-scalar", "int-entry", "blank-entry", "null-entry", "bool-entry", "float-scalar"],
+)
+async def test_allowlist_without_an_open_id_denies_everyone(allowed_users, caplog):
+    # Mappings must not authorize their keys, and a numeric QQ account number
+    # never matches an OpenID: either way the configured restriction stands.
+    with caplog.at_level(logging.ERROR, logger="app.channels.qq"):
+        assert not await admits(allowed_users)
+        assert not await admits(allowed_users, sender="123456789")
+    assert any(record.levelno == logging.ERROR for record in caplog.records)
+
+
 @pytest.mark.asyncio
 async def test_binding_precedes_allowlist_and_never_enters_agent(channel):
     repo = AsyncMock()

config.example.yaml

@@ -3067,7 +3067,7 @@ run_ownership:
 #     enabled: false
 #     app_id: $QQ_APP_ID
 #     client_secret: $QQ_CLIENT_SECRET
-#     allowed_users: []  # QQ OpenIDs, not numeric QQ account numbers; empty allows all
+#     allowed_users: []  # QQ OpenIDs, not numeric QQ account numbers; empty allows all; a single ID is one entry; unparseable non-empty lists fail closed
 #     sandbox: false    # REST sandbox host only; console sandbox membership is separate
 #
 #   feishu: