From 3c08a9f93e7b4abed7a3f5d26ec018958df839c3 Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Sun, 18 Oct 2026 08:48:06 +0900 Subject: [PATCH] fix(sandbox): let the local ls tool list the /mnt/user-data root (#6991) * fix(sandbox): let local file tools read the /mnt/user-data root The local provider maps the aggregate /mnt/user-data root so listing it matches AIO, but the tool layer only accepted paths below it: ls, glob and grep on the root answered "Permission denied" (with a trailing slash, from the resolved-path "path traversal detected" check). validate_local_tool_path now accepts the bare root for reads, and the user-data resolver accepts the shared parent of workspace/uploads/ outputs itself when the caller opts in. Only ls and the shared read path opt in; writes to the root and files placed directly in it stay rejected. * docs(changelog): reference #6991 for the /mnt/user-data root fix * fix(sandbox): list the /mnt/user-data root through its allowed dirs only The first version let glob, grep and ls start at the user-data root after validating only the root itself. The walkers then descended into everything under the host user-data/ directory, so a file or directory stored directly under it was listed and searched although read_file rejects it. ls now lists the root one allowed directory at a time (workspace, uploads and outputs, each one level deep) through the sandbox's own list_dir, so the host root is never walked and list_dir's existing per-directory containment check still filters symlinks. The resolver is back to main's behavior, so glob, grep and read_file keep rejecting the root. --- CHANGELOG.md | 9 +++ CHANGELOG_zh.md | 7 ++ .../harness/deerflow/sandbox/AGENTS.md | 2 +- .../harness/deerflow/sandbox/tools.py | 37 +++++++++-- ...est_local_sandbox_virtual_path_contract.py | 37 +++++++++++ backend/tests/test_sandbox_search_tools.py | 65 +++++++++++++++++++ backend/tests/test_sandbox_tools_security.py | 40 ++++++++++++ 7 files changed, 192 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f34a2adbb..caa6786652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5982,6 +5982,14 @@ This release closes that milestone with **621 merged pull requests**. names the same file as the equivalent relative path, unless the command rebinds `PWD` itself. Variables inside URLs are unaffected. The guard remains best-effort and is not an isolation boundary. ([#6930]) +- **sandbox:** On the local sandbox, `ls` now accepts the `/mnt/user-data` + root itself and lists `workspace`, `uploads` and `outputs` as it does on + AIO. The provider maps that root on purpose, but the tool layer allowed + only paths below it and answered `Permission denied` (with a trailing + slash, from an internal "path traversal detected" check). The listing + covers only those three directories, so anything else stored directly + under the root stays hidden. `glob`, `grep`, `read_file` and writes still + reject the root, because a search would read every file below it. ([#6991]) ### Security @@ -11778,3 +11786,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6897]: https://github.com/bytedance/deer-flow/pull/6897 [#6909]: https://github.com/bytedance/deer-flow/pull/6909 [#6930]: https://github.com/bytedance/deer-flow/pull/6930 +[#6991]: https://github.com/bytedance/deer-flow/pull/6991 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 1d388532ce..f93cf82cc6 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -4538,6 +4538,12 @@ 因为它会把相对路径的 `cd` 移出工作区。`$PWD/...` 仍然允许，因为它与对应的相对路径 指向同一文件，除非命令自己重新绑定了 `PWD`。URL 中的变量不受影响。该守卫仍是尽力 而为的检查，并非隔离边界。([#6930]) +- **沙箱：** 在本地沙箱中，`ls` 现在接受 `/mnt/user-data` 根目录本身，并像 AIO 一样 + 列出 `workspace`、`uploads` 和 `outputs`。提供方有意映射了这个根目录，但工具层只允许 + 其下的路径，因此返回 `Permission denied`（带结尾斜杠时来自内部的 + “path traversal detected” 检查）。列表只包含这三个目录，直接存放在根目录下的其他内容 + 仍然不可见。`glob`、`grep`、`read_file` 和写入仍然拒绝根目录，因为搜索会读取其下的 + 所有文件。([#6991]) ### 安全 @@ -9391,3 +9397,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#6897]: https://github.com/bytedance/deer-flow/pull/6897 [#6909]: https://github.com/bytedance/deer-flow/pull/6909 [#6930]: https://github.com/bytedance/deer-flow/pull/6930 +[#6991]: https://github.com/bytedance/deer-flow/pull/6991 diff --git a/backend/packages/harness/deerflow/sandbox/AGENTS.md b/backend/packages/harness/deerflow/sandbox/AGENTS.md index b0c51b14bc..96baf17e26 100644 --- a/backend/packages/harness/deerflow/sandbox/AGENTS.md +++ b/backend/packages/harness/deerflow/sandbox/AGENTS.md @@ -20,7 +20,7 @@ cancellation before propagating it. Holders stay bound through scope cleanup; AIO reserves idle teardown atomically. Fork lookups reject invalidated clients; cancelled acquires leave borrowers a pending release. **Implementations**: -- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-user/thread `LocalSandbox` (id `local:{user_id}:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Shared runs use category mappings; a policy-scoped run replaces them with one `/mnt/skills` root mapping to the coherent thread view, so structured file tools resolve through one managed boundary. This is not a host filesystem security boundary: an enabled host `bash` subprocess can use canonical paths without `PathMapping`, so `supports_agent_skill_isolation` is dynamic and explicit Agent policies fail closed while host bash is enabled; unrestricted runs then get the shared mappings, never a leftover thread view. Host-to-virtual output masking scans dynamic per-user/per-thread roots directly instead of compiling path-specific regexes, so evicted thread IDs do not remain in Python's global regex caches; a separate 256-entry root cache prevents repeated `realpath()` walks for every glob/grep match while bounding dynamic-path retention, and only the small process-stable skill/integration source set uses a bounded compiled cache. The shared `path_patterns.py` tail stops at `:`, so `$PATH`-style lists mask every entry; a mount symlink resolving outside all mounts keeps its mount path. On Windows, Git Bash/MSYS argument-conversion exclusions are limited to safe non-root virtual path prefixes; do not restore a blanket conversion disable, because host-native CLI launchers need normal MSYS path conversion for their own installation paths. +- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-user/thread `LocalSandbox` (id `local:{user_id}:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data` (file tools: `ls` lists only these three; search/read reject the root), its `{workspace,uploads,outputs}`, and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Shared runs use category mappings; a policy-scoped run replaces them with one `/mnt/skills` root mapping to the coherent thread view, so structured file tools resolve through one managed boundary. This is not a host filesystem security boundary: an enabled host `bash` subprocess can use canonical paths without `PathMapping`, so `supports_agent_skill_isolation` is dynamic and explicit Agent policies fail closed while host bash is enabled; unrestricted runs then get the shared mappings, never a leftover thread view. Host-to-virtual output masking scans dynamic per-user/per-thread roots directly instead of compiling path-specific regexes, so evicted thread IDs do not remain in Python's global regex caches; a separate 256-entry root cache prevents repeated `realpath()` walks for every glob/grep match while bounding dynamic-path retention, and only the small process-stable skill/integration source set uses a bounded compiled cache. The shared `path_patterns.py` tail stops at `:`, so `$PATH`-style lists mask every entry; a mount symlink resolving outside all mounts keeps its mount path. On Windows, Git Bash/MSYS argument-conversion exclusions are limited to safe non-root virtual path prefixes; do not restore a blanket conversion disable, because host-native CLI launchers need normal MSYS path conversion for their own installation paths. PowerShell fallback pairs guarded console UTF-8 setters with explicit UTF-8 pipe decoding; MSYS prefers UTF-8 with an ANSI fallback. Cmd builtin output uses `GetConsoleOutputCP()`, then `GetOEMCP()` when no console page is available, and finally `locale.getencoding()` if the APIs cannot supply a page. Query per command because the console page can change; keep `_run_windows_command(encoding=None)` aligned with Python text-mode semantics. Native tools may use their own encodings. Encoding regressions include real pipes with distinct console/ANSI pages and Windows-only cmd roundtrips in private consoles, plus PowerShell roundtrips with and without a console. - `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker isolation. Reuse enforces minimum persisted shell capacity even without image-env overrides. Upgrades require ownership-fenced teardown; provisioner create returns 409 for undersized Pods without deleting them. Acquire/reuse drops definitively dead active/warm entries before rediscovery/creation. Health-check failures mean unknown, so local discovery skips unverifiable containers. Acquire/reclaim publishes ownership; `_renew_owned_leases` refreshes it. Text appends use native AIO append, never read-modify-write. `reset()` closes acquire pools; `shutdown()` performs full teardown. Async acquire separates lock waiters from holder work; started cache/warm workers drain, queued work/skills projection stay cancellable. Create polls flock; owned work/readiness/registration/cleanup drain before unlock. `uses_thread_data_mounts` defaults to backend detection (`LocalContainerBackend=True`, remote/provisioner=False); `sandbox.thread_data_mounts` overrides this for deployments sharing thread user-data directories. `true` skips upload acquire/sync; a false positive makes uploads unavailable. Explicit Agent policies use four thread projection category mounts and a distinct deterministic identity, excluding older shared-mount containers. The startup snapshot of `skills.container_path` drives mounts, identity, remote requests, and provisioner validation; root changes prevent container/Pod reuse. Gateway and provisioner independently require a canonical absolute root outside reserved platform mounts and derive four category allowlist entries from it. All four category overrides suppress the provisioner's default hostPath/skills-PVC mount; with `USERDATA_PVC_NAME`, categories use subpaths on that shared PVC. Readiness probes and `agent_sandbox` clients set `trust_env=False` for direct control-plane destinations (loopback/private IPs, single-label cluster hosts, Docker/Podman internal hostnames); external FQDNs and public IPs retain environment proxies. - `E2BSandboxProvider` (`packages/harness/deerflow/community/e2b_sandbox/`) provides E2B remote isolation. diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 55cddce534..f043a8729a 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -1013,6 +1013,7 @@ def validate_local_tool_path(path: str, thread_data: ThreadDataState | None, *, Allowed virtual-path families: - ``/mnt/user-data/*`` — always allowed (read + write) + - ``/mnt/user-data`` — the bare root, allowed only when *read_only* is True - ``/mnt/skills/*`` — allowed only when *read_only* is True - ``/mnt/acp-workspace/*`` — allowed only when *read_only* is True - Custom mount paths (from config.yaml) — respects per-mount ``read_only`` flag @@ -1047,8 +1048,10 @@ def validate_local_tool_path(path: str, thread_data: ThreadDataState | None, *, raise PermissionError(f"Write access to ACP workspace is not allowed: {path}") return - # User-data paths - if path.startswith(f"{VIRTUAL_PATH_PREFIX}/"): + # User-data paths. The bare root passes for reads because the local + # provider maps it for parity with AIO; the resolver still rejects it, + # and only ls lists it, one allowed directory at a time. + if path.startswith(f"{VIRTUAL_PATH_PREFIX}/") or (read_only and path == VIRTUAL_PATH_PREFIX): return # Custom mount paths — respect read_only config @@ -1100,6 +1103,29 @@ def _resolve_and_validate_user_data_path(path: str, thread_data: ThreadDataState return str(resolved) +def _user_data_root_dirs(path: str, thread_data: ThreadDataState) -> dict[str, str] | None: + """Map virtual to host workspace/uploads/outputs when *path* is the user-data root. + + The local provider maps the root so ``ls`` matches AIO, but its host + directory can hold more than the three allowed directories. Callers list + each of them instead of walking the root. Any other path returns None. + """ + mappings = _thread_virtual_to_actual_mappings(thread_data) + user_data_root = mappings.pop(VIRTUAL_PATH_PREFIX, None) + if user_data_root is None or Path(replace_virtual_path(path, thread_data)).resolve() != Path(user_data_root).resolve(): + return None + return mappings + + +def _list_user_data_root(sandbox: Sandbox, user_data_dirs: dict[str, str]) -> list[str]: + """List each allowed directory and one level below it, matching ls depth 2.""" + entries: list[str] = [] + for virtual_dir, actual_dir in user_data_dirs.items(): + entries.append(f"{virtual_dir}/") + entries.extend(sandbox.list_dir(actual_dir, max_depth=1)) + return entries + + def _is_non_file_url_token(token: str) -> bool: """Return True for URL tokens that should not be interpreted as paths.""" values = [token] @@ -2577,6 +2603,7 @@ def ls_tool(runtime: Runtime, path: str, description: str = "") -> str: ensure_thread_directories_exist(runtime) requested_path = path thread_data = None + user_data_dirs = None if is_local_sandbox(runtime): thread_data = get_thread_data(runtime) validate_local_tool_path(path, thread_data, read_only=True) @@ -2588,9 +2615,11 @@ def ls_tool(runtime: Runtime, path: str, description: str = "") -> str: # from the sandbox mapping's user_id). pass elif not _is_custom_mount_path(path): - path = _resolve_and_validate_user_data_path(path, thread_data) + user_data_dirs = _user_data_root_dirs(path, thread_data) + if user_data_dirs is None: + path = _resolve_and_validate_user_data_path(path, thread_data) # Custom mount paths and skills/ACP paths are resolved by LocalSandbox._resolve_path() - children = sandbox.list_dir(path) + children = _list_user_data_root(sandbox, user_data_dirs) if user_data_dirs is not None else sandbox.list_dir(path) if not children: return "(empty)" output = "\n".join(children) diff --git a/backend/tests/test_local_sandbox_virtual_path_contract.py b/backend/tests/test_local_sandbox_virtual_path_contract.py index 603d3813b2..51239a79cf 100644 --- a/backend/tests/test_local_sandbox_virtual_path_contract.py +++ b/backend/tests/test_local_sandbox_virtual_path_contract.py @@ -143,6 +143,43 @@ def test_execute_command_lists_aggregate_user_data_root(provider): assert "outputs" in output +@pytest.mark.parametrize("path", ["/mnt/user-data", "/mnt/user-data/"]) +def test_ls_tool_lists_aggregate_user_data_root(provider, path): + """The tool layer lists the root the provider maps, but only its three + allowed dirs: files and directories planted directly under it stay hidden.""" + from deerflow.config.paths import get_paths + from deerflow.sandbox.tools import ls_tool + + sandbox_id = provider.acquire("alpha", user_id="alice") + sbx = provider.get(sandbox_id) + sbx.write_file("/mnt/user-data/uploads/report.txt", "ready") + paths = get_paths() + user_data = paths.sandbox_user_data_dir("alpha", user_id="alice") + (user_data / "private.txt").write_text("PROVIDER_SECRET", encoding="utf-8") + (user_data / "extra").mkdir() + runtime = SimpleNamespace( + state={ + "sandbox": {"sandbox_id": sandbox_id}, + "thread_data": { + "workspace_path": str(paths.sandbox_work_dir("alpha", user_id="alice")), + "uploads_path": str(paths.sandbox_uploads_dir("alpha", user_id="alice")), + "outputs_path": str(paths.sandbox_outputs_dir("alpha", user_id="alice")), + }, + }, + context={"thread_id": "alpha", "user_id": "alice"}, + ) + + with patch("deerflow.sandbox.tools.ensure_sandbox_initialized", return_value=sbx): + result = ls_tool.func(runtime=runtime, description="list user data", path=path) + + for name in ("workspace", "uploads", "outputs"): + assert f"/mnt/user-data/{name}" in result + assert "/mnt/user-data/uploads/report.txt" in result + assert "private.txt" not in result + assert "extra" not in result + assert str(paths.base_dir) not in result + + def test_list_dir_on_user_data_root_does_not_duplicate_subdir_mounts(provider): """Regression: ``list_dir``'s virtual sub-directory overlay must not double-list a mount that the underlying scan already found. diff --git a/backend/tests/test_sandbox_search_tools.py b/backend/tests/test_sandbox_search_tools.py index 4aebe116dc..6d3c5dd9b3 100644 --- a/backend/tests/test_sandbox_search_tools.py +++ b/backend/tests/test_sandbox_search_tools.py @@ -790,6 +790,71 @@ def test_ls_tool_filters_upload_staging_files(tmp_path, monkeypatch) -> None: assert ".upload-active.part" not in result +@pytest.mark.parametrize("path", ["/mnt/user-data", "/mnt/user-data/"]) +def test_ls_tool_lists_user_data_root(tmp_path, monkeypatch, path) -> None: + """The local provider maps the aggregate root, so ls must list it like AIO.""" + runtime = _make_runtime(tmp_path) + (tmp_path / "uploads" / "report.txt").write_text("ready\n", encoding="utf-8") + (tmp_path / "uploads" / ".upload-active.part").write_text("partial\n", encoding="utf-8") + (tmp_path / "workspace" / "src").mkdir() + (tmp_path / "workspace" / "src" / "deep.py").write_text("x = 1\n", encoding="utf-8") + + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: LocalSandbox(id="local")) + + result = ls_tool.func(runtime=runtime, description="list user data", path=path) + + for name in ("workspace", "uploads", "outputs"): + assert f"/mnt/user-data/{name}" in result + assert "/mnt/user-data/uploads/report.txt" in result + assert "/mnt/user-data/workspace/src" in result + # Two levels from the root, like any other ls. + assert "deep.py" not in result + assert ".upload-active.part" not in result + assert str(tmp_path) not in result + + +def _plant_outside_allowed_dirs(tmp_path) -> None: + """Content in user-data/ that is not under workspace, uploads, or outputs.""" + (tmp_path / "private.txt").write_text("ROOT_SECRET\n", encoding="utf-8") + (tmp_path / "extra").mkdir() + (tmp_path / "extra" / "notes.txt").write_text("EXTRA_SECRET\n", encoding="utf-8") + + +@pytest.mark.parametrize("path", ["/mnt/user-data", "/mnt/user-data/"]) +def test_ls_tool_on_user_data_root_lists_only_allowed_dirs(tmp_path, monkeypatch, path) -> None: + runtime = _make_runtime(tmp_path) + _plant_outside_allowed_dirs(tmp_path) + symlink_or_skip(tmp_path / "workspace" / "alias.txt", tmp_path / "private.txt") + + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: LocalSandbox(id="local")) + + result = ls_tool.func(runtime=runtime, description="list user data", path=path) + + assert "/mnt/user-data/workspace" in result + assert "private.txt" not in result + assert "extra" not in result + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + (glob_tool, {"pattern": "**/*.txt"}), + (grep_tool, {"pattern": "SECRET"}), + ], +) +@pytest.mark.parametrize("path", ["/mnt/user-data", "/mnt/user-data/"]) +def test_search_tools_reject_user_data_root(tmp_path, monkeypatch, tool, arguments, path) -> None: + """A search walks every descendant, so it must not start above the allowed dirs.""" + runtime = _make_runtime(tmp_path) + _plant_outside_allowed_dirs(tmp_path) + + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: LocalSandbox(id="local")) + + result = tool.func(runtime=runtime, description="search user data", path=path, **arguments) + + assert result == f"Error: Permission denied: {path}" + + def _make_skills_sandbox(tmp_path, monkeypatch, *, disabled: str): """Skills tree with one disabled and one enabled public skill. diff --git a/backend/tests/test_sandbox_tools_security.py b/backend/tests/test_sandbox_tools_security.py index e9fb31b276..07e32d16d1 100644 --- a/backend/tests/test_sandbox_tools_security.py +++ b/backend/tests/test_sandbox_tools_security.py @@ -24,6 +24,7 @@ from deerflow.sandbox.tools import ( _resolve_and_validate_user_data_path, _resolve_skills_path, bash_tool, + ls_tool, mask_local_paths_in_output, replace_virtual_path, replace_virtual_paths_in_command, @@ -475,6 +476,45 @@ def test_validate_local_tool_path_rejects_bare_virtual_root() -> None: validate_local_tool_path(VIRTUAL_PATH_PREFIX, _THREAD_DATA) +def test_validate_local_tool_path_allows_bare_virtual_root_for_reads() -> None: + """The local provider maps the aggregate root so it can be listed like AIO.""" + validate_local_tool_path(VIRTUAL_PATH_PREFIX, _THREAD_DATA, read_only=True) + + +@pytest.mark.parametrize("path", [VIRTUAL_PATH_PREFIX, f"{VIRTUAL_PATH_PREFIX}/", f"{VIRTUAL_PATH_PREFIX}/stray.txt"]) +def test_resolve_user_data_path_keeps_rejecting_the_root(path: str) -> None: + """Read and search tools resolve here; only ls lists the root, one allowed dir at a time.""" + with pytest.raises(PermissionError, match="path traversal"): + _resolve_and_validate_user_data_path(path, _THREAD_DATA) + + +def test_ls_tool_rejects_user_data_root_without_a_shared_parent(tmp_path) -> None: + """Without a common parent there is no host root to list, so stay closed.""" + thread_data = {name: str(tmp_path / name / name.removesuffix("_path")) for name in ("workspace_path", "uploads_path", "outputs_path")} + for directory in thread_data.values(): + Path(directory).mkdir(parents=True) + runtime = SimpleNamespace(state={"sandbox": {"sandbox_id": "local"}, "thread_data": thread_data}, context={"thread_id": "thread-1"}) + + with patch("deerflow.sandbox.tools.ensure_sandbox_initialized", return_value=LocalSandbox(id="local")): + result = ls_tool.func(runtime=runtime, description="list", path=VIRTUAL_PATH_PREFIX) + + assert result == f"Error: Permission denied: {VIRTUAL_PATH_PREFIX}" + + +@pytest.mark.parametrize("path", [VIRTUAL_PATH_PREFIX, f"{VIRTUAL_PATH_PREFIX}/"]) +def test_write_file_tool_rejects_user_data_root(tmp_path, path: str) -> None: + thread_data = {name: str(tmp_path / "user-data" / name.removesuffix("_path")) for name in ("workspace_path", "uploads_path", "outputs_path")} + for directory in thread_data.values(): + Path(directory).mkdir(parents=True) + runtime = SimpleNamespace(state={"sandbox": {"sandbox_id": "local"}, "thread_data": thread_data}, context={"thread_id": "thread-1"}) + sandbox = SimpleNamespace(write_file=lambda *args, **kwargs: pytest.fail("write reached the sandbox")) + + with patch("deerflow.sandbox.tools.ensure_sandbox_initialized", return_value=sandbox): + result = write_file_tool.func(runtime=runtime, description="write", path=path, content="x") + + assert result == f"Error: Permission denied writing to file: {path}" + + def test_validate_local_tool_path_allows_user_data_paths() -> None: # Should not raise — user-data paths are always allowed validate_local_tool_path(f"{VIRTUAL_PATH_PREFIX}/workspace/file.txt", _THREAD_DATA)