fix: harden archive extraction validation

[steipete]

Summary

• reject NUL and drive-relative archive names, duplicate names, and output collisions introduced by stripping
• keep explicit zero limits effective and count stripped TAR entries toward the archive-wide entry budget
• verify streamed JavaScript ZIP payload CRC and declared size for extraction and bounded reads
• align native and JavaScript duplicate/collision behavior and reject invalid non-finite stream bounds
• clarify that secret read helpers pin identity and enforce link/size policy, while permission validation belongs to readSecureFile

Proof

• pnpm check (699 passed, 32 skipped)
• pnpm test:security (64 passed)
• pnpm native:test (14 passed)
• native + JavaScript archive equivalence (39 passed)
• Codex autoreview clean; no accepted/actionable findings

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 7:44 PM ET / 23:44 UTC.

ClawSweeper review

What this changes

The branch hardens ZIP and TAR extraction and bounded archive reads by rejecting ambiguous names and collisions, preserving explicit limits, validating ZIP integrity metadata, aligning native and JavaScript policy, and clarifying secret-reader guarantees.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 7 items remain

Keep this PR open: its archive-hardening direction is sound, but collision tracking now runs before the documented skip-entry filter policy, so an archive can be rejected for a collision among entries that the caller explicitly chose not to extract. The PR also still needs inspectable real-behavior proof beyond claimed automated test counts.

