diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 202474605..ba0bca9ca 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -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) diff --git a/backend/tests/test_sandbox_tools_security.py b/backend/tests/test_sandbox_tools_security.py index 43af56b01..b30e98d8d 100644 --- a/backend/tests/test_sandbox_tools_security.py +++ b/backend/tests/test_sandbox_tools_security.py @@ -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)."""