mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-14 16:08:41 +00:00
fix(sandbox): reverse-resolve forward-slash spellings of Windows host paths (#5373)
* fix(sandbox): reverse-resolve forward-slash spellings of Windows host paths Forward resolution deliberately spells resolved paths with forward slashes in commands and file content (#3869: backslashes break bash escapes), but the reverse scanner anchored its matches on the native backslash base, so on Windows every forward-resolved path that came back in command output or agent-written files leaked the raw host path instead of mapping to its container path. Match separator-agnostically in LocalSandbox like sandbox.tools already does, align the two regex-cache tests with the documented spellings, and refresh the path_patterns rationale comments that described the old asymmetry. * test(sandbox): pin the reverse mask to separator-agnostic matching The flag is the entire Windows fix but is invisible on POSIX CI, so assert the routing kwargs in the direct-helper wiring test — the same pin test_tools_mask_patterns_route_through_the_helper already applies to the sandbox.tools copy. A revert to separator-exact matching now fails on every platform instead of silently reintroducing the host-path leak on Windows. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
parent
7513f16e0e
commit
d5ae3882b6
@ -391,12 +391,19 @@ class LocalSandbox(Sandbox):
|
||||
# Scan directly instead of compiling one regex per thread root. Python's
|
||||
# global regex caches outlive an evicted LocalSandbox and otherwise keep
|
||||
# high-cardinality thread paths resident.
|
||||
#
|
||||
# The base is resolved with native separators, but forward resolution
|
||||
# emits forward-slash spellings in commands and file content (see
|
||||
# ``_resolve_paths_in_command``), so matching must accept both
|
||||
# separators or the model sees raw host paths that no container path
|
||||
# maps back to.
|
||||
result = output
|
||||
for mapping in self._mappings_by_local_specificity:
|
||||
result = replace_output_path_matches(
|
||||
result,
|
||||
self._resolved_local_paths[mapping],
|
||||
self._reverse_resolve_path,
|
||||
separator_agnostic=True,
|
||||
)
|
||||
|
||||
return result
|
||||
|
||||
@ -16,7 +16,10 @@ segment boundary to the reverse patterns and missed the masking patterns, and
|
||||
#4053 had to add the same boundary to the other copy. This module holds the
|
||||
rule once so a third copy cannot silently disagree.
|
||||
|
||||
The two sites are *not* identical, and the difference is deliberate — see
|
||||
Both sites match separator-agnostically: ``sandbox.tools`` derives its bases
|
||||
from ``_path_variants``, and ``LocalSandbox``'s forward resolution spells
|
||||
resolved paths with forward slashes in commands and file content, so the
|
||||
reverse direction has to accept both spellings of the same host path — see
|
||||
``separator_agnostic``.
|
||||
"""
|
||||
|
||||
@ -66,12 +69,12 @@ def build_output_mask_pattern(base: str, *, separator_agnostic: bool = False) ->
|
||||
base: Host path root to match (already resolved by the caller).
|
||||
separator_agnostic: Accept either separator *inside* the base, so a
|
||||
base captured with ``\\`` still matches output that spells the same
|
||||
path with ``/``. ``sandbox.tools`` needs this because it derives its
|
||||
bases from ``_path_variants`` (which yields Windows-style spellings)
|
||||
and matches them against output whose separators it does not
|
||||
control. ``LocalSandbox`` does not: its bases come from filesystem
|
||||
resolution on the running platform, and relaxing them would widen
|
||||
what it masks.
|
||||
path with ``/``. Both call sites need this: ``sandbox.tools``
|
||||
derives its bases from ``_path_variants`` (which yields
|
||||
Windows-style spellings) and matches them against output whose
|
||||
separators it does not control, and ``LocalSandbox``'s forward
|
||||
resolution emits forward-slash spellings on Windows even though its
|
||||
bases are resolved with native separators.
|
||||
|
||||
Returns:
|
||||
A compiled pattern matching ``base`` at a segment boundary, plus an
|
||||
|
||||
@ -794,8 +794,10 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple
|
||||
# ``deerflow.sandbox.path_patterns`` so the static regex path and dynamic
|
||||
# scanner cannot drift.
|
||||
#
|
||||
# ``separator_agnostic=True`` is the one thing this site does differently:
|
||||
# output separators are outside this layer's control.
|
||||
# ``separator_agnostic=True`` is required here: output separators are
|
||||
# outside this layer's control. ``LocalSandbox`` needs it for the same
|
||||
# reason — its forward resolution spells Windows paths with forward
|
||||
# slashes even though its bases are resolved natively.
|
||||
compiled: list[tuple[re.Pattern[str], str, str]] = []
|
||||
for host_base, virtual_base in sources:
|
||||
seen: set[str] = set()
|
||||
|
||||
@ -4,6 +4,7 @@ host-path masking avoids process-global regex retention.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
@ -63,7 +64,9 @@ def test_empty_mappings_yield_no_pattern(tmp_path):
|
||||
|
||||
def test_command_paths_resolved_to_local(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
ws_local = str((tmp_path / "workspace").resolve())
|
||||
# Command resolution spells the local path with forward slashes so bash
|
||||
# never sees backslash escape sequences on Windows hosts.
|
||||
ws_local = str((tmp_path / "workspace").resolve()).replace("\\", "/")
|
||||
out = sb._resolve_paths_in_command("cat /mnt/user-data/workspace/foo.txt")
|
||||
assert out == f"cat {ws_local}/foo.txt"
|
||||
# Calling again uses the cached pattern and produces the same result.
|
||||
@ -202,8 +205,11 @@ def test_resolved_paths_and_sorted_views_are_cached(tmp_path):
|
||||
def test_forward_resolution_behavior_unchanged(tmp_path):
|
||||
sb = _make_sandbox(tmp_path)
|
||||
ws_local = str((tmp_path / "workspace").resolve())
|
||||
# _resolve_path feeds file operations, so the resolved path keeps the
|
||||
# native separator spelling (os.path.join/realpath).
|
||||
expected = os.path.join(ws_local, "sub", "foo.txt")
|
||||
# Container path resolves to the mapped local path.
|
||||
assert sb._resolve_path("/mnt/user-data/workspace/sub/foo.txt") == f"{ws_local}/sub/foo.txt"
|
||||
assert sb._resolve_path("/mnt/user-data/workspace/sub/foo.txt") == expected
|
||||
# An unmapped path is returned unchanged.
|
||||
assert sb._resolve_path("/etc/hosts") == "/etc/hosts"
|
||||
|
||||
|
||||
@ -161,6 +161,9 @@ def test_separator_agnostic_replacer_avoids_normalization_without_backslashes()
|
||||
|
||||
|
||||
def test_local_sandbox_reverse_mask_routes_through_the_direct_helper(tmp_path: Path, monkeypatch) -> None:
|
||||
"""And it must stay separator-agnostic: forward resolution spells Windows
|
||||
host paths with forward slashes, so a revert to separator-exact matching
|
||||
would reintroduce the host-path leak with no POSIX-visible signal."""
|
||||
local = tmp_path / "skills"
|
||||
local.mkdir()
|
||||
sandbox = LocalSandbox(
|
||||
@ -169,17 +172,17 @@ def test_local_sandbox_reverse_mask_routes_through_the_direct_helper(tmp_path: P
|
||||
)
|
||||
|
||||
resolved = str(Path(local).resolve())
|
||||
calls: list[tuple[str, str]] = []
|
||||
calls: list[tuple[str, str, dict]] = []
|
||||
original = path_patterns_module.replace_output_path_matches
|
||||
|
||||
def recording_replacer(output, base, replacement, **kwargs):
|
||||
calls.append((output, base))
|
||||
calls.append((output, base, kwargs))
|
||||
return original(output, base, replacement, **kwargs)
|
||||
|
||||
monkeypatch.setattr(local_sandbox_module, "replace_output_path_matches", recording_replacer)
|
||||
|
||||
assert sandbox._reverse_resolve_paths_in_output(f"read {resolved}/SKILL.md") == "read /mnt/skills/SKILL.md"
|
||||
assert calls == [(f"read {resolved}/SKILL.md", resolved)]
|
||||
assert calls == [(f"read {resolved}/SKILL.md", resolved, {"separator_agnostic": True})]
|
||||
|
||||
|
||||
def test_tools_mask_patterns_route_through_the_helper(tmp_path: Path) -> None:
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user