mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-26 16:07:53 +00:00
fix(sandbox): guard the output-masking regex with a segment boundary (#4053)
* fix(sandbox): guard the output-masking regex with a segment boundary `_compiled_mask_patterns` builds the same class of host→virtual matcher as `LocalSandbox._reverse_output_patterns`, but without the segment-boundary lookahead that one carries. The trailing group needs a separator to consume anything, so when the character after a host base is `-`, `.`, `_`, a digit or a letter, the group matches empty and the regex still matches the bare base. `replace_match` then takes its `matched_path == base` branch and rewrites the sibling: with `/mnt/skills` mounted at `.../skills`, output naming a sibling `.../skills-extra/data.txt` is handed to the model as `/mnt/skills-extra/data.txt` — a container path forward resolution explicitly refuses to map back, so reading it raises FileNotFoundError. This is the sibling site of #4035, which fixed the identical bug in `local_sandbox.py`. That PR's scope argument enumerated the prefix matchers in that file and missed this one; `mask_local_paths_in_output` runs on every glob/grep match and on local bash output. The boundary class mirrors `_content_pattern`'s, not `_command_pattern`'s: this runs over arbitrary command output, where a base can legitimately be followed by `,`, `:` or `\`, all of which the shell-oriented class rejects. * test(sandbox): anchor the sibling boundary at the ACP source too The sibling-rejection cases only fed the skills source. `_compiled_mask_patterns` builds every source's matcher in one loop, so the ACP workspace carried the same defect: nothing maps its parent, and `/mnt/acp-workspace-backup/hello.py` is unresolvable in both directions. User-data is the exception and is now pinned as such: `_thread_virtual_to_actual_mappings` also maps the virtual root `/mnt/user-data` to the three dirs' common parent, so a sibling of `outputs` is still inside a mount and has a real virtual path — the output is byte-identical with and without the boundary. That test is green on main; it guards the boundary from being narrowed into one that stops translating a mapped path. Reverting only `boundary` turns the 5 skills + 4 ACP cases red and leaves the 3 user-data cases green.
This commit is contained in:
parent
97935b081b
commit
1df9abc924
@ -692,6 +692,20 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple
|
||||
glob/grep match, so without this the same patterns are recompiled per
|
||||
match.
|
||||
"""
|
||||
# Same segment-boundary lookahead as ``LocalSandbox._reverse_output_patterns``
|
||||
# (#4035), so a host base does not match inside a sibling that merely shares
|
||||
# its prefix (``.../skills`` inside ``.../skills-extra``). Without it the
|
||||
# regex yields the bare base, which then *equals* ``base`` in
|
||||
# ``replace_match`` and so the sibling is rewritten to a container path that
|
||||
# forward resolution refuses to map back.
|
||||
#
|
||||
# The class mirrors ``_content_pattern``'s: this runs over arbitrary command
|
||||
# output, where a base can legitimately be followed by ``,`` ``:`` or ``\``.
|
||||
# ``$`` is load-bearing — output ending exactly at a base would otherwise
|
||||
# fail the lookahead and be emitted as the raw host path.
|
||||
boundary = r"(?=/|$|[^\w./-])"
|
||||
tail = r"(?:[/\\][^\s\"';&|<>()]*)?"
|
||||
|
||||
compiled: list[tuple[re.Pattern[str], str, str]] = []
|
||||
for host_base, virtual_base in sources:
|
||||
seen: set[str] = set()
|
||||
@ -705,7 +719,7 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple
|
||||
continue
|
||||
seen.add(variant)
|
||||
escaped = re.escape(variant).replace(r"\\", r"[/\\]")
|
||||
compiled.append((re.compile(escaped + r"(?:[/\\][^\s\"';&|<>()]*)?"), variant, virtual_base))
|
||||
compiled.append((re.compile(escaped + boundary + tail), variant, virtual_base))
|
||||
return tuple(compiled)
|
||||
|
||||
|
||||
|
||||
@ -115,6 +115,115 @@ def test_mask_local_paths_in_output_hides_skills_host_paths() -> None:
|
||||
assert "/mnt/skills/public/bootstrap/SKILL.md" in masked
|
||||
|
||||
|
||||
@pytest.mark.parametrize("suffix", ["-extra/data.txt", "2/x", ".bak", "foo", "_backup/y"])
|
||||
def test_mask_local_paths_does_not_match_inside_longer_sibling(suffix: str) -> None:
|
||||
"""A host base must not match inside a sibling that merely shares its prefix.
|
||||
|
||||
The trailing group needs a separator to consume anything, so without a
|
||||
segment-boundary lookahead the regex matches the bare base and
|
||||
``replace_match`` takes its ``matched_path == base`` branch -- rewriting
|
||||
``.../skills-extra/data.txt`` to ``/mnt/skills-extra/data.txt``, a container
|
||||
path forward resolution refuses to map back. Reverse-direction mirror of
|
||||
``LocalSandbox._reverse_output_patterns`` (#4035).
|
||||
"""
|
||||
with (
|
||||
patch("deerflow.sandbox.tools._get_skills_container_path", return_value="/mnt/skills"),
|
||||
patch("deerflow.sandbox.tools._get_skills_host_path", return_value="/home/user/deer-flow/skills"),
|
||||
):
|
||||
output = f"found /home/user/deer-flow/skills{suffix}"
|
||||
masked = mask_local_paths_in_output(output, None)
|
||||
|
||||
assert masked == output
|
||||
assert "/mnt/skills" not in masked
|
||||
|
||||
|
||||
@pytest.mark.parametrize("suffix", ["-backup/hello.py", "2/hello.py", ".old", "_tmp/x"])
|
||||
def test_mask_local_paths_does_not_match_inside_longer_acp_sibling(suffix: str) -> None:
|
||||
"""Same bug, second source: the ACP workspace has no enclosing virtual root.
|
||||
|
||||
``_compiled_mask_patterns`` builds every source's matcher, so the ACP
|
||||
workspace carried the same defect as skills -- and unlike user-data (see
|
||||
below) nothing maps its parent, so ``/mnt/acp-workspace-backup/hello.py``
|
||||
is unresolvable in both directions.
|
||||
"""
|
||||
acp_host = "/home/user/.deer-flow/acp-workspace"
|
||||
with patch("deerflow.sandbox.tools._get_acp_workspace_host_path", return_value=acp_host):
|
||||
output = f"copied {acp_host}{suffix}"
|
||||
masked = mask_local_paths_in_output(output, _THREAD_DATA)
|
||||
|
||||
assert masked == output
|
||||
assert "/mnt/acp-workspace" not in masked
|
||||
|
||||
|
||||
@pytest.mark.parametrize("suffix", ["2/report.txt", ".bak/report.txt", "-old"])
|
||||
def test_mask_local_paths_user_data_sibling_is_carried_by_the_virtual_root(suffix: str) -> None:
|
||||
"""User-data siblings are benign -- and must stay that way.
|
||||
|
||||
``_thread_virtual_to_actual_mappings`` also maps the virtual root
|
||||
``/mnt/user-data`` to the three dirs' common parent, so a sibling of
|
||||
``outputs`` is still *inside* a mount and has a real virtual path. Whichever
|
||||
pattern wins -- the bare ``outputs`` base (pre-#4053) or the root (post-) --
|
||||
the string is the same, so the boundary changes nothing here.
|
||||
|
||||
Green on ``main`` too: this is not a bug anchor, it guards the boundary from
|
||||
being narrowed into one that would stop translating a mapped path.
|
||||
"""
|
||||
masked = mask_local_paths_in_output(f"wrote /tmp/deer-flow/threads/t1/user-data/outputs{suffix}", _THREAD_DATA)
|
||||
|
||||
assert masked == f"wrote /mnt/user-data/outputs{suffix}"
|
||||
assert replace_virtual_path(f"/mnt/user-data/outputs{suffix}", _THREAD_DATA) == f"/tmp/deer-flow/threads/t1/user-data/outputs{suffix}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("boundary", "expected"),
|
||||
[
|
||||
(", done", "/mnt/skills, done"),
|
||||
(":/other", "/mnt/skills:/other"),
|
||||
(" tail", "/mnt/skills tail"),
|
||||
('"quoted', '/mnt/skills"quoted'),
|
||||
# A backslash is consumed by the trailing group (Windows paths match in
|
||||
# full, separator normalised) rather than acting as a terminator -- but
|
||||
# it must still reach the trailing group, which needs the lookahead to
|
||||
# admit it first.
|
||||
("\\win", "/mnt/skills/win"),
|
||||
],
|
||||
)
|
||||
def test_mask_local_paths_still_matches_base_before_non_slash_boundaries(boundary: str, expected: str) -> None:
|
||||
"""The lookahead must not narrow away boundaries that translate today.
|
||||
|
||||
This runs over arbitrary command output, where a base can legitimately be
|
||||
followed by a comma (prose), a colon (PATH-style concatenation) or a
|
||||
backslash (Windows separator). Borrowing the shell-oriented class from
|
||||
``_command_pattern`` -- ``(?=/|$|[\\s"';&|<>()])`` -- admits none of the
|
||||
three, so the lookahead would fail and the raw host path would be emitted.
|
||||
"""
|
||||
with (
|
||||
patch("deerflow.sandbox.tools._get_skills_container_path", return_value="/mnt/skills"),
|
||||
patch("deerflow.sandbox.tools._get_skills_host_path", return_value="/home/user/deer-flow/skills"),
|
||||
):
|
||||
masked = mask_local_paths_in_output(f"root is /home/user/deer-flow/skills{boundary}", None)
|
||||
|
||||
assert masked == f"root is {expected}"
|
||||
assert "/home/user/deer-flow/skills" not in masked
|
||||
|
||||
|
||||
@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.
|
||||
|
||||
Without it the lookahead fails and the raw host path is handed to the model
|
||||
-- the leak this function exists to prevent.
|
||||
"""
|
||||
with (
|
||||
patch("deerflow.sandbox.tools._get_skills_container_path", return_value="/mnt/skills"),
|
||||
patch("deerflow.sandbox.tools._get_skills_host_path", return_value="/home/user/deer-flow/skills"),
|
||||
):
|
||||
masked = mask_local_paths_in_output(f"{prefix}/home/user/deer-flow/skills", None)
|
||||
|
||||
assert masked == f"{prefix}/mnt/skills"
|
||||
assert "/home/user/deer-flow/skills" not in masked
|
||||
|
||||
|
||||
def test_mask_local_paths_compiled_patterns_are_cached() -> None:
|
||||
"""The compiled patterns for a given source set are built once and reused
|
||||
(mask runs once per glob/grep match, so this avoids per-match recompiles)."""
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user