feat(archive): unify TAR parsing in Rust and WASM - #229
Open
steipete wants to merge 3 commits into
Open
Conversation
Share bounded TAR/PAX/GNU admission across native and portable WASM paths, then replay admitted ranges through existing guarded filesystem writers. Accept strict UTF-8 and newline PAX paths without downstream reinterpretation while preserving raw-field, traversal, link, collision, byte and EOF checks. Bundle the import-free WASM asset and keep native-off archives usable without optional dependencies or a consumer compiler. Preserve gzip member integrity while admitting only zero physical padding from system-tar stdout, retaining the first unused input offset across chunks. Verify real producer and installed-package behavior on macOS and Linux, including the cross-chunk gzip regression, native/WASM parity, and omitted optional-dependency installs. Document the common malformed-mode fallback.
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Add the portable parser target before benchmark, coverage collection and Windows hash-proof builds. Keep runtime, permissions, checks and coverage policy unchanged.
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed September 4, 2026, 11:55 PM ET / September 5, 2026, 03:55 UTC. ClawSweeper reviewWhat this changesThe PR shares one Rust TAR parser between native and bundled WebAssembly extraction, expands supported metadata, validates gzip padding, and updates packaging, documentation, and tests. Merge readiness⛔ Blocked before merge - 6 items remain This remains useful work that current main and v0.8.1 do not implement. No discrete blocking code defect was found, but compatibility acceptance and the previously requested behavior evidence remain unresolved. Priority: P2 Review scores
Verification
How this fits togetherfs-safe accepts untrusted archives and produces files beneath a guarded destination or returns a bounded entry buffer. The shared parser supplies validated names and payload ranges to the existing extraction policy and filesystem publication helpers. flowchart TD
A[Untrusted archive] --> B[Private staged input]
B --> C[Native or WebAssembly TAR parser]
C --> D[Path, filter and byte checks]
D --> E[Reject invalid archive]
D --> F[Replay admitted payload ranges]
F --> G[Guarded filesystem publication]
F --> H[Bounded entry buffer]
Decision needed
Why: The change is documented and intentional, but code correctness cannot determine whether that compatibility cost is acceptable. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep one bounded TAR admission core and the existing publication guards, with demonstrated upgrade behavior and an explicitly accepted malformed-mode permission contract. Do we have a high-confidence way to reproduce the issue? Not applicable as a single bug reproduction: this is a parser consolidation and capability expansion. The permission transition is source-visible, but no reviewer-side runtime reproduction was executed. Is this the best way to solve the issue? The shared parser is a maintainable way to eliminate downstream reinterpretation while retaining existing filesystem policy. Accepting its malformed-mode compatibility change remains a maintainer choice. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 899f91b97d26. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles) |
Keep ASCII component length validation equivalent to NFC/NFD without performing redundant Unicode normalization. Preserve the 255-byte boundary, Unicode normalization checks, path restrictions and manifest budgets. The identical 32-member near-limit WASM probe retains its overflow point while running about ten times faster. Install GNU tar in the Alpine validation container and provide the expected /usr/bin/tar executable path rather than skipping the bound producer proof. No test timeout, resource limit or coverage threshold is increased.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unify native and guarded JavaScript TAR parsing on one Rust core, compiled into the native binding and a bundled, import-free WASM module. TypeScript retains extraction policy and guarded JavaScript filesystem I/O;
off,auto, andrequireretain their availability/containment meanings.The old printable-ASCII PAX restriction compensated for genuine disagreement between node-tar and Rust tar: chunk-split UTF-8 could lose a pathname override, and embedded newlines could make effective payload sizes diverge. Both executors now consume byte-counted admitted member identities and payload ranges rather than asking a second TAR parser to reinterpret them.
Changes
archive-core; use that same crate from native andarchive-wasm.tar::Archivetraversal. npmtarand Rusttarremain development/fixture dependencies only.Real producer compatibility and security
The live producer proof exposed another boundary: BSD tar's
-czf - .stdout can include zero gzip-container padding, while file output omits it. Native now accepts that padding only after fully validated gzip members and only when every remaining physical byte is zero. The JavaScript route rejects nonzero bytes hidden after padding as well.Independent review found an additional cross-chunk case: Node's final
bytesWrittenis a cumulative count, not a physical offset after a skipped padding gap. Serialized input tracking now preserves the first unused physical offset, and both pipelines join teardown on failure. Regression fixtures cover an empty following member in a later chunk, plus legitimate concatenation at the same boundaries. CRC, ISIZE, headers, truncation, physical input budgets and decoded TAR EOF checks remain enforced.The producer smoke uses the actual bound-cwd/device/inode worker and unchanged
/usr/bin/tar -czf - .stdout, not a recompressed or metadata-suppressed substitute. Valid PAX/GNU metadata still does not excuse a raw fallback field truncated inside a UTF-8 sequence.Validation
offand 215/215 inrequire, on both macOS and Linux. Includes real system/npm producers, exact names and payloads, bounded reads, valid/invalid metadata, traversal, links, collisions, limits, ZIP smoke and empty destinations on pre-publication validation rejection. Native loading is verified; WASM runs from a neutral consumer environment.git diff --checkand workflow lint passed.An initial concurrent macOS consumer run hit three 10-second extraction deadlines. The unchanged harness passed when rerun serially; no timeout, limit or assertion was widened.
CI follow-ups install the WASM target in all package-build jobs and provide the expected system-tar path in Alpine. A near-limit ASCII manifest case now avoids redundant Unicode normalization: the identical 32-member/33rd-member-rejection probe improved from a 1.70-second median to 0.17 seconds without changing limits or deadlines.
Final head
7bd7540ce0f016f4b4d23ec6fb4eec4f537836ccpassed cross-platform CI and merged coverage, including Node 22/24, native Windows/macOS/Linux, musl and packed consumer checks. Benchmarks, CodeQL and the Windows hash-identity proof also passed.Repository proof entrypoints:
Scope
This does not change OpenClaw's directory projection, archive-authorization listing, wire schemas or active checkouts. It proves real producers into the installed fs-safe API, not a live Gateway/model session. Cross-platform CI remains the source of Windows and other-target proof. No version bump, publication or release is included.