mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-09 21:49:37 +00:00
fix(sandbox): scope MSYS path conversion exclusions (#5003)
* Preserve Windows CLI compatibility for local sandbox commands MSYS path conversion must remain disabled for DeerFlow virtual paths, but applying a blanket environment override to every POSIX command breaks host-native CLI shims on Windows. Limit MSYS argument-conversion exclusions to safe non-root virtual path prefixes, omit values that would broaden the exclusion pattern, and document the contract. Constraint: Preserve the virtual-path protection introduced by #2765/#2766 Rejected: Disable MSYS conversion for every command | breaks Windows CLI shims Rejected: Toggle blanket conversion only for commands containing virtual paths | host CLIs can receive virtual-path arguments and still need normal conversion for their own paths Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep regression coverage for virtual-path arguments, root mounts, and host-native CLI launchers Tested: test_local_sandbox_encoding.py (12 passed); related sandbox suite (197 passed, 8 skipped, 7 failures matching origin/main); ruff check; ruff format --check; git diff --check; direct LocalSandbox CLI and virtual-path smoke tests Not-tested: Full offline suite completion; stopped at 6% after unrelated Windows and optional-runtime failures Related: #2765 Related: #2766 * Keep MSYS regression tests portable across CI operating systems The Windows-shell environment tests patched os.name to nt while mounting Windows-specific paths. On Linux and macOS, pathlib then attempted to construct WindowsPath during command resolution or output masking, so the backend merge gate failed before exercising the environment contract. Stub the exclusion boundary in execute-command tests and retain mapping-specific filtering coverage in the helper test. Constraint: Backend unit tests run on Linux, while the behavior under test is Windows-only Rejected: Skip the tests outside Windows | would remove CI coverage of the environment contract Rejected: Patch pathlib internals | couples tests to implementation details and hides the platform boundary Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep OS-specific subprocess assertions independent from host-path resolution Tested: test_local_sandbox_encoding.py (12 passed); ruff check; ruff format --check; git diff --check Not-tested: Linux runner execution locally because Docker Desktop is unavailable and WSL cannot access this linked worktree Related: #5003 Related: https://github.com/bytedance/deer-flow/pullrequestreview-5013380238
This commit is contained in:
georgelichen
GitHub
parent
2a261d2276
commit
1c219b6864
@ -1203,7 +1203,7 @@ an explicit **Load full file** action before fetching the remainder or mounting
|
||||
the full code editor. Active HTML, XHTML, and SVG artifacts remain forced
|
||||
downloads at the Gateway boundary.
|
||||
|
||||
With `AioSandboxProvider`, shell execution runs inside isolated containers. With `LocalSandboxProvider`, file tools still map to per-thread directories on the host, but host `bash` is disabled by default because it is not a secure isolation boundary. Re-enable host bash only for fully trusted local workflows. Host bash commands have a wall-clock timeout, and long-lived processes should be started in the background with output redirected to a workspace log.
|
||||
With `AioSandboxProvider`, shell execution runs inside isolated containers. With `LocalSandboxProvider`, file tools still map to per-thread directories on the host, but host `bash` is disabled by default because it is not a secure isolation boundary. Re-enable host bash only for fully trusted local workflows. Host bash commands have a wall-clock timeout, and long-lived processes should be started in the background with output redirected to a workspace log. On Windows, Git Bash/MSYS argument-conversion exclusions are limited to safe non-root virtual path prefixes, so host-native CLI launchers retain their normal MSYS compatibility.
|
||||
|
||||
`AioSandboxProvider` normally detects thread-data mounts from its backend: local
|
||||
containers use the mounted gateway directories, while remote/provisioner
|
||||
|
||||
@ -5,7 +5,7 @@
|
||||
**Authorization gate** (`sandbox:execute`, RFC #4063 Phase 3): every sandbox acquisition passes through `authorize_sandbox_execution` (`deerflow/authz/sandbox_authz.py`) - a binary `authorize(principal, "sandbox", "execute", target="*")` check before `provider.acquire`. The gate lives at the single acquisition entry point (`ensure_sandbox_initialized` / `_acquire_sandbox_async` in `tools.py`, and `SandboxMiddleware.before_agent` / `abefore_agent`), so it cannot be bypassed regardless of which sandbox-dependent tool triggers it; the reuse path (sandbox already in state) skips the re-check. Deny raises `SandboxAuthorizationError` (`sandbox/exceptions.py`), which propagates out of tool execution as a friendly error `ToolMessage` ("sandbox execution is not permitted for your role") - the eager path catches it and skips acquisition instead, deferring the deny to the first sandbox-touching tool call so both paths share the same semantics. Provider errors (both `authorize()` and provider resolution) follow `authorization.fail_closed` / `fail_open`; no readable `config.yaml` or `authorization.enabled: false` makes the gate a no-op (`safe_app_config` tolerates missing config). Gateway auxiliary sync paths (uploads/artifacts routers) call `try_acquire_sandbox_for_request` (`app/gateway/authz.py`), which gates via `authorize_sandbox_for_request` and skips the sync on deny - the upload/artifact edit itself still succeeds. Tests: `tests/test_sandbox_authorization.py`.
|
||||
**Environment policy** (`sandbox/env_policy.py`): `execute_command` no longer inherits the full `os.environ`. `build_sandbox_env()` scrubs secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASS*`/`*CREDENTIAL*`) from the inherited environment before layering injected request secrets on top, so platform credentials (e.g. `OPENAI_API_KEY`) never leak into skill subprocesses. Benign vars (`PATH`, `HOME`, `LANG`, `VIRTUAL_ENV`, ...) are preserved.
|
||||
**Implementations**:
|
||||
- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Public, custom, legacy, and managed integration skill mappings point at stable enabled-only projection roots rather than raw skill directories.
|
||||
- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Public, custom, legacy, and managed integration skill mappings point at stable enabled-only projection roots rather than raw skill directories. On Windows, Git Bash/MSYS argument-conversion exclusions are limited to safe non-root virtual path prefixes; do not restore a blanket conversion disable, because host-native CLI launchers need normal MSYS path conversion for their own installation paths.
|
||||
- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths — it never touches the ownership store (that would be blocking IO on the event loop); ownership is published on acquire/reclaim and refreshed off the event loop by the dedicated renewal thread (`_renew_owned_leases`). `uses_thread_data_mounts` defaults to backend detection (`LocalContainerBackend=True`, remote/provisioner backends=False), while the optional `sandbox.thread_data_mounts` boolean takes precedence for deployments that guarantee the Gateway and sandbox share the same thread user-data directories. Setting it `true` skips upload-time sandbox acquire/sync; a false positive leaves uploads unavailable to the sandbox. Local-container and hostPath-provisioner mounts use the same stable skill projection roots; PVC-backed skills remain governed by the operator-supplied PVC layout until PVC materialization is implemented. Readiness probes and `agent_sandbox` clients classify loopback/private IPs, single-label cluster hosts, and Docker/Podman internal hostnames as direct control-plane destinations and set `trust_env=False`; external FQDNs and public IPs retain environment proxy support.
|
||||
- `E2BSandboxProvider` (`packages/harness/deerflow/community/e2b_sandbox/`) provides E2B remote isolation.
|
||||
New sandboxes receive a one-shot upload from the enabled-only public, custom,
|
||||
|
||||
@ -102,6 +102,25 @@ class LocalSandbox(Sandbox):
|
||||
shell_name = LocalSandbox._shell_name(shell)
|
||||
return shell_name in {"sh.exe", "bash.exe"} and any(part in normalized for part in ("/git/", "/mingw", "/msys"))
|
||||
|
||||
def _msys_path_conversion_exclusions(self) -> str:
|
||||
"""Return the MSYS argument prefixes owned by this sandbox.
|
||||
|
||||
The blanket conversion disable introduced for #2765 also affects child
|
||||
processes launched by Git Bash, including Windows-native CLI shims that
|
||||
need normal MSYS path conversion for their own installation paths.
|
||||
Excluding only the configured virtual roots preserves DeerFlow path
|
||||
arguments without changing unrelated child-process behavior. Root and
|
||||
values containing MSYS exclusion syntax are omitted because they would
|
||||
broaden the exclusion beyond one virtual path prefix.
|
||||
"""
|
||||
safe_roots: dict[str, None] = {}
|
||||
for mapping in self.path_mappings:
|
||||
root = mapping.container_path.rstrip("/")
|
||||
if not root or not root.startswith("/") or ";" in root or "*" in root:
|
||||
continue
|
||||
safe_roots[root] = None
|
||||
return ";".join(safe_roots)
|
||||
|
||||
@staticmethod
|
||||
def _find_first_available_shell(candidates: tuple[str, ...]) -> str | None:
|
||||
"""Return the first executable shell path or command found from candidates."""
|
||||
@ -496,11 +515,12 @@ class LocalSandbox(Sandbox):
|
||||
else:
|
||||
args = [shell, "-c", resolved_command]
|
||||
if self._is_msys_shell(shell):
|
||||
sandbox_env = {
|
||||
**sandbox_env,
|
||||
"MSYS_NO_PATHCONV": "1",
|
||||
"MSYS2_ARG_CONV_EXCL": "*",
|
||||
}
|
||||
exclusions = self._msys_path_conversion_exclusions()
|
||||
if exclusions:
|
||||
sandbox_env = {
|
||||
**sandbox_env,
|
||||
"MSYS2_ARG_CONV_EXCL": exclusions,
|
||||
}
|
||||
|
||||
try:
|
||||
result = subprocess.run(
|
||||
|
||||
@ -2,7 +2,7 @@ import builtins
|
||||
from types import SimpleNamespace
|
||||
|
||||
import deerflow.sandbox.local.local_sandbox as local_sandbox
|
||||
from deerflow.sandbox.local.local_sandbox import LocalSandbox
|
||||
from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping
|
||||
|
||||
|
||||
def _open(base, file, mode="r", *args, **kwargs):
|
||||
@ -115,7 +115,7 @@ def test_execute_command_uses_powershell_command_mode_on_windows(monkeypatch):
|
||||
]
|
||||
|
||||
|
||||
def test_execute_command_uses_posix_shell_command_mode_on_windows(monkeypatch):
|
||||
def test_execute_command_keeps_msys_path_conversion_for_host_commands_on_windows(monkeypatch):
|
||||
calls: list[tuple[object, dict]] = []
|
||||
|
||||
def fake_run(*args, **kwargs):
|
||||
@ -125,6 +125,7 @@ def test_execute_command_uses_posix_shell_command_mode_on_windows(monkeypatch):
|
||||
monkeypatch.setattr(local_sandbox.os, "name", "nt")
|
||||
monkeypatch.setattr(local_sandbox.os, "environ", {"PATH": r"C:\Program Files\Git\bin"})
|
||||
monkeypatch.setattr(LocalSandbox, "_get_shell", staticmethod(lambda: r"C:\Program Files\Git\bin\sh.exe"))
|
||||
monkeypatch.setattr(LocalSandbox, "_msys_path_conversion_exclusions", lambda self: "/mnt/user-data")
|
||||
monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run)
|
||||
|
||||
output = LocalSandbox("t").execute_command("echo hello")
|
||||
@ -140,14 +141,68 @@ def test_execute_command_uses_posix_shell_command_mode_on_windows(monkeypatch):
|
||||
"timeout": 600,
|
||||
"env": {
|
||||
"PATH": r"C:\Program Files\Git\bin",
|
||||
"MSYS_NO_PATHCONV": "1",
|
||||
"MSYS2_ARG_CONV_EXCL": "*",
|
||||
"MSYS2_ARG_CONV_EXCL": "/mnt/user-data",
|
||||
},
|
||||
},
|
||||
)
|
||||
]
|
||||
|
||||
|
||||
def test_execute_command_scopes_msys_path_conversion_exclusions_on_windows(monkeypatch):
|
||||
calls: list[tuple[object, dict]] = []
|
||||
|
||||
def fake_run(*args, **kwargs):
|
||||
calls.append((args[0], kwargs))
|
||||
return SimpleNamespace(stdout="ok", stderr="", returncode=0)
|
||||
|
||||
monkeypatch.setattr(local_sandbox.os, "name", "nt")
|
||||
monkeypatch.setattr(local_sandbox.os, "environ", {"PATH": r"C:\Program Files\Git\bin"})
|
||||
monkeypatch.setattr(LocalSandbox, "_get_shell", staticmethod(lambda: r"C:\Program Files\Git\bin\sh.exe"))
|
||||
monkeypatch.setattr(LocalSandbox, "_msys_path_conversion_exclusions", lambda self: "/mnt/user-data")
|
||||
monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run)
|
||||
|
||||
output = LocalSandbox("t").execute_command("cat /mnt/user-data/workspace/input.txt")
|
||||
|
||||
assert output == "ok"
|
||||
assert calls[0][1]["env"] == {
|
||||
"PATH": r"C:\Program Files\Git\bin",
|
||||
"MSYS2_ARG_CONV_EXCL": "/mnt/user-data",
|
||||
}
|
||||
|
||||
|
||||
def test_execute_command_ignores_root_msys_mapping_for_host_commands_on_windows(monkeypatch):
|
||||
calls: list[tuple[object, dict]] = []
|
||||
|
||||
def fake_run(*args, **kwargs):
|
||||
calls.append((args[0], kwargs))
|
||||
return SimpleNamespace(stdout="ok", stderr="", returncode=0)
|
||||
|
||||
monkeypatch.setattr(local_sandbox.os, "name", "nt")
|
||||
monkeypatch.setattr(local_sandbox.os, "environ", {"PATH": r"C:\Program Files\Git\bin"})
|
||||
monkeypatch.setattr(LocalSandbox, "_get_shell", staticmethod(lambda: r"C:\Program Files\Git\bin\sh.exe"))
|
||||
monkeypatch.setattr(LocalSandbox, "_msys_path_conversion_exclusions", lambda self: "")
|
||||
monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run)
|
||||
|
||||
output = LocalSandbox("t").execute_command("echo hello")
|
||||
|
||||
assert output == "ok"
|
||||
assert calls[0][1]["env"] == {"PATH": r"C:\Program Files\Git\bin"}
|
||||
|
||||
|
||||
def test_msys_path_conversion_exclusions_omit_blanket_patterns():
|
||||
sandbox = LocalSandbox(
|
||||
"t",
|
||||
[
|
||||
PathMapping(container_path="/", local_path="C:\\"),
|
||||
PathMapping(container_path="/mnt/data;*", local_path=r"C:\data"),
|
||||
PathMapping(container_path="/mnt/user-data/", local_path=r"C:\user-data"),
|
||||
PathMapping(container_path="/mnt/user-data", local_path=r"C:\user-data"),
|
||||
],
|
||||
)
|
||||
|
||||
assert sandbox._msys_path_conversion_exclusions() == "/mnt/user-data"
|
||||
|
||||
|
||||
def test_execute_command_does_not_set_msys_env_for_non_msys_posix_shell_on_windows(monkeypatch):
|
||||
calls: list[tuple[object, dict]] = []
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user