mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-10 14:08:52 +00:00
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.
This commit is contained in:
parent
3e1349576e
commit
8e86729aa0
11
CHANGELOG.md
11
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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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`):
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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 (`<think>...</think>`, 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 `<think>` 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`. |
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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")
|
||||
|
||||
@ -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"],
|
||||
|
||||
@ -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")
|
||||
|
||||
145
backend/tests/test_gateway_path_utils.py
Normal file
145
backend/tests/test_gateway_path_utils.py
Normal file
@ -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()
|
||||
@ -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
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user