From abacede85e711bfdca6add3d47150256c5085a29 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 1 Jun 2026 08:39:23 -0700 Subject: [PATCH] fix: sanitize package name in linked-strategy path construction (cherry picked from commit 9f3c97f83443ee00b9ca6beaf3e8cec95d3199ad) --- .../arborist/lib/arborist/isolated-reifier.js | 9 +- workspaces/arborist/lib/isolated-classes.js | 5 + workspaces/arborist/test/arborist/reify.js | 122 ++++++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 14f432ca97745..56f2413dd3ded 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -4,6 +4,7 @@ const { join } = require('node:path') const { depth } = require('treeverse') const crypto = require('node:crypto') const { IsolatedNode, IsolatedLink } = require('../isolated-classes.js') +const nameFromFolder = require('@npmcli/name-from-folder') // generate short hash key based on the dependency tree starting at this node const getKey = (startNode) => { @@ -151,11 +152,14 @@ module.exports = cls => class IsolatedReifier extends cls { this.#externalProxies.set(node, result) await this.#assignCommonProperties(node, result, !node.hasShrinkwrap) if (node.hasShrinkwrap) { + // strip any path traversal from package.json name fields before they hit path.join below + /* istanbul ignore next - packageName is always set for real packages */ + const safeName = nameFromFolder(node.packageName || node.path) const dir = join( node.root.path, 'node_modules', '.store', - `${node.packageName}@${node.version}` + `${safeName}@${node.version}` ) mkdirSync(dir, { recursive: true }) // TODO this approach feels wrong and shouldn't be necessary for shrinkwraps @@ -198,7 +202,8 @@ module.exports = cls => class IsolatedReifier extends cls { result.id = this.counter++ /* istanbul ignore next - packageName is always set for real packages */ result.name = result.isWorkspace ? (node.packageName || node.name) : node.name - result.packageName = node.packageName || node.name + // strip any path traversal from package.json name fields before they hit path.join below + result.packageName = nameFromFolder(node.packageName || node.path) result.package = { ...node.package } result.package.bundleDependencies = undefined diff --git a/workspaces/arborist/lib/isolated-classes.js b/workspaces/arborist/lib/isolated-classes.js index d9770a386791f..8e6e5b79a91f4 100644 --- a/workspaces/arborist/lib/isolated-classes.js +++ b/workspaces/arborist/lib/isolated-classes.js @@ -104,6 +104,11 @@ class IsolatedNode { return !!(hasInstallScript || install || preinstall || postinstall) } + /* istanbul ignore next -- emulate lib/node.js */ + get packageName () { + return this.package.name || null + } + get version () { return this.package.version } diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 90ff99f38b90f..15270c3a8f5b2 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -4065,6 +4065,128 @@ t.test('workspace installs retain existing versions with newer package specs', a 'another-cool-package package.json should be updated to abbrev@1.0.4') }) +for (const poisoned of ['../../../escape-target', '@evil/../../../escape-target']) { + t.test(`install strategy linked sanitizes traversal in lockfile name (${poisoned})`, async t => { + // a poisoned lockfile name field would otherwise escape node_modules/.store + const testDir = t.testdir({ + 'package.json': JSON.stringify({ + dependencies: { + abbrev: '1.1.1', + }, + }), + 'package-lock.json': JSON.stringify({ + lockfileVersion: 3, + requires: true, + packages: { + '': { + dependencies: { + abbrev: '1.1.1', + }, + }, + 'node_modules/abbrev': { + name: poisoned, + version: '1.1.1', + resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz', + integrity: 'sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==', + }, + }, + }), + }) + + const arb = new Arborist({ + path: testDir, + registry: 'https://registry.npmjs.org', + cache: resolve(testDir, 'cache'), + installStrategy: 'linked', + packageLockOnly: true, + }) + await arb.reify({ installStrategy: 'linked', packageLockOnly: true }) + + const external = arb.idealGraph.external + t.equal(external.length, 1, 'one external dep planned') + + const pkgName = external[0].packageName + t.notMatch(pkgName, /\.\./, 'packageName has no traversal segments') + t.ok(!pkgName.includes('/') || pkgName.startsWith('@'), + 'packageName is a single segment (or @scope/name)') + + // joining the sanitized name into the .store layout must not escape + const storePrefix = resolve(testDir, 'node_modules/.store/key/node_modules') + const projected = resolve(storePrefix, pkgName) + t.ok(projected.startsWith(storePrefix), 'projected path stays inside .store') + + // belt-and-suspenders: nothing should have been written outside testDir, + // even if a future change starts materializing paths during reify + t.notOk(fs.existsSync(resolve(testDir, '..', 'escape-target')), + 'no escape-target leaked one level above testDir') + t.notOk(fs.existsSync(resolve(testDir, '..', '..', 'escape-target')), + 'no escape-target leaked two levels above testDir') + t.notOk(fs.existsSync(resolve(testDir, '..', '..', '..', 'escape-target')), + 'no escape-target leaked three levels above testDir') + }) +} + +for (const poisoned of ['../../../escape-target', '@evil/../../../escape-target']) { + t.test(`install strategy linked sanitizes traversal in shrinkwrapped lockfile name (${poisoned})`, async t => { + // the hasShrinkwrap branch in #externalProxy materializes + // node_modules/.store/@ via mkdirSync before any other + // check, so a poisoned name in a shrinkwrapped entry would escape + // node_modules/.store on disk without sanitization + const testDir = t.testdir({ + 'package.json': JSON.stringify({ + dependencies: { + abbrev: '1.1.1', + }, + }), + 'package-lock.json': JSON.stringify({ + lockfileVersion: 3, + requires: true, + packages: { + '': { + dependencies: { + abbrev: '1.1.1', + }, + }, + 'node_modules/abbrev': { + name: poisoned, + version: '1.1.1', + hasShrinkwrap: true, + resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz', + integrity: 'sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==', + }, + }, + }), + }) + + const arb = new Arborist({ + path: testDir, + registry: 'https://registry.npmjs.org', + cache: resolve(testDir, 'cache'), + installStrategy: 'linked', + packageLockOnly: true, + }) + // pacote.extract may fail in this offline test setup; the mkdirSync that + // runs before it is what we care about and runs unconditionally + try { + await arb.reify({ installStrategy: 'linked', packageLockOnly: true }) + } catch { + // expected in offline test setup + } + + // the sanitized store entry must land inside testDir/node_modules/.store + t.ok(fs.existsSync(resolve(testDir, 'node_modules/.store/escape-target@1.1.1')), + 'sanitized .store/escape-target@1.1.1 created inside the project') + + // and no escape-target@1.1.1 directory should appear above testDir + t.notOk(fs.existsSync(resolve(testDir, '..', 'escape-target@1.1.1')), + 'no escape-target@1.1.1 leaked one level above testDir') + t.notOk(fs.existsSync(resolve(testDir, '..', '..', 'escape-target@1.1.1')), + 'no escape-target@1.1.1 leaked two levels above testDir') + t.notOk(fs.existsSync(resolve(testDir, '..', '..', '..', 'escape-target@1.1.1')), + 'no escape-target@1.1.1 leaked three levels above testDir') + }) +} + t.test('externalOptionalDependencies excludes inert optional node with installStrategy linked', async t => { const testDir = t.testdir({ 'package.json': JSON.stringify({