fix(archive): apply tar strip components once - #142
Conversation
JavaScript TAR extraction stripped leading path components twice, under two different rules. tar.x() received strip: N, and the filter separately computed stripArchivePath(info.path, N) for the accepted-entry plan. stripArchivePath() drops empty and "." components before slicing; node-tar's CHECKPATH does a raw split and splices N off whatever it finds. For an entry named ./pkg/hello.txt with stripComponents: 1, node-tar wrote pkg/hello.txt while fs-safe recorded hello.txt, so the post-extraction mode pass failed with ENOENT on a path that was never created, and the entry filter, path-component limit, and collision tracker all ran against a path other than the one on disk. The native backend uses stripArchivePath(), so the two backends also produced different trees for the same archive. The filter now assigns the already-stripped path back to the entry, and tar.x() is given strip: 0. node-tar writes exactly the path fs-safe validated, limited, collision-checked, and chmods. Entries that strip away to nothing are still skipped -- the preflight checker already rejected them for the same reason.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs maintainer review before merge. Reviewed August 23, 2026, 12:55 PM ET / 16:55 UTC. ClawSweeper reviewWhat this changesThe PR makes JavaScript TAR extraction apply Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readinessThis PR remains necessary: current main still gives node-tar a separate strip count after fs-safe has planned the stripped path, while the branch makes that path authoritative and adds focused parity coverage. No actionable correctness or security regression was found. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (partial) — step 2 Assertions:
How this fits togetherArchive extraction validates untrusted TAR paths, plans safe staged outputs, writes the files, applies modes, and then merges the staged tree into the destination. JavaScript and native extraction must apply the same path, collision, depth, and mode policies. flowchart LR
A[Untrusted TAR archive] --> B[Validate entry path]
B --> C[Strip real path components]
C --> D[Check depth and collisions]
D --> E[Write into private staging]
E --> F[Apply entry modes]
F --> G[Safely merge destination]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the single-authoritative-path approach with the included parity regression so both backends keep applying archive policy to the path actually written. Do we have a high-confidence way to reproduce the issue? Yes—source reproducible with a TAR entry named Is this the best way to solve the issue? Yes—the patch removes the duplicate strip implementation rather than changing validation or containment policy, making one fs-safe-normalized path govern both backends and the post-extraction mode pass. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 650e0162c20b. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
Native parity is already covered on this head, so I want to point at it rather than leave it as an open item. The regression went into - name: Test native security and path equivalence
run: pnpm test test/native-integration.test.ts test/native-write-containment.test.ts
test/native-archive-equivalence.test.ts test/native-publish-equivalence.test.ts
test/private-directory.test.tsThat job is green on So the same fixture — Worth stating explicitly why it holds: the native path never had the defect. |
1 participant
What Problem This Solves
Fixes an issue where consumers calling
extractArchive()withstripComponentson a TAR whose members are./-prefixed would have the whole extraction fail with a rawENOENT, and — when the native binding is present — would get a different output tree than the JavaScript backend produces for the same bytes. The affected surface is archive extraction: path validation, entry filtering, the path-component limit, the collision tracker, and mode policy../-prefixed members are not exotic.tar cf pkg.tar .andtar czf pkg.tgz -C dir .both write them, andstripComponentsis exactly what a consumer reaches for when unpacking such an archive.The cause is that stripping happened twice, under two different rules.
tar.x()received the strip count:and the
filtercallback separately computed its own stripped path for the accepted-entry plan:The two disagree on empty and
.components.stripArchivePath()drops them before slicing (src/archive-entry.ts:80-93):node-tar does not.
[CHECKPATH]splits raw and splices (node_modules/tar/dist/esm/unpack.js:261-278, tar 7.5.22):normalizeArchiveEntryPath()only converts backslashes, so the leading./survived into node-tar's split. For./pkg/hello.txtwithstripComponents: 1:<staging>/pkg/hello.txthello.txtinacceptedEntriessrc/archive.ts:444-451then calledfs.chmod(<staging>/hello.txt, ...)Reproduced directly, before the fix:
Two consequences beyond the crash are worth stating separately:
Policy ran against a path other than the one written.
entryFiltersees the entry, andassertArchiveEntryPathComponentsWithinLimit()andcreateArchiveOutputPathTracker()both key on the fs-safe-stripped path (src/archive-tar.ts:82-89), while node-tar wrote a longer one. The depth limit was therefore measured one component short, and two entries could be judged non-colliding under one spelling and written under another. Containment was never at risk — extraction goes into a private staging tree that is merged back through the normal boundary checks — but the policy decisions did not describe the bytes on disk.The backends disagreed.
src/archive-native.ts:107feedsstripArchivePath()into the Rust plan, so the native backend placed the file athello.txt. Same archive, same options, two different trees depending on whether a prebuilt binding was loaded.Why This Change Was Made
fs-safe now owns stripping outright. The
filtercallback assigns the already-stripped path back to the entry, andtar.x()is givenstrip: 0:This works because
filteris a Parser option (node_modules/tar/dist/esm/parse.js:233) and runs beforeUnpack[ONENTRY]→Unpack[CHECKPATH].Unpack extends Parser, so the mutation the filter makes is exactly what[CHECKPATH]andentry.absolutesee.stripArchivePath()becomes the single authoritative strip rule for both backends, which is whattest/extracted-helpers.test.ts:125-126already pins.Behavior that was checked and is deliberately unchanged:
createTarEntryPreflightChecker()hard-rejectsSymbolicLink,Link, and device/FIFO/socket types (src/archive-tar.ts:103-108) beforeUnpackever sees the entry, so node-tar'sentry.type === 'Link'linkpath-rewrite branch was already unreachable here. Droppingstripremoves no live behavior../root member, or a path with fewer real components thanstripComponents) are still skipped rather than crashing. The preflight checker already returnsfalsefor exactly this case (src/archive-tar.ts:79-82), so theif (!relPath) return falseguard in the filter is defensive narrowing, not a new rule.maxDepth. node-tar now measures depth on the stripped path, which has the same or fewer components than before, so it can only become less likely to fire. fs-safe's own 256-component limit remains the binding constraint.[STRIPABSOLUTEPATH]. Unreachable either way:relPathhas already passedvalidateArchiveEntryPath(), which rejects absolute paths,//,.., drive letters, NUL, and (on win32) ADS spellings. Understrict: truethis makes theTAR_ENTRY_INFOwarning strictly less reachable, not more.strip: 0is passed explicitly rather than omitted becauseTarExtractOptionsinsrc/archive-tar-runtime.ts:23declaresstripas required.Non-goals: ZIP extraction already resolves its own output path and is untouched.
stripArchivePath()itself is unchanged — the bug was in having a second, different implementation, not in this one.User Impact
Extracting a
./-prefixed TAR withstripComponentsnow succeeds and places files wherestripComponentssays, instead of failing withENOENTduring the mode pass. The JavaScript and native backends now produce the same tree for the same archive, and the entry filter, path-component limit, and collision tracker now apply to the path that is actually written.For archives without
.or empty path components — including everythingtar.c()produces, and thepackage/-prefixed layout npm and GitHub tarballs use — output paths are byte-identical to before, because the two strip rules already agreed there. That is also why no existing test needed updating.One behavior worth calling out for anyone who was working around the old shape: an entry whose stripped path is empty is skipped rather than extracted at its unstripped location. That was already true — the preflight checker filtered it — so this is a restatement, not a change.
Evidence
One regression test in
test/native-archive-equivalence.test.ts, added next to the two existingstripComponentstests insidedescribe.each(archiveBackends)so it asserts backend parity. That suite falls back to["javascript"]when no binding is built and is re-run with a real binding by the CI native job, so the parity claim is enforced in CI even though I have no Rust toolchain locally.The fixture is the exact shape
tar cf pkg.tar .writes:Before the fix:
After:
The mode assertion is the load-bearing one. It only passes if the mode pass resolved a path that exists, which is the same thing as saying
acceptedEntriesand the extracted tree agree — the exact invariant that was broken.Every existing
stripComponentsandstripArchivePathsite was checked and none needed changing:test/api-coverage.test.ts:499tar.c(...), so members arepackage/...with no.component; both rules already agreedtest/archive.test.ts:132test/coverage-gaps.test.ts:161test/native-archive-equivalence.test.ts:208test/native-archive-equivalence.test.ts:234entry.pathassignmenttest/extracted-helpers.test.ts:125-126stripArchivePath()unit tests; the helper is unchangedtest/property-fuzz-stress.test.ts:152No test in the repository extracted a
./-prefixed or//-containing TAR withstripComponents > 0. The two./-prefixed fixtures that exist (test/native-archive-equivalence.test.ts:88,test/archive-read-boundaries.test.ts:294) go throughreadArchiveEntry()rather thanextractArchive(). That gap is why this stayed latent.Archive suites after the fix:
Native parity, confirmed by CI on this head
The regression sits inside
describe.each(archiveBackends), and.github/workflows/ci.ymlruns that file against a real binding afterpnpm native:build:Green on
326e815across every native target:So the same fixture asserted
readdir === ["hello.txt"]under the shipped binding as well as under the JavaScript fallback. The native path never had the defect —src/archive-native.ts:107already fedstripArchivePath()into the Rust plan — and this PR changes no Rust. The parity test exists so the two cannot drift apart again, which is the failure modedocs/archive.md:153warns about.Full
pnpm checkon Windows 11 / Node 22.20.0 / pnpm 10.34.5:No export moves, so
test/public-api.jsonandcheck-packare untouched. Thedocs/archive.mdaddition is prose, whichscripts/check-doc-examples.mjsdoes not execute; it states the component-counting rule and that the JavaScript TAR path hands node-tar the already-stripped path.The one red test is pre-existing and unrelated
file-lock-sync-failure.test.ts > fails closed when a synchronous lock parent cannot match the Root canonical pathfails on this machine on a cleanorigin/maincheckout, before any change from this PR. It is environment-dependent: the test usesuseTempDirs(), which returns the rawmkdtemp()path without a realpath, and then asserts that a lock path whose spelling cannot matchlockRoot.rootRealis rejected. That mismatch only exists when the ambient temp directory is not already canonical — true on the hosted Windows runner (C:\Users\RUNNER~1\...), false on a Windows account whose profile directory needs no 8.3 alias. Nothing in this diff touches locking.CHANGELOG.mdupdated when release-relevant