Skip to content

fix: harden path and archive parser edge cases - #119

Merged
steipete merged 5 commits into
mainfrom
test/property-fuzz
Aug 3, 2026
Merged

fix: harden path and archive parser edge cases#119
steipete merged 5 commits into
mainfrom
test/property-fuzz

Conversation

@steipete

@steipete steipete commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add deterministic fast-check properties for FileStore key containment, canonical spelling, sync/async agreement, rejection side effects, and portable aliasing
  • add structured TAR/ZIP fuzzing for numeric field maxima, truncation, declared-size mismatches, hostile and colliding names, every extraction-limit boundary, and native/JavaScript decision equivalence
  • fix the minimized parser, path-alias, and limit-accounting bugs exposed by those properties

Every property uses seed 0x5eedc0de, bounded case counts, verbose counterexamples, and fast-check shrink paths for direct replay.

Findings fixed

  • reject non-canonical FileStore keys such as repeated/dot segments, non-NFC spellings, and Windows trailing-dot/space aliases before filesystem mutation
  • validate all 96 TAR base-256 size bits in the native meter instead of ignoring the high-order bytes
  • map raw node-tar and JSZip/native parser failures to the documented archive-header-invalid code
  • normalize accepted backslash TAR names before JavaScript extraction so validation and output use the same spelling
  • reject empty, NUL-truncated, separator-terminated, parent-aliased, and overlong portable archive names before filesystem access
  • accept valid zero-byte ZIP files while retaining size/CRC integrity checks
  • reset JavaScript TAR per-entry byte accounting between entries so exact maxEntryBytes boundaries do not accumulate across files

All minimized regressions were confirmed against an untouched origin/main; the native high-order base-256 fixture was accepted there as a zero-byte file.

Verification

  • pnpm check — 1018 passed, 61 skipped
  • pnpm test:security — 70 passed
  • pnpm native:test — 18 passed
  • native-enabled focused property suite — 29 passed
  • git diff --check
  • autoreview (Codex, gpt-5.6-sol, high) — clean, no accepted/actionable findings

@steipete
steipete requested a review from a team as a code owner August 3, 2026 06:49
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 3, 2026
@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 3, 2026, 4:30 AM ET / 08:30 UTC.

ClawSweeper review

What this changes

The branch hardens FileStore key validation and TAR/ZIP parser error handling and limits, with deterministic property-based regression tests for path aliases and malformed archives.

Merge readiness

Blocked until real behavior proof is added - 8 items remain

Keep open: the archive hardening is substantial, but this PR still makes previously valid FileStore key spellings inaccessible in the released public API. Likely related people: Peter Steinberger (high confidence, current FileStore and archive-area history).

Priority: P1
Reviewed head: 6086d074c7e73a059d3eccb0940f08db46d547ba
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The archive work has useful automated coverage, but a P1 compatibility regression and missing real-behavior proof leave this PR unready to merge.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body reports automated checks only; add redacted after-fix terminal output, runtime logs, or a linked artifact that directly exercises the changed behavior, then update the PR body for a fresh 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 only; add redacted after-fix terminal output, runtime logs, or a linked artifact that directly exercises the changed behavior, then update the PR body for a fresh review.
Evidence reviewed 6 items Released FileStore behavior accepts safe aliases: Current main only trims FileStore keys and rejects drive-relative segments before passing them to the shared safe-relative resolver, whose segment handling removes empty and dot segments. Therefore keys such as cache//entry and cache/./entry remain safely reachable on the released path.
PR rejects those existing spellings before resolution: The proposed canonical-key check rejects empty and dot path segments, NFC-decomposed names, and segments ending in a dot or space before any FileStore operation can reach the existing safe resolver.
Earlier P1 remains unfixed: The FileStore source blob at the prior reviewed head and the current head is identical, so the previously reported compatibility blocker is still present rather than introduced by the final test-only commits.
Findings 1 actionable finding [P1] Preserve existing FileStore key spellings
Security None None.

How this fits together

FileStore converts application-managed relative keys into paths confined under a trusted root, while archive readers validate untrusted entry names and limits before extraction. Both surfaces sit before guarded filesystem operations, so their accepted spellings and error contracts affect callers and existing stored data.

flowchart LR
  A[Application keys and archives] --> B[FileStore and archive validation]
  B --> C{Accepted input?}
  C -->|No| D[Documented error]
  C -->|Yes| E[Root confinement and limits]
  E --> F[Safe read or extraction]
Loading

Decision needed

Question Recommendation
Should FileStore preserve all safely confined historical key spellings in this release, or should canonical-key enforcement be introduced only with an explicit versioned migration or new API? Preserve existing key reachability: Remove the new FileStore canonical-spelling rejection from this PR and add regression coverage for safely confined historical aliases.

Why: The patch intentionally changes a released public storage-key contract; code review cannot determine whether breaking access to existing persisted keys is acceptable product policy.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body reports automated checks only; add redacted after-fix terminal output, runtime logs, or a linked artifact that directly exercises the changed behavior, then update the PR body for a fresh review.
  • Preserve existing FileStore key spellings (P1) - This remains the earlier P1 blocker: current main safely resolves aliases such as cache//entry, cache/./entry, and decomposed Unicode names, but this pre-resolution check rejects them. Existing v0.5.2 stores using those keys can no longer read, write, or remove their files; remove this breaking validation or provide an explicit versioned migration/new API.
  • Resolve merge risk (P1) - Merging would make keys already accepted by v0.5.2—such as dot-segment, repeated-slash, decomposed-Unicode, and trailing-dot/space spellings—unreadable, undeletable, or unwritable through FileStore without a migration path.
  • Resolve merge risk (P1) - The JavaScript/native archive changes span 20 files and are backed by automated checks, but there is no inspectable after-fix runtime artifact to confirm the claimed boundary behavior outside the test harness.
  • Complete next step (P2) - A P1 released-API compatibility choice and missing contributor-run proof require human review; this should not enter an automated repair lane.
  • Improve patch quality - Preserve safely confined historical FileStore key spellings or introduce a documented versioned migration/new API.
  • Improve patch quality - Post redacted live after-fix proof for the affected FileStore and archive paths, then update the PR body to trigger re-review.

