From 2b6254f76de353ce4a4b9d056edf7ca194f7634e Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Sun, 20 Sep 2026 17:22:24 +0900 Subject: [PATCH] fix(client): stop embedded uploads from writing through symlinks (#5578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(client): stop embedded uploads from writing through symlinks DeerFlowClient.upload_files copied each file with shutil.copy2 and let convert_file_to_markdown write the companion straight into the uploads directory. Local and AIO sandboxes can write to that directory, so a symlink planted at an upload name or at the companion's name was followed: the upload's bytes and the converted Markdown landed in whatever host file the link pointed to, and the call reported success. The Gateway refuses symlinked destinations and the IM channels write through write_upload_file_no_symlink; the embedded client never adopted either. Uploads now go through copy_upload_file_no_symlink, a new helper next to write_upload_file_no_symlink. It keeps copy2's content, permission bits and timestamps, so files stay readable to Docker sandboxes, but applies them to the descriptor opened with O_NOFOLLOW and opens the source first so a missing source cannot truncate an existing upload. As in the Gateway, a file with an unsafe destination is skipped and listed in skipped_files, success turns false, and the message says how many were skipped. The companion is converted inside a private temporary directory and then written with write_upload_file_no_symlink; one whose name is unsafe is left out like a failed conversion, and the original upload is kept. * docs(changelog): note embedded upload symlink fix (#5578) * fix(client): keep copy2's same-file guard and companion permissions Review follow-up. Two regressions in the previous commit. copy_upload_file_no_symlink opened the destination before comparing it with the source, and that open truncates. Passing a file that already sits in the thread's uploads directory therefore copied an emptied file over itself: the upload reported success with size 0 and the original bytes were gone, where copy2 raised SameFileError and left the file alone. The destination is now compared with the source through os.path.samestat before anything is opened, so identity — including a hardlink or another spelling of the same path — raises SameFileError as before. The Markdown companion was published with write_upload_file_no_symlink, which creates a new file as 0600 and ignores the converted file's mode. Under umask 022 the companion became 0600 while its own document stayed 0644, so a bind-mounted sandbox running as another uid could read the upload but not the Markdown the response advertises. It now goes through the same copy helper as the upload, which preserves the converter's permission bits. --- CHANGELOG.md | 8 ++ CHANGELOG_zh.md | 5 + backend/docs/FILE_UPLOAD.md | 1 + backend/packages/harness/deerflow/client.py | 60 ++++++--- .../harness/deerflow/uploads/manager.py | 49 +++++++ backend/tests/test_client.py | 122 ++++++++++++++++++ backend/tests/test_uploads_manager.py | 97 ++++++++++++++ 7 files changed, 327 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17ead4193..6058aeea1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2781,6 +2781,13 @@ This release closes that milestone with **765 merged pull requests**. ### Security +- **client:** `DeerFlowClient.upload_files` no longer writes through a + symlink. A symlink planted in the sandbox-writable uploads directory, at an + upload's name or its Markdown companion's name, made the embedded client + overwrite the host file it pointed to while reporting success. The file is + now skipped and listed in `skipped_files` with `success: false`, matching + the Gateway; an unsafe companion is left out and the upload kept. Copies + keep the source's permission bits and timestamps. ([#5578]) - **uploads:** Deleting an upload no longer follows a symlink to delete a different file. A symlink planted in the sandbox-writable uploads directory made `DELETE /api/threads/{id}/uploads/{filename}` (and @@ -4310,3 +4317,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5526]: https://github.com/bytedance/deer-flow/pull/5526 [#5534]: https://github.com/bytedance/deer-flow/pull/5534 [#5547]: https://github.com/bytedance/deer-flow/pull/5547 +[#5578]: https://github.com/bytedance/deer-flow/pull/5578 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index eec703efd..425deeb7a 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -2091,6 +2091,10 @@ ### 安全 +- **客户端：** `DeerFlowClient.upload_files` 不再写穿符号链接。沙箱可写的 uploads 目录中， + 若在上传文件名或其 Markdown 配套文件名处放置符号链接，嵌入式客户端此前会覆盖链接指向的宿主 + 文件并报告成功。现在该文件会被跳过并列入 `skipped_files`，`success` 为 `false`，与 Gateway + 一致；不安全的配套文件会被省略，原上传保留。复制时保留源文件的权限位与时间戳。([#5578]) - **上传：** 删除上传文件时不再跟随符号链接删除另一个文件。沙箱可写的 uploads 目录中若被 放置符号链接，`DELETE /api/threads/{id}/uploads/{filename}`（以及 `DeerFlowClient.delete_upload`）此前会删除链接指向的上传文件及其配套 `.md`，却仍报告 @@ -3512,3 +3516,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5526]: https://github.com/bytedance/deer-flow/pull/5526 [#5534]: https://github.com/bytedance/deer-flow/pull/5534 [#5547]: https://github.com/bytedance/deer-flow/pull/5547 +[#5578]: https://github.com/bytedance/deer-flow/pull/5578 diff --git a/backend/docs/FILE_UPLOAD.md b/backend/docs/FILE_UPLOAD.md index f709554cf..1d02630e9 100644 --- a/backend/docs/FILE_UPLOAD.md +++ b/backend/docs/FILE_UPLOAD.md @@ -236,6 +236,7 @@ backend/.deer-flow/threads/ - 最大文件大小：100MB（可在 nginx.conf 中配置 `client_max_body_size`） - 文件名安全性：系统会自动验证文件路径，防止目录遍历攻击 - 删除只作用于普通文件：上传目录中的符号链接不会被跟随，删除请求按文件不存在（404）处理 +- 上传（HTTP 与嵌入式 `DeerFlowClient`）不会写穿符号链接：目标名已是符号链接的文件会被跳过并列入 `skipped_files`，转换生成的 Markdown 也不会写入同名符号链接 - 线程隔离：每个线程的上传文件相互隔离，无法跨线程访问 - 自动文档转换默认关闭；如需启用，需在 `config.yaml` 中显式设置 `uploads.auto_convert_documents: true` diff --git a/backend/packages/harness/deerflow/client.py b/backend/packages/harness/deerflow/client.py index 09efb3881..33b333f64 100644 --- a/backend/packages/harness/deerflow/client.py +++ b/backend/packages/harness/deerflow/client.py @@ -21,7 +21,7 @@ import copy import logging import mimetypes import os -import shutil +import tempfile import uuid from collections.abc import Generator, Iterator, Mapping, Sequence from dataclasses import dataclass, field @@ -69,7 +69,9 @@ from deerflow.tools.builtins.tool_search import assemble_deferred_tools, build_m from deerflow.trace_context import DEERFLOW_TRACE_METADATA_KEY, bind_trace_id, ensure_trace_id, generate_trace_id, get_current_trace_id, reset_trace_id from deerflow.tracing import build_tracing_callbacks, inject_langfuse_metadata from deerflow.uploads.manager import ( + UnsafeUploadPathError, claim_unique_filename, + copy_upload_file_no_symlink, delete_file_safe, enrich_file_listing, ensure_uploads_dir, @@ -1603,13 +1605,19 @@ class DeerFlowClient: For PDF, PPT, Excel, and Word files, they are also converted to Markdown. + The uploads directory is writable from inside the sandbox, so neither + the upload nor its Markdown companion is ever written through an + existing symlink. As in the Gateway, a file whose destination name is + a symlink or other non-regular file is skipped and listed in + ``skipped_files``; a companion with such a name is left out. + Args: thread_id: Target thread ID. files: List of local file paths to upload. Returns: - Dict with success, files, message — matching the Gateway API - ``UploadResponse`` schema. + Dict with success, files, message, skipped_files — matching the + Gateway API ``UploadResponse`` schema. Raises: FileNotFoundError: If any file does not exist. @@ -1635,6 +1643,7 @@ class DeerFlowClient: uploads_dir = ensure_uploads_dir(thread_id) uploaded_files: list[dict] = [] + skipped_files: list[str] = [] conversion_pool = None if has_convertible_file: @@ -1654,8 +1663,12 @@ class DeerFlowClient: try: for src_path, dest_name in resolved_files: - dest = uploads_dir / dest_name - shutil.copy2(src_path, dest) + try: + dest = copy_upload_file_no_symlink(uploads_dir, dest_name, src_path) + except UnsafeUploadPathError: + logger.warning("Skipping upload with unsafe destination: %s", dest_name) + skipped_files.append(dest_name) + continue info: dict[str, Any] = { "filename": dest_name, @@ -1673,12 +1686,24 @@ class DeerFlowClient: # cannot silently overwrite each other. provisional_md_name = Path(dest_name).with_suffix(".md").name unique_md_name = claim_unique_filename(provisional_md_name, seen_names) - md_output = dest.with_name(unique_md_name) try: - if conversion_pool is not None: - md_path = conversion_pool.submit(_convert_in_thread, dest, md_output).result() - else: - md_path = asyncio.run(convert_file_to_markdown(dest, output_path=md_output)) + # Convert outside the sandbox-writable uploads dir, then + # publish without following a symlink at the companion name. + with tempfile.TemporaryDirectory() as md_dir: + md_output = Path(md_dir) / unique_md_name + if conversion_pool is not None: + converted = conversion_pool.submit(_convert_in_thread, dest, md_output).result() + else: + converted = asyncio.run(convert_file_to_markdown(dest, output_path=md_output)) + md_path = None + if converted is not None: + # copy, not write_bytes: the companion keeps the + # converter's permissions, so a sandbox running as + # another uid can still read it. + md_path = copy_upload_file_no_symlink(uploads_dir, unique_md_name, converted) + except UnsafeUploadPathError: + logger.warning("Skipping markdown companion with unsafe destination: %s", unique_md_name) + md_path = None except Exception: logger.warning( "Failed to convert %s to markdown", @@ -1693,9 +1718,9 @@ class DeerFlowClient: info["markdown_virtual_path"] = upload_virtual_path(md_path.name) info["markdown_artifact_url"] = upload_artifact_url(thread_id, md_path.name) else: - # Conversion failed and wrote nothing, so release the - # claim; holding it would rename a later same-stem - # upload against a name nothing occupies. + # No companion was written, so release the claim; + # holding it would rename a later same-stem upload + # against a name this request never filled. seen_names.discard(unique_md_name) uploaded_files.append(info) @@ -1703,10 +1728,15 @@ class DeerFlowClient: if conversion_pool is not None: conversion_pool.shutdown(wait=True) + message = f"Successfully uploaded {len(uploaded_files)} file(s)" + if skipped_files: + message += f"; skipped {len(skipped_files)} unsafe file(s)" + return { - "success": True, + "success": not skipped_files, "files": uploaded_files, - "message": f"Successfully uploaded {len(uploaded_files)} file(s)", + "message": message, + "skipped_files": skipped_files, } def list_uploads(self, thread_id: str) -> dict: diff --git a/backend/packages/harness/deerflow/uploads/manager.py b/backend/packages/harness/deerflow/uploads/manager.py index db7d1f193..b8d3bd7c9 100644 --- a/backend/packages/harness/deerflow/uploads/manager.py +++ b/backend/packages/harness/deerflow/uploads/manager.py @@ -7,6 +7,7 @@ Both Gateway and Client delegate to these functions. import errno import logging import os +import shutil import stat from pathlib import Path from urllib.parse import quote @@ -284,6 +285,54 @@ def write_upload_file_no_symlink(base_dir: Path, filename: str, data: bytes) -> return dest +def _reject_same_file(base_dir: Path, filename: str, src: Path, src_stat: os.stat_result) -> None: + """Raise :class:`shutil.SameFileError` when *filename* already is *src*. + + Compares identity with ``os.path.samestat`` — what ``copy2`` itself uses — + rather than the path text, so a hardlink or a differently spelled path to + the same file is caught too. + ``lstat`` keeps a planted symlink from being resolved here; the open + itself rejects that destination. + """ + dest = base_dir / normalize_filename(filename) + try: + dest_stat = os.lstat(dest) + except (FileNotFoundError, NotADirectoryError): + return + if os.path.samestat(src_stat, dest_stat): + raise shutil.SameFileError(f"{src!r} and {dest!r} are the same file") + + +def copy_upload_file_no_symlink(base_dir: Path, filename: str, src: Path) -> Path: + """Copy *src* into an upload destination without following a destination symlink. + + Matches ``shutil.copy2`` for content, permission bits and timestamps, but + opens the destination through :func:`open_upload_file_no_symlink` and + applies the metadata to that descriptor, never to the name. The source is + opened first, so a missing source leaves an existing destination intact. + Where descriptor-based ``chmod``/``utime`` are unavailable (Windows), the + destination keeps its default mode and the copy time. + + Copying a file onto itself raises :class:`shutil.SameFileError` as + ``copy2`` does, and does so before the destination is opened: opening it + truncates, which would otherwise leave the caller copying an emptied file + over itself. Re-uploading a file that already sits in the uploads + directory takes exactly that path. + """ + with open(src, "rb") as src_fh: + src_stat = os.fstat(src_fh.fileno()) + _reject_same_file(base_dir, filename, src, src_stat) + dest, fh = open_upload_file_no_symlink(base_dir, filename) + with fh: + shutil.copyfileobj(src_fh, fh) + fh.flush() + if os.chmod in os.supports_fd: + os.chmod(fh.fileno(), stat.S_IMODE(src_stat.st_mode)) + if os.utime in os.supports_fd: + os.utime(fh.fileno(), ns=(src_stat.st_atime_ns, src_stat.st_mtime_ns)) + return dest + + def list_files_in_dir(directory: Path) -> dict: """List files (not directories) in *directory*. diff --git a/backend/tests/test_client.py b/backend/tests/test_client.py index 12ba05544..78c7d9bff 100644 --- a/backend/tests/test_client.py +++ b/backend/tests/test_client.py @@ -3,6 +3,9 @@ import asyncio import concurrent.futures import json +import os +import shutil +import stat import tempfile import zipfile from enum import Enum @@ -2709,6 +2712,51 @@ class TestUploads: assert (uploads_dir / "a.md").read_text(encoding="utf-8") == "FROM:a.pdf" assert not (uploads_dir / "a_1.md").exists() + def test_upload_files_rejects_reuploading_a_file_already_in_the_thread(self, client): + """Uploading an existing upload onto itself must not destroy its bytes.""" + with tempfile.TemporaryDirectory() as tmp: + uploads_dir = Path(tmp) / "uploads" + uploads_dir.mkdir() + existing = uploads_dir / "existing.txt" + existing.write_text("IMPORTANT BYTES") + + with patch("deerflow.client.get_uploads_dir", return_value=uploads_dir), patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir): + with pytest.raises(shutil.SameFileError): + client.upload_files("thread-1", [existing]) + + assert existing.read_text() == "IMPORTANT BYTES" + + def test_upload_files_markdown_companion_keeps_converted_permissions(self, client): + """The companion stays as readable as the converter wrote it (sandbox reads it).""" + if os.chmod not in os.supports_fd: + pytest.skip("descriptor-based chmod is unavailable on this platform") + + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + pdf = tmp_path / "report.pdf" + pdf.write_bytes(b"PDF") + + async def fake_convert(path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else path.with_suffix(".md") + md_path.write_text("converted", encoding="utf-8") + os.chmod(md_path, 0o644) + return md_path + + with ( + patch("deerflow.client.get_uploads_dir", return_value=uploads_dir), + patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir), + patch("deerflow.utils.file_conversion.CONVERTIBLE_EXTENSIONS", {".pdf"}), + patch("deerflow.utils.file_conversion.convert_file_to_markdown", side_effect=fake_convert), + ): + result = client.upload_files("thread-1", [pdf]) + + assert result["files"][0]["markdown_file"] == "report.md" + companion = uploads_dir / "report.md" + assert companion.read_text(encoding="utf-8") == "converted" + assert stat.S_IMODE(companion.stat().st_mode) == 0o644 + def test_list_uploads(self, client): with tempfile.TemporaryDirectory() as tmp: uploads_dir = Path(tmp) @@ -4365,6 +4413,80 @@ class TestUploadDeleteSymlink: assert victim.read_text() == "keep me" assert link.is_symlink() + def test_upload_files_skips_symlinked_destination(self, client): + """A symlink planted at an upload name is skipped, not written through.""" + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + + outside = tmp_path / "outside.txt" + outside.write_text("original") + link = uploads_dir / "note.txt" + try: + link.symlink_to(outside) + except OSError as exc: + if getattr(exc, "winerror", None) == 1314: + pytest.skip("symlink creation requires Developer Mode or elevated privileges on Windows") + raise + + src_dir = tmp_path / "src" + src_dir.mkdir() + (src_dir / "note.txt").write_text("uploaded") + (src_dir / "other.txt").write_text("other") + + with patch("deerflow.client.get_uploads_dir", return_value=uploads_dir), patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir): + result = client.upload_files("thread-1", [src_dir / "note.txt", src_dir / "other.txt"]) + + parsed = UploadResponse(**result) + assert parsed.success is False + assert parsed.skipped_files == ["note.txt"] + assert [f.filename for f in parsed.files] == ["other.txt"] + assert parsed.message == "Successfully uploaded 1 file(s); skipped 1 unsafe file(s)" + assert outside.read_text() == "original" + assert link.is_symlink() + assert (uploads_dir / "other.txt").read_text() == "other" + + def test_upload_files_does_not_write_markdown_companion_through_symlink(self, client): + """A symlink planted at the companion name does not receive converted text.""" + with tempfile.TemporaryDirectory() as tmp: + tmp_path = Path(tmp) + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + + outside = tmp_path / "outside.md" + outside.write_text("original") + link = uploads_dir / "report.md" + try: + link.symlink_to(outside) + except OSError as exc: + if getattr(exc, "winerror", None) == 1314: + pytest.skip("symlink creation requires Developer Mode or elevated privileges on Windows") + raise + + pdf = tmp_path / "report.pdf" + pdf.write_bytes(b"PDF") + + async def fake_convert(path: Path, output_path: Path | None = None) -> Path: + md_path = output_path if output_path is not None else path.with_suffix(".md") + md_path.write_text(f"FROM:{path.name}", encoding="utf-8") + return md_path + + with ( + patch("deerflow.client.get_uploads_dir", return_value=uploads_dir), + patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir), + patch("deerflow.utils.file_conversion.CONVERTIBLE_EXTENSIONS", {".pdf"}), + patch("deerflow.utils.file_conversion.convert_file_to_markdown", side_effect=fake_convert), + ): + result = client.upload_files("thread-1", [pdf]) + + assert result["success"] is True + assert [f["filename"] for f in result["files"]] == ["report.pdf"] + assert "markdown_file" not in result["files"][0] + assert outside.read_text() == "original" + assert link.is_symlink() + assert (uploads_dir / "report.pdf").read_bytes() == b"PDF" + def test_upload_filename_with_spaces_and_unicode(self, client): """Files with spaces and unicode characters in names upload correctly.""" with tempfile.TemporaryDirectory() as tmp: diff --git a/backend/tests/test_uploads_manager.py b/backend/tests/test_uploads_manager.py index 9bbc004eb..2f38405f6 100644 --- a/backend/tests/test_uploads_manager.py +++ b/backend/tests/test_uploads_manager.py @@ -2,6 +2,8 @@ import errno import os +import shutil +import stat from unittest.mock import patch import pytest @@ -11,6 +13,7 @@ from deerflow.uploads.manager import ( UnsafeUploadPathError, claim_unique_filename, cleanup_stale_upload_staging_files, + copy_upload_file_no_symlink, delete_file_safe, list_files_in_dir, normalize_filename, @@ -191,6 +194,100 @@ class TestWriteUploadFileNoSymlink: assert not (tmp_path / "pipe.txt").exists() +# --------------------------------------------------------------------------- +# copy_upload_file_no_symlink +# --------------------------------------------------------------------------- + + +class TestCopyUploadFileNoSymlink: + def test_copies_content_mode_and_timestamps(self, tmp_path): + uploads = tmp_path / "uploads" + uploads.mkdir() + src = tmp_path / "notes.txt" + src.write_bytes(b"hello") + os.chmod(src, 0o640) + os.utime(src, ns=(1_700_000_000_000_000_000, 1_700_000_000_000_000_000)) + + dest = copy_upload_file_no_symlink(uploads, "notes.txt", src) + + assert dest == uploads / "notes.txt" + assert dest.read_bytes() == b"hello" + if os.chmod in os.supports_fd: + assert stat.S_IMODE(os.stat(dest).st_mode) == 0o640 + if os.utime in os.supports_fd: + assert os.stat(dest).st_mtime_ns == 1_700_000_000_000_000_000 + + def test_overwrites_existing_regular_file(self, tmp_path): + uploads = tmp_path / "uploads" + uploads.mkdir() + (uploads / "notes.txt").write_bytes(b"old contents") + src = tmp_path / "notes.txt" + src.write_bytes(b"new contents") + + dest = copy_upload_file_no_symlink(uploads, "notes.txt", src) + + assert dest.read_bytes() == b"new contents" + + def test_rejects_symlink_destination(self, tmp_path): + uploads = tmp_path / "uploads" + uploads.mkdir() + outside = tmp_path / "outside.txt" + outside.write_bytes(b"original") + link = uploads / "notes.txt" + try: + link.symlink_to(outside) + except OSError as exc: + if getattr(exc, "winerror", None) == 1314: + pytest.skip("symlink creation requires Developer Mode or elevated privileges on Windows") + raise + src = tmp_path / "notes.txt" + src.write_bytes(b"attacker-chosen target") + + with pytest.raises(UnsafeUploadPathError): + copy_upload_file_no_symlink(uploads, "notes.txt", src) + + assert outside.read_bytes() == b"original" + assert link.is_symlink() + + def test_rejects_copying_a_file_onto_itself(self, tmp_path): + uploads = tmp_path / "uploads" + uploads.mkdir() + src = uploads / "notes.txt" + src.write_bytes(b"IMPORTANT") + + with pytest.raises(shutil.SameFileError): + copy_upload_file_no_symlink(uploads, "notes.txt", src) + + assert src.read_bytes() == b"IMPORTANT" + + def test_rejects_a_hardlink_to_the_destination(self, tmp_path): + """Identity, not path text: another name for the same inode is the same file.""" + uploads = tmp_path / "uploads" + uploads.mkdir() + dest = uploads / "notes.txt" + dest.write_bytes(b"IMPORTANT") + src = uploads / "same-inode.txt" + try: + os.link(dest, src) + except (OSError, NotImplementedError) as exc: + pytest.skip(f"hardlinks unavailable on this platform: {exc}") + + with pytest.raises(shutil.SameFileError): + copy_upload_file_no_symlink(uploads, "notes.txt", src) + + assert dest.read_bytes() == b"IMPORTANT" + + def test_missing_source_leaves_existing_destination_untouched(self, tmp_path): + uploads = tmp_path / "uploads" + uploads.mkdir() + (uploads / "notes.txt").write_bytes(b"keep me") + + with pytest.raises(FileNotFoundError): + copy_upload_file_no_symlink(uploads, "notes.txt", tmp_path / "missing.txt") + + assert (uploads / "notes.txt").read_bytes() == b"keep me" + + # --------------------------------------------------------------------------- # list_files_in_dir # ---------------------------------------------------------------------------