Priority: P1
Reviewed head: e1750181f2ae4b1112b04d1b5e9d94f6bebece19

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The security-hardening patch has focused tests, but a P1 filter-contract regression and missing inspectable real-behavior proof keep it from merge readiness.
Proof confidence	🧂 unranked krab (1/6)	Needs real behavior proof before merge: The PR body reports automated checks and test counts, but no inspectable after-fix live transcript, recording, linked artifact, or redacted runtime log demonstrates malformed archive rejection and an unchanged destination. After adding redacted proof and updating the PR body, ClawSweeper should re-review automatically; otherwise a maintainer can request @clawsweeper re-review.
Patch quality	🦪 silver shellfish (2/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The PR body reports automated checks and test counts, but no inspectable after-fix live transcript, recording, linked artifact, or redacted runtime log demonstrates malformed archive rejection and an unchanged destination. After adding redacted proof and updating the PR body, ClawSweeper should re-review automatically; otherwise a maintainer can request @clawsweeper re-review.
Evidence reviewed	5 items	Documented filter contract: The archive API explicitly supports onFiltered: "skip-entry" so callers can omit selected validated entries; the documentation’s restore example uses that mode to omit regenerated cache paths while retaining the rest of the archive. ZIP regression introduced by the patch: The ZIP extractor records an output path before it calls shouldExtractArchiveEntry; therefore two paths that collide after stripping reject even when the filter skips one or both entries and no output would be created. Equivalent ordering exists in TAR and native paths: The new tracker is likewise invoked before filter evaluation in the JavaScript TAR checker and native manifest planner, so this is a cross-backend compatibility regression rather than an isolated ZIP-path concern.
Findings	1 actionable finding	[P1] Track collisions after filtering accepted entries
Security	None	None.

How this fits together

The archive subsystem takes untrusted ZIP or TAR entries, validates names and resource limits, then writes accepted entries through a staged destination or returns a bounded entry read. Its policy must remain consistent across JavaScript ZIP, JavaScript TAR, and native extraction paths because callers use filters to decide which validated entries are actually published.

flowchart LR
  A[Untrusted ZIP or TAR] --> B[Archive preflight]
  B --> C[Path and archive-wide limits]
  C --> D[Caller entry filter]
  D --> E[Accepted output plan]
  E --> F[Collision and integrity checks]
  F --> G[Staged extraction or bounded read]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The PR body reports automated checks and test counts, but no inspectable after-fix live transcript, recording, linked artifact, or redacted runtime log demonstrates malformed archive rejection and an unchanged destination. After adding redacted proof and updating the PR body, ClawSweeper should re-review automatically; otherwise a maintainer can request @clawsweeper re-review.
• Track collisions after filtering accepted entries (P1) - onFiltered: "skip-entry" is a supported selective-restore mode, but this records every stripped path before shouldExtractArchiveEntry runs. A skipped one/value.txt can therefore collide with an accepted or skipped two/value.txt and reject an archive despite creating no conflicting output. Move tracking after the policy decision in this path and the matching TAR/native paths.
• Resolve merge risk (P1) - Merging as written breaks the documented onFiltered: "skip-entry" workflow when skipped paths collide after stripComponents, even though those entries are never written.
• Resolve merge risk (P1) - The PR body lists checks but has no inspectable after-fix runtime transcript, recording, artifact, or redacted log demonstrating malformed archive rejection and clean destination behavior.
• Complete next step (P2) - The code repair is mechanically clear, but this external PR also requires contributor-provided real behavior proof before a maintainer can merge it.
• Improve patch quality - Move output-collision tracking after accepted-entry filtering in ZIP, TAR, and native extraction, with a cross-backend regression test.
• Improve patch quality - Post redacted terminal or runtime evidence showing a malformed archive is rejected without destination debris after the repair.

Findings

• [P1] Track collisions after filtering accepted entries — src/archive.ts:259

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch surface	18 files affected; 372 added, 29 removed	The branch changes public archive policy across JavaScript ZIP, JavaScript TAR, native extraction, bounded reads, docs, and tests.
Affected backends	3 extraction paths	The same filter-order regression appears in ZIP, JavaScript TAR, and native manifest planning.

Merge-risk options

Maintainer options:

1. Preserve filtered-entry compatibility (recommended)
   Move output-collision tracking after the accepted-entry policy decision in all three extraction paths and add coverage showing skipped colliding paths do not reject the archive.
2. Pause the hardening change
   Do not merge until the documented selective-restore behavior and contributor-side runtime proof are both restored.

Technical review

Best possible solution:

Keep archive-wide path validation and entry counting before filtering, but record output collisions only after an entry is accepted for extraction in the ZIP, TAR, and native plans; add a cross-backend regression test for skipped colliding entries.

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

Yes — source establishes a direct reproduction: use stripComponents, make two paths collide after stripping, and return "skip" with onFiltered: "skip-entry" for one or both. The tracker throws before the filter in every extraction backend, although this read-only review did not execute the test.

Is this the best way to solve the issue?

No — the hardening should reject collisions among accepted outputs, not entries explicitly omitted by the supported filter contract. Moving tracking after policy acceptance preserves path and archive-wide limit checks while restoring selective extraction.

Full review comments:

• [P1] Track collisions after filtering accepted entries — src/archive.ts:259
onFiltered: "skip-entry" is a supported selective-restore mode, but this records every stripped path before shouldExtractArchiveEntry runs. A skipped one/value.txt can therefore collide with an accepted or skipped two/value.txt and reject an archive despite creating no conflicting output. Move tracking after the policy decision in this path and the matching TAR/native paths.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4696341685a9.

Labels

Label justifications:

• P1: This PR would regress the documented selective archive-restore workflow for existing callers using onFiltered: "skip-entry".
• merge-risk: 🚨 compatibility: The new collision policy rejects archives that callers previously could safely process by filtering the colliding entries out.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports automated checks and test counts, but no inspectable after-fix live transcript, recording, linked artifact, or redacted runtime log demonstrates malformed archive rejection and an unchanged destination. After adding redacted proof and updating the PR body, ClawSweeper should re-review automatically; otherwise a maintainer can request @clawsweeper re-review.

Evidence

What I checked:

• Documented filter contract: The archive API explicitly supports onFiltered: "skip-entry" so callers can omit selected validated entries; the documentation’s restore example uses that mode to omit regenerated cache paths while retaining the rest of the archive. (docs/archive.md:76, 4696341685a9)
• ZIP regression introduced by the patch: The ZIP extractor records an output path before it calls shouldExtractArchiveEntry; therefore two paths that collide after stripping reject even when the filter skips one or both entries and no output would be created. (src/archive.ts:259, e1750181f2ae)
• Equivalent ordering exists in TAR and native paths: The new tracker is likewise invoked before filter evaluation in the JavaScript TAR checker and native manifest planner, so this is a cross-backend compatibility regression rather than an isolated ZIP-path concern. (src/archive-tar.ts:85, e1750181f2ae)
• Current-main and provenance check: The proposed commit is based on current main; the output-collision tracker is not present on the checked-out base, so the central change is neither already implemented nor superseded on current main. (src/archive.ts:230, 4696341685a9)
• Feature-history routing: Current archive helper lines trace to the v0.5.1 release commit authored by Peter Steinberger; the proposed commit is also authored by Peter Steinberger, matching the steipete GitHub identity in PR metadata. (src/archive-entry.ts:13, 16e1bd489ae8)

Likely related people:

• steipete: Git history attributes the current archive implementation to Peter Steinberger in the v0.5.1 release, and the same person authored the proposed archive-hardening commit. (role: original archive implementation and recent area contributor; confidence: high; commits: 16e1bd489ae8, e1750181f2ae; files: src/archive-entry.ts, src/archive.ts, src/archive-tar.ts)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-08-02T23:04:21.074Z sha 4d40aaf :: needs real behavior proof before merge. :: none
• reviewed 2026-08-02T23:38:25.252Z sha 41b698d :: needs real behavior proof before merge. :: none