WS-ART-001-04A2: bounded outer ZIP safety - #266
Conversation
|
Warning Review limit reached
Next review available in: 43 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR implements bounded structural inspection for contributor submission archives. It adds ZIP validation primitives, configurable safety limits, comprehensive test coverage, and initiative activation records for the immutable artifact storage work. ChangesSubmission archive safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PreparedArtifact.inspect
participant SubmissionArchiveInspector
participant zip_directory_layout
PreparedArtifact.inspect->>SubmissionArchiveInspector: inspect prepared archive reader
SubmissionArchiveInspector->>zip_directory_layout: validate bounded directory metadata
zip_directory_layout-->>SubmissionArchiveInspector: return ZIP directory layout
SubmissionArchiveInspector->>SubmissionArchiveInspector: validate members and read bounded content
SubmissionArchiveInspector-->>PreparedArtifact.inspect: return sanitized inspection result or rejection code
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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[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 (6)
backend/app/modules/artifacts/zip_safety.py (2)
47-51: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare the EOCD per-disk entry count with the total entry count.
The non-ZIP64 branch discards
_disk_entries. The ZIP64 branch performs the equivalent check at line 77 (disk_entries64 != entries64). Add the same check here so a crafted EOCD cannot declare different per-disk and total counts on a single-disk archive.♻️ Proposed change
- disk, directory_disk, _disk_entries, entries, directory_bytes, directory_offset = ( + disk, directory_disk, disk_entries, entries, directory_bytes, directory_offset = ( struct.unpack_from("<HHHHII", tail, marker + 4) ) if disk or directory_disk: raise zipfile.BadZipFile("multi-disk archive") + if disk_entries != entries: + raise zipfile.BadZipFile("central directory entry count disagreement")🤖 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 `@backend/app/modules/artifacts/zip_safety.py` around lines 47 - 51, Update the non-ZIP64 EOCD validation near the struct.unpack_from result to reject archives when the per-disk entry count differs from the total entry count, matching the existing ZIP64 check while retaining the multi-disk validation.
52-57: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTreat
directory_offset == 0xFFFFFFFFas a ZIP64 sentinel.The ZIP64 branch triggers only when
entries == 0xFFFFordirectory_bytes == 0xFFFFFFFF. A ZIP64 archive with fewer than 65 535 entries and a small central directory stores the sentinel only indirectory_offset. That archive takes the non-ZIP64 branch, and line 53 then raises"central digrectory offset disagreement"even whenallow_zip64=True. The current 512 MiB source ceiling keeps real offsets below 4 GiB, so this is not reachable today, but the parser is documented as neutral and shared. Include the third sentinel in the branch condition.♻️ Proposed change
- if entries != 0xFFFF and directory_bytes != 0xFFFFFFFF: + if ( + entries != 0xFFFF + and directory_bytes != 0xFFFFFFFF + and directory_offset != 0xFFFFFFFF + ):🤖 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 `@backend/app/modules/artifacts/zip_safety.py` around lines 52 - 57, Update the ZIP directory layout branch condition to treat directory_offset == 0xFFFFFFFF as a ZIP64 sentinel alongside entries and directory_bytes in the surrounding parser logic. Ensure archives using only this offset sentinel follow the ZIP64 path and do not trigger the non-ZIP64 offset disagreement validation.backend/app/modules/artifacts/submission_archive.py (4)
179-188: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the two linear scans with set lookups.
Line 181 scans every recorded path for each file entry. Line 186 scans every accumulated result for each directory entry. Both are O(n²) in the entry count.
maximum_entriesaccepts up to 100 000, so a contributor archive with a large flat tree makes inspection consume the wholemaximum_inspection_secondsbudget and then fail withTIMEOUTinstead of completing. Track explicit directory paths in a set and reusepathsfor the prefix decision.⚡ Proposed change
if entry_type is SubmissionArchiveEntryType.FILE: - prefix = folded + "/" - if any(existing.startswith(prefix) for existing in paths): + if folded in directory_parents: self._reject(SubmissionArchiveFailureCode.COLLISION) paths[folded] = entry_type source_paths.add(folded) - if entry_type is SubmissionArchiveEntryType.DIRECTORY and any( - entry.normalized_path == path for entry in results - ): + if entry_type is SubmissionArchiveEntryType.DIRECTORY and folded in implicit: continueMaintain the two new sets next to
paths: add every folded ancestor todirectory_parentsinside the loop at lines 163-178, and add each derived implicit folded path toimplicitat line 169.🤖 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 `@backend/app/modules/artifacts/submission_archive.py` around lines 179 - 188, In the archive inspection loop around paths, replace both per-entry scans with set membership checks: maintain a directory_parents set containing folded ancestor paths and an implicit set containing derived implicit folded paths, updating them where ancestor and implicit paths are created. Use directory_parents to decide whether a file prefix collides and use paths together with implicit state to skip duplicate implicit directories, preserving existing collision and result behavior.
428-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
_rejectasNoReturn.
_rejectalways raises, but its annotation is-> None. Type checkers therefore treat every call site as a normal return. Two consequences follow.entriesat line 136 is seen as possibly unbound after theexceptblock at line 134.source.tell()at line 305 and line 386 is seen as a possibleNonedereference after theif source is Noneguard.NoReturndocuments the fail-closed contract and removes both false paths.♻️ Proposed change
+from typing import BinaryIO, NoReturn ... `@staticmethod` - def _reject(code: SubmissionArchiveFailureCode) -> None: + def _reject(code: SubmissionArchiveFailureCode) -> NoReturn: raise SubmissionArchiveRejectedError(code)Remove the existing
from typing import BinaryIOat line 12 when you apply the combined import.🤖 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 `@backend/app/modules/artifacts/submission_archive.py` around lines 428 - 430, Update the return type annotation of the _reject method from None to NoReturn to accurately reflect that it always raises an exception and never returns normally. Import NoReturn from the typing module at the top of the file, consolidating it with any existing typing imports. If a separate BinaryIO import from typing exists at line 12, remove it when combining the imports together.
220-234: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMeasure the path limits on the normalized form.
Line 232 measures
len(path.encode("utf-8"))on the pre-normalization name, and line 243 returnsunicodedata.normalize("NFC", path).docs/spec_artifact_storage_service.mdline 869 defines the limit as "UTF-8 bytes in one normalized POSIX path". NFC changes byte length for decomposed input, so the enforced limit and the documented limit differ. Normalize once at the top of the function and validate the normalized value.♻️ Proposed change
- path = raw[:-1] if is_directory and raw.endswith("/") else raw + path = unicodedata.normalize( + "NFC", raw[:-1] if is_directory and raw.endswith("/") else raw + )Then return
pathdirectly at line 243 instead of normalizing again.Also applies to: 243-243
🤖 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 `@backend/app/modules/artifacts/submission_archive.py` around lines 220 - 234, Normalize the path using unicodedata.normalize("NFC", path) once at the beginning of the validation logic, right after the initial trailing slash handling. Update the byte-length validation check for len(path.encode("utf-8")) to measure the normalized form instead of the pre-normalized form. Then return the already-normalized path directly at the return statement instead of normalizing it again, ensuring the enforced limit matches the documented limit on the normalized POSIX path.
267-282: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid decompressing each member twice.
_validate_compressed_streamalready inflates the member and checksactual == info.file_size,decompressor.eof, unused data, and unconsumed tail. The followingarchive.open(info, "r")inflates the same member again for CRC validation. Computezlib.crc32during the first pass for bothZIP_DEFLATEDandZIP_STORED, then compare the result withinfo.CRCso_read_memberdoes not need the second pass.🤖 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 `@backend/app/modules/artifacts/submission_archive.py` around lines 267 - 282, Update _validate_compressed_stream to compute zlib.crc32 while reading both ZIP_DEFLATED and ZIP_STORED members, then compare the final checksum with info.CRC and reject integrity failures on mismatch. Remove the second archive.open pass from _read_member, retaining its output-limit and size checks while reusing the first-pass validation result.
🤖 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
@.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/AUTH_HANDOFF.md:
- Around line 29-30: Update the guide-source ingest statement in AUTH_HANDOFF.md
to match the activation record: describe ingest as merged or implemented only if
it has its own activation record; otherwise remove it from the active list and
retain only the explicitly active binding/create and read chunks.
In
@.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04A2-external-review-response.md:
- Around line 5-16: Update the hosted-gate status in this review response to
explicitly state whether the hosted GitHub shared_foundations check and
CodeRabbit review were attempted, completed, or remain pending, resolving the
contradiction with the PR objectives. Preserve the requirement that both checks
receive a fresh hosted run on the correction commit.
In `@backend/app/core/config.py`:
- Around line 174-176: The configuration allows both
artifact_submission_zip_maximum_inspection_seconds and
artifact_preparation_total_deadline_seconds to be set to 1800.0 simultaneously,
which violates the documented invariant that the inspection deadline must be
strictly inside the preparation deadline with margin for scratch release. Add a
cross-field validator to the config class that enforces
artifact_submission_zip_maximum_inspection_seconds is strictly less than
artifact_preparation_total_deadline_seconds, placing it alongside the existing
entry and expanded field validators to maintain consistency with the codebase's
validation pattern.
In `@backend/tests/test_submission_archive.py`:
- Around line 76-86: Update the Unicode collision entry in
test_collision_and_ancestry_confusion_fail_closed to use explicit Unicode escape
sequences for the NFC and NFD forms of the filename, ensuring the dictionary
retains two distinct keys and the test consistently exercises normalization
collision rejection.
---
Nitpick comments:
In `@backend/app/modules/artifacts/submission_archive.py`:
- Around line 179-188: In the archive inspection loop around paths, replace both
per-entry scans with set membership checks: maintain a directory_parents set
containing folded ancestor paths and an implicit set containing derived implicit
folded paths, updating them where ancestor and implicit paths are created. Use
directory_parents to decide whether a file prefix collides and use paths
together with implicit state to skip duplicate implicit directories, preserving
existing collision and result behavior.
- Around line 428-430: Update the return type annotation of the _reject method
from None to NoReturn to accurately reflect that it always raises an exception
and never returns normally. Import NoReturn from the typing module at the top of
the file, consolidating it with any existing typing imports. If a separate
BinaryIO import from typing exists at line 12, remove it when combining the
imports together.
- Around line 220-234: Normalize the path using unicodedata.normalize("NFC",
path) once at the beginning of the validation logic, right after the initial
trailing slash handling. Update the byte-length validation check for
len(path.encode("utf-8")) to measure the normalized form instead of the
pre-normalized form. Then return the already-normalized path directly at the
return statement instead of normalizing it again, ensuring the enforced limit
matches the documented limit on the normalized POSIX path.
- Around line 267-282: Update _validate_compressed_stream to compute zlib.crc32
while reading both ZIP_DEFLATED and ZIP_STORED members, then compare the final
checksum with info.CRC and reject integrity failures on mismatch. Remove the
second archive.open pass from _read_member, retaining its output-limit and size
checks while reusing the first-pass validation result.
In `@backend/app/modules/artifacts/zip_safety.py`:
- Around line 47-51: Update the non-ZIP64 EOCD validation near the
struct.unpack_from result to reject archives when the per-disk entry count
differs from the total entry count, matching the existing ZIP64 check while
retaining the multi-disk validation.
- Around line 52-57: Update the ZIP directory layout branch condition to treat
directory_offset == 0xFFFFFFFF as a ZIP64 sentinel alongside entries and
directory_bytes in the surrounding parser logic. Ensure archives using only this
offset sentinel follow the ZIP64 path and do not trigger the non-ZIP64 offset
disagreement validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03ff15c2-973f-4894-aef2-e597a7edb08d
📒 Files selected for processing (17)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/AUTH_HANDOFF.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/CHUNK_MAP.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04A2-outer-zip-safety.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-PLAN3-v01-end-to-end-reconciliation.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04A2-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04A2-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04A2-pr-trust-bundle.mdbackend/app/adapters/artifacts/__init__.pybackend/app/core/config.pybackend/app/modules/artifacts/guide_formats.pybackend/app/modules/artifacts/submission_archive.pybackend/app/modules/artifacts/zip_safety.pybackend/scripts/run_test_lanes.pybackend/tests/test_config.pybackend/tests/test_submission_archive.pydocs/spec_artifact_storage_service.md
a283c09 to
6116160
Compare
2 participants
WS-ART-001-04A2 PR Trust Bundle
Chunk
WS-ART-001-04A2 — Bounded Outer-ZIP Safety(L1)Goal and human-approved intent
Inspect one contributor outer ZIP completely and safely inside canonical
private scratch, returning only bounded structural facts for the later semantic
manifest chunk. The ZIP's required contents remain governed by the locked
Project Guide. This chunk creates no route, checker, provider write, admission,
or Submission.
What changed and why
compression-stream, path, collision, type, and quota validation.
zip_safety.pyfor guideand submission reuse without guide-specific recursion.
The later manifest, checker, reviewer, and client chain cannot trust a ZIP if
bytes can hide outside its enumerated tree or scratch ownership leaks.
Design chosen and alternatives rejected
SubmissionArchiveInspectorimplements the existingPreparedArtifactInspectorseam. It reads scratch-owned bytes withoutextracting them and returns sorted process-local facts only. It accepts stored
and raw-DEFLATE members, treats nested ZIPs as opaque files, and proves
continuous byte coverage through the exact EOCD/comment boundary.
Rejected: guide-detector reuse, recursive nested ZIP inspection, direct temp
paths, a second scratch manager, provider writes, caller-owned limits,
self-extracting envelopes, and compression methods without exact-consumption
proof.
Scope control and product behavior
No public/multipart route, durable model or migration, provider I/O, AUTH
activation, checker invocation, semantic hash, executable normalization,
unchanged-work comparison, or Submission behavior was added. Hidden inspection
returns a bounded tree or one stable redacted internal failure. Rejection has no
capacity, provider, review, contribution, payment, or reputation effect.
Acceptance criteria proof
zipfile; exactstored/deflate range consumption is independently proven.
descriptors, multi-disk layouts, traversal, collisions, special entries,
encryption, directory payloads, and bombs reject.
PreparedArtifact.inspect(...).provider fact, authorization handle, hash, or durable identity.
Tests/checks run and test delta
submission_archive.pyandzip_safety.py: each exceeds 90-percent focusedcoverage.
git diff --check:passed.
the new test module exactly once.
CI integrity and reviewer results
No workflow, threshold, dependency, package-script, or skip behavior changed.
The repository-wide 78-percent hosted floor and Backend/Agent Gates remain
required. Architecture, product/operations, documentation, and reuse reviews
pass. Security, QA, senior engineering, CI integrity, and test-delta reviews
pass with only documented low residual risks; all actionable findings were
resolved.
External review
CodeRabbit was rate-limited and produced no substantive comments. The first
hosted
shared_foundationsshard exposed two guide-OOXML stable-classificationregressions in the moved neutral ZIP probe; the helper was corrected and 119
focused guide/submission tests pass. Fresh hosted checks are required on that
correction commit.
Remaining risks and follow-up
Only stored and raw-DEFLATE ZIP members are supported in v0.1. 04A3 adds the
canonical semantic manifest, executable normalization, and unchanged-work gate
only after this PR merges.
Human review focus and merge ownership
Review byte-range accounting, local/central name binding, deflate EOF/unused
data, path collisions, and absence of provider/durable/public reachability.
Only the human repository owner may approve and merge after hosted checks and
external review pass.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation