| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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@fengmk2, you've reached your PR review limit, so we couldn't start this review. 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. 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: 01a23c17-2a42-44a6-b223-9bd276ecb8fd 📥 CommitsReviewing files that changed from the base of the PR and between 0039152 and d6ef46d. 📒 Files selected for processing (1)
WalkthroughArchive 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: copilot 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 96.61017% with 2 lines in your changes missing coverage. Please review.
@@ 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.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8f2be0a-76b3-4b6c-b7d7-cb98c2285637
📥 CommitsReviewing files that changed from the base of the PR and between 0a77278 and 848556a.
📒 Files selected for processing (3)
Sorry, something went wrong.
There was a problem hiding this comment.
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:
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.
Sorry, something went wrong.
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.
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.
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.
[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 📦🚀 |
Sorry, something went wrong.
….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.
[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)
| Back | FazBrowse Home | New Git URL |
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 so a chain realpath() 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