From 8e86729aa0290260a41240ec8c8e2b09087450ae Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Thu, 10 Sep 2026 16:01:56 +0900 Subject: [PATCH] fix(gateway): confine artifact PUT to /mnt/user-data/outputs after path resolution (#5321) * fix(gateway): confine artifact PUT to /mnt/user-data/outputs after path resolution The outputs-only guard on PUT /api/threads/{id}/artifacts/{path} was a string-prefix check on the raw path. A percent-encoded `..` (`outputs/%2e%2e/uploads/x.txt`) survives nginx's variable proxy_pass untouched, is decoded by Starlette, passes the prefix check, and the resolver only confines the result to `user-data/` -- so an owner could overwrite a sibling upload or workspace file in their own thread. Collapse dot segments before the prefix check, and re-check the resolved host path against the resolved outputs root so a symlink planted inside `outputs/` cannot redirect the write either. The normalized virtual path is what the response echoes and what non-mounted sandboxes receive. * refactor(gateway): share the outputs-confinement rule with channel attachments Review follow-up on #5321: the "only under /mnt/user-data/outputs" rule was implemented independently by the artifact editor and by IM-channel attachment delivery, and the two copies had already drifted. Move it into app/gateway/path_utils.py as normalize_outputs_virtual_path (collapse `..` before the prefix check) and resolve_outputs_confined_path (re-check the resolved host path against the resolved outputs root, which also catches a symlink planted inside outputs/). PUT /artifacts and ChannelManager._resolve_attachments both call the helper; artifact_archive keeps its stricter ZIP-member rules layered on top. Tests that previously stubbed resolve_thread_virtual_path for the editor now stub resolve_outputs_confined_path, and the channel attachment tests patch path_utils.get_paths, which the helper binds at import like the other consumers. The confinement itself is pinned by tests/test_gateway_path_utils.py. --- CHANGELOG.md | 11 ++ CHANGELOG_zh.md | 9 ++ backend/app/channels/AGENTS.md | 2 +- backend/app/channels/manager.py | 28 ++-- backend/app/gateway/AGENTS.md | 2 +- backend/app/gateway/path_utils.py | 50 +++++- backend/app/gateway/routers/artifacts.py | 18 ++- backend/tests/test_artifacts_router.py | 105 ++++++++++++- .../tests/test_channel_file_attachments.py | 44 +++++- backend/tests/test_channels.py | 3 + backend/tests/test_gateway_path_utils.py | 145 ++++++++++++++++++ backend/tests/test_sandbox_authorization.py | 2 +- 12 files changed, 379 insertions(+), 40 deletions(-) create mode 100644 backend/tests/test_gateway_path_utils.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 63881901b..c5e2cdea9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -548,6 +548,16 @@ This section accumulates work toward the **2.1.0** milestone ### Fixed +- **artifacts:** Keep `PUT /api/threads/{id}/artifacts/{path}` confined to + `/mnt/user-data/outputs`. The outputs-only guard was a string-prefix check on + the raw path, so a percent-encoded `..` (`outputs/%2e%2e/uploads/x.txt`) — + which nginx forwards untouched and Starlette decodes — passed it, and the + resolver only confines to `user-data/`, letting a caller overwrite a sibling + upload or workspace file in their own thread. Dot segments are now collapsed + before the prefix check, and the resolved host path is re-checked against the + resolved outputs root so a symlink planted inside `outputs/` cannot redirect + the write either. The rule now lives in one shared helper that IM-channel + attachment delivery uses as well, so the two copies cannot drift. ([#5321]) - **gateway:** Stop persisting a caller-supplied `deerflow_trace_id` on the run record. `body.metadata` reaches both the live run config, which the run worker restamps, and the run record echoed verbatim by the runs API; only the @@ -2678,3 +2688,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5282]: https://github.com/bytedance/deer-flow/pull/5282 [#5284]: https://github.com/bytedance/deer-flow/pull/5284 [#5287]: https://github.com/bytedance/deer-flow/pull/5287 +[#5321]: https://github.com/bytedance/deer-flow/pull/5321 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 6cfd804d3..c813874e1 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -392,6 +392,14 @@ ### 修复 +- **Artifact：** `PUT /api/threads/{id}/artifacts/{path}` 现在严格限制在 + `/mnt/user-data/outputs` 之内。此前 outputs-only 校验只是对原始路径做字符串前缀 + 检查，百分号编码的 `..`（`outputs/%2e%2e/uploads/x.txt`，nginx 原样转发、Starlette + 解码后）可以通过，而路径解析器只把结果限制在 `user-data/` 内，因此调用者能覆盖自己 + 线程里的上传文件或 workspace 文件。现在会先折叠 `.`/`..` 段再做前缀检查，并把解析 + 后的宿主机路径与解析后的 outputs 根目录再次比对，`outputs/` 内被植入的符号链接同样 + 无法把写入重定向到别处。该规则现在收敛为一个共享 helper，IM 渠道的附件投递也走同 + 一实现，两处不会再各自漂移。([#5321]) - **运行时：** 会话元数据现在仅在 run 通过启动屏障后才切换为 `running`，待取消的 run 不再短暂呈现 `running` 状态；worker 启动期间客户端可能观察到先前的会话状态 。([#4450]) @@ -2077,3 +2085,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5281]: https://github.com/bytedance/deer-flow/pull/5281 [#5284]: https://github.com/bytedance/deer-flow/pull/5284 [#5287]: https://github.com/bytedance/deer-flow/pull/5287 +[#5321]: https://github.com/bytedance/deer-flow/pull/5321 diff --git a/backend/app/channels/AGENTS.md b/backend/app/channels/AGENTS.md index 7650d4877..ccc5e1e4a 100644 --- a/backend/app/channels/AGENTS.md +++ b/backend/app/channels/AGENTS.md @@ -42,7 +42,7 @@ Bridges external messaging platforms (Feishu, Slack, Telegram, Discord, DingTalk - `FeishuChannel._receive_single_file(...)` / `DingTalkChannel._receive_single_file(...)` — normalize provider filenames, claim a collision-free basename and write it through `write_upload_file_no_symlink` under the same channel lock; the returned basename drives both the agent-visible virtual path and non-local sandbox sync - `sandbox_files.py` — non-mounted Feishu/DingTalk syncs acquire unique non-releasing execution holders, drain blocking `update_file` workers across repeated cancellation, and release only after the last sandbox operation, so a parallel run cannot close the shared client mid-upload - `_ingest_inbound_files(...)` and the underlying `ensure_uploads_dir` / `get_uploads_dir` — owner-scoped via the same kwarg -- `_resolve_attachments` / `_prepare_artifact_delivery` — resolve output artifacts from the bound owner's bucket +- `_resolve_attachments` / `_prepare_artifact_delivery` — resolve output artifacts from the bound owner's bucket through `app.gateway.path_utils.resolve_outputs_confined_path`, the same outputs-only rule the artifact editor uses, so a sibling `uploads/`/`workspace/` path or a symlink planted in `outputs/` is skipped with a warning The cached value is reused for both the blocking (`runs.wait`) and streaming (`_handle_streaming_chat`) paths, so uploads and artifact delivery always target the same bucket even if a channel returns a rewritten `InboundMessage` from `receive_file`. The bucket id matches the memory bucket resolved by `_resolve_memory_user_id` (both normalize through `make_safe_user_id`). **Configuration** (`config.yaml` -> `channels`): diff --git a/backend/app/channels/manager.py b/backend/app/channels/manager.py index a17da6211..193dcd371 100644 --- a/backend/app/channels/manager.py +++ b/backend/app/channels/manager.py @@ -17,6 +17,7 @@ from typing import Any from urllib.parse import quote import httpx +from fastapi import HTTPException from langgraph_sdk.errors import ConflictError from app.channels import buzz_run_policy as _buzz_run_policy # noqa: F401 @@ -40,6 +41,7 @@ from app.gateway.csrf_middleware import CSRF_COOKIE_NAME, CSRF_HEADER_NAME, gene # ChannelManager construction sees the same policy map as gateway bootstrap. from app.gateway.github import run_policy as _github_run_policy # noqa: F401 from app.gateway.internal_auth import create_internal_auth_headers +from app.gateway.path_utils import resolve_outputs_confined_path from deerflow.config.agents_config import list_custom_agents, load_agent_config from deerflow.config.paths import make_safe_user_id from deerflow.runtime import END_SENTINEL, StreamBridge @@ -725,9 +727,6 @@ def _format_artifact_text(artifacts: list[str]) -> str: return "Created Files: 📎 " + "、".join(filenames) -_OUTPUTS_VIRTUAL_PREFIX = "/mnt/user-data/outputs/" - - def _unknown_command_reply(command: str | None = None) -> str: available = " | ".join(sorted(KNOWN_CHANNEL_COMMANDS)) if command: @@ -848,26 +847,19 @@ def _resolve_attachments(thread_id: str, artifacts: list[str], *, user_id: str | Skips artifacts that cannot be resolved (missing files, invalid paths) and logs warnings for them. """ - from deerflow.config.paths import get_paths - attachments: list[ResolvedAttachment] = [] - paths = get_paths() effective_user_id = user_id or get_effective_user_id() - outputs_dir = paths.sandbox_outputs_dir(thread_id, user_id=effective_user_id).resolve() for virtual_path in artifacts: - # Security: only allow files from the agent outputs directory - if not virtual_path.startswith(_OUTPUTS_VIRTUAL_PREFIX): - logger.warning("[Manager] rejected non-outputs artifact path: %s", virtual_path) + # Security: only files under the agent outputs directory may leave the + # thread. The shared helper rejects sibling ``uploads/``/``workspace/`` + # paths both lexically (``..``) and after symlink resolution, so this + # rule cannot drift from the artifact editor's. + try: + actual = resolve_outputs_confined_path(thread_id, virtual_path, user_id=effective_user_id) + except HTTPException as exc: + logger.warning("[Manager] rejected artifact path outside outputs: %s (%s)", virtual_path, exc.detail) continue try: - actual = paths.resolve_virtual_path(thread_id, virtual_path, user_id=effective_user_id) - # Verify the resolved path is actually under the outputs directory - # (guards against path-traversal even after prefix check) - try: - actual.resolve().relative_to(outputs_dir) - except ValueError: - logger.warning("[Manager] artifact path escapes outputs dir: %s -> %s", virtual_path, actual) - continue if not actual.is_file(): logger.warning("[Manager] artifact not found on disk: %s -> %s", virtual_path, actual) continue diff --git a/backend/app/gateway/AGENTS.md b/backend/app/gateway/AGENTS.md index eb624ac15..23d8bcecb 100644 --- a/backend/app/gateway/AGENTS.md +++ b/backend/app/gateway/AGENTS.md @@ -62,7 +62,7 @@ owner-scoped assistant version selection remains enabled. | **Memory** (`/api/memory`) | `GET /` - memory data; `POST /reload` - force reload; `GET /config` - config; `GET /status` - config + data | | **Uploads** (`/api/threads/{id}/uploads`) | `POST /` - upload files (auto-converts PDF/PPT/Excel/Word); non-mounted sandbox sync uses a non-releasing request lease; `GET /list` - list; `DELETE /{filename}` - delete | | **Threads** (`/api/threads/{id}`) | `DELETE /` - remove DeerFlow-managed local thread data after LangGraph thread deletion; `POST /branches` - branch a completed assistant turn with a replay checkpoint; inherited titles take next-free displayed sibling suffixes, including explicit/renamed ones, while explicit titles stay unchanged. Durable `branch` admission rejects races. Workspace files are not checkpointed, so the branch only best-effort copies the current workspace when branching from the **latest** turn (`workspace_clone_mode="current_thread_best_effort"`); branching from an older/historical turn skips the copy (`workspace_clone_mode="skipped_historical_turn"`) so the branch never inherits files that only exist in a later timeline. Thread-scoped runtime channels (`sandbox`, `thread_data`) are not copied onto the branch: the parent's `sandbox_id` binds path mappings and the release lifecycle to the parent's workspace, so the branch lazily acquires its own sandbox instead. Branch creation also seeds the new thread's run-event feed from the branch checkpoint's visible messages (`history_seed_mode` in the response): the thread feed reads run_events, not checkpoints, so without the seed the inherited history disappears from the UI after the branch's first run (#4380). Seeded rows are grouped into one synthetic run per inherited turn (`branch-seed-{thread_id}-{n}`, a new turn opening at every persisted human message, including an allowlisted hidden `ask_clarification` reply) because `run_id` is a turn identity to the feed's consumers, not a provenance tag: regenerating an inherited answer supersedes that row's whole `run_id` in `GET /messages/page`, so one shared id for the entire seed deleted the complete inherited history on a branch's first regenerate (#4458); `GET /goal`, `PUT /goal`, `DELETE /goal` - read, set, and clear the active thread goal; `POST /compact` - manually summarize older active context into `summary_text` and retain the recent message window, blocked while a run is in flight; unexpected failures are logged server-side and return a generic 500 detail | -| **Artifacts** (`/api/threads/{id}/artifacts`) | `GET /{path}` - stream regular text and binary artifacts with `FileResponse`, including byte-`Range` 206/416 behavior used by bounded text previews and media seeking; active content types (`text/html`, `application/xhtml+xml`, `image/svg+xml`) are always forced as download attachments to reduce XSS risk; `?download=true` still forces download for other file types. `PUT /{path}` atomically replaces an existing UTF-8 text file under `/mnt/user-data/outputs` when its expected SHA-256 still matches; active runs conflict, and non-mounted sandbox providers receive the same update under a request lease. Atomic replacement applies the existing POSIX permission handling when descriptor-based APIs are available and otherwise keeps the platform-native temporary-file permissions (Windows). | +| **Artifacts** (`/api/threads/{id}/artifacts`) | `GET /{path}` - stream regular text and binary artifacts with `FileResponse`, including byte-`Range` 206/416 behavior used by bounded text previews and media seeking; active content types (`text/html`, `application/xhtml+xml`, `image/svg+xml`) are always forced as download attachments to reduce XSS risk; `?download=true` still forces download for other file types. `PUT /{path}` atomically replaces an existing UTF-8 text file under `/mnt/user-data/outputs` when its expected SHA-256 still matches; active runs conflict, and non-mounted sandbox providers receive the same update under a request lease. The outputs-only rule is `path_utils.resolve_outputs_confined_path`, shared with IM-channel attachment delivery: it collapses `..` before the prefix check and re-checks the resolved host path against the resolved outputs root, since `resolve_thread_virtual_path` only confines to `user-data/`; a percent-encoded `..` or a symlink planted in `outputs/` must not reach a sibling `uploads/` file. Atomic replacement applies the existing POSIX permission handling when descriptor-based APIs are available and otherwise keeps the platform-native temporary-file permissions (Windows). | | **Suggestions** (`/api/suggestions`) | `GET /config` - returns global suggestions config boolean; `POST /threads/{id}/suggestions` - generate follow-up questions; rich list/block model content is normalized and inline reasoning (`...`, including unclosed/truncated blocks from reasoning models like MiniMax-M3) is stripped before JSON parsing | | **Input Polish** (`/api/input-polish`) | `POST /` - rewrite a composer draft before it is sent. This is a short authenticated `runs:create` LLM request using `input_polish` config; it does not create a LangGraph run, persist a message, or modify thread state. Shares the non-graph one-shot LLM path (`deerflow.utils.oneshot_llm.run_oneshot_llm`) with the suggestions route so model build + Langfuse metadata + invoke stay in one place; validates the same stripped view of the draft it sends to the model, and preserves literal `` substrings in the rewrite (`strip_think_blocks(truncate_unclosed=False)`) | | **Thread Runs** (`/api/threads/{id}/runs`) | `POST /` - create background run; `POST /stream` - create + SSE stream; `POST /wait` - create + block. Before the first journaled run, seed an empty feed from a checkpoint so legacy checkpoint-only history keeps its order and visibility; skip absent checkpoints or populated feeds. `POST /regenerate/prepare` - prepare clean input + checkpoint metadata for regenerating the latest completed or interrupted assistant answer, carrying the latest non-empty thread title in graph input so resuming an older checkpoint cannot roll back a later manual rename (#4457); `POST /edit-regenerate/prepare` - prepare a checkpoint replay from the latest editable human turn with a replacement user message and edit replay metadata; it carries the current thread title the same way, but only when the replay base already has one — an untitled base belongs to a thread the title middleware has not named yet, so pinning the current title there would keep a name generated from the prompt the edit just replaced; `GET /` - newest 100 runs as an array; `GET /page` - keyset history page `{data, has_more, next_before_created_at, next_before_run_id}`; `GET /{rid}` - run details; `POST /{rid}/cancel` - cancel; `GET /{rid}/join` - join SSE; `GET /{rid}/stream` hides action/wait; GET action 405 pre-owner; POST needs `runs:cancel`; `GET /{rid}/messages` - paginated per-run messages `{data, has_more}`; `GET /{rid}/events` - full event stream; `GET /{rid}/workspace-changes` - workspace/output file change summary and optional diffs; `GET/POST /{rid}/artifacts/archive` - receipt manifest / bounded ZIP; `GET /../messages` - legacy thread message array; `GET /../messages/page` - backward thread-global `seq` history page with middleware/subagent-AI/successful-regenerate/edit-replay filtering and page-run-scoped feedback enrichment; subagent AI callbacks remain available through run events while parent `task` ToolMessages stay visible for card restoration; `GET /../token-usage` - aggregate tokens plus an optional `context_usage` percentage. Context usage approximately counts messages from the latest materialized thread state through `build_thread_checkpoint_state_accessor`, so full and delta checkpoint modes expose the same input. The percentage uses the latest run's model and its `context_window`. | diff --git a/backend/app/gateway/path_utils.py b/backend/app/gateway/path_utils.py index 43f4f9abc..c501e9a76 100644 --- a/backend/app/gateway/path_utils.py +++ b/backend/app/gateway/path_utils.py @@ -1,12 +1,17 @@ """Shared path resolution for thread virtual paths (e.g. mnt/user-data/outputs/...).""" +import posixpath from pathlib import Path from fastapi import HTTPException -from deerflow.config.paths import get_paths +from deerflow.config.paths import VIRTUAL_PATH_PREFIX, get_paths from deerflow.runtime.user_context import get_effective_user_id +OUTPUTS_VIRTUAL_ROOT = f"{VIRTUAL_PATH_PREFIX}/outputs" +_OUTPUTS_PREFIX = OUTPUTS_VIRTUAL_ROOT.lstrip("/") + "/" +_OUTPUTS_ONLY_DETAIL = f"Only files under {OUTPUTS_VIRTUAL_ROOT} are allowed" + 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. @@ -30,3 +35,46 @@ def resolve_thread_virtual_path(thread_id: str, virtual_path: str, user_id: str except ValueError as e: status = 403 if "traversal" in str(e) else 400 raise HTTPException(status_code=status, detail=str(e)) + + +def normalize_outputs_virtual_path(virtual_path: str) -> str: + """Return *virtual_path* as a canonical ``/mnt/user-data/outputs/...`` path. + + ``.``/``..`` segments and duplicate slashes are collapsed *before* the + prefix check, so ``outputs/../uploads/x`` (or its percent-encoded form, + which nginx forwards untouched and Starlette decodes) is rejected as a + non-outputs path instead of slipping past a raw string-prefix test. The + outputs directory itself is not a file and is rejected too. + + Raises: + HTTPException: 400 when the path is not strictly inside outputs. + """ + stripped = posixpath.normpath(virtual_path.lstrip("/")).lstrip("/") + if not stripped.startswith(_OUTPUTS_PREFIX): + raise HTTPException(status_code=400, detail=_OUTPUTS_ONLY_DETAIL) + return f"/{stripped}" + + +def resolve_outputs_confined_path(thread_id: str, virtual_path: str, user_id: str | None = None) -> Path: + """Resolve *virtual_path* and guarantee it lives inside the thread's outputs dir. + + ``resolve_thread_virtual_path`` only confines to ``user-data/``. Callers + that must never touch uploads, workspace, or tool results (the artifact + editor, IM-channel attachment delivery) go through this helper so the + outputs rule lives in one place: the path is normalized lexically first, + then the resolved host path is checked against the resolved outputs root, + which also catches a symlink planted inside ``outputs/``. + + Existence is not checked; callers decide how a missing file surfaces. + + Raises: + HTTPException: 400 when the path is not strictly inside outputs; 403 + when the underlying resolver detects traversal above ``user-data/``. + """ + normalized = normalize_outputs_virtual_path(virtual_path) + resolved_user_id = user_id or get_effective_user_id() + actual_path = resolve_thread_virtual_path(thread_id, normalized, user_id=resolved_user_id) + outputs_root = get_paths().sandbox_outputs_dir(thread_id, user_id=resolved_user_id).resolve() + if actual_path == outputs_root or not actual_path.is_relative_to(outputs_root): + raise HTTPException(status_code=400, detail=_OUTPUTS_ONLY_DETAIL) + return actual_path diff --git a/backend/app/gateway/routers/artifacts.py b/backend/app/gateway/routers/artifacts.py index f304af728..eb19c7cf3 100644 --- a/backend/app/gateway/routers/artifacts.py +++ b/backend/app/gateway/routers/artifacts.py @@ -19,7 +19,7 @@ from pydantic import BaseModel, Field from app.gateway.authz import SandboxRequestLease, require_permission, try_acquire_sandbox_for_request from app.gateway.deps import get_run_manager from app.gateway.internal_auth import get_trusted_internal_owner_user_id -from app.gateway.path_utils import resolve_thread_virtual_path +from app.gateway.path_utils import normalize_outputs_virtual_path, resolve_outputs_confined_path, resolve_thread_virtual_path from deerflow.authz.sandbox_authz import safe_app_config from deerflow.config.paths import make_safe_user_id from deerflow.runtime import ConflictError, ThreadOperationKind @@ -40,7 +40,6 @@ ACTIVE_CONTENT_MIME_TYPES = { MAX_SKILL_ARCHIVE_MEMBER_BYTES = 16 * 1024 * 1024 _SKILL_ARCHIVE_READ_CHUNK_SIZE = 64 * 1024 MAX_EDITABLE_ARTIFACT_BYTES = 2 * 1024 * 1024 -_EDITABLE_OUTPUTS_PREFIX = "mnt/user-data/outputs/" _ARTIFACT_EDIT_TEMP_PREFIX = ".artifact-edit-" @@ -68,12 +67,15 @@ async def reserve_artifact_write(request: Request, thread_id: str, *, user_id: s def _normalize_editable_artifact_path(path: str) -> str: - stripped = path.lstrip("/") - if not stripped.startswith(_EDITABLE_OUTPUTS_PREFIX): - raise HTTPException(status_code=400, detail="Only files in /mnt/user-data/outputs can be edited") - if ".skill/" in stripped or stripped.endswith(".skill"): + # The outputs-only rule is shared with channel attachment delivery: + # ``normalize_outputs_virtual_path`` collapses ``..`` before its prefix check + # and ``resolve_outputs_confined_path`` re-checks the resolved host path, so + # neither an encoded ``..`` nor a symlink planted in ``outputs/`` can + # redirect the edit to a sibling ``user-data/`` directory. + virtual_path = normalize_outputs_virtual_path(path) + if ".skill/" in virtual_path or virtual_path.endswith(".skill"): raise HTTPException(status_code=415, detail="Skill archives cannot be edited in the artifacts panel") - return f"/{stripped}" + return virtual_path def _load_editable_artifact(actual_path: Path, path: str, expected_sha256: str) -> tuple[bytes, os.stat_result]: @@ -516,7 +518,7 @@ async def update_artifact( try: async with reserve_artifact_write(request, thread_id, user_id=effective_user_id): actual_path = await asyncio.to_thread( - resolve_thread_virtual_path, + resolve_outputs_confined_path, thread_id, virtual_path, user_id=effective_user_id, diff --git a/backend/tests/test_artifacts_router.py b/backend/tests/test_artifacts_router.py index a2f8f1b96..8d7987beb 100644 --- a/backend/tests/test_artifacts_router.py +++ b/backend/tests/test_artifacts_router.py @@ -15,7 +15,7 @@ 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 +from deerflow.config.paths import Paths, make_safe_user_id from deerflow.sandbox.lease import get_sandbox_lease_manager ACTIVE_ARTIFACT_CASES = [ @@ -102,7 +102,7 @@ def _artifact_sha256(content: str) -> str: def _patch_artifact_update_dependencies(monkeypatch, artifact_path: Path, provider=None) -> None: - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _thread_id, _path, user_id=None: artifact_path) + monkeypatch.setattr(artifacts_router, "resolve_outputs_confined_path", lambda _thread_id, _path, user_id=None: artifact_path) monkeypatch.setattr(artifacts_router, "reserve_artifact_write", _allow_artifact_write) monkeypatch.setattr(artifacts_router, "get_sandbox_provider", lambda: provider or _MountedSandboxProvider()) @@ -193,6 +193,107 @@ def test_update_artifact_rejects_non_output_path(tmp_path, monkeypatch) -> None: assert artifact_path.read_text(encoding="utf-8") == "before" +_REAL_PATHS_THREAD_ID = "thread-1" +_REAL_PATHS_USER_ID = "user-1" + + +def _patch_real_thread_paths(monkeypatch, tmp_path: Path, provider=None) -> tuple[Path, Path]: + """Route ``update_artifact`` through the real virtual-path resolver rooted at *tmp_path*. + + The other update tests stub ``resolve_outputs_confined_path`` so they never + exercise the outputs confinement; these tests need the real thread layout. + Returns the thread's ``outputs`` and ``uploads`` host directories. + """ + paths = Paths(tmp_path) + monkeypatch.setattr("app.gateway.path_utils.get_paths", lambda: paths) + monkeypatch.setattr(artifacts_router, "get_effective_user_id", lambda: _REAL_PATHS_USER_ID) + monkeypatch.setattr(artifacts_router, "get_trusted_internal_owner_user_id", lambda _request: None) + monkeypatch.setattr(artifacts_router, "reserve_artifact_write", _allow_artifact_write) + monkeypatch.setattr(artifacts_router, "get_sandbox_provider", lambda: provider or _MountedSandboxProvider()) + + outputs = paths.sandbox_outputs_dir(_REAL_PATHS_THREAD_ID, user_id=_REAL_PATHS_USER_ID) + uploads = paths.sandbox_uploads_dir(_REAL_PATHS_THREAD_ID, user_id=_REAL_PATHS_USER_ID) + outputs.mkdir(parents=True) + uploads.mkdir(parents=True) + return outputs, uploads + + +def _update_artifact_via_handler(path: str, *, current: str, content: str): + return asyncio.run( + call_unwrapped( + artifacts_router.update_artifact, + _REAL_PATHS_THREAD_ID, + path, + artifacts_router.ArtifactUpdateRequest(content=content, expected_sha256=_artifact_sha256(current)), + _make_request(), + ) + ) + + +def test_update_artifact_rejects_dot_dot_escape_from_outputs(tmp_path, monkeypatch) -> None: + # The outputs-only guard used to be a string-prefix check on the raw path, + # so ``outputs/../uploads/...`` passed it and the resolver only confines to + # ``user-data/`` — letting PUT overwrite a sibling upload. + _, uploads = _patch_real_thread_paths(monkeypatch, tmp_path) + victim = uploads / "victim.txt" + victim.write_text("before", encoding="utf-8") + + with pytest.raises(HTTPException) as exc_info: + _update_artifact_via_handler("mnt/user-data/outputs/../uploads/victim.txt", current="before", content="after") + + assert exc_info.value.status_code == 400 + assert victim.read_text(encoding="utf-8") == "before" + + +def test_update_artifact_rejects_percent_encoded_dot_dot_over_http(tmp_path, monkeypatch) -> None: + # Browsers and HTTP clients collapse a literal ``..`` before sending, but + # ``%2e%2e`` reaches the route intact and Starlette decodes it to ``..``. + _, uploads = _patch_real_thread_paths(monkeypatch, tmp_path) + victim = uploads / "victim.txt" + victim.write_text("before", encoding="utf-8") + + app = make_authed_test_app() + app.include_router(artifacts_router.router) + with TestClient(app) as client: + response = client.put( + f"/api/threads/{_REAL_PATHS_THREAD_ID}/artifacts/mnt/user-data/outputs/%2e%2e/uploads/victim.txt", + json={"content": "after", "expected_sha256": _artifact_sha256("before")}, + ) + + assert response.status_code == 400 + assert victim.read_text(encoding="utf-8") == "before" + + +def test_update_artifact_rejects_symlink_escaping_outputs(tmp_path, monkeypatch) -> None: + outputs, uploads = _patch_real_thread_paths(monkeypatch, tmp_path) + victim = uploads / "victim.txt" + victim.write_text("before", encoding="utf-8") + link = outputs / "linked.txt" + try: + link.symlink_to(victim) + except OSError: + pytest.skip("symlinks are unavailable on this platform") + + with pytest.raises(HTTPException) as exc_info: + _update_artifact_via_handler("mnt/user-data/outputs/linked.txt", current="before", content="after") + + assert exc_info.value.status_code == 400 + assert victim.read_text(encoding="utf-8") == "before" + + +def test_update_artifact_normalizes_dot_segments_before_syncing(tmp_path, monkeypatch) -> None: + provider = _RemoteSandboxProvider() + outputs, _ = _patch_real_thread_paths(monkeypatch, tmp_path, provider=provider) + artifact_path = outputs / "note.txt" + artifact_path.write_text("before", encoding="utf-8") + + response = _update_artifact_via_handler("mnt/user-data/outputs/./nested/../note.txt", current="before", content="after") + + assert artifact_path.read_text(encoding="utf-8") == "after" + assert response.path == "/mnt/user-data/outputs/note.txt" + assert provider.sandbox.updates == [("/mnt/user-data/outputs/note.txt", b"after")] + + def test_update_artifact_rejects_binary_file(tmp_path, monkeypatch) -> None: artifact_path = tmp_path / "blob.bin" artifact_path.write_bytes(b"before\x00binary") diff --git a/backend/tests/test_channel_file_attachments.py b/backend/tests/test_channel_file_attachments.py index 0b8e27496..cce5983c1 100644 --- a/backend/tests/test_channel_file_attachments.py +++ b/backend/tests/test_channel_file_attachments.py @@ -7,6 +7,8 @@ import os from pathlib import Path from unittest.mock import MagicMock, patch +import pytest + from app.channels.base import Channel from app.channels.message_bus import InboundMessage, MessageBus, OutboundMessage, ResolvedAttachment @@ -116,7 +118,7 @@ class TestResolveAttachments: mock_paths.resolve_virtual_path.return_value = test_file mock_paths.sandbox_outputs_dir.return_value = outputs_dir - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments(thread_id, ["/mnt/user-data/outputs/report.pdf"]) assert len(result) == 1 @@ -139,7 +141,7 @@ class TestResolveAttachments: mock_paths.resolve_virtual_path.return_value = img mock_paths.sandbox_outputs_dir.return_value = outputs_dir - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments(thread_id, ["/mnt/user-data/outputs/chart.png"]) assert len(result) == 1 @@ -157,7 +159,7 @@ class TestResolveAttachments: mock_paths.resolve_virtual_path.return_value = outputs_dir / "nonexistent.txt" mock_paths.sandbox_outputs_dir.return_value = outputs_dir - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments("t1", ["/mnt/user-data/outputs/nonexistent.txt"]) assert result == [] @@ -169,7 +171,7 @@ class TestResolveAttachments: mock_paths = MagicMock() mock_paths.resolve_virtual_path.side_effect = ValueError("bad path") - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments("t1", ["/invalid/path"]) assert result == [] @@ -180,7 +182,7 @@ class TestResolveAttachments: mock_paths = MagicMock() - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments("t1", ["/mnt/user-data/uploads/secret.pdf"]) assert result == [] @@ -192,7 +194,7 @@ class TestResolveAttachments: mock_paths = MagicMock() - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments("t1", ["/mnt/user-data/workspace/config.py"]) assert result == [] @@ -214,11 +216,37 @@ class TestResolveAttachments: mock_paths.resolve_virtual_path.return_value = escaped_file mock_paths.sandbox_outputs_dir.return_value = outputs_dir - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments(thread_id, ["/mnt/user-data/outputs/../uploads/stolen.txt"]) assert result == [] + def test_rejects_symlink_planted_in_outputs(self, tmp_path): + """A symlink inside outputs/ pointing at a sibling upload is skipped. + + Uses the real ``Paths`` layout so the shared outputs-confinement helper + (also used by the artifact editor) is exercised end to end. + """ + from app.channels.manager import _resolve_attachments + from deerflow.config.paths import Paths + + paths = Paths(tmp_path) + outputs_dir = paths.sandbox_outputs_dir("t1", user_id="owner-1") + uploads_dir = paths.sandbox_uploads_dir("t1", user_id="owner-1") + outputs_dir.mkdir(parents=True) + uploads_dir.mkdir(parents=True) + victim = uploads_dir / "secret.pdf" + victim.write_bytes(b"%PDF-1.4 secret") + try: + (outputs_dir / "report.pdf").symlink_to(victim) + except OSError: + pytest.skip("symlinks are unavailable on this platform") + + with patch("app.gateway.path_utils.get_paths", return_value=paths): + result = _resolve_attachments("t1", ["/mnt/user-data/outputs/report.pdf"], user_id="owner-1") + + assert result == [] + def test_multiple_artifacts_partial_resolution(self, tmp_path): """Mixed valid/invalid artifacts: only valid ones are returned.""" from app.channels.manager import _resolve_attachments @@ -239,7 +267,7 @@ class TestResolveAttachments: mock_paths.resolve_virtual_path.side_effect = resolve_side_effect - with patch("deerflow.config.paths.get_paths", return_value=mock_paths): + with patch("app.gateway.path_utils.get_paths", return_value=mock_paths): result = _resolve_attachments( thread_id, ["/mnt/user-data/outputs/data.csv", "/mnt/user-data/outputs/missing.txt"], diff --git a/backend/tests/test_channels.py b/backend/tests/test_channels.py index 971089b90..7611a6fc5 100644 --- a/backend/tests/test_channels.py +++ b/backend/tests/test_channels.py @@ -5707,6 +5707,9 @@ class TestHandleChatWithArtifacts: paths = Paths(tmp_path) monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: paths) + # Attachment resolution goes through the shared outputs-confinement + # helper, which binds ``get_paths`` at import like the other consumers. + monkeypatch.setattr("app.gateway.path_utils.get_paths", lambda: paths) outputs_dir = paths.sandbox_outputs_dir("test-thread-123", user_id="owner-1") outputs_dir.mkdir(parents=True) (outputs_dir / "report.md").write_text("owner report", encoding="utf-8") diff --git a/backend/tests/test_gateway_path_utils.py b/backend/tests/test_gateway_path_utils.py new file mode 100644 index 000000000..146ca8b76 --- /dev/null +++ b/backend/tests/test_gateway_path_utils.py @@ -0,0 +1,145 @@ +"""Tests for the shared outputs-confinement helpers in ``app.gateway.path_utils``. + +The artifact editor (``PUT /artifacts``) and IM-channel attachment delivery +both must never touch anything outside ``/mnt/user-data/outputs``. The rule +used to be re-implemented per caller; these tests pin the single shared +implementation so the copies cannot drift again. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from fastapi import HTTPException + +from app.gateway.path_utils import OUTPUTS_VIRTUAL_ROOT, normalize_outputs_virtual_path, resolve_outputs_confined_path +from deerflow.config.paths import Paths + +THREAD_ID = "thread-1" +USER_ID = "user-1" + + +@pytest.fixture +def thread_dirs(tmp_path, monkeypatch) -> tuple[Path, Path]: + """Point the global ``Paths`` at *tmp_path* and return (outputs, uploads).""" + paths = Paths(tmp_path) + monkeypatch.setattr("app.gateway.path_utils.get_paths", lambda: paths) + outputs = paths.sandbox_outputs_dir(THREAD_ID, user_id=USER_ID) + uploads = paths.sandbox_uploads_dir(THREAD_ID, user_id=USER_ID) + outputs.mkdir(parents=True) + uploads.mkdir(parents=True) + return outputs, uploads + + +def _symlink_or_skip(link: Path, target: Path) -> None: + try: + link.symlink_to(target) + except OSError: + pytest.skip("symlinks are unavailable on this platform") + + +class TestNormalizeOutputsVirtualPath: + def test_returns_canonical_absolute_virtual_path(self) -> None: + assert normalize_outputs_virtual_path("mnt/user-data/outputs/report.md") == "/mnt/user-data/outputs/report.md" + assert normalize_outputs_virtual_path("//mnt/user-data/outputs//a/./b.txt") == "/mnt/user-data/outputs/a/b.txt" + + def test_collapses_dot_dot_that_stays_inside_outputs(self) -> None: + assert normalize_outputs_virtual_path("/mnt/user-data/outputs/nested/../note.txt") == "/mnt/user-data/outputs/note.txt" + + @pytest.mark.parametrize( + "virtual_path", + [ + "/mnt/user-data/uploads/secret.pdf", + "/mnt/user-data/workspace/config.py", + "/mnt/user-data/outputs/../uploads/secret.pdf", + "/mnt/user-data/outputsX/file.txt", + "/mnt/user-data/outputs", + "/mnt/user-data/outputs/", + "/invalid/path", + "", + ], + ) + def test_rejects_paths_outside_outputs(self, virtual_path: str) -> None: + with pytest.raises(HTTPException) as exc_info: + normalize_outputs_virtual_path(virtual_path) + assert exc_info.value.status_code == 400 + + def test_root_constant_matches_prefix(self) -> None: + assert OUTPUTS_VIRTUAL_ROOT == "/mnt/user-data/outputs" + + +class TestResolveOutputsConfinedPath: + def test_resolves_regular_file_under_outputs(self, thread_dirs) -> None: + outputs, _ = thread_dirs + target = outputs / "report.md" + target.write_text("hello", encoding="utf-8") + + resolved = resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/report.md", user_id=USER_ID) + + assert resolved == target.resolve() + + def test_does_not_require_the_file_to_exist(self, thread_dirs) -> None: + # Existence is the caller's decision (the editor 404s, channels warn). + outputs, _ = thread_dirs + + resolved = resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/missing.txt", user_id=USER_ID) + + assert resolved == (outputs / "missing.txt").resolve() + + def test_rejects_dot_dot_escape_lexically(self, thread_dirs) -> None: + _, uploads = thread_dirs + (uploads / "victim.txt").write_text("before", encoding="utf-8") + + with pytest.raises(HTTPException) as exc_info: + resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/../uploads/victim.txt", user_id=USER_ID) + + assert exc_info.value.status_code == 400 + + def test_rejects_symlink_pointing_at_sibling_directory(self, thread_dirs) -> None: + outputs, uploads = thread_dirs + victim = uploads / "victim.txt" + victim.write_text("before", encoding="utf-8") + _symlink_or_skip(outputs / "linked.txt", victim) + + with pytest.raises(HTTPException) as exc_info: + resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/linked.txt", user_id=USER_ID) + + assert exc_info.value.status_code == 400 + + def test_symlink_escaping_user_data_is_a_traversal(self, thread_dirs, tmp_path) -> None: + # Above ``user-data/`` the underlying resolver already refuses; that 403 + # must not be downgraded by the outputs check layered on top. + outputs, _ = thread_dirs + outside = tmp_path / "outside.txt" + outside.write_text("outside", encoding="utf-8") + _symlink_or_skip(outputs / "linked.txt", outside) + + with pytest.raises(HTTPException) as exc_info: + resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/linked.txt", user_id=USER_ID) + + assert exc_info.value.status_code == 403 + + def test_symlinked_outputs_root_outside_user_data_is_a_traversal(self, thread_dirs, tmp_path) -> None: + # An outputs root re-pointed outside ``user-data/`` is refused by the + # underlying resolver (matching ``artifact_archive``), not accepted + # because both sides happen to resolve consistently. + outputs, _ = thread_dirs + real_outputs = tmp_path / "real-outputs" + real_outputs.mkdir() + (real_outputs / "report.md").write_text("hello", encoding="utf-8") + outputs.rmdir() + _symlink_or_skip(outputs, real_outputs) + + with pytest.raises(HTTPException) as exc_info: + resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/report.md", user_id=USER_ID) + + assert exc_info.value.status_code == 403 + + def test_defaults_to_effective_user(self, thread_dirs, monkeypatch) -> None: + outputs, _ = thread_dirs + monkeypatch.setattr("app.gateway.path_utils.get_effective_user_id", lambda: USER_ID) + + resolved = resolve_outputs_confined_path(THREAD_ID, "/mnt/user-data/outputs/report.md") + + assert resolved == (outputs / "report.md").resolve() diff --git a/backend/tests/test_sandbox_authorization.py b/backend/tests/test_sandbox_authorization.py index dad3cf9a1..0f2f398b9 100644 --- a/backend/tests/test_sandbox_authorization.py +++ b/backend/tests/test_sandbox_authorization.py @@ -561,7 +561,7 @@ def test_artifact_sandbox_sync_skipped_when_denied(monkeypatch, tmp_path): sandbox_provider.uses_thread_data_mounts = False sandbox_provider.acquire_async = AsyncMock(side_effect=AssertionError("must not acquire")) monkeypatch.setattr(artifacts_router, "get_sandbox_provider", lambda: sandbox_provider) - monkeypatch.setattr(artifacts_router, "resolve_thread_virtual_path", lambda _t, _p, user_id=None: tmp_path / "note.txt") + monkeypatch.setattr(artifacts_router, "resolve_outputs_confined_path", lambda _t, _p, user_id=None: tmp_path / "note.txt") from contextlib import asynccontextmanager