fix(docker): gate cwd workspace mount behind config
Keep Docker sandboxes isolated by default. Add an explicit terminal.docker_mount_cwd_to_workspace opt-in, thread it through terminal/file environment creation, and document the security tradeoff and config.yaml workflow clearly.
This commit is contained in:
parent
8cdbbcaaa2
commit
780ddd102b
11 changed files with 218 additions and 145 deletions
|
|
@ -19,6 +19,8 @@ def _make_dummy_env(**kwargs):
|
|||
task_id=kwargs.get("task_id", "test-task"),
|
||||
volumes=kwargs.get("volumes", []),
|
||||
network=kwargs.get("network", True),
|
||||
host_cwd=kwargs.get("host_cwd"),
|
||||
auto_mount_cwd=kwargs.get("auto_mount_cwd", False),
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -88,24 +90,16 @@ def test_ensure_docker_available_uses_resolved_executable(monkeypatch):
|
|||
|
||||
|
||||
def test_auto_mount_host_cwd_adds_volume(monkeypatch, tmp_path):
|
||||
"""When host_cwd is provided, it should be auto-mounted to /workspace."""
|
||||
import os
|
||||
|
||||
# Create a temp directory to simulate user's project directory
|
||||
"""Opt-in docker cwd mounting should bind the host cwd to /workspace."""
|
||||
project_dir = tmp_path / "my-project"
|
||||
project_dir.mkdir()
|
||||
|
||||
# Mock Docker availability
|
||||
def _run_docker_version(*args, **kwargs):
|
||||
return subprocess.CompletedProcess(args[0], 0, stdout="Docker version", stderr="")
|
||||
|
||||
def _run_docker_create(*args, **kwargs):
|
||||
return subprocess.CompletedProcess(args[0], 1, stdout="", stderr="storage-opt not supported")
|
||||
|
||||
monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
|
||||
monkeypatch.setattr(docker_env.subprocess, "run", _run_docker_version)
|
||||
|
||||
# Mock the inner _Docker class to capture run_args
|
||||
captured_run_args = []
|
||||
|
||||
class MockInnerDocker:
|
||||
|
|
@ -120,33 +114,21 @@ def test_auto_mount_host_cwd_adds_volume(monkeypatch, tmp_path):
|
|||
MockInnerDocker,
|
||||
)
|
||||
|
||||
# Create environment with host_cwd
|
||||
env = docker_env.DockerEnvironment(
|
||||
image="python:3.11",
|
||||
_make_dummy_env(
|
||||
cwd="/workspace",
|
||||
timeout=60,
|
||||
persistent_filesystem=False, # Non-persistent mode uses tmpfs, should be overridden
|
||||
task_id="test-auto-mount",
|
||||
volumes=[],
|
||||
host_cwd=str(project_dir),
|
||||
auto_mount_cwd=True,
|
||||
)
|
||||
|
||||
# Check that the host_cwd was added as a volume mount
|
||||
volume_mount = f"-v {project_dir}:/workspace"
|
||||
run_args_str = " ".join(captured_run_args)
|
||||
assert f"{project_dir}:/workspace" in run_args_str, f"Expected auto-mount in run_args: {run_args_str}"
|
||||
assert f"{project_dir}:/workspace" in run_args_str
|
||||
|
||||
|
||||
def test_auto_mount_disabled_via_env(monkeypatch, tmp_path):
|
||||
"""Auto-mount should be disabled when TERMINAL_DOCKER_NO_AUTO_MOUNT is set."""
|
||||
import os
|
||||
|
||||
def test_auto_mount_disabled_by_default(monkeypatch, tmp_path):
|
||||
"""Host cwd should not be mounted unless the caller explicitly opts in."""
|
||||
project_dir = tmp_path / "my-project"
|
||||
project_dir.mkdir()
|
||||
|
||||
monkeypatch.setenv("TERMINAL_DOCKER_NO_AUTO_MOUNT", "true")
|
||||
|
||||
def _run_docker_version(*args, **kwargs):
|
||||
return subprocess.CompletedProcess(args[0], 0, stdout="Docker version", stderr="")
|
||||
|
||||
|
|
@ -167,26 +149,18 @@ def test_auto_mount_disabled_via_env(monkeypatch, tmp_path):
|
|||
MockInnerDocker,
|
||||
)
|
||||
|
||||
env = docker_env.DockerEnvironment(
|
||||
image="python:3.11",
|
||||
cwd="/workspace",
|
||||
timeout=60,
|
||||
persistent_filesystem=False,
|
||||
task_id="test-no-auto-mount",
|
||||
volumes=[],
|
||||
_make_dummy_env(
|
||||
cwd="/root",
|
||||
host_cwd=str(project_dir),
|
||||
auto_mount_cwd=True,
|
||||
auto_mount_cwd=False,
|
||||
)
|
||||
|
||||
# Check that the host_cwd was NOT added (because env var disabled it)
|
||||
run_args_str = " ".join(captured_run_args)
|
||||
assert f"{project_dir}:/workspace" not in run_args_str, f"Auto-mount should be disabled: {run_args_str}"
|
||||
assert f"{project_dir}:/workspace" not in run_args_str
|
||||
|
||||
|
||||
def test_auto_mount_skipped_when_workspace_already_mounted(monkeypatch, tmp_path):
|
||||
"""Auto-mount should be skipped if /workspace is already mounted via user volumes."""
|
||||
import os
|
||||
|
||||
"""Explicit user volumes for /workspace should take precedence over cwd mount."""
|
||||
project_dir = tmp_path / "my-project"
|
||||
project_dir.mkdir()
|
||||
other_dir = tmp_path / "other"
|
||||
|
|
@ -212,22 +186,52 @@ def test_auto_mount_skipped_when_workspace_already_mounted(monkeypatch, tmp_path
|
|||
MockInnerDocker,
|
||||
)
|
||||
|
||||
# User already configured a volume mount for /workspace
|
||||
env = docker_env.DockerEnvironment(
|
||||
image="python:3.11",
|
||||
_make_dummy_env(
|
||||
cwd="/workspace",
|
||||
timeout=60,
|
||||
persistent_filesystem=False,
|
||||
task_id="test-workspace-exists",
|
||||
volumes=[f"{other_dir}:/workspace"], # User explicitly mounted something to /workspace
|
||||
host_cwd=str(project_dir),
|
||||
auto_mount_cwd=True,
|
||||
volumes=[f"{other_dir}:/workspace"],
|
||||
)
|
||||
|
||||
# The user's explicit mount should be present
|
||||
run_args_str = " ".join(captured_run_args)
|
||||
assert f"{other_dir}:/workspace" in run_args_str
|
||||
assert run_args_str.count(":/workspace") == 1
|
||||
|
||||
# But the auto-mount should NOT add a duplicate
|
||||
assert run_args_str.count(":/workspace") == 1, f"Should only have one /workspace mount: {run_args_str}"
|
||||
|
||||
def test_auto_mount_replaces_persistent_workspace_bind(monkeypatch, tmp_path):
|
||||
"""Persistent mode should still prefer the configured host cwd at /workspace."""
|
||||
project_dir = tmp_path / "my-project"
|
||||
project_dir.mkdir()
|
||||
|
||||
def _run_docker_version(*args, **kwargs):
|
||||
return subprocess.CompletedProcess(args[0], 0, stdout="Docker version", stderr="")
|
||||
|
||||
monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
|
||||
monkeypatch.setattr(docker_env.subprocess, "run", _run_docker_version)
|
||||
|
||||
captured_run_args = []
|
||||
|
||||
class MockInnerDocker:
|
||||
container_id = "mock-container-persistent"
|
||||
config = type("Config", (), {"executable": "/usr/bin/docker", "forward_env": [], "env": {}})()
|
||||
|
||||
def __init__(self, **kwargs):
|
||||
captured_run_args.extend(kwargs.get("run_args", []))
|
||||
|
||||
monkeypatch.setattr(
|
||||
"minisweagent.environments.docker.DockerEnvironment",
|
||||
MockInnerDocker,
|
||||
)
|
||||
|
||||
_make_dummy_env(
|
||||
cwd="/workspace",
|
||||
persistent_filesystem=True,
|
||||
host_cwd=str(project_dir),
|
||||
auto_mount_cwd=True,
|
||||
task_id="test-persistent-auto-mount",
|
||||
)
|
||||
|
||||
run_args_str = " ".join(captured_run_args)
|
||||
assert f"{project_dir}:/workspace" in run_args_str
|
||||
assert "/sandboxes/docker/test-persistent-auto-mount/workspace:/workspace" not in run_args_str
|
||||
|
||||
|
|
|
|||
|
|
@ -91,8 +91,8 @@ class TestCwdHandling:
|
|||
"/home/ paths should be replaced for modal backend."
|
||||
)
|
||||
|
||||
def test_users_path_replaced_for_docker(self):
|
||||
"""TERMINAL_CWD=/Users/... should be replaced with /root for docker."""
|
||||
def test_users_path_replaced_for_docker_by_default(self):
|
||||
"""Docker should keep host paths out of the sandbox unless explicitly enabled."""
|
||||
with patch.dict(os.environ, {
|
||||
"TERMINAL_ENV": "docker",
|
||||
"TERMINAL_CWD": "/Users/someone/projects",
|
||||
|
|
@ -100,8 +100,22 @@ class TestCwdHandling:
|
|||
config = _tt_mod._get_env_config()
|
||||
assert config["cwd"] == "/root", (
|
||||
f"Expected /root, got {config['cwd']}. "
|
||||
"/Users/ paths should be replaced for docker backend."
|
||||
"Host paths should be discarded for docker backend by default."
|
||||
)
|
||||
assert config["host_cwd"] is None
|
||||
assert config["docker_mount_cwd_to_workspace"] is False
|
||||
|
||||
def test_users_path_maps_to_workspace_for_docker_when_enabled(self):
|
||||
"""Docker should map the host cwd into /workspace only when explicitly enabled."""
|
||||
with patch.dict(os.environ, {
|
||||
"TERMINAL_ENV": "docker",
|
||||
"TERMINAL_CWD": "/Users/someone/projects",
|
||||
"TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE": "true",
|
||||
}):
|
||||
config = _tt_mod._get_env_config()
|
||||
assert config["cwd"] == "/workspace"
|
||||
assert config["host_cwd"] == "/Users/someone/projects"
|
||||
assert config["docker_mount_cwd_to_workspace"] is True
|
||||
|
||||
def test_windows_path_replaced_for_modal(self):
|
||||
"""TERMINAL_CWD=C:\\Users\\... should be replaced for modal."""
|
||||
|
|
@ -119,12 +133,27 @@ class TestCwdHandling:
|
|||
# Remove TERMINAL_CWD so it uses default
|
||||
env = os.environ.copy()
|
||||
env.pop("TERMINAL_CWD", None)
|
||||
env.pop("TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE", None)
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
config = _tt_mod._get_env_config()
|
||||
assert config["cwd"] == "/root", (
|
||||
f"Backend {backend}: expected /root default, got {config['cwd']}"
|
||||
)
|
||||
|
||||
def test_docker_default_cwd_maps_current_directory_when_enabled(self):
|
||||
"""Docker should use /workspace when cwd mounting is explicitly enabled."""
|
||||
with patch("tools.terminal_tool.os.getcwd", return_value="/home/user/project"):
|
||||
with patch.dict(os.environ, {
|
||||
"TERMINAL_ENV": "docker",
|
||||
"TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE": "true",
|
||||
}, clear=False):
|
||||
env = os.environ.copy()
|
||||
env.pop("TERMINAL_CWD", None)
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
config = _tt_mod._get_env_config()
|
||||
assert config["cwd"] == "/workspace"
|
||||
assert config["host_cwd"] == "/home/user/project"
|
||||
|
||||
def test_local_backend_uses_getcwd(self):
|
||||
"""Local backend should use os.getcwd(), not /root."""
|
||||
with patch.dict(os.environ, {"TERMINAL_ENV": "local"}, clear=False):
|
||||
|
|
@ -134,6 +163,31 @@ class TestCwdHandling:
|
|||
config = _tt_mod._get_env_config()
|
||||
assert config["cwd"] == os.getcwd()
|
||||
|
||||
def test_create_environment_passes_docker_host_cwd_and_flag(self, monkeypatch):
|
||||
"""Docker host cwd and mount flag should reach DockerEnvironment."""
|
||||
captured = {}
|
||||
sentinel = object()
|
||||
|
||||
def _fake_docker_environment(**kwargs):
|
||||
captured.update(kwargs)
|
||||
return sentinel
|
||||
|
||||
monkeypatch.setattr(_tt_mod, "_DockerEnvironment", _fake_docker_environment)
|
||||
|
||||
env = _tt_mod._create_environment(
|
||||
env_type="docker",
|
||||
image="python:3.11",
|
||||
cwd="/workspace",
|
||||
timeout=60,
|
||||
container_config={"docker_mount_cwd_to_workspace": True},
|
||||
host_cwd="/home/user/project",
|
||||
)
|
||||
|
||||
assert env is sentinel
|
||||
assert captured["cwd"] == "/workspace"
|
||||
assert captured["host_cwd"] == "/home/user/project"
|
||||
assert captured["auto_mount_cwd"] is True
|
||||
|
||||
def test_ssh_preserves_home_paths(self):
|
||||
"""SSH backend should NOT replace /home/ paths (they're valid remotely)."""
|
||||
with patch.dict(os.environ, {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue