mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-28 17:06:05 +00:00
62b73fd2ea
* feat(dingtalk): support inbound file and image attachments DingTalk previously dropped picture and file (document) messages because `_on_chatbot_message` ignored any message with empty text, so users could not send files to the agent. This adds inbound attachment support, mirroring `FeishuChannel`: - `_extract_files` parses `picture`/`richText` image downloadCodes and `file` (document) descriptors. `dingtalk_stream.ChatbotMessage.from_dict` does not parse `file` messages, so `_DingTalkMessageHandler.process` stashes the raw callback payload on the message (`_df_raw_data`) for the document descriptor. - `receive_file` downloads each attachment by `downloadCode` via the robot `messageFiles/download` OpenAPI, persists it into the thread uploads bucket, syncs it into a non-local sandbox, and prepends the sandbox virtual path to the message text so the agent can read the file by path. - Filenames go through the shared `uploads.normalize_filename` helper, which strips directory components and rejects traversal patterns. Outbound `send_file` already existed; this completes DingTalk file parity with Feishu on the inbound side. Adds 21 tests covering extraction, download-by-code, persistence/sandbox sync, filename sanitization, and the handler raw-data stash. * fix(dingtalk): address inbound-file review feedback Follow-up to the review on #4423: - Make the fallback filename safe by construction. `download_code` is attacker-controllable webhook data and was embedded into `fallback_name` unsanitized; it only avoided escaping the uploads directory because the resulting write failed with OSError. It is now restricted to `[A-Za-z0-9_-]` before use. Covered by a test that reproduces the old behaviour (`uploads/dingtalk_../../evil.png`) and by a test that actually exercises the previously untested `except ValueError` branch (`".."`, whose basename — unlike `../../etc/passwd` — does raise). - Log the swallowed `get_image_list()` failure instead of silently returning no images, so an SDK parse failure is distinguishable from a richText message that genuinely has no inline images. - Surface failed downloads to the agent as a short `[failed to load ...]` marker rather than silently omitting the attachment, so a user whose file did not load does not simply appear to be ignored. Keeps the cleaner text shape while restoring the signal Feishu provides. Tests: 119 passed (was 115). * fix(dingtalk): claim unique upload names and refuse symlinked destinations Round 2 review follow-up on #4423. Both findings reproduce as failing tests against the previous head. - Inbound attachments no longer overwrite each other. Generated names repeat across messages (every picture message yields "image.png", richText yields "image_0.png"), so a later attachment silently replaced an earlier one whose virtual path had already been prepended to the message text — the agent could read bytes that were not the ones its prompt referenced. The destination name is now claimed with the shared `claim_unique_filename` against the live directory contents, which also covers a real filename sent twice (`quote.xlsx`), a case Feishu's inline naming does not handle either. The claim and the write happen under one lock so two attachments cannot resolve to the same free name. - Writes go through the shared `write_upload_file_no_symlink` instead of `Path.write_bytes`. Uploads dirs may be mounted into local sandboxes, so a sandbox process could leave a symlink at a future upload name and redirect a gateway-privileged write outside the bucket; the regression test shows the old code creating the out-of-bucket target. Tests: 123 passed (was 119). * fix(dingtalk): harden the inbound download path (self-audit) Proactive hardening pass over the new inbound path; each fix reproduces as a failing test against the previous head. - Contain token failures. `_get_access_token()` sat outside the try in `_download_by_code`, and the manager awaits `receive_file` without one — a DingTalk auth hiccup during a file message aborted the whole chat turn with no reply. Token acquisition moves inside the try, and `receive_file` gains per-attachment isolation so no unforeseen error can escape past the marker. - Cap inbound size. The download buffered arbitrary bytes in memory (`response.content`) with no limit, while outbound uploads already enforce one. The body is now streamed and dropped once it exceeds `_MAX_INBOUND_FILE_SIZE_BYTES` (50 MB), surfacing as a failed-load marker. - Sanitize the failure marker. It embedded the raw webhook `fileName`; a newline could forge a standalone `/mnt/user-data/uploads/...` line inside msg.text and an over-long name bloated it. Markers now collapse whitespace and cap at 80 chars. - Keep blocking IO off the event loop. `ensure_thread_dirs`, the uploads-dir resolve, sync `SandboxProvider.acquire`, and `sandbox.update_file` all ran on the loop; directory prep now lives inside the same `asyncio.to_thread` as the claim+write, and sandbox sync uses `acquire_async` + an offloaded `update_file`. Locked by a strict Blockbuster anchor (tests/blocking_io/test_dingtalk_receive_file.py), verified to fail with `BlockingError: Blocking call to os.mkdir` when the offload is reverted. Tests: 127 + 1 blocking-io anchor (was 123); tests/blocking_io/ suite 55 passed. * fix(dingtalk): surface missing-sandbox sync as a failed load Round 3 follow-up on #4423: - When a non-local sandbox acquire succeeds but the provider cannot resolve the instance, _receive_single_file returned the virtual path anyway — a path the agent's sandbox cannot read. Mirror Feishu: log and return "", so the [failed to load ...] marker fires instead. Red-first test: test_missing_sandbox_after_acquire_yields_marker. - Drop the dead GetResponse / FakeClient.get scaffolding left in test_oversized_download_is_dropped from its red-first iteration. Tests: 128 + 1 blocking-io anchor (was 127 + 1). * fix(dingtalk): treat non-local sandbox sync failure as a failed load Round 4 follow-up on #4423. The sync except-branch logged and still returned the virtual path when acquire or update_file raised on a non-local sandbox — the same handing-the-agent-an-unreadable-path failure mode the sandbox-is-None branch was just fixed for, and exactly the leg the suite did not exercise. Feishu's except-branch returns its failure marker; DingTalk now does the equivalent (return "" so the failed-load marker fires). Red-first test: test_update_file_failure_yields_marker. Tests: 129 + 1 blocking-io anchor (was 128 + 1). --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
57 lines
2.1 KiB
Python
57 lines
2.1 KiB
Python
"""Regression anchor: DingTalk ``receive_file`` must not block the event loop.
|
|
|
|
``_receive_single_file`` prepares the thread directories, resolves the uploads
|
|
dir, scans it for the uniqueness claim, and writes the attachment — all blocking
|
|
filesystem IO that must run inside ``asyncio.to_thread`` (and sandbox sync must
|
|
go through ``acquire_async`` + an offloaded ``update_file``). This anchor drives
|
|
the real ``receive_file`` under the strict Blockbuster gate; if any of that
|
|
regresses back onto the event loop, Blockbuster raises ``BlockingError``.
|
|
|
|
The ``Paths`` construction is offloaded only because ``Paths.__init__`` resolves
|
|
paths synchronously; the surface under test (``receive_file``'s persist path) is
|
|
exercised on the event loop, not bypassed. The download itself is mocked — the
|
|
network leg is httpx-async and not the subject here.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
from types import SimpleNamespace
|
|
from unittest.mock import AsyncMock
|
|
|
|
import pytest
|
|
|
|
pytestmark = pytest.mark.asyncio
|
|
|
|
|
|
async def test_receive_file_persist_does_not_block_event_loop(tmp_path, monkeypatch) -> None:
|
|
from app.channels.dingtalk import DingTalkChannel
|
|
from app.channels.message_bus import MessageBus
|
|
from deerflow.config.paths import Paths
|
|
|
|
paths = await asyncio.to_thread(Paths, str(tmp_path))
|
|
monkeypatch.setattr("app.channels.dingtalk.get_paths", lambda: paths)
|
|
|
|
async def _acquire_async(thread_id, user_id=None):
|
|
return "local"
|
|
|
|
monkeypatch.setattr(
|
|
"app.channels.dingtalk.get_sandbox_provider",
|
|
lambda: SimpleNamespace(acquire_async=_acquire_async, get=lambda sid: None),
|
|
)
|
|
|
|
channel = DingTalkChannel(MessageBus(), config={})
|
|
channel._download_by_code = AsyncMock(return_value=b"DATA")
|
|
|
|
msg = channel._make_inbound(
|
|
chat_id="c",
|
|
user_id="u",
|
|
text="hi",
|
|
thread_ts="m",
|
|
files=[{"type": "file", "download_code": "dc", "filename": "a.pdf"}],
|
|
)
|
|
out = await channel.receive_file(msg, "t1", user_id="default")
|
|
|
|
assert "/uploads/a.pdf" in out.text
|
|
assert out.files == []
|