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 # ---------------------------------------------------------------------------