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

fengmk2 · web-flow · commit 90b2e543d291 · 2026-08-05T20:46:48.000+08:00
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.
diff --git a/lib/utils.js b/lib/utils.js
@@ -5,6 +5,35 @@ const path = require('path');
 const mkdirp = require('mkdirp');
 const pump = require('pump');
 
+// 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 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
+ * @param {function} callback - callback(err)
+ */
+function unlinkSymlink(target, callback) {
+  fs.lstat(target, function(err, stat) {
+    if (err) return callback(err.code === 'ENOENT' ? null : err);
+    if (!stat.isSymbolicLink()) return callback(null);
+    fs.unlink(target, function(err) {
+      callback(err && err.code !== 'ENOENT' ? err : null);
+    });
+  });
+}
+
 /**
  * Check if childPath is within parentPath (prevents path traversal attacks)
  * @param {string} childPath - The path to check
@@ -30,16 +59,34 @@ function isPathWithinParent(childPath, parentPath) {
  * @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)
  * @param {function} callback - callback(err, safe)
+ * @param {number} [depth] - Recursion depth when re-walking a dangling symlink's target
  */
-function isRealPathSafe(targetPath, parentDir, realParentDir, callback) {
+function isRealPathSafe(targetPath, parentDir, realParentDir, callback, depth) {
+  depth = 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 callback(null, 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 callback(null, false);
+  }
+
+  const relative = path.relative(baseDir, targetPath);
   const segments = relative.split(path.sep);
   let i = 0;
-  let current = parentDir;
+  let current = baseDir;
 
   function checkNext() {
     if (i >= segments.length) return callback(null, true);
@@ -58,11 +105,15 @@ function isRealPathSafe(targetPath, parentDir, realParentDir, callback) {
       fs.realpath(current, function(err, resolved) {
         if (err) {
           if (err.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.
             return fs.readlink(current, function(err, linkTarget) {
               if (err) return callback(null, false);
               const absTarget = path.resolve(path.dirname(current), linkTarget);
-              callback(null, isWithinParent(absTarget));
+              if (!isWithinParent(absTarget)) return callback(null, false);
+              isRealPathSafe(absTarget, parentDir, realParentDir, callback, depth + 1);
             });
           }
           // Fail closed: unexpected errors during symlink resolution are unsafe
@@ -214,11 +265,18 @@ exports.makeUncompressFn = StreamClass => {
                   mkdirp(dir, err => {
                     if (err) return reject(err);
 
-                    entryCount++;
-                    pump(stream, fs.createWriteStream(destFilePath, { mode: opts.mode || header.mode }), err => {
+                    unlinkSymlink(destFilePath, err => {
                       if (err) return reject(err);
-                      successCount++;
-                      done();
+
+                      entryCount++;
+                      pump(stream, fs.createWriteStream(destFilePath, {
+                        flags: NO_FOLLOW_WRITE_FLAGS,
+                        mode: opts.mode || header.mode,
+                      }), err => {
+                        if (err) return reject(err);
+                        successCount++;
+                        done();
+                      });
                     });
                   });
                 } else if (header.type === 'symlink') {
diff --git a/test/tar/symlink-resolution.test.js b/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);
+    });
+  });
+});
diff --git a/test/util.js b/test/util.js
