Skip to content

fix(archive): preserve integrity errors with updated ZIP decoder - #997

Merged
steipete merged 2 commits into
mainfrom
fix/zip-audit-patch
Oct 16, 2026
Merged

steipete merged 2 commits into
mainfrom
fix/zip-audit-patch

Conversation

@steipete

@steipete steipete commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Main CI began failing strict Cargo audit after crates.io yanked zip 9.0.0. Update to the unyanked 9.0.3 patch with the same Rust 1.88 minimum and existing feature selection.

The new ZIP reader rejects data exceeding its declared size before returning the bytes. Preserve fs-safe's public error contract: malformed entries produce ArchiveFormatError, while exceeding the caller's byte budget takes precedence as ArchiveLimitError. Extraction still rejects the archive without publishing partial output. Keep the upstream integrity check and strict audit enabled.

Validation

  • Reproduced the old lockfile's audit failure; the updated lockfile passes cargo audit --deny warnings.
  • The initial dependency-only CI exposed the integrity classification regression on multiple platforms. The existing synthetic ZIP tests reproduce it; added zero-byte budget coverage.
  • AWS Crabbox passed a release native build, all ZIP integrity cases with the native backend required, Clippy, and repository lint checks. Run: run_879dfe43c44cf51f59a42f3ad8594a0e.
  • Earlier focused Cargo gate passed all 30 archive tests and strict audit. Run: run_6a181486346992f4551d0312f41ed7ec.
  • Independent code review passed through P2; exact-head CI supplies full cross-platform proof.

A fresh merged-main release dry run will verify packaging after this merge. No release, tag, or publication is requested.

@steipete
steipete requested a review from a team as a code owner October 16, 2026 04:14
@clawsweeper

clawsweeper Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T04:29:37.127980Z 4abdc67 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@steipete steipete changed the title build(deps): replace yanked zip with 9.0.3 fix(archive): preserve integrity errors with updated ZIP decoder Oct 16, 2026
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 16, 2026
@clawsweeper

clawsweeper Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes

This PR updates the native ZIP decoder to 9.0.3 and adapts oversized-entry errors to preserve public error classifications.

Example: Read a 16-byte ZIP entry declared as 1 byte with maxBytes=1.

  • Before: The dependency-only update produces a generic native read error.
  • After: The adapter produces ArchiveLimitError.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) Useful, focused work with integration-test support, but an uncovered input still violates the public error contract.
Proof confidence 🦐 gold shrimp (3/6) The maintainer-authored PR is outside the ordinary contributor proof gate; its supplied Crabbox claims describe native integration tests and Cargo validation rather than a shipped-package execution trace.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: The decoder update changes the error classes callers receive for malformed ZIP payloads and byte-budget violations.
Reason: The maintainer-authorized update preserves an established public contract and replaces the yanked dependency without adding an API or configuration option.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Keep this PR open: the dependency update is useful, but an intermediate byte budget still receives the wrong public error type.

Priority: P2
Reviewed head: 4abdc678585bc51a979ccbc06b381123fb928e8f

Before merge

  • Preserve limit errors when the budget exceeds the declared size (P2) - Use the existing stored ZIP fixture with its 16-byte payload, declaredSize=1, and maxBytes=2. ZIP 9.0.3 rejects a read exceeding the declared size before returning its byte count, so this branch emits archive-header-invalid and the public API throws ArchiveFormatError. Previously the bounded read reached its budget check and threw ArchiveLimitError, as docs/archive.md requires whenever decoded output exceeds maxBytes. The same problem affects deflated entries. Handle declaredSize < maxBytes < decodedSize without disabling integrity enforcement, and add regression coverage alongside the existing budget cases.

Findings

  • [P2] Preserve limit errors when the budget exceeds the declared size — native/src/archive.rs:715-718

Tests

  • Missing end-to-end proof: No shipped-package trace covers a 16-byte ZIP declared as 1 byte with maxBytes=2.
Agent review details

How this fits together

Archive APIs admit ZIP metadata, decode selected payloads through native or JavaScript readers, and return bounded buffers or publish a validated staging tree.

flowchart TD
A[Archive API] --> B[Metadata admission]
B --> C[Native ZIP decoder]
B --> D[JavaScript ZIP decoder]
C --> E[Limits and integrity classification]
D --> E
E --> F[Buffer or staged publication]
Loading

