fix(release): snapshot reviewed dependency inputs - #206
Conversation
📝 WalkthroughWalkthrough검토 입력에 1MiB 제한과 파일 식별자 사전 검증을 추가했습니다. 매니페스트와 런타임 lock 파일을 비공개 스냅샷으로 복사합니다. SBOM 생성기는 원본 대신 스냅샷을 사용합니다. 입력 변경과 크기 경계 조건을 검증하는 테스트를 추가했습니다. Changes검토 입력 스냅샷
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Preparer as prepare_release_evidence.py
participant Inputs as 매니페스트 및 런타임 lock 파일
participant Generator as SBOM 생성기
participant Evidence as 증거 게시 단계
Preparer->>Inputs: 크기와 파일 식별자 사전 검증
Preparer->>Inputs: 비공개 스냅샷 생성
Preparer->>Generator: 두 스냅샷 경로 전달
Generator-->>Preparer: 스냅샷 기반 SBOM 생성
Preparer->>Evidence: 생성된 증거 게시
Preparer->>Inputs: 비공개 스냅샷 삭제
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 |
seonghobae
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/prepare_release_evidence.py (1)
179-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win스냅샷 파일 이름 충돌을 방지하십시오.
Line 203은
path.name만으로 스냅샷 경로를 생성합니다. 서로 다른 디렉터리의requirements.txt파일 두 개는 서로 다른 정규화 경로와 identity를 가질 수 있습니다. 그러나 두 번째 스냅샷은 이미 존재하는 같은 이름 때문에FileExistsError로 실패합니다.의존성 매니페스트와 runtime lock에 서로 다른 고정 스냅샷 이름을 전달하십시오. 같은 basename을 사용하는 두 입력 디렉터리의 회귀 테스트도 추가하십시오.
Also applies to: 452-465
🤖 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/prepare_release_evidence.py` around lines 179 - 203, Update _snapshot_distribution so callers provide distinct fixed snapshot names for dependency manifests and runtime locks instead of deriving snapshot_path from path.name. Ensure the chosen names remain collision-free when inputs from different directories share a basename, and add a regression test covering those same-basename inputs.
🤖 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 `@scripts/ci/prepare_release_evidence.py`:
- Around line 139-145: Normalize all rejection paths in the reviewed-input
prevalidation and snapshot flows, including the logic around
DistributionIdentity and the referenced call sites, to raise the same generic
error. Replace the size-specific “exceeds the safety bound” message with the
existing generic rejection message, and ensure errors do not disclose the
rejection rule, input type, or path.
- Around line 452-478: Update the security-boundary documentation for the SBOM
generation flow around _snapshot_distribution and
generator.build_attestable_sbom: record in CHANGELOG.md, the relevant APA 7th
research documentation, the security model, and the architecture documentation
that the generator now consumes private snapshots rather than mutable
caller-controlled paths.
In `@tests/test_prepare_release_evidence_dependency_input_snapshot.py`:
- Around line 19-72: The test
test_reviewed_dependency_inputs_are_snapshotted_before_generator_load currently
stops on the first build_attestable_sbom call; return a valid SBOM from the
first invocation, validate the accepted snapshots on the second invocation, then
raise SnapshotObserved. Add separate boundary tests covering
MAX_REVIEWED_INPUT_BYTES: exactly 1 MiB inputs must reach generator loading,
while inputs exceeding 1 MiB must be rejected before _load_attestable_generator
runs.
---
Outside diff comments:
In `@scripts/ci/prepare_release_evidence.py`:
- Around line 179-203: Update _snapshot_distribution so callers provide distinct
fixed snapshot names for dependency manifests and runtime locks instead of
deriving snapshot_path from path.name. Ensure the chosen names remain
collision-free when inputs from different directories share a basename, and add
a regression test covering those same-basename inputs.
🪄 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: 63c84aab-f599-46f2-9e9b-bbd299c09d21
📒 Files selected for processing (2)
scripts/ci/prepare_release_evidence.pytests/test_prepare_release_evidence_dependency_input_snapshot.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/ci/prepare_release_evidence.py`:
- Around line 439-446: Wrap both _require_canonical_file() calls for
dependency_manifest and runtime_lock in the REVIEWED_INPUT_REJECTION
normalization boundary so missing or symlinked inputs produce the same generic
rejection as _require_reviewed_input_preflight(), without exposing paths or
rejection causes. Add regression coverage for both missing-input and
symlink-input cases.
🪄 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: cd680e76-688c-4037-8341-565eed9f8be4
📒 Files selected for processing (3)
docs/release-evidence-preparation.mdscripts/ci/prepare_release_evidence.pytests/test_prepare_release_evidence_dependency_input_snapshot.py
|
@coderabbitai review |
|
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review finding
MAJOR — (device, inode, size) does not bind the reviewed input bytes
The repair claims that the generator consumes the exact manifest and runtime-lock bytes accepted before generator loading, but DistributionIdentity contains only (st_dev, st_ino, st_size). _snapshot_distribution() rechecks that triple before and after the copy. A writer can modify a regular file in place while preserving its inode and size, so every current identity check can pass while the private snapshot contains different or even mixed bytes from those accepted at preflight.
The existing tests cover pathname replacement, symlinks, growth, and mutation after the snapshot has already been created; none covers a same-inode, same-size content change between reviewed-input preflight and snapshot, or during the copy. This leaves the stated dependency-evidence TOCTOU boundary incomplete.
Fix direction: bind reviewed inputs to content, not only metadata. A narrow approach is to create a reviewed-input identity that includes a SHA-256 digest computed through a no-follow descriptor under the same finite byte ceiling, with descriptor/path identity checks around that read; then compute the snapshot digest during the bounded copy and require it to match the accepted digest. Keep the generic rejection boundary, owner-only private snapshots, distinct fixed internal names, zero external fetch, and reuse the same detached snapshots for wheel and sdist. Merely adding mtime is weaker than the claimed exact-byte contract.
Regression: after _require_reviewed_input_preflight() returns, rewrite the manifest or lock in place with different bytes of exactly the same length and same inode. Assert preparation fails before _load_attestable_generator(), emits only reviewed input is unreadable or unsafe, removes temporary state, and does not allow wheel/sdist to observe changed bytes. Add a controlled during-copy mutation case if the implementation streams accepted bytes in a separate phase.
|
@opencode-agent fix Address the exact-current-head review finding on |
1 participant
Purpose
Close the release-evidence TOCTOU boundary proven by the test-first predecessor head: once the reviewed runtime dependency manifest and hash-locked runtime requirements are accepted, the attestable SBOM generator must consume those exact bytes rather than reopening caller-controlled mutable paths.
Exact current identity
7faf7a3b8a47980113982914000e724ab6a6cda5;945b143cce31e0499f16a324943b5e2b0390e2dd;adfcb652c37f2152cb7744b9d090c88b7ceae64c;COMMENTEDsubmissions from predecessor heads only; no qualifying approval;Historical checks/reviews are lineage only and do not transfer.
RCA -> narrow repair
scripts/ci/prepare_release_evidence.pypreviously canonicalized the reviewed manifest and runtime-lock paths but passed those same mutable filesystem paths to the generator after archive snapshotting. A pathname replacement after preparer acceptance but before generator use could therefore change the dependency evidence observed by the SBOM generator, and separate wheel/sdist passes could observe different bytes.The current repair:
(device, inode, size)identity before generator loading;CodeRabbit's valid predecessor-head findings were repaired and their threads are resolved. The exact current source and runbook document the resulting release-evidence trust boundary.
Exact-current-head evidence
On unchanged
adfcb652c37f2152cb7744b9d090c88b7ceae64c:31624163662: completed success;31624163671: completed success;31624163638: aggregate success, but dependency-review job94206163842skipped the actual immutable-pinned Dependency review action; wrapper green is therefore non-passing supply-chain evidence;94207380793: completed success as model/check evidence only;31624162033, exact-head job/check94206243911: failed after emittingVULN-0001, which claimed arbitrary code execution because_load_attestable_generator()loads the adjacent repository-onlygenerate_attestable_release_sbom.pywithimportlib.util.spec_from_file_location();9154754908, SHA-25651ffdff9558bc34bcb487e9ad9c91122d35cf7a543b9b4ddd5422d46c3f7c914is bound to this exact head.Strix finding disposition
VULN-0001is not accepted as a product/release-boundary vulnerability on this exact tree after source and canonical-boundary review. Its proof requires an attacker to replace repository executable source beside the preparer (the PoC copies the preparer into an attacker-controlled temporary directory and supplies a malicious sibling generator). That precondition is outside the supported preparation contract:scripts/ci/generate_attestable_release_sbom.pyin that executable source tree can equally rewritescripts/ci/prepare_release_evidence.pyorsrc/egressweave/release_evidence.py, so the reported loader does not create a distinct privilege escalation or new authority boundary;Accordingly no source churn is made merely to satisfy the scanner. If EgressWeave later adopts an accepted ADR that treats its own checked-out executable source as attacker-controlled, source-tree identity verification must be designed comprehensively across all repository-only executable modules rather than patched at one dynamic import.
This disposition does not convert the failed Strix required workflow into passing evidence. The PR remains Draft until a substantive exact-head Strix result is accepted under live organization governance.
Remaining acceptance
ContextualWisdomLab/.github#897or an accepted successor), then obtain fresh unchanged-tree Security Scan evidence where the actual pinned Dependency Review action executes and succeeds.The handoff remains credential-free consistency evidence, not provenance, publication authority, or a SLSA Build-level claim.