From 08fd218b831331821d4908d663c21f571cb2cb4c Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Sun, 12 Jul 2026 09:01:22 -0700 Subject: [PATCH] fix(sandbox): use os.sep in reverse-resolve containment check on Windows (#4058) * fix(sandbox): use os.sep in reverse-resolve containment check on Windows Path.resolve() always renders with the native separator (backslash on Windows), but _reverse_resolve_path's containment check hardcoded a "/" suffix when testing whether a resolved path is nested under a mapping's local root. Only the exact-root case (no separator needed) ever matched; every nested path fell through to the "no mapping found" branch and returned the raw host path -- leaking the real username and full directory tree into list_dir/glob/grep results and bash output masking instead of the virtual /mnt/user-data/... path. _is_read_only_path already does the equivalent check correctly via os.sep, so this aligns _reverse_resolve_path with that pattern: the containment check now compares with os.sep, and the extracted relative portion is normalized to forward slashes before being spliced into the (always POSIX-style) container path. Also fixes a same-file cosmetic bug in list_dir's virtual sub-directory overlay: it compared a bare child name (e.g. "workspace") against a set of full container paths, so the already-listed guard never matched and a mount whose subdirectory the underlying scan already found (the common case for /mnt/user-data/workspace, uploads, outputs) was appended a second time. Continues the same separator-bug class already fixed in this file by #3869 (forward-direction command resolution) and #4035 (reverse regex-boundary matching); neither touched this containment check. * test(sandbox): add host-OS-independent regression test for the os.sep containment fix _reverse_resolve_path's os.sep containment check (and the paired lstrip(os.sep).replace(os.sep, "/") extraction) has no test that would fail if reverted: backend CI runs only on ubuntu-latest, where os.sep == "/" makes the pre-fix hardcoded "/" and the current os.sep form observationally identical, so a plain POSIX-path test can't discriminate between them. Add a test that forces the Windows code path independent of host OS by monkeypatching os.sep to "\" and stubbing both the module's Path name and the sandbox's cached _resolved_local_paths to return backslash-joined strings, mirroring what real WindowsPath.resolve() produces -- without touching the filesystem or requiring an actual Windows host. Verified this fails with the raw host path leaking through when the os.sep fix is reverted to the hardcoded "/" form, and passes with the fix in place. --- .../deerflow/sandbox/local/local_sandbox.py | 27 +++++++-- .../test_local_sandbox_path_regex_cache.py | 56 +++++++++++++++++++ ...est_local_sandbox_virtual_path_contract.py | 25 +++++++++ 3 files changed, 103 insertions(+), 5 deletions(-) diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index e3af31c49..8a394b4dc 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -344,9 +344,20 @@ class LocalSandbox(Sandbox): # Try each mapping (longest local path first for more specific matches) for mapping in self._mappings_by_local_specificity: local_path_resolved = self._resolved_local_paths[mapping] - if path_str == local_path_resolved or path_str.startswith(local_path_resolved + "/"): - # Replace the local path prefix with container path - relative = path_str[len(local_path_resolved) :].lstrip("/") + # ``Path.resolve()`` always renders with the native separator + # (backslash on Windows), regardless of the forward-slash + # normalization above, so the containment check must compare with + # ``os.sep`` here too -- mirroring ``_is_read_only_path`` -- instead + # of a hardcoded "/". A hardcoded "/" can never match a + # backslash-joined nested path on Windows, so every nested path + # silently fell through to the "no mapping found" branch below and + # leaked the raw host path (real username, full directory tree). + if path_str == local_path_resolved or path_str.startswith(local_path_resolved + os.sep): + # Replace the local path prefix with container path. Container + # paths are always POSIX-style, so the extracted relative + # portion (native-separated on Windows) is normalized to + # forward slashes before being spliced in. + relative = path_str[len(local_path_resolved) :].lstrip(os.sep).replace(os.sep, "/") resolved = f"{mapping.container_path}/{relative}" if relative else mapping.container_path return resolved @@ -650,8 +661,14 @@ class LocalSandbox(Sandbox): # 2. It is NOT already present in the result (was skipped by list_dir) if mapping.container_path.startswith(container_path + "/"): child_rel = mapping.container_path[len(container_path) + 1 :] - # Only direct children (no further slashes), e.g. "public", "custom" - if "/" not in child_rel and child_rel not in existing_dirs: + # Only direct children (no further slashes), e.g. "public", "custom". + # Compare the mapping's full container path -- not the bare child + # name -- against existing_dirs, which holds full paths (e.g. + # "/mnt/user-data/workspace"). Comparing the bare name here would + # never match, so an already-listed mount (the common case: real + # nested workspace/uploads/outputs subdirectories under + # /mnt/user-data) would be appended a second time. + if "/" not in child_rel and mapping.container_path.rstrip("/") not in existing_dirs: # Verify the host path exists so we don't add phantom entries try: if Path(mapping.local_path).resolve().is_dir(): diff --git a/backend/tests/test_local_sandbox_path_regex_cache.py b/backend/tests/test_local_sandbox_path_regex_cache.py index f63ccaebf..97ddebb02 100644 --- a/backend/tests/test_local_sandbox_path_regex_cache.py +++ b/backend/tests/test_local_sandbox_path_regex_cache.py @@ -9,6 +9,7 @@ from pathlib import Path import pytest +from deerflow.sandbox.local import local_sandbox as local_sandbox_module from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping @@ -135,6 +136,61 @@ def test_reverse_resolve_translates_a_bare_root_at_end_of_output(tmp_path, prefi assert skills_local not in out +def test_reverse_resolve_path_matches_windows_backslash_containment(monkeypatch): + """Regression for the os.sep containment fix in ``_reverse_resolve_path``. + + ``Path.resolve()`` always renders with the native separator (backslash on + Windows). The containment check used to hardcode a ``"/"`` suffix, so a + backslash-joined nested path could never satisfy + ``path_str.startswith(local_path_resolved + "/")`` on Windows and silently + fell through to the "no mapping found" branch, leaking the raw host path + (real username, full directory tree) instead of the virtual + ``/mnt/user-data/...`` path. + + CI runs only on ``ubuntu-latest`` (``os.sep == "/"``), where the pre-fix and + post-fix code are observationally identical -- neither the hardcoded ``"/"`` + nor ``os.sep`` behave any differently there, so a test that just calls + ``_reverse_resolve_path`` on real POSIX paths cannot discriminate. To force + the Windows code path independent of host OS, ``os.sep`` is monkeypatched to + ``"\\"`` and both the module's ``Path`` name and the sandbox's cached + ``_resolved_local_paths`` are stubbed to return backslash-joined strings -- + exactly what real ``WindowsPath.resolve()`` produces -- without touching the + real filesystem or requiring an actual Windows host. + """ + sb = LocalSandbox( + id="windows-sep-test", + path_mappings=[ + PathMapping(container_path="/mnt/user-data/workspace", local_path="C:\\Users\\test\\workspace"), + ], + ) + mapping = sb.path_mappings[0] + + monkeypatch.setattr(local_sandbox_module.os, "sep", "\\") + # Bypass the real (POSIX) filesystem resolution this cached_property would + # otherwise perform and pin it directly to the Windows-resolved root. + sb._resolved_local_paths = {mapping: "C:\\Users\\test\\workspace"} + + class _FakeWindowsPath: + """Stand-in for ``Path`` inside ``_reverse_resolve_path``. Mimics + ``WindowsPath.resolve()`` -- a backslash-joined ``str()`` -- without + touching the real filesystem, so this runs identically on Linux CI.""" + + def __init__(self, raw: str) -> None: + self._raw = raw + + def resolve(self) -> _FakeWindowsPath: + return _FakeWindowsPath(self._raw.replace("/", "\\")) + + def __str__(self) -> str: + return self._raw + + monkeypatch.setattr(local_sandbox_module, "Path", _FakeWindowsPath) + + result = sb._reverse_resolve_path("C:\\Users\\test\\workspace\\sub\\f.txt") + + assert result == "/mnt/user-data/workspace/sub/f.txt" + + def test_resolved_paths_and_sorted_views_are_cached(tmp_path): sb = _make_sandbox(tmp_path) # Resolved-local map and sorted views are computed once and reused. diff --git a/backend/tests/test_local_sandbox_virtual_path_contract.py b/backend/tests/test_local_sandbox_virtual_path_contract.py index be9a73656..6ffa6078c 100644 --- a/backend/tests/test_local_sandbox_virtual_path_contract.py +++ b/backend/tests/test_local_sandbox_virtual_path_contract.py @@ -143,6 +143,31 @@ def test_execute_command_lists_aggregate_user_data_root(provider): assert "outputs" in output +def test_list_dir_on_user_data_root_does_not_duplicate_subdir_mounts(provider): + """Regression: ``list_dir``'s virtual sub-directory overlay must not + double-list a mount that the underlying scan already found. + + The overlay compared a bare child name (e.g. "workspace") against + ``existing_dirs``, which holds full container paths (e.g. + "/mnt/user-data/workspace") -- so the containment guard never matched and + each of workspace/uploads/outputs (real nested subdirectories the plain + scan already discovers) was appended a second time. + """ + sandbox_id = provider.acquire("alpha") + sbx = provider.get(sandbox_id) + # Touch all three subdirs so they materialise on disk and are found by the + # underlying (non-overlay) directory scan. + sbx.write_file("/mnt/user-data/workspace/.keep", "") + sbx.write_file("/mnt/user-data/uploads/.keep", "") + sbx.write_file("/mnt/user-data/outputs/.keep", "") + + entries = sbx.list_dir("/mnt/user-data") + + for subdir in ("workspace", "uploads", "outputs"): + matches = [e for e in entries if e.rstrip("/") == f"/mnt/user-data/{subdir}"] + assert len(matches) == 1, f"{subdir} listed {len(matches)} time(s), expected exactly 1: {entries}" + + def test_update_file_with_virtual_path_for_remote_sync_scenario(provider): """This is the exact code path used by ``uploads.py:282`` and ``feishu.py:389``.