From 1b9667ea0ec0296b57e3665fd33301e6466de3ef Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Mon, 14 Sep 2026 15:05:21 +0900 Subject: [PATCH] fix(sandbox): mask every host path in a colon-joined list (#5418) * fix(sandbox): mask every host path in a colon-joined list Host-to-virtual output masking matched a host root and then consumed the path tail up to whitespace or shell punctuation, but not `:`. A `:`-joined list such as $PATH or $PYTHONPATH was therefore swallowed into the first match's tail, and scanning resumed after it, so every later entry under the same root reached the model as a raw host path. The regex matcher (process-stable skill roots) and the direct scanner (per-thread roots, LocalSandbox) shared the gap. Each redundant masking pass -- separator variants, the realpath spelling, the /mnt/user-data root mapping, LocalSandbox's own reverse resolution -- happened to recover one entry, which hid the leak for short lists: bash output leaked from the fourth entry, single-pass consumers from the third. The shared tail in path_patterns.py now ends at `:` in both matchers. `;`, the Windows list separator, already ended it. A `:` inside one path (grep -n output, a file name) only shortens the match; the remaining text is copied through verbatim. Shortening the match exposed a second leak. LocalSandbox reverse resolution realpaths the matched path and returned that realpath when no mount contained it, so a symlink inside a mount whose target lies outside every mount was shown as the target's host path. grep -n lines used to hide this only because the whole line resolved as one nonexistent file; whitespace-terminated output and LocalSandbox.glob results already leaked it on main. Reverse resolution now falls back to the link's own spelling, normalized so `mount/../x` does not pass, before giving up. A symlink into another mount still reports that mount's path. * docs(changelog): reference #5418 in the colon-joined path masking entry * docs(changelog): split the #5418 and #5419 entries fused by the merge Resolving the CHANGELOG conflict when main was merged in dropped the opener of the #5419 entry, so the BoxLite grep fix continued inside this PR's bullet in both CHANGELOG.md and CHANGELOG_zh.md. Restore it as its own bullet; the #5419 entry is byte-identical to main again. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 8 ++ CHANGELOG_zh.md | 5 + .../harness/deerflow/sandbox/AGENTS.md | 2 +- .../deerflow/sandbox/local/local_sandbox.py | 32 +++++-- .../harness/deerflow/sandbox/path_patterns.py | 10 +- .../test_local_sandbox_provider_mounts.py | 52 +++++++++++ backend/tests/test_sandbox_path_patterns.py | 91 ++++++++++++++++++- backend/tests/test_sandbox_tools_security.py | 55 +++++++++++ 8 files changed, 237 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a612d9a0..6ddc7e8ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -582,6 +582,13 @@ This section accumulates work toward the **2.1.0** milestone ### Fixed +- **sandbox:** Stop host paths reaching the model when output joins them with + `:`, as `$PATH` and `$PYTHONPATH` do. The matched path ran on through the + rest of the list, so every later entry under the same root was left + unmasked; extra masking passes recovered one entry each, which hid the leak + for short lists. Masking now ends a matched path at `:`. A symlink inside a + mount whose target lies outside every mount is now shown by its mount path + instead of the target's host path in command output and `glob` results. ([#5418]) - **sandbox:** Stop BoxLite `grep` from ignoring the directory part of `glob`. It compared only file names, so `src/*.js` matched every `.js` file in the tree. The glob now applies to the path relative to the search root, the same @@ -2823,4 +2830,5 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5401]: https://github.com/bytedance/deer-flow/pull/5401 [#5403]: https://github.com/bytedance/deer-flow/pull/5403 [#5411]: https://github.com/bytedance/deer-flow/pull/5411 +[#5418]: https://github.com/bytedance/deer-flow/pull/5418 [#5419]: https://github.com/bytedance/deer-flow/pull/5419 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index d849e39b1..3b803529d 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -397,6 +397,10 @@ ### 修复 +- **沙箱：** 当输出用 `:` 连接主机路径（如 `$PATH`、`$PYTHONPATH`）时，主机路径不再暴露给模型。 + 匹配的路径会一直延伸到列表末尾，导致同一根目录下之后的条目都未被遮蔽；多余的遮蔽轮次每次 + 恰好补回一个条目，因此短列表掩盖了这一泄露。现在遮蔽时匹配的路径在 `:` 处结束。 + 挂载目录内指向所有挂载之外的符号链接，在命令输出和 `glob` 结果中改为显示其挂载路径，而不是目标的主机路径。([#5418]) - **沙箱：** BoxLite `grep` 不再忽略 `glob` 的目录部分。此前只比较文件名，`src/*.js` 会匹配整棵目录树中的所有 `.js` 文件。现在 glob 作用于相对搜索根目录的路径，与 `glob()` 及其他 provider 的范围一致。([#5419]) @@ -2161,4 +2165,5 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5401]: https://github.com/bytedance/deer-flow/pull/5401 [#5403]: https://github.com/bytedance/deer-flow/pull/5403 [#5411]: https://github.com/bytedance/deer-flow/pull/5411 +[#5418]: https://github.com/bytedance/deer-flow/pull/5418 [#5419]: https://github.com/bytedance/deer-flow/pull/5419 diff --git a/backend/packages/harness/deerflow/sandbox/AGENTS.md b/backend/packages/harness/deerflow/sandbox/AGENTS.md index 742cccb3d..ad4eb7fda 100644 --- a/backend/packages/harness/deerflow/sandbox/AGENTS.md +++ b/backend/packages/harness/deerflow/sandbox/AGENTS.md @@ -7,7 +7,7 @@ **Authorization gate** (`sandbox:execute`, RFC #4063 Phase 3): every sandbox-backed tool call passes through the gate in `deerflow/authz/sandbox_authz.py` - a binary `authorize(principal, "sandbox", "execute", target="*")` check before either reusing a persisted sandbox id or calling `provider.acquire`. Rechecking reuse is required because authorization config and user roles can change while the sandbox remains cached. Sync tool invocations call `authorize_sandbox_execution`; async tool invocations await `authorize_sandbox_execution_async` exactly once. A task-local `ContextVar` scopes that single decision across the complete composed tool invocation, including `ReadBeforeWriteMiddleware`'s pre-write inspection, tool body, and post-read mark; the value is copied into `asyncio.to_thread` workers. Authorization denial is converted to the normal error `ToolMessage` at the composed middleware boundary and is explicitly excluded from the gate's generic fail-open handlers. Async config loading and provider class discovery/import are offloaded before `aauthorize()` so reused sandbox calls do not hash config files or import custom modules on the event loop; provider construction remains on the running event loop because async providers may initialize loop-affine clients. The gate lives at the single tool initialization entry point (`ensure_sandbox_initialized` / `ensure_sandbox_initialized_async` in `tools.py`), while `SandboxMiddleware.before_agent` / `abefore_agent` apply the matching sync/async check to eager acquisition. Deny raises `SandboxAuthorizationError` (`sandbox/exceptions.py`), which propagates out of ordinary tool execution as a friendly error `ToolMessage` ("sandbox execution is not permitted for your role") - the eager path catches it and skips acquisition instead, deferring the deny to the first sandbox-touching tool call so both paths share the same semantics. Provider errors (authorization calls and provider resolution) follow `authorization.fail_closed` / `fail_open`; no readable `config.yaml` or `authorization.enabled: false` makes the gate a no-op (`safe_app_config` tolerates missing config). Gateway upload/artifact sync calls `try_acquire_sandbox_for_request` (`app/gateway/authz.py`), which gates, returns a request lease, and skips sync on deny while preserving the primary operation. Callers release after their last sandbox operation; artifacts request normal parking, uploads do not. Tests: `tests/test_sandbox_authorization.py` and `tests/blocking_io/test_sandbox_authorization.py`. **Environment policy** (`sandbox/env_policy.py`): `execute_command` no longer inherits the full `os.environ`. `build_sandbox_env()` scrubs secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASS*`/`*CREDENTIAL*`) from the inherited environment before layering injected request secrets on top, so platform credentials (e.g. `OPENAI_API_KEY`) never leak into skill subprocesses. Benign vars (`PATH`, `HOME`, `LANG`, `VIRTUAL_ENV`, ...) are preserved. **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. 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. 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/{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. 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. - `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths — it never touches the ownership store (that would be blocking IO on the event loop); ownership is published on acquire/reclaim and refreshed off the event loop by the dedicated renewal thread (`_renew_owned_leases`). Text appends use the AIO file API's native append mode rather than a client-side read-modify-write, so a failed pre-read cannot turn an append into an overwrite. `reset()` closes the per-instance acquire serializer so replacing the singleton cannot retain its executor workers; full remote sandbox teardown remains `shutdown()`. `uses_thread_data_mounts` defaults to backend detection (`LocalContainerBackend=True`, remote/provisioner backends=False), while the optional `sandbox.thread_data_mounts` boolean takes precedence for deployments that guarantee the Gateway and sandbox share the same thread user-data directories. Setting it `true` skips upload-time sandbox acquire/sync; a false positive leaves uploads unavailable to the sandbox. An explicit Agent policy uses four thread projection category mounts and a distinct deterministic sandbox identity, preventing reuse of an older container created with shared mounts. `skills.container_path` is a provider-startup snapshot shared by mount construction, sandbox identity, the remote Gateway request, and provisioner validation; custom roots are identity-scoped so a container or Pod created for one destination cannot be reused after the root changes. The Gateway and provisioner independently require one canonical absolute root that does not overlap reserved platform mounts, and both derive the four category allowlist entries from that root. The provisioner accepts all four category overrides; when all are present it suppresses the default hostPath or skills-PVC mount. With `USERDATA_PVC_NAME`, the thread projection categories use subpaths on that shared data PVC. Readiness probes and `agent_sandbox` clients classify loopback/private IPs, single-label cluster hosts, and Docker/Podman internal hostnames as direct control-plane destinations and set `trust_env=False`; external FQDNs and public IPs retain environment proxy support. - `E2BSandboxProvider` (`packages/harness/deerflow/community/e2b_sandbox/`) provides E2B remote isolation. New unrestricted sandboxes receive a one-shot upload from the enabled-only diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index eebabcfec..59b600e4d 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -355,28 +355,40 @@ class LocalSandbox(Sandbox): normalized_path = path.replace("\\", "/") path_str = os.path.realpath(normalized_path) + container_path = self._container_path_for_local(path_str) + if container_path is None: + # A symlink under a mount can resolve outside every mount. Its own + # spelling still names a path inside the mount, so translate that + # rather than hand the model the link target's host path. ``normpath`` + # keeps ``mount/../x`` from passing as inside the mount. + container_path = self._container_path_for_local(os.path.normpath(normalized_path)) + if container_path is not None: + return container_path + + # No mapping found, return original path + return path_str + + def _container_path_for_local(self, local_path: str) -> str | None: + """Translate a native-separated host path under a mount, or return ``None``.""" # Try each mapping (longest local path first for more specific matches) for mapping in self._mappings_by_local_specificity: local_path_resolved = self._resolved_local_paths[mapping] # ``Path.resolve()`` always renders with the native separator - # (backslash on Windows), regardless of the forward-slash - # normalization above, so the containment check must compare with + # (backslash on Windows), regardless of the caller's forward-slash + # normalization, so the containment check must compare with # ``os.sep`` here too -- mirroring ``_is_read_only_path`` -- instead # of a hardcoded "/". A hardcoded "/" can never match a # backslash-joined nested path on Windows, so every nested path - # silently fell through to the "no mapping found" branch below and + # silently fell through to the "no mapping found" fallback and # leaked the raw host path (real username, full directory tree). - if path_str == local_path_resolved or path_str.startswith(local_path_resolved + os.sep): + if local_path == local_path_resolved or local_path.startswith(local_path_resolved + os.sep): # Replace the local path prefix with container path. Container # paths are always POSIX-style, so the extracted relative # portion (native-separated on Windows) is normalized to # forward slashes before being spliced in. - relative = path_str[len(local_path_resolved) :].lstrip(os.sep).replace(os.sep, "/") - resolved = f"{mapping.container_path}/{relative}" if relative else mapping.container_path - return resolved - - # No mapping found, return original path - return path_str + relative = local_path[len(local_path_resolved) :].lstrip(os.sep).replace(os.sep, "/") + return f"{mapping.container_path}/{relative}" if relative else mapping.container_path + return None def _reverse_resolve_paths_in_output(self, output: str) -> str: """ diff --git a/backend/packages/harness/deerflow/sandbox/path_patterns.py b/backend/packages/harness/deerflow/sandbox/path_patterns.py index 980ed1fe2..850920b21 100644 --- a/backend/packages/harness/deerflow/sandbox/path_patterns.py +++ b/backend/packages/harness/deerflow/sandbox/path_patterns.py @@ -44,10 +44,16 @@ _SEGMENT_BOUNDARY = r"(?=/|$|[^\w./-])" # The path tail following the base. ``[/\\]`` keeps Windows-separated paths # matching; the negated class stops at whitespace and shell punctuation so a # path embedded in a larger line is not over-consumed. -_PATH_TAIL = r"(?:[/\\][^\s\"';&|<>()]*)?" +# +# ``:`` ends the tail as well. Scanning resumes after a match, so a tail that +# ran on through a ``:``-joined list ($PATH, $PYTHONPATH) carried every later +# entry under the same base to the model unmasked. ``;``, the Windows list +# separator, already ended it. A ``:`` inside one path (``grep -n`` output, +# a file name) only shortens the match; the rest is copied through verbatim. +_PATH_TAIL = r"(?:[/\\][^\s\"';&|<>():]*)?" _SEGMENT_BOUNDARY_CHAR = re.compile(r"[^\w./-]") -_PATH_TAIL_TERMINATORS = frozenset("\"';&|<>()") +_PATH_TAIL_TERMINATORS = frozenset("\"';&|<>():") def normalize_mask_tail(tail: str) -> str: diff --git a/backend/tests/test_local_sandbox_provider_mounts.py b/backend/tests/test_local_sandbox_provider_mounts.py index 8eb0b2604..9d832ff21 100644 --- a/backend/tests/test_local_sandbox_provider_mounts.py +++ b/backend/tests/test_local_sandbox_provider_mounts.py @@ -588,6 +588,58 @@ class TestMultipleMounts: assert "/mnt/data/file.txt" in masked assert str(mount_dir) not in masked + @pytest.mark.parametrize("suffix", ["", ":3:needle", " 3 needle"]) + def test_reverse_resolve_keeps_mount_spelling_for_symlink_resolving_outside(self, tmp_path, suffix): + """A link under a mount whose target is outside every mount must not turn + into the target's host path. + + ``grep -n`` output (``link.py:3:...``) used to hide this only because the + whole line was resolved as one nonexistent file; once a match ends at + ``:``, the link itself is resolved like any whitespace-terminated path. + """ + workspace = (tmp_path / "workspace").resolve() + workspace.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.py").write_text("needle\n") + _symlink_to(outside / "secret.py", workspace / "link.py") + sandbox = LocalSandbox("test", [PathMapping(container_path="/mnt/user-data/workspace", local_path=str(workspace))]) + + masked = sandbox._reverse_resolve_paths_in_output(f"{workspace}/link.py{suffix}") + + assert masked == f"/mnt/user-data/workspace/link.py{suffix}" + # Structured results take the same path back: ``glob`` returned the target's host path. + assert sandbox.glob("/mnt/user-data/workspace", "*.py") == (["/mnt/user-data/workspace/link.py"], False) + + def test_reverse_resolve_prefers_the_mount_a_symlink_resolves_into(self, tmp_path): + """The spelling fallback applies only when resolution leaves every mount.""" + workspace = (tmp_path / "workspace").resolve() + uploads = (tmp_path / "uploads").resolve() + workspace.mkdir() + uploads.mkdir() + (uploads / "doc.md").write_text("x\n") + _symlink_to(uploads / "doc.md", workspace / "doc.md") + sandbox = LocalSandbox( + "test", + [ + PathMapping(container_path="/mnt/user-data/workspace", local_path=str(workspace)), + PathMapping(container_path="/mnt/user-data/uploads", local_path=str(uploads)), + ], + ) + + assert sandbox._reverse_resolve_path(str(workspace / "doc.md")) == "/mnt/user-data/uploads/doc.md" + + def test_reverse_resolve_spelling_fallback_does_not_keep_dot_dot_escapes(self, tmp_path): + """``mount/../x`` is outside the mount by spelling too, so it must not come + back as ``/mnt/.../../x`` -- a virtual path forward resolution rejects.""" + workspace = (tmp_path / "workspace").resolve() + workspace.mkdir() + sandbox = LocalSandbox("test", [PathMapping(container_path="/mnt/user-data/workspace", local_path=str(workspace))]) + + resolved = sandbox._reverse_resolve_path(f"{workspace}/../outside/secret.py") + + assert not resolved.startswith("/mnt/user-data/workspace") + class TestLocalSandboxProviderMounts: def test_skill_isolation_capability_fails_closed_when_host_bash_is_enabled(self): diff --git a/backend/tests/test_sandbox_path_patterns.py b/backend/tests/test_sandbox_path_patterns.py index 3fd3b1c4c..655662f96 100644 --- a/backend/tests/test_sandbox_path_patterns.py +++ b/backend/tests/test_sandbox_path_patterns.py @@ -10,7 +10,9 @@ The move itself was cleared by a differential against the *real* pre-extraction expressions, run once on the parent commit. That run cannot be committed: after this lands there is no old inline expression left to diff against, only the frozen copies below. So the committed guard is the weaker snapshot, and its -red-ness rests on those literals — not on the length of ``_BASES``. +red-ness rests on those literals — not on the length of ``_BASES``. The tail has +changed exactly once since then (it now stops at ``:``); the snapshot names that +delta instead of re-freezing the literals, so any other drift still goes red. """ from __future__ import annotations @@ -23,7 +25,7 @@ import pytest from deerflow.sandbox import path_patterns as path_patterns_module from deerflow.sandbox.local import local_sandbox as local_sandbox_module from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping -from deerflow.sandbox.path_patterns import build_output_mask_pattern +from deerflow.sandbox.path_patterns import build_output_mask_pattern, normalize_mask_tail from deerflow.sandbox.tools import _compiled_mask_patterns @@ -38,6 +40,18 @@ def _legacy_local_pattern(base: str) -> re.Pattern[str]: return re.compile(re.escape(base) + r"(?=/|$|[^\w./-])" + r"(?:[/\\][^\s\"';&|<>()]*)?") +# The only intentional change to either expression since the extraction: the +# tail also stops at ``:``, so a ``:``-joined path list ($PATH, $PYTHONPATH) +# cannot swallow a second host path into the first match's tail. +_LEGACY_TAIL = r"(?:[/\\][^\s\"';&|<>()]*)?" +_CURRENT_TAIL = r"(?:[/\\][^\s\"';&|<>():]*)?" + + +def _with_current_tail(legacy: str) -> str: + assert legacy.endswith(_LEGACY_TAIL) + return legacy.removesuffix(_LEGACY_TAIL) + _CURRENT_TAIL + + _BASES = [ "/host/skills", "/host/dir with spaces", @@ -55,13 +69,14 @@ _BASES = [ @pytest.mark.parametrize("base", _BASES) def test_helper_reproduces_the_pre_extraction_expressions(base: str) -> None: - """Byte-identical to what each call site built inline, for both separator modes. + """Byte-identical to what each call site built inline, for both separator modes, + apart from the one named tail change. This is the anchor for the move itself: edit the helper in a way that changes either site's regex and this goes red. """ - assert build_output_mask_pattern(base, separator_agnostic=True).pattern == _legacy_tools_pattern(base).pattern - assert build_output_mask_pattern(base).pattern == _legacy_local_pattern(base).pattern + assert build_output_mask_pattern(base, separator_agnostic=True).pattern == _with_current_tail(_legacy_tools_pattern(base).pattern) + assert build_output_mask_pattern(base).pattern == _with_current_tail(_legacy_local_pattern(base).pattern) def test_separator_agnostic_is_the_only_difference_between_the_two_modes() -> None: @@ -135,6 +150,72 @@ def test_direct_replacer_normalizes_nested_tail_to_virtual_posix_style() -> None ) +def _regex_mask(output: str, base: str, virtual: str) -> str: + """The static-source splice ``mask_local_paths_in_output`` applies per pattern.""" + + def replace(match: re.Match[str]) -> str: + relative = normalize_mask_tail(match.group(0)[len(base) :]) + return f"{virtual}/{relative}" if relative else virtual + + return build_output_mask_pattern(base, separator_agnostic=True).sub(replace, output) + + +def _scanner_mask(output: str, base: str, virtual: str) -> str: + return path_patterns_module.replace_output_path_matches(output, base, virtual, separator_agnostic=True) + + +_MASKERS = [pytest.param(_regex_mask, id="regex"), pytest.param(_scanner_mask, id="scanner")] + + +@pytest.mark.parametrize("mask", _MASKERS) +@pytest.mark.parametrize( + ("output", "expected"), + [ + ( + "PATH=/host/ws/.venv/bin:/host/ws/node_modules/.bin:/host/ws/bin:/usr/bin", + "PATH=/mnt/ws/.venv/bin:/mnt/ws/node_modules/.bin:/mnt/ws/bin:/usr/bin", + ), + ("PYTHONPATH=/host/ws/a:/host/ws/b", "PYTHONPATH=/mnt/ws/a:/mnt/ws/b"), + ("/host/ws:/host/ws/lib", "/mnt/ws:/mnt/ws/lib"), + ], +) +def test_tail_stops_at_colon_so_a_path_list_masks_every_entry(mask, output: str, expected: str) -> None: + """A ``:``-joined list must not hide a second host path inside the first tail. + + Once a match consumes the rest of the list, scanning resumes after it, so + every later entry under the same base reaches the model as a raw host path. + (``;``, the Windows list separator, already ended the tail.) + """ + assert mask(output, "/host/ws", "/mnt/ws") == expected + + +@pytest.mark.parametrize("mask", _MASKERS) +@pytest.mark.parametrize( + "tail", + ["/pkg/app.py:12:def main():", "/logs/10:00:00.log", "/a.py:3: /b.py:4:"], +) +def test_colon_inside_a_single_path_leaves_the_rendered_output_unchanged(mask, tail: str) -> None: + """``grep -n`` lines and ``:`` in file names only shorten the match: the rest + of the text is copied through verbatim right after the virtual prefix.""" + assert mask(f"/host/ws{tail}", "/host/ws", "/mnt/ws") == f"/mnt/ws{tail}" + + +def test_local_sandbox_reverse_mask_handles_a_colon_joined_path_list(tmp_path: Path) -> None: + """The callable replacement path: ``_reverse_resolve_path`` receives each + entry separately instead of the whole list as one fake path.""" + local = tmp_path / "workspace" + local.mkdir() + sandbox = LocalSandbox( + id="local", + path_mappings=[PathMapping(container_path="/mnt/user-data/workspace", local_path=str(local))], + ) + resolved = str(local.resolve()) + + output = sandbox._reverse_resolve_paths_in_output(f"{resolved}/.venv/bin:{resolved}/bin") + + assert output == "/mnt/user-data/workspace/.venv/bin:/mnt/user-data/workspace/bin" + + def test_separator_agnostic_replacer_avoids_normalization_without_backslashes() -> None: class ReplaceTrackingString(str): def __init__(self, value: str) -> None: diff --git a/backend/tests/test_sandbox_tools_security.py b/backend/tests/test_sandbox_tools_security.py index 5fc51951e..67a9983f7 100644 --- a/backend/tests/test_sandbox_tools_security.py +++ b/backend/tests/test_sandbox_tools_security.py @@ -1,3 +1,4 @@ +import os import threading from pathlib import Path from types import SimpleNamespace @@ -6,6 +7,7 @@ from unittest.mock import patch import pytest from deerflow.sandbox.exceptions import SandboxError +from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping from deerflow.sandbox.tools import ( VIRTUAL_PATH_PREFIX, _apply_cwd_prefix, @@ -209,6 +211,59 @@ def test_mask_local_paths_still_matches_base_before_non_slash_boundaries(boundar assert "/home/user/deer-flow/skills" not in masked +# Every redundant masking pass -- separator variants, the realpath spelling, the +# ``/mnt/user-data`` root mapping, ``LocalSandbox``'s own reverse resolution -- +# happened to recover one swallowed entry, which hid the leak for short lists. +# The lists below outnumber those passes on every platform. +_PATH_LIST_ENTRIES = 8 + + +def test_mask_local_paths_masks_every_entry_of_a_colon_joined_path_list() -> None: + """Both source kinds: skills use the compiled regex, user-data the scanner.""" + skills = "/home/user/deer-flow/skills" + workspace = _THREAD_DATA["workspace_path"] + hosts = [f"{skills}/s{i}/bin" for i in range(_PATH_LIST_ENTRIES)] + [f"{workspace}/w{i}/bin" for i in range(_PATH_LIST_ENTRIES)] + virtuals = [f"/mnt/skills/s{i}/bin" for i in range(_PATH_LIST_ENTRIES)] + [f"/mnt/user-data/workspace/w{i}/bin" for i in range(_PATH_LIST_ENTRIES)] + + with ( + patch("deerflow.sandbox.tools._get_skills_container_path", return_value="/mnt/skills"), + patch("deerflow.sandbox.tools._get_skills_host_path", return_value=skills), + ): + masked = mask_local_paths_in_output("PATH=" + ":".join([*hosts, "/usr/bin"]), _THREAD_DATA) + + assert masked == "PATH=" + ":".join([*virtuals, "/usr/bin"]) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX shell and ':'-separated PATH") +def test_local_bash_tool_output_masks_every_entry_of_a_colon_joined_path_list(tmp_path: Path, monkeypatch) -> None: + """End to end through ``bash_tool``: output passes ``LocalSandbox``'s reverse + resolution and then ``mask_local_paths_in_output`` before reaching the model.""" + thread_root = tmp_path / "threads" / "t1" / "user-data" + thread_data = {f"{name}_path": str(thread_root / name) for name in ("workspace", "uploads", "outputs")} + for path in thread_data.values(): + Path(path).mkdir(parents=True) + workspace = str(Path(thread_data["workspace_path"]).resolve()) + sandbox = LocalSandbox( + id="local", + path_mappings=[PathMapping(container_path=f"{VIRTUAL_PATH_PREFIX}/workspace", local_path=workspace)], + ) + runtime = SimpleNamespace( + state={"sandbox": {"sandbox_id": "local"}, "thread_data": thread_data}, + context={"thread_id": "t1"}, + config={}, + ) + monkeypatch.setattr("deerflow.sandbox.tools.ensure_sandbox_initialized", lambda runtime: sandbox) + monkeypatch.setattr("deerflow.sandbox.tools.is_host_bash_allowed", lambda: True) + + # The shell expands ``$PWD`` (the host workspace), so the host paths exist only + # in the output -- the ``export PATH="$PWD/.venv/bin:$PATH"`` shape. + command = "echo " + ":".join(f"$PWD/e{i}/bin" for i in range(_PATH_LIST_ENTRIES)) + result = bash_tool.func(runtime=runtime, description="print PATH", command=command) + + assert str(tmp_path) not in result + assert result.strip() == ":".join(f"{VIRTUAL_PATH_PREFIX}/workspace/e{i}/bin" for i in range(_PATH_LIST_ENTRIES)) + + @pytest.mark.parametrize("prefix", ["", "cwd: ", "see "]) def test_mask_local_paths_translates_a_bare_base_at_end_of_output(prefix: str) -> None: """``$`` is load-bearing: output ending exactly at a host base still masks.