diff --git a/README.md b/README.md index 55ee67354..1720554cb 100644 --- a/README.md +++ b/README.md @@ -961,7 +961,7 @@ DeerFlow doesn't just *talk* about doing things. It has its own computer. Each task gets its own execution environment with a full filesystem view — skills, workspace, uploads, outputs. The agent reads, writes, and edits files. It can view images and, when configured safely, execute shell commands. -Uploads from the Web UI, embedded client, and IM channels share one collision-safe storage rule. A completed payload is published only if its candidate name does not exist; concurrent `report.pdf` uploads become `report.pdf`, `report_1.pdf`, `report_2.pdf`, and so on without replacing one another. A busy candidate lease is treated as another collision instead of waiting, so inverse multi-file batches cannot deadlock. The selected name is leased through conversion and sandbox synchronization, so deleting that exact name waits for its active upload lifecycle while unrelated filenames continue concurrently. Internal staging names matching `.upload-*.part` and basenames that cannot be represented losslessly on Windows are rejected. +Uploads from the Web UI, embedded client, and IM channels share one collision-safe storage rule. A completed payload is published only if its candidate name does not exist; concurrent `report.pdf` uploads become `report.pdf`, `report_1.pdf`, `report_2.pdf`, and so on without replacing one another. Collision suffixes remain within the filesystem's 255-byte UTF-8 component limit, including names whose original suffix consumes nearly the whole limit. A busy candidate lease is treated as another collision instead of waiting, so inverse multi-file batches cannot deadlock. The selected name is leased through conversion and sandbox synchronization, so deleting that exact name waits for its active upload lifecycle while unrelated filenames continue concurrently. Internal staging names matching `.upload-*.part` and basenames that cannot be represented losslessly on Windows are rejected for new uploads; exact legacy POSIX names already returned by the list endpoint remain deletable after upgrade. Optional document conversions are system-owned assets under `/mnt/user-data/.upload-conversions/`. Normal targets use `.md`; names that would exceed the filesystem component limit use a deterministic UTF-8-safe prefix plus the full SHA-256 digest. The exact generated path is returned through the upload response and omitted from the primary upload listing. Mounted AIO sandboxes expose this namespace through a read-only mount, and Local structured file APIs enforce the same rule through path mappings. Local host bash is outside that mapping boundary and must remain disabled for untrusted tasks. Non-mounted remote providers receive a private synchronized copy that may be writable but cannot mutate the authoritative host conversion or lock state. Deleting a primary removes only its exact generated asset and never infers that a user-uploaded sibling such as `uploads/report.md` is disposable. diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 8431e53f6..02aff7fbb 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -1461,7 +1461,7 @@ Multi-file upload with automatic document conversion: - Reuses one conversion worker per request when called from an active event loop - Files stored in thread-isolated directories under the resolving user's bucket (`users/{user_id}/threads/{thread_id}/user-data/uploads`). For IM channels the owner is threaded explicitly via the `user_id=` kwarg (see IM Channels → Owner-scoped file storage); HTTP/embedded callers resolve it from `get_effective_user_id()` - Every ingress stages a complete payload and atomically publishes it without replacing an existing entry. Collisions across requests, processes, HTTP, embedded, and IM adapters use `name.ext`, `name_1.ext`, `name_2.ext`; storage that cannot provide atomic no-replace publication fails explicitly. -- Exact-name generation leases use a portable NFC-plus-casefold coordination key with Win32 trailing-dot/space folding, so filesystem aliases cannot bypass an active generation; filenames that Windows cannot represent losslessly are rejected before staging. The original filename remains the published name. Publication tries each candidate lease without blocking and treats a busy canonical key as a collision, so same-batch and inverse concurrent batches advance to `_N` instead of deadlocking while retaining earlier generations. Alias-based deletion resolves the primary's actual directory entry by inode, requires that entry to remain exclusive, and only then derives a long-name conversion path. Final lease release is the commit point: cancellation newly arriving during release is delayed and swallowed so a committed upload is returned as success rather than an indeterminate cancelled result. +- Exact-name generation leases use a portable NFC-plus-casefold coordination key with Win32 trailing-dot/space folding, so filesystem aliases cannot bypass an active generation; filenames that Windows cannot represent losslessly are rejected before staging, while exact legacy POSIX basenames remain deletable after upgrade. The original filename remains the published name. Publication tries each candidate lease without blocking and treats a busy canonical key as a collision, so same-batch and inverse concurrent batches advance to a UTF-8-bounded `_N` candidate instead of deadlocking while retaining earlier generations; pathological long suffixes fall back to truncating the complete basename. Alias-based deletion resolves the primary's actual directory entry by inode, requires that entry to remain exclusive, and only then derives a long-name conversion path. Final lease release is the commit point: cancellation newly arriving during release is delayed and swallowed so a committed upload is returned as success rather than an indeterminate cancelled result. Embedded-client finalization attempts every lease release and conversion-pool shutdown independently, logging cleanup failures rather than changing an already-committed response or stranding later leases. - Filenames containing NUL, `<`, `>`, or reserved model-context boundary markers are rejected before staging so accepted filenames and exact virtual paths remain lossless in model-visible upload context. Legacy files discovered on disk are still neutralized when listed. - Gateway HTTP uploads use same-directory `.upload-*.part` staging files. Each active stage holds a cross-process liveness lock under `.upload-conversions/.locks/stages/`; startup cleanup skips held stages and sweeps only crash-orphaned files. Cancellation during staging creation drains the worker and aborts the returned stage before propagating. Staging files are hidden from upload listings, agent upload context, and sandbox listing/search tools. - Generated Markdown is owned by `user-data/.upload-conversions/.md` and is omitted from primary upload listings. Deletion removes only the selected primary and that exact generated asset; it never guesses or deletes a legacy/user-owned `uploads/.md` sibling. diff --git a/backend/docs/API.md b/backend/docs/API.md index 76e74e3a8..706ef9b4b 100644 --- a/backend/docs/API.md +++ b/backend/docs/API.md @@ -636,7 +636,7 @@ Content-Type: multipart/form-data - Excel (`.xls`, `.xlsx`) - Word (`.doc`, `.docx`) -All upload entry points publish complete payloads without replacing an existing name. Concurrent collisions are returned as `document.pdf`, `document_1.pdf`, `document_2.pdf`, and so on. A published filename remains leased through conversion, permission adjustment, sandbox synchronization, and response construction; portable case, Unicode-normalization, and Win32 trailing-dot/space aliases share the same coordination key. Publication never waits on a busy candidate lease and advances to `_N`, preventing inverse multi-file batches from deadlocking; deletion waits for the target generation and rejects ambiguous hard-linked identities. Mounted providers make the exact published paths sandbox-readable; non-mounted providers receive exact private copies for Gateway, embedded-client, and IM-channel ingresses. If a non-mounted sandbox update later fails or the request is cancelled, DeerFlow removes every exact remote path attempted by that request before rolling back its host generations. Gateway cancellation also drains and aborts an in-flight staging creation. Final lease release is the commit point: cancellation newly arriving during release is delayed and the already-built successful response is returned. Basenames matching the internal `.upload-*.part` staging pattern, containing NUL, `<`, or `>`, containing reserved model-context boundary markers, or invalid/reserved on Windows are rejected before staging so every accepted model-visible filename and path can be rendered losslessly. Embedded multi-file calls are request-atomic: a later failure rolls back every earlier host and remote generation in that call. +All upload entry points publish complete payloads without replacing an existing name. Concurrent collisions are returned as `document.pdf`, `document_1.pdf`, `document_2.pdf`, and so on. Collision candidates remain within the 255-byte UTF-8 component limit; when a pathological suffix consumes nearly the entire component, DeerFlow truncates the complete basename before appending `_N`. A published filename remains leased through conversion, permission adjustment, sandbox synchronization, and response construction; portable case, Unicode-normalization, and Win32 trailing-dot/space aliases share the same coordination key. Publication never waits on a busy candidate lease and advances to `_N`, preventing inverse multi-file batches from deadlocking; deletion waits for the target generation and rejects ambiguous hard-linked identities. Mounted providers make the exact published paths sandbox-readable; non-mounted providers receive exact private copies for Gateway, embedded-client, and IM-channel ingresses. If a non-mounted sandbox update later fails or the request is cancelled, DeerFlow removes every exact remote path attempted by that request before rolling back its host generations. Gateway cancellation also drains and aborts an in-flight staging creation. Final lease release is the commit point: cancellation newly arriving during release is delayed and the already-built successful response is returned. Basenames matching the internal `.upload-*.part` staging pattern, containing NUL, `<`, or `>`, containing reserved model-context boundary markers, or invalid/reserved on Windows are rejected before staging so every accepted model-visible filename and path can be rendered losslessly. Embedded multi-file calls are request-atomic: a later failure rolls back every earlier host and remote generation in that call. Generated Markdown is stored outside the primary namespace and is not returned by the list endpoint. Normal conversion names are `.md`; if that component would exceed 255 UTF-8 bytes, the response contains a deterministic UTF-8-safe prefix plus the full SHA-256 digest and `.md`. Clients must consume the returned `markdown_*` fields rather than derive the path. Mounted AIO sandboxes use a read-only conversion mount, while Local structured file APIs reject writes through a read-only path mapping; Local host bash is outside that boundary. Non-mounted providers receive a private synchronized copy rather than the authoritative host namespace. Deleting `document.pdf` also deletes only its exact generated conversion; an independent `uploads/document.md` is preserved. @@ -680,7 +680,9 @@ DELETE /api/threads/{thread_id}/uploads/{filename} If an upload, conversion, or sandbox synchronization still owns this exact filename, the delete waits for that lifecycle to finish before removing the primary and its generated -conversion. Work on unrelated filenames is not serialized. +conversion. Work on unrelated filenames is not serialized. On POSIX deployments, exact +legacy names returned by the list endpoint remain deletable after upgrade even when the +same names would fail the stricter cross-platform validation applied to new uploads. ### Thread Cleanup diff --git a/backend/docs/FILE_UPLOAD.md b/backend/docs/FILE_UPLOAD.md index 014bbeed8..0615e4fe2 100644 --- a/backend/docs/FILE_UPLOAD.md +++ b/backend/docs/FILE_UPLOAD.md @@ -110,7 +110,7 @@ DELETE /api/threads/{thread_id}/uploads/{filename} } ``` -删除 `document.pdf` 时，会先等待该实际文件名当前正在进行的上传、转换或沙箱同步生命周期结束，然后只额外删除它精确拥有的生成资产。系统不会推断或删除 `uploads/document.md`；该文件可能是用户独立上传的内容。其他文件名不会被这次等待阻塞。 +删除 `document.pdf` 时，会先等待该实际文件名当前正在进行的上传、转换或沙箱同步生命周期结束，然后只额外删除它精确拥有的生成资产。系统不会推断或删除 `uploads/document.md`；该文件可能是用户独立上传的内容。其他文件名不会被这次等待阻塞。在 POSIX 部署上，升级前已经存在且能被列表接口返回的 Windows 非兼容文件名（例如 `CON`、`report?.pdf` 或末尾带空格的名称）仍可按返回的精确名称删除；新上传仍执行严格的跨平台文件名校验。 ## 支持的文档格式 @@ -130,7 +130,7 @@ Deletion: 删除 report.pdf 时只删除 .upload-conversions/report.pdf.md； /mnt/user-data/uploads/report.md 永远不会被推断为生成文件或自动删除。 ``` -通常生成名为 `<实际主文件名>.md`。如果这一文件名组件会超过 255 个 UTF-8 字节，系统会使用 UTF-8 安全截断的主文件名前缀、完整 SHA-256 摘要和 `.md`，并在响应中返回精确的 `markdown_*` 路径。客户端和 Agent 不应自行拼接生成路径。AIO 挂载模式把 `.upload-conversions` 显式挂载为只读；Local 的结构化文件 API 通过只读路径映射拒绝写入，但可选的 Local 宿主机 bash 不受该映射约束，不应对不受信任任务启用。非挂载远端沙箱得到的是独立同步副本，该副本可能可写，但不会修改宿主机上的权威生成文件或内部锁。 +通常生成名为 `<实际主文件名>.md`。如果这一文件名组件会超过 255 个 UTF-8 字节，系统会使用 UTF-8 安全截断的主文件名前缀、完整 SHA-256 摘要和 `.md`，并在响应中返回精确的 `markdown_*` 路径。上传名称碰撞产生的 `_N` 名称同样始终限制在 255 个 UTF-8 字节内；极端情况下，如果后缀本身占满几乎整个组件，系统会截断完整原名后再追加 `_N`。客户端和 Agent 不应自行拼接生成路径。AIO 挂载模式把 `.upload-conversions` 显式挂载为只读；Local 的结构化文件 API 通过只读路径映射拒绝写入，但可选的 Local 宿主机 bash 不受该映射约束，不应对不受信任任务启用。非挂载远端沙箱得到的是独立同步副本，该副本可能可写，但不会修改宿主机上的权威生成文件或内部锁。 默认情况下，自动转换是关闭的，以避免在网关主机上对不受信任的 Office/PDF 上传执行解析。只有在受信任部署中明确接受此风险时，才应将 `uploads.auto_convert_documents` 设置为 `true`。 diff --git a/backend/packages/harness/deerflow/client.py b/backend/packages/harness/deerflow/client.py index ee154c242..0cba5f23b 100644 --- a/backend/packages/harness/deerflow/client.py +++ b/backend/packages/harness/deerflow/client.py @@ -1626,9 +1626,19 @@ class DeerFlowClient: raise finally: for publication in reversed(publications): - publication.release() + try: + publication.release() + except Exception: + logger.warning( + "Failed to release committed embedded upload lease: %s", + publication.path, + exc_info=True, + ) if conversion_pool is not None: - conversion_pool.shutdown(wait=True) + try: + conversion_pool.shutdown(wait=True) + except Exception: + logger.warning("Failed to shut down embedded upload conversion pool", exc_info=True) def list_uploads(self, thread_id: str) -> dict: """List files in a thread's uploads directory. diff --git a/backend/packages/harness/deerflow/uploads/manager.py b/backend/packages/harness/deerflow/uploads/manager.py index dfd8d8e3b..5acb8684c 100644 --- a/backend/packages/harness/deerflow/uploads/manager.py +++ b/backend/packages/harness/deerflow/uploads/manager.py @@ -111,6 +111,24 @@ def normalize_filename(filename: str) -> str: return safe +def _normalize_existing_filename(filename: str) -> str: + """Validate an exact existing basename without applying new-upload policy. + + POSIX deployments may contain names accepted by older DeerFlow versions + that are not portable to Windows. They remain deletable after upgrade. + """ + if not filename: + raise ValueError("Filename is empty") + if "\0" in filename: + raise ValueError(f"Filename contains NUL byte: {filename!r}") + safe = Path(filename).name + if not safe or safe in {".", ".."}: + raise ValueError(f"Filename is unsafe: {filename!r}") + if is_upload_staging_file(safe): + raise ValueError(f"Filename uses reserved upload staging pattern: {filename!r}") + return safe + + def claim_unique_filename(name: str, seen: set[str]) -> str: """Generate a unique filename by appending ``_N`` suffix on collision. @@ -233,13 +251,31 @@ def _filename_candidates(name: str) -> Iterator[str]: counter = 1 while True: marker = f"_{counter}" - max_stem_bytes = 255 - len(marker.encode("utf-8")) - len(suffix.encode("utf-8")) + marker_bytes = len(marker.encode("utf-8")) + suffix_bytes = len(suffix.encode("utf-8")) + max_stem_bytes = 255 - marker_bytes - suffix_bytes if max_stem_bytes < 1: - raise AtomicUploadPublishError("Filename suffix leaves no room for collision marker") - candidate_stem = _truncate_utf8(stem, max_stem_bytes) + # Path.suffix can consume almost the entire component (for example + # ``a.`` plus 253 extension bytes). In that pathological case, + # treat the complete basename as the collision stem and truncate + # its tail so the marker always fits. + candidate_source = name + candidate_suffix = "" + max_stem_bytes = 255 - marker_bytes + else: + candidate_source = stem + candidate_suffix = suffix + if max_stem_bytes < 1: + raise AtomicUploadPublishError("Filename leaves no room for collision marker") + candidate_stem = _truncate_utf8(candidate_source, max_stem_bytes) + if not candidate_stem and candidate_suffix: + candidate_source = name + candidate_suffix = "" + max_stem_bytes = 255 - marker_bytes + candidate_stem = _truncate_utf8(candidate_source, max_stem_bytes) if not candidate_stem: raise AtomicUploadPublishError("Filename stem leaves no room for collision marker") - yield f"{candidate_stem}{marker}{suffix}" + yield f"{candidate_stem}{marker}{candidate_suffix}" counter += 1 @@ -593,7 +629,7 @@ def delete_file_safe(base_dir: Path, filename: str) -> dict: FileNotFoundError: If the file does not exist. PathTraversalError: If path traversal is detected. """ - safe_name = normalize_filename(filename) + safe_name = _normalize_existing_filename(filename) if safe_name != filename: raise PathTraversalError("Path traversal detected") try: diff --git a/backend/tests/test_client.py b/backend/tests/test_client.py index 9a8da2e3d..3549affbb 100644 --- a/backend/tests/test_client.py +++ b/backend/tests/test_client.py @@ -3,6 +3,7 @@ import asyncio import concurrent.futures import json +import os import tempfile import threading import zipfile @@ -2124,6 +2125,61 @@ class TestMemoryManagement: class TestUploads: + @pytest.mark.asyncio + async def test_release_failure_does_not_strand_later_leases_or_conversion_pool(self, client, tmp_path): + from deerflow.uploads.lease import UploadNameLease + + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + first = tmp_path / "one.pdf" + second = tmp_path / "two.txt" + first.write_bytes(b"PDF") + second.write_bytes(b"text") + real_executor = concurrent.futures.ThreadPoolExecutor + created_executors = [] + + class TrackingExecutor(real_executor): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.shutdown_called = False + created_executors.append(self) + + def shutdown(self, *args, **kwargs): + self.shutdown_called = True + return super().shutdown(*args, **kwargs) + + async def no_conversion(_path, *, publication=None): + assert publication is not None + return None + + real_release = UploadNameLease.release + injected = False + + def release_with_one_error(lease): + nonlocal injected + real_release(lease) + if lease.filename == "two.txt" and not injected: + injected = True + raise OSError("injected release failure") + + with ( + patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir), + patch("deerflow.utils.file_conversion.CONVERTIBLE_EXTENSIONS", {".pdf"}), + patch("deerflow.client.convert_uploaded_file_to_markdown", side_effect=no_conversion), + patch("concurrent.futures.ThreadPoolExecutor", TrackingExecutor), + patch.object(UploadNameLease, "release", autospec=True, side_effect=release_with_one_error), + ): + result = client.upload_files("thread-release", [first, second]) + + assert result["success"] is True + assert injected + assert len(created_executors) == 1 + assert created_executors[0].shutdown_called + for filename in ["one.pdf", "two.txt"]: + lease = UploadNameLease.try_acquire(uploads_dir, filename) + assert lease is not None + lease.release() + def test_delete_waits_for_client_conversion_and_metadata(self, client, tmp_path): uploads_dir = tmp_path / "user-data" / "uploads" uploads_dir.mkdir(parents=True) @@ -2568,6 +2624,19 @@ class TestUploads: with pytest.raises(FileNotFoundError): client.delete_upload("thread-1", "nope.txt") + @pytest.mark.skipif(os.name == "nt", reason="POSIX legacy filenames are not representable on Windows") + def test_delete_upload_accepts_legacy_posix_filename(self, client, tmp_path): + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + legacy = uploads_dir / "report?.pdf" + legacy.write_bytes(b"legacy") + + with patch("deerflow.client.get_uploads_dir", return_value=uploads_dir): + result = client.delete_upload("thread-1", legacy.name) + + assert result["success"] is True + assert not legacy.exists() + def test_delete_upload_path_traversal(self, client): with tempfile.TemporaryDirectory() as tmp: uploads_dir = Path(tmp) diff --git a/backend/tests/test_uploads_manager.py b/backend/tests/test_uploads_manager.py index b6023b337..7e78d6e80 100644 --- a/backend/tests/test_uploads_manager.py +++ b/backend/tests/test_uploads_manager.py @@ -566,6 +566,17 @@ class TestUploadPublication: assert result.read_bytes() == b"new contents" assert os.stat(dest).st_nlink == 1 + @pytest.mark.parametrize("filename", ["a." + "x" * 253, "é." + "x" * 251]) + def test_max_length_filename_with_long_suffix_still_gets_collision_candidate(self, tmp_path, filename): + first = publish_upload_bytes(tmp_path, filename, b"first") + second = publish_upload_bytes(tmp_path, filename, b"second") + + assert first.name == filename + assert second.name.endswith("_1") + assert len(second.name.encode("utf-8")) <= 255 + assert first.read_bytes() == b"first" + assert second.read_bytes() == b"second" + def test_existing_symlink_is_preserved_and_skipped(self, tmp_path): outside = tmp_path / "outside.txt" outside.write_bytes(b"protected") @@ -835,6 +846,18 @@ class TestDeleteFileSafe: with pytest.raises(FileNotFoundError): delete_file_safe(tmp_path, "nope.txt") + @pytest.mark.skipif(os.name == "nt", reason="POSIX legacy filenames are not representable on Windows") + @pytest.mark.parametrize("filename", ["CON", "report?.pdf", "trailing "]) + def test_delete_accepts_listed_legacy_posix_filename(self, tmp_path, filename): + legacy = tmp_path / filename + legacy.write_bytes(b"legacy") + + assert filename in {entry["filename"] for entry in list_files_in_dir(tmp_path)["files"]} + result = delete_file_safe(tmp_path, filename) + + assert result == {"success": True, "message": f"Deleted {filename}"} + assert not legacy.exists() + def test_delete_from_nonexistent_directory_raises_file_not_found(self, tmp_path): with pytest.raises(FileNotFoundError, match="ghost.txt"): delete_file_safe(tmp_path / "missing-uploads", "ghost.txt") diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 6279db838..0f4ae71f5 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -1276,6 +1276,27 @@ def test_delete_uploaded_file_removes_owned_conversion_and_preserves_user_markdo assert user_markdown.read_text(encoding="utf-8") == "user" +@pytest.mark.skipif(os.name == "nt", reason="POSIX legacy filenames are not representable on Windows") +def test_delete_uploaded_file_accepts_listed_legacy_posix_filename(tmp_path): + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + legacy = thread_uploads_dir / "CON" + legacy.write_bytes(b"legacy") + + with patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir): + result = asyncio.run( + call_unwrapped( + uploads.delete_uploaded_file, + "thread-legacy", + legacy.name, + request=MagicMock(), + ) + ) + + assert result == {"success": True, "message": "Deleted CON"} + assert not legacy.exists() + + def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors(): class BrokenConfig: def __getattribute__(self, name):