mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-27 21:50:36 +00:00
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 <willem.jiang@gmail.com>
This commit is contained in:
parent
f7d02a7515
commit
797e093ddc
@ -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
|
||||
|
||||
@ -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"(?<!{_PATH_START_BLOCKERS.pattern})"
|
||||
|
||||
# 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.
|
||||
@ -83,13 +103,13 @@ def build_output_mask_pattern(base: str, *, separator_agnostic: bool = False) ->
|
||||
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
|
||||
|
||||
@ -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("\\", "/")
|
||||
|
||||
@ -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"(?<![\w.~-])"
|
||||
|
||||
|
||||
def _with_current_rules(legacy: str) -> 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
|
||||
|
||||
@ -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"),
|
||||
[
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user