mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-10 22:18:59 +00:00
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.
This commit is contained in:
parent
c82fba41d9
commit
08fd218b83
@ -344,9 +344,20 @@ class LocalSandbox(Sandbox):
|
|||||||
# Try each mapping (longest local path first for more specific matches)
|
# Try each mapping (longest local path first for more specific matches)
|
||||||
for mapping in self._mappings_by_local_specificity:
|
for mapping in self._mappings_by_local_specificity:
|
||||||
local_path_resolved = self._resolved_local_paths[mapping]
|
local_path_resolved = self._resolved_local_paths[mapping]
|
||||||
if path_str == local_path_resolved or path_str.startswith(local_path_resolved + "/"):
|
# ``Path.resolve()`` always renders with the native separator
|
||||||
# Replace the local path prefix with container path
|
# (backslash on Windows), regardless of the forward-slash
|
||||||
relative = path_str[len(local_path_resolved) :].lstrip("/")
|
# 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
|
resolved = f"{mapping.container_path}/{relative}" if relative else mapping.container_path
|
||||||
return resolved
|
return resolved
|
||||||
|
|
||||||
@ -650,8 +661,14 @@ class LocalSandbox(Sandbox):
|
|||||||
# 2. It is NOT already present in the result (was skipped by list_dir)
|
# 2. It is NOT already present in the result (was skipped by list_dir)
|
||||||
if mapping.container_path.startswith(container_path + "/"):
|
if mapping.container_path.startswith(container_path + "/"):
|
||||||
child_rel = mapping.container_path[len(container_path) + 1 :]
|
child_rel = mapping.container_path[len(container_path) + 1 :]
|
||||||
# Only direct children (no further slashes), e.g. "public", "custom"
|
# Only direct children (no further slashes), e.g. "public", "custom".
|
||||||
if "/" not in child_rel and child_rel not in existing_dirs:
|
# 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
|
# Verify the host path exists so we don't add phantom entries
|
||||||
try:
|
try:
|
||||||
if Path(mapping.local_path).resolve().is_dir():
|
if Path(mapping.local_path).resolve().is_dir():
|
||||||
|
|||||||
@ -9,6 +9,7 @@ from pathlib import Path
|
|||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
|
from deerflow.sandbox.local import local_sandbox as local_sandbox_module
|
||||||
from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping
|
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
|
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):
|
def test_resolved_paths_and_sorted_views_are_cached(tmp_path):
|
||||||
sb = _make_sandbox(tmp_path)
|
sb = _make_sandbox(tmp_path)
|
||||||
# Resolved-local map and sorted views are computed once and reused.
|
# Resolved-local map and sorted views are computed once and reused.
|
||||||
|
|||||||
@ -143,6 +143,31 @@ def test_execute_command_lists_aggregate_user_data_root(provider):
|
|||||||
assert "outputs" in output
|
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):
|
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``.
|
"""This is the exact code path used by ``uploads.py:282`` and ``feishu.py:389``.
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user