fix(security): isolate web E2E commands and readiness probes - #1347
Conversation
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough웹 검증 helper가 기본 Linux bubblewrap 격리를 사용한다. 실행 명령은 Changes웹 검증 명령 격리
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The PR strengthens sandboxing and SSRF protection, but a valid workspace containing an ancestor symlink can currently fail before backend, frontend, or E2E verification starts. This concrete execution failure should be fixed or explicitly accepted before merge; the documentation and test portability issues should also be followed up. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant main
participant isolation_backend
participant backend_service
participant frontend_service
participant wait_for_url
participant e2e_command
main->>isolation_backend: bubblewrap capability 확인
isolation_backend-->>main: 격리 backend 반환
main->>backend_service: 격리된 backend 명령 실행
main->>frontend_service: 격리된 frontend 명령 실행
main->>wait_for_url: loopback readiness URL 확인
wait_for_url-->>main: 준비 상태 반환
main->>e2e_command: 격리된 E2E 명령 실행
e2e_command-->>main: 결과와 isolation metadata 반환
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 114 functions across 5 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reconfirmed the three tracked PRs' opencode-review failures are the known async-verdict wait state, not a defect. Re-ran .github#1438's noema-review once after the sidecar's gateway-preflight hit the known 120s/0-byte hang signature again (shared review infra, not this PR's diff). Kicked off parallel research for naruon G-06's next increment, G-15's next increment, and .github#1347's SSRF merge conflict; results land in a follow-up entry.
…-isolation-clean # Conflicts: # CHANGELOG.md # scripts/ci/sandboxed_web_e2e.py # tests/test_sandboxed_web_e2e.py
|
Resolved the stale merge conflict against current Key finding —
Resolution: dropped this PR's In
Test evidence on the merged tree:
Generated by Claude Code |
…-isolation-clean # Conflicts: # CHANGELOG.md # scripts/ci/sandboxed_web_e2e.py # tests/test_sandboxed_web_e2e.py
…clean' into fix/sandboxed-web-e2e-isolation-clean # Conflicts: # CHANGELOG.md # tests/test_sandboxed_web_e2e.py
naruon G-06's human-correction slice shipped (naruon#1486). .github#1347's SSRF/isolation merge conflict resolved via ordinary merge commit, including reconciling with a concurrent session that had already pushed an equivalent resolution to the same branch (adopted their CHANGELOG wording and dropped a harmless duplicate test assertion; both sessions independently found and fixed the same empty-readiness-URL regression). G-15 scoped but not yet implemented.
|
"Hop boundary remains closed" info note and, more importantly, the sibling security finding Devin flagged on the parallel PR #1280 ("Nested directory links escape sandbox") — that PR shares this file's identical hop-walk design, so I checked whether it applied here too. It does, and I reproduced it directly against this file before fixing (see below). Root causeThe whole-target Fix (
|
…ank commands
Devin flagged five findings on this PR; four are confirmed real bugs, fixed
here with fail-before/pass-after regression tests. The fifth (readiness
probes sharing the runner's loopback network namespace) is a real gap but
has no small, non-regressing fix available -- posted as a separate PR
comment for maintainer review instead of a speculative code change.
1. sandboxed_web_e2e.isolated_command rejected valid repository launchers
(e.g. ./gradlew): shutil.which resolves any path-separator-bearing
command against the *wrapper process's* own cwd, never against the
copied repository's cwd the caller actually passes in. Added
_resolve_isolated_executable, which resolves an explicit-path argv[0]
against the sandboxed cwd instead, and widened the bind-root check to
also permit executables inside sandbox_root (mounted at /workspace),
while still fail-closed rejecting path traversal and external paths.
2. sandboxed_verify's DEFAULT_IGNORE ".env.*" glob excluded committed,
secret-free templates (.env.example, .env.sample, .env.template) right
along with real dotenv credential files. Added
DEFAULT_ENV_TEMPLATE_ALLOWLIST and _ignore_with_env_template_allowlist,
which wraps shutil.ignore_patterns to spare those specific names.
3. _probe_isolation_capability resolved its probe shell via
shutil.which("sh") against the caller's own PATH, which can return a
binary outside every root isolated_command actually bind-mounts (e.g. a
PATH entry shadowing sh with a home-directory executable). That shell is
invisible inside the sandbox, so a real working bubblewrap install fails
the probe. Added _probe_shell(), which only picks from PROBE_SHELL_PATHS
(/bin/sh, /usr/bin/sh) -- the same mounted roots isolated_command uses --
and fails clearly if neither exists.
4. A whitespace-only backend/frontend/e2e command made isolated_command
raise ValueError("command must not be empty"), which the call site's
except clause (catching only RuntimeError) let propagate as an uncaught
traceback instead of the documented isolation-rejection exit code 126.
The except clause now also catches ValueError.
Validation: PYTHONPATH=. python3 -m pytest tests -q -> 1942 passed, 1
skipped, 21 subtests passed. coverage on scripts/ci/sandboxed_verify.py and
scripts/ci/sandboxed_web_e2e.py -> 100% statement+branch. Full-repo
coverage and interrogate docstring coverage both 100%.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
…clean' into fix/sandboxed-web-e2e-isolation-clean
Devin findings 1, 2, 4, 5 — triaged and fixedPicked up Devin's 5 new findings independently (parallel session). Findings 1, 2, 4, and 5 are confirmed real bugs and are fixed in commit 1. Repository launchers rejected (
|
Devin finding 3 — "Readiness probes reach runner services" — real gap, no small fix; needs a maintainer decisionConfirmed as real. Not pushing a code change for it — writing this up instead per the task's own instruction for this finding. What's exploitable: Why there's no small patch here:
Recommendation: leave this open for a maintainer/threat-model owner to decide between (a) accepting the residual risk as documented (the loopback check already blocks public hosts, cloud metadata, unspecified binds, and userinfo-confusion — this is specifically about other loopback services on the same runner, a narrower and more CI-runner-specific threat), (b) commissioning the Generated by Claude Code |
…seen-forever Devin flagged "Valid symlink paths are rejected": a symlink referenced twice in one chain -- once fully resolved before the second reference is ever reached, not a real loop -- was incorrectly treated as a cycle, because the walk's `seen` set recorded every symlink ever dereferenced for the whole top-level candidate and never removed one once its resolution completed. Reproduced directly: `shared -> real_dir`, `link -> "shared/../shared/file.txt"` (a path the OS resolves without issue, referencing `shared` twice non-recursively) raised "workspace symlink could not be resolved" before this fix. Fix: restructured the walk from an iterative work-queue with a permanent `seen` set into a genuinely recursive component resolver with an `active` set -- a symlink is added to `active` only while its own target is being resolved (a fresh recursive call) and removed again as soon as that call returns successfully. A cycle is then precisely "a symlink that, directly or through others, points back to itself while still being resolved", which is what `step in active` now tests, rather than "was ever dereferenced anywhere in this chain". The true self-loop test (`a -> b -> a`) still raises, since `a` is still on the active call stack when it is encountered again. Verified: all existing symlink tests pass unchanged (escape, absolute, internal, dangling, excluded-by-DEFAULT_IGNORE, cycle, hop-limit boundary), plus a new regression test for the shared-non-cyclic-reference case. Full suite: 1978 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings.
|
"Valid symlink paths are rejected" — confirmed and fixed in Reproduced directly before fixing: Fix: restructured the walk from an iterative work-queue with a permanent Verified: all existing symlink tests pass unchanged (escape, absolute, internal, dangling, excluded-by- (Porting the same fix to the parallel PR #1280, which shares this design and had the identical latent issue plus a design question it raised — its "accept self-contained cycles" behavior turns out to be unsound once resolution can genuinely continue past a cyclic symlink into further path components, so I'm changing it to reject cycles too, matching this PR's behavior — details there.) Generated by Claude Code |
… tracking Two problems in one fix, both stemming from the same design flaw as the identical hop-walk in the parallel PR #1347: 1. The nested-alias escape fix from an earlier, uncommitted pass on this branch was lost when the worktree was cleaned up before pushing. Redone here as part of this rewrite: reproduced directly (self-alias -> ".", combined with a second symlink targeting "self-alias/../outside-secret.txt", was not caught -- the whole target string collapsed lexically instead of re-resolving the intermediate alias component). 2. "Accept a self-contained cycle" cannot be done soundly once symlinks are resolved component-by-component: a genuine cycle has no well-defined resolved position to hand back to a caller that needs to keep resolving further path components past it, so treating it as merely "contained" risks silently accepting an escape hiding in components that never get processed because the walk stops early. Devin's "Valid symlink paths are rejected" finding on the sibling PR #1347 also applies here in a different, more consequential way: this file's old flat "visited" set treated "resolved once, earlier" the same as "currently resolving", so a legitimate same-symlink-twice reference would have hit the (former) accept-on-revisit branch by coincidence -- for the wrong reason, and silently, without validating whatever came after it in the chain. Fix: rewrote _validate_contained_symlink_cycle as a thin entry point over a new recursive _resolve_repository_symlink_components, matching the component-by-component, active-set design just applied to #1347. A symlink is added to `active` only while its own target is being resolved and removed once that recursive call returns successfully, so the same symlink referenced twice non-recursively (fully resolved once, then referenced again later) is accepted, while a genuine self-referential cycle raises RepositoryPathBoundaryError -- changing this file's cycle policy from "accept if contained" to "reject", matching the sibling PR's simpler, already-established, more conservative behavior. Updated the two tests that asserted cycle-acceptance to assert rejection instead (renamed accordingly), and added a regression test for the shared-non-cyclic reference case. Verified: nested-alias escape now caught; shared-reference case now accepted; all other existing symlink tests (escape, absolute, internal, excluded-by-ignore, hop-limit boundary) pass unchanged. Full suite: 1997 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings.
Devin flagged (thread PRRT_kwDOS_C14s6dh6My) that sandboxed_verify.py's main() calls copy_workspace() with no except around it: a symlink-escape rejection propagated as an uncaught ValueError, printing a raw Python traceback and exiting with Python's default uncaught-exception status instead of this module's own clean "sandboxed-verify: ..." message and coded exit (e.g. 124 for the timeout path). --keep-sandbox still retains the rejected copy either way, which matches its documented "for debugging" purpose -- not a bug to fix here. sandboxed_web_e2e.py calls the same copy_workspace() with the identical gap, found independently while fixing the sibling script; fixed the same way (exit 125, matching the code already used for its other ValueError rejections like an invalid readiness URL). Added a regression test per script asserting a clean exit 125, no traceback, and a still-emitted result payload.
…t; resolve bare commands via relative PATH entries Devin review findings on PR #1347: - sandboxed_verify.py: _ignore_with_env_template_allowlist merged DEFAULT_IGNORE and the caller's extra_ignores into one combined pattern set before restoring DEFAULT_ENV_TEMPLATE_ALLOWLIST names, so an explicit --ignore .env.example (or any caller-supplied extra_ignores entry) was silently overridden and the file still landed in the writable sandbox. Now builds two separate ignore functions -- one from DEFAULT_IGNORE alone (whose broad .env.* glob the allowlist exists to except from) and one from extra_ignores alone (never overridden) -- and only restores a name matched solely by the former. - sandboxed_web_e2e.py: _resolve_isolated_executable resolved a bare PATH-searched command purely via shutil.which(), which always resolves a relative PATH entry against the wrapper process's own cwd with no way to override that. A PATH with a relative entry meant to be read relative to the copied repository (e.g. PATH=bin:/usr/bin) therefore failed isolation (exit 126) even when the tool legitimately existed under the sandboxed cwd. Falls back to a new _which_relative_to_cwd that mirrors shutil.which's PATH-splitting/executable-bit checks by hand, anchoring relative entries at cwd; a relative entry that would resolve outside sandbox_root is skipped without touching the real filesystem, keeping the existing fail-closed behavior for a traversal PATH like ../../... Regression tests added for both, verified fail-before/pass-after against a temporary revert of each fix. Full suite: 1982 passed, 1 skipped, 21 subtests passed. scripts/ci/sandboxed_verify.py and scripts/ci/sandboxed_web_e2e.py: 100% statement+branch coverage, 100% docstring coverage (interrogate). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
…clean' into fix/sandboxed-web-e2e-isolation-clean-local
Triage of the 2 new Devin findings1. "Explicit exclusions cannot remove environment templates" — CONFIRMED, fixed.
Fix: Regression test added: 2. "Relative PATH tools fail isolation" — CONFIRMED, fixed.
Fix: added Regression tests added:
Confirmed fail-before (reverted the fix, the relative-PATH resolution test failed with the exact reported Process notes
Commits pushed:
Validation on the final pushed tree:
_Generated by Claude Code Generated by Claude Code |
…-isolation-clean # Conflicts: # CHANGELOG.md
|
Reconciled the merge conflict against What conflicted: only How resolved: kept both sets of entries in full — this PR's isolation/SSRF-hardening changelog entries first, followed by Verified this PR's actual code is untouched by the conflict: Merge mechanics: ordinary Post-merge verification (all green):
Branch pushed as a merge commit ( Generated by Claude Code |
| def isolated_command( | ||
| command: str, | ||
| *, | ||
| backend: str, | ||
| cwd: Path, | ||
| sandbox_root: Path, | ||
| env: dict[str, str], | ||
| ) -> str: |
There was a problem hiding this comment.
…-command, and workspace-path gaps
Four Devin findings on this PR's bubblewrap sandboxing:
- wait_for_url's opener now passes an explicit ProxyHandler({}) alongside
NoRedirectHandler, so HTTP_PROXY/HTTPS_PROXY/*_proxy environment variables
can never route a "loopback-only, isolated" readiness probe through an
external proxy (mirrors the fix already applied to a different opener in
materialize_base_python_requirements.py).
- New require_unoccupied_readiness_port rejects a readiness URL whose port
already answers before this run starts its own service, called once in
main() right before start_service. isolated_command does not create a
network namespace for the commands it wraps (the host readiness poller and
the E2E command both need to reach the same loopback ports), so this
closes the "polls some other, unrelated runner service" gap without
breaking that shared-loopback design.
- parse_args now shell-tokenizes all three of --backend-cmd/--frontend-cmd/
--e2e-cmd up front, independent of --isolation, and rejects a blank or
unmatched-quote command through argparse's own clean SystemExit(2) path.
Previously, with isolation disabled, such a command bypassed
isolated_command entirely and crashed with an uncaught ValueError deep
inside start_service/run_shell's own shlex.split call.
- isolated_command now rewrites an absolute executable path that resolves
inside the sandbox copy to its /workspace-relative form, and
_sandbox_environment now does the same for PATH entries rooted under the
sandbox copy -- bubblewrap binds the copy at /workspace, not at its
original host path, so an absolute copied-repo launcher or PATH entry
previously failed to launch inside the sandbox unchanged.
Also folds in a CodeRabbit finding on the same head, in sandboxed_verify.py:
_reject_escaping_symlinks walked the unresolved destination path but checked
each symlink against the resolved root, so a sandbox root reached through a
symlinked ancestor (e.g. a symlinked default temp directory) made
path.relative_to(root) raise for every symlink in an otherwise-legitimate
copy. Now walks from the already-resolved root instead; escape detection
itself is unchanged and still covered.
Plus two quick-win CodeRabbit items: documents the DEFAULT_ENV_TEMPLATE_ALLOWLIST
carve-out in the command-isolation doc, and fixes four tests that mocked
shutil.which for _probe_isolation_capability's shell selection after it was
changed to check fixed PROBE_SHELL_PATHS directly instead -- those mocks were
silent no-ops relying on whatever shell the host happened to have mounted.
All new behavior is covered by new regression tests reproducing each bug
against pre-fix code; scripts/ci stays at 100% line+branch coverage and 100%
docstring coverage.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
|
Pushed 1. 🟥 Proxy bypass on the readiness probe — fixed
Fix: Regression test ( 2. 🟨 Readiness probes reach unrelated runner services — fixed with a different mechanism than either suggested optionConfirmed real: I didn't implement either literal suggested option, for reasons grounded in this PR's own doc (
Instead, added Regression tests: 3. 🟡 Debug-mode crash on malformed commands — fixedConfirmed: with Fix: Note this necessarily changes one existing test: Added 4. 🟡 Absolute workspace paths don't launch — fixedConfirmed: Fix: Tests: 5. CodeRabbit: ancestor-symlink false-positive rejection in
|
| 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) |
…-isolation-clean # Conflicts: # CHANGELOG.md # scripts/ci/sandboxed_web_e2e.py
Merge conflict resolved (merge commit, no rebase)
Conflicts found (2 files — verified directly, not assumed)1. Verified via CPython's 2. Non-conflicts checked explicitly (flagged as risk areas in the task): Verification (all green, post-merge, before push)
No force-push, no history rewrite — this branch's prior commits and authorship are untouched. Generated by Claude Code |
| 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}" | ||
| ) |
There was a problem hiding this comment.
…#1467) * fix(sandboxed-web-e2e): probe explicit port 0, not the scheme default require_unoccupied_readiness_port() derived the probe port with `parsed.port or (443 if https else 80)`. urllib.parse's .port returns the int 0 for a URL with an explicit :0 port, and `0 or X` evaluates to X in Python, so an explicitly-requested port 0 was silently replaced with the scheme's default port (80/443) instead of actually being probed. Devin's review on PR #1347 flagged this pattern but it was out of scope for that PR's authorized task. Switch to an explicit None check so port 0 is honored, and add a regression test that monkeypatches socket.create_connection to record the probed address and assert it names port 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(security): scope Bandit tmpfs waivers to isolation targets Bandit B108 correctly began running after the central file-detection repair and identified the two literal /tmp arguments passed to bubblewrap. These are tmpfs mount targets inside a new isolated namespace, not host temporary-file paths. Add B108-only waivers with adjacent rationale and a regression that pins both the scope and count, preserving every other Bandit rule and host-path finding. * fix(lint): pair Ruff S108 waivers with the existing Bandit B108 ones CodeRabbit ran `ruff check --select S108` against this file and found the two bubblewrap tmpfs `"/tmp"` mount-target arguments still fail Ruff's own insecure-temp-path rule -- `# nosec B108` only silences Bandit, not Ruff. Add `# noqa: S108` alongside each existing waiver and extend the regression test that already pins the Bandit waiver's exact text/count/rationale to also pin the Ruff waiver, so this scoped exception can't silently broaden to other paths or rules. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Verification
python3 -m pytest -q tests/test_sandboxed_web_e2e.py tests/test_repository_branch_coverage_execution_sandboxes.py tests/test_opencode_existing_approval_gate.py tests/test_opencode_security_boundaries.py(92 passed)python3 -m ruff check scripts/ci/sandboxed_web_e2e.py tests/test_sandboxed_web_e2e.py tests/test_repository_branch_coverage_execution_sandboxes.pygit diff --checkThis clean PR contains only the SSRF/isolation change; it supersedes stale #1342 without its unrelated workflow reverts.
Summary by CodeRabbit
새로운 기능
/workspace만 수정할 수 있습니다.버그 수정
보안
문서