From 848556a1df5ab518c2838f9c8cf620d7c3ada69e Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 5 Aug 2026 17:39:56 +0800 Subject: [PATCH 1/4] fix: resolve symlink chains fully when extracting 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. --- lib/utils.js | 48 +++++- test/tar/symlink-resolution.test.js | 248 ++++++++++++++++++++++++++++ test/util.js | 23 ++- 3 files changed, 314 insertions(+), 5 deletions(-) create mode 100644 test/tar/symlink-resolution.test.js diff --git a/lib/utils.js b/lib/utils.js index 0940071..481de0d 100644 --- a/lib/utils.js +++ b/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,9 +55,14 @@ 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) + * @param {number} depth - Recursion depth when re-walking a dangling symlink's target * @returns {Promise} 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); } @@ -49,10 +81,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 +233,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(); diff --git a/test/tar/symlink-resolution.test.js b/test/tar/symlink-resolution.test.js new file mode 100644 index 0000000..72b6dfe --- /dev/null +++ b/test/tar/symlink-resolution.test.js @@ -0,0 +1,248 @@ +'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('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 index 3951255..f365671 100644 --- a/test/util.js +++ b/test/util.js @@ -31,4 +31,25 @@ function createTarBuffer(entries) { }); } -module.exports = { pipelinePromise, createTarBuffer }; +/** + * Create a ZIP buffer with given file entries + * @param {Array<{name: string, content?: string}>} entries + * @returns {Promise} + */ +function createZipBuffer(entries) { + return new Promise((resolve, reject) => { + const compressing = require('..'); + const zipStream = new compressing.zip.Stream(); + const chunks = []; + + for (const entry of entries) { + zipStream.addEntry(Buffer.from(entry.content || ''), { relativePath: entry.name }); + } + + zipStream.on('data', chunk => chunks.push(chunk)); + zipStream.on('end', () => resolve(Buffer.concat(chunks))); + zipStream.on('error', reject); + }); +} + +module.exports = { pipelinePromise, createTarBuffer, createZipBuffer }; From 003915271fc1d608514b18383b57e1fa9e45a2ae Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 5 Aug 2026 17:53:04 +0800 Subject: [PATCH 2/4] fix: walk from the extraction root that contains the link target 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. --- lib/utils.js | 20 ++++++++++++++++---- test/tar/symlink-resolution.test.js | 29 +++++++++++++++++++++++++++++ test/util.js | 22 ++++++++++++++-------- 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 481de0d..b2ae90d 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -56,20 +56,32 @@ 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 {number} depth - Recursion depth when re-walking a dangling symlink's target - * @returns {Promise} true if safe, false if any segment escapes via symlink + * @return {Promise} true if safe, false if any segment escapes via symlink */ 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; + 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); diff --git a/test/tar/symlink-resolution.test.js b/test/tar/symlink-resolution.test.js index 72b6dfe..ea28c27 100644 --- a/test/tar/symlink-resolution.test.js +++ b/test/tar/symlink-resolution.test.js @@ -229,6 +229,35 @@ describe('test/tar/symlink-resolution.test.js', () => { }); }); + 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. + it('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'); diff --git a/test/util.js b/test/util.js index f365671..9a24298 100644 --- a/test/util.js +++ b/test/util.js @@ -5,8 +5,8 @@ const pipelinePromise = stream.promises.pipeline; /** * Create a TAR buffer with given entries - * @param {Array<{name: string, type?: string, linkname?: string, content?: string}>} entries - * @returns {Promise} + * @param {Array<{name: string, type?: string, linkname?: string, content?: string}>} entries - Entries to put in the archive + * @return {Promise} The archive contents */ function createTarBuffer(entries) { return new Promise((resolve, reject) => { @@ -33,22 +33,28 @@ function createTarBuffer(entries) { /** * Create a ZIP buffer with given file entries - * @param {Array<{name: string, content?: string}>} entries - * @returns {Promise} + * @param {Array<{name: string, content?: string}>} entries - Files to put in the archive + * @return {Promise} The archive contents */ function createZipBuffer(entries) { return new Promise((resolve, reject) => { + // An empty archive never finalizes, so the promise would never settle. + if (!entries || entries.length === 0) { + return reject(new Error('createZipBuffer requires at least one entry')); + } + const compressing = require('..'); const zipStream = new compressing.zip.Stream(); const chunks = []; - for (const entry of entries) { - zipStream.addEntry(Buffer.from(entry.content || ''), { relativePath: entry.name }); - } - + // Listeners first, so an entry rejected during addEntry() settles the promise. zipStream.on('data', chunk => chunks.push(chunk)); zipStream.on('end', () => resolve(Buffer.concat(chunks))); zipStream.on('error', reject); + + for (const entry of entries) { + zipStream.addEntry(Buffer.from(entry.content || ''), { relativePath: entry.name }); + } }); } From d6ef46d8d4534d33f500725cd2fc6afd79c04255 Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 5 Aug 2026 17:58:35 +0800 Subject: [PATCH 3/4] test: skip the namespace case on Windows 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. --- test/tar/symlink-resolution.test.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/tar/symlink-resolution.test.js b/test/tar/symlink-resolution.test.js index ea28c27..4183d4d 100644 --- a/test/tar/symlink-resolution.test.js +++ b/test/tar/symlink-resolution.test.js @@ -233,7 +233,13 @@ describe('test/tar/symlink-resolution.test.js', () => { // 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. - it('should accept a dangling target written in the real namespace', async () => { + // + // 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 }); From 6d29424223b30dd9fb1ca5c1cc3c95542a2eb4fa Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 5 Aug 2026 20:30:23 +0800 Subject: [PATCH 4/4] ci: add Node.js 26 to the test matrix Node 26 is the current release line and becomes LTS in October, so running it now surfaces any breakage before the promotion. --- .github/workflows/nodejs.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index e3ac637..b737e1a 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -15,6 +15,6 @@ jobs: uses: node-modules/github-actions/.github/workflows/node-test.yml@master with: os: 'ubuntu-latest, macos-latest, windows-latest' - version: '18, 20, 22, 24' + version: '18, 20, 22, 24, 26' secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}