Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ This is a one-time fix-up, please be patient...

if (hasShrinkwrap) {
await new Arborist({ ...this.options, path })
.loadVirtual({ root: node })
.loadVirtual({ root: node, subtreeOnly: true })
}

if (hasBundle) {
Expand Down
36 changes: 35 additions & 1 deletion workspaces/arborist/lib/arborist/load-virtual.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { resolve } = require('node:path')
const { isAbsolute, resolve } = require('node:path')
// mixin providing the loadVirtual method
const mapWorkspaces = require('@npmcli/map-workspaces')
const PackageJson = require('@npmcli/package-json')
Expand All @@ -17,6 +17,8 @@ const setWorkspaces = Symbol.for('setWorkspaces')

module.exports = cls => class VirtualLoader extends cls {
#rootOptionProvided
// when true, lockfile entries must stay inside `this.path`
#subtreeOnly = false

// public method
async loadVirtual (options = {}) {
Expand All @@ -28,6 +30,8 @@ module.exports = cls => class VirtualLoader extends cls {
// XXX: deprecate separate reify() options object.
options = { ...this.options, ...options }

this.#subtreeOnly = !!options.subtreeOnly

if (options.root && options.root.meta) {
await this.#loadFromShrinkwrap(options.root.meta, options.root)
return treeCheck(this.virtualTree)
Expand Down Expand Up @@ -166,12 +170,40 @@ module.exports = cls => class VirtualLoader extends cls {
}
}

// throw if the resolved path is outside `base`, only in subtreeOnly mode
#assertContained (base, resolvedPath, location) {
Comment thread
owlstronaut marked this conversation as resolved.
if (!this.#subtreeOnly) {
return
}
if (isAbsolute(location)) {
Comment thread
owlstronaut marked this conversation as resolved.
throw Object.assign(
new Error(`invalid lockfile entry: "${location}" must be a relative location`),
{ code: 'EINVALIDLOCATION', location, base }
)
}
const rel = relpath(base, resolvedPath)
if (
rel === '..' ||
rel.startsWith('../') ||
isAbsolute(rel) ||
// non-root key that collapses back to base (e.g. 'node_modules/..')
(rel === '' && location !== '')
Comment thread
owlstronaut marked this conversation as resolved.
) {
throw Object.assign(
new Error(`invalid lockfile entry: "${location}" resolves outside of "${base}"`),
{ code: 'EINVALIDLOCATION', location, base, resolvedPath }
)
}
}

// links is the set of metadata, and nodes is the map of non-Link nodes
// Set the targets to nodes in the set, if we have them (we might not)
// XXX build-ideal-tree also has a #resolveLinks, is there overlap?
async #resolveLinks (links, nodes) {
for (const [location, meta] of links.entries()) {
const targetPath = resolve(this.path, meta.resolved)
// check before nodes.get so we surface EINVALIDLOCATION instead of EMISSINGTARGET
Comment thread
owlstronaut marked this conversation as resolved.
this.#assertContained(this.path, targetPath, meta.resolved || location)
const targetLoc = relpath(this.path, targetPath)
const target = nodes.get(targetLoc)

Expand Down Expand Up @@ -230,6 +262,7 @@ To fix:
#loadNode (location, sw, loadOverrides) {
const p = this.virtualTree ? this.virtualTree.realpath : this.path
const path = resolve(p, location)
this.#assertContained(p, path, location)
// shrinkwrap doesn't include package name unless necessary
if (!sw.name) {
sw.name = nameFromFolder(path)
Expand Down Expand Up @@ -259,6 +292,7 @@ To fix:

#loadLink (location, targetLoc, target) {
const path = resolve(this.path, location)
this.#assertContained(this.path, path, location)
const link = new Link({
installLinks: this.installLinks,
legacyPeerDeps: this.legacyPeerDeps,
Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/arborist/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ module.exports = cls => class Reifier extends cls {
.then(nodes => promiseAllRejectLate(nodes.map(node => new Arborist({
...this.options,
path: node.path,
}).loadVirtual({ root: node }))))
}).loadVirtual({ root: node, subtreeOnly: true }))))
// reload the diff and sparse tree because the ideal tree changed
.then(() => this[_diffTrees]())
.then(() => this[_createSparseTree]())
Expand Down
154 changes: 154 additions & 0 deletions workspaces/arborist/test/arborist/load-virtual.js
Original file line number Diff line number Diff line change
Expand Up @@ -286,3 +286,157 @@ t.test('error when link target is missing', async t => {
message: /Missing target in lock file:.*but does not exist/,
})
})

t.test('subtreeOnly containment', t => {
// simulates the sub-Arborist invocation reify uses when inflating a
// dependency's bundled npm-shrinkwrap.json. a dep must not be able to
// declare entries that resolve outside its own install directory.

const makeDep = (sw) => {
const path = t.testdir({
'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }),
'npm-shrinkwrap.json': JSON.stringify({
name: 'dep',
version: '1.0.0',
lockfileVersion: 3,
requires: true,
packages: {
'': { name: 'dep', version: '1.0.0' },
...sw,
},
}),
})
const root = new Node({
path,
pkg: require(path + '/package.json'),
Comment thread
owlstronaut marked this conversation as resolved.
})
return { path, root }
}

t.test('rejects traversal package key', async t => {
const { path, root } = makeDep({
'../sibling': { name: 'sibling', version: '9.9.9' },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION', location: '../sibling' },
'traversal package entry is rejected with EINVALIDLOCATION'
)
})

t.test('rejects deeply traversing package key', async t => {
const { path, root } = makeDep({
'node_modules/x/../../../escape': { name: 'escape', version: '1.0.0' },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION' },
'normalized traversal is rejected'
)
})

t.test('rejects absolute package key', async t => {
const { path, root } = makeDep({
'/etc/passwd': { name: 'whatever', version: '1.0.0' },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION', location: '/etc/passwd' },
'absolute package key is rejected'
)
})

t.test('rejects non-root location that normalizes back to base', async t => {
const { path, root } = makeDep({
'node_modules/..': { name: 'dep', version: '9.9.9' },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION' },
'collapsed-to-base path is rejected'
)
})

t.test('rejects link entry whose location traverses', async t => {
const { path, root } = makeDep({
'../evil-link': { resolved: 'node_modules/x', link: true },
'node_modules/x': { name: 'x', version: '1.0.0' },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION' },
'link location that escapes the subtree is rejected'
)
})

t.test('rejects link entry whose resolved target traverses', async t => {
const { path, root } = makeDep({
'node_modules/x': { resolved: '../escape-target', link: true },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION' },
'link resolved target outside the subtree is rejected'
)
})

t.test('rejects link entry with empty resolved (falls back to location)', async t => {
const { path, root } = makeDep({
'node_modules/x': { resolved: '', link: true },
})
await t.rejects(
new Arborist({ path }).loadVirtual({ root, subtreeOnly: true }),
{ code: 'EINVALIDLOCATION', location: 'node_modules/x' },
'link without a real resolved target is rejected'
)
})

t.test('permits legitimate intra-subtree entries', async t => {
const { path, root } = makeDep({
'node_modules/abbrev': {
version: '1.1.1',
resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz',
integrity: 'sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==',
},
})
const tree = await new Arborist({ path }).loadVirtual({ root, subtreeOnly: true })
t.equal(tree, root, 'returns the supplied root')
t.ok(tree.children.get('abbrev'), 'abbrev node was materialized')
})

t.test('without subtreeOnly, root lockfiles with file:../ entries still load', async t => {
// root lockfiles can legitimately point at external file: deps; the
// containment check only applies in subtreeOnly mode.
const dir = t.testdir({
sibling: {
'package.json': JSON.stringify({ name: 'sibling', version: '1.0.0' }),
},
project: {
'package.json': JSON.stringify({
name: 'project',
version: '1.0.0',
dependencies: { sibling: 'file:../sibling' },
}),
'package-lock.json': JSON.stringify({
name: 'project',
version: '1.0.0',
lockfileVersion: 3,
requires: true,
packages: {
'': {
name: 'project',
version: '1.0.0',
dependencies: { sibling: 'file:../sibling' },
},
'../sibling': { name: 'sibling', version: '1.0.0' },
'node_modules/sibling': { resolved: '../sibling', link: true },
},
}),
},
})
const tree = await loadVirtual(resolve(dir, 'project'))
t.ok(tree.children.get('sibling'), 'external file:../ link still loads')
})

t.end()
})
Loading