Findings

  • [P1] Preserve existing FileStore key spellings — src/file-store.ts:115-120
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 20 files; source +219/-60, tests +790, metadata/docs +18 The broad JavaScript/native parser and FileStore change makes focused compatibility proof important before merge.

Merge-risk options

Maintainer options:

  1. Keep stored keys reachable (recommended)
    Remove or defer FileStore canonical-key rejection and add focused compatibility tests for safely confined aliases before merge.
  2. Accept a versioned breaking change
    If canonical-only keys are intentional, require an explicit migration or new API plus documented upgrade behavior before accepting the compatibility break.

Technical review

Best possible solution:

Retain existing FileStore key reachability in this release and land the archive parser fixes with direct live proof; move portable canonical-key enforcement to a documented, versioned migration or an explicit new API rather than silently changing existing keys.

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

Yes—source reproducible with high confidence: current main safely normalizes empty and dot segments in relative paths, while this PR rejects those spellings before FileStore can resolve an existing key.

Is this the best way to solve the issue?

No: archive hardening is a reasonable direction, but silently rejecting historical FileStore key spellings is not the narrowest compatible fix; preserve them or introduce canonical enforcement through a versioned migration or new API.

Full review comments:

  • [P1] Preserve existing FileStore key spellings — src/file-store.ts:115-120
    This remains the earlier P1 blocker: current main safely resolves aliases such as cache//entry, cache/./entry, and decomposed Unicode names, but this pre-resolution check rejects them. Existing v0.5.2 stores using those keys can no longer read, write, or remove their files; remove this breaking validation or provide an explicit versioned migration/new API.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5f528c322461.

Labels

Label justifications:

  • P1: The patch can break normal FileStore reads and writes for existing released consumers with persisted non-canonical keys.
  • merge-risk: 🚨 compatibility: New pre-resolution key rejection changes the set of FileStore keys accepted by the released public API.
  • 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 only; add redacted after-fix terminal output, runtime logs, or a linked artifact that directly exercises the changed behavior, then update the PR body for a fresh review.

Evidence

What I checked:

  • Released FileStore behavior accepts safe aliases: Current main only trims FileStore keys and rejects drive-relative segments before passing them to the shared safe-relative resolver, whose segment handling removes empty and dot segments. Therefore keys such as cache//entry and cache/./entry remain safely reachable on the released path. (src/file-store.ts:104, 32759e8923a1)
  • PR rejects those existing spellings before resolution: The proposed canonical-key check rejects empty and dot path segments, NFC-decomposed names, and segments ending in a dot or space before any FileStore operation can reach the existing safe resolver. (src/file-store.ts:115, 6086d074c7e7)
  • Earlier P1 remains unfixed: The FileStore source blob at the prior reviewed head and the current head is identical, so the previously reported compatibility blocker is still present rather than introduced by the final test-only commits. (src/file-store.ts:115, 6086d074c7e7)
  • Public contract and release provenance: The package documents FileStore as handling safe relative paths, and current main includes release commit 32759e8923a121307d499c75c4e771c7307cf828, tagged v0.5.2; the proposed rejection would therefore affect existing released consumers rather than only new inputs. (README.md:360, 32759e8923a1)
  • No inspectable real-behavior proof: The PR body reports checks and property-suite totals but provides no terminal capture, runtime log, recording, or linked artifact showing the after-fix behavior in a real setup. (6086d074c7e7)
  • Area provenance: Current FileStore and archive history is consistently authored by Peter Steinberger, including the v0.5.2 release and the prior hardened atomic-replace work affecting these paths. (src/file-store.ts:104, 66e59e0ce95e)

Likely related people:

  • Peter Steinberger: Git history for the current FileStore and archive-validation paths, including the release and prior hardening work, consistently identifies this contributor. (role: feature owner and recent area contributor; confidence: high; commits: 32759e8923a1, 66e59e0ce95e, 5ddca800c6aa; files: src/file-store.ts, src/archive-entry.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 (3 earlier review cycles)
  • reviewed 2026-08-03T06:57:02.462Z sha a76190a :: needs real behavior proof before merge. :: [P1] Preserve access to existing non-canonical FileStore keys
  • reviewed 2026-08-03T07:07:48.095Z sha 9bfc4bf :: needs real behavior proof before merge. :: [P1] Preserve compatibility for existing FileStore key spellings
  • reviewed 2026-08-03T08:04:10.122Z sha 9bfc4bf :: needs real behavior proof before merge. :: [P1] Preserve existing FileStore key spellings

@steipete
steipete force-pushed the test/property-fuzz branch from a76190a to 9bfc4bf Compare August 3, 2026 07:03
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 3, 2026
@steipete
steipete merged commit ed06666 into main Aug 3, 2026
22 checks passed
@steipete
steipete deleted the test/property-fuzz branch August 3, 2026 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant