mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-19 19:16:17 +00:00
fix(uploads): delete the requested upload, not a symlink's target (#5547)
* fix(uploads): delete the requested upload, not a symlink's target delete_file_safe resolved the requested path before unlinking it. The uploads directory is writable from local and AIO sandboxes, so a symlink planted under an upload name was followed: deleting alias.pdf removed the victim.pdf it pointed to, and the companion cleanup then removed victim.md, while the link itself survived and the call reported "Deleted alias.pdf". A link resolving outside the directory was already refused by the traversal check, so the damage stayed inside the thread's uploads. The function now checks and unlinks the requested entry itself and treats a symlink as not found, the same way list_files_in_dir already hides it. unlink() never follows the final component, so a file swapped for a link between the check and the unlink removes only the link. Tests cover the helper, the Gateway DELETE route, and DeerFlowClient.delete_upload. * docs(changelog): note upload delete symlink fix (#5547)
This commit is contained in:
parent
3776f6f5ec
commit
f9f3127dc1
@ -2772,6 +2772,13 @@ This release closes that milestone with **765 merged pull requests**.
|
||||
|
||||
### Security
|
||||
|
||||
- **uploads:** Deleting an upload no longer follows a symlink to delete a
|
||||
different file. A symlink planted in the sandbox-writable uploads directory
|
||||
made `DELETE /api/threads/{id}/uploads/{filename}` (and
|
||||
`DeerFlowClient.delete_upload`) remove the upload it pointed to, plus that
|
||||
file's companion `.md`, while reporting the requested name as deleted.
|
||||
Symlinks now return 404, matching the upload listing; links that leave the
|
||||
uploads directory are still rejected with 400. ([#5547])
|
||||
- **frontend:** Tool steps no longer turn non-web URLs into links. The
|
||||
`web_fetch` URL and `web_search` / `image_search` result links in the
|
||||
chain-of-thought panel skipped the scheme allowlist that markdown links use,
|
||||
@ -4292,3 +4299,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag.
|
||||
[#5505]: https://github.com/bytedance/deer-flow/pull/5505
|
||||
[#5524]: https://github.com/bytedance/deer-flow/pull/5524
|
||||
[#5526]: https://github.com/bytedance/deer-flow/pull/5526
|
||||
[#5547]: https://github.com/bytedance/deer-flow/pull/5547
|
||||
|
||||
@ -2085,6 +2085,11 @@
|
||||
|
||||
### 安全
|
||||
|
||||
- **上传:** 删除上传文件时不再跟随符号链接删除另一个文件。沙箱可写的 uploads 目录中若被
|
||||
放置符号链接,`DELETE /api/threads/{id}/uploads/{filename}`(以及
|
||||
`DeerFlowClient.delete_upload`)此前会删除链接指向的上传文件及其配套 `.md`,却仍报告
|
||||
删除的是请求的文件名。现在符号链接返回 404,与上传列表一致;指向 uploads 目录之外的
|
||||
链接仍以 400 拒绝。([#5547])
|
||||
- **前端:** 工具步骤不再把非 Web URL 渲染为链接。思维链面板中的 `web_fetch` URL 与
|
||||
`web_search` / `image_search` 结果链接此前绕过了 Markdown 链接使用的协议白名单,
|
||||
被提示注入的工具调用可在聊天中放入 `file:` 或系统协议处理程序链接(`ms-msdt:`、
|
||||
@ -3499,3 +3504,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写,核心包含子
|
||||
[#5505]: https://github.com/bytedance/deer-flow/pull/5505
|
||||
[#5524]: https://github.com/bytedance/deer-flow/pull/5524
|
||||
[#5526]: https://github.com/bytedance/deer-flow/pull/5526
|
||||
[#5547]: https://github.com/bytedance/deer-flow/pull/5547
|
||||
|
||||
@ -89,7 +89,7 @@ owner-scoped assistant version selection remains enabled.
|
||||
| **Subagents** (`/api/subagents`) | Admin managed-worker CRUD and listing. |
|
||||
| **Integrations** (`/api/integrations`) | `GET /lark/status` - inspect managed Lark/Feishu CLI integration state, including `sandbox_runtime_mode` / `sandbox_runtime_ready` (whether `lark-cli` will actually be present in the sandbox at chat time); `POST /lark/install` - admin-only install of the official `lark-*` managed skill pack; `POST /lark/config/start` and `/lark/config/complete` - internal first-time Lark connection setup; `POST /lark/config/credentials` - atomically switch the caller's per-user Lark app after validating the new `app_id`/`app_secret` through the official CLI's live tenant-token probe, revoke/remove the previous OAuth tokens, and restore the prior credential tree if the switch fails; `POST /lark/auth/start` and `/lark/auth/complete` - browser device-flow user authorization without terminal access, with optional `domains` / exact `scope` for incremental permission grants. Config and auth flows carry a server-issued, per-user generation persisted under the credential lock; a rejected direct switch leaves the current generation unchanged, stale completions return 409, and browser re-registration uses the same token-clearing/revocation transaction as direct credential switches. |
|
||||
| **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 |
|
||||
| **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 a regular file; a symlink is never followed and 404s like `GET /list` |
|
||||
| **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` - summarize older active context, deriving memory policy and bucket from the state-producing checkpoint rather than request `agent_name`, and block while a run is in flight; unexpected failures 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 (`text/html`, `text/xml`, `application/xml`, `text/xsl`, any `+xml` type such as XHTML/SVG; `.skill` members too) is always forced as a download attachment 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 |
|
||||
|
||||
@ -235,6 +235,7 @@ backend/.deer-flow/threads/
|
||||
|
||||
- 最大文件大小:100MB(可在 nginx.conf 中配置 `client_max_body_size`)
|
||||
- 文件名安全性:系统会自动验证文件路径,防止目录遍历攻击
|
||||
- 删除只作用于普通文件:上传目录中的符号链接不会被跟随,删除请求按文件不存在(404)处理
|
||||
- 线程隔离:每个线程的上传文件相互隔离,无法跨线程访问
|
||||
- 自动文档转换默认关闭;如需启用,需在 `config.yaml` 中显式设置 `uploads.auto_convert_documents: true`
|
||||
|
||||
|
||||
@ -324,6 +324,11 @@ def delete_file_safe(base_dir: Path, filename: str, *, convertible_extensions: s
|
||||
If *convertible_extensions* is provided and the file's extension matches,
|
||||
the companion ``.md`` file is also removed (if it exists).
|
||||
|
||||
Only regular files are deleted. Upload directories may be mounted into
|
||||
local sandboxes, so a sandbox process can plant a symlink under an upload
|
||||
name; following it would delete the upload it aliases instead. Such
|
||||
entries are reported as not found, matching ``list_files_in_dir``.
|
||||
|
||||
Args:
|
||||
base_dir: Directory containing the file.
|
||||
filename: Name of file to delete.
|
||||
@ -337,10 +342,10 @@ def delete_file_safe(base_dir: Path, filename: str, *, convertible_extensions: s
|
||||
FileNotFoundError: If the file does not exist.
|
||||
PathTraversalError: If path traversal is detected.
|
||||
"""
|
||||
file_path = (base_dir / filename).resolve()
|
||||
file_path = base_dir / filename
|
||||
validate_path_traversal(file_path, base_dir)
|
||||
|
||||
if not file_path.is_file():
|
||||
if file_path.is_symlink() or not file_path.is_file():
|
||||
raise FileNotFoundError(f"File not found: {filename}")
|
||||
|
||||
file_path.unlink()
|
||||
|
||||
@ -4341,6 +4341,30 @@ class TestUploadDeleteSymlink:
|
||||
# The outside file must NOT have been deleted.
|
||||
assert outside.exists()
|
||||
|
||||
def test_delete_upload_symlink_to_sibling_upload(self, client):
|
||||
"""A symlink aliasing another upload is not followed to delete that upload."""
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
uploads_dir = Path(tmp) / "uploads"
|
||||
uploads_dir.mkdir()
|
||||
|
||||
victim = uploads_dir / "victim.txt"
|
||||
victim.write_text("keep me")
|
||||
|
||||
link = uploads_dir / "alias.txt"
|
||||
try:
|
||||
link.symlink_to(victim.name)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("symlink creation requires Developer Mode or elevated privileges on Windows")
|
||||
raise
|
||||
|
||||
with patch("deerflow.client.get_uploads_dir", return_value=uploads_dir):
|
||||
with pytest.raises(FileNotFoundError):
|
||||
client.delete_upload("thread-1", "alias.txt")
|
||||
|
||||
assert victim.read_text() == "keep me"
|
||||
assert link.is_symlink()
|
||||
|
||||
def test_upload_filename_with_spaces_and_unicode(self, client):
|
||||
"""Files with spaces and unicode characters in names upload correctly."""
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
|
||||
@ -286,3 +286,24 @@ class TestDeleteFileSafe:
|
||||
def test_delete_traversal_raises(self, tmp_path):
|
||||
with pytest.raises(PathTraversalError, match="traversal"):
|
||||
delete_file_safe(tmp_path, "../outside.txt")
|
||||
|
||||
def test_delete_symlink_to_sibling_upload_keeps_target(self, tmp_path):
|
||||
"""A symlink planted in the uploads dir must not delete the upload it aliases."""
|
||||
victim = tmp_path / "victim.pdf"
|
||||
victim.write_bytes(b"pdf-bytes")
|
||||
companion = tmp_path / "victim.md"
|
||||
companion.write_text("converted", encoding="utf-8")
|
||||
alias = tmp_path / "alias.pdf"
|
||||
try:
|
||||
alias.symlink_to(victim.name)
|
||||
except OSError as exc:
|
||||
if getattr(exc, "winerror", None) == 1314:
|
||||
pytest.skip("symlink creation requires Developer Mode or elevated privileges on Windows")
|
||||
raise
|
||||
|
||||
with pytest.raises(FileNotFoundError):
|
||||
delete_file_safe(tmp_path, "alias.pdf", convertible_extensions={".pdf"})
|
||||
|
||||
assert victim.read_bytes() == b"pdf-bytes"
|
||||
assert companion.exists()
|
||||
assert alias.is_symlink()
|
||||
|
||||
@ -949,6 +949,24 @@ def test_delete_uploaded_file_removes_generated_markdown_companion(tmp_path):
|
||||
assert not (thread_uploads_dir / "report.md").exists()
|
||||
|
||||
|
||||
def test_delete_uploaded_file_rejects_symlink_to_sibling_upload(tmp_path):
|
||||
thread_uploads_dir = tmp_path / "uploads"
|
||||
thread_uploads_dir.mkdir(parents=True)
|
||||
victim = thread_uploads_dir / "victim.pdf"
|
||||
victim.write_bytes(b"pdf-bytes")
|
||||
(thread_uploads_dir / "victim.md").write_text("converted", encoding="utf-8")
|
||||
_symlink_to_or_skip(thread_uploads_dir / "alias.pdf", victim)
|
||||
|
||||
with patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
asyncio.run(call_unwrapped(uploads.delete_uploaded_file, "thread-local", "alias.pdf", request=MagicMock()))
|
||||
|
||||
assert exc_info.value.status_code == 404
|
||||
assert victim.read_bytes() == b"pdf-bytes"
|
||||
assert (thread_uploads_dir / "victim.md").exists()
|
||||
assert (thread_uploads_dir / "alias.pdf").is_symlink()
|
||||
|
||||
|
||||
def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors():
|
||||
class BrokenConfig:
|
||||
def __getattribute__(self, name):
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user