fix: resolve symlink chains fully when extracting - #140
Merged
Conversation
isRealPathSafe() stopped walking as soon as realpath() failed on a dangling link, checking only that link's immediate target. A destination reached through several hops, or through a linked directory, was therefore only partially resolved, and the entry could land somewhere the check had not accounted for. Resolve the remaining hops by hand instead, bounded by MAX_SYMLINK_DEPTH so a chain realpath() cannot see does not recurse without end. File entries no longer write through a symlink sitting at the destination. The link is replaced by the entry, which is how tar(1), node-tar, tar-fs and libarchive all behave. On platforms that have it, the write also opens with O_NOFOLLOW so the destination is never resolved through a link. Linked directories inside the extraction directory are still traversed, so entries written beneath them land where they always did.
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughArchive extraction now limits recursive symlink resolution, rejects unsafe chains, removes destination symlinks before file writes, and uses no-follow flags where supported. Tests cover tar, tgz, and zip extraction scenarios. ChangesSymlink-safe extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArchiveExtraction
participant isRealPathSafe
participant DestinationFilesystem
ArchiveExtraction->>isRealPathSafe: validate recursive symlink target
isRealPathSafe->>DestinationFilesystem: resolve target components
DestinationFilesystem-->>isRealPathSafe: resolved path or cycle
isRealPathSafe-->>ArchiveExtraction: allow or reject extraction
ArchiveExtraction->>DestinationFilesystem: unlink destination symlink
ArchiveExtraction->>DestinationFilesystem: open file with no-follow flags
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #140 +/- ##
==========================================
+ Coverage 96.41% 97.15% +0.74%
==========================================
Files 19 19
Lines 1145 1197 +52
Branches 294 309 +15
==========================================
+ Hits 1104 1163 +59
+ Misses 41 34 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/utils.js`:
- Around line 58-59: Update the JSDoc for the function documented by the depth
parameter in lib/utils.js, changing the return annotation from `@returns` to the
configured `@return` tag while preserving its Promise<boolean> description.
- Around line 84-91: Update the recursive dangling-symlink handling in
isRealPathSafe so it selects parentDir or realParentDir based on which namespace
contains absTarget before computing the next relative path and current entry.
Use that matching extraction root for the recursive call, preserving the
existing safety check and depth increment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8f2be0a-76b3-4b6c-b7d7-cb98c2285637
📒 Files selected for processing (3)
lib/utils.jstest/tar/symlink-resolution.test.jstest/util.js
There was a problem hiding this comment.
Pull request overview
This PR hardens archive extraction against path traversal via dangling symlink chains by continuing resolution hop-by-hop when realpath() can’t fully resolve the chain, and by changing file writes to replace an existing destination symlink rather than writing through it.
Changes:
- Add bounded recursive symlink-target walking (
MAX_SYMLINK_DEPTH) to ensure dangling symlink chains are fully accounted for during safety checks. - Ensure file extraction replaces an existing destination symlink (and uses
O_NOFOLLOWwhere available) to avoid writing through symlinks. - Add cross-format tests (tar/tgz/zip) covering multi-hop chains, linked-directory targets, cycles, and linked-directory traversal behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/utils.js |
Implements bounded hop-by-hop symlink resolution and adjusts file writing to avoid following destination symlinks. |
test/util.js |
Adds a ZIP buffer helper used to exercise zip extraction behavior in tests. |
test/tar/symlink-resolution.test.js |
Adds regression tests for symlink-chain resolution and destination-symlink replacement behavior across formats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The recursive walk always computed its relative path from parentDir. When a dangling link named its target in the real namespace, as with /var against /private/var on macOS, that relative path climbed out through '..' and the walk rejected a target that was in fact inside the extraction directory, so the entry was skipped. Pick the root that actually contains the target before walking, and fail closed when neither does. Adds a regression test that builds the two namespaces itself rather than relying on the host having a symlinked temp directory. Also settle the promise in createZipBuffer() when called with no entries, since an empty archive never finalizes, and register its listeners before adding entries.
Windows resolves the dangling link in that setup differently and skips the entry, which predates this change. The /var against /private/var divergence the test covers is a POSIX shape, and macOS and Linux still exercise it.
fengmk2
added a commit
that referenced
this pull request
Aug 5, 2026
Stacked on #140, so the diff here is just the workflow change. Node 26 is the current release line (26.6.0, released 2026-08-03) and becomes LTS in October. Running it now surfaces breakage before the promotion rather than after. `engines` stays at `>= 18`, so this only widens what CI covers.
fengmk2
added a commit
that referenced
this pull request
Aug 5, 2026
Backport of #140 to 1.x. `isRealPathSafe()` stopped walking as soon as `realpath()` failed on a dangling link, checking only that link's immediate target. A destination reached through several hops, or through a linked directory, was only partially resolved, so an entry could land somewhere the check had not accounted for. It now resolves the remaining hops itself, bounded by `MAX_SYMLINK_DEPTH`, and walks from whichever extraction root actually contains the target so a link named in the real namespace is not rejected. Behaviour change worth noting: a file entry landing on a symlink now replaces that link instead of writing through to whatever it points at. This matches tar(1), node-tar and libarchive. Where the platform has it, the write also opens with `O_NOFOLLOW`. Linked directories inside the extraction directory are still traversed. Written in the callback style the surrounding 1.x code uses, so it stays compatible with the branch's Node range. Suite is 166 passing on this branch.
fengmk2
pushed a commit
that referenced
this pull request
Aug 5, 2026
fengmk2
pushed a commit
that referenced
this pull request
Aug 5, 2026
[skip ci] ## <small>2.1.2 (2026-08-05)</small> * test: use node:crypto randomUUID instead of uuid dependency (#143) ([c0d269f](c0d269f)), closes [#143](#143) [#139](#139) * ci: add Node.js 26 to the test matrix (#141) ([5c725e6](5c725e6)), closes [#141](#141) [#140](#140) * fix: resolve symlink chains fully when extracting (#140) ([72a3c84](72a3c84)), closes [#140](#140)
|
🎉 This PR is included in version 2.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fengmk2
added a commit
that referenced
this pull request
Aug 5, 2026
….3.1 (#145) Fixes the Node 26 CI failure on master, and moves both zip dependencies to their upstream, maintained versions. ## yauzl: the Node 26 fix `@eggjs/yauzl` depends on `fd-slicer2`, whose `ReadStream` loses data when piped on Node 26. Any zip entry over the 64 KiB `highWaterMark` delivers roughly the first chunk and then stalls, with no `end`, no `error`, no `close`. That is why `zip.uncompress()` hangs until the 60s timeout on Node 26 while passing on 18 through 24. Not our code: released 2.1.1 reproduces it identically. Reported upstream at node-modules/yauzl#3. `yauzl@3.4.0` dropped `fd-slicer` entirely (only dependency is now `pend`) and does not have the bug. The fork was adopted for `decodeStrings: false` so absolute paths survive `validateFileName`. I checked that still holds against the `contain-absolute-path.zip` fixture rather than assuming: | | @eggjs/yauzl 2.11.0 | upstream 3.4.0 | | --- | --- | --- | | entries | 31 | 31 | | `fileName` is Buffer | 31 | 31 | | `externalFileAttributes` present | 31 | 31 | | files read | 21 | 21 | | leading `/` entry | preserved | preserved | Only visible difference: yauzl 3 capitalises the "End of central directory record signature not found" message, so that assertion is now case-insensitive. ## yazl 3 and the early-finalize bug it exposed yazl 3 turns "add entries after calling `end()`" from a tolerated no-op into a thrown error, and compressing trips it immediately. `_onEntryFinish()` finalizes as soon as the entry queue is momentarily empty. For zip the finish callback runs synchronously, so a caller doing: ```js zipStream.addEntry(streamA, ...); zipStream.addEntry(bufferB, ...); ``` closed the archive after the first entry, and the second threw. Tar avoids it only because its `fs.stat` makes the callback async, which lets the later entries queue first. Worth being precise about the old behaviour: **yazl 2 did not drop those entries.** I checked, and the produced archive contained all of them. So this was latent, not a live data-loss bug. Fix is to finalize on the next tick and skip it if an entry arrived meanwhile. Verified the produced archive still contains every entry. Residual limitation, unchanged in spirit from before: entries added after a longer async gap still finalize early. That is the existing drain heuristic, and giving the stream an explicit "done adding" call would be an API change worth doing separately. ## Result **171 passing on both Node 24 and Node 26**, lint and `tsc` clean. On Node 26 the zip suite finishes in ~495ms where it previously hung for 60s. The symlink cases from #140 were re-checked through the new zip path and still block. Drops `fd-slicer2` and `buffer-crc32` from the tree. Supersedes #132.
fengmk2
pushed a commit
that referenced
this pull request
Aug 5, 2026
[skip ci] ## <small>2.1.3 (2026-08-05)</small> * fix: replace @eggjs/yauzl with upstream yauzl 3.4.0, update yazl to 3.3.1 (#145) ([572a0ba](572a0ba)), closes [#145](#145) [#140](#140) * chore(deps): update dependency @types/node to v24 (#123) ([3499eb2](3499eb2)), closes [#123](#123) * chore(deps): update dependency iconv-lite to ^0.7.0 (#122) ([e7ba2a5](e7ba2a5)), closes [#122](#122)
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
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.
isRealPathSafe()stopped walking as soon asrealpath()failed on a dangling link, checking only that link's immediate target. A destination reached through several hops, or through a linked directory, was only partially resolved, so an entry could land somewhere the check had not accounted for.It now resolves the remaining hops itself, bounded by
MAX_SYMLINK_DEPTHso a chainrealpath()cannot see does not recurse without end.Behaviour change worth noting: a file entry landing on a symlink now replaces that link instead of writing through to whatever it points at. This matches tar(1), node-tar, tar-fs and libarchive. Where the platform has it, the write also opens with
O_NOFOLLOW. Linked directories inside the extraction directory are still traversed, so entries beneath them land where they always did.Tests cover chains of two and three hops, chains through a linked directory, cycles, and the traversal case, across tar, tgz and zip.
Summary by CodeRabbit
Bug Fixes
Tests