diff --git a/backend/app/gateway/path_utils.py b/backend/app/gateway/path_utils.py index ded348c78..43f4f9abc 100644 --- a/backend/app/gateway/path_utils.py +++ b/backend/app/gateway/path_utils.py @@ -8,13 +8,16 @@ from deerflow.config.paths import get_paths from deerflow.runtime.user_context import get_effective_user_id -def resolve_thread_virtual_path(thread_id: str, virtual_path: str) -> Path: +def resolve_thread_virtual_path(thread_id: str, virtual_path: str, user_id: str | None = None) -> Path: """Resolve a virtual path to the actual filesystem path under thread user-data. Args: thread_id: The thread ID. virtual_path: The virtual path as seen inside the sandbox (e.g., /mnt/user-data/outputs/file.txt). + user_id: The user whose storage to resolve under. Defaults to the + effective user when not given; callers acting on behalf of a + specific owner (e.g. trusted internal callers) pass it explicitly. Returns: The resolved filesystem path. @@ -23,7 +26,7 @@ def resolve_thread_virtual_path(thread_id: str, virtual_path: str) -> Path: HTTPException: If the path is invalid or outside allowed directories. """ try: - return get_paths().resolve_virtual_path(thread_id, virtual_path, user_id=get_effective_user_id()) + return get_paths().resolve_virtual_path(thread_id, virtual_path, user_id=user_id or get_effective_user_id()) except ValueError as e: status = 403 if "traversal" in str(e) else 400 raise HTTPException(status_code=status, detail=str(e)) diff --git a/backend/app/gateway/routers/artifacts.py b/backend/app/gateway/routers/artifacts.py index 34b86cafb..d7580eb40 100644 --- a/backend/app/gateway/routers/artifacts.py +++ b/backend/app/gateway/routers/artifacts.py @@ -9,7 +9,9 @@ from fastapi import APIRouter, HTTPException, Request from fastapi.responses import FileResponse, PlainTextResponse, Response from app.gateway.authz import require_permission +from app.gateway.internal_auth import get_trusted_internal_owner_user_id from app.gateway.path_utils import resolve_thread_virtual_path +from deerflow.config.paths import make_safe_user_id logger = logging.getLogger(__name__) @@ -181,6 +183,16 @@ async def get_artifact(thread_id: str, path: str, request: Request, download: bo - Download file: `/api/threads/abc123/artifacts/mnt/user-data/outputs/data.csv?download=true` - Active web content such as `.html`, `.xhtml`, and `.svg` artifacts is always downloaded """ + # Trusted internal callers may act on behalf of a thread's owner via the + # owner-user-id header (honored only after the internal token validates). + # The header carries the raw platform owner id, while runs store files + # under the make_safe_user_id bucket (the same normalization the channel + # file pipeline and the memory router apply), so resolution uses the + # normalized id. Browser/API callers get None here and fall back to the + # effective user. + raw_owner_user_id = get_trusted_internal_owner_user_id(request) + owner_user_id = make_safe_user_id(raw_owner_user_id) if raw_owner_user_id else None + # Check if this is a request for a file inside a .skill archive (e.g., xxx.skill/SKILL.md) if ".skill/" in path: # Split the path at ".skill/" to get the ZIP file path and internal path @@ -189,7 +201,7 @@ async def get_artifact(thread_id: str, path: str, request: Request, download: bo skill_file_path = path[: marker_pos + len(".skill")] # e.g., "mnt/user-data/outputs/my-skill.skill" internal_path = path[marker_pos + len(skill_marker) :] # e.g., "SKILL.md" - actual_skill_path = await asyncio.to_thread(resolve_thread_virtual_path, thread_id, skill_file_path) + actual_skill_path = await asyncio.to_thread(resolve_thread_virtual_path, thread_id, skill_file_path, user_id=owner_user_id) # Offload the stat probes + ZIP open/extract + MIME sniff (blocking filesystem IO). content, mime_type = await asyncio.to_thread(_load_skill_archive_member, actual_skill_path, skill_file_path, internal_path) @@ -209,7 +221,7 @@ async def get_artifact(thread_id: str, path: str, request: Request, download: bo except UnicodeDecodeError: return Response(content=content, media_type=mime_type or "application/octet-stream", headers=cache_headers) - actual_path = await asyncio.to_thread(resolve_thread_virtual_path, thread_id, path) + actual_path = await asyncio.to_thread(resolve_thread_virtual_path, thread_id, path, user_id=owner_user_id) logger.info(f"Resolving artifact path: thread_id={thread_id}, requested_path={path}, actual_path={actual_path}") diff --git a/backend/tests/test_artifacts_router.py b/backend/tests/test_artifacts_router.py index f0627ff7b..f53078e0b 100644 --- a/backend/tests/test_artifacts_router.py +++ b/backend/tests/test_artifacts_router.py @@ -1,6 +1,7 @@ import asyncio import zipfile from pathlib import Path +from types import SimpleNamespace import pytest from _router_auth_helpers import call_unwrapped, make_authed_test_app @@ -10,6 +11,8 @@ from starlette.requests import Request from starlette.responses import FileResponse import app.gateway.routers.artifacts as artifacts_router +from app.gateway.internal_auth import INTERNAL_OWNER_USER_ID_HEADER_NAME, INTERNAL_SYSTEM_ROLE +from deerflow.config.paths import make_safe_user_id ACTIVE_ARTIFACT_CASES = [ ("poc.html", ""), @@ -34,7 +37,7 @@ def test_get_artifact_reads_utf8_text_file_on_windows_locale(tmp_path, monkeypat return original_read_text(self, *args, **kwargs) monkeypatch.setattr(Path, "read_text", read_text_with_gbk_default) - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path: artifact_path) + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path) request = _make_request() response = asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/note.txt", request)) @@ -48,7 +51,7 @@ def test_get_artifact_forces_download_for_active_content(tmp_path, monkeypatch, artifact_path = tmp_path / filename artifact_path.write_text(content, encoding="utf-8") - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path: artifact_path) + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path) response = asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", f"mnt/user-data/outputs/{filename}", _make_request())) @@ -62,7 +65,7 @@ def test_get_artifact_forces_download_for_active_content_in_skill_archive(tmp_pa with zipfile.ZipFile(skill_path, "w") as zip_ref: zip_ref.writestr(filename, content) - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path: skill_path) + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: skill_path) response = asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", f"mnt/user-data/outputs/sample.skill/{filename}", _make_request())) @@ -74,7 +77,7 @@ def test_get_artifact_download_false_does_not_force_attachment(tmp_path, monkeyp artifact_path = tmp_path / "note.txt" artifact_path.write_text("hello", encoding="utf-8") - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path: artifact_path) + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path) app = make_authed_test_app() app.include_router(artifacts_router.router) @@ -92,7 +95,7 @@ def test_get_artifact_download_true_forces_attachment_for_skill_archive(tmp_path with zipfile.ZipFile(skill_path, "w") as zip_ref: zip_ref.writestr("notes.txt", "hello") - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path: skill_path) + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: skill_path) app = make_authed_test_app() app.include_router(artifacts_router.router) @@ -105,6 +108,83 @@ def test_get_artifact_download_true_forces_attachment_for_skill_archive(tmp_path assert response.headers.get("content-disposition", "").startswith("attachment;") +def _make_internal_request(owner: str | None, *, system_role: str = INTERNAL_SYSTEM_ROLE) -> Request: + """A request as it arrives from a trusted internal caller. + + ``system_role`` is stamped onto ``request.state.user`` the way + ``AuthMiddleware`` does after validating the internal token. When *owner* + is given it is carried in the owner-user-id header. + """ + headers: list[tuple[bytes, bytes]] = [] + if owner is not None: + headers.append((INTERNAL_OWNER_USER_ID_HEADER_NAME.lower().encode(), owner.encode())) + request = Request({"type": "http", "method": "GET", "path": "/", "headers": headers, "query_string": b""}) + request.state.user = SimpleNamespace(id="default", system_role=system_role) + return request + + +def _capture_resolved_user_id(monkeypatch, tmp_path) -> dict: + """Patch resolve_thread_virtual_path to record the user_id it is called with.""" + artifact_path = tmp_path / "index.html" + artifact_path.write_text("", encoding="utf-8") + seen: dict = {} + + def fake_resolve(_thread_id, _path, user_id=None): + seen["user_id"] = user_id + return artifact_path + + monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", fake_resolve) + return seen + + +def test_get_artifact_scopes_to_trusted_owner_header(tmp_path, monkeypatch) -> None: + # An internal caller acting for an owner must resolve the artifact under + # that owner's storage, not the synthetic internal user. + seen = _capture_resolved_user_id(monkeypatch, tmp_path) + request = _make_internal_request("owner-123") + + asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/index.html", request)) + + assert seen["user_id"] == "owner-123" + + +def test_get_artifact_normalizes_raw_owner_id_from_trusted_header(tmp_path, monkeypatch) -> None: + # The trusted header carries the raw platform owner id (channel workers + # send it unsanitized; see ChannelManager._owner_headers), while run files + # live under the make_safe_user_id bucket — so a raw id with chars outside + # [A-Za-z0-9_-] must resolve to the normalized bucket, not the raw one. + seen = _capture_resolved_user_id(monkeypatch, tmp_path) + raw_owner = "ou_7d8a.6e6d@example:id" + request = _make_internal_request(raw_owner) + + asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/index.html", request)) + + assert seen["user_id"] == make_safe_user_id(raw_owner) + assert seen["user_id"] != raw_owner + + +def test_get_artifact_without_owner_header_falls_back_to_effective_user(tmp_path, monkeypatch) -> None: + # No owner header → no override; resolution falls back to the effective user + # (user_id=None lets resolve_thread_virtual_path apply its default). + seen = _capture_resolved_user_id(monkeypatch, tmp_path) + request = _make_internal_request(None) + + asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/index.html", request)) + + assert seen["user_id"] is None + + +def test_get_artifact_ignores_owner_header_for_non_internal_caller(tmp_path, monkeypatch) -> None: + # The owner header is only trusted for internal callers; a normal user + # carrying it must not be able to read another user's storage. + seen = _capture_resolved_user_id(monkeypatch, tmp_path) + request = _make_internal_request("owner-123", system_role="user") + + asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/index.html", request)) + + assert seen["user_id"] is None + + def test_skill_archive_preview_rejects_oversized_member_before_decompression(tmp_path) -> None: skill_path = tmp_path / "sample.skill" payload = b"A" * (artifacts_router.MAX_SKILL_ARCHIVE_MEMBER_BYTES + 1)