mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-08-01 10:56:02 +00:00
fix(artifacts): honor trusted owner-user-id header (#3982)
* fix(artifacts): honor trusted owner-user-id header The artifact endpoint resolved paths only via the effective user, so a trusted internal caller acting on behalf of an owner (carrying X-DeerFlow-Owner-User-Id) read the synthetic internal user's storage and 404'd on files the owner's run had written. Resolve the owner via get_trusted_internal_owner_user_id and pass it through resolve_thread_virtual_path (now accepting an optional user_id), matching the memory and threads routers. Browser/API callers send no such header and fall back to the effective user, so their behavior is unchanged. * fix(artifacts): normalize trusted owner id through make_safe_user_id The owner-user-id header carries the raw platform owner id, while runs store files under the make_safe_user_id bucket. Resolve artifacts with the normalized id, mirroring the memory router.
This commit is contained in:
parent
2839a3632e
commit
8ade23d779
@ -8,13 +8,16 @@ from deerflow.config.paths import get_paths
|
|||||||
from deerflow.runtime.user_context import get_effective_user_id
|
from deerflow.runtime.user_context import get_effective_user_id
|
||||||
|
|
||||||
|
|
||||||
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.
|
"""Resolve a virtual path to the actual filesystem path under thread user-data.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
thread_id: The thread ID.
|
thread_id: The thread ID.
|
||||||
virtual_path: The virtual path as seen inside the sandbox
|
virtual_path: The virtual path as seen inside the sandbox
|
||||||
(e.g., /mnt/user-data/outputs/file.txt).
|
(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:
|
Returns:
|
||||||
The resolved filesystem path.
|
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.
|
HTTPException: If the path is invalid or outside allowed directories.
|
||||||
"""
|
"""
|
||||||
try:
|
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:
|
except ValueError as e:
|
||||||
status = 403 if "traversal" in str(e) else 400
|
status = 403 if "traversal" in str(e) else 400
|
||||||
raise HTTPException(status_code=status, detail=str(e))
|
raise HTTPException(status_code=status, detail=str(e))
|
||||||
|
|||||||
@ -9,7 +9,9 @@ from fastapi import APIRouter, HTTPException, Request
|
|||||||
from fastapi.responses import FileResponse, PlainTextResponse, Response
|
from fastapi.responses import FileResponse, PlainTextResponse, Response
|
||||||
|
|
||||||
from app.gateway.authz import require_permission
|
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 app.gateway.path_utils import resolve_thread_virtual_path
|
||||||
|
from deerflow.config.paths import make_safe_user_id
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
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`
|
- 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
|
- 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)
|
# Check if this is a request for a file inside a .skill archive (e.g., xxx.skill/SKILL.md)
|
||||||
if ".skill/" in path:
|
if ".skill/" in path:
|
||||||
# Split the path at ".skill/" to get the ZIP file path and internal 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"
|
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"
|
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).
|
# 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)
|
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:
|
except UnicodeDecodeError:
|
||||||
return Response(content=content, media_type=mime_type or "application/octet-stream", headers=cache_headers)
|
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}")
|
logger.info(f"Resolving artifact path: thread_id={thread_id}, requested_path={path}, actual_path={actual_path}")
|
||||||
|
|
||||||
|
|||||||
@ -1,6 +1,7 @@
|
|||||||
import asyncio
|
import asyncio
|
||||||
import zipfile
|
import zipfile
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
from _router_auth_helpers import call_unwrapped, make_authed_test_app
|
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
|
from starlette.responses import FileResponse
|
||||||
|
|
||||||
import app.gateway.routers.artifacts as artifacts_router
|
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 = [
|
ACTIVE_ARTIFACT_CASES = [
|
||||||
("poc.html", "<html><body><script>alert('xss')</script></body></html>"),
|
("poc.html", "<html><body><script>alert('xss')</script></body></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)
|
return original_read_text(self, *args, **kwargs)
|
||||||
|
|
||||||
monkeypatch.setattr(Path, "read_text", read_text_with_gbk_default)
|
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()
|
request = _make_request()
|
||||||
response = asyncio.run(call_unwrapped(artifacts_router.get_artifact, "thread-1", "mnt/user-data/outputs/note.txt", 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 = tmp_path / filename
|
||||||
artifact_path.write_text(content, encoding="utf-8")
|
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()))
|
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:
|
with zipfile.ZipFile(skill_path, "w") as zip_ref:
|
||||||
zip_ref.writestr(filename, content)
|
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()))
|
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 = tmp_path / "note.txt"
|
||||||
artifact_path.write_text("hello", encoding="utf-8")
|
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 = make_authed_test_app()
|
||||||
app.include_router(artifacts_router.router)
|
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:
|
with zipfile.ZipFile(skill_path, "w") as zip_ref:
|
||||||
zip_ref.writestr("notes.txt", "hello")
|
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 = make_authed_test_app()
|
||||||
app.include_router(artifacts_router.router)
|
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;")
|
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("<html>", 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:
|
def test_skill_archive_preview_rejects_oversized_member_before_decompression(tmp_path) -> None:
|
||||||
skill_path = tmp_path / "sample.skill"
|
skill_path = tmp_path / "sample.skill"
|
||||||
payload = b"A" * (artifacts_router.MAX_SKILL_ARCHIVE_MEMBER_BYTES + 1)
|
payload = b"A" * (artifacts_router.MAX_SKILL_ARCHIVE_MEMBER_BYTES + 1)
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user