mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
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 <willem.jiang@gmail.com>
This commit is contained in:
parent
90d466c917
commit
f74a290c42
13
README.md
13
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.
|
||||
|
||||
```
|
||||
|
||||
@ -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}")
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
{
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
|
||||
@ -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})")
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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"):
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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"],
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user