diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d402ee428..6682241627 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,41 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Web verification now runs backend, frontend, and E2E commands inside an + isolated Linux bubblewrap workspace by default (`--isolation required`), + mounting a read-only runtime root with a single writable `/workspace` + bind; trusted local debugging may opt out with `--isolation disabled`. + Isolation-backend resolution and the existing loopback readiness-URL + boundary are now both checked before any service starts, so an + 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`. +- (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 안에 유지). - Fix two live-on-`main` regressions Devin Review found immediately after PRs #1456 and #1459 merged (both bypass-merged past the org-wide `opencode-review` outage; these hotfixes correct real defects the local diff --git a/docs/doctoring/sandboxed-web-command-isolation.md b/docs/doctoring/sandboxed-web-command-isolation.md new file mode 100644 index 0000000000..55105deb3b --- /dev/null +++ b/docs/doctoring/sandboxed-web-command-isolation.md @@ -0,0 +1,85 @@ +# Sandboxed web command isolation + +`sandboxed_web_e2e.py` requires Linux `bubblewrap` (`bwrap`) by default. Each +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 +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 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 +mistaken for an OS-isolated run. If required isolation is unavailable, the +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. + +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. 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 +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 +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 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 c2ce78cf03..758ef2961a 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 처리. + ## 2026-08-30 sidecar preflight `max_tokens`: explicit owner critique, ADR-0005 (revised after Devin Review) Direct owner feedback after #1436's `max_tokens` 16→4096 raise moved the sidecar's gateway preflight diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index aace18d454..94797c2038 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 @@ -33,6 +33,26 @@ "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", + # Note: DEFAULT_ENV_TEMPLATE_ALLOWLIST below carves committed, + # secret-free dotenv templates back out of the ".env.*" glob above. + ".netrc", + ".npmrc", + ".pypirc", + ".pgpass", + ".git-credentials", + ".ssh", + ".gnupg", + ".aws", + ".kube", + ".docker", ) SECRET_ENV_TOKENS = ( "TOKEN", @@ -55,8 +75,19 @@ "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 def parse_args(argv: Sequence[str] | None = None) -> argparse.Namespace: @@ -138,14 +169,169 @@ 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 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. + + 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 root.rglob("*"): + if path.is_symlink(): + _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. 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. + """ + for component in parts: + 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 active: + raise ValueError(f"workspace symlink could not be resolved: {candidate}") + if hops_remaining[0] <= 0: + raise ValueError(f"workspace symlink could not be resolved: {candidate}") + 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}" + ) + resolved = _resolve_symlink_components( + target.parts, resolved, root, active, hops_remaining, candidate + ) + active.discard(step) + return resolved + + +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 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. + """ + default_ignore = shutil.ignore_patterns(*default_patterns) + extra_ignore = shutil.ignore_patterns(*extra_patterns) + + def _ignore(directory: str, names: list[str]) -> set[str]: + """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 + + 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 @@ -208,7 +394,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 ab7fb969e7..1be073104a 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -6,6 +6,7 @@ import ipaddress import json import os +import platform import signal import shutil import shlex @@ -28,6 +29,7 @@ RESULT_MARKER = "SANDBOXED_WEB_E2E_RESULT" +SANDBOX_MOUNT = "/workspace" class NoRedirectHandler(urllib.request.HTTPRedirectHandler): @@ -66,6 +68,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,9 +109,341 @@ 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") + + +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()] + + +# 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. + + 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 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 = _probe_shell() + bind_args: list[str] = [] + for root in _bind_roots(): + bind_args.extend(("--ro-bind", str(root), str(root))) + 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}") + + +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") + _probe_isolation_capability(backend) + 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) + 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``. + + ``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 + 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 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)) + if candidate.is_file() and os.access(candidate, os.X_OK): + return candidate + return None + found = shutil.which(argv0, path=path) + 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( + command: str, + *, + backend: str, + cwd: Path, + sandbox_root: Path, + env: dict[str, str], +) -> str: + """Wrap one command in a read-only-root bubblewrap workspace. + + 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. 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: + raise ValueError("command must not be empty") + bind_roots = _bind_roots() + 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()): + raise RuntimeError("commands from the host home directory are not allowed in isolation") + 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}" + ) + 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))) + 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( + ( + "--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" @@ -153,13 +496,21 @@ 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, 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. """ 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") @@ -169,8 +520,55 @@ 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) @@ -183,6 +581,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 @@ -249,6 +648,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, } @@ -268,23 +669,91 @@ 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) + 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)) - 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]) + command_env = _sandbox_environment(env, sandbox) if backend else 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 + ) + 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 + ) + except (RuntimeError, ValueError) as exc: + print(f"sandboxed-web-e2e: isolation rejected command: {exc}", file=sys.stderr) + exit_code = 126 + return exit_code + 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 + 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: + 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 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_verify.py b/tests/test_sandboxed_verify.py index c711f34898..d89b9bb956 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -74,12 +74,381 @@ 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_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_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"): 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_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. + + 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() + (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_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" + 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_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. + + 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_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. + + 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_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. + + ``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) == "" @@ -175,6 +544,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 5ac3145db1..4ab489c179 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -1,6 +1,8 @@ import json import os +import re import runpy +import shutil import socket import subprocess import sys @@ -56,6 +58,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", @@ -111,14 +115,14 @@ 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://127.0.0.1:1/", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("http://localhost:1/", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("http://localhost./health", 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 assert sandboxed_web_e2e.wait_for_url("http://[::1]:1/health", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("HTTP://[::ffff:127.0.0.1]:1/", 1, exited_service) is False assert sandboxed_web_e2e.wait_for_url("https://127.0.0.1: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) sandboxed_web_e2e.stop_service(exited_service) assert sandboxed_web_e2e.tail_text(tmp_path / "missing.log") == "" @@ -205,7 +209,8 @@ def poll(self): return None class Response: - status = 204 + def __init__(self, status): + self.status = status def __enter__(self): return self @@ -220,17 +225,19 @@ 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) + 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") 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 sleeps == [1, 1] assert sandboxed_web_e2e.tail_text(log_path).splitlines()[0] == "line-10" with pytest.raises(ValueError, match="URL cannot target external hostname: example\\.com"): @@ -279,6 +286,49 @@ def test_wait_for_url_ignores_environment_proxy_configuration(monkeypatch, tmp_p sandboxed_web_e2e.stop_service(service) +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) @@ -306,6 +356,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) @@ -356,6 +414,209 @@ 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_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. + + 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" + 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 @@ -426,6 +687,8 @@ def fake_start(label, command, cwd, env, logs_dir): [ "--repo-root", str(repo), + "--isolation", + "disabled", "--backend-cmd", "backend", "--frontend-cmd", @@ -464,25 +727,36 @@ def fake_start(label, command, cwd, env, logs_dir): assert payload["evidence_note"] == "needs browser auth" -def test_main_reports_stubbed_readiness_failure(monkeypatch, tmp_path, capsys): - """Main exits distinctly when a stubbed service never becomes ready.""" +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} not ready\n", encoding="utf-8") + 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) - def fake_wait(url, timeout, service): - return service.label == "frontend" - + 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", fake_wait) + 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( @@ -493,46 +767,36 @@ def fake_wait(url, timeout, service): "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 "service readiness failed" in captured.err + 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/") 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["backend_ready"] is False - assert payload["frontend_ready"] is True - assert payload["exit_code"] == 125 + assert payload["isolation"] == "required" + assert payload["isolation_backend"] == "/usr/bin/bwrap" -def test_main_reports_stubbed_e2e_timeout(monkeypatch, tmp_path, capsys): - """Main preserves timeout output from stubbed E2E execution.""" +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 = [] - 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(f"{label} tail\n", encoding="utf-8") - return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) - - def fake_run_shell(command, cwd, env, timeout): - raise subprocess.TimeoutExpired(command, timeout, output=b"e2e-out", stderr=b"e2e-err") - - 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", fake_run_shell) - monkeypatch.setattr(sandboxed_web_e2e, "stop_service", lambda service: None) + 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( [ @@ -542,72 +806,433 @@ def fake_run_shell(command, cwd, env, timeout): "backend", "--frontend-cmd", "frontend", - "--e2e-timeout", - "3", "--e2e-cmd", "e2e", ] ) captured = capsys.readouterr() - assert exit_code == 124 - assert "e2e-out" in captured.out - assert "e2e-err" in captured.err - assert "e2e command timed out after 3s" in captured.err + 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_coded_failure_for_whitespace_only_command(monkeypatch, tmp_path, capsys): + """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() + 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)) -@POSIX_PROCESS_GROUPS -def test_sandboxed_web_e2e_reports_readiness_failure(tmp_path, capsys): - """Readiness failures return a distinct nonzero exit code.""" + 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 exc_info.value.code == 2 + assert not started + assert "--backend-cmd must not be blank" in captured.err + assert "Traceback" not in captured.err + assert "Traceback" 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() - backend_port = free_port() - frontend_port = free_port() + 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", - http_server_command(backend_port, "backend"), + "backend", "--frontend-cmd", - http_server_command(frontend_port, "frontend"), - "--backend-ready-url", - "http://127.0.0.1:1/not-ready", - "--frontend-ready-url", - f"http://127.0.0.1:{frontend_port}/", - "--startup-timeout", - "1", - "--e2e-timeout", - "5", + "frontend", "--e2e-cmd", - f"{sys.executable} -c \"raise SystemExit(99)\"", + "e2e", ] ) captured = capsys.readouterr() - assert exit_code == 125 - assert "service readiness failed" in captured.err - assert "SANDBOXED_WEB_E2E_RESULT" in captured.out + 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" -@POSIX_PROCESS_GROUPS -def test_sandboxed_web_e2e_reports_e2e_timeout(monkeypatch, tmp_path, capsys): - """E2E command timeout is reported without losing captured output.""" +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" repo.mkdir() - def fake_run_shell(command, cwd, env, timeout): - raise subprocess.TimeoutExpired(command, timeout, output="e2e-out", stderr="e2e-err") + class DoneProcess: + def poll(self): + return 0 - monkeypatch.setattr(sandboxed_web_e2e, "run_shell", fake_run_shell) + def fake_start(label, command, cwd, env, logs_dir): + log_path = logs_dir / f"{label}.log" + log_path.write_text(f"{label} not ready\n", encoding="utf-8") + return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) - exit_code = sandboxed_web_e2e.main( - [ - "--repo-root", - str(repo), - "--backend-cmd", - f"{sys.executable} -c \"import time; time.sleep(3)\"", + def fake_wait(url, timeout, service): + return service.label == "frontend" + + monkeypatch.setattr(sandboxed_web_e2e, "start_service", fake_start) + monkeypatch.setattr(sandboxed_web_e2e, "wait_for_url", fake_wait) + 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 "service readiness failed" 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["backend_ready"] is False + assert payload["frontend_ready"] is True + 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() + 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) + + 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 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_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" + 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" + 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(f"{label} tail\n", encoding="utf-8") + return sandboxed_web_e2e.Service(label, command, DoneProcess(), log_path) + + def fake_run_shell(command, cwd, env, timeout): + raise subprocess.TimeoutExpired(command, timeout, output=b"e2e-out", stderr=b"e2e-err") + + 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", fake_run_shell) + 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", + "--e2e-timeout", + "3", + "--e2e-cmd", + "e2e", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 124 + assert "e2e-out" in captured.out + 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): + """Readiness failures return a distinct nonzero exit code.""" + repo = tmp_path / "repo" + repo.mkdir() + backend_port = free_port() + frontend_port = free_port() + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + http_server_command(backend_port, "backend"), + "--frontend-cmd", + http_server_command(frontend_port, "frontend"), + "--backend-ready-url", + "http://127.0.0.1:1/not-ready", + "--frontend-ready-url", + f"http://127.0.0.1:{frontend_port}/", + "--startup-timeout", + "1", + "--e2e-timeout", + "5", + "--e2e-cmd", + f"{sys.executable} -c \"raise SystemExit(99)\"", + ] + ) + captured = capsys.readouterr() + + assert exit_code == 125 + assert "service readiness failed" in captured.err + assert "SANDBOXED_WEB_E2E_RESULT" in captured.out + + +@POSIX_PROCESS_GROUPS +def test_sandboxed_web_e2e_reports_e2e_timeout(monkeypatch, tmp_path, capsys): + """E2E command timeout is reported without losing captured output.""" + repo = tmp_path / "repo" + repo.mkdir() + + def fake_run_shell(command, cwd, env, timeout): + raise subprocess.TimeoutExpired(command, timeout, output="e2e-out", stderr="e2e-err") + + monkeypatch.setattr(sandboxed_web_e2e, "run_shell", fake_run_shell) + + exit_code = sandboxed_web_e2e.main( + [ + "--repo-root", + str(repo), + "--isolation", + "disabled", + "--backend-cmd", + f"{sys.executable} -c \"import time; time.sleep(3)\"", "--frontend-cmd", f"{sys.executable} -c \"import time; time.sleep(3)\"", "--e2e-cmd", @@ -625,6 +1250,581 @@ 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_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 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) + 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. + + ``_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", + 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. + + 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", + 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. + + 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") + + 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_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_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. + + 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) + + 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_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") + 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 "--tmpfs /" in command + 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): + """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=re.escape("host home directory")): + sandboxed_web_e2e.isolated_command( + "tool", + backend="/usr/bin/bwrap", + cwd=repo, + sandbox_root=sandbox, + env={"PATH": "/usr/bin"}, + ) + + +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=re.escape("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_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_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_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" + 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) + 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. + + 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" + 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": "/usr/bin"}, + ) + + +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_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")): + 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): @@ -668,6 +1868,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") @@ -710,6 +2014,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",