From 47b6755ce9f761fae2e870dfc7e43f5d646d31eb Mon Sep 17 00:00:00 2001 From: "Zhengxu (Joshua) Jin" <33077399+Jinzhengxu@users.noreply.github.com> Date: Mon, 12 Oct 2026 22:19:11 +0800 Subject: [PATCH] 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 --- README.md | 2 +- backend/app/channels/AGENTS.md | 2 +- backend/app/channels/allowed_users.py | 9 +++ backend/app/channels/qq.py | 29 ++++++++- backend/app/channels/wechat.py | 3 +- backend/docs/IM_CHANNEL_CONNECTIONS.md | 9 +++ backend/tests/test_allowed_users.py | 83 ++++++++++++++++++++++++ backend/tests/test_qq_channel.py | 89 ++++++++++++++++++++++++++ config.example.yaml | 2 +- 9 files changed, 221 insertions(+), 7 deletions(-) create mode 100644 backend/tests/test_allowed_users.py diff --git a/README.md b/README.md index a22f82d55e..efa1f04e9c 100644 --- a/README.md +++ b/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 diff --git a/backend/app/channels/AGENTS.md b/backend/app/channels/AGENTS.md index 6b53c985ed..2ed323669d 100644 --- a/backend/app/channels/AGENTS.md +++ b/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 ` 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` 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}`. diff --git a/backend/app/channels/allowed_users.py b/backend/app/channels/allowed_users.py index 0d8d8e9062..12982191be 100644 --- a/backend/app/channels/allowed_users.py +++ b/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 diff --git a/backend/app/channels/qq.py b/backend/app/channels/qq.py index 1ccfd1c528..334ee8dc4c 100644 --- a/backend/app/channels/qq.py +++ b/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( diff --git a/backend/app/channels/wechat.py b/backend/app/channels/wechat.py index 6a1e038cab..d15a542ad5 100644 --- a/backend/app/channels/wechat.py +++ b/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, ) diff --git a/backend/docs/IM_CHANNEL_CONNECTIONS.md b/backend/docs/IM_CHANNEL_CONNECTIONS.md index 37531f7858..74f5f45797 100644 --- a/backend/docs/IM_CHANNEL_CONNECTIONS.md +++ b/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 diff --git a/backend/tests/test_allowed_users.py b/backend/tests/test_allowed_users.py new file mode 100644 index 0000000000..503807f32f --- /dev/null +++ b/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 == [] diff --git a/backend/tests/test_qq_channel.py b/backend/tests/test_qq_channel.py index bb54702deb..6a931ffba6 100644 --- a/backend/tests/test_qq_channel.py +++ b/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() diff --git a/config.example.yaml b/config.example.yaml index e8ad228d56..ffa7f41f1a 100644 --- a/config.example.yaml +++ b/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: