From 616c6e2e3673e92b2ebef97ad02fd9739c78aaec Mon Sep 17 00:00:00 2001 From: Linyao Du Date: Fri, 2 Oct 2026 22:50:30 +0800 Subject: [PATCH] fix(uploads): reject Windows staging aliases before batch ingestion (#6149) * fix(uploads): reject Windows staging aliases before batch ingestion * fix(uploads): cover shelf names and case-insensitive staging aliases * test(uploads): isolate legacy alias filesystem cases --- README.md | 8 +- backend/app/gateway/routers/uploads.py | 6 +- backend/docs/FILE_UPLOAD.md | 4 +- .../harness/deerflow/projects/documents.py | 7 +- .../harness/deerflow/uploads/__init__.py | 2 + .../harness/deerflow/uploads/manager.py | 12 +- .../tests/blocking_io/test_uploads_router.py | 4 +- backend/tests/test_client.py | 5 +- .../tests/test_project_documents_promotion.py | 35 ++++-- .../test_upload_windows_staging_aliases.py | 108 ++++++++++++++++++ backend/tests/test_uploads_manager.py | 2 +- 11 files changed, 169 insertions(+), 24 deletions(-) create mode 100644 backend/tests/test_upload_windows_staging_aliases.py diff --git a/README.md b/README.md index 83e6f14a1b..c1cbd4dbc1 100644 --- a/README.md +++ b/README.md @@ -1898,13 +1898,17 @@ 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 restriction includes Windows aliases with trailing dots or spaces and +case variants, such as `.upload-notes.part.`, `.upload-notes.part `, and +`.UPLOAD-NOTES.PART`, on every host. 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 +batch before copying any files. New project document names follow the same +restriction, whether defaulted from the source or explicitly supplied on upload +or promotion to the shelf; 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. diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index ee26953e82..e1badb90b5 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -30,7 +30,7 @@ from deerflow.uploads.manager import ( enrich_file_listing, ensure_uploads_dir, get_uploads_dir, - is_upload_staging_file, + is_reserved_upload_filename, list_files_in_dir, normalize_filename, upload_artifact_url, @@ -411,11 +411,11 @@ 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 + # Check reserved staging basenames and Win32 aliases using either 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): + if file.filename and is_reserved_upload_filename(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.", diff --git a/backend/docs/FILE_UPLOAD.md b/backend/docs/FILE_UPLOAD.md index 31241c1d6b..942635afac 100644 --- a/backend/docs/FILE_UPLOAD.md +++ b/backend/docs/FILE_UPLOAD.md @@ -26,7 +26,9 @@ POST /api/threads/{thread_id}/uploads 文件名匹配 `.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`。请先下载、改名，再上传到聊天。本修复不会迁移或恢复旧聊天目录中已经匹配该临时文件规则的文件。 +检查也会拒绝 Windows 去掉末尾点号或空格、忽略大小写后会变成保留名称的别名，例如 `.upload-notes.part.`、`.upload-notes.part `、`.UPLOAD-NOTES.PART` 和 `.Upload-NoTeS.Part .`。这项检查在所有主机上生效，HTTP 上传会在整批文件写入前返回 `400`，提示改名。 + +嵌入式 `DeerFlowClient.upload_files` 同样在复制前检查整批文件名，保留名称会抛出 `ValueError`。项目资料库上传和从聊天加入资料库时的重命名也遵循这项限制，包括沿用来源文件名和手动指定新名称两种情况；旧资料库中使用保留名称的文件仍可下载，但直接附加到聊天会返回 `400`。请先下载、改名，再上传到聊天。本修复不会迁移或恢复旧聊天目录中已经匹配该临时文件规则的文件。 **响应：** ```json diff --git a/backend/packages/harness/deerflow/projects/documents.py b/backend/packages/harness/deerflow/projects/documents.py index 4ec01a3bd2..1df3e5a194 100644 --- a/backend/packages/harness/deerflow/projects/documents.py +++ b/backend/packages/harness/deerflow/projects/documents.py @@ -34,7 +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.uploads.manager import is_reserved_upload_filename 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 @@ -57,7 +57,8 @@ def validate_shelf_filename(name: str) -> str: The display filename occupies its own path component (no hash prefix or suffix), so it must be a bare, non-empty filename within the 255 UTF-8 byte filesystem limit — path separators are rejected outright rather than - stripped (§6.5). + stripped (§6.5). New display names share upload staging-name validation, + including Win32 aliases; existing shelf rows are not revalidated on reads. """ candidate = name.strip() if name else "" if not candidate: @@ -68,7 +69,7 @@ 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): + if is_reserved_upload_filename(candidate): raise ValueError(f"Filename uses reserved upload staging pattern: {name!r}") return candidate diff --git a/backend/packages/harness/deerflow/uploads/__init__.py b/backend/packages/harness/deerflow/uploads/__init__.py index f0747f9c6e..26159d3238 100644 --- a/backend/packages/harness/deerflow/uploads/__init__.py +++ b/backend/packages/harness/deerflow/uploads/__init__.py @@ -8,6 +8,7 @@ from .manager import ( enrich_file_listing, ensure_uploads_dir, get_uploads_dir, + is_reserved_upload_filename, is_upload_staging_file, list_files_in_dir, normalize_filename, @@ -26,6 +27,7 @@ __all__ = [ "UPLOAD_STAGING_SUFFIX", "claim_unique_filename", "cleanup_stale_upload_staging_files", + "is_reserved_upload_filename", "is_upload_staging_file", "validate_path_traversal", "list_files_in_dir", diff --git a/backend/packages/harness/deerflow/uploads/manager.py b/backend/packages/harness/deerflow/uploads/manager.py index 4cb7137cb2..69c8f5f6b7 100644 --- a/backend/packages/harness/deerflow/uploads/manager.py +++ b/backend/packages/harness/deerflow/uploads/manager.py @@ -74,7 +74,7 @@ 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): + if is_reserved_upload_filename(safe): raise ValueError(f"Filename uses reserved upload staging pattern: {filename!r}") reason = windows_incompatible_segment(safe) if reason: @@ -135,6 +135,16 @@ def is_upload_staging_file(filename: str) -> bool: return filename.startswith(UPLOAD_STAGING_PREFIX) and filename.endswith(UPLOAD_STAGING_SUFFIX) +def is_reserved_upload_filename(filename: str) -> bool: + """Check a new basename against the staging namespace, including Win32 aliases. + + Win32 trims trailing dots and spaces and normally ignores case when opening + a path. Reject those aliases on every host, without changing the name or + the on-disk staging predicate used by listings and cleanup of existing files. + """ + return is_upload_staging_file(filename.rstrip(" .").lower()) + + def validate_path_traversal(path: Path, base: Path) -> None: """Verify that *path* is inside *base*. diff --git a/backend/tests/blocking_io/test_uploads_router.py b/backend/tests/blocking_io/test_uploads_router.py index ee35629be6..7cf45255b1 100644 --- a/backend/tests/blocking_io/test_uploads_router.py +++ b/backend/tests/blocking_io/test_uploads_router.py @@ -92,7 +92,9 @@ 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"]) +@pytest.mark.parametrize( + "filename", [".upload-notes.part", r"folder\.upload-notes.part", r"C:\users\.upload-notes.part", ".upload-notes.part.", ".upload-notes.part ", r"folder\.upload-notes.part. ", ".UPLOAD-NOTES.PART", r"folder\.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)) diff --git a/backend/tests/test_client.py b/backend/tests/test_client.py index 33cb16186c..c150a14a4e 100644 --- a/backend/tests/test_client.py +++ b/backend/tests/test_client.py @@ -2849,10 +2849,11 @@ 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): + @pytest.mark.parametrize("filename", [".upload-notes.part", ".UPLOAD-NOTES.PART", ".Upload-NoTeS.Part"]) + def test_upload_files_rejects_reserved_name_before_copying_batch(self, client, tmp_path, filename): normal = tmp_path / "normal.txt" normal.write_bytes(b"normal document") - reserved = tmp_path / ".upload-notes.part" + reserved = tmp_path / filename reserved.write_bytes(b"reserved document") uploads_dir = tmp_path / "uploads" uploads_dir.mkdir() diff --git a/backend/tests/test_project_documents_promotion.py b/backend/tests/test_project_documents_promotion.py index 0609ee6a41..421f66aabf 100644 --- a/backend/tests/test_project_documents_promotion.py +++ b/backend/tests/test_project_documents_promotion.py @@ -171,25 +171,35 @@ 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): + @pytest.mark.parametrize("filename", [".upload-notes.part", ".upload-notes.part.", ".upload-notes.part.. ", ".UPLOAD-NOTES.PART", ".Upload-NoTeS.Part ."]) + def test_reserved_staging_name_rejected_on_shelf_upload(self, tmp_path, explicit_name, filename): 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) + kwargs = {"data": {"name": filename}} if explicit_name else {} + with patch.object(project_documents, "stage_document_bytes", new_callable=AsyncMock, wraps=project_documents.stage_document_bytes) as stage: + response = client.post(f"/api/projects/{pid}/documents", files={"file": ("notes.txt" if explicit_name else filename, b"user document")}, **kwargs) assert response.status_code == 400 assert "reserved upload staging" in response.json()["detail"] + stage.assert_not_awaited() assert client.get(f"/api/projects/{pid}/documents").json()["total"] == 0 - def test_reserved_staging_shelf_rename_rejected_before_promotion(self, tmp_path): + @pytest.mark.parametrize("filename", [".upload-notes.part", ".upload-notes.part.", ".upload-notes.part.. ", ".UPLOAD-NOTES.PART", ".Upload-NoTeS.Part ."]) + def test_reserved_staging_shelf_rename_rejected_before_promotion(self, tmp_path, filename): 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") + with ( + patch.object(project_documents, "_resolve_thread_source", wraps=project_documents._resolve_thread_source) as resolve_source, + patch.object(project_documents, "stage_document_bytes", new_callable=AsyncMock, wraps=project_documents.stage_document_bytes) as stage, + ): + response = _promote(client, pid, thread_id="thread-1", kind="output", name="notes.txt", shelf_name=filename) assert response.status_code == 400 assert "reserved upload staging" in response.json()["detail"] + resolve_source.assert_not_called() + stage.assert_not_awaited() assert source.read_bytes() == b"user document" assert client.get(f"/api/projects/{pid}/documents").json()["total"] == 0 @@ -345,20 +355,25 @@ class TestFromThread: class TestAttach: - def test_legacy_reserved_name_reports_client_error_and_preserves_shelf(self, tmp_path): + @pytest.mark.parametrize("filename", [".upload-notes.part", ".upload-notes.part.", ".UPLOAD-NOTES.PART", ".Upload-NoTeS.Part ."]) + def test_legacy_reserved_name_reports_client_error_and_preserves_shelf(self, tmp_path, filename): 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"] + with patch.object(project_documents, "validate_shelf_filename", return_value=filename): + doc = client.post(f"/api/projects/{pid}/documents", files={"file": ("notes.txt", b"legacy document")}, data={"name": filename}).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" + assert not (_thread_uploads("thread-1") / filename).exists() + content = client.get(f"/api/projects/{pid}/documents/{doc['id']}/content?download=true") + assert content.status_code == 200 + assert content.content == b"legacy document" + listed = client.get(f"/api/projects/{pid}/documents").json()["documents"] + assert listed[0]["name"] == filename def test_attach_mounted_provider_copies_without_sync(self, tmp_path): app = _build_app(tmp_path) diff --git a/backend/tests/test_upload_windows_staging_aliases.py b/backend/tests/test_upload_windows_staging_aliases.py new file mode 100644 index 0000000000..71c6dae344 --- /dev/null +++ b/backend/tests/test_upload_windows_staging_aliases.py @@ -0,0 +1,108 @@ +"""Reserved upload names must reject Win32 aliases before any batch side effects.""" + +import asyncio +import os +from io import BytesIO +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from _router_auth_helpers import call_unwrapped, make_authed_test_app +from fastapi import HTTPException, UploadFile +from fastapi.testclient import TestClient + +from app.gateway.deps import get_config +from app.gateway.routers import uploads +from deerflow.uploads.manager import cleanup_stale_upload_staging_files, list_files_in_dir, normalize_filename, write_upload_file_no_symlink + +ALIASES = [".upload-notes.part.", ".upload-notes.part ", ".upload-notes.part.. ", ".upload-notes.part .", ".UPLOAD-NOTES.PART", ".Upload-NoTeS.Part", ".UPLOAD-NOTES.PART.", ".Upload-NoTeS.Part ."] + + +@pytest.mark.parametrize("filename", ALIASES) +def test_normalize_filename_rejects_windows_staging_alias(filename): + with pytest.raises(ValueError, match="reserved upload staging"): + normalize_filename(filename) + + +@pytest.mark.parametrize("filename", ALIASES) +@pytest.mark.parametrize("prefix", ["", "folder/", "folder\\", "C:\\users\\"]) +@pytest.mark.parametrize("batch", ["single", "reserved_first", "reserved_last"]) +def test_http_rejects_windows_staging_alias_before_starting_batch(tmp_path, filename, prefix, batch): + uploads_dir = tmp_path / "uploads" + uploads_dir.mkdir() + existing = 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", (prefix + 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] + provider = MagicMock() + provider.uses_thread_data_mounts = True + + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=uploads_dir) as ensure_dir, + patch.object(uploads, "get_sandbox_provider", return_value=provider) as get_provider, + TestClient(app) as client, + ): + response = client.post("/api/threads/thread-alias/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 uploads_dir.iterdir()] == ["existing.txt"] + assert existing.read_bytes() == b"existing document" + + +@pytest.mark.parametrize("filename", ALIASES) +@pytest.mark.parametrize("reserved_first", [True, False]) +def test_adapter_rejects_windows_staging_alias_before_reading_bytes(tmp_path, filename, reserved_first): + normal = UploadFile(filename="normal.txt", file=BytesIO(b"normal document")) + reserved = UploadFile(filename=filename, file=BytesIO(b"reserved document")) + files = [reserved, normal] if reserved_first else [normal, reserved] + provider = MagicMock() + provider.uses_thread_data_mounts = True + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=tmp_path) as ensure_dir, + patch.object(uploads, "get_sandbox_provider", return_value=provider) as get_provider, + pytest.raises(HTTPException) as exc_info, + ): + asyncio.run(call_unwrapped(uploads.upload_files, "thread-alias", 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.file.tell() == 0 for file in files) + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.skipif(os.name != "nt", reason="Requires native Win32 filename aliasing") +@pytest.mark.parametrize("filename", ALIASES) +def test_native_windows_alias_rejection_preserves_existing_file(tmp_path, filename): + canonical = tmp_path / ".upload-notes.part" + canonical.write_bytes(b"existing staging bytes") + alias = tmp_path / filename + # These reads use the real Windows filesystem, with no Path mocks. + assert alias.read_bytes() == b"existing staging bytes" + with pytest.raises(ValueError, match="reserved upload staging"): + write_upload_file_no_symlink(tmp_path, filename, b"new user document") + assert canonical.read_bytes() == b"existing staging bytes" + assert [path.name for path in tmp_path.iterdir()] == [".upload-notes.part"] + + +@pytest.mark.skipif(os.name == "nt", reason="Windows cannot store literal trailing dots or spaces") +@pytest.mark.parametrize("filename", ALIASES) +def test_posix_legacy_alias_files_remain_visible_and_are_not_swept(tmp_path, filename): + # Each alias gets its own directory so case variants cannot collide on macOS. + uploads_dir = tmp_path / "threads" / "thread-alias" / "user-data" / "uploads" + uploads_dir.mkdir(parents=True) + legacy_file = uploads_dir / filename + legacy_file.write_bytes(b"legacy user document") + assert cleanup_stale_upload_staging_files(tmp_path) == 0 + assert [item["filename"] for item in list_files_in_dir(uploads_dir)["files"]] == [filename] + assert legacy_file.read_bytes() == b"legacy user document" diff --git a/backend/tests/test_uploads_manager.py b/backend/tests/test_uploads_manager.py index 9b416ed00d..1febee1ba4 100644 --- a/backend/tests/test_uploads_manager.py +++ b/backend/tests/test_uploads_manager.py @@ -83,7 +83,7 @@ class TestNormalizeFilename: with pytest.raises(ValueError, match="reserved upload staging"): normalize_filename(filename) - @pytest.mark.parametrize("filename", [".upload-notes.txt", "notes.part", ".env"]) + @pytest.mark.parametrize("filename", [".upload-notes.txt", "notes.part", ".env", ".UPLOAD-notes.txt", "notes.PART"]) def test_keeps_non_staging_names(self, filename): assert normalize_filename(filename) == filename