fix(security): contain sandbox paths and output - #931
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthrough샌드박스 명령과 웹 E2E 서비스에 출력·로그 바이트 제한을 추가했습니다. 초과 시 프로세스 그룹을 종료하고 종료 코드 Changes샌드박스 자원 제한
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
|
@coderabbitai review Please review the unchanged exact source head |
|
I will report semantic source findings separately from infrastructure and policy readiness. This review does not approve the pull request.
|
seonghobae
changed the title
|
@coderabbitai review Please review exact source head |
|
I will treat only exact-head evidence as authoritative. I will keep semantic source findings separate from CI, security, supply-chain, and policy readiness. This review does not approve the draft pull request or infer independent human approval. ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
scripts/ci/bounded_subprocess.py (1)
109-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value도달 불가능한 분기가 100% 분기 커버리지 요구와 충돌할 수 있습니다.
validate_output_limit는 최소 한도를MINIMUM_OUTPUT_LIMIT_BYTES(4,096)로 강제합니다.TRUNCATION_MARKER는 그보다 훨씬 짧습니다. 따라서suffix_budget은 검증된 한도에서 항상 양수입니다.if suffix_budget else b""의 거짓 분기는 정상 경로에서 실행되지 않습니다.
scripts/ci/코드에 100% 분기 커버리지를 유지해야 하므로, 이 분기를 직접 호출하는 단위 테스트를 추가하거나 분기를 제거하십시오.As per coding guidelines: "Maintain 100% test coverage and 100% interrogate docstring coverage for code under
scripts/ci/".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/bounded_subprocess.py` around lines 109 - 117, The suffix_budget zero branch in _render_bounded_bytes is unreachable after validate_output_limit enforces MINIMUM_OUTPUT_LIMIT_BYTES. Remove the conditional fallback and construct the suffix using the guaranteed-positive budget, preserving the marker-plus-suffix truncation behavior and 100% branch coverage.Source: Coding guidelines
scripts/ci/sandboxed_verify.py (3)
288-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value타입이 확정된 결과에는
getattr기본값이 필요하지 않습니다.
completed는bounded_subprocess.BoundedCompletedProcess입니다. 이 dataclass는output_limited: bool필드를 항상 가집니다.getattr(completed, "output_limited", False)의 기본값은 실행되지 않으며, 나중에 계약이 깨져도 조용히False로 처리합니다.312행의
getattr은 다릅니다. 해당except는 기반 클래스subprocess.TimeoutExpired를 잡고, 그 클래스에는output_limited가 없습니다. 그곳의 기본값은 유지하십시오.♻️ 제안 리팩터
- output_limited = bool(getattr(completed, "output_limited", False)) + output_limited = completed.output_limited🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/sandboxed_verify.py` at line 288, Update the assignment in the completed-process handling path to access the guaranteed output_limited field directly on completed instead of using getattr with a fallback. Leave the getattr usage in the TimeoutExpired exception handler unchanged, since that handler receives a base exception without this field.
159-188: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win구현은 정확합니다. 실행 시점에 생성되는 링크에 대한 한계를 문서화하십시오.
경계 검사 자체는 견고합니다.
os.walk(..., followlinks=False)는 링크된 디렉터리로 내려가지 않으므로candidate.parent의 모든 구성 요소는source_root아래의 실제 디렉터리입니다. 따라서(candidate.parent / target).resolve(strict=False)는 신뢰할 수 있는 기준점에서 시작합니다. 링크 체인도resolve가 중간 링크를 따라가므로 각 링크가 개별적으로 차단됩니다. 절대 링크를 거부하는 결정도 복사본이 원본 체크아웃을 다시 가리키는 경우를 막습니다.한 가지 경계 조건이 남습니다. 이 검증은 명령 실행 전에 한 번만 실행됩니다. 신뢰할 수 없는 검증 명령은 실행 중에 복사본 안에 탈출 심볼릭 링크를 새로 만들고 그것을 따라갈 수 있습니다. 이는 사전 검사로는 막을 수 없는 고유한 한계입니다.
docs/doctoring/sandboxed-verification-symlink-boundary.md에 이 한계를 명시하면 이후 검토자가 이 통제를 실행 시점 봉쇄로 오해하지 않습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/sandboxed_verify.py` around lines 159 - 188, Document the runtime symlink limitation in docs/doctoring/sandboxed-verification-symlink-boundary.md: explain that validate_repository_symlinks performs only a pre-execution scan, so an untrusted verification command may create and follow an escaping symlink during execution; clarify that this check is not runtime containment.
197-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win심볼릭 링크 거부가 처리되지 않은 예외로 전파됩니다.
validate_repository_symlinks는ValueError를 발생시킵니다.main()의try블록은 이 예외를 처리하지 않습니다.finally의emit_result는 실행되지만exit_code는 초기값 1로 남고, 그다음 예외가main()밖으로 전파됩니다. 호출자는 명확한 종료 코드 대신 traceback을 받습니다.동작은 fail-closed이므로 보안 결함은 아닙니다. 다만 이 PR이 출력 제한에 대해
123을 도입한 방식과 일관되게, 경로 경계 거부에도 명시적 메시지와 안정적인 종료 코드를 부여하십시오. 그러면 방출되는 증거의exit_code가 일반 실패와 경계 거부를 구분합니다.♻️ 제안 리팩터
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 error: + print(f"sandboxed-verify: {error}", file=sys.stderr) + exit_code = PATH_BOUNDARY_EXIT_CODE + return exit_code env = scrubbed_env(sandbox, args.allow_env)
PATH_BOUNDARY_EXIT_CODE는123,124,125와 충돌하지 않는 값으로 정의하고, 해당 값을docs/doctoring/sandboxed-verification-symlink-boundary.md와CHANGELOG.md에 기록하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/sandboxed_verify.py` around lines 197 - 198, Handle the ValueError raised by validate_repository_symlinks within main() so it does not escape after emit_result; emit an explicit path-boundary rejection message and set a dedicated stable PATH_BOUNDARY_EXIT_CODE distinct from 123, 124, and 125. Ensure the emitted evidence records that code while preserving the existing general-failure behavior, and document the new code in the specified symlink-boundary guide and CHANGELOG.md.tests/test_bounded_subprocess_contract.py (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value전역
os모듈에서killpg를 삭제하면 같은 프로세스의 다른 테스트에 영향을 줄 수 있습니다.
bounded.os는 표준 라이브러리os모듈 자체입니다.monkeypatch.delattr(bounded.os, "killpg")는 테스트 동안os.killpg를 프로세스 전역에서 제거합니다.bounded_subprocess의 drain 스레드는 데몬 스레드이고 오버플로 시kill_process_group을 통해os.killpg를 호출합니다. 이전 테스트에서 남은 데몬 스레드가 이 구간에서 실행되면AttributeError가 발생합니다.
hasattr검사를 모듈 수준 헬퍼로 감싸고 그 헬퍼를 패치하면 전역 변경 없이 같은 fail-closed 경로를 검증할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_bounded_subprocess_contract.py` around lines 26 - 32, Update test_supported_platform_requires_posix_killpg and the platform-support check to use a module-level helper that reports whether os.killpg is available, then monkeypatch that helper to return false instead of deleting killpg from the global os module. Preserve the expected OutputLimitUnsupportedError fail-closed behavior.tests/test_sandboxed_web_e2e_branch_contract.py (1)
112-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win정확한 경계값 4096에 대한 어서션을 추가하십시오.
현재 테스트는 4바이트와 4097바이트만 검증합니다.
service_output_limited는st_size > log_limit_bytes를 사용합니다. 비교 연산자가>=로 바뀌어도 이 테스트는 통과합니다. 정확히 한도와 같은 크기를 추가하면 off-by-one 회귀를 잡습니다.♻️ 제안 보강
service.log_path.write_bytes(b"safe") assert not sandboxed_web_e2e.service_output_limited(service) + service.log_path.write_bytes(b"x" * 4096) + assert not sandboxed_web_e2e.service_output_limited(service) service.log_path.write_bytes(b"x" * 4097) assert sandboxed_web_e2e.service_output_limited(service)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sandboxed_web_e2e_branch_contract.py` around lines 112 - 122, Extend test_service_limit_fallback_handles_missing_small_and_large_files to write exactly 4096 bytes and assert service_output_limited(service) is false, while preserving the existing checks for smaller and larger files.scripts/ci/sandboxed_web_e2e.py (2)
253-265: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
max_lines검증을 파일 존재 검사보다 먼저 수행하십시오.현재
path.exists()가 False이면max_lines검증을 건너뜁니다. 그 결과tail_text(missing_path, max_lines=0)은 예외 없이 빈 문자열을 반환합니다. 같은 잘못된 입력이 파일 존재 여부에 따라 다르게 처리됩니다. 인자 검증을 함수 시작부로 옮기면 계약이 일관됩니다.♻️ 제안 리팩터
def tail_text( path: Path, max_lines: int = 80, max_bytes: int = DEFAULT_TAIL_BYTES, ) -> str: """Return final lines after a byte-bounded service evidence read.""" - if not path.exists(): - return "" if max_lines <= 0: raise ValueError("max_lines must be positive") + if not path.exists(): + return "" bounded_text = bounded_subprocess.read_bounded_suffix(path, max_bytes)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/sandboxed_web_e2e.py` around lines 253 - 265, Move the max_lines validation in tail_text before the path.exists() check so non-positive values always raise ValueError, including for missing paths. Preserve the existing empty-string result for nonexistent paths with valid arguments.
404-415: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
stop_service실패 후 프로세스 그룹을 강제 종료하십시오.
stop_service가 예외를 던지면 루프는 해당 서비스를 건너뛰고 다음 서비스로 진행합니다. 이때 해당 서비스의 자식 프로세스 그룹은 종료되지 않은 상태로 남을 수 있습니다. 라인 441에서 sandbox 디렉터리는 삭제되지만 프로세스는 계속 실행됩니다. CI 러너에 고아 프로세스가 누적됩니다.예외 경로에서
bounded_subprocess.kill_process_group을 최선 노력으로 한 번 더 호출하면 이 누수가 닫힙니다.♻️ 제안 보강
for service in reversed(services): try: stop_service(service) except (OSError, RuntimeError, subprocess.SubprocessError): + with contextlib.suppress(OSError, subprocess.SubprocessError): + bounded_subprocess.kill_process_group(service.process) output_limited = True if exit_code != 124: exit_code = bounded_subprocess.OUTPUT_LIMIT_EXIT_CODE print( "sandboxed-web-e2e: bounded service capture failed", file=sys.stderr, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/sandboxed_web_e2e.py` around lines 404 - 415, Update the exception path in the finally cleanup loop around stop_service to make a best-effort bounded_subprocess.kill_process_group call for the failed service before continuing to the next service. Preserve the existing exception handling and exit-code behavior, and ensure cleanup errors from the fallback kill do not interrupt cleanup of remaining services.tests/test_sandboxed_verify_output_limits.py (1)
161-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value두 CLI 예산 거부 테스트가 for 루프로 여러 케이스를 검증합니다. 공통 근본 원인은 파라미터화 대신 루프를 사용한 점입니다. 루프에서는 첫 케이스가 실패하면 나머지 케이스가 실행되지 않고, 실패 보고에 어느 값이 문제인지 나타나지 않습니다.
tests/test_sandboxed_verify_output_limits.py#L161-L179:for value in [...]루프를@pytest.mark.parametrize("value", ["4095", str(bounded.MAXIMUM_OUTPUT_LIMIT_BYTES + 1)])로 바꾸고 본문에서 단일 값만 검증하십시오.tests/test_sandboxed_web_e2e_output_limits.py#L241-L264:for option, value in [...]루프를@pytest.mark.parametrize("option, value", [...])로 바꾸고 본문에서 단일(option, value)쌍만 검증하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sandboxed_verify_output_limits.py` around lines 161 - 179, Replace the loop in tests/test_sandboxed_verify_output_limits.py:161-179 with `@pytest.mark.parametrize`("value", ["4095", str(bounded.MAXIMUM_OUTPUT_LIMIT_BYTES + 1)]) and have the test validate one value per invocation. Also replace the loop in tests/test_sandboxed_web_e2e_output_limits.py:241-264 with `@pytest.mark.parametrize`("option, value", [...]) so each option/value pair runs as a separate test case with clear failure attribution.tests/test_sandboxed_web_e2e_output_limits.py (2)
42-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
4096한도와4097상한 검사가 두 테스트에 리터럴로 중복됩니다. 공통 근본 원인은한도 + 1바이트계약이 이름 없는 리터럴로 표현된 점입니다. 한도를 변경하면 각 테스트에서 두 개의 리터럴을 함께 수정해야 하고, 한쪽만 수정하면 어서션이 조용히 느슨해집니다.
tests/test_sandboxed_web_e2e_output_limits.py#L42-L65: 지역 변수log_limit_bytes = 4096을 도입하고 라인 58의 인자와 라인 62의<= log_limit_bytes + 1검사에 사용하십시오.tests/test_sandboxed_web_e2e_output_limits.py#L267-L301: 같은 지역 변수를 도입하고 라인 289의--service-log-limit-bytes값과 라인 299의<= log_limit_bytes + 1검사에 사용하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sandboxed_web_e2e_output_limits.py` around lines 42 - 65, Introduce a local log_limit_bytes = 4096 in test_sandboxed_web_e2e_output_limits.py at lines 42-65 and use it for the start_service limit argument and the <= log_limit_bytes + 1 assertion. Apply the same local variable and substitutions at lines 267-301 for the --service-log-limit-bytes argument and size assertion.
144-175: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win서비스 준비 출력에 대한 동기화 지점이 없습니다. 테스트가 불안정할 수 있습니다.
라인 171과 172는 서비스 로그 tail에
backend-ready와frontend-ready가 나타난다고 단언합니다. 그러나 이 실행은--backend-ready-url과--frontend-ready-url을 지정하지 않습니다. 따라서wait_for_url은 즉시 True를 반환하고main은 곧바로 E2E 명령을 실행한 뒤 정리 단계로 진입합니다.서비스 자식은 별도의 Python 인터프리터입니다. 부하가 높은 러너에서 인터프리터 시작이 늦어지면
stop_service가 SIGTERM을 보냅니다. 그 경우 로그가 비어 있고 어서션이 실패합니다.준비 URL을 사용하거나, 서비스가 준비 표시를 기록할 때까지 로그 파일을 폴링하면 이 경쟁 조건이 제거됩니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sandboxed_web_e2e_output_limits.py` around lines 144 - 175, Update test_normal_services_and_e2e_preserve_existing_success_contract to synchronize service startup before running the E2E command: either provide reachable --backend-ready-url and --frontend-ready-url values or poll each service log until backend-ready and frontend-ready are recorded. Preserve the existing success and payload assertions while ensuring both readiness messages are emitted before cleanup can terminate the child processes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 16-17: Under the “### Fixed” section of CHANGELOG.md, add a
changelog entry for the security boundary introduced by
validate_repository_symlinks: reject repository symlinks that resolve outside
the copied verification workspace, including absolute links, causing
verification to fail.
In `@scripts/ci/bounded_subprocess.py`:
- Around line 322-341: Update _cleanup_capture_startup_failure to close only
streams not owned by successfully started captures, rather than closing every
stream in streams while reader threads may still be active. Use each
BoundedOutputCapture’s stream ownership (adding a read-only stream property if
needed) to exclude captured streams from the close loop, while preserving the
existing cleanup and join behavior.
In `@scripts/ci/sandboxed_verify.py`:
- Around line 298-304: In main(), stop setting output_limited for the
OutputLimitUnsupportedError path because this exception indicates platform
capability failure, not an output-budget breach. Initialize
output_limit_unsupported to False, set it to True in that exception handler, and
pass it to emit_result under a separate evidence field while preserving the
existing fail-closed exit code.
In `@tests/test_sandboxed_verify_symlink_boundary.py`:
- Around line 64-82: 보이지 않는 증거 방출을 검증하도록
test_timeout_without_partial_streams_still_emits_failed_evidence를 수정하십시오.
capsys로 SANDBOXED_VERIFY_RESULT 출력 행을 읽고 JSON으로 파싱한 뒤, 실패 결과 페이로드의 구조와
output_limit_bytes 및 output_limited 필드를 포함한 정확한 값을 단언하십시오. 이를 위해 json을 import하고
기존 반환 코드 124 단언은 유지하십시오.
---
Nitpick comments:
In `@scripts/ci/bounded_subprocess.py`:
- Around line 109-117: The suffix_budget zero branch in _render_bounded_bytes is
unreachable after validate_output_limit enforces MINIMUM_OUTPUT_LIMIT_BYTES.
Remove the conditional fallback and construct the suffix using the
guaranteed-positive budget, preserving the marker-plus-suffix truncation
behavior and 100% branch coverage.
In `@scripts/ci/sandboxed_verify.py`:
- Line 288: Update the assignment in the completed-process handling path to
access the guaranteed output_limited field directly on completed instead of
using getattr with a fallback. Leave the getattr usage in the TimeoutExpired
exception handler unchanged, since that handler receives a base exception
without this field.
- Around line 159-188: Document the runtime symlink limitation in
docs/doctoring/sandboxed-verification-symlink-boundary.md: explain that
validate_repository_symlinks performs only a pre-execution scan, so an untrusted
verification command may create and follow an escaping symlink during execution;
clarify that this check is not runtime containment.
- Around line 197-198: Handle the ValueError raised by
validate_repository_symlinks within main() so it does not escape after
emit_result; emit an explicit path-boundary rejection message and set a
dedicated stable PATH_BOUNDARY_EXIT_CODE distinct from 123, 124, and 125. Ensure
the emitted evidence records that code while preserving the existing
general-failure behavior, and document the new code in the specified
symlink-boundary guide and CHANGELOG.md.
In `@scripts/ci/sandboxed_web_e2e.py`:
- Around line 253-265: Move the max_lines validation in tail_text before the
path.exists() check so non-positive values always raise ValueError, including
for missing paths. Preserve the existing empty-string result for nonexistent
paths with valid arguments.
- Around line 404-415: Update the exception path in the finally cleanup loop
around stop_service to make a best-effort bounded_subprocess.kill_process_group
call for the failed service before continuing to the next service. Preserve the
existing exception handling and exit-code behavior, and ensure cleanup errors
from the fallback kill do not interrupt cleanup of remaining services.
In `@tests/test_bounded_subprocess_contract.py`:
- Around line 26-32: Update test_supported_platform_requires_posix_killpg and
the platform-support check to use a module-level helper that reports whether
os.killpg is available, then monkeypatch that helper to return false instead of
deleting killpg from the global os module. Preserve the expected
OutputLimitUnsupportedError fail-closed behavior.
In `@tests/test_sandboxed_verify_output_limits.py`:
- Around line 161-179: Replace the loop in
tests/test_sandboxed_verify_output_limits.py:161-179 with
`@pytest.mark.parametrize`("value", ["4095",
str(bounded.MAXIMUM_OUTPUT_LIMIT_BYTES + 1)]) and have the test validate one
value per invocation. Also replace the loop in
tests/test_sandboxed_web_e2e_output_limits.py:241-264 with
`@pytest.mark.parametrize`("option, value", [...]) so each option/value pair runs
as a separate test case with clear failure attribution.
In `@tests/test_sandboxed_web_e2e_branch_contract.py`:
- Around line 112-122: Extend
test_service_limit_fallback_handles_missing_small_and_large_files to write
exactly 4096 bytes and assert service_output_limited(service) is false, while
preserving the existing checks for smaller and larger files.
In `@tests/test_sandboxed_web_e2e_output_limits.py`:
- Around line 42-65: Introduce a local log_limit_bytes = 4096 in
test_sandboxed_web_e2e_output_limits.py at lines 42-65 and use it for the
start_service limit argument and the <= log_limit_bytes + 1 assertion. Apply the
same local variable and substitutions at lines 267-301 for the
--service-log-limit-bytes argument and size assertion.
- Around line 144-175: Update
test_normal_services_and_e2e_preserve_existing_success_contract to synchronize
service startup before running the E2E command: either provide reachable
--backend-ready-url and --frontend-ready-url values or poll each service log
until backend-ready and frontend-ready are recorded. Preserve the existing
success and payload assertions while ensuring both readiness messages are
emitted before cleanup can terminate the child processes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01f67cd3-3b3c-4a93-9c08-53fb8c91dc3e
📒 Files selected for processing (17)
CHANGELOG.mddocs/doctoring/sandboxed-output-resource-bounds.mddocs/doctoring/sandboxed-verification-symlink-boundary.mdscripts/ci/bounded_subprocess.pyscripts/ci/sandboxed_verify.pyscripts/ci/sandboxed_web_e2e.pytests/test_bounded_subprocess.pytests/test_bounded_subprocess_capture_startup.pytests/test_bounded_subprocess_contract.pytests/test_repository_branch_coverage_execution_sandboxes.pytests/test_sandboxed_entrypoint_and_cleanup_coverage.pytests/test_sandboxed_service_capture_startup.pytests/test_sandboxed_verify_output_limits.pytests/test_sandboxed_verify_symlink_boundary.pytests/test_sandboxed_web_e2e.pytests/test_sandboxed_web_e2e_branch_contract.pytests/test_sandboxed_web_e2e_output_limits.py
|
@coderabbitai review Re-evaluate exact current head |
|
I will evaluate this head independently. I will keep semantic source findings separate from workflow and policy status. This review does not infer independent human approval or merge authority.
|
|
@opencode-agent review Evaluate exact current head |
1 participant
Buyer and security outcome
This draft now closes two repository-owned availability and filesystem boundaries in the central sandbox wrappers without weakening execution, review, or merge policy:
Issue #766 remains open until protected integration and protected-main operational acceptance. The separately reviewed credential-redaction line in #906 is not claimed as shipped or bundled here.
Exact identity
dd2132b5aedc56859ffa81568d42ab9e81e52b656eb06cdd08c79a06f7b390069d4ffa49e2eb7dbamaintip:6eb06cdd08c79a06f7b390069d4ffa49e2eb7dbaNo predecessor-head, synthetic merge revision, queued workflow, status-only result, automated model result, or author action transfers to this head.
Symlink containment
copy_workspacevalidates the exact copied tree after ignore rules are applied. Absolute links and relative links resolving outside the copied repository fail closed. Internal relative links remain links; ignored paths do not create false positives. This is filesystem containment, not an operating-system or network sandbox claim.The original RED commit was
faca1f145f237ce7b561218d707a40ae33471b88; the ignored-path regression was preserved at6f597306a7414e4ab027af42c8d8672f8da8de39.Output-resource RCA and remedy
The first failing boundary was retention:
subprocess.run(..., stdout=PIPE, stderr=PIPE)buffered complete short-lived streams in parent memory, services wrote ordinary unbounded log files, andtail_text()read complete files before selecting final lines.Rejected remedies:
RLIMIT_FSIZE, because it would also cap legitimate coverage databases, build artifacts, archives, and application files;communicate()orread_text(), because exhaustion occurs before that boundary;Implemented:
bounded_subprocess.pywith one binary reader thread per pipe, locked final-suffix buffers, one overflow transition, and finite 30-second reader joins;shell=False, structured argv,start_new_session=True, and whole-process-group termination on the first overflowing stream;123, timeout124, and readiness125, with timeout retaining precedence;126with non-sensitivepath_boundary_rejectedevidence;The test-only RED head
b4547a55a732f3f2f6b64e5924bca11e10871599failed collection because the bounded runner did not exist. GREEN is the exact current head.Exact local proof
At the exact current tree:
git diff --check: pass.Tests exercise isolated platform-capability probes, missing-file budget validation, the exact service evidence ceiling, real stdout and stderr floods, service-log overflow before E2E sentinel execution, Unicode and partial UTF-8 suffixes, real readiness-synchronized ordinary success, exact 4 KiB boundary behavior, timeouts, reader errors, stuck readers, sibling finalization, capture-startup cleanup, unsupported platforms, invalid budgets, bounded kept-sandbox files, symlink containment, and exit-code precedence.
Governance, limitations, and rollback
Environment scrubbing, readiness SSRF controls, exact-head evidence, semantic review, independent approval, and branch protection remain separate authorities. This slice does not cap repository-copy size, application artifacts, CPU beyond existing timeouts, process count, address space, network traffic, or unrelated processes. A descendant that creates a new session can escape process-group termination, but finite reader joins convert retained descriptors into deterministic failure instead of an unbounded workflow wait.
Rollback requires a separately reviewed replacement proving bounded parent memory, bounded service evidence files, finite reader finalization, timeout/cleanup behavior, and realistic flood resistance.
Merge gate
Keep Draft until every required exact-head CI/security/supply-chain workflow is terminal-success, semantic review has no valid unresolved finding, live-base compatibility is refetched, qualifying independent non-author formal approval exists where required, and repository protection permits integration. After merge, run protected-main command and service flood canaries before closing #766.
Summary by CodeRabbit
새로운 기능
문서
버그 수정