diff --git a/backend/packages/harness/deerflow/uploads/__init__.py b/backend/packages/harness/deerflow/uploads/__init__.py index b5cfc1640..e335c2fb8 100644 --- a/backend/packages/harness/deerflow/uploads/__init__.py +++ b/backend/packages/harness/deerflow/uploads/__init__.py @@ -1,3 +1,15 @@ +from .conversion import convert_uploaded_file_to_markdown +from .layout import ( + UPLOAD_CONVERSIONS_DIRNAME, + UnsafeConversionPathError, + artifact_url_for_virtual_path, + conversion_dir_for_uploads, + conversion_path_for_upload, + conversion_virtual_path, + ensure_conversion_dir, + existing_conversion_path_for_upload, + validate_conversion_dir, +) from .manager import ( UPLOAD_STAGING_PREFIX, UPLOAD_STAGING_SUFFIX, @@ -18,6 +30,7 @@ from .manager import ( publish_staged_upload, publish_upload_bytes, publish_upload_copy, + replace_system_owned_staged_file, upload_artifact_url, upload_virtual_path, validate_path_traversal, @@ -26,6 +39,16 @@ from .manager import ( __all__ = [ "get_uploads_dir", + "UPLOAD_CONVERSIONS_DIRNAME", + "UnsafeConversionPathError", + "conversion_dir_for_uploads", + "conversion_path_for_upload", + "conversion_virtual_path", + "artifact_url_for_virtual_path", + "validate_conversion_dir", + "ensure_conversion_dir", + "existing_conversion_path_for_upload", + "convert_uploaded_file_to_markdown", "ensure_uploads_dir", "normalize_filename", "PathTraversalError", @@ -39,6 +62,7 @@ __all__ = [ "publish_staged_upload", "publish_upload_bytes", "publish_upload_copy", + "replace_system_owned_staged_file", "cleanup_stale_upload_staging_files", "is_upload_staging_file", "validate_path_traversal", diff --git a/backend/packages/harness/deerflow/uploads/conversion.py b/backend/packages/harness/deerflow/uploads/conversion.py new file mode 100644 index 000000000..a12074ae6 --- /dev/null +++ b/backend/packages/harness/deerflow/uploads/conversion.py @@ -0,0 +1,34 @@ +"""Safe publication of Markdown generated from primary uploads.""" + +from pathlib import Path + +from deerflow.uploads.layout import ( + UnsafeConversionPathError, + conversion_path_for_upload, + ensure_conversion_dir, +) +from deerflow.uploads.manager import ( + abort_staged_upload, + create_upload_staging_file, + replace_system_owned_staged_file, +) +from deerflow.utils.file_conversion import convert_file_to_markdown + + +async def convert_uploaded_file_to_markdown(upload_path: Path) -> Path | None: + """Convert one primary upload and atomically publish its owned Markdown.""" + conversion_dir = ensure_conversion_dir(upload_path.parent) + target = conversion_path_for_upload(upload_path) + staged = create_upload_staging_file(conversion_dir) + staged.handle.close() + try: + result = await convert_file_to_markdown(upload_path, output_path=staged.path) + if result is None: + abort_staged_upload(staged) + return None + if Path(result) != staged.path: + raise UnsafeConversionPathError("Converter returned an unexpected output path") + return replace_system_owned_staged_file(staged, target.name) + except Exception: + abort_staged_upload(staged) + raise diff --git a/backend/packages/harness/deerflow/uploads/layout.py b/backend/packages/harness/deerflow/uploads/layout.py index 9f0b234f3..de68f0466 100644 --- a/backend/packages/harness/deerflow/uploads/layout.py +++ b/backend/packages/harness/deerflow/uploads/layout.py @@ -1,5 +1,7 @@ """Path and URL layout helpers for primary uploads and generated assets.""" +import os +import stat from pathlib import Path from urllib.parse import quote @@ -8,6 +10,10 @@ from deerflow.config.paths import VIRTUAL_PATH_PREFIX UPLOAD_CONVERSIONS_DIRNAME = ".upload-conversions" +class UnsafeConversionPathError(ValueError): + """Raised when the generated-conversion namespace is unsafe.""" + + def conversion_dir_for_uploads(uploads_dir: Path) -> Path: """Return the system-owned conversion directory beside ``uploads_dir``.""" return uploads_dir.parent / UPLOAD_CONVERSIONS_DIRNAME @@ -18,6 +24,45 @@ def conversion_path_for_upload(upload_path: Path) -> Path: return conversion_dir_for_uploads(upload_path.parent) / f"{upload_path.name}.md" +def validate_conversion_dir(uploads_dir: Path) -> Path | None: + """Return an existing real conversion directory without following links.""" + conversion_dir = conversion_dir_for_uploads(uploads_dir) + try: + conversion_stat = os.lstat(conversion_dir) + except FileNotFoundError: + return None + if stat.S_ISLNK(conversion_stat.st_mode) or not stat.S_ISDIR(conversion_stat.st_mode): + raise UnsafeConversionPathError("Unsafe upload conversion directory") + return conversion_dir + + +def ensure_conversion_dir(uploads_dir: Path) -> Path: + """Create and validate the system-owned conversion directory.""" + conversion_dir = conversion_dir_for_uploads(uploads_dir) + try: + conversion_dir.mkdir(mode=0o755) + except FileExistsError: + pass + validated = validate_conversion_dir(uploads_dir) + if validated is None: + raise UnsafeConversionPathError("Upload conversion directory disappeared") + return validated + + +def existing_conversion_path_for_upload(upload_path: Path) -> Path | None: + """Return an existing safe generated file owned by ``upload_path``.""" + if validate_conversion_dir(upload_path.parent) is None: + return None + candidate = conversion_path_for_upload(upload_path) + try: + candidate_stat = os.lstat(candidate) + except FileNotFoundError: + return None + if not stat.S_ISREG(candidate_stat.st_mode) or candidate_stat.st_nlink != 1: + raise UnsafeConversionPathError("Unsafe upload conversion file") + return candidate + + def upload_virtual_path(filename: str) -> str: """Build the sandbox virtual path for a primary upload.""" return f"{VIRTUAL_PATH_PREFIX}/uploads/{filename}" diff --git a/backend/packages/harness/deerflow/uploads/manager.py b/backend/packages/harness/deerflow/uploads/manager.py index cfef74225..4f3334c62 100644 --- a/backend/packages/harness/deerflow/uploads/manager.py +++ b/backend/packages/harness/deerflow/uploads/manager.py @@ -17,7 +17,12 @@ from typing import BinaryIO from deerflow.config.paths import get_paths from deerflow.runtime.user_context import get_effective_user_id -from deerflow.uploads.layout import artifact_url_for_virtual_path, upload_virtual_path +from deerflow.uploads.layout import ( + UPLOAD_CONVERSIONS_DIRNAME, + artifact_url_for_virtual_path, + existing_conversion_path_for_upload, + upload_virtual_path, +) from deerflow.utils.thread_id import validate_thread_id @@ -245,6 +250,18 @@ def publish_upload_copy(base_dir: Path, preferred_filename: str, source_path: Pa raise +def replace_system_owned_staged_file(staged: StagedUpload, filename: str) -> Path: + """Atomically replace one generated file inside the owned namespace.""" + if staged.base_dir.name != UPLOAD_CONVERSIONS_DIRNAME: + raise UnsafeUploadPathError("System-owned replace requires conversion directory") + if not staged.handle.closed: + staged.handle.close() + _validate_staged_upload(staged) + target = staged.base_dir / normalize_filename(filename) + os.replace(staged.path, target) + return target + + def validate_path_traversal(path: Path, base: Path) -> None: """Verify that *path* is inside *base*. @@ -276,17 +293,25 @@ def validate_upload_destination(base_dir: Path, filename: str) -> Path: return dest -def _iter_upload_dirs(base_dir: Path): - yield from base_dir.glob("threads/*/user-data/uploads") - yield from base_dir.glob("users/*/threads/*/user-data/uploads") +def _iter_upload_storage_dirs(base_dir: Path): + for user_data_dir in base_dir.glob("threads/*/user-data"): + yield user_data_dir / "uploads" + yield user_data_dir / UPLOAD_CONVERSIONS_DIRNAME + for user_data_dir in base_dir.glob("users/*/threads/*/user-data"): + yield user_data_dir / "uploads" + yield user_data_dir / UPLOAD_CONVERSIONS_DIRNAME def cleanup_stale_upload_staging_files(base_dir: Path | str | None = None) -> int: """Remove orphaned Gateway upload staging files left by a hard crash.""" root = Path(base_dir) if base_dir is not None else get_paths().base_dir removed = 0 - for uploads_dir in _iter_upload_dirs(root): - if not uploads_dir.is_dir(): + for uploads_dir in _iter_upload_storage_dirs(root): + try: + directory_stat = os.lstat(uploads_dir) + except FileNotFoundError: + continue + if stat.S_ISLNK(directory_stat.st_mode) or not stat.S_ISDIR(directory_stat.st_mode): continue try: with os.scandir(uploads_dir) as entries: @@ -346,18 +371,12 @@ def list_files_in_dir(directory: Path) -> dict: return {"files": files, "count": len(files)} -def delete_file_safe(base_dir: Path, filename: str, *, convertible_extensions: set[str] | None = None) -> dict: - """Delete a file inside *base_dir* after path-traversal validation. - - If *convertible_extensions* is provided and the file's extension matches, - the companion ``.md`` file is also removed (if it exists). +def delete_file_safe(base_dir: Path, filename: str) -> dict: + """Delete a primary upload and only its exact owned conversion. Args: base_dir: Directory containing the file. filename: Name of file to delete. - convertible_extensions: Lowercase extensions (e.g. ``{".pdf", ".docx"}``) - whose companion markdown should be cleaned up. - Returns: Dict with success and message. @@ -365,17 +384,22 @@ def delete_file_safe(base_dir: Path, filename: str, *, convertible_extensions: s FileNotFoundError: If the file does not exist. PathTraversalError: If path traversal is detected. """ - file_path = (base_dir / filename).resolve() - validate_path_traversal(file_path, base_dir) - - if not file_path.is_file(): + safe_name = normalize_filename(filename) + if safe_name != filename: + raise PathTraversalError("Path traversal detected") + base_dir = _validate_upload_directory(Path(base_dir)) + file_path = base_dir / safe_name + try: + file_stat = os.lstat(file_path) + except FileNotFoundError: raise FileNotFoundError(f"File not found: {filename}") + if not stat.S_ISREG(file_stat.st_mode) or file_stat.st_nlink != 1: + raise UnsafeUploadPathError(f"Unsafe upload file: {safe_name}") + owned_conversion = existing_conversion_path_for_upload(file_path) file_path.unlink() - - # Clean up companion markdown generated during upload conversion. - if convertible_extensions and file_path.suffix.lower() in convertible_extensions: - file_path.with_suffix(".md").unlink(missing_ok=True) + if owned_conversion is not None: + owned_conversion.unlink(missing_ok=True) return {"success": True, "message": f"Deleted {filename}"} diff --git a/backend/packages/harness/deerflow/utils/file_outline.py b/backend/packages/harness/deerflow/utils/file_outline.py index f474b1305..e1f7524a3 100644 --- a/backend/packages/harness/deerflow/utils/file_outline.py +++ b/backend/packages/harness/deerflow/utils/file_outline.py @@ -10,6 +10,11 @@ import logging import re from pathlib import Path +from deerflow.uploads.layout import ( + UnsafeConversionPathError, + existing_conversion_path_for_upload, +) + logger = logging.getLogger(__name__) # Regex for bold structural headings produced by pymupdf4llm when it can't @@ -127,8 +132,7 @@ def extract_outline(md_path: Path) -> list[dict]: def extract_outline_for_file(file_path: Path) -> tuple[list[dict], list[str]]: """Return the document outline and fallback preview for *file_path*. - Looks for a sibling ``.md`` file produced by the upload conversion - pipeline. + Looks only for the system-owned Markdown generated for this exact upload. Returns: (outline, preview) where: @@ -138,8 +142,12 @@ def extract_outline_for_file(file_path: Path) -> tuple[list[dict], list[str]]: anchor when outline is empty so the agent has some context. Empty when outline is non-empty (no fallback needed). """ - md_path = file_path.with_suffix(".md") - if not md_path.is_file(): + try: + md_path = existing_conversion_path_for_upload(file_path) + except UnsafeConversionPathError: + logger.warning("Ignoring unsafe generated conversion for %s", file_path.name) + return [], [] + if md_path is None: return [], [] outline = extract_outline(md_path) diff --git a/backend/tests/test_list_uploaded_files_tool.py b/backend/tests/test_list_uploaded_files_tool.py index 4987e98b2..99b545fb9 100644 --- a/backend/tests/test_list_uploaded_files_tool.py +++ b/backend/tests/test_list_uploaded_files_tool.py @@ -10,6 +10,7 @@ from langchain_core.messages import HumanMessage, ToolMessage from deerflow.config.paths import Paths from deerflow.tools.builtins.list_uploaded_files_tool import _format_omitted_summary, _list_uploaded_files_impl, _resolve_thread_id +from deerflow.uploads.layout import conversion_path_for_upload def _paths(tmp_path): @@ -24,6 +25,13 @@ def _uploads_dir(tmp_path: Path, thread_id: str = "thread-abc") -> Path: return d +def _write_conversion(upload_path: Path, content: str) -> Path: + conversion = conversion_path_for_upload(upload_path) + conversion.parent.mkdir(parents=True, exist_ok=True) + conversion.write_text(content, encoding="utf-8") + return conversion + + def _runtime(thread_id: str = "thread-abc", state_uploaded: list[dict] | None = None): rt = MagicMock() rt.context = {"thread_id": thread_id} @@ -177,8 +185,9 @@ class TestListUploadedFiles: def test_include_outline_true(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "doc.pdf").write_bytes(b"%PDF") - (uploads_dir / "doc.md").write_text("# Heading 1\n\n## Heading 2\n\nBody text.\n", encoding="utf-8") + primary = uploads_dir / "doc.pdf" + primary.write_bytes(b"%PDF") + _write_conversion(primary, "# Heading 1\n\n## Heading 2\n\nBody text.\n") result = _list_uploaded_files_impl(include_outline=True, runtime=_runtime(), _paths=_paths(tmp_path)) @@ -189,10 +198,12 @@ class TestListUploadedFiles: def test_include_outline_list(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "a.pdf").write_bytes(b"%PDF") - (uploads_dir / "a.md").write_text("# A Heading\n", encoding="utf-8") - (uploads_dir / "b.pdf").write_bytes(b"%PDF") - (uploads_dir / "b.md").write_text("# B Heading\n", encoding="utf-8") + primary_a = uploads_dir / "a.pdf" + primary_a.write_bytes(b"%PDF") + _write_conversion(primary_a, "# A Heading\n") + primary_b = uploads_dir / "b.pdf" + primary_b.write_bytes(b"%PDF") + _write_conversion(primary_b, "# B Heading\n") result = _list_uploaded_files_impl(include_outline=["a.pdf"], runtime=_runtime(), _paths=_paths(tmp_path)) @@ -202,8 +213,9 @@ class TestListUploadedFiles: def test_include_outline_false(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "doc.pdf").write_bytes(b"%PDF") - (uploads_dir / "doc.md").write_text("# Heading\n", encoding="utf-8") + primary = uploads_dir / "doc.pdf" + primary.write_bytes(b"%PDF") + _write_conversion(primary, "# Heading\n") result = _list_uploaded_files_impl(include_outline=False, runtime=_runtime(), _paths=_paths(tmp_path)) @@ -212,8 +224,9 @@ class TestListUploadedFiles: def test_fallback_preview_when_no_headings(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "plain.pdf").write_bytes(b"%PDF") - (uploads_dir / "plain.md").write_text("Just some text.\nNo headings.\n", encoding="utf-8") + primary = uploads_dir / "plain.pdf" + primary.write_bytes(b"%PDF") + _write_conversion(primary, "Just some text.\nNo headings.\n") result = _list_uploaded_files_impl(include_outline=True, runtime=_runtime(), _paths=_paths(tmp_path)) @@ -222,6 +235,17 @@ class TestListUploadedFiles: assert "outline_preview" in f assert "Just some text." in f["outline_preview"] + def test_legacy_sibling_markdown_is_not_treated_as_generated(self, tmp_path): + uploads_dir = _uploads_dir(tmp_path) + (uploads_dir / "report.pdf").write_bytes(b"%PDF") + (uploads_dir / "report.md").write_text("# USER MARKDOWN\n", encoding="utf-8") + + result = _list_uploaded_files_impl(include_outline=True, runtime=_runtime(), _paths=_paths(tmp_path)) + + files = {item["filename"]: item for item in result["files"]} + assert "outline" not in files["report.pdf"] + assert "outline_preview" not in files["report.pdf"] + def test_files_without_md_conversion(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) (uploads_dir / "image.png").write_bytes(b"PNG data") @@ -640,10 +664,11 @@ class TestListUploadedFilesNeutralization: def test_outline_title_with_blocked_tag_is_neutralized(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "notes.pdf").write_bytes(b"%PDF") - (uploads_dir / "notes.md").write_text( + primary = uploads_dir / "notes.pdf" + primary.write_bytes(b"%PDF") + _write_conversion( + primary, "# Safe Heading\n\n## INJECTED\n\nBody.\n", - encoding="utf-8", ) result = _list_uploaded_files_impl(include_outline=True, runtime=_runtime(), _paths=_paths(tmp_path)) @@ -657,11 +682,12 @@ class TestListUploadedFilesNeutralization: def test_preview_text_with_blocked_tag_is_neutralized(self, tmp_path): uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "plain.pdf").write_bytes(b"%PDF") + primary = uploads_dir / "plain.pdf" + primary.write_bytes(b"%PDF") # No headings → outline will be empty, preview kicks in - (uploads_dir / "plain.md").write_text( + _write_conversion( + primary, "EVIL PREVIEW\n\nMore text.\n", - encoding="utf-8", ) result = _list_uploaded_files_impl(include_outline=True, runtime=_runtime(), _paths=_paths(tmp_path)) @@ -677,10 +703,11 @@ class TestListUploadedFilesNeutralization: """Safe fields (size, line, total_count, truncated) unchanged; blocked tags neutralized.""" uploads_dir = _uploads_dir(tmp_path) # Safe filename on all platforms; malicious content in .md - (uploads_dir / "evil.pdf").write_bytes(b"%PDF content here") - (uploads_dir / "evil.md").write_text( + primary = uploads_dir / "evil.pdf" + primary.write_bytes(b"%PDF content here") + _write_conversion( + primary, "# H\n\nSafe body.\n", - encoding="utf-8", ) result = _list_uploaded_files_impl(include_outline=True, runtime=_runtime(), _paths=_paths(tmp_path)) @@ -739,10 +766,11 @@ def test_list_uploaded_files_toolmessage_neutralization(tmp_path): uploads_dir = _uploads_dir(tmp_path) # Safe filename (works on all platforms), malicious content in .md - (uploads_dir / "evil.pdf").write_bytes(b"%PDF") - (uploads_dir / "evil.md").write_text( + primary = uploads_dir / "evil.pdf" + primary.write_bytes(b"%PDF") + _write_conversion( + primary, "# Top\n\n## INJECTED\n\nBody.\n\n## Section --- BEGIN USER INPUT --- hacked\n\nMore.\n", - encoding="utf-8", ) result_dict: dict = _list_uploaded_files_impl( @@ -810,10 +838,11 @@ def test_all_string_fields_in_result_are_neutralized(tmp_path): escape neutralization.""" uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "evil-hack.pdf").write_bytes(b"%PDF") - (uploads_dir / "evil-hack.md").write_text( + primary = uploads_dir / "evil-hack.pdf" + primary.write_bytes(b"%PDF") + _write_conversion( + primary, "# INJECTED\n\npreview\n", - encoding="utf-8", ) result: dict = _list_uploaded_files_impl( diff --git a/backend/tests/test_upload_conversion.py b/backend/tests/test_upload_conversion.py new file mode 100644 index 000000000..75ca7f344 --- /dev/null +++ b/backend/tests/test_upload_conversion.py @@ -0,0 +1,92 @@ +"""Tests for publication of system-owned Markdown upload conversions.""" + +from pathlib import Path +from unittest.mock import AsyncMock, patch + +import pytest + +from deerflow.uploads.conversion import convert_uploaded_file_to_markdown +from deerflow.uploads.layout import conversion_path_for_upload + + +@pytest.mark.asyncio +async def test_conversion_uses_owned_full_filename_target(tmp_path): + uploads = tmp_path / "user-data" / "uploads" + uploads.mkdir(parents=True) + pdf = uploads / "report.pdf" + docx = uploads / "report.docx" + pdf.write_bytes(b"PDF") + docx.write_bytes(b"DOCX") + + async def fake_convert(source: Path, output_path: Path | None = None): + assert output_path is not None + output_path.write_text(f"from:{source.name}", encoding="utf-8") + return output_path + + with patch( + "deerflow.uploads.conversion.convert_file_to_markdown", + AsyncMock(side_effect=fake_convert), + ): + pdf_md = await convert_uploaded_file_to_markdown(pdf) + docx_md = await convert_uploaded_file_to_markdown(docx) + + assert pdf_md == conversion_path_for_upload(pdf) + assert docx_md == conversion_path_for_upload(docx) + assert pdf_md.read_text(encoding="utf-8") == "from:report.pdf" + assert docx_md.read_text(encoding="utf-8") == "from:report.docx" + + +@pytest.mark.asyncio +async def test_conversion_failure_cleans_stage_and_keeps_user_markdown(tmp_path): + uploads = tmp_path / "user-data" / "uploads" + uploads.mkdir(parents=True) + source = uploads / "report.pdf" + source.write_bytes(b"PDF") + user_markdown = uploads / "report.md" + user_markdown.write_text("user", encoding="utf-8") + + with patch( + "deerflow.uploads.conversion.convert_file_to_markdown", + AsyncMock(return_value=None), + ): + assert await convert_uploaded_file_to_markdown(source) is None + + assert user_markdown.read_text(encoding="utf-8") == "user" + conversion_dir = uploads.parent / ".upload-conversions" + assert not list(conversion_dir.glob(".upload-*.part")) + + +@pytest.mark.asyncio +async def test_conversion_directory_symlink_is_rejected(tmp_path): + uploads = tmp_path / "user-data" / "uploads" + uploads.mkdir(parents=True) + source = uploads / "report.pdf" + source.write_bytes(b"PDF") + outside = tmp_path / "outside" + outside.mkdir() + (uploads.parent / ".upload-conversions").symlink_to(outside, target_is_directory=True) + + with pytest.raises(ValueError, match="conversion directory"): + await convert_uploaded_file_to_markdown(source) + + assert list(outside.iterdir()) == [] + + +@pytest.mark.asyncio +async def test_unexpected_converter_output_is_rejected_and_cleaned(tmp_path): + uploads = tmp_path / "user-data" / "uploads" + uploads.mkdir(parents=True) + source = uploads / "report.pdf" + source.write_bytes(b"PDF") + unexpected = uploads / "report.md" + + with patch( + "deerflow.uploads.conversion.convert_file_to_markdown", + AsyncMock(return_value=unexpected), + ): + with pytest.raises(ValueError, match="unexpected output path"): + await convert_uploaded_file_to_markdown(source) + + assert not unexpected.exists() + conversion_dir = uploads.parent / ".upload-conversions" + assert not list(conversion_dir.glob(".upload-*.part")) diff --git a/backend/tests/test_uploads_manager.py b/backend/tests/test_uploads_manager.py index 1e93b359d..04af37de2 100644 --- a/backend/tests/test_uploads_manager.py +++ b/backend/tests/test_uploads_manager.py @@ -15,6 +15,7 @@ from deerflow.uploads.layout import ( from deerflow.uploads.manager import ( AtomicUploadPublishError, PathTraversalError, + UnsafeUploadPathError, claim_unique_filename, cleanup_stale_upload_staging_files, delete_file_safe, @@ -298,12 +299,22 @@ class TestCleanupStaleUploadStagingFiles: def test_removes_only_stale_staging_files_from_all_upload_layouts(self, tmp_path): legacy_uploads = tmp_path / "threads" / "thread-legacy" / "user-data" / "uploads" user_uploads = tmp_path / "users" / "owner-1" / "threads" / "thread-owned" / "user-data" / "uploads" + legacy_conversions = legacy_uploads.parent / ".upload-conversions" + user_conversions = user_uploads.parent / ".upload-conversions" unrelated_uploads = tmp_path / "misc" / "thread-other" / "user-data" / "uploads" - for uploads_dir in (legacy_uploads, user_uploads, unrelated_uploads): + for uploads_dir in ( + legacy_uploads, + user_uploads, + legacy_conversions, + user_conversions, + unrelated_uploads, + ): uploads_dir.mkdir(parents=True) (legacy_uploads / ".upload-old.part").write_text("legacy partial") (user_uploads / ".upload-new.part").write_text("user partial") + (legacy_conversions / ".upload-converted-old.part").write_text("legacy conversion partial") + (user_conversions / ".upload-converted-new.part").write_text("user conversion partial") (unrelated_uploads / ".upload-ignore.part").write_text("outside layout") (legacy_uploads / ".env").write_text("intentional dotfile") (legacy_uploads / ".upload-note.txt").write_text("intentional upload") @@ -311,14 +322,28 @@ class TestCleanupStaleUploadStagingFiles: removed = cleanup_stale_upload_staging_files(tmp_path) - assert removed == 2 + assert removed == 4 assert not (legacy_uploads / ".upload-old.part").exists() assert not (user_uploads / ".upload-new.part").exists() + assert not (legacy_conversions / ".upload-converted-old.part").exists() + assert not (user_conversions / ".upload-converted-new.part").exists() assert (unrelated_uploads / ".upload-ignore.part").exists() assert (legacy_uploads / ".env").exists() assert (legacy_uploads / ".upload-note.txt").exists() assert (legacy_uploads / "draft.part").exists() + def test_does_not_follow_symlinked_conversion_directory(self, tmp_path): + user_data = tmp_path / "threads" / "thread-legacy" / "user-data" + (user_data / "uploads").mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + staged = outside / ".upload-outside.part" + staged.write_text("outside") + (user_data / ".upload-conversions").symlink_to(outside, target_is_directory=True) + + assert cleanup_stale_upload_staging_files(tmp_path) == 0 + assert staged.read_text() == "outside" + # --------------------------------------------------------------------------- # delete_file_safe @@ -340,3 +365,43 @@ class TestDeleteFileSafe: def test_delete_traversal_raises(self, tmp_path): with pytest.raises(PathTraversalError, match="traversal"): delete_file_safe(tmp_path, "../outside.txt") + + def test_delete_rejects_path_components(self, tmp_path): + primary = tmp_path / "report.pdf" + primary.write_bytes(b"PDF") + + with pytest.raises(PathTraversalError, match="traversal"): + delete_file_safe(tmp_path, "folder/report.pdf") + + assert primary.exists() + + def test_delete_rejects_symlink_instead_of_unlinking_target(self, tmp_path): + outside = tmp_path / "outside.txt" + outside.write_text("protected", encoding="utf-8") + uploads = tmp_path / "uploads" + uploads.mkdir() + planted = uploads / "report.pdf" + planted.symlink_to(outside) + + with pytest.raises(UnsafeUploadPathError): + delete_file_safe(uploads, "report.pdf") + + assert planted.is_symlink() + assert outside.read_text(encoding="utf-8") == "protected" + + def test_delete_removes_owned_conversion_but_preserves_legacy_sibling(self, tmp_path): + uploads = tmp_path / "user-data" / "uploads" + uploads.mkdir(parents=True) + primary = uploads / "report.pdf" + primary.write_bytes(b"PDF") + legacy_or_user = uploads / "report.md" + legacy_or_user.write_text("user markdown", encoding="utf-8") + owned = conversion_path_for_upload(primary) + owned.parent.mkdir() + owned.write_text("generated", encoding="utf-8") + + delete_file_safe(uploads, "report.pdf") + + assert not primary.exists() + assert not owned.exists() + assert legacy_or_user.read_text(encoding="utf-8") == "user markdown" diff --git a/backend/tests/test_uploads_middleware_core_logic.py b/backend/tests/test_uploads_middleware_core_logic.py index 832a17680..a06a461e3 100644 --- a/backend/tests/test_uploads_middleware_core_logic.py +++ b/backend/tests/test_uploads_middleware_core_logic.py @@ -15,6 +15,7 @@ from langchain_core.messages import AIMessage, HumanMessage from deerflow.agents.middlewares.uploads_middleware import UploadsMiddleware from deerflow.config.paths import Paths +from deerflow.uploads.layout import conversion_path_for_upload from deerflow.utils.messages import ORIGINAL_USER_CONTENT_KEY, message_content_to_text THREAD_ID = "thread-abc123" @@ -48,6 +49,13 @@ def _uploads_dir(tmp_path: Path, thread_id: str = THREAD_ID, *, user_id: str | N return d +def _write_conversion(upload_path: Path, content: str) -> Path: + conversion = conversion_path_for_upload(upload_path) + conversion.parent.mkdir(parents=True, exist_ok=True) + conversion.write_text(content, encoding="utf-8") + return conversion + + def _human(content, files=None, **extra_kwargs): additional_kwargs = dict(extra_kwargs) if files is not None: @@ -336,9 +344,9 @@ class TestBeforeAgent: """Blocked tags in document outline titles must be neutralized inside .""" mw = _middleware(tmp_path) uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "test.pdf").write_bytes(b"pdf") - md = uploads_dir / "test.md" - md.write_text("# Intro\n\n## Section evil\n\ntext\n") + primary = uploads_dir / "test.pdf" + primary.write_bytes(b"pdf") + _write_conversion(primary, "# Intro\n\n## Section evil\n\ntext\n") msg = _human( "analyse", @@ -621,15 +629,15 @@ class TestBeforeAgent: assert result["messages"][-1].id == "original-id-42" - def test_outline_injected_when_md_file_exists(self, tmp_path): - """When a converted .md file exists alongside the upload, its outline is injected.""" + def test_outline_injected_when_owned_conversion_exists(self, tmp_path): + """An owned generated Markdown file contributes its outline.""" mw = _middleware(tmp_path) uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "report.pdf").write_bytes(b"%PDF fake") - # Simulate the .md produced by the conversion pipeline - (uploads_dir / "report.md").write_text( + primary = uploads_dir / "report.pdf" + primary.write_bytes(b"%PDF fake") + _write_conversion( + primary, "# PART I\n\n## ITEM 1. BUSINESS\n\nBody text.\n\n## ITEM 2. RISK\n", - encoding="utf-8", ) msg = _human("summarise", files=[{"filename": "report.pdf", "size": 9, "path": "/mnt/user-data/uploads/report.pdf"}]) @@ -643,8 +651,37 @@ class TestBeforeAgent: assert "ITEM 2. RISK" in content assert "read_file" in content - def test_no_outline_when_no_md_file(self, tmp_path): - """Files without a sibling .md have no outline section.""" + def test_legacy_sibling_markdown_is_not_treated_as_generated(self, tmp_path): + mw = _middleware(tmp_path) + uploads_dir = _uploads_dir(tmp_path) + (uploads_dir / "report.pdf").write_bytes(b"%PDF fake") + (uploads_dir / "report.md").write_text("# USER MARKDOWN\n", encoding="utf-8") + + msg = _human("summarise", files=[{"filename": "report.pdf", "size": 9, "path": "/mnt/user-data/uploads/report.pdf"}]) + result = mw.before_agent(self._state(msg), _runtime()) + + assert result is not None + assert "USER MARKDOWN" not in result["messages"][-1].content + + def test_unsafe_owned_conversion_symlink_is_ignored(self, tmp_path): + mw = _middleware(tmp_path) + uploads_dir = _uploads_dir(tmp_path) + primary = uploads_dir / "report.pdf" + primary.write_bytes(b"%PDF fake") + outside = tmp_path / "outside.md" + outside.write_text("# OUTSIDE SECRET\n", encoding="utf-8") + conversion = conversion_path_for_upload(primary) + conversion.parent.mkdir(parents=True) + conversion.symlink_to(outside) + + msg = _human("summarise", files=[{"filename": "report.pdf", "size": 9, "path": "/mnt/user-data/uploads/report.pdf"}]) + result = mw.before_agent(self._state(msg), _runtime()) + + assert result is not None + assert "OUTSIDE SECRET" not in result["messages"][-1].content + + def test_no_outline_when_no_generated_markdown(self, tmp_path): + """Files without an owned conversion have no outline section.""" mw = _middleware(tmp_path) uploads_dir = _uploads_dir(tmp_path) (uploads_dir / "data.xlsx").write_bytes(b"fake-xlsx") @@ -662,10 +699,11 @@ class TestBeforeAgent: mw = _middleware(tmp_path) uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "big.pdf").write_bytes(b"%PDF fake") + primary = uploads_dir / "big.pdf" + primary.write_bytes(b"%PDF fake") # Write MAX_OUTLINE_ENTRIES + 5 headings so truncation is triggered headings = "\n".join(f"# Heading {i}" for i in range(MAX_OUTLINE_ENTRIES + 5)) - (uploads_dir / "big.md").write_text(headings, encoding="utf-8") + _write_conversion(primary, headings) msg = _human("read", files=[{"filename": "big.pdf", "size": 9, "path": "/mnt/user-data/uploads/big.pdf"}]) result = mw.before_agent(self._state(msg), _runtime()) @@ -679,8 +717,9 @@ class TestBeforeAgent: """Short outlines (under the cap) must not show a truncation hint.""" mw = _middleware(tmp_path) uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "short.pdf").write_bytes(b"%PDF fake") - (uploads_dir / "short.md").write_text("# Intro\n\n# Conclusion\n", encoding="utf-8") + primary = uploads_dir / "short.pdf" + primary.write_bytes(b"%PDF fake") + _write_conversion(primary, "# Intro\n\n# Conclusion\n") msg = _human("read", files=[{"filename": "short.pdf", "size": 9, "path": "/mnt/user-data/uploads/short.pdf"}]) result = mw.before_agent(self._state(msg), _runtime()) @@ -693,11 +732,11 @@ class TestBeforeAgent: """When .md exists but has no headings, first lines are shown as a preview.""" mw = _middleware(tmp_path) uploads_dir = _uploads_dir(tmp_path) - (uploads_dir / "report.pdf").write_bytes(b"%PDF fake") - # .md with no # headings — plain prose only - (uploads_dir / "report.md").write_text( + primary = uploads_dir / "report.pdf" + primary.write_bytes(b"%PDF fake") + _write_conversion( + primary, "Annual Financial Report 2024\n\nThis document summarises key findings.\n\nRevenue grew by 12%.\n", - encoding="utf-8", ) msg = _human("analyse", files=[{"filename": "report.pdf", "size": 9, "path": "/mnt/user-data/uploads/report.pdf"}])