Technical review

Best possible solution:

Preserve byte-limit precedence for intermediate budgets while retaining decoder integrity enforcement and unpublished extraction failures.

Do we have a high-confidence way to reproduce the issue?

The existing 16-byte fixture with declaredSize=1 and maxBytes=2 follows the decoder's verified InvalidData path into ArchiveFormatError; this was established from source without running target code.

Is this the best way to solve the issue?

Updating the decoder and adapting its errors is appropriate, but declared size alone cannot determine whether an intermediate caller budget was exceeded.

Full review comments:

  • [P2] Preserve limit errors when the budget exceeds the declared size — native/src/archive.rs:715-718
    Use the existing stored ZIP fixture with its 16-byte payload, declaredSize=1, and maxBytes=2. ZIP 9.0.3 rejects a read exceeding the declared size before returning its byte count, so this branch emits archive-header-invalid and the public API throws ArchiveFormatError. Previously the bounded read reached its budget check and threw ArchiveLimitError, as docs/archive.md requires whenever decoded output exceeds maxBytes. The same problem affects deflated entries. Handle declaredSize < maxBytes < decodedSize without disabling integrity enforcement, and add regression coverage alongside the existing budget cases.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 44cd047b54e6.

Provenance checked

  • Native ZIP integrity and bounded reads keeps the original intent (59b9cc4: Reject selected ZIP payloads whose decoded size differs from the manifest while retaining bounded reads and CRC checks; later buffered-reader and reservation refactors retained that contract.)
  • Owned native error construction keeps the original intent (eb0f242: Transfer already-owned error messages without another string allocation while preserving status and message semantics.)
  • ZIP dependency selection changes intended behavior with a stated reason (54531f8: Upgrade to ZIP 9 while preserving raw metadata admission, enabled codecs, and Rust 1.88 compatibility.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

Acceptance criteria:

  • pnpm native:build
  • FS_SAFE_NATIVE_MODE=require pnpm test test/archive-zip-integrity.test.ts
  • pnpm check
  • git diff --check

What I checked:

  • Introduced classification defect: The pinned diff maps decoder size errors to ArchiveLimitError only when max_bytes <= expected_size; every larger budget takes the format-error branch before the output-length check. (native/src/archive.rs:715, 4abdc678585b)
  • Documented byte-limit precedence: The public contract says output exceeding maxBytes throws ArchiveLimitError, while size mismatches within that cap throw ArchiveFormatError on both backends. (docs/archive.md:730, 4abdc678585b)
  • Decoder failure mechanism: The target directly imports this decoder through native/Cargo.toml. At v9.0.3, Crc32Reader::read consumes bytes, checks their count against the declared size, and returns InvalidData before returning the count; this independently establishes the intermediate-budget failure. (src/crc32.rs:71, 06b1fe7d5184)
  • Prior integrity contract and feature ownership: fix: guard lock handoffs and ZIP read integrity #189 established native decoded-size verification; chore(deps): update all dependencies #883 deliberately preserved archive admission behavior during the ZIP 9 upgrade. History also identifies the buffered-reader and bounded-reservation refactors. Following the integrity test history ended at an unavailable creation blob; the relevant earlier integrity patch was independently readable. (native/src/archive.rs:821, 59b9cc4ea1b1)
  • Policy and validation review: Read the complete root AGENTS.md; no nested policies or maintainer-note entries were present. The added test exercises maxBytes=0 but omits declaredSize < maxBytes < decodedSize. Supplied validation reports native integration tests and Cargo gates; no target code was executed during this review, diff whitespace checks passed, and the checkout remained clean. (test/archive-zip-integrity.test.ts:118, 4abdc678585b)

Labels

Label changes:

  • add P2: Malformed ZIP reads can return the wrong documented error class, with limited impact and no demonstrated publication or confinement bypass.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦐 gold shrimp and patch quality is 🦪 silver shellfish.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: Malformed ZIP reads can return the wrong documented error class, with limited impact and no demonstrated publication or confinement bypass.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦐 gold shrimp and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 16, 2026, 12:40 AM ET / 04:40 UTC.

@steipete
steipete merged commit 3960707 into main Oct 16, 2026
58 of 60 checks passed
@steipete
steipete deleted the fix/zip-audit-patch branch October 16, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant