fix: make gateway uploads collision-safe

This commit is contained in:
hetaoBackend 2026-08-06 10:15:17 +08:00
parent b513921c60
commit 09d3fc04cd
2 changed files with 190 additions and 194 deletions

View File

@ -3,10 +3,7 @@
import logging import logging
import os import os
import stat import stat
import tempfile
from dataclasses import dataclass
from pathlib import Path from pathlib import Path
from typing import BinaryIO
from fastapi import APIRouter, Depends, File, HTTPException, Request, UploadFile from fastapi import APIRouter, Depends, File, HTTPException, Request, UploadFile
from pydantic import BaseModel, Field from pydantic import BaseModel, Field
@ -17,23 +14,24 @@ from deerflow.config.app_config import AppConfig
from deerflow.config.paths import get_paths from deerflow.config.paths import get_paths
from deerflow.runtime.user_context import get_effective_user_id from deerflow.runtime.user_context import get_effective_user_id
from deerflow.sandbox.sandbox_provider import SandboxProvider, get_sandbox_provider from deerflow.sandbox.sandbox_provider import SandboxProvider, get_sandbox_provider
from deerflow.uploads.conversion import convert_uploaded_file_to_markdown
from deerflow.uploads.layout import artifact_url_for_virtual_path, conversion_virtual_path
from deerflow.uploads.manager import ( from deerflow.uploads.manager import (
UPLOAD_STAGING_PREFIX,
UPLOAD_STAGING_SUFFIX,
PathTraversalError, PathTraversalError,
UnsafeUploadPathError, StagedUpload,
claim_unique_filename, abort_staged_upload,
create_upload_staging_file,
delete_file_safe, delete_file_safe,
enrich_file_listing, enrich_file_listing,
ensure_uploads_dir, ensure_uploads_dir,
get_uploads_dir, get_uploads_dir,
list_files_in_dir, list_files_in_dir,
normalize_filename, normalize_filename,
publish_staged_upload,
upload_artifact_url, upload_artifact_url,
upload_virtual_path, upload_virtual_path,
validate_upload_destination,
) )
from deerflow.utils.file_conversion import CONVERTIBLE_EXTENSIONS, convert_file_to_markdown from deerflow.utils.file_conversion import CONVERTIBLE_EXTENSIONS
from deerflow.utils.file_io import run_file_io from deerflow.utils.file_io import run_file_io
from deerflow.utils.thread_id import ThreadId from deerflow.utils.thread_id import ThreadId
@ -47,13 +45,6 @@ DEFAULT_MAX_FILE_SIZE = 50 * 1024 * 1024
DEFAULT_MAX_TOTAL_SIZE = 100 * 1024 * 1024 DEFAULT_MAX_TOTAL_SIZE = 100 * 1024 * 1024
@dataclass(slots=True)
class _UploadTempFile:
file_path: Path
temp_path: Path
handle: BinaryIO
class UploadedFileInfo(BaseModel): class UploadedFileInfo(BaseModel):
"""Uploaded file metadata exposed by upload and list APIs.""" """Uploaded file metadata exposed by upload and list APIs."""
@ -178,52 +169,6 @@ def _cleanup_uploaded_paths(paths: list[os.PathLike[str] | str]) -> None:
logger.warning("Failed to clean up upload path after rejected request: %s", path, exc_info=True) logger.warning("Failed to clean up upload path after rejected request: %s", path, exc_info=True)
def _prepare_upload_destination(uploads_dir: os.PathLike[str] | str, display_filename: str) -> _UploadTempFile:
uploads_dir_path = Path(uploads_dir)
file_path = validate_upload_destination(uploads_dir_path, display_filename)
temp_fd, temp_path_str = tempfile.mkstemp(prefix=UPLOAD_STAGING_PREFIX, suffix=UPLOAD_STAGING_SUFFIX, dir=uploads_dir_path)
temp_path = Path(temp_path_str)
try:
handle = os.fdopen(temp_fd, "wb")
except Exception:
try:
os.close(temp_fd)
except OSError:
pass
try:
os.unlink(temp_path)
except FileNotFoundError:
pass
raise
return _UploadTempFile(file_path=file_path, temp_path=temp_path, handle=handle)
def _write_upload_chunk(upload_temp: _UploadTempFile, chunk: bytes) -> None:
upload_temp.handle.write(chunk)
def _abort_upload_temp(upload_temp: _UploadTempFile) -> None:
try:
upload_temp.handle.close()
finally:
try:
os.unlink(upload_temp.temp_path)
except FileNotFoundError:
pass
def _commit_upload_temp(upload_temp: _UploadTempFile) -> None:
upload_temp.handle.close()
try:
os.replace(upload_temp.temp_path, upload_temp.file_path)
except Exception:
try:
os.unlink(upload_temp.temp_path)
except FileNotFoundError:
pass
raise
def _make_uploaded_paths_sandbox_readable(paths: list[os.PathLike[str] | str]) -> None: def _make_uploaded_paths_sandbox_readable(paths: list[os.PathLike[str] | str]) -> None:
for file_path in paths: for file_path in paths:
_make_file_sandbox_readable(file_path) _make_file_sandbox_readable(file_path)
@ -247,7 +192,7 @@ def _list_uploaded_files_for_thread(thread_id: str, user_id: str) -> dict:
def _delete_uploaded_file_for_thread(thread_id: str, filename: str, user_id: str) -> dict: def _delete_uploaded_file_for_thread(thread_id: str, filename: str, user_id: str) -> dict:
uploads_dir = get_uploads_dir(thread_id, user_id=user_id) uploads_dir = get_uploads_dir(thread_id, user_id=user_id)
return delete_file_safe(uploads_dir, filename, convertible_extensions=CONVERTIBLE_EXTENSIONS) return delete_file_safe(uploads_dir, filename)
async def _write_upload_file_with_limits( async def _write_upload_file_with_limits(
@ -260,9 +205,9 @@ async def _write_upload_file_with_limits(
total_size: int, total_size: int,
) -> tuple[os.PathLike[str] | str, int, int]: ) -> tuple[os.PathLike[str] | str, int, int]:
file_size = 0 file_size = 0
upload_temp: _UploadTempFile | None = None upload_temp: StagedUpload | None = None
try: try:
upload_temp = await run_file_io(_prepare_upload_destination, uploads_dir, display_filename) upload_temp = await run_file_io(create_upload_staging_file, Path(uploads_dir))
while chunk := await file.read(UPLOAD_CHUNK_SIZE): while chunk := await file.read(UPLOAD_CHUNK_SIZE):
file_size += len(chunk) file_size += len(chunk)
total_size += len(chunk) total_size += len(chunk)
@ -270,14 +215,13 @@ async def _write_upload_file_with_limits(
raise HTTPException(status_code=413, detail=f"File too large: {display_filename}") raise HTTPException(status_code=413, detail=f"File too large: {display_filename}")
if total_size > max_total_size: if total_size > max_total_size:
raise HTTPException(status_code=413, detail="Total upload size too large") raise HTTPException(status_code=413, detail="Total upload size too large")
await run_file_io(_write_upload_chunk, upload_temp, chunk) await run_file_io(upload_temp.handle.write, chunk)
await run_file_io(_commit_upload_temp, upload_temp) file_path = await run_file_io(publish_staged_upload, upload_temp, display_filename)
file_path = upload_temp.file_path
upload_temp = None upload_temp = None
except Exception: except Exception:
if upload_temp is not None: if upload_temp is not None:
await run_file_io(_abort_upload_temp, upload_temp) await run_file_io(abort_staged_upload, upload_temp)
raise raise
return file_path, file_size, total_size return file_path, file_size, total_size
@ -324,11 +268,6 @@ async def upload_files(
sandbox_sync_targets = [] sandbox_sync_targets = []
skipped_files = [] skipped_files = []
total_size = 0 total_size = 0
# Track filenames within this request so duplicate form parts do not
# silently truncate each other. Existing uploads keep the historical
# overwrite behavior for a single replacement upload.
seen_filenames: set[str] = set()
sandbox_provider = get_sandbox_provider() sandbox_provider = get_sandbox_provider()
sync_to_sandbox = not _uses_thread_data_mounts(sandbox_provider) sync_to_sandbox = not _uses_thread_data_mounts(sandbox_provider)
sandbox = None sandbox = None
@ -345,7 +284,6 @@ async def upload_files(
try: try:
original_filename = normalize_filename(file.filename) original_filename = normalize_filename(file.filename)
safe_filename = claim_unique_filename(original_filename, seen_filenames)
except ValueError: except ValueError:
logger.warning(f"Skipping file with unsafe filename: {file.filename!r}") logger.warning(f"Skipping file with unsafe filename: {file.filename!r}")
continue continue
@ -354,12 +292,13 @@ async def upload_files(
file_path, file_size, total_size = await _write_upload_file_with_limits( file_path, file_size, total_size = await _write_upload_file_with_limits(
file, file,
uploads_dir=uploads_dir, uploads_dir=uploads_dir,
display_filename=safe_filename, display_filename=original_filename,
max_single_file_size=limits.max_file_size, max_single_file_size=limits.max_file_size,
max_total_size=limits.max_total_size, max_total_size=limits.max_total_size,
total_size=total_size, total_size=total_size,
) )
written_paths.append(file_path) written_paths.append(file_path)
safe_filename = Path(file_path).name
virtual_path = upload_virtual_path(safe_filename) virtual_path = upload_virtual_path(safe_filename)
@ -380,39 +319,28 @@ async def upload_files(
file_ext = file_path.suffix.lower() file_ext = file_path.suffix.lower()
if auto_convert_documents and file_ext in CONVERTIBLE_EXTENSIONS: if auto_convert_documents and file_ext in CONVERTIBLE_EXTENSIONS:
# Reserve the companion .md name in this request's seen set try:
# before writing so conversion cannot silently truncate another md_path = await convert_uploaded_file_to_markdown(file_path)
# uploaded or derived file (same invariant as form-part dedupe). except Exception:
provisional_md_name = Path(safe_filename).with_suffix(".md").name logger.warning("Failed to convert uploaded file: %s", file_path, exc_info=True)
unique_md_name = claim_unique_filename(provisional_md_name, seen_filenames) md_path = None
md_output = file_path.with_name(unique_md_name)
md_path = await convert_file_to_markdown(file_path, output_path=md_output)
if md_path: if md_path:
written_paths.append(md_path) written_paths.append(md_path)
md_virtual_path = upload_virtual_path(md_path.name) md_virtual_path = conversion_virtual_path(safe_filename)
if sync_to_sandbox: if sync_to_sandbox:
sandbox_sync_targets.append((md_path, md_virtual_path)) sandbox_sync_targets.append((md_path, md_virtual_path))
file_info["markdown_file"] = md_path.name file_info["markdown_file"] = md_path.name
file_info["markdown_path"] = str(sandbox_uploads / md_path.name) file_info["markdown_path"] = str(md_path)
file_info["markdown_virtual_path"] = md_virtual_path file_info["markdown_virtual_path"] = md_virtual_path
file_info["markdown_artifact_url"] = upload_artifact_url(thread_id, md_path.name) file_info["markdown_artifact_url"] = artifact_url_for_virtual_path(thread_id, md_virtual_path)
else:
# Conversion failed and wrote nothing, so release the claim;
# holding it would rename a later same-stem upload against
# a name nothing occupies.
seen_filenames.discard(unique_md_name)
uploaded_files.append(file_info) uploaded_files.append(file_info)
except HTTPException as e: except HTTPException as e:
await run_file_io(_cleanup_uploaded_paths, written_paths) await run_file_io(_cleanup_uploaded_paths, written_paths)
raise e raise e
except UnsafeUploadPathError as e:
logger.warning("Skipping upload with unsafe destination %s: %s", file.filename, e)
skipped_files.append(safe_filename)
continue
except Exception as e: except Exception as e:
logger.error(f"Failed to upload {file.filename}: {e}") logger.error(f"Failed to upload {file.filename}: {e}")
await run_file_io(_cleanup_uploaded_paths, written_paths) await run_file_io(_cleanup_uploaded_paths, written_paths)

View File

@ -13,6 +13,7 @@ from fastapi.testclient import TestClient
from app.gateway.deps import get_config from app.gateway.deps import get_config
from app.gateway.routers import uploads from app.gateway.routers import uploads
from deerflow.uploads.layout import conversion_path_for_upload
class ChunkedUpload: class ChunkedUpload:
@ -45,6 +46,20 @@ def _symlink_to_or_skip(link_path: Path, target_path: Path) -> None:
raise raise
def _fake_owned_conversion(content_by_source: dict[str, str] | None = None):
async def fake_convert(file_path: Path) -> Path:
md_path = conversion_path_for_upload(file_path)
md_path.parent.mkdir(parents=True, exist_ok=True)
if content_by_source is not None and file_path.name in content_by_source:
text = content_by_source[file_path.name]
else:
text = f"converted-from:{file_path.name}"
md_path.write_text(text, encoding="utf-8")
return md_path
return fake_convert
def test_upload_files_writes_thread_storage_and_skips_local_sandbox_sync(tmp_path): def test_upload_files_writes_thread_storage_and_skips_local_sandbox_sync(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
@ -132,6 +147,59 @@ def test_upload_files_auto_renames_duplicate_form_filenames(tmp_path):
assert (thread_uploads_dir / "data_1.txt").read_bytes() == b"second" assert (thread_uploads_dir / "data_1.txt").read_bytes() == b"second"
def test_separate_upload_requests_never_replace_same_name(tmp_path):
thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True)
async def upload(payload: bytes):
return await call_unwrapped(
uploads.upload_files,
"thread-local",
request=MagicMock(),
files=[UploadFile(filename="report.txt", file=BytesIO(payload))],
config=SimpleNamespace(),
)
with (
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()),
):
first = asyncio.run(upload(b"first"))
second = asyncio.run(upload(b"second"))
assert [first.files[0].filename, second.files[0].filename] == ["report.txt", "report_1.txt"]
assert (thread_uploads_dir / "report.txt").read_bytes() == b"first"
assert (thread_uploads_dir / "report_1.txt").read_bytes() == b"second"
def test_concurrent_upload_requests_preserve_all_payloads(tmp_path):
thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True)
payloads = [f"payload-{index}".encode() for index in range(8)]
async def upload(payload: bytes):
return await call_unwrapped(
uploads.upload_files,
"thread-local",
request=MagicMock(),
files=[UploadFile(filename="same.bin", file=BytesIO(payload))],
config=SimpleNamespace(),
)
async def run_all():
return await asyncio.gather(*(upload(payload) for payload in payloads))
with (
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()),
):
results = asyncio.run(run_all())
paths = [thread_uploads_dir / result.files[0].filename for result in results]
assert len({path.name for path in paths}) == len(payloads)
assert {path.read_bytes() for path in paths} == set(payloads)
def test_upload_files_skips_acquire_when_thread_data_is_mounted(tmp_path): def test_upload_files_skips_acquire_when_thread_data_is_mounted(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
@ -170,7 +238,7 @@ def test_upload_files_does_not_auto_convert_documents_by_default(tmp_path):
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "get_sandbox_provider", return_value=provider),
patch.object(uploads, "_auto_convert_documents_enabled", return_value=False), patch.object(uploads, "_auto_convert_documents_enabled", return_value=False),
patch.object(uploads, "convert_file_to_markdown", AsyncMock()) as convert_mock, patch.object(uploads, "convert_uploaded_file_to_markdown", AsyncMock()) as convert_mock,
): ):
file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes"))
result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace())) result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace()))
@ -180,7 +248,7 @@ def test_upload_files_does_not_auto_convert_documents_by_default(tmp_path):
assert result.files[0].filename == "report.pdf" assert result.files[0].filename == "report.pdf"
assert result.files[0].markdown_file is None assert result.files[0].markdown_file is None
convert_mock.assert_not_called() convert_mock.assert_not_called()
assert not (thread_uploads_dir / "report.md").exists() assert not conversion_path_for_upload(thread_uploads_dir / "report.pdf").exists()
def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path): def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path):
@ -194,17 +262,16 @@ def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path):
sandbox = MagicMock() sandbox = MagicMock()
provider.get.return_value = sandbox provider.get.return_value = sandbox
async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path:
md_path = output_path if output_path is not None else file_path.with_suffix(".md")
md_path.write_text("converted", encoding="utf-8")
return md_path
with ( with (
patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "get_sandbox_provider", return_value=provider),
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=fake_convert)), patch.object(
uploads,
"convert_uploaded_file_to_markdown",
AsyncMock(side_effect=_fake_owned_conversion({"report.pdf": "converted"})),
),
): ):
file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes"))
result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-aio", request=MagicMock(), files=[file], config=SimpleNamespace())) result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-aio", request=MagicMock(), files=[file], config=SimpleNamespace()))
@ -213,13 +280,16 @@ def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path):
assert len(result.files) == 1 assert len(result.files) == 1
file_info = result.files[0] file_info = result.files[0]
assert file_info.filename == "report.pdf" assert file_info.filename == "report.pdf"
assert file_info.markdown_file == "report.md" assert file_info.markdown_file == "report.pdf.md"
assert file_info.markdown_virtual_path == "/mnt/user-data/.upload-conversions/report.pdf.md"
assert file_info.markdown_artifact_url == ("/api/threads/thread-aio/artifacts/mnt/user-data/.upload-conversions/report.pdf.md")
assert (thread_uploads_dir / "report.pdf").read_bytes() == b"pdf-bytes" assert (thread_uploads_dir / "report.pdf").read_bytes() == b"pdf-bytes"
assert (thread_uploads_dir / "report.md").read_text(encoding="utf-8") == "converted" conversion = conversion_path_for_upload(thread_uploads_dir / "report.pdf")
assert conversion.read_text(encoding="utf-8") == "converted"
sandbox.update_file.assert_any_call("/mnt/user-data/uploads/report.pdf", b"pdf-bytes") sandbox.update_file.assert_any_call("/mnt/user-data/uploads/report.pdf", b"pdf-bytes")
sandbox.update_file.assert_any_call("/mnt/user-data/uploads/report.md", b"converted") sandbox.update_file.assert_any_call("/mnt/user-data/.upload-conversions/report.pdf.md", b"converted")
def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path): def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path):
@ -233,17 +303,16 @@ def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path):
sandbox = MagicMock() sandbox = MagicMock()
provider.get.return_value = sandbox provider.get.return_value = sandbox
async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path:
md_path = output_path if output_path is not None else file_path.with_suffix(".md")
md_path.write_text("converted", encoding="utf-8")
return md_path
with ( with (
patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "get_sandbox_provider", return_value=provider),
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=fake_convert)), patch.object(
uploads,
"convert_uploaded_file_to_markdown",
AsyncMock(side_effect=_fake_owned_conversion({"report.pdf": "converted"})),
),
patch.object(uploads, "_make_file_sandbox_writable") as make_writable, patch.object(uploads, "_make_file_sandbox_writable") as make_writable,
): ):
file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes"))
@ -251,7 +320,7 @@ def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path):
assert result.success is True assert result.success is True
make_writable.assert_any_call(thread_uploads_dir / "report.pdf") make_writable.assert_any_call(thread_uploads_dir / "report.pdf")
make_writable.assert_any_call(thread_uploads_dir / "report.md") make_writable.assert_any_call(conversion_path_for_upload(thread_uploads_dir / "report.pdf"))
def test_upload_files_does_not_adjust_permissions_for_local_sandbox(tmp_path): def test_upload_files_does_not_adjust_permissions_for_local_sandbox(tmp_path):
@ -432,7 +501,7 @@ def test_upload_files_does_not_sync_non_local_sandbox_when_total_size_exceeds_li
sandbox.update_file.assert_not_called() sandbox.update_file.assert_not_called()
def test_upload_files_does_not_sync_non_local_sandbox_when_conversion_fails(tmp_path): def test_upload_files_keeps_and_syncs_primary_when_conversion_fails(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
@ -448,18 +517,31 @@ def test_upload_files_does_not_sync_non_local_sandbox_when_conversion_fails(tmp_
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "get_sandbox_provider", return_value=provider),
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=RuntimeError("conversion failed"))), patch.object(
uploads,
"convert_uploaded_file_to_markdown",
AsyncMock(side_effect=RuntimeError("conversion failed")),
),
): ):
file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes"))
with pytest.raises(HTTPException) as exc_info: result = asyncio.run(
asyncio.run(call_unwrapped(uploads.upload_files, "thread-aio", request=MagicMock(), files=[file], config=SimpleNamespace())) call_unwrapped(
uploads.upload_files,
"thread-aio",
request=MagicMock(),
files=[file],
config=SimpleNamespace(),
)
)
assert exc_info.value.status_code == 500 assert result.success is True
assert result.files[0].filename == "report.pdf"
assert result.files[0].markdown_file is None
provider.acquire.assert_not_called() provider.acquire.assert_not_called()
provider.acquire_async.assert_awaited_once_with("thread-aio", user_id="owner-upload") provider.acquire_async.assert_awaited_once_with("thread-aio", user_id="owner-upload")
provider.get.assert_called_once_with("aio-1") provider.get.assert_called_once_with("aio-1")
sandbox.update_file.assert_not_called() sandbox.update_file.assert_called_once_with("/mnt/user-data/uploads/report.pdf", b"pdf-bytes")
assert not (thread_uploads_dir / "report.pdf").exists() assert (thread_uploads_dir / "report.pdf").read_bytes() == b"pdf-bytes"
def test_make_file_sandbox_writable_adds_write_bits_for_regular_files(tmp_path): def test_make_file_sandbox_writable_adds_write_bits_for_regular_files(tmp_path):
@ -575,7 +657,7 @@ def test_upload_files_rejects_dotdot_and_dot_filenames(tmp_path):
assert [f.name for f in thread_uploads_dir.iterdir()] == ["passwd"] assert [f.name for f in thread_uploads_dir.iterdir()] == ["passwd"]
def test_upload_files_rejects_preexisting_symlink_destination(tmp_path): def test_upload_files_renames_around_preexisting_symlink_destination(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
outside_file = tmp_path / "outside.txt" outside_file = tmp_path / "outside.txt"
@ -593,15 +675,15 @@ def test_upload_files_rejects_preexisting_symlink_destination(tmp_path):
file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload")) file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload"))
result = asyncio.run(uploads.upload_files("thread-local", files=[file])) result = asyncio.run(uploads.upload_files("thread-local", files=[file]))
assert result.success is False assert result.success is True
assert result.files == [] assert result.files[0].filename == "victim_1.txt"
assert result.skipped_files == ["victim.txt"] assert result.files[0].original_filename == "victim.txt"
assert "skipped 1 unsafe file" in result.message
assert outside_file.read_text(encoding="utf-8") == "protected" assert outside_file.read_text(encoding="utf-8") == "protected"
assert (thread_uploads_dir / "victim.txt").is_symlink() assert (thread_uploads_dir / "victim.txt").is_symlink()
assert (thread_uploads_dir / "victim_1.txt").read_bytes() == b"attacker upload"
def test_upload_files_rejects_dangling_symlink_destination(tmp_path): def test_upload_files_renames_around_dangling_symlink_destination(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
missing_target = tmp_path / "missing-target.txt" missing_target = tmp_path / "missing-target.txt"
@ -618,14 +700,14 @@ def test_upload_files_rejects_dangling_symlink_destination(tmp_path):
file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload")) file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload"))
result = asyncio.run(uploads.upload_files("thread-local", files=[file])) result = asyncio.run(uploads.upload_files("thread-local", files=[file]))
assert result.success is False assert result.success is True
assert result.files == [] assert result.files[0].filename == "victim_1.txt"
assert result.skipped_files == ["victim.txt"]
assert not missing_target.exists() assert not missing_target.exists()
assert (thread_uploads_dir / "victim.txt").is_symlink() assert (thread_uploads_dir / "victim.txt").is_symlink()
assert (thread_uploads_dir / "victim_1.txt").read_bytes() == b"attacker upload"
def test_upload_files_rejects_hardlinked_destination_without_truncating(tmp_path): def test_upload_files_renames_around_hardlinked_destination_without_truncating(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
outside_file = tmp_path / "outside.txt" outside_file = tmp_path / "outside.txt"
@ -643,14 +725,14 @@ def test_upload_files_rejects_hardlinked_destination_without_truncating(tmp_path
file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload")) file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload"))
result = asyncio.run(uploads.upload_files("thread-local", files=[file])) result = asyncio.run(uploads.upload_files("thread-local", files=[file]))
assert result.success is False assert result.success is True
assert result.files == [] assert result.files[0].filename == "victim_1.txt"
assert result.skipped_files == ["victim.txt"]
assert outside_file.read_text(encoding="utf-8") == "protected" assert outside_file.read_text(encoding="utf-8") == "protected"
assert (thread_uploads_dir / "victim.txt").read_text(encoding="utf-8") == "protected" assert (thread_uploads_dir / "victim.txt").read_text(encoding="utf-8") == "protected"
assert (thread_uploads_dir / "victim_1.txt").read_bytes() == b"attacker upload"
def test_upload_files_overwrites_existing_regular_file(tmp_path): def test_upload_files_renames_existing_regular_file(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
existing_file = thread_uploads_dir / "notes.txt" existing_file = thread_uploads_dir / "notes.txt"
@ -669,8 +751,9 @@ def test_upload_files_overwrites_existing_regular_file(tmp_path):
result = asyncio.run(uploads.upload_files("thread-local", files=[file])) result = asyncio.run(uploads.upload_files("thread-local", files=[file]))
assert result.success is True assert result.success is True
assert [file_info.filename for file_info in result.files] == ["notes.txt"] assert [file_info.filename for file_info in result.files] == ["notes_1.txt"]
assert existing_file.read_bytes() == b"new upload" assert existing_file.read_bytes() == b"old upload"
assert (thread_uploads_dir / "notes_1.txt").read_bytes() == b"new upload"
assert existing_file.stat().st_nlink == 1 assert existing_file.stat().st_nlink == 1
@ -706,18 +789,24 @@ def test_upload_files_oversized_replacement_preserves_existing_regular_file(tmp_
assert [path.name for path in thread_uploads_dir.iterdir()] == ["a.txt"] assert [path.name for path in thread_uploads_dir.iterdir()] == ["a.txt"]
def test_delete_uploaded_file_removes_generated_markdown_companion(tmp_path): def test_delete_uploaded_file_removes_owned_conversion_and_preserves_user_markdown(tmp_path):
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
(thread_uploads_dir / "report.pdf").write_bytes(b"pdf-bytes") primary = thread_uploads_dir / "report.pdf"
(thread_uploads_dir / "report.md").write_text("converted", encoding="utf-8") primary.write_bytes(b"pdf-bytes")
user_markdown = thread_uploads_dir / "report.md"
user_markdown.write_text("user", encoding="utf-8")
conversion = conversion_path_for_upload(primary)
conversion.parent.mkdir()
conversion.write_text("converted", encoding="utf-8")
with patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir): with patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir):
result = asyncio.run(call_unwrapped(uploads.delete_uploaded_file, "thread-aio", "report.pdf", request=MagicMock())) result = asyncio.run(call_unwrapped(uploads.delete_uploaded_file, "thread-aio", "report.pdf", request=MagicMock()))
assert result == {"success": True, "message": "Deleted report.pdf"} assert result == {"success": True, "message": "Deleted report.pdf"}
assert not (thread_uploads_dir / "report.pdf").exists() assert not primary.exists()
assert not (thread_uploads_dir / "report.md").exists() assert not conversion.exists()
assert user_markdown.read_text(encoding="utf-8") == "user"
def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors(): def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors():
@ -819,28 +908,8 @@ def test_upload_files_uses_configured_file_count_limit(tmp_path):
assert exc_info.value.status_code == 413 assert exc_info.value.status_code == 413
def _fake_convert_honoring_output_path(content_by_source: dict[str, str] | None = None):
"""Mimic convert_file_to_markdown, including optional output_path."""
async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path:
md_path = output_path if output_path is not None else file_path.with_suffix(".md")
if content_by_source is not None and file_path.name in content_by_source:
text = content_by_source[file_path.name]
else:
text = f"converted-from:{file_path.name}"
md_path.write_text(text, encoding="utf-8")
return md_path
return fake_convert
def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_path): def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_path):
"""Companion .md from auto-convert must not clobber a same-request .md upload. """Owned conversion output must not clobber a same-request Markdown upload."""
Declared invariant (upload_files): filenames within one request must not
silently truncate each other. convert_file_to_markdown used to write
stem.md unconditionally, bypassing claim_unique_filename.
"""
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
@ -851,8 +920,8 @@ def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_pa
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object( patch.object(
uploads, uploads,
"convert_file_to_markdown", "convert_uploaded_file_to_markdown",
AsyncMock(side_effect=_fake_convert_honoring_output_path({"notes.docx": "FROM_DOCX"})), AsyncMock(side_effect=_fake_owned_conversion({"notes.docx": "FROM_DOCX"})),
), ),
): ):
result = asyncio.run( result = asyncio.run(
@ -872,10 +941,8 @@ def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_pa
assert [f.filename for f in result.files] == ["notes.md", "notes.docx"] assert [f.filename for f in result.files] == ["notes.md", "notes.docx"]
# User upload preserved # User upload preserved
assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN"
# Converted companion got a unique name instead of overwriting assert result.files[1].markdown_file == "notes.docx.md"
assert result.files[1].markdown_file == "notes_1.md" assert conversion_path_for_upload(thread_uploads_dir / "notes.docx").read_text(encoding="utf-8") == "FROM_DOCX"
assert (thread_uploads_dir / "notes_1.md").read_text(encoding="utf-8") == "FROM_DOCX"
assert not (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM_DOCX"
def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path): def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path):
@ -890,8 +957,8 @@ def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object( patch.object(
uploads, uploads,
"convert_file_to_markdown", "convert_uploaded_file_to_markdown",
AsyncMock(side_effect=_fake_convert_honoring_output_path({"a.docx": "FROM_DOCX", "a.pdf": "FROM_PDF"})), AsyncMock(side_effect=_fake_owned_conversion({"a.docx": "FROM_DOCX", "a.pdf": "FROM_PDF"})),
), ),
): ):
result = asyncio.run( result = asyncio.run(
@ -908,17 +975,14 @@ def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path
) )
assert result.success is True assert result.success is True
assert result.files[0].markdown_file == "a.md" assert result.files[0].markdown_file == "a.docx.md"
assert result.files[1].markdown_file == "a_1.md" assert result.files[1].markdown_file == "a.pdf.md"
assert (thread_uploads_dir / "a.md").read_text(encoding="utf-8") == "FROM_DOCX" assert conversion_path_for_upload(thread_uploads_dir / "a.docx").read_text(encoding="utf-8") == "FROM_DOCX"
assert (thread_uploads_dir / "a_1.md").read_text(encoding="utf-8") == "FROM_PDF" assert conversion_path_for_upload(thread_uploads_dir / "a.pdf").read_text(encoding="utf-8") == "FROM_PDF"
# Each response entry points at content that belongs to that source
assert (thread_uploads_dir / result.files[0].markdown_file).read_text(encoding="utf-8") == "FROM_DOCX"
assert (thread_uploads_dir / result.files[1].markdown_file).read_text(encoding="utf-8") == "FROM_PDF"
def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten(tmp_path): def test_upload_files_user_markdown_after_convertible_keeps_its_name(tmp_path):
"""If convert claims stem.md first, a later same-request .md is renamed.""" """Generated output uses a separate namespace from a later user Markdown."""
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
@ -929,8 +993,8 @@ def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object( patch.object(
uploads, uploads,
"convert_file_to_markdown", "convert_uploaded_file_to_markdown",
AsyncMock(side_effect=_fake_convert_honoring_output_path({"notes.docx": "FROM_DOCX"})), AsyncMock(side_effect=_fake_owned_conversion({"notes.docx": "FROM_DOCX"})),
), ),
): ):
result = asyncio.run( result = asyncio.run(
@ -948,11 +1012,11 @@ def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten
assert result.success is True assert result.success is True
assert result.files[0].filename == "notes.docx" assert result.files[0].filename == "notes.docx"
assert result.files[0].markdown_file == "notes.md" assert result.files[0].markdown_file == "notes.docx.md"
assert result.files[1].filename == "notes_1.md" assert result.files[1].filename == "notes.md"
assert result.files[1].original_filename == "notes.md" assert result.files[1].original_filename is None
assert (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM_DOCX" assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN"
assert (thread_uploads_dir / "notes_1.md").read_bytes() == b"USER_MARKDOWN" assert conversion_path_for_upload(thread_uploads_dir / "notes.docx").read_text(encoding="utf-8") == "FROM_DOCX"
def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_path): def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_path):
@ -965,7 +1029,7 @@ def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_p
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()),
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object(uploads, "convert_file_to_markdown", AsyncMock(return_value=None)), patch.object(uploads, "convert_uploaded_file_to_markdown", AsyncMock(return_value=None)),
): ):
result = asyncio.run( result = asyncio.run(
call_unwrapped( call_unwrapped(
@ -985,7 +1049,7 @@ def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_p
assert result.files[1].filename == "notes.md" assert result.files[1].filename == "notes.md"
assert result.files[1].original_filename is None assert result.files[1].original_filename is None
assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN"
assert not (thread_uploads_dir / "notes_1.md").exists() assert not conversion_path_for_upload(thread_uploads_dir / "notes.docx").exists()
def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suffix(tmp_path): def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suffix(tmp_path):
@ -993,10 +1057,11 @@ def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suff
thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True) thread_uploads_dir.mkdir(parents=True)
async def convert_failing_on_docx(file_path: Path, output_path: Path | None = None) -> Path | None: async def convert_failing_on_docx(file_path: Path) -> Path | None:
if file_path.suffix.lower() == ".docx": if file_path.suffix.lower() == ".docx":
return None return None
md_path = output_path if output_path is not None else file_path.with_suffix(".md") md_path = conversion_path_for_upload(file_path)
md_path.parent.mkdir(parents=True, exist_ok=True)
md_path.write_text(f"FROM:{file_path.name}", encoding="utf-8") md_path.write_text(f"FROM:{file_path.name}", encoding="utf-8")
return md_path return md_path
@ -1005,7 +1070,11 @@ def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suff
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()),
patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True),
patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=convert_failing_on_docx)), patch.object(
uploads,
"convert_uploaded_file_to_markdown",
AsyncMock(side_effect=convert_failing_on_docx),
),
): ):
result = asyncio.run( result = asyncio.run(
call_unwrapped( call_unwrapped(
@ -1022,6 +1091,5 @@ def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suff
assert result.success is True assert result.success is True
assert result.files[0].markdown_file is None assert result.files[0].markdown_file is None
assert result.files[1].markdown_file == "notes.md" assert result.files[1].markdown_file == "notes.pdf.md"
assert (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM:notes.pdf" assert conversion_path_for_upload(thread_uploads_dir / "notes.pdf").read_text(encoding="utf-8") == "FROM:notes.pdf"
assert not (thread_uploads_dir / "notes_1.md").exists()