From 1df9abc9247692e7b5b3cc04ec938a6e12157f53 Mon Sep 17 00:00:00 2001 From: Aari Date: Sat, 11 Jul 2026 23:32:11 +0800 Subject: [PATCH] fix(sandbox): guard the output-masking regex with a segment boundary (#4053) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .../harness/deerflow/sandbox/tools.py | 16 ++- backend/tests/test_sandbox_tools_security.py | 109 ++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) 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)."""