From 3db0789f17c0a09da075ea3e41c744ea4a87cbc5 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 08:33:13 +0900 Subject: [PATCH 01/21] fix(security): require OS isolation for web e2e commands --- scripts/ci/sandboxed_web_e2e.py | 125 +++++++++++++++++- ...ory_branch_coverage_execution_sandboxes.py | 2 + tests/test_sandboxed_web_e2e.py | 67 ++++++++++ 3 files changed, 191 insertions(+), 3 deletions(-) diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index ae0c3105ac..5e0b635ce6 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -5,6 +5,7 @@ import argparse import json import os +import platform import signal import shutil import shlex @@ -25,6 +26,7 @@ RESULT_MARKER = "SANDBOXED_WEB_E2E_RESULT" +SANDBOX_MOUNT = "/workspace" class NoRedirectHandler(urllib.request.HTTPRedirectHandler): @@ -63,6 +65,15 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: parser.add_argument("--startup-timeout", type=int, default=120, help="Seconds to wait for readiness URLs.") parser.add_argument("--e2e-timeout", type=int, default=600, help="Seconds to allow the E2E command to run.") parser.add_argument("--keep-sandbox", action="store_true", help="Keep the temporary sandbox after execution.") + parser.add_argument( + "--isolation", + choices=("required", "disabled"), + default="required", + help=( + "Require a bubblewrap OS sandbox (the default). Use disabled only for " + "trusted local debugging when bubblewrap is unavailable." + ), + ) parser.add_argument( "--allow-env", action="append", @@ -98,6 +109,70 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: return args +def isolation_backend(mode: str) -> str | None: + """Resolve the requested OS isolation backend without silently downgrading.""" + if mode == "disabled": + return None + if platform.system() != "Linux": + raise RuntimeError("required isolation is only supported on Linux with bubblewrap") + backend = shutil.which("bwrap") + if backend is None: + raise RuntimeError("required isolation needs bubblewrap (bwrap) on PATH") + return backend + + +def _sandbox_environment(env: dict[str, str], sandbox_root: Path) -> dict[str, str]: + """Map host sandbox paths to the path exposed inside the bubblewrap mount.""" + source = str(sandbox_root) + mapped = dict(env) + for key in ("HOME", "TMPDIR", "XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME"): + value = mapped.get(key) + if value: + mapped[key] = value.replace(source, SANDBOX_MOUNT, 1) + return mapped + + +def isolated_command( + command: str, + *, + backend: str, + cwd: Path, + sandbox_root: Path, + env: dict[str, str], +) -> str: + """Wrap one command in a read-only-root bubblewrap workspace.""" + argv = shlex.split(command) + if not argv: + raise ValueError("command must not be empty") + executable = shutil.which(argv[0], path=env.get("PATH")) + if executable is not None and Path(executable).is_relative_to(Path.home()): + raise RuntimeError("commands from the host home directory are not allowed in isolation") + bind_roots = [Path(path) for path in ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") if Path(path).exists()] + args = [backend, "--die-with-parent", "--new-session", "--unshare-pid"] + for root in bind_roots: + args.extend(("--ro-bind", str(root), str(root))) + for path in ("/etc/ssl", "/etc/hosts", "/etc/resolv.conf", "/etc/localtime"): + if Path(path).exists(): + args.extend(("--ro-bind", path, path)) + args.extend( + ( + "--proc", + "/proc", + "--dev", + "/dev", + "--tmpfs", + "/tmp", + "--bind", + str(sandbox_root), + SANDBOX_MOUNT, + "--chdir", + f"{SANDBOX_MOUNT}/{cwd.relative_to(sandbox_root)}", + "--", + ) + ) + return shlex.join([*args, *argv]) + + def start_service(label: str, command: str, cwd: Path, env: dict[str, str], logs_dir: Path) -> Service: """Start a service command in its own process group.""" log_path = logs_dir / f"{label}.log" @@ -195,6 +270,8 @@ def emit_result( "frontend_cmd": args.frontend_cmd, "frontend_ready": frontend_ready, "network": args.network, + "isolation": args.isolation, + "isolation_backend": getattr(args, "isolation_backend", "unknown"), "sandbox": str(sandbox_root) if args.keep_sandbox else "(removed)", "sandboxed": True, } @@ -216,13 +293,55 @@ def main(argv: Sequence[str] | None = None) -> int: try: copied_repo = sandboxed_verify.copy_workspace(Path(args.repo_root), sandbox, args.ignore) env = sandboxed_verify.scrubbed_env(sandbox, args.allow_env) + try: + backend = isolation_backend(args.isolation) + except RuntimeError as exc: + print(f"sandboxed-web-e2e: {exc}", file=sys.stderr) + args.isolation_backend = "unavailable" + exit_code = 126 + return exit_code + args.isolation_backend = backend or "disabled" print(f"sandboxed-web-e2e: cwd={copied_repo}") if args.allow_env: print(f"sandboxed-web-e2e: allowed env names={','.join(sorted(set(args.allow_env)))}") if args.network != "default": print(f"sandboxed-web-e2e: network={args.network}") - services.append(start_service("backend", args.backend_cmd, copied_repo, env, logs_dir)) - services.append(start_service("frontend", args.frontend_cmd, copied_repo, env, logs_dir)) + command_env = _sandbox_environment(env, sandbox) if backend else env + backend_cmd = ( + isolated_command( + args.backend_cmd, + backend=backend, + cwd=copied_repo, + sandbox_root=sandbox, + env=env, + ) + if backend + else args.backend_cmd + ) + frontend_cmd = ( + isolated_command( + args.frontend_cmd, + backend=backend, + cwd=copied_repo, + sandbox_root=sandbox, + env=env, + ) + if backend + else args.frontend_cmd + ) + e2e_cmd = ( + isolated_command( + args.e2e_cmd, + backend=backend, + cwd=copied_repo, + sandbox_root=sandbox, + env=env, + ) + if backend + else args.e2e_cmd + ) + services.append(start_service("backend", backend_cmd, copied_repo, command_env, logs_dir)) + services.append(start_service("frontend", frontend_cmd, copied_repo, command_env, logs_dir)) backend_ready = wait_for_url(args.backend_ready_url, args.startup_timeout, services[0]) frontend_ready = wait_for_url(args.frontend_ready_url, args.startup_timeout, services[1]) if not backend_ready or not frontend_ready: @@ -230,7 +349,7 @@ def main(argv: Sequence[str] | None = None) -> int: exit_code = 125 return exit_code try: - completed = run_shell(args.e2e_cmd, copied_repo, env, args.e2e_timeout) + completed = run_shell(e2e_cmd, copied_repo, command_env, args.e2e_timeout) if completed.stdout: print(completed.stdout, end="") if completed.stderr: diff --git a/tests/test_repository_branch_coverage_execution_sandboxes.py b/tests/test_repository_branch_coverage_execution_sandboxes.py index f8912272a1..7d1ec0a431 100644 --- a/tests/test_repository_branch_coverage_execution_sandboxes.py +++ b/tests/test_repository_branch_coverage_execution_sandboxes.py @@ -216,6 +216,8 @@ def timeout_runner( [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", "backend", "--frontend-cmd", diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 6e092c2930..e015b0cdb0 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -56,6 +56,8 @@ def test_sandboxed_web_e2e_runs_services_and_does_not_mutate_source(tmp_path, ca [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", http_server_command(backend_port, "backend"), "--frontend-cmd", @@ -298,6 +300,8 @@ def fake_start(label, command, cwd, env, logs_dir): [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", "backend", "--frontend-cmd", @@ -361,6 +365,8 @@ def fake_wait(url, timeout, service): [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", "backend", "--frontend-cmd", @@ -410,6 +416,8 @@ def fake_run_shell(command, cwd, env, timeout): [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", "backend", "--frontend-cmd", @@ -440,6 +448,8 @@ def test_sandboxed_web_e2e_reports_readiness_failure(tmp_path, capsys): [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", http_server_command(backend_port, "backend"), "--frontend-cmd", @@ -478,6 +488,8 @@ def fake_run_shell(command, cwd, env, timeout): [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", f"{sys.executable} -c \"import time; time.sleep(3)\"", "--frontend-cmd", @@ -497,6 +509,59 @@ def fake_run_shell(command, cwd, env, timeout): assert "SANDBOXED_WEB_E2E_RESULT" in captured.out +def test_isolation_backend_fails_closed_outside_linux(monkeypatch): + """Required isolation never silently falls back to a host process.""" + monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Darwin") + with pytest.raises(RuntimeError, match="only supported on Linux"): + sandboxed_web_e2e.isolation_backend("required") + assert sandboxed_web_e2e.isolation_backend("disabled") is None + + +def test_isolated_command_mounts_only_workspace(monkeypatch, tmp_path): + """Bubblewrap commands expose the copied workspace and not the host root.""" + monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") + monkeypatch.setattr( + sandboxed_web_e2e.shutil, + "which", + lambda name, path=None: "/usr/bin/bwrap" if name == "bwrap" else "/usr/bin/python3", + ) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + env = {"PATH": "/usr/bin", "HOME": str(sandbox / "home")} + command = sandboxed_web_e2e.isolated_command( + "python3 -c 'print(1)'", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env=env, + ) + assert command.startswith("/usr/bin/bwrap") + assert "--bind" in command + assert "--chdir /workspace/repo" in command + assert "--ro-bind / /" not in command + + +def test_isolated_command_rejects_host_home_executable(monkeypatch, tmp_path): + """Executable paths from a user's home cannot enter the isolated runner.""" + monkeypatch.setattr( + sandboxed_web_e2e.shutil, + "which", + lambda *_args, **_kwargs: str(Path.home() / "bin/tool"), + ) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + with pytest.raises(RuntimeError, match="host home directory"): + sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + def test_parse_args_rejects_invalid_inputs(): """The CLI rejects unusable timeout and environment values.""" with pytest.raises(SystemExit): @@ -582,6 +647,8 @@ def test_module_import_and_main_entrypoint(monkeypatch, tmp_path): "sandboxed_web_e2e.py", "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", f"{sys.executable} -c \"import time; time.sleep(0.2)\"", "--frontend-cmd", From d42a97f196ede04517e286e839b596cd570ac1da Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 08:36:09 +0900 Subject: [PATCH 02/21] fix(security): harden loopback and document isolation --- .../doctoring/sandboxed-web-command-isolation.md | 16 ++++++++++++++++ scripts/ci/sandboxed_web_e2e.py | 12 ++++++++++++ tests/test_sandboxed_web_e2e.py | 10 ++++++++++ 3 files changed, 38 insertions(+) create mode 100644 docs/doctoring/sandboxed-web-command-isolation.md diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md new file mode 100644 index 0000000000..5dbf40b730 --- /dev/null +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -0,0 +1,16 @@ +# Sandboxed web command isolation + +`sandboxed_web_e2e.py` requires Linux `bubblewrap` (`bwrap`) by default. Each +backend, frontend, and E2E command runs with a read-only runtime root and a +single writable mount at `/workspace`; the copied repository and temporary +homes are mapped there. The host filesystem is therefore not reachable through +absolute paths or `..` traversal. + +Use `--isolation disabled` only for trusted local debugging. The result marker +records the requested mode and resolved backend so CI evidence cannot be +mistaken for an OS-isolated run. If required isolation is unavailable, the +command exits with code `126` before starting any service. + +Readiness polling remains loopback-only and does not follow redirects. The +network declaration is evidence metadata; callers that need stronger network +policy must run this helper inside a network-restricted runner or container. diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 5e0b635ce6..7bec55c338 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -3,6 +3,7 @@ from __future__ import annotations import argparse +import ipaddress import json import os import platform @@ -14,6 +15,7 @@ import tempfile import time import urllib.error +import urllib.parse import urllib.request from collections.abc import Sequence from dataclasses import dataclass @@ -196,6 +198,16 @@ def wait_for_url(url: str, timeout: int, service: Service) -> bool: return True if not (url.startswith("http://") or url.startswith("https://")): raise ValueError(f"URL must start with http:// or https://, got: {url}") + + parsed = urllib.parse.urlparse(url) + hostname = (parsed.hostname or "").lower() + try: + is_loopback = hostname == "localhost" or ipaddress.ip_address(hostname).is_loopback + except ValueError: + is_loopback = False + if not is_loopback: + raise ValueError(f"URL cannot target external hostname: {hostname}") + deadline = time.monotonic() + timeout opener = urllib.request.build_opener(NoRedirectHandler()) while time.monotonic() < deadline: diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index e015b0cdb0..0365feaf93 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -113,9 +113,19 @@ def test_wait_helpers_and_service_cleanup_edges(monkeypatch, tmp_path): exited_service = sandboxed_web_e2e.Service("done", "true", exited, tmp_path / "missing.log") assert sandboxed_web_e2e.wait_for_url("", 1, exited_service) is True + assert sandboxed_web_e2e.wait_for_url("http://localhost:1/", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("http://127.0.0.1:1/", 1, exited_service) is False + assert sandboxed_web_e2e.wait_for_url("http://127.0.0.2:1/", 1, exited_service) is False with pytest.raises(ValueError, match="URL must start with http:// or https://"): sandboxed_web_e2e.wait_for_url("file:///etc/passwd", 1, exited_service) + with pytest.raises(ValueError, match="URL cannot target external hostname: external.example.com"): + sandboxed_web_e2e.wait_for_url("http://external.example.com/ready", 1, exited_service) + with pytest.raises(ValueError, match="URL cannot target external hostname: app.localhost"): + sandboxed_web_e2e.wait_for_url("http://app.localhost:8000/health", 1, exited_service) + with pytest.raises(ValueError, match="URL cannot target external hostname: 169.254.169.254"): + sandboxed_web_e2e.wait_for_url("http://169.254.169.254/latest/meta-data/", 1, exited_service) + with pytest.raises(ValueError, match="URL cannot target external hostname: 0.0.0.0"): + sandboxed_web_e2e.wait_for_url("http://0.0.0.0:8000/health", 1, exited_service) sandboxed_web_e2e.stop_service(exited_service) assert sandboxed_web_e2e.tail_text(tmp_path / "missing.log") == "" From 823cd89a8698918fb303b6bab6f0330c8b7684ea Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 08:38:05 +0900 Subject: [PATCH 03/21] fix(security): mount isolated root filesystem --- scripts/ci/sandboxed_web_e2e.py | 2 +- tests/test_sandboxed_web_e2e.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 7bec55c338..c37b188c59 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -150,7 +150,7 @@ def isolated_command( if executable is not None and Path(executable).is_relative_to(Path.home()): raise RuntimeError("commands from the host home directory are not allowed in isolation") bind_roots = [Path(path) for path in ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") if Path(path).exists()] - args = [backend, "--die-with-parent", "--new-session", "--unshare-pid"] + args = [backend, "--die-with-parent", "--new-session", "--unshare-pid", "--tmpfs", "/"] for root in bind_roots: args.extend(("--ro-bind", str(root), str(root))) for path in ("/etc/ssl", "/etc/hosts", "/etc/resolv.conf", "/etc/localtime"): diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 0365feaf93..985100dd01 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -547,6 +547,7 @@ def test_isolated_command_mounts_only_workspace(monkeypatch, tmp_path): env=env, ) assert command.startswith("/usr/bin/bwrap") + assert "--tmpfs /" in command assert "--bind" in command assert "--chdir /workspace/repo" in command assert "--ro-bind / /" not in command From ce93f556b35edc9665ac78dd374266a774b76f26 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 09:06:59 +0900 Subject: [PATCH 04/21] docs(security): document isolated web verification --- CHANGELOG.md | 3 +++ docs/doctoring/sandboxed-web-command-isolation.md | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cef0acda6b..e88bf96785 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Require Linux bubblewrap isolation for backend, frontend, and E2E commands + in the web verification helper, mount only a writable workspace, and reject + non-loopback readiness URLs or redirects so SSRF probes fail closed. - Route Strix cross-provider fallbacks to explicit direct-OpenAI models (`openai-direct/...`) through the OpenAI inference endpoint instead of inheriting a provider-specific primary base: the workflow now provisions diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index 5dbf40b730..3f8467d5be 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -14,3 +14,8 @@ command exits with code `126` before starting any service. Readiness polling remains loopback-only and does not follow redirects. The network declaration is evidence metadata; callers that need stronger network policy must run this helper inside a network-restricted runner or container. + +## References + +MITRE. (2026). *CWE-918: Server-side request forgery (SSRF)*. +https://cwe.mitre.org/data/definitions/918.html From 871d50bb14fd88bfa2d14e725b70e6bc61bedffd Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 09:39:15 +0900 Subject: [PATCH 05/21] fix(e2e): cover and harden isolated command paths --- CHANGELOG.md | 3 + .../sandboxed-web-command-isolation.md | 13 +- scripts/ci/sandboxed_web_e2e.py | 25 +- tests/test_sandboxed_web_e2e.py | 240 ++++++++++++++++++ 4 files changed, 273 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e88bf96785..278175d3a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ Semantic Versioning where the repository publishes a release. - Require Linux bubblewrap isolation for backend, frontend, and E2E commands in the web verification helper, mount only a writable workspace, and reject non-loopback readiness URLs or redirects so SSRF probes fail closed. +- Fail closed when an isolated command resolves outside the mounted system + roots, return a coded readiness failure for invalid URLs, and cover required + isolation plus sandbox-environment path mapping in the 100% branch contract. - Route Strix cross-provider fallbacks to explicit direct-OpenAI models (`openai-direct/...`) through the OpenAI inference endpoint instead of inheriting a provider-specific primary base: the workflow now provisions diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index 3f8467d5be..1e862cfc33 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -6,14 +6,21 @@ single writable mount at `/workspace`; the copied repository and temporary homes are mapped there. The host filesystem is therefore not reachable through absolute paths or `..` traversal. +Before wrapping a command, the helper resolves its executable and rejects paths +outside the read-only system roots mounted by bubblewrap. A tool installed in a +host-only location must be installed into one of those roots or the run exits +before any service starts. + Use `--isolation disabled` only for trusted local debugging. The result marker records the requested mode and resolved backend so CI evidence cannot be mistaken for an OS-isolated run. If required isolation is unavailable, the command exits with code `126` before starting any service. -Readiness polling remains loopback-only and does not follow redirects. The -network declaration is evidence metadata; callers that need stronger network -policy must run this helper inside a network-restricted runner or container. +Readiness polling remains loopback-only and does not follow redirects. Invalid +readiness URLs are reported as a coded readiness failure (`125`) rather than an +uncaught traceback. The network declaration is evidence metadata; callers that +need stronger network policy must run this helper inside a network-restricted +runner or container. ## References diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index c37b188c59..a99ebee451 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -146,10 +146,20 @@ def isolated_command( argv = shlex.split(command) if not argv: raise ValueError("command must not be empty") + bind_roots = [ + Path(path) + for path in ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") + if Path(path).exists() + ] executable = shutil.which(argv[0], path=env.get("PATH")) - if executable is not None and Path(executable).is_relative_to(Path.home()): - raise RuntimeError("commands from the host home directory are not allowed in isolation") - bind_roots = [Path(path) for path in ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") if Path(path).exists()] + if executable is not None: + executable_path = Path(executable) + if executable_path.is_relative_to(Path.home()): + raise RuntimeError("commands from the host home directory are not allowed in isolation") + if not any(executable_path.is_relative_to(root) for root in bind_roots): + raise RuntimeError( + f"executable is outside the isolated bind roots: {executable_path}" + ) args = [backend, "--die-with-parent", "--new-session", "--unshare-pid", "--tmpfs", "/"] for root in bind_roots: args.extend(("--ro-bind", str(root), str(root))) @@ -354,8 +364,13 @@ def main(argv: Sequence[str] | None = None) -> int: ) services.append(start_service("backend", backend_cmd, copied_repo, command_env, logs_dir)) services.append(start_service("frontend", frontend_cmd, copied_repo, command_env, logs_dir)) - backend_ready = wait_for_url(args.backend_ready_url, args.startup_timeout, services[0]) - frontend_ready = wait_for_url(args.frontend_ready_url, args.startup_timeout, services[1]) + try: + backend_ready = wait_for_url(args.backend_ready_url, args.startup_timeout, services[0]) + frontend_ready = wait_for_url(args.frontend_ready_url, args.startup_timeout, services[1]) + except ValueError as exc: + print(f"sandboxed-web-e2e: invalid readiness URL: {exc}", file=sys.stderr) + exit_code = 125 + return exit_code if not backend_ready or not frontend_ready: print("sandboxed-web-e2e: service readiness failed", file=sys.stderr) exit_code = 125 diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 985100dd01..3fb2fbfb8b 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -350,6 +350,92 @@ def fake_start(label, command, cwd, env, logs_dir): assert payload["evidence_note"] == "needs browser auth" +def test_main_runs_required_isolation_with_mapped_environment(monkeypatch, tmp_path, capsys): + """Required isolation wraps every command and maps sandbox paths into /workspace.""" + repo = tmp_path / "repo" + repo.mkdir() + wrapped = [] + started = [] + + class DoneProcess: + def poll(self): + return 0 + + def fake_isolated(command, **kwargs): + wrapped.append((command, kwargs)) + return f"wrapped {command}" + + def fake_start(label, command, cwd, env, logs_dir): + log_path = logs_dir / f"{label}.log" + log_path.write_text(f"{label} ready\n", encoding="utf-8") + started.append((label, command, cwd, env)) + return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) + + monkeypatch.setattr(sandboxed_web_e2e, "isolation_backend", lambda mode: "/usr/bin/bwrap") + monkeypatch.setattr(sandboxed_web_e2e, "isolated_command", fake_isolated) + monkeypatch.setattr(sandboxed_web_e2e, "start_service", fake_start) + monkeypatch.setattr(sandboxed_web_e2e, "wait_for_url", lambda url, timeout, service: True) + monkeypatch.setattr( + sandboxed_web_e2e, + "run_shell", + lambda command, cwd, env, timeout: subprocess.CompletedProcess(command, 0), + ) + monkeypatch.setattr(sandboxed_web_e2e, "stop_service", lambda service: None) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 0 + assert [item[0] for item in wrapped] == ["backend", "frontend", "e2e"] + assert [item[0] for item in started] == ["backend", "frontend"] + assert all(item[1].startswith("wrapped ") for item in started) + assert started[0][3]["HOME"].startswith("/workspace/") + assert "isolation_backend=unknown" not in captured.out + + +def test_main_reports_unavailable_required_isolation(monkeypatch, tmp_path, capsys): + """Required isolation errors exit before starting services with code 126.""" + repo = tmp_path / "repo" + repo.mkdir() + monkeypatch.setattr( + sandboxed_web_e2e, + "isolation_backend", + lambda mode: (_ for _ in ()).throw(RuntimeError("bwrap unavailable")), + ) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 126 + assert "bwrap unavailable" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 126 + assert payload["isolation_backend"] == "unavailable" + + def test_main_reports_stubbed_readiness_failure(monkeypatch, tmp_path, capsys): """Main exits distinctly when a stubbed service never becomes ready.""" repo = tmp_path / "repo" @@ -400,6 +486,55 @@ def fake_wait(url, timeout, service): assert payload["exit_code"] == 125 +def test_main_reports_invalid_readiness_url(monkeypatch, tmp_path, capsys): + """Invalid readiness input exits with the same clean readiness failure code.""" + repo = tmp_path / "repo" + repo.mkdir() + + class DoneProcess: + def poll(self): + return 0 + + def fake_start(label, command, cwd, env, logs_dir): + log_path = logs_dir / f"{label}.log" + log_path.write_text("ready\n", encoding="utf-8") + return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) + + monkeypatch.setattr(sandboxed_web_e2e, "start_service", fake_start) + monkeypatch.setattr( + sandboxed_web_e2e, + "wait_for_url", + lambda url, timeout, service: (_ for _ in ()).throw(ValueError("bad host")), + ) + monkeypatch.setattr(sandboxed_web_e2e, "stop_service", lambda service: None) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--backend-ready-url", + "http://external.example/health", + "--frontend-ready-url", + "http://127.0.0.1:3000/", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert "invalid readiness URL: bad host" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + def test_main_reports_stubbed_e2e_timeout(monkeypatch, tmp_path, capsys): """Main preserves timeout output from stubbed E2E execution.""" repo = tmp_path / "repo" @@ -527,6 +662,21 @@ def test_isolation_backend_fails_closed_outside_linux(monkeypatch): assert sandboxed_web_e2e.isolation_backend("disabled") is None +def test_isolation_backend_fails_closed_without_bwrap(monkeypatch): + """Linux isolation refuses to continue when bubblewrap is not installed.""" + monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: None) + with pytest.raises(RuntimeError, match="needs bubblewrap"): + sandboxed_web_e2e.isolation_backend("required") + + +def test_isolation_backend_returns_bwrap_path_on_linux(monkeypatch): + """Linux isolation returns the resolved bubblewrap executable.""" + monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/usr/bin/bwrap") + assert sandboxed_web_e2e.isolation_backend("required") == "/usr/bin/bwrap" + + def test_isolated_command_mounts_only_workspace(monkeypatch, tmp_path): """Bubblewrap commands expose the copied workspace and not the host root.""" monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") @@ -573,6 +723,96 @@ def test_isolated_command_rejects_host_home_executable(monkeypatch, tmp_path): ) +def test_isolated_command_rejects_executable_outside_bound_roots(monkeypatch, tmp_path): + """Resolved tools outside read-only mounts fail before entering bubblewrap.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: "/snap/bin/tool") + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + with pytest.raises(RuntimeError, match="outside the isolated bind roots"): + sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + +def test_isolated_command_allows_unresolved_executable_for_bwrap(monkeypatch, tmp_path): + """Commands with shell-resolved executables still receive the isolated wrapper.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + command = sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + assert command.startswith("/usr/bin/bwrap") + + +def test_isolated_command_skips_unavailable_optional_mount(monkeypatch, tmp_path): + """Optional runtime mounts are omitted when a host path is unavailable.""" + original_exists = Path.exists + + def fake_exists(path): + if str(path) == "/etc/ssl": + return False + return original_exists(path) + + monkeypatch.setattr(Path, "exists", fake_exists) + monkeypatch.setattr( + sandboxed_web_e2e.shutil, + "which", + lambda name, path=None: "/usr/bin/python3", + ) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + command = sandboxed_web_e2e.isolated_command( + "python3", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + assert "--ro-bind /etc/ssl /etc/ssl" not in command + + +def test_sandbox_environment_maps_host_paths_to_workspace(tmp_path): + """Only configured sandbox paths are rewritten for the mounted workspace.""" + sandbox = tmp_path / "sandbox" + env = { + "HOME": str(sandbox / "home"), + "TMPDIR": str(sandbox / "tmp"), + "PATH": "/usr/bin", + } + + mapped = sandboxed_web_e2e._sandbox_environment(env, sandbox) + + assert mapped is not env + assert mapped["HOME"] == "/workspace/home" + assert mapped["TMPDIR"] == "/workspace/tmp" + assert mapped["PATH"] == "/usr/bin" + assert "XDG_CACHE_HOME" not in mapped + + +def test_isolated_command_rejects_empty_command(tmp_path): + """Empty commands fail before bubblewrap arguments are constructed.""" + with pytest.raises(ValueError, match="command must not be empty"): + sandboxed_web_e2e.isolated_command( + " ", + backend="/usr/bin/bwrap", + cwd=tmp_path, + sandbox_root=tmp_path, + env={"PATH": "/usr/bin"}, + ) + + def test_parse_args_rejects_invalid_inputs(): """The CLI rejects unusable timeout and environment values.""" with pytest.raises(SystemExit): From 8b9d884f6db7788847253eb9496046b6ead532d9 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 10:00:12 +0900 Subject: [PATCH 06/21] docs: make sandbox changelog actionable --- CHANGELOG.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 278175d3a7..7423627071 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,12 +5,12 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] -- Require Linux bubblewrap isolation for backend, frontend, and E2E commands - in the web verification helper, mount only a writable workspace, and reject - non-loopback readiness URLs or redirects so SSRF probes fail closed. -- Fail closed when an isolated command resolves outside the mounted system - roots, return a coded readiness failure for invalid URLs, and cover required - isolation plus sandbox-environment path mapping in the 100% branch contract. +- Web verification now runs backend, frontend, and E2E commands in an isolated + workspace and accepts only local readiness URLs. Run it on a supported Linux + runner; trusted local debugging may opt out with `--isolation disabled`. +- Invalid readiness URLs and unavailable isolation now fail with clear + diagnostics before services start, so update the URL or runner instead of + retrying the same setup. - Route Strix cross-provider fallbacks to explicit direct-OpenAI models (`openai-direct/...`) through the OpenAI inference endpoint instead of inheriting a provider-specific primary base: the workflow now provisions From c50e26be529f473e6cdbce6dd9a7540cb750e7a0 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 10:10:41 +0900 Subject: [PATCH 07/21] fix(e2e): fail closed on rejected sandbox commands --- .../sandboxed-web-command-isolation.md | 12 +- scripts/ci/sandboxed_web_e2e.py | 75 +++++++----- tests/test_sandboxed_web_e2e.py | 112 ++++++++++++++++-- 3 files changed, 151 insertions(+), 48 deletions(-) diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index 1e862cfc33..ee2b6a3340 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -1,15 +1,17 @@ # Sandboxed web command isolation `sandboxed_web_e2e.py` requires Linux `bubblewrap` (`bwrap`) by default. Each -backend, frontend, and E2E command runs with a read-only runtime root and a -single writable mount at `/workspace`; the copied repository and temporary -homes are mapped there. The host filesystem is therefore not reachable through -absolute paths or `..` traversal. +backend, frontend, and E2E command runs with a fresh writable `tmpfs` root and +`/tmp`, plus one writable copied-repository bind at `/workspace`; the copied +repository and temporary homes are mapped there. Host runtime roots and the +minimal `/etc` identity, DNS, and time files are mounted read-only, so the host +filesystem is not reachable through absolute paths or `..` traversal. Before wrapping a command, the helper resolves its executable and rejects paths outside the read-only system roots mounted by bubblewrap. A tool installed in a host-only location must be installed into one of those roots or the run exits -before any service starts. +with code `126` before any service starts; the result marker records that code +and the selected backend. Use `--isolation disabled` only for trusted local debugging. The result marker records the requested mode and resolved backend so CI evidence cannot be diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index a99ebee451..beddb52c31 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -163,7 +163,15 @@ def isolated_command( args = [backend, "--die-with-parent", "--new-session", "--unshare-pid", "--tmpfs", "/"] for root in bind_roots: args.extend(("--ro-bind", str(root), str(root))) - for path in ("/etc/ssl", "/etc/hosts", "/etc/resolv.conf", "/etc/localtime"): + for path in ( + "/etc/ssl", + "/etc/hosts", + "/etc/resolv.conf", + "/etc/localtime", + "/etc/passwd", + "/etc/group", + "/etc/nsswitch.conf", + ): if Path(path).exists(): args.extend(("--ro-bind", path, path)) args.extend( @@ -329,39 +337,44 @@ def main(argv: Sequence[str] | None = None) -> int: if args.network != "default": print(f"sandboxed-web-e2e: network={args.network}") command_env = _sandbox_environment(env, sandbox) if backend else env - backend_cmd = ( - isolated_command( - args.backend_cmd, - backend=backend, - cwd=copied_repo, - sandbox_root=sandbox, - env=env, + try: + backend_cmd = ( + isolated_command( + args.backend_cmd, + backend=backend, + cwd=copied_repo, + sandbox_root=sandbox, + env=env, + ) + if backend + else args.backend_cmd ) - if backend - else args.backend_cmd - ) - frontend_cmd = ( - isolated_command( - args.frontend_cmd, - backend=backend, - cwd=copied_repo, - sandbox_root=sandbox, - env=env, + frontend_cmd = ( + isolated_command( + args.frontend_cmd, + backend=backend, + cwd=copied_repo, + sandbox_root=sandbox, + env=env, + ) + if backend + else args.frontend_cmd ) - if backend - else args.frontend_cmd - ) - e2e_cmd = ( - isolated_command( - args.e2e_cmd, - backend=backend, - cwd=copied_repo, - sandbox_root=sandbox, - env=env, + e2e_cmd = ( + isolated_command( + args.e2e_cmd, + backend=backend, + cwd=copied_repo, + sandbox_root=sandbox, + env=env, + ) + if backend + else args.e2e_cmd ) - if backend - else args.e2e_cmd - ) + except RuntimeError as exc: + print(f"sandboxed-web-e2e: isolation rejected command: {exc}", file=sys.stderr) + exit_code = 126 + return exit_code services.append(start_service("backend", backend_cmd, copied_repo, command_env, logs_dir)) services.append(start_service("frontend", frontend_cmd, copied_repo, command_env, logs_dir)) try: diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 3fb2fbfb8b..a14926321f 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -1,5 +1,6 @@ import json import os +import re import runpy import socket import subprocess @@ -116,15 +117,15 @@ def test_wait_helpers_and_service_cleanup_edges(monkeypatch, tmp_path): assert sandboxed_web_e2e.wait_for_url("http://localhost:1/", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("http://127.0.0.1:1/", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("http://127.0.0.2:1/", 1, exited_service) is False - with pytest.raises(ValueError, match="URL must start with http:// or https://"): + with pytest.raises(ValueError, match=re.escape("URL must start with http:// or https://")): sandboxed_web_e2e.wait_for_url("file:///etc/passwd", 1, exited_service) - with pytest.raises(ValueError, match="URL cannot target external hostname: external.example.com"): + with pytest.raises(ValueError, match=re.escape("URL cannot target external hostname: external.example.com")): sandboxed_web_e2e.wait_for_url("http://external.example.com/ready", 1, exited_service) - with pytest.raises(ValueError, match="URL cannot target external hostname: app.localhost"): + with pytest.raises(ValueError, match=re.escape("URL cannot target external hostname: app.localhost")): sandboxed_web_e2e.wait_for_url("http://app.localhost:8000/health", 1, exited_service) - with pytest.raises(ValueError, match="URL cannot target external hostname: 169.254.169.254"): + with pytest.raises(ValueError, match=re.escape("URL cannot target external hostname: 169.254.169.254")): sandboxed_web_e2e.wait_for_url("http://169.254.169.254/latest/meta-data/", 1, exited_service) - with pytest.raises(ValueError, match="URL cannot target external hostname: 0.0.0.0"): + with pytest.raises(ValueError, match=re.escape("URL cannot target external hostname: 0.0.0.0")): sandboxed_web_e2e.wait_for_url("http://0.0.0.0:8000/health", 1, exited_service) sandboxed_web_e2e.stop_service(exited_service) assert sandboxed_web_e2e.tail_text(tmp_path / "missing.log") == "" @@ -211,7 +212,8 @@ def poll(self): return None class Response: - status = 204 + def __init__(self, status): + self.status = status def __enter__(self): return self @@ -226,7 +228,7 @@ def open(self, url, timeout): attempts.append((url, timeout)) if len(attempts) == 1: raise sandboxed_web_e2e.urllib.error.URLError("not ready") - return Response() + return Response(500 if len(attempts) == 2 else 204) monkeypatch.setattr(sandboxed_web_e2e.urllib.request, "build_opener", lambda *args: FakeOpener()) monkeypatch.setattr(sandboxed_web_e2e.time, "sleep", lambda seconds: None) @@ -236,7 +238,7 @@ def open(self, url, timeout): service = sandboxed_web_e2e.Service("web", "serve", RunningProcess(), log_path) assert sandboxed_web_e2e.wait_for_url("http://127.0.0.1:8000/health", 10, service) is True - assert len(attempts) == 2 + assert len(attempts) == 3 assert sandboxed_web_e2e.tail_text(log_path).splitlines()[0] == "line-10" @@ -401,7 +403,47 @@ def fake_start(label, command, cwd, env, logs_dir): assert [item[0] for item in started] == ["backend", "frontend"] assert all(item[1].startswith("wrapped ") for item in started) assert started[0][3]["HOME"].startswith("/workspace/") - assert "isolation_backend=unknown" not in captured.out + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["isolation"] == "required" + assert payload["isolation_backend"] == "/usr/bin/bwrap" + + +def test_main_reports_rejected_isolated_command(monkeypatch, tmp_path, capsys): + """Rejected commands fail before services start and emit coded evidence.""" + repo = tmp_path / "repo" + repo.mkdir() + started = [] + + monkeypatch.setattr(sandboxed_web_e2e, "isolation_backend", lambda mode: "/usr/bin/bwrap") + monkeypatch.setattr( + sandboxed_web_e2e, + "isolated_command", + lambda command, **kwargs: (_ for _ in ()).throw(RuntimeError("host-only tool")), + ) + monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 126 + assert not started + assert "isolation rejected command: host-only tool" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 126 + assert payload["isolation_backend"] == "/usr/bin/bwrap" def test_main_reports_unavailable_required_isolation(monkeypatch, tmp_path, capsys): @@ -580,6 +622,48 @@ def fake_run_shell(command, cwd, env, timeout): assert "e2e-err" in captured.err assert "e2e command timed out after 3s" in captured.err + def fake_run_shell_with_newlines(command, cwd, env, timeout): + raise subprocess.TimeoutExpired(command, timeout, output=b"e2e-out\n", stderr=b"e2e-err\n") + + monkeypatch.setattr(sandboxed_web_e2e, "run_shell", fake_run_shell_with_newlines) + assert sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-timeout", + "3", + "--e2e-cmd", + "e2e", + ] + ) == 124 + + def fake_run_shell_without_output(command, cwd, env, timeout): + raise subprocess.TimeoutExpired(command, timeout) + + monkeypatch.setattr(sandboxed_web_e2e, "run_shell", fake_run_shell_without_output) + assert sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-timeout", + "3", + "--e2e-cmd", + "e2e", + ] + ) == 124 + @POSIX_PROCESS_GROUPS def test_sandboxed_web_e2e_reports_readiness_failure(tmp_path, capsys): @@ -701,6 +785,10 @@ def test_isolated_command_mounts_only_workspace(monkeypatch, tmp_path): assert "--bind" in command assert "--chdir /workspace/repo" in command assert "--ro-bind / /" not in command + assert "--ro-bind /etc/passwd /etc/passwd" in command + assert "--ro-bind /etc/group /etc/group" in command + if Path("/etc/nsswitch.conf").exists(): + assert "--ro-bind /etc/nsswitch.conf /etc/nsswitch.conf" in command def test_isolated_command_rejects_host_home_executable(monkeypatch, tmp_path): @@ -713,7 +801,7 @@ def test_isolated_command_rejects_host_home_executable(monkeypatch, tmp_path): sandbox = tmp_path / "sandbox" repo = sandbox / "repo" repo.mkdir(parents=True) - with pytest.raises(RuntimeError, match="host home directory"): + with pytest.raises(RuntimeError, match=re.escape("host home directory")): sandboxed_web_e2e.isolated_command( "tool", backend="/usr/bin/bwrap", @@ -729,7 +817,7 @@ def test_isolated_command_rejects_executable_outside_bound_roots(monkeypatch, tm sandbox = tmp_path / "sandbox" repo = sandbox / "repo" repo.mkdir(parents=True) - with pytest.raises(RuntimeError, match="outside the isolated bind roots"): + with pytest.raises(RuntimeError, match=re.escape("outside the isolated bind roots")): sandboxed_web_e2e.isolated_command( "tool", backend="/usr/bin/bwrap", @@ -803,7 +891,7 @@ def test_sandbox_environment_maps_host_paths_to_workspace(tmp_path): def test_isolated_command_rejects_empty_command(tmp_path): """Empty commands fail before bubblewrap arguments are constructed.""" - with pytest.raises(ValueError, match="command must not be empty"): + with pytest.raises(ValueError, match=re.escape("command must not be empty")): sandboxed_web_e2e.isolated_command( " ", backend="/usr/bin/bwrap", From 391233f13c9f57d365f19868c4e7ae2b8e9ac79d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 10:34:20 +0900 Subject: [PATCH 08/21] fix(e2e): validate readiness before launch --- scripts/ci/sandboxed_web_e2e.py | 20 +++++++++++-- tests/test_sandboxed_web_e2e.py | 53 ++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index beddb52c31..9bdf0820a9 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -210,10 +210,10 @@ def start_service(label: str, command: str, cwd: Path, env: dict[str, str], logs return Service(label=label, command=command, process=process, log_path=log_path) -def wait_for_url(url: str, timeout: int, service: Service) -> bool: - """Poll a readiness URL until it responds or the service exits.""" +def validate_readiness_url(url: str) -> None: + """Reject a readiness URL that is not an HTTP(S) loopback target.""" if not url: - return True + return if not (url.startswith("http://") or url.startswith("https://")): raise ValueError(f"URL must start with http:// or https://, got: {url}") @@ -226,6 +226,13 @@ def wait_for_url(url: str, timeout: int, service: Service) -> bool: if not is_loopback: raise ValueError(f"URL cannot target external hostname: {hostname}") + +def wait_for_url(url: str, timeout: int, service: Service) -> bool: + """Poll a validated readiness URL until it responds or the service exits.""" + validate_readiness_url(url) + if not url: + return True + deadline = time.monotonic() + timeout opener = urllib.request.build_opener(NoRedirectHandler()) while time.monotonic() < deadline: @@ -375,6 +382,13 @@ def main(argv: Sequence[str] | None = None) -> int: print(f"sandboxed-web-e2e: isolation rejected command: {exc}", file=sys.stderr) exit_code = 126 return exit_code + try: + validate_readiness_url(args.backend_ready_url) + validate_readiness_url(args.frontend_ready_url) + except ValueError as exc: + print(f"sandboxed-web-e2e: invalid readiness URL: {exc}", file=sys.stderr) + exit_code = 125 + return exit_code services.append(start_service("backend", backend_cmd, copied_repo, command_env, logs_dir)) services.append(start_service("frontend", frontend_cmd, copied_repo, command_env, logs_dir)) try: diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index a14926321f..a229fdd41f 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -532,12 +532,14 @@ def test_main_reports_invalid_readiness_url(monkeypatch, tmp_path, capsys): """Invalid readiness input exits with the same clean readiness failure code.""" repo = tmp_path / "repo" repo.mkdir() + started = [] class DoneProcess: def poll(self): return 0 def fake_start(label, command, cwd, env, logs_dir): + started.append(label) log_path = logs_dir / f"{label}.log" log_path.write_text("ready\n", encoding="utf-8") return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) @@ -571,12 +573,61 @@ def fake_start(label, command, cwd, env, logs_dir): captured = capsys.readouterr() assert exit_code == 125 - assert "invalid readiness URL: bad host" in captured.err + assert not started + assert "invalid readiness URL: URL cannot target external hostname" in captured.err result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) assert payload["exit_code"] == 125 +def test_main_reports_readiness_exception_after_start(monkeypatch, tmp_path, capsys): + """Unexpected readiness errors after launch still clean up services.""" + repo = tmp_path / "repo" + repo.mkdir() + started = [] + + class DoneProcess: + def poll(self): + return 0 + + def fake_start(label, command, cwd, env, logs_dir): + started.append(label) + log_path = logs_dir / f"{label}.log" + return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) + + monkeypatch.setattr(sandboxed_web_e2e, "start_service", fake_start) + monkeypatch.setattr( + sandboxed_web_e2e, + "wait_for_url", + lambda url, timeout, service: (_ for _ in ()).throw(ValueError("bad host")), + ) + monkeypatch.setattr(sandboxed_web_e2e, "stop_service", lambda service: None) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--backend-ready-url", + "http://127.0.0.1:8000/health", + "--frontend-ready-url", + "http://127.0.0.1:3000/", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert started == ["backend", "frontend"] + assert "invalid readiness URL: bad host" in captured.err + + def test_main_reports_stubbed_e2e_timeout(monkeypatch, tmp_path, capsys): """Main preserves timeout output from stubbed E2E execution.""" repo = tmp_path / "repo" From 524093e130176b6bd86e2e6e4730bb182c5897ba Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 26 Aug 2026 10:42:39 +0900 Subject: [PATCH 09/21] fix(e2e): back off after server readiness errors --- scripts/ci/sandboxed_web_e2e.py | 1 + tests/test_sandboxed_web_e2e.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 9bdf0820a9..5dbdbf2b83 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -242,6 +242,7 @@ def wait_for_url(url: str, timeout: int, service: Service) -> bool: with opener.open(url, timeout=2) as response: # nosec B310 if 200 <= response.status < 500: return True + time.sleep(1) except (urllib.error.URLError, TimeoutError): time.sleep(1) return False diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index a229fdd41f..eff8e31338 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -231,7 +231,8 @@ def open(self, url, timeout): return Response(500 if len(attempts) == 2 else 204) monkeypatch.setattr(sandboxed_web_e2e.urllib.request, "build_opener", lambda *args: FakeOpener()) - monkeypatch.setattr(sandboxed_web_e2e.time, "sleep", lambda seconds: None) + sleeps = [] + monkeypatch.setattr(sandboxed_web_e2e.time, "sleep", lambda seconds: sleeps.append(seconds)) log_path = tmp_path / "service.log" log_path.write_text("\n".join(f"line-{index}" for index in range(90)), encoding="utf-8") @@ -239,6 +240,7 @@ def open(self, url, timeout): assert sandboxed_web_e2e.wait_for_url("http://127.0.0.1:8000/health", 10, service) is True assert len(attempts) == 3 + assert sleeps == [1, 1] assert sandboxed_web_e2e.tail_text(log_path).splitlines()[0] == "line-10" From c01c1aa2b5aa5d9f21659cdfb84729962208c30c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:06:03 +0000 Subject: [PATCH 10/21] fix(e2e): validate readiness ports and fail closed on isolation gaps Addresses three live Devin Review findings on PR #1347: - require_loopback_readiness_url never read the parsed port, so a nonnumeric or out-of-range port (e.g. "http://127.0.0.1:abc/health") reached urllib.request.urlopen and raised an uncaught http.client.InvalidURL instead of the documented exit code 125. Now validated up front and converted to ValueError. - isolated_command silently skipped the read-only-root validation whenever shutil.which could not resolve the executable, letting an unvalidated command through. It now fails closed the same way an out-of-bind-root executable already does. - isolation_backend accepted any bwrap binary discovered on PATH without proving it can actually create the namespaces bubblewrap needs. A bounded capability preflight (_probe_isolation_capability) now exercises the same essential namespace/mount operations isolated_command depends on and classifies a denied host as unavailable isolation (exit 126) instead of a confusing later failure. The "workspace symlinks escape filesystem isolation" finding was investigated and found not reproducible: isolated_command's --tmpfs / root replacement means any bind not explicitly listed (read-only roots, /etc identity files, /workspace) simply does not exist inside the sandbox, so a symlink to an unbound host path dangles (ENOENT) rather than resolving. Verified empirically with a real bwrap build against the actual copy_workspace/isolated_command code path. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- .../sandboxed-web-command-isolation.md | 17 ++- scripts/ci/sandboxed_web_e2e.py | 97 +++++++++++-- tests/test_sandboxed_web_e2e.py | 132 ++++++++++++++++-- 3 files changed, 216 insertions(+), 30 deletions(-) diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index ee2b6a3340..0e0f3590d4 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -8,15 +8,22 @@ minimal `/etc` identity, DNS, and time files are mounted read-only, so the host filesystem is not reachable through absolute paths or `..` traversal. Before wrapping a command, the helper resolves its executable and rejects paths -outside the read-only system roots mounted by bubblewrap. A tool installed in a -host-only location must be installed into one of those roots or the run exits -with code `126` before any service starts; the result marker records that code -and the selected backend. +outside the read-only system roots mounted by bubblewrap. An executable that +cannot be resolved at all is rejected the same way, rather than passed through +unvalidated. A tool installed in a host-only location must be installed into +one of those roots or the run exits with code `126` before any service starts; +the result marker records that code and the selected backend. Use `--isolation disabled` only for trusted local debugging. The result marker records the requested mode and resolved backend so CI evidence cannot be mistaken for an OS-isolated run. If required isolation is unavailable, the -command exits with code `126` before starting any service. +command exits with code `126` before starting any service. A `bwrap` binary on +PATH is not, by itself, taken as proof isolation works: a bounded capability +probe exercises the same essential namespace and mount operations +`isolated_command` depends on before any service starts, so a restricted host +that can locate `bwrap` but cannot create the required namespaces is also +classified as unavailable (`126`) instead of failing later as a confusing +readiness or test error. Readiness polling remains loopback-only and does not follow redirects. Invalid readiness URLs are reported as a coded readiness failure (`125`) rather than an diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 59f030c82c..14acb09cdb 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -112,6 +112,61 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: return args +BIND_ROOTS = ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") + + +def _bind_roots() -> list[Path]: + """Return the read-only host runtime roots bubblewrap mounts, if present.""" + return [Path(path) for path in BIND_ROOTS if Path(path).exists()] + + +def _probe_isolation_capability(backend: str) -> None: + """Prove bubblewrap can create the sandbox namespaces before any service starts. + + A discovered ``bwrap`` binary on PATH only proves the tool is installed; + it does not prove the host actually permits creating the unprivileged + user, PID, and mount namespaces bubblewrap depends on. A restricted Linux + host (for example one with unprivileged user namespaces disabled, or a + seccomp policy that denies ``unshare``/``clone``) can have a working + ``bwrap`` binary that still fails on every real invocation. This runs the + same essential namespace and mount operations ``isolated_command`` relies + on against a harmless no-op executable, so that kind of failure is + classified as unavailable isolation (exit code 126) instead of surfacing + later as a confusing service-readiness or test failure. + """ + probe_executable = shutil.which("true") or "/bin/true" + bind_args: list[str] = [] + for root in _bind_roots(): + bind_args.extend(("--ro-bind", str(root), str(root))) + probe_command = [ + backend, + "--die-with-parent", + "--unshare-pid", + "--tmpfs", + "/", + *bind_args, + "--proc", + "/proc", + "--dev", + "/dev", + "--", + probe_executable, + ] + try: + result = subprocess.run( + probe_command, + capture_output=True, + text=True, + timeout=10, + check=False, + ) + except (OSError, subprocess.TimeoutExpired) as exc: + raise RuntimeError(f"bubblewrap capability probe could not run: {exc}") from exc + if result.returncode != 0: + detail = result.stderr.strip() or f"exit code {result.returncode}" + raise RuntimeError(f"bubblewrap cannot create required namespaces: {detail}") + + def isolation_backend(mode: str) -> str | None: """Resolve the requested OS isolation backend without silently downgrading.""" if mode == "disabled": @@ -121,6 +176,7 @@ def isolation_backend(mode: str) -> str | None: backend = shutil.which("bwrap") if backend is None: raise RuntimeError("required isolation needs bubblewrap (bwrap) on PATH") + _probe_isolation_capability(backend) return backend @@ -143,24 +199,28 @@ def isolated_command( sandbox_root: Path, env: dict[str, str], ) -> str: - """Wrap one command in a read-only-root bubblewrap workspace.""" + """Wrap one command in a read-only-root bubblewrap workspace. + + The command's executable must resolve on ``PATH`` (or as a literal path) + and land inside the read-only bind roots. An executable ``shutil.which`` + cannot find is rejected rather than passed through unvalidated, so a + lookup failure can never silently bypass the read-only-root check it was + supposed to receive. + """ argv = shlex.split(command) if not argv: raise ValueError("command must not be empty") - bind_roots = [ - Path(path) - for path in ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") - if Path(path).exists() - ] + bind_roots = _bind_roots() executable = shutil.which(argv[0], path=env.get("PATH")) - if executable is not None: - executable_path = Path(executable) - if executable_path.is_relative_to(Path.home()): - raise RuntimeError("commands from the host home directory are not allowed in isolation") - if not any(executable_path.is_relative_to(root) for root in bind_roots): - raise RuntimeError( - f"executable is outside the isolated bind roots: {executable_path}" - ) + if executable is None: + raise RuntimeError(f"executable could not be resolved for isolation validation: {argv[0]}") + executable_path = Path(executable) + if executable_path.is_relative_to(Path.home()): + raise RuntimeError("commands from the host home directory are not allowed in isolation") + if not any(executable_path.is_relative_to(root) for root in bind_roots): + raise RuntimeError( + f"executable is outside the isolated bind roots: {executable_path}" + ) args = [backend, "--die-with-parent", "--new-session", "--unshare-pid", "--tmpfs", "/"] for root in bind_roots: args.extend(("--ro-bind", str(root), str(root))) @@ -245,13 +305,20 @@ def require_loopback_readiness_url(url: str) -> None: Literal ``localhost`` is resolved and every answer must be loopback, so a poisoned hosts file cannot smuggle a public A/AAAA record through the name allowlist. IPv4-mapped IPv6 addresses are unwrapped and re-checked - so ``::ffff:8.8.8.8`` cannot bypass the loopback rule. + so ``::ffff:8.8.8.8`` cannot bypass the loopback rule. A nonnumeric or + out-of-range port is rejected here too, so a malformed readiness URL + fails with the documented invalid-readiness diagnostic instead of an + uncaught exception once an HTTP client actually opens it. """ parsed = urllib.parse.urlparse(url) if parsed.scheme.lower() not in {"http", "https"}: raise ValueError(f"URL must start with http:// or https://, got: {url}") if parsed.username or parsed.password: raise ValueError("URL cannot include userinfo") + try: + _ = parsed.port + except ValueError as exc: + raise ValueError(f"URL has a malformed port: {url}") from exc hostname = (parsed.hostname or "").lower().rstrip(".") if not hostname: raise ValueError("URL must include a loopback hostname") diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index a9d06b93d6..a1698f9892 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -267,6 +267,14 @@ def test_wait_for_url_rejects_non_loopback_and_confused_deputy_targets(tmp_path) sandboxed_web_e2e.stop_service(exited_service) +def test_require_loopback_readiness_url_rejects_malformed_port(): + """A nonnumeric or out-of-range port fails closed instead of an uncaught exception.""" + with pytest.raises(ValueError, match="URL has a malformed port"): + sandboxed_web_e2e.require_loopback_readiness_url("http://127.0.0.1:abc/health") + with pytest.raises(ValueError, match="URL has a malformed port"): + sandboxed_web_e2e.require_loopback_readiness_url("http://127.0.0.1:99999/health") + + def test_localhost_resolution_must_stay_loopback(monkeypatch, tmp_path): """Literal localhost is allowed only when every resolved address is loopback.""" exited = subprocess.Popen([sys.executable, "-c", ""], text=True) @@ -655,6 +663,47 @@ def fake_start(label, command, cwd, env, logs_dir): assert payload["exit_code"] == 125 +def test_main_reports_malformed_readiness_port(monkeypatch, tmp_path, capsys): + """A nonnumeric readiness port exits 125 before any service starts.""" + repo = tmp_path / "repo" + repo.mkdir() + started = [] + + def fake_start(label, command, cwd, env, logs_dir): + started.append(label) + raise AssertionError("services must not start before readiness URLs are validated") + + monkeypatch.setattr(sandboxed_web_e2e, "start_service", fake_start) + monkeypatch.setattr(sandboxed_web_e2e, "stop_service", lambda service: None) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--backend-ready-url", + "http://127.0.0.1:abc/health", + "--frontend-ready-url", + "http://127.0.0.1:3000/", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert not started + assert "invalid readiness URL: URL has a malformed port" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + def test_main_reports_readiness_exception_after_start(monkeypatch, tmp_path, capsys): """Unexpected readiness errors after launch still clean up services.""" repo = tmp_path / "repo" @@ -884,9 +933,72 @@ def test_isolation_backend_returns_bwrap_path_on_linux(monkeypatch): """Linux isolation returns the resolved bubblewrap executable.""" monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/usr/bin/bwrap") + monkeypatch.setattr(sandboxed_web_e2e, "_probe_isolation_capability", lambda backend: None) assert sandboxed_web_e2e.isolation_backend("required") == "/usr/bin/bwrap" +def test_isolation_backend_fails_closed_when_namespaces_denied(monkeypatch): + """A discovered bwrap binary that cannot create namespaces is unavailable.""" + monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/usr/bin/bwrap") + monkeypatch.setattr( + sandboxed_web_e2e, + "_probe_isolation_capability", + lambda backend: (_ for _ in ()).throw(RuntimeError("bubblewrap cannot create required namespaces: denied")), + ) + with pytest.raises(RuntimeError, match="cannot create required namespaces"): + sandboxed_web_e2e.isolation_backend("required") + + +def test_probe_isolation_capability_accepts_working_bwrap(monkeypatch): + """A probe that exits zero proves bubblewrap can build the sandbox.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/true") + monkeypatch.setattr( + sandboxed_web_e2e.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess(args, 0, stdout="", stderr=""), + ) + sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") + + +def test_probe_isolation_capability_rejects_denied_namespaces(monkeypatch): + """A nonzero probe exit is classified as bubblewrap being unable to isolate.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/true") + monkeypatch.setattr( + sandboxed_web_e2e.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + args, 1, stdout="", stderr="bwrap: Creating new namespace failed: Operation not permitted" + ), + ) + with pytest.raises(RuntimeError, match="Operation not permitted"): + sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") + + +def test_probe_isolation_capability_rejects_when_probe_cannot_run(monkeypatch): + """A probe that cannot even start is classified as unavailable isolation.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/true") + + def _raise(*args, **kwargs): + raise OSError("no such file or directory") + + monkeypatch.setattr(sandboxed_web_e2e.subprocess, "run", _raise) + with pytest.raises(RuntimeError, match="could not run"): + sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") + + +def test_probe_isolation_capability_rejects_on_timeout(monkeypatch): + """A probe that hangs past its bounded timeout is classified as unavailable.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: None) + + def _raise(*args, **kwargs): + raise subprocess.TimeoutExpired(cmd="bwrap", timeout=10) + + monkeypatch.setattr(sandboxed_web_e2e.subprocess, "run", _raise) + with pytest.raises(RuntimeError, match="could not run"): + sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") + + def test_isolated_command_mounts_only_workspace(monkeypatch, tmp_path): """Bubblewrap commands expose the copied workspace and not the host root.""" monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") @@ -953,20 +1065,20 @@ def test_isolated_command_rejects_executable_outside_bound_roots(monkeypatch, tm ) -def test_isolated_command_allows_unresolved_executable_for_bwrap(monkeypatch, tmp_path): - """Commands with shell-resolved executables still receive the isolated wrapper.""" +def test_isolated_command_rejects_unresolved_executable(monkeypatch, tmp_path): + """An executable shutil.which cannot find is rejected, not silently unwrapped.""" monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) sandbox = tmp_path / "sandbox" repo = sandbox / "repo" repo.mkdir(parents=True) - command = sandboxed_web_e2e.isolated_command( - "tool", - backend="/usr/bin/bwrap", - cwd=repo, - sandbox_root=sandbox, - env={"PATH": "/usr/bin"}, - ) - assert command.startswith("/usr/bin/bwrap") + with pytest.raises(RuntimeError, match="could not be resolved"): + sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) def test_isolated_command_skips_unavailable_optional_mount(monkeypatch, tmp_path): From 4088430afaed6d4244d74873415ab5cb3375f307 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:20:16 +0000 Subject: [PATCH 11/21] Merge remote-tracking branch 'origin/fix/sandboxed-web-e2e-isolation-clean' into fix/sandboxed-web-e2e-isolation-clean # Conflicts: # docs/doctoring/sandboxed-web-command-isolation.md # scripts/ci/sandboxed_web_e2e.py # tests/test_sandboxed_web_e2e.py --- CHANGELOG.md | 15 ++ .../sandboxed-web-command-isolation.md | 50 +++++-- ...ndboxed-web-readiness-loopback-boundary.md | 25 +++- docs/product-technical-gap-baseline.md | 62 ++++++++ scripts/ci/sandboxed_verify.py | 32 +++++ scripts/ci/sandboxed_web_e2e.py | 11 +- tests/test_sandboxed_verify.py | 84 +++++++++++ tests/test_sandboxed_web_e2e.py | 134 +++++++++++++++++- 8 files changed, 392 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08024e5a14..9a0cac5b74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,21 @@ Semantic Versioning where the repository publishes a release. unavailable isolation backend or an invalid readiness URL fails closed with a clear diagnostic (exit code 126/125) instead of after services are already running. +- Close four gaps a Devin Review pass found in the same web E2E isolation + helper (`scripts/ci/sandboxed_web_e2e.py`, `scripts/ci/sandboxed_verify.py`): + a non-numeric or out-of-range readiness-URL port now raises the same + `ValueError` every other readiness check raises, instead of an uncaught + `http.client.InvalidURL` escaping past `main`'s exit-125 handling; a `bwrap` + binary on `PATH` now passes a bounded capability preflight (proving it can + actually create the sandbox's namespaces) before isolation is trusted as + available, so a restricted host fails closed with exit 126 instead of a + later, confusing readiness/test failure; an executable that cannot be + resolved on `PATH` is now a hard `isolated_command` failure rather than a + silent fallthrough that ran unwrapped and unvalidated; and the shared + workspace copy now rejects (fails the whole copy closed) any symlink whose + resolved target lands outside the copied tree, since `copytree(..., + symlinks=True)` otherwise preserves an escaping symlink as a live link + inside the bind-mounted `/workspace`. - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index 0e0f3590d4..113674ff95 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -8,22 +8,48 @@ minimal `/etc` identity, DNS, and time files are mounted read-only, so the host filesystem is not reachable through absolute paths or `..` traversal. Before wrapping a command, the helper resolves its executable and rejects paths -outside the read-only system roots mounted by bubblewrap. An executable that -cannot be resolved at all is rejected the same way, rather than passed through -unvalidated. A tool installed in a host-only location must be installed into -one of those roots or the run exits with code `126` before any service starts; -the result marker records that code and the selected backend. +outside the read-only system roots mounted by bubblewrap. A tool installed in a +host-only location must be installed into one of those roots or the run exits +with code `126` before any service starts; the result marker records that code +and the selected backend. An executable that cannot be resolved on `PATH` at +all is rejected the same way — it is never handed unvalidated to bubblewrap or +the shell to resolve on its own. + +A `bwrap` binary discovered on `PATH` is not by itself proof that isolation +works: a restricted host (unprivileged user namespaces disabled, or a +seccomp-restricted CI runner) can have the binary present yet unable to create +the requested namespaces. Before starting either service, `isolation_backend` +runs a bounded, cheap capability preflight — the same minimal namespace and +mount shape `isolated_command` uses (new PID namespace, tmpfs root, the +standard read-only binds, `/proc`, `/dev`, a tmpfs `/tmp`) around a trivial +no-op executable. A non-zero exit, or a failure to even launch the probe, is +classified as isolation-unavailable and exits with code `126`, the same as a +missing `bwrap` binary, instead of surfacing later as a confusing readiness or +test failure. Use `--isolation disabled` only for trusted local debugging. The result marker records the requested mode and resolved backend so CI evidence cannot be mistaken for an OS-isolated run. If required isolation is unavailable, the -command exits with code `126` before starting any service. A `bwrap` binary on -PATH is not, by itself, taken as proof isolation works: a bounded capability -probe exercises the same essential namespace and mount operations -`isolated_command` depends on before any service starts, so a restricted host -that can locate `bwrap` but cannot create the required namespaces is also -classified as unavailable (`126`) instead of failing later as a confusing -readiness or test error. +command exits with code `126` before starting any service. + +The workspace copy this helper and `sandboxed_verify.py` share +(`sandboxed_verify.copy_workspace`) preserves symlinks rather than +dereferencing them. Under `--isolation required`, a symlink whose absolute +target is not one of the explicitly bound paths already dangles safely +(`ENOENT`) inside bubblewrap's `tmpfs` root — verified empirically against +this code path. That containment does not extend to two paths that share the +same copy step: `--isolation disabled` (documented as trusted local debugging +only, but the copy itself makes no such distinction) runs the wrapped commands +directly on the host with no OS sandboxing at all, and `sandboxed_verify.py`'s +own verification command never runs inside bubblewrap in the first place. In +both, a repository-supplied symlink whose target is an absolute host path, or +a relative path with enough `..` segments to exit the copy, remains a live +symlink that a command following it can use to read or write host files +outside the intended workspace. Every symlink under the copy is therefore +resolved and checked against the workspace root immediately after +`shutil.copytree`, in `copy_workspace` itself so both callers get the same +protection; the first one found to escape fails the whole copy closed rather +than being silently dropped or repaired. Readiness polling remains loopback-only and does not follow redirects. Invalid readiness URLs are reported as a coded readiness failure (`125`) rather than an diff --git a/docs/doctoring/sandboxed-web-readiness-loopback-boundary.md b/docs/doctoring/sandboxed-web-readiness-loopback-boundary.md index 65438e75d4..dc555ec801 100644 --- a/docs/doctoring/sandboxed-web-readiness-loopback-boundary.md +++ b/docs/doctoring/sandboxed-web-readiness-loopback-boundary.md @@ -17,6 +17,16 @@ subdomains, cloud-metadata link-local addresses, missing hosts, and userinfo-confused URLs such as `http://user@127.0.0.1/`. A mapped public address such as `::ffff:8.8.8.8` cannot pass merely because it is IPv6. +The port is validated too: `urllib.parse.ParseResult.port` is accessed inside +the same function and any `ValueError` it raises (a non-numeric port such as +`:abc`, or one out of the 0-65535 range) is re-raised as the same `ValueError` +class every other check here raises. Before this, a malformed port passed the +URL parse silently — the port was never read — and only surfaced later as an +uncaught `http.client.InvalidURL` from the HTTP client itself, a class that is +neither `ValueError` nor `urllib.error.URLError` and so was not covered by +`main`'s exit-125 handling. It now fails the same way every other rejection +in this function does, before any request opens. + The boundary uses the standard library rather than a second address table. It therefore follows the runtime's maintained special-purpose definitions and keeps one fail-closed validation point before any network request. Do not add @@ -40,16 +50,19 @@ The regression exercises literal `localhost`, a trailing-dot `localhost.`, `127.0.0.1`, another address in `127.0.0.0/8`, IPv6 `::1`, mapped loopback `::ffff:127.0.0.1`, an unspecified address, a `.localhost` subdomain, a public hostname, the common cloud metadata address, mapped public IPv6, -userinfo, a missing host, and poisoned localhost resolution (public A, -mapped public AAAA, empty answers, resolver errors, and non-IP answers). -The existing no-redirect test continues to prove that an allowed readiness -endpoint cannot redirect the poller across the boundary. +userinfo, a missing host, poisoned localhost resolution (public A, +mapped public AAAA, empty answers, resolver errors, and non-IP answers), and +a non-numeric or out-of-range port on both the backend and frontend readiness +URL, checked through both the standalone function and a `main()` run that +never starts a service. The existing no-redirect test continues to prove that +an allowed readiness endpoint cannot redirect the poller across the boundary. ```mermaid flowchart TD Url["Readiness URL"] Scheme{"http or https?"} Userinfo{"userinfo present?"} + Port{"port numeric and 0-65535?"} Host{"loopback IP, or localhost whose every resolved answer is loopback?"} Open["Poll with redirects disabled"] Reject["Fail closed before any request"] @@ -58,7 +71,9 @@ flowchart TD Scheme -->|"no"| Reject Scheme -->|"yes"| Userinfo Userinfo -->|"yes"| Reject - Userinfo -->|"no"| Host + Userinfo -->|"no"| Port + Port -->|"no"| Reject + Port -->|"yes"| Host Host -->|"no"| Reject Host -->|"yes"| Open ``` diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 794dc9de9c..886b2f2236 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1292,6 +1292,68 @@ conflicting** PRs address pieces of this: currently blocked by the sidecar-preflight outage above, so neither could be re-reviewed to a genuine pass yet regardless of which approach wins. +## 2026-08-30 PR #1347 Devin Review 6건 검증: 4건 실재 결함 수정, 2건 확인 후 해소 + +`ContextualWisdomLab/.github#1347` (`fix/sandboxed-web-e2e-isolation-clean`, +bubblewrap 격리 + SSRF-safe readiness-URL 검증)의 commit `7ac8298b` 기준 Devin +Review 미해결 6건을 HEAD 코드 기준으로 개별 재검증했다. Finding 텍스트를 그대로 +신뢰하지 않고 각각 실제 동작을 재현해 확인했다. + +- **Finding 1 (🟡 malformed readiness port, line 423) — 실재.** + `require_loopback_readiness_url`는 `parsed.port`를 한 번도 읽지 않아, 비숫자 + 포트(`:abc`)는 `urllib.parse`를 그대로 통과한 뒤 `http.client.InvalidURL`을 + 발생시켰다 — 이 예외는 `ValueError`도 `urllib.error.URLError`도 아니어서 + `main()`의 어떤 핸들러에도 잡히지 않고 스크립트가 uncaught traceback으로 + 죽는다(재현 확인). `parsed.port` 접근을 함수 안으로 추가해 동일한 + `ValueError` 클래스로 통일했다. 백엔드/프런트엔드 readiness URL 양쪽에 대해 + 비숫자·범위초과 포트 테스트를 추가. +- **Finding 2 (🟡 installed-but-unusable isolation, line 124) — 실재.** + `isolation_backend`는 `shutil.which("bwrap")`만 확인하고 실제 namespace 생성 + 가능 여부는 전혀 검증하지 않았다. `isolated_command`가 실제로 쓰는 것과 같은 + 최소 namespace/mount 구성(new PID ns, tmpfs root, 표준 read-only bind, + `/proc`, `/dev`, tmpfs `/tmp`)으로 현재 인터프리터의 no-op(`-c pass`)을 + 5초 timeout으로 실행하는 preflight를 추가했다. 실패 시 exit 126로 조기 + 분류. +- **Finding 3 (📝 child-executable containment, line 163) — 정보성, 정확함.** + `--unshare-pid` + 암묵적 mount namespace는 wrapped 프로세스가 낳는 모든 + 자손 프로세스에도 적용되므로 추가 escape 경로가 없음을 코드로 확인. 코드 + 변경 없이 스레드에 확인 회신. +- **Finding 4 (📝 mapped-home writability, line 135) — 정보성, 정확함.** + `_sandbox_environment`가 `HOME` 등을 `/workspace` 하위로 재매핑하고, + `sandboxed_verify.scrubbed_env`가 그 경로를 미리 생성하며, `isolated_command`가 + 동일 sandbox_root를 `--bind`(read-write)로 마운트하므로 재매핑된 홈이 실제로 + 존재하고 쓰기 가능함을 확인. 코드 변경 없이 회신. +- **Finding 5 (🟥 workspace symlink escape, line 188) — 실재, 최우선 처리.** + `sandboxed_verify.copy_workspace`가 `shutil.copytree(..., symlinks=True)`를 + 써서 심볼릭 링크를 역참조 없이 그대로 보존한다는 것을 확인. 저장소에 포함된 + 심볼릭 링크가 절대경로 또는 `..` 다단 상대경로로 복사 트리 바깥을 가리키면, + 복사 후에도 그 링크가 살아있어 `/workspace`에 bind-mount된 이후 이를 + 따라가는 명령이 sandbox 경계 밖 호스트 파일에 접근할 수 있다. 복사 직후 + 트리 전체를 순회(`rglob`, 심볼릭 디렉터리 내부로는 재귀하지 않음 — 순환 + 링크로 인한 무한 루프/과다 순회 방지)하며 모든 심볼릭 링크의 최종 resolve + 경로가 sandbox root 하위인지 검증하고, 하나라도 벗어나면 복사 전체를 + `ValueError`로 fail-closed 처리하도록 `_reject_escaping_symlinks`를 추가. + 절대경로 escape, `../..` 상대경로 escape, 디렉터리 심볼릭 링크 escape, + 풀 수 없는 순환 심볼릭 링크(RuntimeError/OSError 양쪽 Python 버전 차이 + 모두 처리) 각각에 대한 회귀 테스트와, 내부 상대 심볼릭 링크는 그대로 + 보존되는지 확인하는 회귀 테스트를 추가했다. +- **Finding 6 (🟨 unresolved-executable bypass, line 156) — 실재.** + `isolated_command`는 `shutil.which(argv[0])`가 `None`을 반환하면 전체 + 검증 블록을 건너뛰고 원본 argv를 그대로 bubblewrap에 넘겼다 — 이 버그를 + 그대로 문서화하고 있던 기존 테스트 + (`test_isolated_command_allows_unresolved_executable_for_bwrap`)를 발견, + fail-closed로 전환하는 테스트로 교체했다. 해석 실패 시 다른 검증과 동일한 + `RuntimeError`(exit 126 경로)를 던지도록 수정. + +수정 파일: `scripts/ci/sandboxed_web_e2e.py`, `scripts/ci/sandboxed_verify.py`, +`tests/test_sandboxed_web_e2e.py`, `tests/test_sandboxed_verify.py`, +`docs/doctoring/sandboxed-web-command-isolation.md`, +`docs/doctoring/sandboxed-web-readiness-loopback-boundary.md`, `CHANGELOG.md`. +전체 스위트(`pytest tests`, 1924 passed) 및 대상 두 모듈 100% line/branch +coverage, 100% docstring coverage(`interrogate`), `ruff check` 모두 통과 확인. +GitHub 스레드 6건 각각에 회신하고, 실재 결함 4건 + 정보성 확인 2건 총 6건 +모두 resolve 처리. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index aace18d454..f1dd36f13a 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -138,6 +138,37 @@ def scrubbed_env(sandbox_root: Path, allow_env: Sequence[str] = ()) -> dict[str, return env +def _reject_escaping_symlinks(destination: Path) -> None: + """Fail closed if any symlink copied into the workspace resolves outside it. + + ``shutil.copytree(..., symlinks=True)`` preserves the exact target string + of every symlink instead of dereferencing it, so a repository can carry a + symlink whose (possibly absolute, possibly ``..``-laden) target resolves + outside the copied tree. A command later run against the copy — under OS + sandboxing or, in ``--isolation disabled`` debugging mode, directly on the + host — must never be able to follow such a link to read or write a file + outside the workspace boundary, defeating the isolation this module + exists to provide. Every symlink under ``destination`` is therefore fully + resolved and checked against the workspace root before the copy is + trusted; the first symlink found to escape aborts the whole copy rather + than being silently dropped or repaired, since a repository author who + plants one such link cannot be assumed not to have planted others. + """ + root = destination.resolve() + for path in destination.rglob("*"): + if not path.is_symlink(): + continue + try: + resolved = path.resolve() + except (OSError, RuntimeError) as exc: + # A symlink cycle (a -> b -> a) raises RuntimeError on some Python + # versions and OSError (ELOOP) on others; either way it cannot be + # trusted to stay inside the sandbox root. + raise ValueError(f"workspace symlink could not be resolved: {path}") from exc + if resolved != root and root not in resolved.parents: + raise ValueError(f"workspace symlink escapes the sandbox root: {path} -> {resolved}") + + def copy_workspace(repo_root: Path, sandbox_root: Path, extra_ignores: Sequence[str]) -> Path: """Copy the repository into the sandbox and return the copied root.""" source = repo_root.resolve() @@ -146,6 +177,7 @@ def copy_workspace(repo_root: Path, sandbox_root: Path, extra_ignores: Sequence[ destination = sandbox_root / "repo" ignore = shutil.ignore_patterns(*(DEFAULT_IGNORE + tuple(extra_ignores))) shutil.copytree(source, destination, ignore=ignore, symlinks=True) + _reject_escaping_symlinks(destination) return destination diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 14acb09cdb..6d2bcba743 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -130,7 +130,10 @@ def _probe_isolation_capability(backend: str) -> None: seccomp policy that denies ``unshare``/``clone``) can have a working ``bwrap`` binary that still fails on every real invocation. This runs the same essential namespace and mount operations ``isolated_command`` relies - on against a harmless no-op executable, so that kind of failure is + on (new PID namespace, tmpfs root, the standard read-only binds, + ``/proc``, ``/dev``, a tmpfs ``/tmp``) against a harmless no-op + executable (``true``, resolved on PATH so it lands inside one of the same + bind roots rather than assuming a fixed path), so that kind of failure is classified as unavailable isolation (exit code 126) instead of surfacing later as a confusing service-readiness or test failure. """ @@ -141,6 +144,7 @@ def _probe_isolation_capability(backend: str) -> None: probe_command = [ backend, "--die-with-parent", + "--new-session", "--unshare-pid", "--tmpfs", "/", @@ -149,6 +153,8 @@ def _probe_isolation_capability(backend: str) -> None: "/proc", "--dev", "/dev", + "--tmpfs", + "/tmp", "--", probe_executable, ] @@ -306,7 +312,8 @@ def require_loopback_readiness_url(url: str) -> None: poisoned hosts file cannot smuggle a public A/AAAA record through the name allowlist. IPv4-mapped IPv6 addresses are unwrapped and re-checked so ``::ffff:8.8.8.8`` cannot bypass the loopback rule. A nonnumeric or - out-of-range port is rejected here too, so a malformed readiness URL + out-of-range port is rejected here too, as the same ``ValueError`` class + every other check in this function raises, so a malformed readiness URL fails with the documented invalid-readiness diagnostic instead of an uncaught exception once an HTTP client actually opens it. """ diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index c711f34898..0c57484309 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -80,6 +80,90 @@ def test_copy_workspace_rejects_missing_repo_root(tmp_path): sandboxed_verify.copy_workspace(tmp_path / "missing", tmp_path / "sandbox", []) +def test_copy_workspace_rejects_absolute_symlink_escaping_sandbox_root(tmp_path): + """A workspace symlink pointing at a host path outside the copy fails the whole copy closed. + + ``shutil.copytree(..., symlinks=True)`` preserves a symlink's exact target + string instead of dereferencing it. Left unchecked, a repository-supplied + symlink pointing outside the copied tree would still be a live symlink + inside the workspace handed to sandboxed commands, so a command that + follows it could read or write host files outside the intended sandbox + boundary — defeating the point of the isolation this module provides. + Failing the whole copy closed guarantees the resulting tree can never be + used to reach outside the sandbox root through that link. + """ + outside = tmp_path / "outside-secret.txt" + outside.write_text("host-only-content", encoding="utf-8") + repo = tmp_path / "repo" + repo.mkdir() + (repo / "escape-link").symlink_to(outside) + + with pytest.raises(ValueError, match="workspace symlink escapes the sandbox root"): + sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + +def test_copy_workspace_rejects_relative_symlink_escaping_via_parent_traversal(tmp_path): + """A relative, ``..``-laden symlink target that exits the copied tree is also rejected.""" + outside = tmp_path / "outside-secret.txt" + outside.write_text("host-only-content", encoding="utf-8") + repo = tmp_path / "repo" + repo.mkdir() + sandbox = tmp_path / "sandbox" + # Once copied to sandbox/repo/escape-link, two ".." segments reach tmp_path. + (repo / "escape-link").symlink_to(Path("../../outside-secret.txt")) + + with pytest.raises(ValueError, match="workspace symlink escapes the sandbox root"): + sandboxed_verify.copy_workspace(repo, sandbox, []) + + +def test_copy_workspace_rejects_directory_symlink_escaping_sandbox_root(tmp_path): + """A directory symlink escaping the copy is rejected without recursing into it. + + Descending into an escaping directory symlink to look for further + problems would itself be an unbounded walk of host filesystem the sandbox + is supposed to keep out of reach; the escaping symlink must be rejected + at the point it is found, not traversed. + """ + outside_dir = tmp_path / "outside-dir" + outside_dir.mkdir() + (outside_dir / "secret.txt").write_text("host-only-content", encoding="utf-8") + repo = tmp_path / "repo" + repo.mkdir() + (repo / "escape-dir").symlink_to(outside_dir, target_is_directory=True) + + with pytest.raises(ValueError, match="workspace symlink escapes the sandbox root"): + sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + +def test_copy_workspace_rejects_unresolvable_symlink_cycle(tmp_path): + """A symlink cycle that cannot be resolved fails closed instead of crashing. + + Resolving a symlink cycle raises ``RuntimeError`` on some Python versions + and ``OSError`` (ELOOP) on others; either way it must become the same + ``ValueError`` every other escape case in this function raises. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "a").symlink_to("b") + (repo / "b").symlink_to("a") + + with pytest.raises(ValueError, match="workspace symlink could not be resolved"): + sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + +def test_copy_workspace_keeps_internal_symlinks_intact(tmp_path): + """A symlink whose target stays inside the copied tree is preserved and still resolves.""" + repo = tmp_path / "repo" + repo.mkdir() + (repo / "real.txt").write_text("payload", encoding="utf-8") + (repo / "link.txt").symlink_to("real.txt") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "link.txt").is_symlink() + assert (copied / "link.txt").read_text(encoding="utf-8") == "payload" + + def test_timeout_output_text_normalizes_subprocess_payloads(): """Timeout output normalization handles subprocess bytes and missing streams.""" assert sandboxed_verify.timeout_output_text(None) == "" diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index a1698f9892..c62ff5b909 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -325,6 +325,97 @@ def _unresolved(host, port): sandboxed_web_e2e.stop_service(exited_service) +def test_require_loopback_readiness_url_rejects_malformed_ports(): + """Non-numeric and out-of-range ports are rejected before any request opens. + + Previously this function never inspected the parsed port at all, so a + non-numeric port (e.g. ``:abc``) reached ``urllib``'s HTTP client and + raised an uncaught ``http.client.InvalidURL`` — a class that is neither + ``ValueError`` nor ``urllib.error.URLError`` and so was not covered by any + handler in this module, crashing the script instead of returning exit + code 125. Both the non-numeric and the out-of-range cases must now raise + the same ``ValueError`` class every other validation in this function + raises, on both the backend and the frontend readiness URL. + """ + for host in ("localhost", "127.0.0.1", "[::1]"): + with pytest.raises(ValueError, match=re.escape("URL has a malformed port")): + sandboxed_web_e2e.require_loopback_readiness_url(f"http://{host}:abc/ready") + with pytest.raises(ValueError, match=re.escape("URL has a malformed port")): + sandboxed_web_e2e.require_loopback_readiness_url(f"http://{host}:99999/ready") + with pytest.raises(ValueError, match=re.escape("URL has a malformed port")): + sandboxed_web_e2e.require_loopback_readiness_url(f"http://{host}:-1/ready") + + +def test_main_reports_malformed_backend_port_before_starting_services(monkeypatch, tmp_path, capsys): + """A malformed backend readiness port fails closed with exit 125, not a crash.""" + repo = tmp_path / "repo" + repo.mkdir() + started = [] + monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--backend-ready-url", + "http://127.0.0.1:abc/health", + "--frontend-ready-url", + "http://127.0.0.1:3000/", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert not started + assert "invalid readiness URL: URL has a malformed port" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + +def test_main_reports_malformed_frontend_port_before_starting_services(monkeypatch, tmp_path, capsys): + """A malformed frontend readiness port fails closed with exit 125, not a crash.""" + repo = tmp_path / "repo" + repo.mkdir() + started = [] + monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--backend-ready-url", + "http://127.0.0.1:8000/health", + "--frontend-ready-url", + "http://127.0.0.1:99999/", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert not started + assert "invalid readiness URL: URL has a malformed port" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + def test_no_redirect_handler_raises_httperror_without_following(): """Readiness checks must raise HTTPError on redirects to prevent attacker-controlled internal URLs.""" import urllib.error @@ -930,7 +1021,7 @@ def test_isolation_backend_fails_closed_without_bwrap(monkeypatch): def test_isolation_backend_returns_bwrap_path_on_linux(monkeypatch): - """Linux isolation returns the resolved bubblewrap executable.""" + """Linux isolation returns the resolved bubblewrap executable after a passing preflight.""" monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/usr/bin/bwrap") monkeypatch.setattr(sandboxed_web_e2e, "_probe_isolation_capability", lambda backend: None) @@ -999,6 +1090,40 @@ def _raise(*args, **kwargs): sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") +def test_isolation_backend_preflight_runs_with_no_readiness_urls_configured(monkeypatch, tmp_path, capsys): + """A broken bwrap is still caught, and reported as exit code 126, with no readiness URLs at all.""" + repo = tmp_path / "repo" + repo.mkdir() + monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/usr/bin/bwrap") + monkeypatch.setattr( + sandboxed_web_e2e, + "_probe_isolation_capability", + lambda backend: (_ for _ in ()).throw(RuntimeError("bubblewrap cannot create required namespaces: denied")), + ) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 126 + assert "cannot create required namespaces" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 126 + assert payload["isolation_backend"] == "unavailable" + + def test_isolated_command_mounts_only_workspace(monkeypatch, tmp_path): """Bubblewrap commands expose the copied workspace and not the host root.""" monkeypatch.setattr(sandboxed_web_e2e.platform, "system", lambda: "Linux") @@ -1066,7 +1191,12 @@ def test_isolated_command_rejects_executable_outside_bound_roots(monkeypatch, tm def test_isolated_command_rejects_unresolved_executable(monkeypatch, tmp_path): - """An executable shutil.which cannot find is rejected, not silently unwrapped.""" + """A command whose executable cannot be resolved on PATH must fail closed. + + Letting an unresolved name fall through to bubblewrap/the shell would run + it without ever receiving the read-only-root/bind-mount validation this + function exists to apply. + """ monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) sandbox = tmp_path / "sandbox" repo = sandbox / "repo" From 96f82b710ab82b3dfe8091c82ae727b9a79fd244 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:33:03 +0000 Subject: [PATCH 12/21] fix(sandboxed-verify): use strict resolve() for symlink escape check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit path.resolve() with the default strict=False does not reliably raise for a symlink cycle across Python versions — some CPython releases detect the cycle and silently return a partially-resolved path instead of raising RuntimeError/OSError. This let a genuine a->b->a symlink cycle slip past _reject_escaping_symlinks undetected on CI's Python, failing test_copy_workspace_rejects_unresolvable_symlink_cycle with "DID NOT RAISE ValueError". resolve(strict=True) requires the fully-resolved path to actually exist, so both a cycle and a plain dangling target reliably raise OSError (ELOOP/ENOENT), which the existing except clause already converts to the intended ValueError. --- scripts/ci/sandboxed_verify.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index f1dd36f13a..c90cc841eb 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -159,11 +159,15 @@ def _reject_escaping_symlinks(destination: Path) -> None: if not path.is_symlink(): continue try: - resolved = path.resolve() + resolved = path.resolve(strict=True) except (OSError, RuntimeError) as exc: - # A symlink cycle (a -> b -> a) raises RuntimeError on some Python - # versions and OSError (ELOOP) on others; either way it cannot be - # trusted to stay inside the sandbox root. + # strict=True forces resolve() to confirm the fully-resolved path + # actually exists, so both a symlink cycle (a -> b -> a) and a + # plain dangling target reliably raise OSError (ELOOP/ENOENT) here + # across Python versions. Non-strict resolve() is not sufficient: + # some CPython versions detect a cycle and silently return a + # partially-resolved path instead of raising, which would let an + # unresolvable symlink slip past this check. raise ValueError(f"workspace symlink could not be resolved: {path}") from exc if resolved != root and root not in resolved.parents: raise ValueError(f"workspace symlink escapes the sandbox root: {path} -> {resolved}") From bde444d45a78b4f362e1c948097975349357b2f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:34:25 +0000 Subject: [PATCH 13/21] fix(e2e): mirror full probe operations, exclude credential paths from sandbox copy Addresses the two remaining live Devin Review findings on PR #1347: - _probe_isolation_capability now mirrors every operation isolated_command actually performs (--new-session, /tmp tmpfs, and a writable bind+chdir into the same mount point real commands use) against a real throwaway temp directory, instead of a reduced probe that could pass on a host denying one of those specific operations and only fail later once a real service starts. - copy_workspace's default ignore list now excludes common credential-bearing dotfiles/dirs (.env*, .netrc, .npmrc, .pypirc, .pgpass, .git-credentials, .ssh, .gnupg, .aws, .kube, .docker) so a repo checkout that happens to carry one of these never rides into the sandboxed command's writable, readable /workspace mount. Logs and per-command scrubbed homes stay in that same mount deliberately (the tested command needs to write them) -- this narrows what's copied in, it does not split the mount by service. Verification: full suite 1930 passed/1 skipped/21 subtests, coverage 100% (sandboxed_verify.py 120/120, sandboxed_web_e2e.py 282/282), interrogate 100%, ruff clean. --- CHANGELOG.md | 11 +++ .../sandboxed-web-command-isolation.md | 31 ++++++-- scripts/ci/sandboxed_verify.py | 18 +++++ scripts/ci/sandboxed_web_e2e.py | 79 +++++++++++-------- tests/test_sandboxed_verify.py | 30 +++++++ tests/test_sandboxed_web_e2e.py | 26 ++++++ 6 files changed, 153 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a0cac5b74..059d9253dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,17 @@ Semantic Versioning where the repository publishes a release. resolved target lands outside the copied tree, since `copytree(..., symlinks=True)` otherwise preserves an escaping symlink as a live link inside the bind-mounted `/workspace`. +- (Devin review 반영, 후속 라운드) 같은 sandboxed web E2E isolation 헬퍼에 두 건을 추가로 + hardening했습니다: (1) `_probe_isolation_capability`가 이제 `isolated_command`가 실제로 + 수행하는 모든 연산(`--new-session`, `/tmp` tmpfs, 실제 명령이 사용하는 것과 동일한 mount + point로의 쓰기 가능한 bind+chdir)을 진짜 임시 디렉터리로 그대로 재현합니다 — 이전의 축소된 + probe는 이 중 하나를 거부하는 host에서는 통과했다가 실제 서비스 실행에서만 실패할 수 + 있었습니다. (2) `scripts/ci/sandboxed_verify.py`의 `copy_workspace` 기본 제외 목록에 + 자격증명 관련 dotfile/디렉터리(`.env*`, `.netrc`, `.npmrc`, `.pypirc`, `.pgpass`, + `.git-credentials`, `.ssh`, `.gnupg`, `.aws`, `.kube`, `.docker`)를 추가했습니다 — 쓰기 + 가능한 `/workspace` mount는 테스트 대상 명령이 읽고 쓸 수 있으므로, repo checkout에 우연히 + 존재하는 자격증명 파일이 그대로 복사되어서는 안 됩니다(로그·per-command home은 명령이 실제로 + 써야 하므로 의도적으로 동일 mount 안에 유지). - Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index 113674ff95..ecb90e7e2e 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -19,13 +19,17 @@ A `bwrap` binary discovered on `PATH` is not by itself proof that isolation works: a restricted host (unprivileged user namespaces disabled, or a seccomp-restricted CI runner) can have the binary present yet unable to create the requested namespaces. Before starting either service, `isolation_backend` -runs a bounded, cheap capability preflight — the same minimal namespace and -mount shape `isolated_command` uses (new PID namespace, tmpfs root, the -standard read-only binds, `/proc`, `/dev`, a tmpfs `/tmp`) around a trivial -no-op executable. A non-zero exit, or a failure to even launch the probe, is -classified as isolation-unavailable and exits with code `126`, the same as a -missing `bwrap` binary, instead of surfacing later as a confusing readiness or -test failure. +runs a bounded, cheap capability preflight that mirrors *every* operation +`isolated_command` actually performs — new-session creation, the new PID +namespace, tmpfs root, the standard read-only binds, `/proc`, `/dev`, a tmpfs +`/tmp`, and a writable bind+chdir into the same mount point real commands run +from, exercised against a real (throwaway) temp directory rather than a +trivial no-op. A reduced probe that skips one of these can pass on a host that +specifically denies that operation, then fail later once a real service +starts; mirroring the full set closes that gap. A non-zero exit, or a failure +to even launch the probe, is classified as isolation-unavailable and exits +with code `126`, the same as a missing `bwrap` binary, instead of surfacing +later as a confusing readiness or test failure. Use `--isolation disabled` only for trusted local debugging. The result marker records the requested mode and resolved backend so CI evidence cannot be @@ -51,6 +55,19 @@ resolved and checked against the workspace root immediately after protection; the first one found to escape fails the whole copy closed rather than being silently dropped or repaired. +The writable `/workspace` mount is a copy of the caller's repository checkout, +not the checkout itself. `copy_workspace` (`scripts/ci/sandboxed_verify.py`) +excludes VCS/cache/build noise by default, and now also excludes common +credential-bearing dotfiles/dirs a checkout can carry (`.env*`, `.netrc`, +`.npmrc`, `.pypirc`, `.pgpass`, `.git-credentials`, `.ssh`, `.gnupg`, `.aws`, +`.kube`, `.docker`) so a repository that happens to have one of these present +at copy time never rides along into the sandboxed command's writable, +readable mount. Logs and the scrubbed per-command home directories are +intentionally part of that same writable mount — the tested command needs to +write them — this exclusion list narrows what "writable and readable by the +command under test" actually contains; it does not attempt to split the mount +by service. + Readiness polling remains loopback-only and does not follow redirects. Invalid readiness URLs are reported as a coded readiness failure (`125`) rather than an uncaught traceback. The network declaration is evidence metadata; callers that diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index f1dd36f13a..2f2f74f986 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -33,6 +33,24 @@ "htmlcov", "dist", "build", + # Credential-bearing dotfiles/dirs a repo checkout can carry (npm/pip + # registry tokens, git credential helpers, cloud/SSH/GPG config). The + # sandboxed command's own workspace mount is writable, so anything copied + # in here is both readable and tamperable by the command under test -- + # these must never ride along with an ordinary repo copy. + ".env", + ".env.*", + ".envrc", + ".netrc", + ".npmrc", + ".pypirc", + ".pgpass", + ".git-credentials", + ".ssh", + ".gnupg", + ".aws", + ".kube", + ".docker", ) SECRET_ENV_TOKENS = ( "TOKEN", diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 6d2bcba743..db714f01c3 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -128,46 +128,55 @@ def _probe_isolation_capability(backend: str) -> None: user, PID, and mount namespaces bubblewrap depends on. A restricted Linux host (for example one with unprivileged user namespaces disabled, or a seccomp policy that denies ``unshare``/``clone``) can have a working - ``bwrap`` binary that still fails on every real invocation. This runs the - same essential namespace and mount operations ``isolated_command`` relies - on (new PID namespace, tmpfs root, the standard read-only binds, - ``/proc``, ``/dev``, a tmpfs ``/tmp``) against a harmless no-op - executable (``true``, resolved on PATH so it lands inside one of the same - bind roots rather than assuming a fixed path), so that kind of failure is - classified as unavailable isolation (exit code 126) instead of surfacing + ``bwrap`` binary that still fails on every real invocation. This mirrors + every operation ``isolated_command`` actually performs -- new-session + creation, the new PID namespace, tmpfs root, the standard read-only + binds, ``/proc``, ``/dev``, a tmpfs ``/tmp``, and a writable bind+chdir + into the same mount point real commands run from -- against a real + (throwaway) temp directory, so a host that permits a reduced probe but + denies one of these still-untested operations is classified as + unavailable isolation (exit code 126) up front, instead of surfacing later as a confusing service-readiness or test failure. """ - probe_executable = shutil.which("true") or "/bin/true" + probe_executable = shutil.which("sh") or "/bin/sh" bind_args: list[str] = [] for root in _bind_roots(): bind_args.extend(("--ro-bind", str(root), str(root))) - probe_command = [ - backend, - "--die-with-parent", - "--new-session", - "--unshare-pid", - "--tmpfs", - "/", - *bind_args, - "--proc", - "/proc", - "--dev", - "/dev", - "--tmpfs", - "/tmp", - "--", - probe_executable, - ] - try: - result = subprocess.run( - probe_command, - capture_output=True, - text=True, - timeout=10, - check=False, - ) - except (OSError, subprocess.TimeoutExpired) as exc: - raise RuntimeError(f"bubblewrap capability probe could not run: {exc}") from exc + with tempfile.TemporaryDirectory(prefix="sandboxed-web-e2e-probe-") as probe_workspace: + probe_command = [ + backend, + "--die-with-parent", + "--new-session", + "--unshare-pid", + "--tmpfs", + "/", + *bind_args, + "--proc", + "/proc", + "--dev", + "/dev", + "--tmpfs", + "/tmp", + "--bind", + probe_workspace, + SANDBOX_MOUNT, + "--chdir", + SANDBOX_MOUNT, + "--", + probe_executable, + "-c", + f"test -w {SANDBOX_MOUNT} && test -w /tmp", + ] + try: + result = subprocess.run( + probe_command, + capture_output=True, + text=True, + timeout=10, + check=False, + ) + except (OSError, subprocess.TimeoutExpired) as exc: + raise RuntimeError(f"bubblewrap capability probe could not run: {exc}") from exc if result.returncode != 0: detail = result.stderr.strip() or f"exit code {result.returncode}" raise RuntimeError(f"bubblewrap cannot create required namespaces: {detail}") diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index 0c57484309..036a133d83 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -74,6 +74,36 @@ def test_copy_workspace_excludes_default_noise_and_keeps_sources(tmp_path): assert not (copied / "__pycache__").exists() +def test_copy_workspace_excludes_credential_bearing_paths(tmp_path): + """A repo checkout's credential files must never ride into the writable sandbox.""" + repo = tmp_path / "repo" + repo.mkdir() + (repo / "script.py").write_text("print('ok')\n", encoding="utf-8") + (repo / ".env").write_text("SECRET=leaked\n", encoding="utf-8") + (repo / ".env.production").write_text("SECRET=leaked\n", encoding="utf-8") + (repo / ".npmrc").write_text("//registry.example.com/:_authToken=leaked\n", encoding="utf-8") + (repo / ".netrc").write_text("machine example.com login x password leaked\n", encoding="utf-8") + (repo / ".git-credentials").write_text("https://x:leaked@example.com\n", encoding="utf-8") + (repo / ".ssh").mkdir() + (repo / ".ssh" / "id_rsa").write_text("leaked-key\n", encoding="utf-8") + (repo / ".aws").mkdir() + (repo / ".aws" / "credentials").write_text("leaked\n", encoding="utf-8") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "script.py").read_text(encoding="utf-8") == "print('ok')\n" + for excluded in ( + ".env", + ".env.production", + ".npmrc", + ".netrc", + ".git-credentials", + ".ssh", + ".aws", + ): + assert not (copied / excluded).exists(), excluded + + def test_copy_workspace_rejects_missing_repo_root(tmp_path): """Workspace copy fails clearly when the source root is invalid.""" with pytest.raises(ValueError, match="repo root is not a directory"): diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index c62ff5b909..eab75b1d5c 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -1078,6 +1078,32 @@ def _raise(*args, **kwargs): sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") +def test_probe_isolation_capability_exercises_the_same_operations_as_real_commands(monkeypatch): + """The probe must not pass on a host that would fail the real invocation. + + A reduced probe (missing --new-session, /tmp, or the bind+chdir into the + same mount real commands use) can pass on a host that denies one of + those specific operations, then fail later once a real service starts. + """ + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/sh") + captured: dict[str, object] = {} + + def _fake_run(command, **kwargs): + captured["command"] = command + return subprocess.CompletedProcess(command, 0, stdout="", stderr="") + + monkeypatch.setattr(sandboxed_web_e2e.subprocess, "run", _fake_run) + sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") + + command = captured["command"] + assert "--new-session" in command + assert command.count("--tmpfs") == 2 + assert "/tmp" in command + assert "--bind" in command + assert sandboxed_web_e2e.SANDBOX_MOUNT in command + assert "--chdir" in command + + def test_probe_isolation_capability_rejects_on_timeout(monkeypatch): """A probe that hangs past its bounded timeout is classified as unavailable.""" monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: None) From be77d299ec1226c2289fd946c81d75129d6be60f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:44:40 +0000 Subject: [PATCH 14/21] fix(sandboxed-verify): use lexical symlink walk, not resolve(), to reject escapes Devin flagged that _reject_escaping_symlinks's Path.resolve(strict=True) rejected any unresolvable symlink uniformly, including a plain dangling target whose own file was legitimately excluded from the copy by DEFAULT_IGNORE/extra_ignores (or is simply broken) -- aborting an otherwise-valid verification run over a link that was never actually a host-escape attempt. Replaced the resolve()-based check with a hop-by-hop lexical walk (os.readlink + os.path.normpath, tracked via a visited set), matching the existing design in the parallel PR #1280's sandboxed_verify.py. This never requires a target to exist, so a dangling-but-contained symlink is now accepted while an actual escape (absolute target, or a normalized target outside the sandbox root) or an unresolvable cycle (revisiting an already-followed path) still raises the same ValueError as before. The hop count is bounded so a chain that never repeats due to purely lexical normalization still fails closed instead of walking forever. Added regression tests for: a dangling target with no cycle, a target excluded from the copy by DEFAULT_IGNORE, and a chain exceeding the hop limit without ever cycling or escaping. --- scripts/ci/sandboxed_verify.py | 72 ++++++++++++++++++++++++---------- tests/test_sandboxed_verify.py | 66 +++++++++++++++++++++++++++++-- 2 files changed, 114 insertions(+), 24 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index 6360dd15e9..136b4245f5 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -75,6 +75,7 @@ ) RESULT_MARKER = "SANDBOXED_VERIFY_RESULT" ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +MAXIMUM_SYMLINK_HOPS = 40 def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: @@ -166,29 +167,60 @@ def _reject_escaping_symlinks(destination: Path) -> None: sandboxing or, in ``--isolation disabled`` debugging mode, directly on the host — must never be able to follow such a link to read or write a file outside the workspace boundary, defeating the isolation this module - exists to provide. Every symlink under ``destination`` is therefore fully - resolved and checked against the workspace root before the copy is - trusted; the first symlink found to escape aborts the whole copy rather - than being silently dropped or repaired, since a repository author who - plants one such link cannot be assumed not to have planted others. + exists to provide. Every symlink under ``destination`` is walked hop by + hop purely lexically (see ``_reject_escaping_symlink_chain``), so a link + whose own target was itself excluded from the copy by ``DEFAULT_IGNORE`` + or ``extra_ignores`` -- or is simply broken -- is not confused with one + that escapes; the first symlink found to actually escape, or whose chain + cannot be resolved, aborts the whole copy rather than being silently + dropped or repaired, since a repository author who plants one such link + cannot be assumed not to have planted others. """ - root = destination.resolve() + root = destination.resolve(strict=True) for path in destination.rglob("*"): - if not path.is_symlink(): - continue + if path.is_symlink(): + _reject_escaping_symlink_chain(path, root) + + +def _reject_escaping_symlink_chain(candidate: Path, root: Path) -> None: + """Walk one symlink's target chain lexically, raising on escape or cycle. + + Uses ``os.readlink`` plus ``os.path.normpath`` at every hop instead of + ``Path.resolve()``, which requires the fully-resolved path to exist + (``strict=True``) or is unreliable for detecting a cycle across Python + versions (``strict=False``, the default) -- either way conflating a + symlink escape with a symlink that merely points at a target this + function never had to check for existence. A dangling target -- for + example one whose file was excluded from the copy by ``DEFAULT_IGNORE`` + -- is therefore accepted as long as it still lexically resolves inside + ``root``: verification must still run despite the broken link. Only an + absolute target, a target that normalizes outside ``root``, or a chain + that revisits a path it has already followed (an unresolvable cycle) + raises. The hop count is bounded so a chain that never repeats (due to + purely lexical, not real-path, normalization) still fails closed instead + of walking forever. + """ + visited: set[Path] = set() + current = candidate + for _ in range(MAXIMUM_SYMLINK_HOPS): + if current in visited: + raise ValueError(f"workspace symlink could not be resolved: {candidate}") + visited.add(current) + if not current.is_symlink(): + return + target = Path(os.readlink(current)) + if target.is_absolute(): + raise ValueError( + f"workspace symlink escapes the sandbox root: {candidate} -> {target}" + ) + current = Path(os.path.normpath(current.parent / target)) try: - resolved = path.resolve(strict=True) - except (OSError, RuntimeError) as exc: - # strict=True forces resolve() to confirm the fully-resolved path - # actually exists, so both a symlink cycle (a -> b -> a) and a - # plain dangling target reliably raise OSError (ELOOP/ENOENT) here - # across Python versions. Non-strict resolve() is not sufficient: - # some CPython versions detect a cycle and silently return a - # partially-resolved path instead of raising, which would let an - # unresolvable symlink slip past this check. - raise ValueError(f"workspace symlink could not be resolved: {path}") from exc - if resolved != root and root not in resolved.parents: - raise ValueError(f"workspace symlink escapes the sandbox root: {path} -> {resolved}") + current.relative_to(root) + except ValueError as exc: + raise ValueError( + f"workspace symlink escapes the sandbox root: {candidate} -> {target}" + ) from exc + raise ValueError(f"workspace symlink could not be resolved: {candidate}") def copy_workspace(repo_root: Path, sandbox_root: Path, extra_ignores: Sequence[str]) -> Path: diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index 036a133d83..169c2a66fb 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -166,11 +166,12 @@ def test_copy_workspace_rejects_directory_symlink_escaping_sandbox_root(tmp_path def test_copy_workspace_rejects_unresolvable_symlink_cycle(tmp_path): - """A symlink cycle that cannot be resolved fails closed instead of crashing. + """A symlink cycle that never terminates fails closed instead of hanging. - Resolving a symlink cycle raises ``RuntimeError`` on some Python versions - and ``OSError`` (ELOOP) on others; either way it must become the same - ``ValueError`` every other escape case in this function raises. + The chain walk tracks every path it has already followed; revisiting one + without ever leaving the sandbox root means the chain cannot be resolved + to a real, bounded target, so it becomes the same ``ValueError`` every + other unresolvable case in this function raises. """ repo = tmp_path / "repo" repo.mkdir() @@ -194,6 +195,63 @@ def test_copy_workspace_keeps_internal_symlinks_intact(tmp_path): assert (copied / "link.txt").read_text(encoding="utf-8") == "payload" +def test_copy_workspace_keeps_symlink_dangling_from_a_missing_internal_target(tmp_path): + """A symlink whose target was never present is accepted, not treated as an escape. + + A dangling target is not evidence of an escape attempt: the link's own + normalized path still lands inside the sandbox root, it simply names a + file that does not exist. Verification must still run against the rest + of the copy instead of aborting the whole copy over a broken link. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "dangling.txt").symlink_to("does-not-exist.txt") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "dangling.txt").is_symlink() + assert not (copied / "dangling.txt").exists() + + +def test_copy_workspace_rejects_symlink_chain_past_the_hop_limit(tmp_path): + """A long, never-repeating, never-escaping symlink chain still fails closed. + + Purely lexical normalization means a chain of distinct symlink names can + walk forever without ever revisiting a path or leaving the sandbox root; + the hop limit exists precisely to bound that case instead of hanging. + """ + repo = tmp_path / "repo" + repo.mkdir() + chain_length = sandboxed_verify.MAXIMUM_SYMLINK_HOPS + 5 + for index in range(chain_length): + (repo / f"hop-{index}").symlink_to(f"hop-{index + 1}") + (repo / f"hop-{chain_length}").write_text("payload", encoding="utf-8") + + with pytest.raises(ValueError, match="workspace symlink could not be resolved"): + sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + +def test_copy_workspace_keeps_symlink_whose_target_was_excluded_from_the_copy(tmp_path): + """A symlink into a directory excluded by DEFAULT_IGNORE is accepted, not an escape. + + ``shutil.copytree``'s ignore patterns can omit a symlink's target from + the copy (for example a link into ``node_modules``) while the link + itself, sitting outside the ignored directory, is still copied. The + resulting dangling link is workspace-bound and must not abort the copy. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "node_modules").mkdir() + (repo / "node_modules" / "leaf.js").write_text("module.exports = {}", encoding="utf-8") + (repo / "bin-link.js").symlink_to("node_modules/leaf.js") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert not (copied / "node_modules").exists() + assert (copied / "bin-link.js").is_symlink() + assert not (copied / "bin-link.js").exists() + + def test_timeout_output_text_normalizes_subprocess_payloads(): """Timeout output normalization handles subprocess bytes and missing streams.""" assert sandboxed_verify.timeout_output_text(None) == "" From fe237c4f28c69d2acbf4988bea805e7c1c8d0120 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:51:06 +0000 Subject: [PATCH 15/21] fix(sandboxed-verify): fix off-by-one in symlink hop-limit walk Devin flagged that a valid chain of exactly MAXIMUM_SYMLINK_HOPS (40) real, OS-resolvable symlinks was incorrectly rejected: each loop iteration checks one position and, if it is a symlink, advances to the next -- so resolving N real hops needs N+1 checks (N to walk them, one more to confirm the final landing position is not itself a further symlink). range(MAXIMUM_SYMLINK_HOPS) only provided N checks, so the walk always fell through to "could not be resolved" one check short of reaching a real target on an exactly-N-hop chain. Reproduced directly: a 40-real-symlink chain terminating in a real file raised before the fix and copies cleanly after. Fix: range(MAXIMUM_SYMLINK_HOPS + 1). Verified this doesn't change any existing test's outcome, including the past-the-limit and cycle-detection tests (a cycle is still caught well within the budget regardless of the +1; a chain intentionally longer than the limit still exceeds the new budget too). Added a boundary regression test for exactly-N-hops success to lock this in. Full suite: 1934 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings. --- scripts/ci/sandboxed_verify.py | 9 +++++++-- tests/test_sandboxed_verify.py | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index 136b4245f5..f9ddda9d73 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -198,11 +198,16 @@ def _reject_escaping_symlink_chain(candidate: Path, root: Path) -> None: that revisits a path it has already followed (an unresolvable cycle) raises. The hop count is bounded so a chain that never repeats (due to purely lexical, not real-path, normalization) still fails closed instead - of walking forever. + of walking forever. The walk allows one more iteration than the hop + limit: each iteration checks one position and then, if it is a symlink, + advances to the next, so a chain of exactly ``MAXIMUM_SYMLINK_HOPS`` + real, resolvable symlinks needs a final iteration to confirm the landing + position is not itself a further symlink -- without it, such a chain + would be rejected even though the OS itself can resolve it. """ visited: set[Path] = set() current = candidate - for _ in range(MAXIMUM_SYMLINK_HOPS): + for _ in range(MAXIMUM_SYMLINK_HOPS + 1): if current in visited: raise ValueError(f"workspace symlink could not be resolved: {candidate}") visited.add(current) diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index 169c2a66fb..a2e6cda8d9 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -231,6 +231,29 @@ def test_copy_workspace_rejects_symlink_chain_past_the_hop_limit(tmp_path): sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) +def test_copy_workspace_accepts_a_chain_of_exactly_the_hop_limit(tmp_path): + """A chain of exactly MAXIMUM_SYMLINK_HOPS real symlinks is still accepted. + + The walk checks one position per iteration and only advances past it if + it is itself a further symlink, so resolving a chain of N real symlinks + needs N+1 checks: one per hop, plus one to confirm the final landing + position is a real, non-symlink target. A chain of exactly the hop limit + is something the OS can resolve without issue and must not be rejected. + """ + repo = tmp_path / "repo" + repo.mkdir() + chain_length = sandboxed_verify.MAXIMUM_SYMLINK_HOPS + for index in range(chain_length - 1): + (repo / f"hop-{index}").symlink_to(f"hop-{index + 1}") + (repo / f"hop-{chain_length - 1}").symlink_to("real.txt") + (repo / "real.txt").write_text("payload", encoding="utf-8") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "hop-0").is_symlink() + assert (copied / "real.txt").read_text(encoding="utf-8") == "payload" + + def test_copy_workspace_keeps_symlink_whose_target_was_excluded_from_the_copy(tmp_path): """A symlink into a directory excluded by DEFAULT_IGNORE is accepted, not an escape. From 297bcea3458545d47df1aa60c3b5a6c9d9fc1636 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 15:05:52 +0000 Subject: [PATCH 16/21] fix(sandboxed-verify): resolve symlink targets component-by-component Devin flagged "nested directory links escape sandbox" on the parallel PR #1280, which shares this file's hop-walk design: a symlink target that itself contains an intermediate component which is a symlink (e.g. "some-alias/../secret") defeats a check that collapses the whole target string in one os.path.normpath call, because normpath cancels "some-alias" against the following ".." purely textually without ever re-examining whether some-alias is itself a symlink needing its own resolution first. Reproduced directly against this file (not just #1280's): a self-alias symlink pointing at "." (its own parent, the repo root -- entirely legitimate and safe standing on its own) combined with a second symlink whose target is "self-alias/../outside-secret.txt" was NOT caught -- the whole string collapsed lexically to "outside-secret.txt" (looking safe), while resolving it for real, one component at a time, correctly shows that following self-alias lands at the repo root itself (zero depth), so the very next ".." immediately exits it. Fix: replaced the whole-target os.path.normpath collapse with a component-by-component walk that re-checks is_symlink() after every single path segment, substituting a symlink's own target components back onto the work queue instead of treating the whole original target string as one atomic lexical unit. This also fixes the hop-limit off-by-one from fe237c4f as a natural consequence: a hop's budget is now spent only when a symlink is actually dereferenced, not once per loop iteration, so a chain of exactly MAXIMUM_SYMLINK_HOPS real symlinks needs no special-casing. Verified against all existing symlink tests unchanged (escape, absolute, internal, dangling, excluded-by-DEFAULT_IGNORE, cycle, hop-limit boundary) plus two new ones: the nested-alias escape (must now raise) and a legitimate cross-directory ".." traversal that stays in-bounds (must still be accepted, for branch coverage on the successful ".." path). Full suite: 1969 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings. --- scripts/ci/sandboxed_verify.py | 70 +++++++++++++++++++++------------- tests/test_sandboxed_verify.py | 43 +++++++++++++++++++++ 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index f9ddda9d73..3aa60d4273 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -194,38 +194,54 @@ def _reject_escaping_symlink_chain(candidate: Path, root: Path) -> None: example one whose file was excluded from the copy by ``DEFAULT_IGNORE`` -- is therefore accepted as long as it still lexically resolves inside ``root``: verification must still run despite the broken link. Only an - absolute target, a target that normalizes outside ``root``, or a chain - that revisits a path it has already followed (an unresolvable cycle) - raises. The hop count is bounded so a chain that never repeats (due to - purely lexical, not real-path, normalization) still fails closed instead - of walking forever. The walk allows one more iteration than the hop - limit: each iteration checks one position and then, if it is a symlink, - advances to the next, so a chain of exactly ``MAXIMUM_SYMLINK_HOPS`` - real, resolvable symlinks needs a final iteration to confirm the landing - position is not itself a further symlink -- without it, such a chain - would be rejected even though the OS itself can resolve it. + absolute target, a target segment that normalizes outside ``root``, or a + chain that revisits a symlink it has already followed (an unresolvable + cycle) raises. The walk processes the candidate's path one component at a + time rather than resolving each symlink's whole target in a single + ``os.path.normpath`` call, because a target can itself contain an + intermediate component that is a symlink -- for example + ``some-alias/../secret`` where ``some-alias`` is itself a relative, + entirely-legitimate-looking internal symlink. Collapsing that whole + string lexically in one step would cancel ``some-alias`` against the + following ``..`` textually, silently ignoring that following + ``some-alias`` for real can land somewhere shallower or deeper than one + directory level -- exactly the gap a real ``os.path.normpath`` call + cannot see, since it never re-examines whether an intermediate segment is + itself a symlink needing its own resolution first. Processing one + component at a time and re-checking ``is_symlink()`` after every step + closes that gap without ever calling ``Path.resolve()``. A hop budget + bounds the total number of symlinks followed so a chain that never + repeats still fails closed instead of walking forever; only actually + dereferencing a symlink spends one unit of that budget, so a chain of + exactly ``MAXIMUM_SYMLINK_HOPS`` real, resolvable symlinks is accepted. """ - visited: set[Path] = set() - current = candidate - for _ in range(MAXIMUM_SYMLINK_HOPS + 1): - if current in visited: + seen: set[Path] = set() + resolved = root + stack = list(candidate.relative_to(root).parts) + hops_remaining = MAXIMUM_SYMLINK_HOPS + while stack: + component = stack.pop(0) + if component == "..": + if resolved == root: + raise ValueError(f"workspace symlink escapes the sandbox root: {candidate}") + resolved = resolved.parent + continue + step = resolved / component + if not step.is_symlink(): + resolved = step + continue + if step in seen: raise ValueError(f"workspace symlink could not be resolved: {candidate}") - visited.add(current) - if not current.is_symlink(): - return - target = Path(os.readlink(current)) + if hops_remaining <= 0: + raise ValueError(f"workspace symlink could not be resolved: {candidate}") + seen.add(step) + hops_remaining -= 1 + target = Path(os.readlink(step)) if target.is_absolute(): raise ValueError( - f"workspace symlink escapes the sandbox root: {candidate} -> {target}" + f"workspace symlink escapes the sandbox root: {step} -> {target}" ) - current = Path(os.path.normpath(current.parent / target)) - try: - current.relative_to(root) - except ValueError as exc: - raise ValueError( - f"workspace symlink escapes the sandbox root: {candidate} -> {target}" - ) from exc - raise ValueError(f"workspace symlink could not be resolved: {candidate}") + stack = list(target.parts) + stack def copy_workspace(repo_root: Path, sandbox_root: Path, extra_ignores: Sequence[str]) -> Path: diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index a2e6cda8d9..225b1b4934 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -165,6 +165,28 @@ def test_copy_workspace_rejects_directory_symlink_escaping_sandbox_root(tmp_path sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) +def test_copy_workspace_rejects_escape_via_intermediate_directory_alias(tmp_path): + """An intermediate alias component inside a target is resolved, not skipped. + + ``self-alias`` points at ``.`` (its own parent, the repo root) -- entirely + legitimate and safe standing on its own. But ``link``'s target, + ``self-alias/../outside-secret.txt``, only *looks* safe if the whole + string is collapsed lexically in one step (``self-alias/..`` cancels to + nothing, leaving what looks like a plain in-repo reference). Resolved for + real, component by component, following ``self-alias`` lands at the repo + root itself (zero depth), so the very next ``..`` immediately exits the + repo. A check that only ran ``os.path.normpath`` on the whole target + string once would miss this; walking one component at a time must not. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "self-alias").symlink_to(".", target_is_directory=True) + (repo / "link").symlink_to("self-alias/../outside-secret.txt") + + with pytest.raises(ValueError, match="workspace symlink escapes the sandbox root"): + sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + def test_copy_workspace_rejects_unresolvable_symlink_cycle(tmp_path): """A symlink cycle that never terminates fails closed instead of hanging. @@ -195,6 +217,27 @@ def test_copy_workspace_keeps_internal_symlinks_intact(tmp_path): assert (copied / "link.txt").read_text(encoding="utf-8") == "payload" +def test_copy_workspace_keeps_cross_directory_symlink_using_parent_traversal(tmp_path): + """A relative ``..`` that climbs back into the repo, not out of it, is accepted. + + ``subdir/link.txt -> ../sibling.txt`` needs exactly one ``..`` to reach a + real sibling file at the repo root -- a common, legitimate pattern (e.g. + ``bin/tool -> ../lib/tool``). This must not be confused with a ``..`` + that pops above the sandbox root itself. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "sibling.txt").write_text("payload", encoding="utf-8") + subdir = repo / "subdir" + subdir.mkdir() + (subdir / "link.txt").symlink_to("../sibling.txt") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "subdir" / "link.txt").is_symlink() + assert (copied / "subdir" / "link.txt").read_text(encoding="utf-8") == "payload" + + def test_copy_workspace_keeps_symlink_dangling_from_a_missing_internal_target(tmp_path): """A symlink whose target was never present is accepted, not treated as an escape. From fe68c2fa3d549e10240524a1ecf6f294e09b02d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 15:08:19 +0000 Subject: [PATCH 17/21] fix(sandboxed-ci): repo launchers, env templates, shell shadowing, blank commands Devin flagged five findings on this PR; four are confirmed real bugs, fixed here with fail-before/pass-after regression tests. The fifth (readiness probes sharing the runner's loopback network namespace) is a real gap but has no small, non-regressing fix available -- posted as a separate PR comment for maintainer review instead of a speculative code change. 1. sandboxed_web_e2e.isolated_command rejected valid repository launchers (e.g. ./gradlew): shutil.which resolves any path-separator-bearing command against the *wrapper process's* own cwd, never against the copied repository's cwd the caller actually passes in. Added _resolve_isolated_executable, which resolves an explicit-path argv[0] against the sandboxed cwd instead, and widened the bind-root check to also permit executables inside sandbox_root (mounted at /workspace), while still fail-closed rejecting path traversal and external paths. 2. sandboxed_verify's DEFAULT_IGNORE ".env.*" glob excluded committed, secret-free templates (.env.example, .env.sample, .env.template) right along with real dotenv credential files. Added DEFAULT_ENV_TEMPLATE_ALLOWLIST and _ignore_with_env_template_allowlist, which wraps shutil.ignore_patterns to spare those specific names. 3. _probe_isolation_capability resolved its probe shell via shutil.which("sh") against the caller's own PATH, which can return a binary outside every root isolated_command actually bind-mounts (e.g. a PATH entry shadowing sh with a home-directory executable). That shell is invisible inside the sandbox, so a real working bubblewrap install fails the probe. Added _probe_shell(), which only picks from PROBE_SHELL_PATHS (/bin/sh, /usr/bin/sh) -- the same mounted roots isolated_command uses -- and fails clearly if neither exists. 4. A whitespace-only backend/frontend/e2e command made isolated_command raise ValueError("command must not be empty"), which the call site's except clause (catching only RuntimeError) let propagate as an uncaught traceback instead of the documented isolation-rejection exit code 126. The except clause now also catches ValueError. Validation: PYTHONPATH=. python3 -m pytest tests -q -> 1942 passed, 1 skipped, 21 subtests passed. coverage on scripts/ci/sandboxed_verify.py and scripts/ci/sandboxed_web_e2e.py -> 100% statement+branch. Full-repo coverage and interrogate docstring coverage both 100%. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/sandboxed_verify.py | 38 ++++++- scripts/ci/sandboxed_web_e2e.py | 84 ++++++++++++++-- tests/test_sandboxed_verify.py | 30 ++++++ tests/test_sandboxed_web_e2e.py | 171 ++++++++++++++++++++++++++++++++ 4 files changed, 310 insertions(+), 13 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index f9ddda9d73..4f0ee7cbb9 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -11,7 +11,7 @@ import sys import tempfile import time -from collections.abc import Sequence +from collections.abc import Callable, Sequence from pathlib import Path @@ -41,6 +41,8 @@ ".env", ".env.*", ".envrc", + # Note: DEFAULT_ENV_TEMPLATE_ALLOWLIST below carves committed, + # secret-free dotenv templates back out of the ".env.*" glob above. ".netrc", ".npmrc", ".pypirc", @@ -73,6 +75,16 @@ "TZ", "PYTHONPATH", ) +# Committed, secret-free dotenv templates. These match the ".env.*" glob in +# DEFAULT_IGNORE (which exists to exclude real credential-bearing dotenv +# variants such as ".env.local" or ".env.production") but carry no secrets +# themselves, so verification commands that read them for local defaults +# must still find them in the sandboxed copy. +DEFAULT_ENV_TEMPLATE_ALLOWLIST = ( + ".env.example", + ".env.sample", + ".env.template", +) RESULT_MARKER = "SANDBOXED_VERIFY_RESULT" ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") MAXIMUM_SYMLINK_HOPS = 40 @@ -228,13 +240,35 @@ def _reject_escaping_symlink_chain(candidate: Path, root: Path) -> None: raise ValueError(f"workspace symlink could not be resolved: {candidate}") +def _ignore_with_env_template_allowlist(patterns: Sequence[str]) -> Callable[[str, list[str]], set[str]]: + """Build a ``copytree`` ignore function that spares committed env templates. + + ``shutil.ignore_patterns`` has no way to match a glob like ``.env.*`` + while excepting specific names from it, so a committed, secret-free + template such as ``.env.example`` matches the same pattern used to + exclude real credential-bearing dotenv files and would otherwise vanish + from the sandboxed copy right along with them. This wraps the + pattern-based ignore function and un-ignores any name found in + ``DEFAULT_ENV_TEMPLATE_ALLOWLIST`` after the underlying patterns have + been applied. + """ + base_ignore = shutil.ignore_patterns(*patterns) + + def _ignore(directory: str, names: list[str]) -> set[str]: + """Apply the pattern-based ignore rules, then spare env template names.""" + ignored = base_ignore(directory, names) + return {name for name in ignored if name not in DEFAULT_ENV_TEMPLATE_ALLOWLIST} + + return _ignore + + def copy_workspace(repo_root: Path, sandbox_root: Path, extra_ignores: Sequence[str]) -> Path: """Copy the repository into the sandbox and return the copied root.""" source = repo_root.resolve() if not source.is_dir(): raise ValueError(f"repo root is not a directory: {source}") destination = sandbox_root / "repo" - ignore = shutil.ignore_patterns(*(DEFAULT_IGNORE + tuple(extra_ignores))) + ignore = _ignore_with_env_template_allowlist(DEFAULT_IGNORE + tuple(extra_ignores)) shutil.copytree(source, destination, ignore=ignore, symlinks=True) _reject_escaping_symlinks(destination) return destination diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index db714f01c3..c8d1e41462 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -120,6 +120,39 @@ def _bind_roots() -> list[Path]: return [Path(path) for path in BIND_ROOTS if Path(path).exists()] +# Common system shell locations that live under BIND_ROOTS -- the only host +# paths isolated_command actually bind-mounts read-only into the sandbox. +# Covers both a traditional split /bin and /usr/bin and a merged-/usr layout +# where /bin is itself a symlink into /usr/bin. +PROBE_SHELL_PATHS = ("/bin/sh", "/usr/bin/sh") + + +def _probe_shell() -> str: + """Pick a probe shell that is guaranteed to be visible inside the sandbox. + + ``isolated_command`` only ever bind-mounts ``BIND_ROOTS`` (plus a small + fixed set of ``/etc`` files) read-only into the sandbox. Resolving the + probe shell from the caller's own ``PATH`` -- as opposed to this fixed, + known-mounted set -- can return a binary that lives outside every mounted + root, for example when a ``PATH`` entry earlier than the system one + shadows ``sh`` with a home-directory executable. Such a shell is invisible + inside the sandbox, so the probe fails even though a real invocation + using an actually-mounted shell would succeed, misclassifying a working + host as one with isolation unavailable. Restricting the choice to + ``PROBE_SHELL_PATHS`` keeps the probe representative of what a real + isolated command can execute. Failure to find any of them is reported + clearly instead of silently substituting an unvalidated fallback. + """ + for candidate in PROBE_SHELL_PATHS: + path = Path(candidate) + if path.is_file() and os.access(path, os.X_OK): + return candidate + raise RuntimeError( + "bubblewrap capability probe needs a system shell at one of: " + f"{', '.join(PROBE_SHELL_PATHS)}" + ) + + def _probe_isolation_capability(backend: str) -> None: """Prove bubblewrap can create the sandbox namespaces before any service starts. @@ -138,7 +171,7 @@ def _probe_isolation_capability(backend: str) -> None: unavailable isolation (exit code 126) up front, instead of surfacing later as a confusing service-readiness or test failure. """ - probe_executable = shutil.which("sh") or "/bin/sh" + probe_executable = _probe_shell() bind_args: list[str] = [] for root in _bind_roots(): bind_args.extend(("--ro-bind", str(root), str(root))) @@ -206,6 +239,32 @@ def _sandbox_environment(env: dict[str, str], sandbox_root: Path) -> dict[str, s return mapped +def _resolve_isolated_executable(argv0: str, *, cwd: Path, path: str | None) -> Path | None: + """Resolve ``argv0`` the way it will actually run inside the sandbox. + + ``shutil.which`` resolves any command string that contains a path + separator (for example a repository launcher like ``./gradlew``) + against the *calling process's* current working directory -- it never + looks at an explicit ``cwd`` argument. That is correct for a bare + command name looked up on ``PATH``, but wrong for a repository-local + launcher: the wrapper process's own cwd is not the copied repository + that will be mounted into the sandbox, so a perfectly valid + ``./gradlew`` is resolved (or silently missed) against the wrong + directory. When ``argv0`` names an explicit path -- it contains a + directory component, whether relative or absolute -- it is resolved + against ``cwd`` instead, matching where the command will actually be + launched from once isolated. A bare name with no directory component + keeps the original ``PATH``-search behavior. + """ + if os.path.dirname(argv0): + candidate = Path(os.path.normpath(cwd / argv0)) + if candidate.is_file() and os.access(candidate, os.X_OK): + return candidate + return None + found = shutil.which(argv0, path=path) + return Path(found) if found is not None else None + + def isolated_command( command: str, *, @@ -216,23 +275,26 @@ def isolated_command( ) -> str: """Wrap one command in a read-only-root bubblewrap workspace. - The command's executable must resolve on ``PATH`` (or as a literal path) - and land inside the read-only bind roots. An executable ``shutil.which`` - cannot find is rejected rather than passed through unvalidated, so a - lookup failure can never silently bypass the read-only-root check it was - supposed to receive. + The command's executable must resolve on ``PATH``, as a repository-local + path resolved against ``cwd``, or as a literal host path, and land + inside the sandboxed workspace or the read-only bind roots. An + executable that cannot be resolved is rejected rather than passed + through unvalidated, so a lookup failure can never silently bypass the + workspace/read-only-root check it was supposed to receive. """ argv = shlex.split(command) if not argv: raise ValueError("command must not be empty") bind_roots = _bind_roots() - executable = shutil.which(argv[0], path=env.get("PATH")) - if executable is None: + executable_path = _resolve_isolated_executable(argv[0], cwd=cwd, path=env.get("PATH")) + if executable_path is None: raise RuntimeError(f"executable could not be resolved for isolation validation: {argv[0]}") - executable_path = Path(executable) if executable_path.is_relative_to(Path.home()): raise RuntimeError("commands from the host home directory are not allowed in isolation") - if not any(executable_path.is_relative_to(root) for root in bind_roots): + if not ( + executable_path.is_relative_to(sandbox_root) + or any(executable_path.is_relative_to(root) for root in bind_roots) + ): raise RuntimeError( f"executable is outside the isolated bind roots: {executable_path}" ) @@ -495,7 +557,7 @@ def main(argv: Sequence[str] | None = None) -> int: if backend else args.e2e_cmd ) - except RuntimeError as exc: + except (RuntimeError, ValueError) as exc: print(f"sandboxed-web-e2e: isolation rejected command: {exc}", file=sys.stderr) exit_code = 126 return exit_code diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index a2e6cda8d9..afa282f88d 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -104,6 +104,36 @@ def test_copy_workspace_excludes_credential_bearing_paths(tmp_path): assert not (copied / excluded).exists(), excluded +def test_copy_workspace_preserves_env_templates_but_excludes_env_secrets(tmp_path): + """Committed dotenv templates survive the copy while real dotenv secrets are excluded. + + ``.env.*`` in ``DEFAULT_IGNORE`` exists to exclude credential-bearing + dotenv variants such as ``.env.local`` or ``.env.production``, but the + same glob also matches committed, secret-free templates like + ``.env.example`` that verification commands may rely on for local + defaults. Those specific template names must remain in the copy even + though they match the exclusion glob. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "script.py").write_text("print('ok')\n", encoding="utf-8") + (repo / ".env.example").write_text("SECRET=set-me\n", encoding="utf-8") + (repo / ".env.sample").write_text("SECRET=set-me\n", encoding="utf-8") + (repo / ".env.template").write_text("SECRET=set-me\n", encoding="utf-8") + (repo / ".env").write_text("SECRET=leaked\n", encoding="utf-8") + (repo / ".env.local").write_text("SECRET=leaked\n", encoding="utf-8") + (repo / ".env.production").write_text("SECRET=leaked\n", encoding="utf-8") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "script.py").read_text(encoding="utf-8") == "print('ok')\n" + for preserved in (".env.example", ".env.sample", ".env.template"): + assert (copied / preserved).exists(), preserved + assert (copied / preserved).read_text(encoding="utf-8") == "SECRET=set-me\n" + for excluded in (".env", ".env.local", ".env.production"): + assert not (copied / excluded).exists(), excluded + + def test_copy_workspace_rejects_missing_repo_root(tmp_path): """Workspace copy fails clearly when the source root is invalid.""" with pytest.raises(ValueError, match="repo root is not a directory"): diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index eab75b1d5c..64f9665715 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -2,6 +2,7 @@ import os import re import runpy +import shutil import socket import subprocess import sys @@ -620,6 +621,46 @@ def test_main_reports_rejected_isolated_command(monkeypatch, tmp_path, capsys): assert payload["isolation_backend"] == "/usr/bin/bwrap" +def test_main_reports_coded_failure_for_whitespace_only_command(monkeypatch, tmp_path, capsys): + """A whitespace-only command fails closed with coded 126, not an uncaught traceback. + + ``isolated_command`` raises ``ValueError`` (not ``RuntimeError``) for a + command that is empty once split, so the previous except clause around + these calls let it propagate out of ``main`` uncaught -- printing a + Python traceback and skipping the documented isolation-rejection exit + code entirely. + """ + repo = tmp_path / "repo" + repo.mkdir() + started = [] + + monkeypatch.setattr(sandboxed_web_e2e, "isolation_backend", lambda mode: "/usr/bin/bwrap") + monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--backend-cmd", + " ", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 126 + assert not started + assert "isolation rejected command" in captured.err + assert "Traceback" not in captured.err + assert "Traceback" not in captured.out + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 126 + + def test_main_reports_unavailable_required_isolation(monkeypatch, tmp_path, capsys): """Required isolation errors exit before starting services with code 126.""" repo = tmp_path / "repo" @@ -1104,6 +1145,48 @@ def _fake_run(command, **kwargs): assert "--chdir" in command +def test_probe_isolation_capability_ignores_path_shadowed_shell(monkeypatch, tmp_path): + """A PATH entry that shadows sh with an inaccessible binary must not be used. + + Resolving the probe shell from the caller's ``PATH`` (as the old + implementation did via ``shutil.which("sh")``) can return a binary + outside every root bubblewrap actually bind-mounts, for example a + home-directory ``sh`` earlier on ``PATH`` than the real system shell. + That shadowed shell is invisible inside the sandbox, so a real, working + bubblewrap install would fail the probe. The probe must keep choosing a + shell from the fixed, known-mounted ``PROBE_SHELL_PATHS`` regardless of + what ``PATH`` (or ``shutil.which``) would otherwise resolve. + """ + shadow_dir = tmp_path / "home-bin" + shadow_dir.mkdir() + shadow_sh = shadow_dir / "sh" + shadow_sh.write_text("#!/bin/sh\nexit 1\n", encoding="utf-8") + shadow_sh.chmod(0o755) + monkeypatch.setenv("PATH", f"{shadow_dir}{os.pathsep}{os.environ.get('PATH', '')}") + assert shutil.which("sh") == str(shadow_sh) + + captured: dict[str, object] = {} + + def _fake_run(command, **kwargs): + captured["command"] = command + return subprocess.CompletedProcess(command, 0, stdout="", stderr="") + + monkeypatch.setattr(sandboxed_web_e2e.subprocess, "run", _fake_run) + sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") + + command = captured["command"] + probe_executable = command[-3] + assert probe_executable in sandboxed_web_e2e.PROBE_SHELL_PATHS + assert probe_executable != str(shadow_sh) + + +def test_probe_shell_fails_clearly_when_no_mounted_shell_exists(monkeypatch, tmp_path): + """No usable mounted shell is a clear, documented failure, not a silent fallback.""" + monkeypatch.setattr(sandboxed_web_e2e, "PROBE_SHELL_PATHS", (str(tmp_path / "no-such-sh"),)) + with pytest.raises(RuntimeError, match="needs a system shell"): + sandboxed_web_e2e._probe_shell() + + def test_probe_isolation_capability_rejects_on_timeout(monkeypatch): """A probe that hangs past its bounded timeout is classified as unavailable.""" monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: None) @@ -1216,6 +1299,94 @@ def test_isolated_command_rejects_executable_outside_bound_roots(monkeypatch, tm ) +def test_isolated_command_resolves_repo_local_launcher_against_sandboxed_cwd(monkeypatch, tmp_path): + """A ./gradlew-style repository launcher resolves against the sandboxed cwd. + + ``shutil.which`` resolves any command string containing a path separator + against the *calling process's* own current working directory, never an + explicit ``cwd`` argument -- so it can never find a launcher relative to + the copied repository. Forcing it to return ``None`` here proves + resolution instead goes through the sandboxed-``cwd``-relative path this + fix adds, not a PATH search. + """ + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + launcher = repo / "gradlew" + launcher.write_text("#!/bin/sh\necho gradlew\n", encoding="utf-8") + launcher.chmod(0o755) + + command = sandboxed_web_e2e.isolated_command( + "./gradlew build", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + assert command.startswith("/usr/bin/bwrap") + assert "--chdir /workspace/repo" in command + assert command.endswith("./gradlew build") + + +def test_isolated_command_rejects_repo_local_path_traversal(monkeypatch, tmp_path): + """A repo-local launcher path that lexically escapes the sandbox root is still rejected.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + outside = tmp_path / "outside-tool" + outside.write_text("#!/bin/sh\necho pwned\n", encoding="utf-8") + outside.chmod(0o755) + + with pytest.raises(RuntimeError, match=re.escape("outside the isolated bind roots")): + sandboxed_web_e2e.isolated_command( + "../../outside-tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + +def test_isolated_command_rejects_explicit_external_path(monkeypatch, tmp_path): + """An explicit absolute path outside the workspace and bind roots is still rejected.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + outside = tmp_path / "outside-tool" + outside.write_text("#!/bin/sh\necho pwned\n", encoding="utf-8") + outside.chmod(0o755) + + with pytest.raises(RuntimeError, match=re.escape("outside the isolated bind roots")): + sandboxed_web_e2e.isolated_command( + str(outside), + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + +def test_isolated_command_rejects_missing_repo_local_launcher(monkeypatch, tmp_path): + """A repo-local launcher path that does not exist in the copy is rejected as unresolved.""" + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + + with pytest.raises(RuntimeError, match="could not be resolved"): + sandboxed_web_e2e.isolated_command( + "./missing-tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + def test_isolated_command_rejects_unresolved_executable(monkeypatch, tmp_path): """A command whose executable cannot be resolved on PATH must fail closed. From bd0697aab5e169595bba86aa7a5322711e4a3d1d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 15:18:27 +0000 Subject: [PATCH 18/21] fix(sandboxed-verify): track cycles with a recursive active-set, not seen-forever Devin flagged "Valid symlink paths are rejected": a symlink referenced twice in one chain -- once fully resolved before the second reference is ever reached, not a real loop -- was incorrectly treated as a cycle, because the walk's `seen` set recorded every symlink ever dereferenced for the whole top-level candidate and never removed one once its resolution completed. Reproduced directly: `shared -> real_dir`, `link -> "shared/../shared/file.txt"` (a path the OS resolves without issue, referencing `shared` twice non-recursively) raised "workspace symlink could not be resolved" before this fix. Fix: restructured the walk from an iterative work-queue with a permanent `seen` set into a genuinely recursive component resolver with an `active` set -- a symlink is added to `active` only while its own target is being resolved (a fresh recursive call) and removed again as soon as that call returns successfully. A cycle is then precisely "a symlink that, directly or through others, points back to itself while still being resolved", which is what `step in active` now tests, rather than "was ever dereferenced anywhere in this chain". The true self-loop test (`a -> b -> a`) still raises, since `a` is still on the active call stack when it is encountered again. Verified: all existing symlink tests pass unchanged (escape, absolute, internal, dangling, excluded-by-DEFAULT_IGNORE, cycle, hop-limit boundary), plus a new regression test for the shared-non-cyclic-reference case. Full suite: 1978 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings. --- scripts/ci/sandboxed_verify.py | 94 +++++++++++++++++++--------------- tests/test_sandboxed_verify.py | 33 ++++++++++-- 2 files changed, 82 insertions(+), 45 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index e8962dec51..2716b7204f 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -191,48 +191,56 @@ def _reject_escaping_symlinks(destination: Path) -> None: root = destination.resolve(strict=True) for path in destination.rglob("*"): if path.is_symlink(): - _reject_escaping_symlink_chain(path, root) - - -def _reject_escaping_symlink_chain(candidate: Path, root: Path) -> None: - """Walk one symlink's target chain lexically, raising on escape or cycle. - - Uses ``os.readlink`` plus ``os.path.normpath`` at every hop instead of - ``Path.resolve()``, which requires the fully-resolved path to exist - (``strict=True``) or is unreliable for detecting a cycle across Python - versions (``strict=False``, the default) -- either way conflating a - symlink escape with a symlink that merely points at a target this - function never had to check for existence. A dangling target -- for - example one whose file was excluded from the copy by ``DEFAULT_IGNORE`` - -- is therefore accepted as long as it still lexically resolves inside - ``root``: verification must still run despite the broken link. Only an - absolute target, a target segment that normalizes outside ``root``, or a - chain that revisits a symlink it has already followed (an unresolvable - cycle) raises. The walk processes the candidate's path one component at a - time rather than resolving each symlink's whole target in a single - ``os.path.normpath`` call, because a target can itself contain an - intermediate component that is a symlink -- for example + _resolve_symlink_components( + path.relative_to(root).parts, root, root, set(), [MAXIMUM_SYMLINK_HOPS], path + ) + + +def _resolve_symlink_components( + parts: Sequence[str], + resolved: Path, + root: Path, + active: set[Path], + hops_remaining: list[int], + candidate: Path, +) -> Path: + """Resolve ``parts`` one component at a time, raising on escape or cycle. + + Uses ``os.readlink`` at every hop instead of ``Path.resolve()``, which + requires the fully-resolved path to exist (``strict=True``) or is + unreliable for detecting a cycle across Python versions (``strict=False``, + the default) -- either way conflating a symlink escape with a symlink + that merely points at a target this function never had to check for + existence. A dangling target -- for example one whose file was excluded + from the copy by ``DEFAULT_IGNORE`` -- is therefore accepted as long as + it still resolves inside ``root``: verification must still run despite + the broken link. Only an absolute target, a component that steps outside + ``root``, or a chain that revisits a symlink it is *currently in the + middle of following* (an unresolvable cycle) raises. + + Each path component is checked individually, and a component found to be + a symlink is resolved via a recursive call, rather than resolving a whole + target string in one ``os.path.normpath`` call -- a target can itself + contain an intermediate component that is a symlink, for example ``some-alias/../secret`` where ``some-alias`` is itself a relative, entirely-legitimate-looking internal symlink. Collapsing that whole string lexically in one step would cancel ``some-alias`` against the following ``..`` textually, silently ignoring that following ``some-alias`` for real can land somewhere shallower or deeper than one - directory level -- exactly the gap a real ``os.path.normpath`` call - cannot see, since it never re-examines whether an intermediate segment is - itself a symlink needing its own resolution first. Processing one - component at a time and re-checking ``is_symlink()`` after every step - closes that gap without ever calling ``Path.resolve()``. A hop budget - bounds the total number of symlinks followed so a chain that never - repeats still fails closed instead of walking forever; only actually - dereferencing a symlink spends one unit of that budget, so a chain of - exactly ``MAXIMUM_SYMLINK_HOPS`` real, resolvable symlinks is accepted. + directory level. Recursion is what makes the cycle check precise: a + symlink is added to ``active`` only while its own target is being + resolved and removed again as soon as that resolution returns + successfully, so the *same* symlink referenced twice in one chain -- + once fully resolved before the second reference is ever reached, not a + real loop -- is accepted, while a symlink that (directly or through + others) points back to itself while still being resolved is rejected. A + hop budget, shared across the whole recursive walk, bounds the total + number of symlinks followed so a chain that never repeats still fails + closed instead of walking forever; only actually dereferencing a symlink + spends one unit of that budget, so a chain of exactly + ``MAXIMUM_SYMLINK_HOPS`` real, resolvable symlinks is accepted. """ - seen: set[Path] = set() - resolved = root - stack = list(candidate.relative_to(root).parts) - hops_remaining = MAXIMUM_SYMLINK_HOPS - while stack: - component = stack.pop(0) + for component in parts: if component == "..": if resolved == root: raise ValueError(f"workspace symlink escapes the sandbox root: {candidate}") @@ -242,18 +250,22 @@ def _reject_escaping_symlink_chain(candidate: Path, root: Path) -> None: if not step.is_symlink(): resolved = step continue - if step in seen: + if step in active: raise ValueError(f"workspace symlink could not be resolved: {candidate}") - if hops_remaining <= 0: + if hops_remaining[0] <= 0: raise ValueError(f"workspace symlink could not be resolved: {candidate}") - seen.add(step) - hops_remaining -= 1 + active.add(step) + hops_remaining[0] -= 1 target = Path(os.readlink(step)) if target.is_absolute(): raise ValueError( f"workspace symlink escapes the sandbox root: {step} -> {target}" ) - stack = list(target.parts) + stack + resolved = _resolve_symlink_components( + target.parts, resolved, root, active, hops_remaining, candidate + ) + active.discard(step) + return resolved def _ignore_with_env_template_allowlist(patterns: Sequence[str]) -> Callable[[str, list[str]], set[str]]: diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index b8738c13d7..334897ed66 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -220,10 +220,11 @@ def test_copy_workspace_rejects_escape_via_intermediate_directory_alias(tmp_path def test_copy_workspace_rejects_unresolvable_symlink_cycle(tmp_path): """A symlink cycle that never terminates fails closed instead of hanging. - The chain walk tracks every path it has already followed; revisiting one - without ever leaving the sandbox root means the chain cannot be resolved - to a real, bounded target, so it becomes the same ``ValueError`` every - other unresolvable case in this function raises. + The walk tracks every symlink it is currently in the middle of + following; revisiting one of those without ever leaving the sandbox root + means the chain cannot be resolved to a real, bounded target, so it + becomes the same ``ValueError`` every other unresolvable case in this + function raises. """ repo = tmp_path / "repo" repo.mkdir() @@ -234,6 +235,30 @@ def test_copy_workspace_rejects_unresolvable_symlink_cycle(tmp_path): sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) +def test_copy_workspace_accepts_the_same_symlink_referenced_twice_non_recursively(tmp_path): + """A symlink resolved twice in one chain, not as part of a loop, is accepted. + + ``link -> shared/../shared/file.txt`` references ``shared`` twice, but + the first reference is fully resolved (and its bookkeeping cleared) + before the second one is ever reached -- this is not a cycle, just an + ordinary path that happens to name the same symlink in two places, and + the OS itself resolves it without issue. A cycle check that treats + "already resolved once, earlier" the same as "currently being resolved" + would reject this valid path. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "real_dir").mkdir() + (repo / "real_dir" / "file.txt").write_text("payload", encoding="utf-8") + (repo / "shared").symlink_to("real_dir", target_is_directory=True) + (repo / "link").symlink_to("shared/../shared/file.txt") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", []) + + assert (copied / "link").is_symlink() + assert (copied / "link").read_text(encoding="utf-8") == "payload" + + def test_copy_workspace_keeps_internal_symlinks_intact(tmp_path): """A symlink whose target stays inside the copied tree is preserved and still resolves.""" repo = tmp_path / "repo" From 528a1eae96b26cf60a76a912d2e733b1954794ca Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 15:26:49 +0000 Subject: [PATCH 19/21] fix(sandboxed): report a clean coded failure when copy_workspace rejects Devin flagged (thread PRRT_kwDOS_C14s6dh6My) that sandboxed_verify.py's main() calls copy_workspace() with no except around it: a symlink-escape rejection propagated as an uncaught ValueError, printing a raw Python traceback and exiting with Python's default uncaught-exception status instead of this module's own clean "sandboxed-verify: ..." message and coded exit (e.g. 124 for the timeout path). --keep-sandbox still retains the rejected copy either way, which matches its documented "for debugging" purpose -- not a bug to fix here. sandboxed_web_e2e.py calls the same copy_workspace() with the identical gap, found independently while fixing the sibling script; fixed the same way (exit 125, matching the code already used for its other ValueError rejections like an invalid readiness URL). Added a regression test per script asserting a clean exit 125, no traceback, and a still-emitted result payload. --- scripts/ci/sandboxed_verify.py | 7 ++++- scripts/ci/sandboxed_web_e2e.py | 7 ++++- tests/test_sandboxed_verify.py | 31 +++++++++++++++++++++++ tests/test_sandboxed_web_e2e.py | 45 +++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index 2716b7204f..2b1d80c39f 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -361,7 +361,12 @@ def main(argv: Sequence[str] | None = None) -> int: exit_code = 1 copied_repo = sandbox / "repo" try: - copied_repo = copy_workspace(Path(args.repo_root), sandbox, args.ignore) + try: + copied_repo = copy_workspace(Path(args.repo_root), sandbox, args.ignore) + except ValueError as exc: + print(f"sandboxed-verify: workspace copy rejected: {exc}", file=sys.stderr) + exit_code = 125 + return exit_code env = scrubbed_env(sandbox, args.allow_env) print(f"sandboxed-verify: cwd={copied_repo}") print(f"sandboxed-verify: command={' '.join(args.command)}") diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index c8d1e41462..e00553a503 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -507,7 +507,12 @@ def main(argv: Sequence[str] | None = None) -> int: exit_code = 1 start = time.monotonic() try: - copied_repo = sandboxed_verify.copy_workspace(Path(args.repo_root), sandbox, args.ignore) + try: + copied_repo = sandboxed_verify.copy_workspace(Path(args.repo_root), sandbox, args.ignore) + except ValueError as exc: + print(f"sandboxed-web-e2e: workspace copy rejected: {exc}", file=sys.stderr) + exit_code = 125 + return exit_code env = sandboxed_verify.scrubbed_env(sandbox, args.allow_env) try: backend = isolation_backend(args.isolation) diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index 334897ed66..13b5fa42e5 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -468,6 +468,37 @@ def test_main_reports_allowed_env_network_stderr_timeout_and_kept_sandbox(monkey shutil.rmtree(payload["sandbox"], ignore_errors=True) +def test_main_reports_a_clean_failure_when_the_workspace_copy_is_rejected(tmp_path, capsys): + """A symlink-escape rejection from ``copy_workspace`` must not surface as an + uncaught traceback. + + ``main()`` previously called ``copy_workspace`` with no ``except`` around + it, so a rejected copy (see the ``test_copy_workspace_rejects_*`` tests + above) propagated as an uncaught ``ValueError`` -- a raw Python traceback + on stderr and Python's default uncaught-exception exit status, instead of + the clean ``sandboxed-verify: ...`` message and coded exit this module + uses for every other config-time rejection (e.g. the timeout path's 124). + """ + outside = tmp_path / "outside-secret.txt" + outside.write_text("host-only-content", encoding="utf-8") + repo = tmp_path / "repo" + repo.mkdir() + (repo / "escape-link").symlink_to(outside) + + exit_code = sandboxed_verify.main( + ["--repo-root", str(repo), "--", "true"] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert "Traceback" not in captured.err + assert "workspace copy rejected" in captured.err + assert "workspace symlink escapes the sandbox root" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_verify.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_verify.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + def test_parse_args_rejects_invalid_inputs(): """The CLI rejects invocations without a command or with invalid options.""" with pytest.raises(SystemExit): diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 64f9665715..80d879cd4d 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -347,6 +347,51 @@ def test_require_loopback_readiness_url_rejects_malformed_ports(): sandboxed_web_e2e.require_loopback_readiness_url(f"http://{host}:-1/ready") +def test_main_reports_a_clean_failure_when_the_workspace_copy_is_rejected(monkeypatch, tmp_path, capsys): + """A symlink-escape rejection from the shared ``copy_workspace`` helper + must not surface as an uncaught traceback here either. + + This script calls ``sandboxed_verify.copy_workspace`` directly with no + ``except`` around it -- the same gap ``sandboxed_verify.py``'s own + ``main()`` had (a rejected copy propagated as a raw Python traceback and + Python's default uncaught-exception status instead of this module's own + clean ``sandboxed-web-e2e: ...`` message and coded exit, e.g. the 125 + already used for an invalid readiness URL below). + """ + outside = tmp_path / "outside-secret.txt" + outside.write_text("host-only-content", encoding="utf-8") + repo = tmp_path / "repo" + repo.mkdir() + (repo / "escape-link").symlink_to(outside) + started = [] + monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert not started + assert "Traceback" not in captured.err + assert "workspace copy rejected" in captured.err + assert "workspace symlink escapes the sandbox root" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + def test_main_reports_malformed_backend_port_before_starting_services(monkeypatch, tmp_path, capsys): """A malformed backend readiness port fails closed with exit 125, not a crash.""" repo = tmp_path / "repo" From 5b96f8494a0ad1abd41581a8ef86f50ee1a57a0a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 15:31:46 +0000 Subject: [PATCH 20/21] fix(sandboxed-ci): honor explicit --ignore over env-template allowlist; resolve bare commands via relative PATH entries Devin review findings on PR #1347: - sandboxed_verify.py: _ignore_with_env_template_allowlist merged DEFAULT_IGNORE and the caller's extra_ignores into one combined pattern set before restoring DEFAULT_ENV_TEMPLATE_ALLOWLIST names, so an explicit --ignore .env.example (or any caller-supplied extra_ignores entry) was silently overridden and the file still landed in the writable sandbox. Now builds two separate ignore functions -- one from DEFAULT_IGNORE alone (whose broad .env.* glob the allowlist exists to except from) and one from extra_ignores alone (never overridden) -- and only restores a name matched solely by the former. - sandboxed_web_e2e.py: _resolve_isolated_executable resolved a bare PATH-searched command purely via shutil.which(), which always resolves a relative PATH entry against the wrapper process's own cwd with no way to override that. A PATH with a relative entry meant to be read relative to the copied repository (e.g. PATH=bin:/usr/bin) therefore failed isolation (exit 126) even when the tool legitimately existed under the sandboxed cwd. Falls back to a new _which_relative_to_cwd that mirrors shutil.which's PATH-splitting/executable-bit checks by hand, anchoring relative entries at cwd; a relative entry that would resolve outside sandbox_root is skipped without touching the real filesystem, keeping the existing fail-closed behavior for a traversal PATH like ../../... Regression tests added for both, verified fail-before/pass-after against a temporary revert of each fix. Full suite: 1982 passed, 1 skipped, 21 subtests passed. scripts/ci/sandboxed_verify.py and scripts/ci/sandboxed_web_e2e.py: 100% statement+branch coverage, 100% docstring coverage (interrogate). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/sandboxed_verify.py | 37 +++++++++++++----- scripts/ci/sandboxed_web_e2e.py | 59 ++++++++++++++++++++++++++-- tests/test_sandboxed_verify.py | 28 +++++++++++++ tests/test_sandboxed_web_e2e.py | 69 +++++++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+), 14 deletions(-) diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index 2716b7204f..eba2cd30c9 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -268,24 +268,41 @@ def _resolve_symlink_components( return resolved -def _ignore_with_env_template_allowlist(patterns: Sequence[str]) -> Callable[[str, list[str]], set[str]]: +def _ignore_with_env_template_allowlist( + default_patterns: Sequence[str], extra_patterns: Sequence[str] +) -> Callable[[str, list[str]], set[str]]: """Build a ``copytree`` ignore function that spares committed env templates. ``shutil.ignore_patterns`` has no way to match a glob like ``.env.*`` while excepting specific names from it, so a committed, secret-free template such as ``.env.example`` matches the same pattern used to exclude real credential-bearing dotenv files and would otherwise vanish - from the sandboxed copy right along with them. This wraps the - pattern-based ignore function and un-ignores any name found in - ``DEFAULT_ENV_TEMPLATE_ALLOWLIST`` after the underlying patterns have - been applied. + from the sandboxed copy right along with them. This builds two + *separate* pattern-based ignore functions -- one from ``default_patterns`` + (``DEFAULT_IGNORE``, whose broad ``.env.*`` glob the allowlist exists to + carve an exception out of) and one from ``extra_patterns`` (a caller's + explicit ``--ignore``/``extra_ignores``) -- and un-ignores a name found in + ``DEFAULT_ENV_TEMPLATE_ALLOWLIST`` only when it was matched *solely* by + the default patterns. A name the caller explicitly asked to exclude via + ``extra_patterns`` -- for example because in their repository a file + named ``.env.example`` happens to carry something sensitive despite the + generic name -- stays excluded even though it is also one of the generic + template names: the allowlist must never override an explicit caller + exclusion, only the built-in broad glob. """ - base_ignore = shutil.ignore_patterns(*patterns) + default_ignore = shutil.ignore_patterns(*default_patterns) + extra_ignore = shutil.ignore_patterns(*extra_patterns) def _ignore(directory: str, names: list[str]) -> set[str]: - """Apply the pattern-based ignore rules, then spare env template names.""" - ignored = base_ignore(directory, names) - return {name for name in ignored if name not in DEFAULT_ENV_TEMPLATE_ALLOWLIST} + """Apply both pattern sets, sparing env template names not explicitly excluded.""" + default_ignored = default_ignore(directory, names) + extra_ignored = extra_ignore(directory, names) + protected = { + name + for name in default_ignored + if name in DEFAULT_ENV_TEMPLATE_ALLOWLIST and name not in extra_ignored + } + return (default_ignored | extra_ignored) - protected return _ignore @@ -296,7 +313,7 @@ def copy_workspace(repo_root: Path, sandbox_root: Path, extra_ignores: Sequence[ if not source.is_dir(): raise ValueError(f"repo root is not a directory: {source}") destination = sandbox_root / "repo" - ignore = _ignore_with_env_template_allowlist(DEFAULT_IGNORE + tuple(extra_ignores)) + ignore = _ignore_with_env_template_allowlist(DEFAULT_IGNORE, tuple(extra_ignores)) shutil.copytree(source, destination, ignore=ignore, symlinks=True) _reject_escaping_symlinks(destination) return destination diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index c8d1e41462..d1f717116f 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -239,7 +239,48 @@ def _sandbox_environment(env: dict[str, str], sandbox_root: Path) -> dict[str, s return mapped -def _resolve_isolated_executable(argv0: str, *, cwd: Path, path: str | None) -> Path | None: +def _which_relative_to_cwd(argv0: str, *, cwd: Path, sandbox_root: Path, path: str | None) -> Path | None: + """Search ``PATH`` for ``argv0`` like ``shutil.which``, anchoring relative entries at ``cwd``. + + ``shutil.which`` joins every ``PATH`` entry -- relative or absolute -- + with the plain command name and, for a relative entry, only ever checks + the result against the *calling process's* own current working + directory; there is no parameter to override that. Some build tooling + sets up a ``PATH`` with a relative entry (for example + ``PATH=bin:/usr/bin``) meant to be read relative to the project being + built, which can never be found this way for a command about to run + from a different directory (``cwd``, the sandboxed copy of the + repository) than the wrapper process's own cwd. This mirrors + ``shutil.which``'s ``PATH``-splitting and executable-bit checks by hand, + joining a relative entry with ``cwd`` instead of leaving it to resolve + against ``os.getcwd()``; an absolute entry is used exactly as + ``shutil.which`` would use it. A relative entry that, once joined with + ``cwd``, would resolve outside ``sandbox_root`` is skipped without ever + being checked against the real filesystem, so a ``PATH`` entry such as + ``../../..`` cannot be used to probe for -- or resolve to -- executables + on the host outside the sandboxed copy; it fails closed the same way an + unresolvable command already does. ``PATH`` falls back to + ``os.environ["PATH"]`` and then ``os.defpath``, matching + ``shutil.which``'s own fallback for a caller that passes no ``PATH``. + """ + search_path = path if path is not None else os.environ.get("PATH", os.defpath) + if not search_path: + return None + for entry in search_path.split(os.pathsep): + directory = Path(entry) if entry else cwd + if not directory.is_absolute(): + directory = Path(os.path.normpath(cwd / directory)) + if not directory.is_relative_to(sandbox_root): + continue + candidate = directory / argv0 + if candidate.is_file() and os.access(candidate, os.X_OK): + return candidate + return None + + +def _resolve_isolated_executable( + argv0: str, *, cwd: Path, sandbox_root: Path, path: str | None +) -> Path | None: """Resolve ``argv0`` the way it will actually run inside the sandbox. ``shutil.which`` resolves any command string that contains a path @@ -254,7 +295,13 @@ def _resolve_isolated_executable(argv0: str, *, cwd: Path, path: str | None) -> directory component, whether relative or absolute -- it is resolved against ``cwd`` instead, matching where the command will actually be launched from once isolated. A bare name with no directory component - keeps the original ``PATH``-search behavior. + keeps the original ``PATH``-search behavior via ``shutil.which`` first; + ``shutil.which`` itself resolves a *relative* ``PATH`` entry only + against the wrapper's own process cwd, so when it comes back empty this + falls through to ``_which_relative_to_cwd``, which retries the search + with relative ``PATH`` entries anchored at ``cwd`` instead -- covering + build tooling that sets up a ``PATH`` meant to be read relative to the + repository under test. """ if os.path.dirname(argv0): candidate = Path(os.path.normpath(cwd / argv0)) @@ -262,7 +309,9 @@ def _resolve_isolated_executable(argv0: str, *, cwd: Path, path: str | None) -> return candidate return None found = shutil.which(argv0, path=path) - return Path(found) if found is not None else None + if found is not None: + return Path(found) + return _which_relative_to_cwd(argv0, cwd=cwd, sandbox_root=sandbox_root, path=path) def isolated_command( @@ -286,7 +335,9 @@ def isolated_command( if not argv: raise ValueError("command must not be empty") bind_roots = _bind_roots() - executable_path = _resolve_isolated_executable(argv[0], cwd=cwd, path=env.get("PATH")) + executable_path = _resolve_isolated_executable( + argv[0], cwd=cwd, sandbox_root=sandbox_root, path=env.get("PATH") + ) if executable_path is None: raise RuntimeError(f"executable could not be resolved for isolation validation: {argv[0]}") if executable_path.is_relative_to(Path.home()): diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index 334897ed66..8c146b40c1 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -134,6 +134,34 @@ def test_copy_workspace_preserves_env_templates_but_excludes_env_secrets(tmp_pat assert not (copied / excluded).exists(), excluded +def test_copy_workspace_extra_ignore_overrides_env_template_allowlist(tmp_path): + """An explicit caller exclusion for a template name is not restored by the allowlist. + + ``DEFAULT_ENV_TEMPLATE_ALLOWLIST`` exists to carve committed, secret-free + templates back out of the broad ``.env.*`` glob in ``DEFAULT_IGNORE``. It + must never also override a caller's own explicit ``extra_ignores`` (the + ``--ignore`` CLI flag) -- for example because in a particular repository + ``.env.example`` happens to carry something sensitive despite the + generic name. If the allowlist restored a name regardless of *why* it + was ignored, that explicit exclusion would be silently defeated and the + file would ride into the writable, command-readable sandbox anyway. + """ + repo = tmp_path / "repo" + repo.mkdir() + (repo / "script.py").write_text("print('ok')\n", encoding="utf-8") + (repo / ".env.example").write_text("SECRET=actually-sensitive\n", encoding="utf-8") + (repo / ".env.sample").write_text("SECRET=set-me\n", encoding="utf-8") + + copied = sandboxed_verify.copy_workspace(repo, tmp_path / "sandbox", [".env.example"]) + + assert (copied / "script.py").read_text(encoding="utf-8") == "print('ok')\n" + assert not (copied / ".env.example").exists() + # A template the caller did NOT explicitly exclude is still preserved -- + # the allowlist keeps working for everything except the explicit ask. + assert (copied / ".env.sample").exists() + assert (copied / ".env.sample").read_text(encoding="utf-8") == "SECRET=set-me\n" + + def test_copy_workspace_rejects_missing_repo_root(tmp_path): """Workspace copy fails clearly when the source root is invalid.""" with pytest.raises(ValueError, match="repo root is not a directory"): diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 64f9665715..a7cb77cae4 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -1330,6 +1330,75 @@ def test_isolated_command_resolves_repo_local_launcher_against_sandboxed_cwd(mon assert command.endswith("./gradlew build") +def test_isolated_command_resolves_bare_command_via_relative_path_entry(monkeypatch, tmp_path): + """A bare command resolves when PATH itself has a relative entry. + + ``shutil.which`` joins a relative ``PATH`` entry with the *calling + process's* own cwd, with no way to override that -- so build tooling + that sets up a ``PATH`` like ``bin:/usr/bin`` meant to be read relative + to the project being built can never be resolved this way for a command + about to run from a different directory (the sandboxed copy). Mocking + ``shutil.which`` to ``None`` here proves resolution instead falls + through to the cwd-anchored ``PATH`` search this fix adds. + """ + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + bin_dir = repo / "bin" + bin_dir.mkdir(parents=True) + tool = bin_dir / "tool" + tool.write_text("#!/bin/sh\necho tool\n", encoding="utf-8") + tool.chmod(0o755) + + command = sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "bin"}, + ) + + assert command.startswith("/usr/bin/bwrap") + assert "--chdir /workspace/repo" in command + assert command.endswith("tool") + + +def test_which_relative_to_cwd_returns_none_for_empty_path(tmp_path): + """An empty PATH string yields no matches without touching the filesystem.""" + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + + result = sandboxed_web_e2e._which_relative_to_cwd( + "tool", cwd=repo, sandbox_root=sandbox, path="" + ) + + assert result is None + + +def test_isolated_command_rejects_relative_path_entry_escaping_sandbox(monkeypatch, tmp_path): + """A relative PATH entry cannot be used to search the real host filesystem outside the copy. + + ``PATH=../../..`` resolved against ``cwd`` would otherwise land on a + real host directory outside the sandboxed copy. That must fail closed + the same way an unresolved command already does, without the lookup + itself probing the host filesystem outside the sandbox root. + """ + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + + with pytest.raises(RuntimeError, match="could not be resolved"): + sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "../../.."}, + ) + + def test_isolated_command_rejects_repo_local_path_traversal(monkeypatch, tmp_path): """A repo-local launcher path that lexically escapes the sandbox root is still rejected.""" monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) From 49d4d6b63c0cf78b2b4fd323025eabbc80d7e90e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 01:48:48 +0000 Subject: [PATCH 21/21] fix(sandboxed-web-e2e): close proxy bypass, port-occupancy, malformed-command, and workspace-path gaps Four Devin findings on this PR's bubblewrap sandboxing: - wait_for_url's opener now passes an explicit ProxyHandler({}) alongside NoRedirectHandler, so HTTP_PROXY/HTTPS_PROXY/*_proxy environment variables can never route a "loopback-only, isolated" readiness probe through an external proxy (mirrors the fix already applied to a different opener in materialize_base_python_requirements.py). - New require_unoccupied_readiness_port rejects a readiness URL whose port already answers before this run starts its own service, called once in main() right before start_service. isolated_command does not create a network namespace for the commands it wraps (the host readiness poller and the E2E command both need to reach the same loopback ports), so this closes the "polls some other, unrelated runner service" gap without breaking that shared-loopback design. - parse_args now shell-tokenizes all three of --backend-cmd/--frontend-cmd/ --e2e-cmd up front, independent of --isolation, and rejects a blank or unmatched-quote command through argparse's own clean SystemExit(2) path. Previously, with isolation disabled, such a command bypassed isolated_command entirely and crashed with an uncaught ValueError deep inside start_service/run_shell's own shlex.split call. - isolated_command now rewrites an absolute executable path that resolves inside the sandbox copy to its /workspace-relative form, and _sandbox_environment now does the same for PATH entries rooted under the sandbox copy -- bubblewrap binds the copy at /workspace, not at its original host path, so an absolute copied-repo launcher or PATH entry previously failed to launch inside the sandbox unchanged. Also folds in a CodeRabbit finding on the same head, in sandboxed_verify.py: _reject_escaping_symlinks walked the unresolved destination path but checked each symlink against the resolved root, so a sandbox root reached through a symlinked ancestor (e.g. a symlinked default temp directory) made path.relative_to(root) raise for every symlink in an otherwise-legitimate copy. Now walks from the already-resolved root instead; escape detection itself is unchanged and still covered. Plus two quick-win CodeRabbit items: documents the DEFAULT_ENV_TEMPLATE_ALLOWLIST carve-out in the command-isolation doc, and fixes four tests that mocked shutil.which for _probe_isolation_capability's shell selection after it was changed to check fixed PROBE_SHELL_PATHS directly instead -- those mocks were silent no-ops relying on whatever shell the host happened to have mounted. All new behavior is covered by new regression tests reproducing each bug against pre-fix code; scripts/ci stays at 100% line+branch coverage and 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- .../sandboxed-web-command-isolation.md | 7 +- scripts/ci/sandboxed_verify.py | 18 +- scripts/ci/sandboxed_web_e2e.py | 117 +++++- tests/test_sandboxed_verify.py | 48 +++ tests/test_sandboxed_web_e2e.py | 368 ++++++++++++++++-- 5 files changed, 521 insertions(+), 37 deletions(-) diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md index ecb90e7e2e..55105deb3b 100644 --- a/docs/doctoring/sandboxed-web-command-isolation.md +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -62,7 +62,12 @@ credential-bearing dotfiles/dirs a checkout can carry (`.env*`, `.netrc`, `.npmrc`, `.pypirc`, `.pgpass`, `.git-credentials`, `.ssh`, `.gnupg`, `.aws`, `.kube`, `.docker`) so a repository that happens to have one of these present at copy time never rides along into the sandboxed command's writable, -readable mount. Logs and the scrubbed per-command home directories are +readable mount. The broad `.env*` exclusion has one deliberate carve-out: +`DEFAULT_ENV_TEMPLATE_ALLOWLIST` (`.env.example`, `.env.sample`, +`.env.template`) still copies those committed, secret-free dotenv templates +through, since verification commands read them for local defaults; a caller +can still force one of those names back out with an explicit `--ignore`. +Logs and the scrubbed per-command home directories are intentionally part of that same writable mount — the tested command needs to write them — this exclusion list narrows what "writable and readable by the command under test" actually contains; it does not attempt to split the mount diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index 33f508f2cd..94797c2038 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -187,9 +187,25 @@ def _reject_escaping_symlinks(destination: Path) -> None: cannot be resolved, aborts the whole copy rather than being silently dropped or repaired, since a repository author who plants one such link cannot be assumed not to have planted others. + + Walking starts from ``root`` -- ``destination`` fully resolved -- rather + than ``destination`` itself, and every symlink found is then checked + with ``path.relative_to(root)``. When some *ancestor* of ``destination`` + is itself reached through a symlink (for example a temp directory whose + default OS location is a symlink, unrelated to anything the copied + repository controls), ``destination`` and ``root`` are different, only + lexically equal-looking strings for the same real location. Walking from + the unresolved ``destination`` would then yield paths still prefixed + with that unresolved string, which are never actually relative to + ``root`` -- so ``relative_to`` raises before this function's own escape + check ever runs, rejecting an entirely legitimate copy that contains no + escaping symlink at all. Walking from ``root`` instead guarantees every + yielded path already shares ``root``'s own resolved prefix, so + ``relative_to`` only ever fails for the cases this function exists to + reject. """ root = destination.resolve(strict=True) - for path in destination.rglob("*"): + for path in root.rglob("*"): if path.is_symlink(): _resolve_symlink_components( path.relative_to(root).parts, root, root, set(), [MAXIMUM_SYMLINK_HOPS], path diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index 9ccf90bfa4..c86a780441 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -109,9 +109,41 @@ def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: for name in args.allow_env: if not sandboxed_verify.ENV_NAME_RE.match(name): parser.error(f"--allow-env must be an environment variable name: {name}") + for flag, value in ( + ("--backend-cmd", args.backend_cmd), + ("--frontend-cmd", args.frontend_cmd), + ("--e2e-cmd", args.e2e_cmd), + ): + _require_parseable_command(parser, flag, value) return args +def _require_parseable_command(parser: argparse.ArgumentParser, flag: str, value: str) -> None: + """Reject a command that fails to shell-tokenize or tokenizes to nothing. + + ``isolated_command`` performs this exact ``shlex.split`` validation + itself, but only reaches it when isolation is enabled. With + ``--isolation disabled`` (the explicit, documented "trusted local + debugging" escape hatch), a command string bypasses ``isolated_command`` + entirely and is handed straight to ``start_service``/``run_shell``, which + call ``shlex.split`` directly with no ``except`` around either call. A + blank command, or one with an unmatched shell-quote character, then + raised an uncaught ``ValueError`` from deep inside ``main`` instead of + the clean, coded CLI failure every other bad input in this module + produces. Validating here, in ``parse_args``, runs for both isolation + modes -- disabled included -- so a malformed command is always rejected + the same way, through argparse's own clean-exit path, before ``main`` + ever tries to run it. + """ + try: + tokens = shlex.split(value) + except ValueError as exc: + parser.error(f"{flag} is not a valid shell command: {exc}") + else: + if not tokens: + parser.error(f"{flag} must not be blank") + + BIND_ROOTS = ("/usr", "/bin", "/sbin", "/lib", "/lib64", "/opt") @@ -236,9 +268,32 @@ def _sandbox_environment(env: dict[str, str], sandbox_root: Path) -> dict[str, s value = mapped.get(key) if value: mapped[key] = value.replace(source, SANDBOX_MOUNT, 1) + path_value = mapped.get("PATH") + if path_value: + mapped["PATH"] = os.pathsep.join( + _translate_sandbox_path_entry(entry, source) for entry in path_value.split(os.pathsep) + ) return mapped +def _translate_sandbox_path_entry(entry: str, source: str) -> str: + """Rewrite one ``PATH`` entry rooted under the host sandbox copy into its ``/workspace`` form. + + A command that relies on ``PATH`` lookup for a workspace-local binary + (rather than naming it by an explicit path) is launched with the *host* + copy's absolute path still present in its inherited ``PATH`` -- the same + host path ``isolated_command`` resolves it against for validation, but + one that does not exist inside the bubblewrap mount, where only + ``SANDBOX_MOUNT`` is bound. An entry that is not rooted under the sandbox + copy (for example a bind-mounted system directory like ``/usr/bin``) is + returned unchanged, matching how ``HOME``/``TMPDIR`` and friends above + are left alone when they do not reference the sandbox copy either. + """ + if entry == source or entry.startswith(source + os.sep): + return entry.replace(source, SANDBOX_MOUNT, 1) + return entry + + def _which_relative_to_cwd(argv0: str, *, cwd: Path, sandbox_root: Path, path: str | None) -> Path | None: """Search ``PATH`` for ``argv0`` like ``shutil.which``, anchoring relative entries at ``cwd``. @@ -329,7 +384,12 @@ def isolated_command( inside the sandboxed workspace or the read-only bind roots. An executable that cannot be resolved is rejected rather than passed through unvalidated, so a lookup failure can never silently bypass the - workspace/read-only-root check it was supposed to receive. + workspace/read-only-root check it was supposed to receive. An absolute + executable path that resolves inside the sandbox copy is rewritten to + its ``SANDBOX_MOUNT``-relative form: bubblewrap binds ``sandbox_root`` at + ``SANDBOX_MOUNT``, not at its original host path, so the literal host + absolute path this function validated against would not exist inside + the sandbox and the command would fail to launch there unchanged. """ argv = shlex.split(command) if not argv: @@ -349,6 +409,8 @@ def isolated_command( raise RuntimeError( f"executable is outside the isolated bind roots: {executable_path}" ) + if Path(argv[0]).is_absolute() and executable_path.is_relative_to(sandbox_root): + argv[0] = str(Path(SANDBOX_MOUNT) / executable_path.relative_to(sandbox_root)) args = [backend, "--die-with-parent", "--new-session", "--unshare-pid", "--tmpfs", "/"] for root in bind_roots: args.extend(("--ro-bind", str(root), str(root))) @@ -457,13 +519,60 @@ def require_loopback_readiness_url(url: str) -> None: _require_loopback_ip_text(hostname, hostname) +def require_unoccupied_readiness_port(url: str) -> None: + """Reject a readiness URL whose port already answers before this run starts a service. + + ``require_loopback_readiness_url`` only proves the URL targets loopback; + it says nothing about *which* process on loopback will eventually answer + it. ``isolated_command`` does not create a network namespace for the + commands it wraps -- the backend, frontend, and E2E command all still + need to reach the same host loopback interface the readiness poller + itself uses (the poller runs unsandboxed, in this process), so giving the + sandboxed commands a private network namespace is not an available + option here without breaking that readiness/E2E flow. On a shared + loopback interface, an operator- or config-supplied readiness URL that + happens to name a port some other, unrelated process on the CI runner + already occupies would otherwise be polled exactly like the real target: + a response from that unrelated process reads as this run's service being + ready, and any later request the E2E command makes to the same address + reaches it too, whether or not it was ever meant to be reachable this + way. Calling this once, immediately after ``require_loopback_readiness_url`` + and before ``start_service`` starts anything, ensures a port that + answers now can only be attributed to some other process -- this run's + own service cannot yet be listening -- so it is rejected here rather + than trusted. A connection refusal or timeout means nothing is listening + yet, which is the expected, accepted state before the service starts. + """ + parsed = urllib.parse.urlparse(url) + hostname = parsed.hostname or "127.0.0.1" + port = parsed.port or (443 if parsed.scheme.lower() == "https" else 80) + try: + with socket.create_connection((hostname, port), timeout=0.2): + pass + except OSError: + return + raise ValueError( + f"readiness port is already in use by another process before this run started its service: {url}" + ) + + def wait_for_url(url: str, timeout: int, service: Service) -> bool: - """Poll a readiness URL until it responds or the service exits.""" + """Poll a readiness URL until it responds or the service exits. + + The opener is built with an explicitly empty ``ProxyHandler({})`` so this + loopback-only poll can never be routed through an ``HTTP_PROXY`` / + ``HTTPS_PROXY`` / ``*_proxy`` environment variable. ``urllib.request`` + otherwise installs a default ``ProxyHandler`` (via ``getproxies()``) for + every opener that does not already carry one, so a caller's process + environment could silently forward this "loopback-only, isolated" + readiness probe to an external proxy server, defeating the point of + ``require_loopback_readiness_url``'s SSRF check below. + """ if not url: return True require_loopback_readiness_url(url) deadline = time.monotonic() + timeout - opener = urllib.request.build_opener(NoRedirectHandler()) + opener = urllib.request.build_opener(urllib.request.ProxyHandler({}), NoRedirectHandler()) while time.monotonic() < deadline: if service.process.poll() is not None: return False @@ -620,8 +729,10 @@ def main(argv: Sequence[str] | None = None) -> int: try: if args.backend_ready_url: require_loopback_readiness_url(args.backend_ready_url) + require_unoccupied_readiness_port(args.backend_ready_url) if args.frontend_ready_url: require_loopback_readiness_url(args.frontend_ready_url) + require_unoccupied_readiness_port(args.frontend_ready_url) except ValueError as exc: print(f"sandboxed-web-e2e: invalid readiness URL: {exc}", file=sys.stderr) exit_code = 125 diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index 53e5606fae..d89b9bb956 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -339,6 +339,54 @@ def test_copy_workspace_keeps_symlink_dangling_from_a_missing_internal_target(tm assert not (copied / "dangling.txt").exists() +def test_copy_workspace_accepts_internal_symlink_when_sandbox_root_is_reached_via_symlinked_ancestor(tmp_path): + """A benign internal symlink is accepted even when an *ancestor* of the sandbox + root is itself reached through a symlink (for example a symlinked default + temp directory, unrelated to anything the copied repository controls). + + Before this fix, ``_reject_escaping_symlinks`` walked ``destination.rglob("*")`` + -- the *unresolved* path -- but checked each symlink's position with + ``path.relative_to(root)``, where ``root`` is ``destination`` fully + *resolved*. When some ancestor directory leading to ``destination`` is a + symlink, those two strings diverge even though they name the same real + location, so ``relative_to`` raised ``ValueError`` for every symlink in an + entirely legitimate copy, aborting the whole run with no actual escape + present. + """ + real_root = tmp_path / "real_sandbox_root" + real_root.mkdir() + linked_root = tmp_path / "linked_sandbox_root" + linked_root.symlink_to(real_root, target_is_directory=True) + + repo = tmp_path / "repo" + repo.mkdir() + (repo / "real.txt").write_text("payload", encoding="utf-8") + (repo / "link.txt").symlink_to("real.txt") + + copied = sandboxed_verify.copy_workspace(repo, linked_root, []) + + assert (copied / "link.txt").is_symlink() + assert (copied / "link.txt").read_text(encoding="utf-8") == "payload" + + +def test_copy_workspace_still_rejects_escape_when_sandbox_root_is_reached_via_symlinked_ancestor(tmp_path): + """A genuinely escaping symlink is still rejected when the sandbox root is + itself reached through a symlinked ancestor -- walking from the resolved + root (this fix) must not weaken the escape check itself. + """ + real_root = tmp_path / "real_sandbox_root" + real_root.mkdir() + linked_root = tmp_path / "linked_sandbox_root" + linked_root.symlink_to(real_root, target_is_directory=True) + + repo = tmp_path / "repo" + repo.mkdir() + (repo / "evil.txt").symlink_to("/etc/passwd") + + with pytest.raises(ValueError, match="workspace symlink escapes the sandbox root"): + sandboxed_verify.copy_workspace(repo, linked_root, []) + + def test_copy_workspace_rejects_symlink_chain_past_the_hop_limit(tmp_path): """A long, never-repeating, never-escaping symlink chain still fails closed. diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 72344985b0..29c426e227 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -241,6 +241,49 @@ def open(self, url, timeout): assert sandboxed_web_e2e.tail_text(log_path).splitlines()[0] == "line-10" +def test_wait_for_url_ignores_proxy_environment_variables(monkeypatch, tmp_path): + """Readiness polling must not honor HTTP_PROXY/HTTPS_PROXY environment variables. + + ``require_loopback_readiness_url`` only proves the *target* is loopback; + without an explicit, empty ``ProxyHandler``, ``urllib.request.build_opener`` + still installs a default proxy handler that reads ``http_proxy``/ + ``https_proxy`` (etc.) from the process environment via ``getproxies()``, + so the actual HTTP request could still be routed through an external + proxy server even though the URL itself was validated as loopback-only -- + completely defeating the point of the loopback check. This test proves + the opener ignores the environment by pointing the proxy at a definitely + closed local port: if the proxy were honored, every poll attempt would + be refused by that dead port and ``wait_for_url`` would time out and + return ``False``; with the proxy ignored, the request goes directly to + the real local server and succeeds quickly. + """ + + class RunningProcess: + def poll(self): + return None + + with socket.socket() as probe: + probe.bind(("127.0.0.1", 0)) + dead_port = probe.getsockname()[1] + + port = free_port() + server_code = ( + "import http.server, socketserver; " + "socketserver.TCPServer.allow_reuse_address=True; " + f"server=socketserver.TCPServer(('127.0.0.1', {port}), http.server.SimpleHTTPRequestHandler); " + "server.serve_forever()" + ) + server = subprocess.Popen([sys.executable, "-c", server_code], text=True) + monkeypatch.setenv("http_proxy", f"http://127.0.0.1:{dead_port}") + monkeypatch.setenv("https_proxy", f"http://127.0.0.1:{dead_port}") + try: + service = sandboxed_web_e2e.Service("web", "serve", RunningProcess(), tmp_path / "missing.log") + assert sandboxed_web_e2e.wait_for_url(f"http://127.0.0.1:{port}/", 10, service) is True + finally: + server.terminate() + server.wait(timeout=5) + + def test_wait_for_url_rejects_non_loopback_and_confused_deputy_targets(tmp_path): """Readiness polling must fail closed on public, metadata, and userinfo targets.""" exited = subprocess.Popen([sys.executable, "-c", ""], text=True) @@ -347,6 +390,73 @@ def test_require_loopback_readiness_url_rejects_malformed_ports(): sandboxed_web_e2e.require_loopback_readiness_url(f"http://{host}:-1/ready") +def test_require_unoccupied_readiness_port_rejects_pre_existing_listener(): + """A port already answering before this run's service starts is rejected. + + ``isolated_command`` does not create a network namespace for the + commands it wraps -- the backend, frontend, and E2E command all still + need to reach the same host loopback interface the readiness poller + itself uses, so a private network namespace is not an option here. + Without this check, a readiness URL naming a port some other, unrelated + process on the CI runner already occupies would be polled exactly like + the real target, and any later request the E2E command makes would + reach it too. + """ + with socket.socket() as sock: + sock.bind(("127.0.0.1", 0)) + sock.listen(1) + port = sock.getsockname()[1] + with pytest.raises(ValueError, match="readiness port is already in use"): + sandboxed_web_e2e.require_unoccupied_readiness_port(f"http://127.0.0.1:{port}/health") + + +def test_require_unoccupied_readiness_port_allows_a_free_port(): + """A port nothing is listening on yet passes the pre-start occupancy check.""" + with socket.socket() as sock: + sock.bind(("127.0.0.1", 0)) + port = sock.getsockname()[1] + # The socket above is closed (and never listened), so the port is free again. + sandboxed_web_e2e.require_unoccupied_readiness_port(f"http://127.0.0.1:{port}/health") + + +def test_main_reports_occupied_readiness_port_before_starting_services(monkeypatch, tmp_path, capsys): + """A readiness port already occupied by another process fails closed with exit 125.""" + repo = tmp_path / "repo" + repo.mkdir() + started = [] + monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) + + with socket.socket() as sock: + sock.bind(("127.0.0.1", 0)) + sock.listen(1) + port = sock.getsockname()[1] + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--backend-ready-url", + f"http://127.0.0.1:{port}/health", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert not started + assert "invalid readiness URL: readiness port is already in use" in captured.err + result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] + payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) + assert payload["exit_code"] == 125 + + def test_main_reports_a_clean_failure_when_the_workspace_copy_is_rejected(monkeypatch, tmp_path, capsys): """A symlink-escape rejection from the shared ``copy_workspace`` helper must not surface as an uncaught traceback here either. @@ -667,13 +777,20 @@ def test_main_reports_rejected_isolated_command(monkeypatch, tmp_path, capsys): def test_main_reports_coded_failure_for_whitespace_only_command(monkeypatch, tmp_path, capsys): - """A whitespace-only command fails closed with coded 126, not an uncaught traceback. - - ``isolated_command`` raises ``ValueError`` (not ``RuntimeError``) for a - command that is empty once split, so the previous except clause around - these calls let it propagate out of ``main`` uncaught -- printing a - Python traceback and skipping the documented isolation-rejection exit - code entirely. + """A whitespace-only command fails closed via argparse, not an uncaught traceback. + + ``isolated_command`` raises ``ValueError`` for a command that is empty + once split, but that check is only ever reached when isolation is + enabled -- ``--isolation disabled`` bypasses ``isolated_command`` + entirely and used to let a blank command reach ``shlex.split`` deep + inside ``start_service``/``run_shell`` uncaught (see the disabled-mode + tests below). ``parse_args`` now validates all three commands up front, + independent of isolation mode, so this required-isolation case is now + rejected even earlier than before -- through argparse's own clean + ``SystemExit(2)`` usage-error path -- before ``isolation_backend`` or any + service ever starts. ``isolated_command``'s own defensive check for a + blank command is unchanged and still independently covered by + ``test_isolated_command_rejects_empty_command``. """ repo = tmp_path / "repo" repo.mkdir() @@ -682,28 +799,26 @@ def test_main_reports_coded_failure_for_whitespace_only_command(monkeypatch, tmp monkeypatch.setattr(sandboxed_web_e2e, "isolation_backend", lambda mode: "/usr/bin/bwrap") monkeypatch.setattr(sandboxed_web_e2e, "start_service", lambda *args: started.append(args)) - exit_code = sandboxed_web_e2e.main( - [ - "--repo-root", - str(repo), - "--backend-cmd", - " ", - "--frontend-cmd", - "frontend", - "--e2e-cmd", - "e2e", - ] - ) + with pytest.raises(SystemExit) as exc_info: + sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--backend-cmd", + " ", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "e2e", + ] + ) captured = capsys.readouterr() - assert exit_code == 126 + assert exc_info.value.code == 2 assert not started - assert "isolation rejected command" in captured.err + assert "--backend-cmd must not be blank" in captured.err assert "Traceback" not in captured.err assert "Traceback" not in captured.out - result_line = [line for line in captured.out.splitlines() if line.startswith(sandboxed_web_e2e.RESULT_MARKER)][-1] - payload = json.loads(result_line.removeprefix(sandboxed_web_e2e.RESULT_MARKER).strip()) - assert payload["exit_code"] == 126 def test_main_reports_unavailable_required_isolation(monkeypatch, tmp_path, capsys): @@ -1128,8 +1243,16 @@ def test_isolation_backend_fails_closed_when_namespaces_denied(monkeypatch): def test_probe_isolation_capability_accepts_working_bwrap(monkeypatch): - """A probe that exits zero proves bubblewrap can build the sandbox.""" - monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/true") + """A probe that exits zero proves bubblewrap can build the sandbox. + + ``_probe_isolation_capability`` resolves its probe shell through + ``_probe_shell`` (checked against the fixed ``PROBE_SHELL_PATHS``), not + through ``shutil.which`` -- mocking ``_probe_shell`` directly is what + actually controls this test's inputs; a ``shutil.which`` mock here would + be a silent no-op and this test would instead depend on whatever real + shell the host happens to have mounted. + """ + monkeypatch.setattr(sandboxed_web_e2e, "_probe_shell", lambda: "/bin/true") monkeypatch.setattr( sandboxed_web_e2e.subprocess, "run", @@ -1139,8 +1262,12 @@ def test_probe_isolation_capability_accepts_working_bwrap(monkeypatch): def test_probe_isolation_capability_rejects_denied_namespaces(monkeypatch): - """A nonzero probe exit is classified as bubblewrap being unable to isolate.""" - monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/true") + """A nonzero probe exit is classified as bubblewrap being unable to isolate. + + Mocks ``_probe_shell`` directly rather than ``shutil.which``, which + ``_probe_isolation_capability`` no longer consults for its probe shell. + """ + monkeypatch.setattr(sandboxed_web_e2e, "_probe_shell", lambda: "/bin/true") monkeypatch.setattr( sandboxed_web_e2e.subprocess, "run", @@ -1153,8 +1280,12 @@ def test_probe_isolation_capability_rejects_denied_namespaces(monkeypatch): def test_probe_isolation_capability_rejects_when_probe_cannot_run(monkeypatch): - """A probe that cannot even start is classified as unavailable isolation.""" - monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: "/bin/true") + """A probe that cannot even start is classified as unavailable isolation. + + Mocks ``_probe_shell`` directly rather than ``shutil.which``, which + ``_probe_isolation_capability`` no longer consults for its probe shell. + """ + monkeypatch.setattr(sandboxed_web_e2e, "_probe_shell", lambda: "/bin/true") def _raise(*args, **kwargs): raise OSError("no such file or directory") @@ -1233,8 +1364,15 @@ def test_probe_shell_fails_clearly_when_no_mounted_shell_exists(monkeypatch, tmp def test_probe_isolation_capability_rejects_on_timeout(monkeypatch): - """A probe that hangs past its bounded timeout is classified as unavailable.""" - monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda name, path=None: None) + """A probe that hangs past its bounded timeout is classified as unavailable. + + Mocks ``_probe_shell`` directly rather than ``shutil.which``, which + ``_probe_isolation_capability`` no longer consults for its probe shell; + a ``None`` return from a ``shutil.which`` mock would previously have + been a silent no-op here, leaving this test's actual behavior dependent + on whether the host happens to have a mounted probe shell. + """ + monkeypatch.setattr(sandboxed_web_e2e, "_probe_shell", lambda: "/bin/sh") def _raise(*args, **kwargs): raise subprocess.TimeoutExpired(cmd="bwrap", timeout=10) @@ -1408,6 +1546,39 @@ def test_isolated_command_resolves_bare_command_via_relative_path_entry(monkeypa assert command.endswith("tool") +def test_isolated_command_translates_absolute_workspace_launcher_to_sandbox_mount(monkeypatch, tmp_path): + """An absolute copied-repo launcher path is rewritten to its /workspace equivalent. + + ``isolated_command`` binds ``sandbox_root`` at ``SANDBOX_MOUNT`` inside + bubblewrap, not at its original host path. A caller that copies a + repo-local script alongside the source and invokes it by its absolute + host path (as opposed to a relative ``./launch.sh``-style launcher) + would otherwise pass that literal host path straight through -- a path + that does not exist inside the sandbox, where only ``SANDBOX_MOUNT`` is + bound, so the command would fail to launch there unchanged. + """ + monkeypatch.setattr(sandboxed_web_e2e.shutil, "which", lambda *_args, **_kwargs: None) + sandbox = tmp_path / "sandbox" + repo = sandbox / "repo" + repo.mkdir(parents=True) + launcher = repo / "launch.sh" + launcher.write_text("#!/bin/sh\necho launch\n", encoding="utf-8") + launcher.chmod(0o755) + + command = sandboxed_web_e2e.isolated_command( + f"{launcher} --flag", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + assert command.startswith("/usr/bin/bwrap") + assert "--chdir /workspace/repo" in command + assert command.endswith("/workspace/repo/launch.sh --flag") + assert str(launcher) not in command + + def test_which_relative_to_cwd_returns_none_for_empty_path(tmp_path): """An empty PATH string yields no matches without touching the filesystem.""" sandbox = tmp_path / "sandbox" @@ -1568,6 +1739,35 @@ def test_sandbox_environment_maps_host_paths_to_workspace(tmp_path): assert "XDG_CACHE_HOME" not in mapped +def test_sandbox_environment_translates_workspace_path_entries(tmp_path): + """A PATH entry rooted under the sandbox copy is rewritten to its /workspace form. + + A command that relies on PATH lookup for a workspace-local binary (as + opposed to naming it by an explicit path) inherits this environment + unchanged once launched. Without this translation its PATH would still + name the host copy's absolute directory, which does not exist inside + the bubblewrap mount -- only SANDBOX_MOUNT is bound there -- so the + lookup would fail at runtime even though ``isolated_command`` validated + the same executable successfully ahead of time. + """ + sandbox = tmp_path / "sandbox" + workspace_bin = sandbox / "repo" / "bin" + env = {"PATH": f"{workspace_bin}{os.pathsep}/usr/bin"} + + mapped = sandboxed_web_e2e._sandbox_environment(env, sandbox) + + assert mapped["PATH"] == f"/workspace/repo/bin{os.pathsep}/usr/bin" + + +def test_sandbox_environment_skips_path_translation_when_path_is_absent(tmp_path): + """No PATH key is added when the source environment does not carry one.""" + sandbox = tmp_path / "sandbox" + + mapped = sandboxed_web_e2e._sandbox_environment({"HOME": str(sandbox / "home")}, sandbox) + + assert "PATH" not in mapped + + def test_isolated_command_rejects_empty_command(tmp_path): """Empty commands fail before bubblewrap arguments are constructed.""" with pytest.raises(ValueError, match=re.escape("command must not be empty")): @@ -1623,6 +1823,110 @@ def test_parse_args_rejects_invalid_inputs(): ) +def test_parse_args_rejects_blank_backend_frontend_e2e_commands(capsys): + """A blank command on any of the three flags is rejected during parse_args. + + This validation is independent of ``--isolation`` -- unlike + ``isolated_command``'s own blank-command check, which only ever runs + when isolation is enabled -- so a blank command is rejected the same way + whether or not isolation is later requested as ``disabled``. + """ + base = ["--backend-cmd", "backend", "--frontend-cmd", "frontend", "--e2e-cmd", "e2e"] + for flag in ("--backend-cmd", "--frontend-cmd", "--e2e-cmd"): + argv = list(base) + argv[base.index(flag) + 1] = " " + with pytest.raises(SystemExit) as exc_info: + sandboxed_web_e2e.parse_args(argv) + assert exc_info.value.code == 2 + assert f"{flag} must not be blank" in capsys.readouterr().err + + +def test_parse_args_rejects_malformed_quoting_in_commands(capsys): + """A command with an unmatched shell-quote character is rejected during parse_args. + + Previously, with isolation disabled, this exact input reached + ``shlex.split`` uncaught deep inside ``start_service``/``run_shell`` and + crashed with a raw ``ValueError`` traceback instead of a clean CLI + failure. Validating in ``parse_args`` catches it up front for both + isolation modes. + """ + base = ["--backend-cmd", "backend", "--frontend-cmd", "frontend", "--e2e-cmd", "e2e"] + for flag in ("--backend-cmd", "--frontend-cmd", "--e2e-cmd"): + argv = list(base) + argv[base.index(flag) + 1] = "echo 'unterminated" + with pytest.raises(SystemExit) as exc_info: + sandboxed_web_e2e.parse_args(argv) + assert exc_info.value.code == 2 + assert f"{flag} is not a valid shell command" in capsys.readouterr().err + + +def test_main_disabled_isolation_reports_clean_failure_for_blank_command(tmp_path, capsys): + """Disabled isolation still fails a blank command closed, not with a traceback. + + This is the exact bug this validation fixes: with ``--isolation + disabled``, a blank command used to bypass ``isolated_command`` entirely + and reach ``shlex.split`` inside ``start_service`` uncaught. + """ + repo = tmp_path / "repo" + repo.mkdir() + + with pytest.raises(SystemExit) as exc_info: + sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + " ", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exc_info.value.code == 2 + assert "--frontend-cmd must not be blank" in captured.err + assert "Traceback" not in captured.err + assert "Traceback" not in captured.out + + +def test_main_disabled_isolation_reports_clean_failure_for_malformed_quoting(tmp_path, capsys): + """Disabled isolation still fails malformed shell-quoting closed, not with a traceback. + + This is the exact bug this validation fixes: with ``--isolation + disabled``, unmatched shell-quote characters used to bypass + ``isolated_command`` entirely and raise an uncaught ``ValueError`` from + ``shlex.split`` inside ``run_shell``. + """ + repo = tmp_path / "repo" + repo.mkdir() + + with pytest.raises(SystemExit) as exc_info: + sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + "backend", + "--frontend-cmd", + "frontend", + "--e2e-cmd", + "echo 'unterminated", + ] + ) + captured = capsys.readouterr() + + assert exc_info.value.code == 2 + assert "--e2e-cmd is not a valid shell command" in captured.err + assert "Traceback" not in captured.err + assert "Traceback" not in captured.out + + def test_module_main_entrypoint_parse_error(monkeypatch): """The module entrypoint reaches main and propagates argument errors.""" runpy.run_path(str(Path(sandboxed_web_e2e.__file__)), run_name="not_main")