From f74a290c427a32f65f2dce50c5fc06f0df54eba3 Mon Sep 17 00:00:00 2001 From: SuperSgdk Date: Thu, 1 Oct 2026 10:22:38 +0800 Subject: [PATCH] fix(uploads): reject reserved staging filenames before batch ingestion (#6122) * fix(uploads): reject filenames reserved for temporary staging Guard thread and shelf upload names, validate SDK batches before copying, and preserve legacy shelf downloads. Add regression coverage and document the reserved staging namespace. * fix(uploads): report reserved names before HTTP batch ingestion Preflight reserved staging basenames before thread storage or sandbox setup. Return HTTP 400 with a rename hint; add HTTP and strict blocking-I/O regressions and update upload documentation. * fix(uploads): reject Windows-style staging basenames on POSIX Normalize backslashes before the HTTP reserved-name preflight. Add POSIX basename regressions for single and mixed batches and strict blocking-I/O coverage. Clarify the preflight scope and upload documentation. Follow-up to the review of PR #6122; AI-assisted with OpenAI Codex. --------- Co-authored-by: Willem Jiang --- README.md | 13 +++ .../app/gateway/routers/project_documents.py | 4 +- backend/app/gateway/routers/uploads.py | 11 +++ backend/docs/FILE_UPLOAD.md | 4 + backend/packages/harness/deerflow/client.py | 6 +- .../harness/deerflow/projects/documents.py | 3 + .../harness/deerflow/uploads/manager.py | 4 +- .../tests/blocking_io/test_uploads_router.py | 32 ++++++- backend/tests/test_client.py | 16 ++++ .../tests/test_project_documents_promotion.py | 38 +++++++++ backend/tests/test_uploads_manager.py | 14 ++++ backend/tests/test_uploads_router.py | 83 ++++++++++++++++++- 12 files changed, 220 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 8328abbbca..fc24188edb 100644 --- a/README.md +++ b/README.md @@ -1883,6 +1883,19 @@ also fails, for example because of a Windows sharing violation. Once the file is published, a temporary-file cleanup failure is logged without failing the upload; hidden staging files are left for the startup sweep. +Uploaded filenames matching `.upload-*.part` are rejected because that pattern is +reserved for temporary staging files. Rename such a file before uploading it. +The HTTP check recognizes both `/` and `\` as path separators, including on +Linux, when extracting the basename. The endpoint returns `400` with a rename +hint before publishing any file in a batch containing a reserved name, so the +chat reports the upload error instead +of continuing without the attachment. The SDK validates filenames for the whole +batch before copying any files. Project document names follow the same +restriction; an older shelf document with a reserved name +remains downloadable but cannot be attached directly. Download it, rename it, +and upload it to the thread. This change does not recover or migrate older +thread uploads that already match the staging pattern. + This is the difference between a chatbot with tool access and an agent with an actual execution environment. ``` diff --git a/backend/app/gateway/routers/project_documents.py b/backend/app/gateway/routers/project_documents.py index b080edc8a3..985c942bac 100644 --- a/backend/app/gateway/routers/project_documents.py +++ b/backend/app/gateway/routers/project_documents.py @@ -380,9 +380,9 @@ async def attach_project_document_to_thread( await service.open() file_info = await service.ingest_chunks(read_file_chunks(staged_path, chunk_size=UPLOAD_CHUNK_SIZE), display_name=row["name"]) except UnsafeFilenameError as exc: - # Shelf names always normalize, so this cannot fire — fail closed. + # Legacy shelf rows may predate current upload-name validation. await service.cleanup_written() - raise HTTPException(status_code=500, detail=f"Failed to upload {row['name']}: {exc}") + raise HTTPException(status_code=400, detail=f"Cannot attach {row['name']}: {exc}. Download it, rename it, and upload it directly to the thread.") except UnsafeUploadDestinationError as exc: await service.cleanup_written() raise HTTPException(status_code=500, detail=f"Failed to upload {row['name']}: {exc}") diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index 449f8cd630..ee26953e82 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -30,6 +30,7 @@ from deerflow.uploads.manager import ( enrich_file_listing, ensure_uploads_dir, get_uploads_dir, + is_upload_staging_file, list_files_in_dir, normalize_filename, upload_artifact_url, @@ -410,6 +411,16 @@ async def upload_files( if len(files) > limits.max_files: raise HTTPException(status_code=413, detail=f"Too many files: maximum is {limits.max_files}") + # Check reserved staging basenames using either path separator before + # opening storage or a sandbox, so a later reserved name cannot partially + # upload the batch. Other unsafe filenames keep ingestion's skip behavior. + for file in files: + if file.filename and is_upload_staging_file(Path(file.filename.replace("\\", "/")).name): + raise HTTPException( + status_code=400, + detail=f"Filename uses reserved upload staging pattern: {file.filename!r}. Rename the file and upload it again.", + ) + # Setup runs INSIDE the cleanup scope: open() can acquire the sandbox # request lease and then raise (e.g. the acquired lease yields no # sandbox), and the finally's aclose() is what releases that partially diff --git a/backend/docs/FILE_UPLOAD.md b/backend/docs/FILE_UPLOAD.md index 6c55bd97c3..6ca03cf925 100644 --- a/backend/docs/FILE_UPLOAD.md +++ b/backend/docs/FILE_UPLOAD.md @@ -24,6 +24,10 @@ POST /api/threads/{thread_id}/uploads 网关会在应用层限制上传规模，默认最多 10 个文件、单文件 50 MiB、单次请求总计 100 MiB。可通过 `config.yaml` 的 `uploads.max_files`、`uploads.max_file_size`、`uploads.max_total_size` 调整；前端会读取同一组限制并在选择文件时提示，超过限制时后端返回 `413 Payload Too Large`。 +文件名匹配 `.upload-*.part`（例如 `.upload-notes.part`）时，网关会返回 `400 Bad Request`，提示改名后重新上传。这是系统保留的临时文件命名规则；文件名按去掉目录后的 basename 判断，HTTP 检查在 Linux 上也同时识别 `/` 和 `\` 两种路径分隔符，例如 `folder\.upload-notes.part`。网关会先检查整批文件，再开始写入聊天的上传目录或获取沙箱，因此保留名称排在批次末尾也不会留下部分上传。聊天界面收到该错误后会提示用户，并停止本次消息发送。`.upload-notes.txt`、`notes.part` 和 `.env` 仍可上传。 + +嵌入式 `DeerFlowClient.upload_files` 同样在复制前检查整批文件名，保留名称会抛出 `ValueError`。项目资料库上传和重命名也遵循这项限制；旧资料库中使用保留名称的文件仍可下载，但直接附加到聊天会返回 `400`。请先下载、改名，再上传到聊天。本修复不会迁移或恢复旧聊天目录中已经匹配该临时文件规则的文件。 + **响应：** ```json { diff --git a/backend/packages/harness/deerflow/client.py b/backend/packages/harness/deerflow/client.py index 0203dc965d..a8c3181a08 100644 --- a/backend/packages/harness/deerflow/client.py +++ b/backend/packages/harness/deerflow/client.py @@ -79,6 +79,7 @@ from deerflow.uploads.manager import ( ensure_uploads_dir, get_uploads_dir, list_files_in_dir, + normalize_filename, upload_artifact_url, upload_virtual_path, ) @@ -1717,7 +1718,8 @@ class DeerFlowClient: Raises: FileNotFoundError: If any file does not exist. - ValueError: If any supplied path exists but is not a regular file. + ValueError: If any supplied path exists but is not a regular file, + or its filename is unsafe or reserved for upload staging. """ validate_thread_id(thread_id) from deerflow.utils.file_conversion import CONVERTIBLE_EXTENSIONS, convert_file_to_markdown @@ -1732,7 +1734,7 @@ class DeerFlowClient: raise FileNotFoundError(f"File not found: {f}") if not p.is_file(): raise ValueError(f"Path is not a file: {f}") - dest_name = claim_unique_filename(p.name, seen_names) + dest_name = claim_unique_filename(normalize_filename(p.name), seen_names) resolved_files.append((p, dest_name)) if not has_convertible_file and p.suffix.lower() in CONVERTIBLE_EXTENSIONS: has_convertible_file = True diff --git a/backend/packages/harness/deerflow/projects/documents.py b/backend/packages/harness/deerflow/projects/documents.py index 621cc332ac..4ec01a3bd2 100644 --- a/backend/packages/harness/deerflow/projects/documents.py +++ b/backend/packages/harness/deerflow/projects/documents.py @@ -34,6 +34,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any from deerflow.config.paths import Paths +from deerflow.uploads.manager import is_upload_staging_file from deerflow.utils.file_conversion import CONVERTIBLE_EXTENSIONS, convert_file_to_markdown from deerflow.utils.file_io import run_file_io from deerflow.utils.text_detection import is_text_file_by_content @@ -67,6 +68,8 @@ def validate_shelf_filename(name: str) -> str: raise ValueError(f"Filename is unsafe: {name!r}") if len(candidate.encode("utf-8")) > _MAX_FILENAME_BYTES: raise ValueError(f"Filename exceeds {_MAX_FILENAME_BYTES} UTF-8 bytes") + if is_upload_staging_file(candidate): + raise ValueError(f"Filename uses reserved upload staging pattern: {name!r}") return candidate diff --git a/backend/packages/harness/deerflow/uploads/manager.py b/backend/packages/harness/deerflow/uploads/manager.py index a65a269988..4cb7137cb2 100644 --- a/backend/packages/harness/deerflow/uploads/manager.py +++ b/backend/packages/harness/deerflow/uploads/manager.py @@ -59,7 +59,7 @@ def normalize_filename(filename: str) -> str: Safe filename (basename only). Raises: - ValueError: If filename is empty or resolves to a traversal pattern. + ValueError: If filename is empty, unsafe, too long, or uses the reserved staging pattern. """ if not filename: raise ValueError("Filename is empty") @@ -74,6 +74,8 @@ def normalize_filename(filename: str) -> str: raise ValueError(f"Filename contains backslash: {filename!r}") if len(safe.encode("utf-8")) > _MAX_FILENAME_BYTES: raise ValueError(f"Filename too long: {len(safe)} chars") + if is_upload_staging_file(safe): + raise ValueError(f"Filename uses reserved upload staging pattern: {filename!r}") reason = windows_incompatible_segment(safe) if reason: raise ValueError(f"Filename is not portable to Windows: {filename!r} ({reason})") diff --git a/backend/tests/blocking_io/test_uploads_router.py b/backend/tests/blocking_io/test_uploads_router.py index 39dd913206..ee35629be6 100644 --- a/backend/tests/blocking_io/test_uploads_router.py +++ b/backend/tests/blocking_io/test_uploads_router.py @@ -4,12 +4,12 @@ from __future__ import annotations import asyncio from io import BytesIO -from pathlib import Path +from pathlib import Path, PurePosixPath from types import SimpleNamespace import pytest from _router_auth_helpers import call_unwrapped -from fastapi import UploadFile +from fastapi import HTTPException, UploadFile from app.gateway.routers import uploads from deerflow.runtime.user_context import get_effective_user_id @@ -92,6 +92,34 @@ async def test_upload_endpoint_mounted_provider_does_not_block_event_loop(tmp_pa assert await asyncio.to_thread(target.read_bytes) == b"hello uploads" +@pytest.mark.parametrize("filename", [".upload-notes.part", r"folder\.upload-notes.part", r"C:\users\.upload-notes.part"]) +async def test_reserved_name_rejects_batch_without_blocking_or_writing(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, filename: str) -> None: + # Emulate Linux parsing of the multipart name while keeping disk paths native. + monkeypatch.setattr(uploads, "Path", lambda value: PurePosixPath(value) if value == filename else Path(value)) + _reset_paths(tmp_path, monkeypatch) + uploads_dir = await _thread_uploads_dir("t-reserved") + existing = uploads_dir / "existing.txt" + await asyncio.to_thread(existing.write_bytes, b"existing document") + monkeypatch.setattr(uploads, "get_sandbox_provider", lambda: _MountedProvider()) + + with pytest.raises(HTTPException) as exc_info: + await call_unwrapped( + uploads.upload_files, + "t-reserved", + request=None, + files=[ + UploadFile(filename="normal.txt", file=BytesIO(b"normal document")), + UploadFile(filename=filename, file=BytesIO(b"reserved document")), + ], + config=SimpleNamespace(), + ) + + assert exc_info.value.status_code == 400 + assert "reserved upload staging" in exc_info.value.detail + assert await asyncio.to_thread(lambda: sorted(path.name for path in uploads_dir.iterdir())) == ["existing.txt"] + assert await asyncio.to_thread(existing.read_bytes) == b"existing document" + + async def test_upload_endpoint_remote_provider_syncs_without_blocking_event_loop(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: _reset_paths(tmp_path, monkeypatch) provider = _RemoteProvider() diff --git a/backend/tests/test_client.py b/backend/tests/test_client.py index daccabdbf2..11cc112106 100644 --- a/backend/tests/test_client.py +++ b/backend/tests/test_client.py @@ -2849,6 +2849,22 @@ class TestUploads: with pytest.raises(FileNotFoundError): client.upload_files("thread-1", ["/nonexistent/file.txt"]) + def test_upload_files_rejects_reserved_name_before_copying_batch(self, client, tmp_path): + normal = tmp_path / "normal.txt" + normal.write_bytes(b"normal document") + reserved = tmp_path / ".upload-notes.part" + reserved.write_bytes(b"reserved document") + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + + with patch("deerflow.client.ensure_uploads_dir", return_value=uploads_dir): + with pytest.raises(ValueError, match="reserved upload staging"): + client.upload_files("thread-1", [normal, reserved]) + + assert list(uploads_dir.iterdir()) == [] + assert normal.read_bytes() == b"normal document" + assert reserved.read_bytes() == b"reserved document" + def test_upload_files_rejects_directory_path(self, client): with tempfile.TemporaryDirectory() as tmp: with pytest.raises(ValueError, match="Path is not a file"): diff --git a/backend/tests/test_project_documents_promotion.py b/backend/tests/test_project_documents_promotion.py index 0384601ead..0609ee6a41 100644 --- a/backend/tests/test_project_documents_promotion.py +++ b/backend/tests/test_project_documents_promotion.py @@ -170,6 +170,29 @@ def _remote_provider(sandbox: MagicMock | None = None) -> tuple[MagicMock, Magic class TestFromThread: + @pytest.mark.parametrize("explicit_name", [False, True]) + def test_reserved_staging_name_rejected_on_shelf_upload(self, tmp_path, explicit_name): + app = _build_app(tmp_path) + with TestClient(app) as client: + pid = _create_project(client)["id"] + kwargs = {"data": {"name": ".upload-notes.part"}} if explicit_name else {} + response = client.post(f"/api/projects/{pid}/documents", files={"file": ("notes.txt" if explicit_name else ".upload-notes.part", b"user document")}, **kwargs) + assert response.status_code == 400 + assert "reserved upload staging" in response.json()["detail"] + assert client.get(f"/api/projects/{pid}/documents").json()["total"] == 0 + + def test_reserved_staging_shelf_rename_rejected_before_promotion(self, tmp_path): + app = _build_app(tmp_path) + with TestClient(app) as client: + pid = _create_project(client)["id"] + _seed_thread(app, "thread-1") + source = _thread_file("thread-1", "output", "notes.txt", b"user document") + response = _promote(client, pid, thread_id="thread-1", kind="output", name="notes.txt", shelf_name=".upload-notes.part") + assert response.status_code == 400 + assert "reserved upload staging" in response.json()["detail"] + assert source.read_bytes() == b"user document" + assert client.get(f"/api/projects/{pid}/documents").json()["total"] == 0 + def test_promote_upload_with_default_name_records_provenance(self, tmp_path): app = _build_app(tmp_path) with TestClient(app) as client: @@ -322,6 +345,21 @@ class TestFromThread: class TestAttach: + def test_legacy_reserved_name_reports_client_error_and_preserves_shelf(self, tmp_path): + app = _build_app(tmp_path) + provider = _mounted_provider() + with TestClient(app) as client, patch.object(uploads, "get_sandbox_provider", return_value=provider): + pid = _create_project(client)["id"] + # Seed a document accepted before reserved-name validation existed. + with patch.object(project_documents, "validate_shelf_filename", return_value=".upload-notes.part"): + doc = client.post(f"/api/projects/{pid}/documents", files={"file": ("notes.txt", b"legacy document")}, data={"name": ".upload-notes.part"}).json()["document"] + _seed_thread(app, "thread-1") + response = _attach(client, pid, doc["id"], "thread-1") + assert response.status_code == 400 + assert "reserved upload staging" in response.json()["detail"] + assert not (_thread_uploads("thread-1") / ".upload-notes.part").exists() + assert client.get(f"/api/projects/{pid}/documents/{doc['id']}/content?download=true").content == b"legacy document" + def test_attach_mounted_provider_copies_without_sync(self, tmp_path): app = _build_app(tmp_path) provider = _mounted_provider() diff --git a/backend/tests/test_uploads_manager.py b/backend/tests/test_uploads_manager.py index 408724d981..9b416ed00d 100644 --- a/backend/tests/test_uploads_manager.py +++ b/backend/tests/test_uploads_manager.py @@ -78,6 +78,20 @@ class TestNormalizeFilename: with pytest.raises(ValueError, match="NUL"): normalize_filename("report\x00.pdf") + @pytest.mark.parametrize("filename", [".upload-notes.part", ".upload-.part", "folder/.upload-notes.part"]) + def test_rejects_reserved_staging_names(self, filename): + with pytest.raises(ValueError, match="reserved upload staging"): + normalize_filename(filename) + + @pytest.mark.parametrize("filename", [".upload-notes.txt", "notes.part", ".env"]) + def test_keeps_non_staging_names(self, filename): + assert normalize_filename(filename) == filename + + def test_reserved_name_rejected_before_writing_upload(self, tmp_path): + with pytest.raises(ValueError, match="reserved upload staging"): + write_upload_file_no_symlink(tmp_path, ".upload-notes.part", b"user document") + assert list(tmp_path.iterdir()) == [] + @pytest.mark.parametrize( "filename", ["CON", "con.txt", "PRN", "AUX", "NUL", "COM1", "COM9", "LPT1", "LPT9", "file.txt.", "file.txt ", "a.", "folder/CON"], diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 4c4f19c2a5..82f59df951 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -4,7 +4,7 @@ import os import stat import threading from io import BytesIO -from pathlib import Path +from pathlib import Path, PurePosixPath from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -1017,6 +1017,87 @@ def test_upload_files_adjusts_read_permissions_for_mounted_non_local_sandbox(tmp assert called_path.name == "notes.txt" +@pytest.mark.parametrize("filename", [".upload-notes.part", ".upload-.part", "folder/.upload-notes.part"]) +@pytest.mark.parametrize("batch", ["single", "reserved_first", "reserved_last"]) +def test_upload_files_rejects_reserved_names_before_starting_batch(tmp_path, filename, batch): + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir() + existing = thread_uploads_dir / "existing.txt" + existing.write_bytes(b"existing document") + app = make_authed_test_app() + app.include_router(uploads.router) + app.dependency_overrides[get_config] = lambda: SimpleNamespace() + + reserved = ("files", (filename, b"reserved document")) + normal = ("files", ("normal.txt", b"normal document")) + files = [reserved] if batch == "single" else [reserved, normal] if batch == "reserved_first" else [normal, reserved] + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir) as ensure_dir, + patch.object(uploads, "get_sandbox_provider") as get_provider, + TestClient(app) as client, + ): + response = client.post("/api/threads/thread-local/uploads", files=files) + + assert response.status_code == 400 + assert "reserved upload staging" in response.json()["detail"] + assert "Rename" in response.json()["detail"] + ensure_dir.assert_not_called() + get_provider.assert_not_called() + assert [path.name for path in thread_uploads_dir.iterdir()] == ["existing.txt"] + assert existing.read_bytes() == b"existing document" + + +@pytest.mark.parametrize("filename", [r"folder\.upload-notes.part", r"C:\users\.upload-notes.part"]) +@pytest.mark.parametrize("batch", ["single", "reserved_first", "reserved_last"]) +def test_upload_files_rejects_windows_reserved_names_with_posix_basename_rules(tmp_path, monkeypatch, filename, batch): + # Emulate Linux basename parsing only for the multipart filename; keep + # native filesystem paths for ingestion and the test fixtures. + monkeypatch.setattr(uploads, "Path", lambda value: PurePosixPath(value) if value == filename else Path(value)) + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir() + existing = thread_uploads_dir / "existing.txt" + existing.write_bytes(b"existing document") + reserved = ChunkedUpload(filename, [b"reserved document"]) + normal = ChunkedUpload("normal.txt", [b"normal document"]) + files = [reserved] if batch == "single" else [reserved, normal] if batch == "reserved_first" else [normal, reserved] + + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir) as ensure_dir, + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()) as get_provider, + pytest.raises(HTTPException) as exc_info, + ): + asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=files, config=SimpleNamespace())) + + assert exc_info.value.status_code == 400 + assert "reserved upload staging" in exc_info.value.detail + assert "Rename" in exc_info.value.detail + ensure_dir.assert_not_called() + get_provider.assert_not_called() + assert all(file.read_calls == [] for file in files) + assert [path.name for path in thread_uploads_dir.iterdir()] == ["existing.txt"] + assert existing.read_bytes() == b"existing document" + + +@pytest.mark.parametrize("filename", [".upload-notes.txt", "notes.part", ".env"]) +def test_upload_files_accepts_names_near_reserved_pattern(tmp_path, filename): + app = make_authed_test_app() + app.include_router(uploads.router) + app.dependency_overrides[get_config] = lambda: SimpleNamespace() + provider = MagicMock() + provider.uses_thread_data_mounts = True + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=tmp_path), + patch.object(uploads, "get_sandbox_provider", return_value=provider), + TestClient(app) as client, + ): + response = client.post("/api/threads/thread-local/uploads", files={"files": (filename, b"user document")}) + + assert response.status_code == 200 + assert response.json()["success"] is True + assert [entry["filename"] for entry in response.json()["files"]] == [filename] + assert (tmp_path / filename).read_bytes() == b"user document" + + def test_upload_files_rejects_dotdot_and_dot_filenames(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True)