fix: resolve symlink chains fully when extracting (#140)

`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.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
  * Improved archive extraction safety when handling symbolic links.
* Prevented symlink chains, cycles, and linked-directory paths from
escaping the extraction destination.
* Prevented extracted files from overwriting locations targeted by
existing symbolic links.
* Added protections against unsafe writes through symbolic links across
TAR, TGZ, and ZIP archives.

* **Tests**
* Added comprehensive coverage for symlink resolution, traversal,
cycles, and destination-link replacement scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Author: fengmk2

lib/utils.js

@@ -4,6 +4,33 @@ const fs = require('fs');
 const path = require('path');
 const { pipeline: pump } = require('stream');
 
+// Matches the kernel's own symlink chain limit closely enough to reject loops
+// that realpath() never sees, without rejecting any realistic layout.
+const MAX_SYMLINK_DEPTH = 32;
+
+// Numeric flags are accepted here per the "File system flags" section of the fs
+// docs, the same way node:zip opens with O_NOFOLLOW. The flag makes open() fail
+// with ELOOP when the final component is a symlink, so the write never resolves
+// one. It is undefined on Windows, where unlinkSymlink() below does the work.
+const NO_FOLLOW_WRITE_FLAGS = typeof fs.constants.O_NOFOLLOW === 'number'
+  ? fs.constants.O_NOFOLLOW | fs.constants.O_CREAT | fs.constants.O_TRUNC | fs.constants.O_WRONLY
+  : 'w';
+
+/**
+ * Remove a symlink sitting at the exact path an entry is about to be written to.
+ * Extraction replaces such a link rather than writing through to whatever it
+ * points at, which is how tar(1), node-tar and libarchive all behave.
+ * @param {string} target - Absolute path of the entry destination
+ */
+async function unlinkSymlink(target) {
+  try {
+    const stat = await fs.promises.lstat(target);
+    if (stat.isSymbolicLink()) await fs.promises.unlink(target);
+  } catch (e) {
+    if (e.code !== 'ENOENT') throw e;
+  }
+}
+
 /**
  * Check if childPath is within parentPath (prevents path traversal attacks)
  * @param {string} childPath - The path to check
@@ -28,16 +55,33 @@ function isPathWithinParent(childPath, parentPath) {
  * @param {string} targetPath - Absolute path to validate
  * @param {string} parentDir - Absolute path of the extraction root
  * @param {string} realParentDir - Pre-resolved real path of parentDir (handles OS-level symlinks like /var -> /private/var on macOS)
- * @returns {Promise<boolean>} true if safe, false if any segment escapes via symlink
+ * @param {number} depth - Recursion depth when re-walking a dangling symlink's target
+ * @return {Promise<boolean>} true if safe, false if any segment escapes via symlink
  */
-async function isRealPathSafe(targetPath, parentDir, realParentDir) {
+async function isRealPathSafe(targetPath, parentDir, realParentDir, depth = 0) {
+  // realpath() rejects long chains with ELOOP, but the dangling branch below resolves
+  // hop by hop without the kernel's help, so it needs its own bound.
+  if (depth >= MAX_SYMLINK_DEPTH) return false;
+
   function isWithinParent(p) {
     return isPathWithinParent(p, parentDir) || isPathWithinParent(p, realParentDir);
   }
 
-  const relative = path.relative(parentDir, targetPath);
+  // A link target may be written in either namespace when the two differ, as with
+  // /var -> /private/var on macOS. Walk from whichever root actually contains it,
+  // or the relative path below would climb out through '..' and reject a safe link.
+  let baseDir;
+  if (isPathWithinParent(targetPath, parentDir)) {
+    baseDir = parentDir;
+  } else if (isPathWithinParent(targetPath, realParentDir)) {
+    baseDir = realParentDir;
+  } else {
+    return false;
+  }
+
+  const relative = path.relative(baseDir, targetPath);
   const segments = relative.split(path.sep);
-  let current = parentDir;
+  let current = baseDir;
   for (const segment of segments) {
     if (!segment || segment === '.') continue;
     current = path.join(current, segment);
@@ -49,10 +93,14 @@ async function isRealPathSafe(targetPath, parentDir, realParentDir) {
           resolved = await fs.promises.realpath(current);
         } catch (e) {
           if (e.code === 'ENOENT') {
-            // Dangling symlink - check textual target
+            // Dangling symlink: realpath() gave up, so resolve the textual target
+            // ourselves. Checking the target string alone is not enough, because the
+            // target may itself be a symlink, or sit under a directory that is one,
+            // and both get resolved when the entry is actually written.
             const linkTarget = await fs.promises.readlink(current);
             const absTarget = path.resolve(path.dirname(current), linkTarget);
-            return isWithinParent(absTarget);
+            if (!isWithinParent(absTarget)) return false;
+            return await isRealPathSafe(absTarget, parentDir, realParentDir, depth + 1);
           }
           // Fail closed: unexpected errors during symlink resolution are unsafe
           return false;
@@ -197,8 +245,12 @@ exports.makeUncompressFn = StreamClass => {
           if (header.type === 'file') {
             const dir = path.dirname(destFilePath);
             await fs.promises.mkdir(dir, { recursive: true });
+            await unlinkSymlink(destFilePath);
             entryCount++;
-            pump(stream, fs.createWriteStream(destFilePath, { mode: opts.mode || header.mode }), err => {
+            pump(stream, fs.createWriteStream(destFilePath, {
+              flags: NO_FOLLOW_WRITE_FLAGS,
+              mode: opts.mode || header.mode,
+            }), err => {
               if (err) return reject(err);
               successCount++;
               done();

test/tar/symlink-resolution.test.js

@@ -0,0 +1,283 @@
+'use strict';
+
+const fs = require('fs');
+const os = require('os');
+const path = require('path');
+const zlib = require('zlib');
+const uuid = require('uuid');
+const assert = require('assert');
+const compressing = require('../..');
+const { createTarBuffer, createZipBuffer } = require('../util');
+
+// Extraction resolves a symlink chain hop by hop when realpath() cannot, so an
+// entry whose destination passes through several links still lands where the
+// resolved chain actually points, and never outside the extraction directory.
+describe('test/tar/symlink-resolution.test.js', () => {
+  let tempDir;
+
+  beforeEach(() => {
+    tempDir = path.join(os.tmpdir(), uuid.v4());
+    fs.mkdirSync(tempDir, { recursive: true });
+  });
+
+  afterEach(() => {
+    fs.rmSync(tempDir, { recursive: true, force: true });
+  });
+
+  function gzipBuffer(buf) {
+    return new Promise((resolve, reject) => {
+      zlib.gzip(buf, (err, result) => {
+        if (err) return reject(err);
+        resolve(result);
+      });
+    });
+  }
+
+  // destDir/entry -> destDir/hop -> outsideDir/other.txt, which does not exist,
+  // so realpath() cannot resolve the chain and each hop is walked by hand.
+  function setupChain(destDir, outsideDir) {
+    fs.mkdirSync(outsideDir, { recursive: true });
+    fs.mkdirSync(destDir, { recursive: true });
+    fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry'));
+    fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop'));
+  }
+
+  // destDir/entry -> linkedDir/other.txt, where destDir/linkedDir -> outsideDir
+  function setupLinkedDir(destDir, outsideDir) {
+    fs.mkdirSync(outsideDir, { recursive: true });
+    fs.mkdirSync(destDir, { recursive: true });
+    fs.symlinkSync(outsideDir, path.join(destDir, 'linkedDir'));
+    fs.symlinkSync(path.join('linkedDir', 'other.txt'), path.join(destDir, 'entry'));
+  }
+
+  describe('a chain whose first hop stays inside destDir', () => {
+    it('should not write past the end of the chain', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupChain(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.existsSync(path.join(outsideDir, 'other.txt')),
+        false,
+        'The entry should not be written at the end of the chain'
+      );
+    });
+
+    it('should handle a chain longer than two hops', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      fs.mkdirSync(outsideDir, { recursive: true });
+      fs.mkdirSync(destDir, { recursive: true });
+      fs.symlinkSync(path.join(destDir, 'hop1'), path.join(destDir, 'entry'));
+      fs.symlinkSync(path.join(destDir, 'hop2'), path.join(destDir, 'hop1'));
+      fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop2'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+
+    it('should behave the same in tgz extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupChain(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+      await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+
+    it('should behave the same in zip extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupChain(destDir, outsideDir);
+
+      const zipBuffer = await createZipBuffer([
+        { name: 'entry', content: 'content' },
+      ]);
+      await compressing.zip.uncompress(zipBuffer, destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+  });
+
+  describe('a chain passing through a linked directory', () => {
+    it('should resolve the directory component of the link target', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupLinkedDir(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.existsSync(path.join(outsideDir, 'other.txt')),
+        false,
+        'The linked directory in the target should be resolved, not taken literally'
+      );
+    });
+
+    it('should behave the same in tgz extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupLinkedDir(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+      await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+
+    it('should behave the same in zip extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupLinkedDir(destDir, outsideDir);
+
+      const zipBuffer = await createZipBuffer([
+        { name: 'entry', content: 'content' },
+      ]);
+      await compressing.zip.uncompress(zipBuffer, destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+  });
+
+  describe('a symlink at the entry destination', () => {
+    it('should be replaced by the entry instead of written through', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      fs.mkdirSync(destDir, { recursive: true });
+      fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry'));
+      fs.symlinkSync(path.join(destDir, 'final.txt'), path.join(destDir, 'hop'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(),
+        false,
+        'The symlink at the destination should have been replaced by a regular file'
+      );
+      assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'content');
+      assert.strictEqual(
+        fs.existsSync(path.join(destDir, 'final.txt')),
+        false,
+        'The chain should not have been followed to its target'
+      );
+    });
+
+    it('should leave the file the symlink points at untouched', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      fs.mkdirSync(destDir, { recursive: true });
+      const target = path.join(destDir, 'target.txt');
+      fs.writeFileSync(target, 'ORIGINAL_CONTENT');
+      fs.symlinkSync(target, path.join(destDir, 'entry'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'new content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.readFileSync(target, 'utf8'),
+        'ORIGINAL_CONTENT',
+        'Writing an entry must not reach through a symlink to its target'
+      );
+      assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'new content');
+    });
+  });
+
+  describe('linked directories inside destDir', () => {
+    it('should still be traversed when writing an entry beneath them', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const realDir = path.join(destDir, 'real');
+      fs.mkdirSync(realDir, { recursive: true });
+      fs.symlinkSync(realDir, path.join(destDir, 'linkDir'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'linkDir/final.txt', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.readFileSync(path.join(realDir, 'final.txt'), 'utf8'),
+        'content',
+        'A linked directory inside destDir should still be traversed'
+      );
+    });
+  });
+
+  describe('an extraction directory reached through a symlink', () => {
+    // destDir is given as linkBase/dest while its real path is realBase/dest, the
+    // shape /var -> /private/var produces on macOS. A link target written in the
+    // real namespace must still be recognised as living inside destDir.
+    //
+    // Skipped on Windows, where a dangling link resolves differently and the entry
+    // is skipped regardless. That behaviour predates this change, and the namespace
+    // divergence covered here is a POSIX shape.
+    const itPosix = process.platform === 'win32' ? it.skip : it;
+
+    itPosix('should accept a dangling target written in the real namespace', async () => {
+      const realBase = path.join(tempDir, 'realBase');
+      const linkBase = path.join(tempDir, 'linkBase');
+      fs.mkdirSync(path.join(realBase, 'dest'), { recursive: true });
+      fs.symlinkSync(realBase, linkBase);
+
+      const destDir = path.join(linkBase, 'dest');
+      // realpathSync, not the realBase path: tempDir may itself sit behind a symlink.
+      const realDest = fs.realpathSync(path.join(realBase, 'dest'));
+      fs.symlinkSync(path.join(realDest, 'final.txt'), path.join(destDir, 'entry'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.readFileSync(path.join(destDir, 'entry'), 'utf8'),
+        'content',
+        'A target inside destDir should be accepted whichever namespace names it'
+      );
+    });
+  });
+
+  describe('symlink cycles', () => {
+    it('should terminate rather than loop', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      fs.mkdirSync(destDir, { recursive: true });
+      fs.symlinkSync(path.join(destDir, 'b'), path.join(destDir, 'entry'));
+      fs.symlinkSync(path.join(destDir, 'entry'), path.join(destDir, 'b'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(), true);
+    });
+  });
+});