feat(ci): add a bounded subprocess primitive - #1280
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPOSIX bounded subprocess 경계를 추가했다. 검증과 웹 E2E 실행은 출력 한도, 프로세스 그룹 정리, UTF-8 suffix, 심볼릭 링크 경계, 실행 오류 분류를 사용한다. 결과 필드와 회귀 테스트도 갱신했다. Changes샌드박스 실행 경계
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: ⚪ Minimal · up to This change ensures the service-startup failure path is tested without requiring the isolation backend. No merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant sandboxed_verify
participant RepositoryCopy
participant bounded_subprocess
participant CommandProcess
sandboxed_verify->>RepositoryCopy: workspace 복사 및 symlink 경계 검사
sandboxed_verify->>bounded_subprocess: bounded 검증 명령 실행
bounded_subprocess-->>sandboxed_verify: 제한된 출력과 종료 상태 반환
sandboxed_verify->>CommandProcess: 오류를 종료 코드와 결과 필드로 분류
sequenceDiagram
participant sandboxed_web_e2e
participant BackendService
participant FrontendService
participant bounded_subprocess
sandboxed_web_e2e->>BackendService: bounded 서비스 시작
sandboxed_web_e2e->>FrontendService: bounded 서비스 시작
sandboxed_web_e2e->>BackendService: HTTP readiness 확인
sandboxed_web_e2e->>FrontendService: HTTP readiness 확인
sandboxed_web_e2e->>bounded_subprocess: bounded E2E 명령 실행
bounded_subprocess-->>sandboxed_web_e2e: 제한된 출력과 종료 상태 반환
sandboxed_web_e2e->>BackendService: 서비스 종료 및 capture 정리
sandboxed_web_e2e->>FrontendService: 서비스 종료 및 capture 정리
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
88f5fcc62671ca6e635be05a9ab583adf7399c7a. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/strix: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/32663339769/job/97252770534)
- Strix Security Scan/strix: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/32663339769/job/97252770534)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: sandboxed-output-resource-bounds.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: sandboxed-output-resource-bounds.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: bounded_subprocess.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: bounded_subprocess.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (3 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (3 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed. Findings1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
Failed checks:
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["CI script (3 files)"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script (3 files)"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test (12 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test (12 files)"]
R4 --> V4["targeted test run"]
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head local evidence for |
|
@opencode-agent review | Independent exact-current-head formal review requested for |
|
Exact-head RCA for e587c05: Strix run 32691970400 failed closed after repeated NVIDIA NIM HTTP 429 responses, then the distinct fallback returned HTTP 404; no vulnerability report artifact was produced. This is provider/backend infrastructure evidence, not a source finding in the bounded subprocess change. Keep the fail-closed gate and rerun the failed Strix job normally. |
Current-head re-review request\n\nCurrent exact head: e587c05\n\nThe previous REQUEST_CHANGES review is tied to an older Strix/provider-failure head. Current-head evidence must be re-established from this SHA; predecessor failures and approvals are not being reused. Please inspect the complete current diff and publish a substantive review after the current required Checks finish. No merge is requested until exact-head checks and independent approval are current. |
Exact-head verification receipt
No source, workflow, ruleset, protected-branch, or bypass mutation was used. |
|
Exact-head formal review request for e587c05. Please inspect the complete changed-file diff, with emphasis on bounded subprocess behavior, timeout/cancellation, and authority boundary. Publish a substantive Reviews API verdict tied only to this exact SHA, including security/control-plane impact, test gaps, and residual uncertainty. Do not reuse predecessor-head evidence or failed provider checks as source findings. @opencode-agent |
|
Exact-head |
* fix(sandbox): bound verification evidence * fix(sandbox): classify missing verification executable * fix(sandbox): classify non-executable commands
* fix(sandbox): bound verification evidence * fix(sandbox): bound web E2E evidence * fix(sandbox): classify missing verification executable * fix(sandbox): classify missing web executables * fix(sandbox): classify non-executable commands * fix(sandbox): reject invalid readiness URL schemes
e587c05 to
70ad61f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…rsing The previous empty-command validation called shlex.split(command) directly, which raises an uncaught ValueError on unmatched quoting and leaked a traceback instead of the established clean parser.error path.
|
One more small finding on my own just-pushed fix: the empty-command validation called Validation: full suite Generated by Claude Code |
| try: | ||
| tokens = shlex.split(command) | ||
| except ValueError as error: | ||
| parser.error(f"{option} is invalid: {error}") | ||
| if not tokens: | ||
| parser.error(f"{option} must not be empty") |
There was a problem hiding this comment.
📝 Info: Validation matches command execution
parse_args uses the same tokenizer as execution. Malformed quoting now follows the argument-error contract instead of escaping with a traceback.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ject escapes Devin flagged that _reject_escaping_symlinks's Path.resolve(strict=True) rejected any unresolvable symlink uniformly, including a plain dangling target whose own file was legitimately excluded from the copy by DEFAULT_IGNORE/extra_ignores (or is simply broken) -- aborting an otherwise-valid verification run over a link that was never actually a host-escape attempt. Replaced the resolve()-based check with a hop-by-hop lexical walk (os.readlink + os.path.normpath, tracked via a visited set), matching the existing design in the parallel PR #1280's sandboxed_verify.py. This never requires a target to exist, so a dangling-but-contained symlink is now accepted while an actual escape (absolute target, or a normalized target outside the sandbox root) or an unresolvable cycle (revisiting an already-followed path) still raises the same ValueError as before. The hop count is bounded so a chain that never repeats due to purely lexical normalization still fails closed instead of walking forever. Added regression tests for: a dangling target with no cycle, a target excluded from the copy by DEFAULT_IGNORE, and a chain exceeding the hop limit without ever cycling or escaping.
validate_repository_symlinks only ran _validate_contained_symlink_cycle's absolute-hop/lexical-escape/hop-limit checks as a Path.resolve()-triggered RuntimeError fallback. That fallback never fires: resolve() only reports absoluteness for a chain's first hop (silently following any absolute hop reached partway through), and its behavior on a genuine cycle is not reliable evidence either way -- it raises RuntimeError on Python < 3.13 but silently returns a partially-resolved path, with no error at all, starting in 3.13 (confirmed directly: 3.11/3.12 raise, 3.13 does not). The hosted CI gate runs Python 3.14, so a fully-contained two-node symlink cycle already returned a resolved path there without ever exercising the manual walk, leaving _validate_contained_symlink_cycle's cycle-return branch and the fallback's own call site permanently uncovered (Python 3.14 full quality gate, job 99271248147: sandboxed_verify.py at 98%, missing 191 and 239-241) -- and, more importantly, meant its absolute-hop and lexical-escape protections for hops after the first were never actually reachable from the real copy_workspace entry point on any Python version, only from the direct unit tests that call the helper itself. Replaced the resolve()-based single-hop check with an unconditional call to _validate_contained_symlink_cycle for every symlink found. It already implements the full walk (absolute-hop rejection, lexical-escape rejection, cycle-acceptance, hop-limit) purely via os.readlink + os.path.normpath, without touching Path.resolve() at all, so behavior is now deterministic across Python versions and every hop of a chain is checked, not just the first. All existing symlink-boundary tests (escape, absolute, internal, contained-cycle, hop-limit) pass unchanged since this is a strict superset of the prior single-hop resolve() check. Verified: full suite 1995 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage (was 98%) and 100% docstrings, reproduced on Python 3.11 (this sandbox) after confirming the resolve() cycle-detection version split directly (3.11/3.12 raise RuntimeError, 3.13 does not).
|
Fixed the "Python 3.14 full quality gate" failure (job Root cause, confirmed directly rather than guessed: Beyond the coverage number, this also meant Fix ( Verified: full suite Generated by Claude Code |
| if candidate.is_symlink(): | ||
| _validate_contained_symlink_cycle(candidate, source_root) |
There was a problem hiding this comment.
📝 Info: Intermediate absolute links remain covered
os.walk validates each copied symlink independently. An absolute intermediate link therefore reaches _validate_contained_symlink_cycle even when another chain stops before it.
Was this helpful? React with 👍 or 👎 to provide feedback.
The same off-by-one Devin flagged on the parallel PR #1347 (which shares this file's identical hop-walk design) applies here too, and my prior commit (da0532d, routing every symlink through this walk unconditionally instead of only on a resolve()-triggered fallback) made it newly reachable in production rather than just from direct unit tests: a chain of exactly MAXIMUM_SYMLINK_HOPS (40) real, OS-resolvable symlinks was incorrectly rejected. Each loop iteration checks one position and, only if it is a symlink, advances to the next -- so resolving N real hops needs N+1 checks (N to walk them, one more to confirm the final landing position is not itself a further symlink). range(MAXIMUM_SYMLINK_HOPS) only gave N checks. Reproduced directly: a 40-real-symlink chain terminating in a real file raised "exceeds the supported hop limit" before the fix, copies cleanly after. Fix: range(MAXIMUM_SYMLINK_HOPS + 1). Verified this doesn't change any existing test's outcome, including test_validate_contained_symlink_cycle_fails_closed_past_hop_limit (a 3-node cycle with the hop limit monkeypatched to 2 still exceeds the new, also-monkeypatched-relative budget) and the cycle-acceptance tests (a real cycle is caught well within budget regardless of the +1). Added a boundary regression test for exactly-N-hops success. Full suite: 1996 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings.
|
Proactive fix, not from a bot finding on this PR: Devin flagged an off-by-one in the identical hop-walk design on the parallel PR #1347 ("Forty-link chains fail verification") — a chain of exactly My prior commit here ( Fixed in Verified: full suite Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin flagged "nested directory links escape sandbox" on the parallel PR #1280, which shares this file's hop-walk design: a symlink target that itself contains an intermediate component which is a symlink (e.g. "some-alias/../secret") defeats a check that collapses the whole target string in one os.path.normpath call, because normpath cancels "some-alias" against the following ".." purely textually without ever re-examining whether some-alias is itself a symlink needing its own resolution first. Reproduced directly against this file (not just #1280's): a self-alias symlink pointing at "." (its own parent, the repo root -- entirely legitimate and safe standing on its own) combined with a second symlink whose target is "self-alias/../outside-secret.txt" was NOT caught -- the whole string collapsed lexically to "outside-secret.txt" (looking safe), while resolving it for real, one component at a time, correctly shows that following self-alias lands at the repo root itself (zero depth), so the very next ".." immediately exits it. Fix: replaced the whole-target os.path.normpath collapse with a component-by-component walk that re-checks is_symlink() after every single path segment, substituting a symlink's own target components back onto the work queue instead of treating the whole original target string as one atomic lexical unit. This also fixes the hop-limit off-by-one from fe237c4 as a natural consequence: a hop's budget is now spent only when a symlink is actually dereferenced, not once per loop iteration, so a chain of exactly MAXIMUM_SYMLINK_HOPS real symlinks needs no special-casing. Verified against all existing symlink tests unchanged (escape, absolute, internal, dangling, excluded-by-DEFAULT_IGNORE, cycle, hop-limit boundary) plus two new ones: the nested-alias escape (must now raise) and a legitimate cross-directory ".." traversal that stays in-bounds (must still be accepted, for branch coverage on the successful ".." path). Full suite: 1969 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings.
… 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.
|
Two fixes in 1. The nested-alias escape fix from an earlier pass on this branch was lost — I'd written and verified it in a worktree but the worktree got cleaned up before I pushed it, amid a burst of notification triage. Redone here: reproduced directly ( 2. "Accept a self-contained cycle" cannot be done soundly once symlinks are resolved component-by-component. Devin's "Valid symlink paths are rejected" finding on #1347 (same hop-walk design) also applies here, in a more consequential way: this file's old flat Fix: rewrote 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: Generated by Claude Code |
| for component in parts: | ||
| if component == "..": | ||
| if resolved == source_root: | ||
| raise RepositoryPathBoundaryError( | ||
| f"symlink escapes repository verification sandbox: {candidate}" | ||
| ) | ||
| resolved = resolved.parent | ||
| continue | ||
| step = resolved / component | ||
| if not step.is_symlink(): | ||
| resolved = step | ||
| continue | ||
| if step in active: | ||
| raise RepositoryPathBoundaryError( | ||
| f"symlink chain could not be resolved: {candidate}" | ||
| ) | ||
| if hops_remaining[0] <= 0: | ||
| raise RepositoryPathBoundaryError( | ||
| f"symlink chain exceeds the supported hop limit: {candidate}" | ||
| ) | ||
| active.add(step) | ||
| hops_remaining[0] -= 1 | ||
| target = Path(os.readlink(step)) | ||
| if target.is_absolute(): | ||
| raise RepositoryPathBoundaryError( | ||
| f"symlink escapes repository verification sandbox via absolute target: " | ||
| f"{step} -> {target}" | ||
| ) | ||
| resolved = _resolve_repository_symlink_components( | ||
| target.parts, resolved, source_root, active, hops_remaining, candidate | ||
| ) | ||
| active.discard(step) | ||
| return resolved |
There was a problem hiding this comment.
📝 Info: Intermediate symlinks resolve before traversal
The walker resolves each alias before later .. components. Active-chain tracking rejects recursive cycles without rejecting completed repeated references.
Was this helpful? React with 👍 or 👎 to provide feedback.
…killpg race Devin flagged "Cleanup race leaves service zombies": stop_service's poll() is None check only proves the leader was alive at that instant -- if it exits before os.killpg runs, the whole process group is already gone and killpg raises ProcessLookupError. The exception handler swallowed that with a bare `pass`, never calling wait() on a leader that is a genuine zombie at that point (exited, but not yet reaped by this parent), leaving it unreaped until the wrapper process itself exits. Fix: the ProcessLookupError handler now calls service.process.wait(timeout=10) to reap it, matching Devin's suggested fix exactly. Added a regression test simulating the race (killpg always raises ProcessLookupError against a still-poll()-alive process) and asserting wait() is called; verified it fails against the pre-fix bare `pass` and passes against the fix. Full suite: 1998 passed, 1 skipped, 21 subtests passed; sandboxed_web_e2e.py at 100% statement/branch coverage and 100% docstrings.
|
"Cleanup race leaves service zombies" — confirmed and fixed in
Fix: the Added a regression test simulating the race ( Full suite: _Generated by Claude Code Generated by Claude Code |
|
Generated by Claude Code |
| except ProcessLookupError: | ||
| return | ||
| service.process.wait(timeout=10) | ||
|
|
||
|
|
||
| def tail_text(path: Path, max_lines: int = 80) -> str: | ||
| """Return the final lines of a service log.""" | ||
| service.process.wait(timeout=10) |
There was a problem hiding this comment.
📝 Info: Leader-exit race remains bounded
When killpg loses the exit race, wait(timeout=10) reaps the leader. Its timeout still enters the bounded fallback cleanup.
Was this helpful? React with 👍 or 👎 to provide feedback.
* fix(security): require OS isolation for web e2e commands * fix(security): harden loopback and document isolation * fix(security): mount isolated root filesystem * docs(security): document isolated web verification * fix(e2e): cover and harden isolated command paths * docs: make sandbox changelog actionable * fix(e2e): fail closed on rejected sandbox commands * fix(e2e): validate readiness before launch * fix(e2e): back off after server readiness errors * fix(e2e): validate readiness ports and fail closed on isolation gaps Addresses three live Devin Review findings on PR #1347: - require_loopback_readiness_url never read the parsed port, so a nonnumeric or out-of-range port (e.g. "http://127.0.0.1:abc/health") reached urllib.request.urlopen and raised an uncaught http.client.InvalidURL instead of the documented exit code 125. Now validated up front and converted to ValueError. - isolated_command silently skipped the read-only-root validation whenever shutil.which could not resolve the executable, letting an unvalidated command through. It now fails closed the same way an out-of-bind-root executable already does. - isolation_backend accepted any bwrap binary discovered on PATH without proving it can actually create the namespaces bubblewrap needs. A bounded capability preflight (_probe_isolation_capability) now exercises the same essential namespace/mount operations isolated_command depends on and classifies a denied host as unavailable isolation (exit 126) instead of a confusing later failure. The "workspace symlinks escape filesystem isolation" finding was investigated and found not reproducible: isolated_command's --tmpfs / root replacement means any bind not explicitly listed (read-only roots, /etc identity files, /workspace) simply does not exist inside the sandbox, so a symlink to an unbound host path dangles (ENOENT) rather than resolving. Verified empirically with a real bwrap build against the actual copy_workspace/isolated_command code path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * Merge remote-tracking branch 'origin/fix/sandboxed-web-e2e-isolation-clean' into fix/sandboxed-web-e2e-isolation-clean # Conflicts: # docs/doctoring/sandboxed-web-command-isolation.md # scripts/ci/sandboxed_web_e2e.py # tests/test_sandboxed_web_e2e.py * fix(sandboxed-verify): use strict resolve() for symlink escape check path.resolve() with the default strict=False does not reliably raise for a symlink cycle across Python versions — some CPython releases detect the cycle and silently return a partially-resolved path instead of raising RuntimeError/OSError. This let a genuine a->b->a symlink cycle slip past _reject_escaping_symlinks undetected on CI's Python, failing test_copy_workspace_rejects_unresolvable_symlink_cycle with "DID NOT RAISE ValueError". resolve(strict=True) requires the fully-resolved path to actually exist, so both a cycle and a plain dangling target reliably raise OSError (ELOOP/ENOENT), which the existing except clause already converts to the intended ValueError. * fix(e2e): mirror full probe operations, exclude credential paths from sandbox copy Addresses the two remaining live Devin Review findings on PR #1347: - _probe_isolation_capability now mirrors every operation isolated_command actually performs (--new-session, /tmp tmpfs, and a writable bind+chdir into the same mount point real commands use) against a real throwaway temp directory, instead of a reduced probe that could pass on a host denying one of those specific operations and only fail later once a real service starts. - copy_workspace's default ignore list now excludes common credential-bearing dotfiles/dirs (.env*, .netrc, .npmrc, .pypirc, .pgpass, .git-credentials, .ssh, .gnupg, .aws, .kube, .docker) so a repo checkout that happens to carry one of these never rides into the sandboxed command's writable, readable /workspace mount. Logs and per-command scrubbed homes stay in that same mount deliberately (the tested command needs to write them) -- this narrows what's copied in, it does not split the mount by service. Verification: full suite 1930 passed/1 skipped/21 subtests, coverage 100% (sandboxed_verify.py 120/120, sandboxed_web_e2e.py 282/282), interrogate 100%, ruff clean. * fix(sandboxed-verify): use lexical symlink walk, not resolve(), to reject escapes Devin flagged that _reject_escaping_symlinks's Path.resolve(strict=True) rejected any unresolvable symlink uniformly, including a plain dangling target whose own file was legitimately excluded from the copy by DEFAULT_IGNORE/extra_ignores (or is simply broken) -- aborting an otherwise-valid verification run over a link that was never actually a host-escape attempt. Replaced the resolve()-based check with a hop-by-hop lexical walk (os.readlink + os.path.normpath, tracked via a visited set), matching the existing design in the parallel PR #1280's sandboxed_verify.py. This never requires a target to exist, so a dangling-but-contained symlink is now accepted while an actual escape (absolute target, or a normalized target outside the sandbox root) or an unresolvable cycle (revisiting an already-followed path) still raises the same ValueError as before. The hop count is bounded so a chain that never repeats due to purely lexical normalization still fails closed instead of walking forever. Added regression tests for: a dangling target with no cycle, a target excluded from the copy by DEFAULT_IGNORE, and a chain exceeding the hop limit without ever cycling or escaping. * fix(sandboxed-verify): fix off-by-one in symlink hop-limit walk Devin flagged that a valid chain of exactly MAXIMUM_SYMLINK_HOPS (40) real, OS-resolvable symlinks was incorrectly rejected: each loop iteration checks one position and, if it is a symlink, advances to the next -- so resolving N real hops needs N+1 checks (N to walk them, one more to confirm the final landing position is not itself a further symlink). range(MAXIMUM_SYMLINK_HOPS) only provided N checks, so the walk always fell through to "could not be resolved" one check short of reaching a real target on an exactly-N-hop chain. Reproduced directly: a 40-real-symlink chain terminating in a real file raised before the fix and copies cleanly after. Fix: range(MAXIMUM_SYMLINK_HOPS + 1). Verified this doesn't change any existing test's outcome, including the past-the-limit and cycle-detection tests (a cycle is still caught well within the budget regardless of the +1; a chain intentionally longer than the limit still exceeds the new budget too). Added a boundary regression test for exactly-N-hops success to lock this in. Full suite: 1934 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings. * fix(sandboxed-verify): resolve symlink targets component-by-component Devin flagged "nested directory links escape sandbox" on the parallel PR #1280, which shares this file's hop-walk design: a symlink target that itself contains an intermediate component which is a symlink (e.g. "some-alias/../secret") defeats a check that collapses the whole target string in one os.path.normpath call, because normpath cancels "some-alias" against the following ".." purely textually without ever re-examining whether some-alias is itself a symlink needing its own resolution first. Reproduced directly against this file (not just #1280's): a self-alias symlink pointing at "." (its own parent, the repo root -- entirely legitimate and safe standing on its own) combined with a second symlink whose target is "self-alias/../outside-secret.txt" was NOT caught -- the whole string collapsed lexically to "outside-secret.txt" (looking safe), while resolving it for real, one component at a time, correctly shows that following self-alias lands at the repo root itself (zero depth), so the very next ".." immediately exits it. Fix: replaced the whole-target os.path.normpath collapse with a component-by-component walk that re-checks is_symlink() after every single path segment, substituting a symlink's own target components back onto the work queue instead of treating the whole original target string as one atomic lexical unit. This also fixes the hop-limit off-by-one from fe237c4 as a natural consequence: a hop's budget is now spent only when a symlink is actually dereferenced, not once per loop iteration, so a chain of exactly MAXIMUM_SYMLINK_HOPS real symlinks needs no special-casing. Verified against all existing symlink tests unchanged (escape, absolute, internal, dangling, excluded-by-DEFAULT_IGNORE, cycle, hop-limit boundary) plus two new ones: the nested-alias escape (must now raise) and a legitimate cross-directory ".." traversal that stays in-bounds (must still be accepted, for branch coverage on the successful ".." path). Full suite: 1969 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings. * fix(sandboxed-ci): repo launchers, env templates, shell shadowing, blank commands Devin flagged five findings on this PR; four are confirmed real bugs, fixed here with fail-before/pass-after regression tests. The fifth (readiness probes sharing the runner's loopback network namespace) is a real gap but has no small, non-regressing fix available -- posted as a separate PR comment for maintainer review instead of a speculative code change. 1. sandboxed_web_e2e.isolated_command rejected valid repository launchers (e.g. ./gradlew): shutil.which resolves any path-separator-bearing command against the *wrapper process's* own cwd, never against the copied repository's cwd the caller actually passes in. Added _resolve_isolated_executable, which resolves an explicit-path argv[0] against the sandboxed cwd instead, and widened the bind-root check to also permit executables inside sandbox_root (mounted at /workspace), while still fail-closed rejecting path traversal and external paths. 2. sandboxed_verify's DEFAULT_IGNORE ".env.*" glob excluded committed, secret-free templates (.env.example, .env.sample, .env.template) right along with real dotenv credential files. Added DEFAULT_ENV_TEMPLATE_ALLOWLIST and _ignore_with_env_template_allowlist, which wraps shutil.ignore_patterns to spare those specific names. 3. _probe_isolation_capability resolved its probe shell via shutil.which("sh") against the caller's own PATH, which can return a binary outside every root isolated_command actually bind-mounts (e.g. a PATH entry shadowing sh with a home-directory executable). That shell is invisible inside the sandbox, so a real working bubblewrap install fails the probe. Added _probe_shell(), which only picks from PROBE_SHELL_PATHS (/bin/sh, /usr/bin/sh) -- the same mounted roots isolated_command uses -- and fails clearly if neither exists. 4. A whitespace-only backend/frontend/e2e command made isolated_command raise ValueError("command must not be empty"), which the call site's except clause (catching only RuntimeError) let propagate as an uncaught traceback instead of the documented isolation-rejection exit code 126. The except clause now also catches ValueError. Validation: PYTHONPATH=. python3 -m pytest tests -q -> 1942 passed, 1 skipped, 21 subtests passed. coverage on scripts/ci/sandboxed_verify.py and scripts/ci/sandboxed_web_e2e.py -> 100% statement+branch. Full-repo coverage and interrogate docstring coverage both 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(sandboxed-verify): track cycles with a recursive active-set, not seen-forever Devin flagged "Valid symlink paths are rejected": a symlink referenced twice in one chain -- once fully resolved before the second reference is ever reached, not a real loop -- was incorrectly treated as a cycle, because the walk's `seen` set recorded every symlink ever dereferenced for the whole top-level candidate and never removed one once its resolution completed. Reproduced directly: `shared -> real_dir`, `link -> "shared/../shared/file.txt"` (a path the OS resolves without issue, referencing `shared` twice non-recursively) raised "workspace symlink could not be resolved" before this fix. Fix: restructured the walk from an iterative work-queue with a permanent `seen` set into a genuinely recursive component resolver with an `active` set -- a symlink is added to `active` only while its own target is being resolved (a fresh recursive call) and removed again as soon as that call returns successfully. A cycle is then precisely "a symlink that, directly or through others, points back to itself while still being resolved", which is what `step in active` now tests, rather than "was ever dereferenced anywhere in this chain". The true self-loop test (`a -> b -> a`) still raises, since `a` is still on the active call stack when it is encountered again. Verified: all existing symlink tests pass unchanged (escape, absolute, internal, dangling, excluded-by-DEFAULT_IGNORE, cycle, hop-limit boundary), plus a new regression test for the shared-non-cyclic-reference case. Full suite: 1978 passed, 1 skipped, 21 subtests passed; sandboxed_verify.py at 100% statement/branch coverage and 100% docstrings. * fix(sandboxed): report a clean coded failure when copy_workspace rejects Devin flagged (thread PRRT_kwDOS_C14s6dh6My) that sandboxed_verify.py's main() calls copy_workspace() with no except around it: a symlink-escape rejection propagated as an uncaught ValueError, printing a raw Python traceback and exiting with Python's default uncaught-exception status instead of this module's own clean "sandboxed-verify: ..." message and coded exit (e.g. 124 for the timeout path). --keep-sandbox still retains the rejected copy either way, which matches its documented "for debugging" purpose -- not a bug to fix here. sandboxed_web_e2e.py calls the same copy_workspace() with the identical gap, found independently while fixing the sibling script; fixed the same way (exit 125, matching the code already used for its other ValueError rejections like an invalid readiness URL). Added a regression test per script asserting a clean exit 125, no traceback, and a still-emitted result payload. * fix(sandboxed-ci): honor explicit --ignore over env-template allowlist; resolve bare commands via relative PATH entries Devin review findings on PR #1347: - sandboxed_verify.py: _ignore_with_env_template_allowlist merged DEFAULT_IGNORE and the caller's extra_ignores into one combined pattern set before restoring DEFAULT_ENV_TEMPLATE_ALLOWLIST names, so an explicit --ignore .env.example (or any caller-supplied extra_ignores entry) was silently overridden and the file still landed in the writable sandbox. Now builds two separate ignore functions -- one from DEFAULT_IGNORE alone (whose broad .env.* glob the allowlist exists to except from) and one from extra_ignores alone (never overridden) -- and only restores a name matched solely by the former. - sandboxed_web_e2e.py: _resolve_isolated_executable resolved a bare PATH-searched command purely via shutil.which(), which always resolves a relative PATH entry against the wrapper process's own cwd with no way to override that. A PATH with a relative entry meant to be read relative to the copied repository (e.g. PATH=bin:/usr/bin) therefore failed isolation (exit 126) even when the tool legitimately existed under the sandboxed cwd. Falls back to a new _which_relative_to_cwd that mirrors shutil.which's PATH-splitting/executable-bit checks by hand, anchoring relative entries at cwd; a relative entry that would resolve outside sandbox_root is skipped without touching the real filesystem, keeping the existing fail-closed behavior for a traversal PATH like ../../... Regression tests added for both, verified fail-before/pass-after against a temporary revert of each fix. Full suite: 1982 passed, 1 skipped, 21 subtests passed. scripts/ci/sandboxed_verify.py and scripts/ci/sandboxed_web_e2e.py: 100% statement+branch coverage, 100% docstring coverage (interrogate). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX * fix(sandboxed-web-e2e): close proxy bypass, port-occupancy, malformed-command, and workspace-path gaps Four Devin findings on this PR's bubblewrap sandboxing: - wait_for_url's opener now passes an explicit ProxyHandler({}) alongside NoRedirectHandler, so HTTP_PROXY/HTTPS_PROXY/*_proxy environment variables can never route a "loopback-only, isolated" readiness probe through an external proxy (mirrors the fix already applied to a different opener in materialize_base_python_requirements.py). - New require_unoccupied_readiness_port rejects a readiness URL whose port already answers before this run starts its own service, called once in main() right before start_service. isolated_command does not create a network namespace for the commands it wraps (the host readiness poller and the E2E command both need to reach the same loopback ports), so this closes the "polls some other, unrelated runner service" gap without breaking that shared-loopback design. - parse_args now shell-tokenizes all three of --backend-cmd/--frontend-cmd/ --e2e-cmd up front, independent of --isolation, and rejects a blank or unmatched-quote command through argparse's own clean SystemExit(2) path. Previously, with isolation disabled, such a command bypassed isolated_command entirely and crashed with an uncaught ValueError deep inside start_service/run_shell's own shlex.split call. - isolated_command now rewrites an absolute executable path that resolves inside the sandbox copy to its /workspace-relative form, and _sandbox_environment now does the same for PATH entries rooted under the sandbox copy -- bubblewrap binds the copy at /workspace, not at its original host path, so an absolute copied-repo launcher or PATH entry previously failed to launch inside the sandbox unchanged. Also folds in a CodeRabbit finding on the same head, in sandboxed_verify.py: _reject_escaping_symlinks walked the unresolved destination path but checked each symlink against the resolved root, so a sandbox root reached through a symlinked ancestor (e.g. a symlinked default temp directory) made path.relative_to(root) raise for every symlink in an otherwise-legitimate copy. Now walks from the already-resolved root instead; escape detection itself is unchanged and still covered. Plus two quick-win CodeRabbit items: documents the DEFAULT_ENV_TEMPLATE_ALLOWLIST carve-out in the command-isolation doc, and fixes four tests that mocked shutil.which for _probe_isolation_capability's shell selection after it was changed to check fixed PROBE_SHELL_PATHS directly instead -- those mocks were silent no-ops relying on whatever shell the host happened to have mounted. All new behavior is covered by new regression tests reproducing each bug against pre-fix code; scripts/ci stays at 100% line+branch coverage and 100% docstring coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --------- Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Claude Code <seongho.iopsy@gmail.com>
Attempted merge, aborted — genuine architectural divergence in
|
충돌 해결 시도 결과 — 14블록은 판정 완료,
|
| 블록 | 판정 |
|---|---|
| 1 | theirs만 — ours의 MAXIMUM_SYMLINK_HOPS = 40은 이미 충돌 밖 102행에 정의돼 있어 중복이 됩니다 |
| 2·3·4·5·7 | ours (브랜치의 개명·타입 예외 체계) |
| 6 | 합집합 — main의 _ignore_with_env_template_allowlist + 브랜치의 validate_repository_symlinks |
| 8 | ours 두 핸들러 + theirs의 except ValueError 를 뒤에 (두 예외가 ValueError 하위 클래스라 순서가 중요하고, RepositoryRootError 핸들러에는 자체 return exit_code가 필요합니다 — 공유되던 return이 theirs 쪽으로 붙기 때문) |
적용 후 중복 정의 0, _reject_escaping_symlinks 잔존 0, 구문 OK를 확인했습니다.
scripts/ci/sandboxed_web_e2e.py — 5블록은 판정, 1블록은 막힙니다
블록 1·3·5는 합집합, 블록 4(run_shell)는 ours입니다.
블록 2도 풀립니다. 겉보기엔 최악(ours 74줄 3함수 vs theirs 341줄 10함수, 양쪽이 start_service 정의)이지만, main의 start_service는 merge-base 대비 shell=False 한 줄만 다르고 브랜치 판본에는 이미 그 줄이 있습니다. 그래서 theirs의 신규 10함수 + ours의 3함수, main의 start_service는 버리면 무손실입니다.
막히는 지점 — 블록 6 (main() 본체, ours 115줄 / theirs 101줄)
main이 이 브랜치가 갈라진 뒤 E2E 러너에 OS 수준 격리 백엔드 전체를 새로 지었습니다. merge-base에는 하나도 없던 것들입니다:
| 심볼 | merge-base | 브랜치 판본 | main 판본 |
|---|---|---|---|
isolation_backend |
0 | 0 | 5 |
isolated_command |
0 | 0 | 11 |
_sandbox_environment |
0 | 0 | 2 |
require_unoccupied_readiness_port |
0 | 0 | 3 |
require_loopback_readiness_url |
— | 2 | 7 |
bounded_subprocess |
0 | 31 | 0 |
브랜치의 main()은 isolation_backend(args.isolation) 실패 시 exit 126, _sandbox_environment(env, sandbox), 백엔드/프론트엔드/E2E 명령을 각각 isolated_command(...)로 감싸기, readiness URL의 SSRF 검사 — 이 전부를 모릅니다. 브랜치 쪽을 그대로 취하면 이것들이 사라집니다.
반대로 main 쪽을 취하면 이 PR의 바운디드 출력 기능(_services_output_limited, OUTPUT_LIMIT_EXIT_CODE, 타입 예외 핸들러 6개)이 사라집니다.
합집합이 기계적으로 나오지 않는 이유: 두 제어 흐름이 같은 함수 안에서 얽혀 있습니다. 격리 백엔드는 명령 문자열을 isolated_command로 감싼 뒤 start_service/run_shell에 넘기는데, 이 PR은 그 두 함수의 시그니처와 반환 타입을 바꿉니다. 올바른 결과는 "바운디드 프리미티브를 격리 백엔드 를 통과해 실행"이고, 그건 병합이 아니라 설계입니다. 게다가 브랜치 쪽에는 main의 격리 기능을 덮는 테스트가 없어서, 제가 만든 합성본이 격리를 조용히 약화시켜도 초록으로 보일 수 있습니다. 샌드박스 격리는 그렇게 다룰 경계가 아닙니다.
제안
main()을 이 두 축으로 재작성하는 것이 이 PR의 남은 작업입니다:
isolation_backend/_sandbox_environment/isolated_command/ readiness SSRF 검사(main 흐름)를 먼저 세우고- 그 결과 명령을 이 PR의
start_service(..., service_log_limit_bytes)와run_shell(..., output_limit_bytes)로 실행한 뒤 _services_output_limited와OutputLimitUnsupportedError/RepositoryPathBoundaryError등 타입 예외 핸들러를 붙이는
순서입니다. 위 14블록 판정은 그대로 쓰시면 되고, 재작업은 블록 6 하나입니다.
수용 오라클
python3 -m pytest -q tests/test_bounded_subprocess.py tests/test_sandboxed_verify.py tests/test_sandboxed_verify_output_limits.py tests/test_sandboxed_verify_symlink_boundary.py tests/test_sandboxed_web_e2e.py tests/test_sandboxed_web_e2e_output_limits.py tests/test_sandboxed_web_e2e_branch_contract.py여기에 main의 격리 계약(isolation_backend / isolated_command를 덮는 tests/test_sandboxed_web_e2e.py의 기존 테스트)이 함께 초록이어야 합니다.
🤖 Generated with Claude Code
Resolves the 5-file/22-block conflict between this PR's bounded subprocess/output-limit hardening and origin/main's independently developed dotenv-template allowlist (sandboxed_verify.py) and OS-level bubblewrap isolation backend for the web E2E runner (sandboxed_web_e2e.py). scripts/ci/sandboxed_verify.py (8 blocks): union of both sides' orthogonal hardening. Kept this PR's renamed/typed symlink validation (validate_repository_symlinks, RepositoryPathBoundaryError, RepositoryRootError, PATH_BOUNDARY_EXIT_CODE) throughout, added main's committed dotenv-template allowlist (DEFAULT_ENV_TEMPLATE_ALLOWLIST, _ignore_with_env_template_allowlist) as an independent function, and kept both typed exception handlers in main() with main's generic `except ValueError` appended after them -- then removed that generic handler once coverage proved it fully unreachable (copy_workspace can now only ever raise the two typed subclasses). scripts/ci/sandboxed_web_e2e.py (6 blocks): 3 straightforward unions (module constants, wait_for_url docstring, emit_result payload fields), run_shell resolved to this PR's bounded version, start_service resolved to this PR's bounded version plus main's 10 new isolation helper functions (main's own start_service differed from merge-base by exactly `shell=False`, already present on this PR's side, so it was dropped as a pure duplicate). main() was redesigned, not spliced: isolation_backend/_sandbox_environment/isolated_command/the readiness SSRF checks now run first exactly as main's flow did, the resulting (possibly isolation-wrapped) commands are executed through this PR's bounded start_service(..., service_log_limit_bytes=...) and run_shell(..., output_limit_bytes=...), and this PR's typed exception handling (CommandExecutableNotFoundError, CommandNotExecutableError, OutputLimitUnsupportedError, RepositoryPathBoundaryError, RepositoryRootError) still wraps the whole flow, with isolation's own RuntimeError/ValueError rejections caught locally first so they keep main's exact 126/125 exit codes and messages instead of being swallowed by this PR's generic (OSError, RuntimeError) catch-all. Also removed sandboxed_web_e2e.py's now fully-redundant _require_parseable_command (main's blank/malformed-command validation): this PR's own earlier parse_args loop already rejects the same inputs first, so main's check was dead code once merged -- confirmed via coverage before removing it, and the 6 tests that asserted its messages were updated to the (already-covered, equally strict) message text that actually fires. Test files (test_sandboxed_verify.py, test_sandboxed_web_e2e.py): message strings updated to match the merged typed-exception contract; two message-duplicated main tests dropped in favor of this PR's existing parametrized equivalents; mock signatures in test_sandboxed_web_e2e.py, test_sandboxed_web_e2e_output_limits.py, test_sandboxed_web_e2e_branch_contract.py, and test_sandboxed_entrypoint_and_cleanup_coverage.py updated for the merged start_service/run_shell signatures, and `--isolation disabled` added to tests that exercise this PR's bounded-output/typed-exception behavior via fakes or real subprocesses unrelated to main's isolation feature (which is separately covered by its own now-mocked tests). Verification: full tests/ suite (3003 passed, 1 skipped), 100% branch coverage on scripts/ci/, 100% docstring coverage (interrogate), ruff --select F821 clean, py_compile clean, no duplicate definitions or leftover references to renamed symbols. Isolation invariant verified by deliberately reverting the isolation wiring twice (backend/frontend start_service commands, then separately the e2e run_shell command) and confirming main()'s own isolation test fails both times -- the second attempt exposed a real gap (the existing test only proved isolated_command was *called* for the E2E command, not that its output reached run_shell), so test_main_runs_required_isolation_with_mapped_environment was strengthened to assert on run_shell's actual received command and env before restoring the correct implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
병합 완료 —
|
…ndary_fails_closed
Every other test in this file that needs to reach start_service directly
passes --isolation disabled. This one didn't, so on a runner without
bubblewrap installed (the actual GitHub-hosted ubuntu-latest runner for
this workflow), main()'s isolation_backend("required") call raises
RuntimeError and returns exit code 126 before the mocked start_service
(and its OutputLimitUnsupportedError) is ever reached, failing the
assertion against OUTPUT_LIMIT_EXIT_CODE (123).
Reproduced locally by mocking shutil.which("bwrap") -> None to simulate
the CI runner: confirmed 126 == 123 failure pre-fix, confirmed pass
post-fix, and confirmed the full suite (3003 passed, 1 skipped) is clean
with the fix applied.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Fixed
|
|
2 participants
Outcome
First clean current-main layer replacing the monolithic #931.
This layer intentionally does not change sandboxed_verify, sandboxed_web_e2e, result schemas, workspace symlink policy, or the pip-audit lock. Those consumers remain separate stack layers.
Evidence
Part 1 of the clean replacement for #931.
Summary by CodeRabbit
새로운 기능
버그 수정
문서