mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-19 11:06:18 +00:00
* fix(channels): stream-cap and validate WeChat/WeCom inbound media downloads, fixes #5223 * fix(channels): address WeCom APPID, decompression, and log-sanitization review findings (#5223) Round-4 review follow-ups on the inbound-media download cap: - The COS bucket numeric suffix is the owner's Tencent Cloud APPID and bucket names are user-chosen, so any Tencent Cloud account could register a matching ww-aibot-img-* bucket and pass the shape gate. The built-in rule now admits only the APPID observed in Tencent's published aibot callback examples (1258476243), across regions; any other account (including a future WeCom rotation) goes through channels.wecom.allowed_media_hosts. - aiter_bytes() transparently decodes Content-Encoding, and the decoder allocates the full decompressed body before the byte cap sees a chunk (an ~8 KB gzip wire chunk decoding to 8 MiB reproduces it). Both URL readers now send Accept-Encoding: identity, refuse a response with a residual Content-Encoding before reading, and iterate aiter_raw(). - httpx.HTTPStatusError formats the signed URL (path + query credentials) into its message, so _ingest_inbound_files' reader-failure branch logs a sanitized summary (class + status) instead of logger.exception, and the WeChat extract paths catch httpx.HTTPError so the polling loop's per-message logger.exception can never render a media URL. Every change ships with a red/green regression: the reviewer's 403 mock-transport repro asserted against caplog.text (fully formatted logs), the reviewer's different-APPID bucket host, and gzip bombs driven through real httpx mock transports in both readers. Docs (channels AGENTS.md, README, config.example.yaml) updated for the APPID pinning and encoding gate. * docs(channels): document why the inbound-media cap is 50 MB, not WeCom's 100 MB ceiling * fix(logging): redact URLs in httpx request logs down to scheme + host, fixes #5223 httpx emits 'HTTP Request: GET <full URL>' at the Gateway's INFO level before any response handling runs, so even successful signed-media downloads leaked their credentials. HttpxUrlQueryRedactionFilter (installed by configure_logging) rewrites those records in place — path and query become /<redacted>, method/status/duration observability is preserved — which also keeps Telegram's token-bearing Bot API paths out of the logs. Reader-level regression tests run at production INFO level with a real MockTransport, success paths included. * fix(logging): blank userinfo credentials in httpx request-log redaction * fix(logging): redact authority-only URLs and cover urllib3 redirect logs Two follow-ups from the review plus one extrapolation of the same class: - rest is now optional in _URL_REDACT_RE, so an authority-only URL (scheme://user:pass@host, no path) is rewritten too — userinfo had nowhere else to hide and previously passed through verbatim. A bare credential-free origin still passes through unchanged. - Renamed to UrlRedactionFilter / install_url_log_redaction and attached to the urllib3 logger as well: urllib3 logs 'Redirecting <url> -> <url>' at INFO with full URLs on both sides, the same leak class on a different library logger. No gateway path today both uses requests and redirects a signed URL, but the class stays closed instead of dormant. - Unit tests now build records with the real httpx 0.28.1 format string ('HTTP Request: %s %s "%s %d %s"', 5 args) and httpx.URL args, per the nit, instead of a synthetic shape httpx never emits. * fix(logging): install URL redaction at handler level so propagated records are covered A logging.Filter on a logger only runs for records emitted through that exact logger — child loggers neither inherit it nor trigger it on propagation — so the previous attachment to the bare urllib3 logger was dead code: urllib3 emits Redirecting via urllib3.poolmanager at INFO and urllib3.connectionpool at DEBUG. The filter is now attached to every root handler (mirroring _install_trace_filter, which already iterates root handlers; handler-level filters see propagated records) in addition to the httpx logger (httpx emits via the bare name, and emission-point coverage survives handlers added later). The wiring is pinned by tests that emit through the real urllib3 child loggers — a mutation removing the handler-level install turns them red. Comments, docstrings, and AGENTS.md now state the actual emitter names and levels. * fix(logging): redact urllib3 DEBUG request lines, whose split shape evaded the URL regex urllib3's per-request line (connectionpool.py:545 on 2.7.0) renders as `scheme://host:port "METHOD /path?query HTTP/x.x" status len` — the authority ends at a space so _URL_REDACT_RE's bare-origin early return applies, and the quoted origin-form target has no scheme, so neither half was rewritten. UrlRedactionFilter now runs a dedicated request-line shape first (collapsing the target to /<redacted>, keeping scheme+host+method+ version), then the absolute-URL pass. Regressions pin the exact format string both at unit level and through the real urllib3.connectionpool DEBUG emit path; AGENTS.md wording now names both covered DEBUG shapes. * fix(logging): redact urllib3 retry lines and linearize scheme scanning Closes the two open review threads on the inbound-media log hardening: Retry/redirect targets: urllib3 logs the request target with no scheme in five shapes the generic absolute-URL pass cannot see - `Retry: <target>` (connectionpool.py:954 DEBUG), `Incremented Retry for (url='<target>')` (util/retry.py:545 DEBUG, absolute on the redirect path), `Retrying (...) after connection broken by '<err>': <target>` (connectionpool.py:869 WARNING, above the INFO root), and origin-form halves of both Redirecting emitters (poolmanager.py:500 INFO / connectionpool.py:922 DEBUG). Each gets a rewrite anchored to the exact urllib3 format, collapsing the target to /<redacted>; the generic pass's rest now stops at quote characters so a quoted URL keeps its closing punctuation (previously the absolute-form increment line was mangled), and the request-line method class accepts any case. The emitter enumeration in channels AGENTS.md is closed against the installed urllib3 2.7.0 source. Quadratic scanning: both scheme-bearing patterns start with a character class, so re.sub retried every suffix of a long token - 64K paths cost ~1.8s and URL-free 64K error bodies ~3.1s per record, synchronously in every root handler. The two passes are now driven from literal "://" occurrences: _scheme_starts walks back over the scheme charset to each run's first letter and the pattern is attempted only there, reproducing re.sub's leftmost-non-overlapping result in linear time (256K path: 5.6ms; worst adversarial shapes <= 28ms). Long-input regressions pin the URL-bearing and URL-free cases with mutation-verified bounds, plus nested-scheme and digit-headed-run equivalence cases. Validation: tests/test_logging_config.py 12/12; scheme-pass equivalence against the old re.sub pipeline verified by two independent 30k+ case fuzz runs; full-suite A/B against HEAD shows zero tests that pass on HEAD and fail with this diff. * fix(logging): boundary-aware quote stops and whole-message Redirecting anchor Two follow-ups on the urllib3 redaction shapes: Embedded quotes: `rest` treated ANY quote as a closing mark, so a URL with an apostrophe in the path kept everything after it verbatim (`https://h/path'quoted'?token=Q` rendered the credential suffix in full) while the class docstring claimed path/query/fragment are replaced. A quote now closes `rest` only at a boundary - followed by whitespace, a closing parenthesis, or end of string - so urllib3's Incremented Retry (url='...') scaffolding keeps its ') closer while an embedded quote stays consumed. The increment line's url capture gets the same rule narrowed to its fixed ')' closer. Redirecting anchoring: the origin-half pass matched `(? <=-> )/path` as a substring, and an `-> /path` arrow is not urllib3-owned shape - the sandbox provider's actionable mount error (`sandbox.mounts entry <host> -> /mnt/knowledge ignored: ...`) had its container path rewritten to /<redacted>, failing test_setup_path_mappings_logs_actionable_error_for_missing_host_path on CI (backend-unit-tests shard 3). The pass is now anchored to the whole `Redirecting <t> -> <t>` message, which is exactly urllib3's record; origin slots collapse, absolute slots stay for the generic pass. Regression tests pin the embedded-quote shapes and the sandbox error's byte-for-byte passthrough; both mutations verified red. Validation: tests/test_logging_config.py 14/14; the CI-failing sandbox test green locally; every test file asserting redaction/arrow log content passes (attachments, support bundle, run metadata, skill secrets, ragflow, skillscan, sandbox provider); full offline backend suite 14084 passed / 164 failed with the failure set matching this machine's documented Windows-environment baseline (NTFS chmod/symlink, docker/lark/langfuse absences) - no failure involves redaction output. * fix(logging): redact redirects with spaced locations * fix(logging): grammar-complete Redirecting anchor; neutral WeChat guard labels Round-13 P3 (Redirecting anchor strictness): the whole-message anchor kept the ^Redirecting prefix (the urllib3-owned literal that stops the sandbox false positive) but required BOTH slots whitespace-free, so a Location header with an interior space voided the pass and leaked the origin-form request target in the first slot - redirect_location is the raw header string and interior spaces are legal field syntax. The tail is now loose (\S.*$) and the first slot gets the same grammar treatment (\S.*?): the recursive urlopen frame passes the previous raw Location as its url, so t1 can carry interior spaces too, lazy-split at the first arrow the way the line is constructed. A space-carrying slot collapses whole when it starts with /; the sandbox mount error keeps passing through untouched. Round-14 nit (None conflation): _download_cdn_bytes returns None for two reasons (in-flight cap abort, Content-Encoding refusal) but both image and file callers labeled it "exceeds size limit (N bytes)" - contradicting the accurate encoding line right above it, and reporting the plaintext limit for a ciphertext-cap decision. Callers now log a neutral "skipped by download guard" line (the manager reader callers' shape); the accurate reason stays inside the download function. The same sweep also logs _stage_downloaded_file's silent None (no state dir configured), which made an attachment vanish with no log line at all. Also anchors the emitter-enumeration closure to its urllib3 version: the closure reopens if an upgrade changes these format strings, so the comment now says so explicitly. Validation: logging 15/15 and attachments 60/62 (the two pre-existing Windows symlink-privilege failures documented in the PR body); three mutations verified red (old wording, strict t1, silent staging None); ruff clean. Full offline suite run before push (per round-11 lesson). --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
115 lines
4.7 KiB
Python
115 lines
4.7 KiB
Python
"""Regression anchor: WeChat channel filesystem IO must not block the event loop.
|
|
|
|
Two production paths touch the filesystem, and both must stay off the asyncio
|
|
loop:
|
|
|
|
1. **Construction** — ``ChannelService._start_channel()`` instantiates the
|
|
channel directly on the async path (``channel_cls(...)`` with no thread
|
|
offload), so ``WechatChannel.__init__`` must be IO-free; persisted state is
|
|
loaded later in ``start()`` via ``asyncio.to_thread``. Constructing the
|
|
channel in an async context used to raise ``BlockingError: Blocking call to
|
|
os.stat`` because ``__init__`` called ``_load_state`` synchronously.
|
|
|
|
2. **Runtime** — ``_handle_update`` stages downloaded inbound files
|
|
(``mkdir`` + ``write_bytes``) and ``_ensure_authenticated`` reads persisted
|
|
auth state (``read_text``); both offload via ``asyncio.to_thread``.
|
|
|
|
If any of this regresses back onto the event loop, the strict Blockbuster gate
|
|
raises ``BlockingError`` and these tests fail.
|
|
|
|
The constructors below are invoked *directly* on the event loop (no
|
|
``asyncio.to_thread`` wrapper) on purpose: that mirrors the production
|
|
``_start_channel`` path the gate is meant to protect. Test-only file setup
|
|
(writing the auth fixture) is still wrapped in ``asyncio.to_thread`` because
|
|
that scaffolding IO would otherwise trip the gate even though it is not the
|
|
code under test.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
import json
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
from app.channels.message_bus import MessageBus
|
|
from app.channels.wechat import WechatChannel, _encrypt_aes_128_ecb
|
|
|
|
pytestmark = pytest.mark.asyncio
|
|
|
|
|
|
async def test_wechat_constructor_is_io_free_on_async_path(tmp_path: Path) -> None:
|
|
"""``__init__`` must not touch the filesystem — ``_start_channel`` constructs inline."""
|
|
bus = MessageBus()
|
|
# Seed an auth file that WOULD restore a different token if __init__ read it.
|
|
auth_path = tmp_path / "wechat-auth.json"
|
|
await asyncio.to_thread(auth_path.write_text, json.dumps({"status": "confirmed", "bot_token": "from-disk"}))
|
|
|
|
# Direct construction on the event loop, no asyncio.to_thread wrapper —
|
|
# this is exactly the production _start_channel path. If __init__ regresses
|
|
# to doing os.stat / read_text (e.g. re-adding _load_state here), the gate
|
|
# raises BlockingError right at this line.
|
|
channel = WechatChannel(bus=bus, config={"bot_token": "from-config", "state_dir": str(tmp_path)})
|
|
# Token came from config, not the disk file — proving __init__ did not read it.
|
|
assert channel._bot_token == "from-config"
|
|
|
|
|
|
async def test_wechat_inbound_file_staging_does_not_block_event_loop(tmp_path: Path) -> None:
|
|
"""Staging a downloaded inbound image writes through ``asyncio.to_thread``."""
|
|
bus = MessageBus()
|
|
published = []
|
|
|
|
async def capture(msg):
|
|
published.append(msg)
|
|
|
|
bus.publish_inbound = capture # type: ignore[method-assign]
|
|
|
|
channel = WechatChannel(bus=bus, config={"bot_token": "test-token", "state_dir": str(tmp_path)})
|
|
|
|
plaintext = b"fake-image-bytes"
|
|
aes_key = b"1234567890abcdef"
|
|
encrypted = _encrypt_aes_128_ecb(plaintext, aes_key)
|
|
|
|
async def _fake_download(_url: str, *, timeout: float | None = None, max_bytes: int | None = None):
|
|
return encrypted
|
|
|
|
channel._download_cdn_bytes = _fake_download # type: ignore[method-assign]
|
|
|
|
await channel._handle_update(
|
|
{
|
|
"message_type": 1,
|
|
"message_id": 101,
|
|
"from_user_id": "wx-1",
|
|
"context_token": "ctx-img",
|
|
"item_list": [
|
|
{
|
|
"type": 2,
|
|
"image_item": {"aeskey": aes_key.hex(), "media": {"full_url": "https://cdn.weixin.qq.com/image.bin"}},
|
|
}
|
|
],
|
|
}
|
|
)
|
|
|
|
assert len(published) == 1
|
|
assert len(published[0].files) == 1
|
|
staged = Path(published[0].files[0]["path"])
|
|
staged_exists = await asyncio.to_thread(staged.exists)
|
|
assert staged_exists, "inbound image should be staged under the tmp state dir"
|
|
|
|
|
|
async def test_wechat_auth_state_load_does_not_block_event_loop(tmp_path: Path) -> None:
|
|
"""``_ensure_authenticated`` reads persisted auth state through ``asyncio.to_thread``."""
|
|
bus = MessageBus()
|
|
# bot_token="" forces _ensure_authenticated to fall through to the
|
|
# _load_auth_state branch (offloaded) instead of returning early.
|
|
channel = WechatChannel(bus=bus, config={"bot_token": "", "state_dir": str(tmp_path)})
|
|
|
|
auth_path = tmp_path / "wechat-auth.json"
|
|
await asyncio.to_thread(auth_path.write_text, json.dumps({"status": "confirmed", "bot_token": "loaded-token"}))
|
|
|
|
result = await channel._ensure_authenticated()
|
|
|
|
assert result is True
|
|
assert channel._bot_token == "loaded-token"
|