From 797e093ddc65d512df4eb7ef044497c43ef7758e Mon Sep 17 00:00:00 2001 From: Evan <1178327328@qq.com> Date: Mon, 26 Oct 2026 20:44:29 -0400 Subject: [PATCH] fix(sandbox): require a path start before translating virtual and host roots (#7582) * fix(sandbox): require a path start before translating virtual and host roots Path translation only checked the right-hand segment boundary (#4035, #4053), so a root embedded in a longer path or URL was rewritten too. DeerFlow's own artifact URLs contain /mnt/user-data, so curl .../artifacts/mnt/user-data/outputs/x.html had the thread's host directory spliced into the URL, and a mount at /data turned an unrelated /var/data/app.log into /var/mnt/data/app.log in model-visible output. Add a shared left-hand PATH_START_BOUNDARY: a root must not follow a word character, '.', '~' or '-'. '/' is still allowed so file:///mnt/user-data keeps mapping. It applies to the bash command rewrite, LocalSandbox command/content resolution, and both reverse masking paths. * refactor(sandbox): derive the path-start lookbehind from one class Build PATH_START_BOUNDARY from _PATH_START_BLOCKERS so the regex and scanner paths cannot drift, and document the attached short-option value (-o/data/x) as a known false negative of the start rule. --------- Co-authored-by: Willem Jiang --- .../deerflow/sandbox/local/local_sandbox.py | 6 +- .../harness/deerflow/sandbox/path_patterns.py | 29 ++++++++- .../harness/deerflow/sandbox/tools.py | 8 ++- backend/tests/test_sandbox_path_patterns.py | 60 +++++++++++++++++-- backend/tests/test_sandbox_tools_security.py | 29 +++++++++ 5 files changed, 118 insertions(+), 14 deletions(-) diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index 9764a3c8e6..bb27a76e46 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -21,7 +21,7 @@ from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.sandbox.download import DEFAULT_DOWNLOAD_LIMIT, download_limit from deerflow.sandbox.env_policy import build_sandbox_env from deerflow.sandbox.local.list_dir import list_dir -from deerflow.sandbox.path_patterns import replace_output_path_matches +from deerflow.sandbox.path_patterns import PATH_START_BOUNDARY, replace_output_path_matches from deerflow.sandbox.sandbox import Sandbox, _validate_extra_env from deerflow.sandbox.search import GrepMatch, find_glob_matches, find_grep_matches, path_matches from deerflow.utils.atomic_write import publish_bytes_atomically @@ -498,7 +498,7 @@ class LocalSandbox(Sandbox): # substitution resumes after a match, so a tail that ran through a # ":"-joined list ($PATH, $PYTHONPATH) swallowed every later entry and # left those container paths unresolved. - patterns = [re.escape(m.container_path) + r"(?=/|$|[\s\"';&|<>()])(?:/[^\s\"';&|<>():]*)?" for m in mappings] + patterns = [PATH_START_BOUNDARY + re.escape(m.container_path) + r"(?=/|$|[\s\"';&|<>()])(?:/[^\s\"';&|<>():]*)?" for m in mappings] return re.compile("|".join(f"({p})" for p in patterns)) @cached_property @@ -511,7 +511,7 @@ class LocalSandbox(Sandbox): # write_file content regularly stores PATH-style exports, and a tail # that ran through the ":" left the later list entries virtual in the # host file (read_file masked them back, so the corruption was hidden). - patterns = [re.escape(m.container_path) + r"(?=/|$|[^\w./-])(?:/[^\s\"';&|<>():]*)?" for m in mappings] + patterns = [PATH_START_BOUNDARY + re.escape(m.container_path) + r"(?=/|$|[^\w./-])(?:/[^\s\"';&|<>():]*)?" for m in mappings] return re.compile("|".join(f"({p})" for p in patterns)) @cached_property diff --git a/backend/packages/harness/deerflow/sandbox/path_patterns.py b/backend/packages/harness/deerflow/sandbox/path_patterns.py index 850920b21e..885ae0dd32 100644 --- a/backend/packages/harness/deerflow/sandbox/path_patterns.py +++ b/backend/packages/harness/deerflow/sandbox/path_patterns.py @@ -41,6 +41,26 @@ from collections.abc import Callable # fail the lookahead and be emitted as the raw host path. _SEGMENT_BOUNDARY = r"(?=/|$|[^\w./-])" +# The mirror rule on the left: a base must start a path, not continue one. +# Without it ``/mnt/user-data`` matches inside the artifact URL +# ``.../artifacts/mnt/user-data/outputs/x.html`` or a relative +# ``backup/mnt/user-data``, and a mount at ``/data`` inside ``/var/data``. +# ``/`` is deliberately allowed so ``file:///mnt/user-data`` still maps; ``.`` +# and ``~`` reject ``./`` and ``~/`` relative spellings. Forward-resolution +# patterns (container → host) in ``LocalSandbox`` and ``sandbox.tools`` share +# this rule, so it is public. +# +# Known false negative: a value attached directly to a short option +# (``curl -o/data/x``, ``ssh -i/data/key``, ``gcc -I/data/inc``) follows a +# letter, so it is left virtual instead of being translated. Spell those with a +# space (``-o /data/x``) or ``=``. Rejecting embedded roots matters more: the +# alternative rewrites unrelated paths and URLs. +# +# The scanner and the regex lookbehind derive from one character class so +# ``replace_output_path_matches`` and ``build_output_mask_pattern`` cannot drift. +_PATH_START_BLOCKERS = re.compile(r"[\w.~-]") +PATH_START_BOUNDARY = rf"(? bases are resolved with native separators. Returns: - A compiled pattern matching ``base`` at a segment boundary, plus an - optional path tail. + A compiled pattern matching ``base`` where it starts a path and ends + at a segment boundary, plus an optional path tail. """ escaped = re.escape(base) if separator_agnostic: escaped = escaped.replace(r"\\", r"[/\\]") - return re.compile(escaped + _SEGMENT_BOUNDARY + _PATH_TAIL) + return re.compile(PATH_START_BOUNDARY + escaped + _SEGMENT_BOUNDARY + _PATH_TAIL) def replace_output_path_matches( @@ -119,6 +139,9 @@ def replace_output_path_matches( match_start = searchable_output.find(searchable_base, search_from) if match_start < 0: break + if match_start > 0 and _PATH_START_BLOCKERS.fullmatch(searchable_output[match_start - 1]): + search_from = match_start + 1 + continue base_end = match_start + len(searchable_base) match_end = base_end diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index c851633fe8..c84db99bdf 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -42,7 +42,7 @@ from deerflow.sandbox.lease import ( ) from deerflow.sandbox.local.local_sandbox import LocalSandbox, escape_powershell_path from deerflow.sandbox.overwrite import unwrap_sandbox -from deerflow.sandbox.path_patterns import build_output_mask_pattern, normalize_mask_tail, replace_output_path_matches +from deerflow.sandbox.path_patterns import PATH_START_BOUNDARY, build_output_mask_pattern, normalize_mask_tail, replace_output_path_matches from deerflow.sandbox.prewarm import claim_sandbox_prewarm from deerflow.sandbox.read_file_contract import ( READ_FILE_EMPTY, @@ -1921,7 +1921,9 @@ def replace_virtual_paths_in_command(command: str, thread_data: ThreadDataState # bare root still matches and the sibling is rewritten into the thread's # host directory — a real path outside the mount contract. Same defect as # #4035 (reverse patterns) and #4053 (masking patterns), mirrored into - # this direction. + # this direction. ``PATH_START_BOUNDARY`` is the left-hand counterpart: + # the root must start a path, so an artifact URL such as + # ``.../artifacts/mnt/user-data/outputs/x.html`` is not rewritten. # # The class mirrors ``LocalSandbox._content_pattern``'s rather than # ``_command_pattern``'s: a virtual root can legitimately be followed by @@ -1933,7 +1935,7 @@ def replace_virtual_paths_in_command(command: str, thread_data: ThreadDataState # boundary above admits PATH-style concatenation, but a tail that ran # through the ``:`` swallowed every later entry of the list, leaving # those virtual paths unresolved in the command. - pattern = re.compile(rf"{re.escape(VIRTUAL_PATH_PREFIX)}(?=/|$|[^\w./-])(/[^\s\"';&|<>():]*)?") + pattern = re.compile(rf"{PATH_START_BOUNDARY}{re.escape(VIRTUAL_PATH_PREFIX)}(?=/|$|[^\w./-])(/[^\s\"';&|<>():]*)?") def replace_user_data_match(match: re.Match) -> str: resolved = replace_virtual_path(match.group(0), thread_data).replace("\\", "/") diff --git a/backend/tests/test_sandbox_path_patterns.py b/backend/tests/test_sandbox_path_patterns.py index 655662f96e..bf7030ef2f 100644 --- a/backend/tests/test_sandbox_path_patterns.py +++ b/backend/tests/test_sandbox_path_patterns.py @@ -11,12 +11,14 @@ 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``. 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. +changed exactly once since then (it now stops at ``:``), and the base gained a +left-hand path-start boundary; the snapshot names those deltas instead of +re-freezing the literals, so any other drift still goes red. """ from __future__ import annotations +import os import re from pathlib import Path @@ -52,6 +54,15 @@ def _with_current_tail(legacy: str) -> str: return legacy.removesuffix(_LEGACY_TAIL) + _CURRENT_TAIL +# The second intentional change: a base must start a path, so it no longer +# matches inside a longer path or URL (``.../artifacts/mnt/user-data/...``). +_CURRENT_START = r"(? str: + return _CURRENT_START + _with_current_tail(legacy) + + _BASES = [ "/host/skills", "/host/dir with spaces", @@ -70,13 +81,13 @@ _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, - apart from the one named tail change. + apart from the named tail and path-start changes. 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 == _with_current_tail(_legacy_tools_pattern(base).pattern) - assert build_output_mask_pattern(base).pattern == _with_current_tail(_legacy_local_pattern(base).pattern) + assert build_output_mask_pattern(base, separator_agnostic=True).pattern == _with_current_rules(_legacy_tools_pattern(base).pattern) + assert build_output_mask_pattern(base).pattern == _with_current_rules(_legacy_local_pattern(base).pattern) def test_separator_agnostic_is_the_only_difference_between_the_two_modes() -> None: @@ -277,3 +288,42 @@ def test_tools_mask_patterns_route_through_the_helper(tmp_path: Path) -> None: for pattern, variant, virtual_base in compiled: assert virtual_base == "/mnt/skills" assert pattern.pattern == build_output_mask_pattern(variant, separator_agnostic=True).pattern + + +@pytest.mark.parametrize("mask", _MASKERS) +@pytest.mark.parametrize( + ("output", "expected"), + [ + # A base inside a longer path is not that path. + ("log at /var/data/app.log", "log at /var/data/app.log"), + ("backup/data/x and ./data/y and ~/data/z", "backup/data/x and ./data/y and ~/data/z"), + ("see https://example.com/data/report.csv", "see https://example.com/data/report.csv"), + # A base that starts a path still maps, including after ``/`` and punctuation. + ("file:///data/report.html", "file:///mnt/data/report.html"), + ("--out=/data/x, PATH=/data/bin:/data/lib", "--out=/mnt/data/x, PATH=/mnt/data/bin:/mnt/data/lib"), + ("/data/a (/data/b)", "/mnt/data/a (/mnt/data/b)"), + ], +) +def test_base_must_start_a_path(mask, output: str, expected: str) -> None: + """The left-hand mirror of the #4035/#4053 segment boundary. + + Without it a mount at ``/data`` rewrites the unrelated ``/var/data/app.log`` + into ``/var/mnt/data/app.log`` before the model sees it. + """ + assert mask(output, "/data", "/mnt/data") == expected + + +def test_local_sandbox_forward_patterns_require_a_path_start() -> None: + sandbox = LocalSandbox("t", [PathMapping(container_path="/mnt/user-data", local_path="/srv/t/user-data")]) + resolved = os.path.realpath("/srv/t/user-data").replace("\\", "/") + url = "http://localhost:2026/api/threads/abc/artifacts/mnt/user-data/outputs/report.html" + + command = sandbox._resolve_paths_in_command(f"curl -s {url} -o /mnt/user-data/outputs/copy.html; cat backup/mnt/user-data/b") + assert url in command + assert "backup/mnt/user-data/b" in command + assert command.count("/mnt/user-data") == 2 + + content = sandbox._resolve_paths_in_content(f'URL = "{url}"\nOUT = "/mnt/user-data/outputs/x"\n') + assert url in content + assert "/mnt/user-data/outputs/x" not in content + assert resolved.split(":", 1)[-1] in content diff --git a/backend/tests/test_sandbox_tools_security.py b/backend/tests/test_sandbox_tools_security.py index f14d1e41e5..4535bf707f 100644 --- a/backend/tests/test_sandbox_tools_security.py +++ b/backend/tests/test_sandbox_tools_security.py @@ -834,6 +834,35 @@ def test_replace_virtual_paths_in_command_does_not_rewrite_prefix_siblings(sibli assert "/tmp/deer-flow/threads/t1" not in result +@pytest.mark.parametrize( + "embedded", + [ + "http://localhost:2026/api/threads/abc/artifacts/mnt/user-data/outputs/report.html", + "https://example.com/mnt/user-data/outputs/a.txt", + "backup/mnt/user-data/outputs/a.txt", + "./mnt/user-data/outputs/a.txt", + ], +) +def test_replace_virtual_paths_in_command_does_not_rewrite_embedded_roots(embedded: str) -> None: + """The left-hand mirror of the prefix-sibling rule above. + + ``/mnt/user-data`` inside a longer path or URL — notably DeerFlow's own + artifact URLs — is not a virtual path. Rewriting it splices the thread's host + directory into the URL, so the request misses and leaks the host layout. + """ + result = replace_virtual_paths_in_command(f"curl -s {embedded}", _THREAD_DATA) + + assert result == f"curl -s {embedded}" + assert "/tmp/deer-flow/threads/t1" not in result + + +def test_replace_virtual_paths_in_command_still_rewrites_roots_that_start_a_path() -> None: + result = replace_virtual_paths_in_command("cat file:///mnt/user-data/outputs/a.txt --out=/mnt/user-data/outputs/b.txt", _THREAD_DATA) + + assert "/mnt/user-data" not in result + assert result.count("/tmp/deer-flow/threads/t1/user-data/outputs/") == 2 + + @pytest.mark.parametrize( ("command", "expected"), [