From e9e5ee560e2baf694843df852d027fb9f2dbcb06 Mon Sep 17 00:00:00 2001 From: Gar Date: Thu, 19 Aug 2021 06:58:48 -0700 Subject: [PATCH] @npmcli/arborist@2.8.2 * fix: treat top-level global packages as "top" nodes * fix: load global symlinks implicitly as file: deps * fix(reify): debug crash when extracting into symlink * fix: node_modules must be a directory * fix: make Node.children() a case-insensitive Map * fix(reify): verify existing deps in nm are dirs --- .../arborist/lib/arborist/build-ideal-tree.js | 21 +++- .../arborist/lib/arborist/load-actual.js | 6 +- .../@npmcli/arborist/lib/arborist/reify.js | 98 +++++++++++++++---- .../arborist/lib/case-insensitive-map.js | 48 +++++++++ .../arborist/lib/deepest-nesting-target.js | 2 +- node_modules/@npmcli/arborist/lib/edge.js | 4 +- node_modules/@npmcli/arborist/lib/node.js | 40 ++++++-- node_modules/@npmcli/arborist/package.json | 3 +- package-lock.json | 16 ++- package.json | 2 +- test/lib/utils/completion/installed-deep.js | 4 + 11 files changed, 198 insertions(+), 46 deletions(-) create mode 100644 node_modules/@npmcli/arborist/lib/case-insensitive-map.js diff --git a/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js b/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js index 679d52582cb25..cda7f8acfb517 100644 --- a/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js +++ b/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js @@ -9,6 +9,9 @@ const { resolve, dirname } = require('path') const { promisify } = require('util') const treeCheck = require('../tree-check.js') const readdir = promisify(require('readdir-scoped-modules')) +const fs = require('fs') +const lstat = promisify(fs.lstat) +const readlink = promisify(fs.readlink) const { depth } = require('treeverse') const { @@ -407,7 +410,14 @@ module.exports = cls => class IdealTreeBuilder extends cls { if (this[_updateAll] || updateName) { if (updateName) globalExplicitUpdateNames.push(name) - tree.package.dependencies[name] = '*' + const dir = resolve(nm, name) + const st = await lstat(dir).catch(/* istanbul ignore next */ er => null) + if (st && st.isSymbolicLink()) { + const target = await readlink(dir) + const real = resolve(dirname(dir), target) + tree.package.dependencies[name] = `file:${real}` + } else + tree.package.dependencies[name] = '*' } } } @@ -1015,6 +1025,15 @@ This is a one-time fix-up, please be patient... // keep track of the thing that caused this node to be included. const src = parent.sourceReference this[_peerSetSource].set(node, src) + + // do not load the peers along with the set if this is a global top pkg + // otherwise we'll be tempted to put peers as other top-level installed + // things, potentially clobbering what's there already, which is not + // what we want. the missing edges will be picked up on the next pass. + if (this[_global] && edge.from.isProjectRoot) + return node + + // otherwise, we have to make sure that our peers can go along with us. return this[_loadPeerSet](node, required) } diff --git a/node_modules/@npmcli/arborist/lib/arborist/load-actual.js b/node_modules/@npmcli/arborist/lib/arborist/load-actual.js index 86856d868b426..0338b2cd847b2 100644 --- a/node_modules/@npmcli/arborist/lib/arborist/load-actual.js +++ b/node_modules/@npmcli/arborist/lib/arborist/load-actual.js @@ -188,8 +188,10 @@ module.exports = cls => class ActualLoader extends cls { const tree = this[_actualTree] const actualRoot = tree.isLink ? tree.target : tree const { dependencies = {} } = actualRoot.package - for (const name of actualRoot.children.keys()) - dependencies[name] = dependencies[name] || '*' + for (const [name, kid] of actualRoot.children.entries()) { + const def = kid.isLink ? `file:${kid.realpath}` : '*' + dependencies[name] = dependencies[name] || def + } actualRoot.package = { ...actualRoot.package, dependencies } } return this[_actualTree] diff --git a/node_modules/@npmcli/arborist/lib/arborist/reify.js b/node_modules/@npmcli/arborist/lib/arborist/reify.js index 1cfa6034eadb8..965435f84fb41 100644 --- a/node_modules/@npmcli/arborist/lib/arborist/reify.js +++ b/node_modules/@npmcli/arborist/lib/arborist/reify.js @@ -6,11 +6,13 @@ const AuditReport = require('../audit-report.js') const {subset, intersects} = require('semver') const npa = require('npm-package-arg') const debug = require('../debug.js') +const walkUp = require('walk-up-path') const {dirname, resolve, relative} = require('path') const {depth: dfwalk} = require('treeverse') const fs = require('fs') const {promisify} = require('util') +const lstat = promisify(fs.lstat) const symlink = promisify(fs.symlink) const mkdirp = require('mkdirp-infer-owner') const justMkdirp = require('mkdirp') @@ -19,6 +21,7 @@ const rimraf = promisify(require('rimraf')) const PackageJson = require('@npmcli/package-json') const packageContents = require('@npmcli/installed-package-contents') const { checkEngine, checkPlatform } = require('npm-install-checks') +const _force = Symbol.for('force') const treeCheck = require('../tree-check.js') const relpath = require('../relpath.js') @@ -76,8 +79,10 @@ const _copyIdealToActual = Symbol('copyIdealToActual') const _addOmitsToTrashList = Symbol('addOmitsToTrashList') const _packageLockOnly = Symbol('packageLockOnly') const _dryRun = Symbol('dryRun') +const _validateNodeModules = Symbol('validateNodeModules') +const _nmValidated = Symbol('nmValidated') const _validatePath = Symbol('validatePath') -const _reifyPackages = Symbol('reifyPackages') +const _reifyPackages = Symbol.for('reifyPackages') const _omitDev = Symbol('omitDev') const _omitOptional = Symbol('omitOptional') @@ -119,6 +124,7 @@ module.exports = cls => class Reifier extends cls { this[_bundleUnpacked] = new Set() // child nodes we'd EXPECT to be included in a bundle, but aren't this[_bundleMissing] = new Set() + this[_nmValidated] = new Set() } // public method @@ -159,6 +165,9 @@ module.exports = cls => class Reifier extends cls { // recursively, because it can have other side effects to do that // in a project directory. We just want to make it if it's missing. await justMkdirp(resolve(this.path)) + + // do not allow the top-level node_modules to be a symlink + await this[_validateNodeModules](resolve(this.path, 'node_modules')) } async [_reifyPackages] () { @@ -328,12 +337,13 @@ module.exports = cls => class Reifier extends cls { ideal: this.idealTree, }) - for (const node of this.diff.removed) { - // a node in a dep bundle will only be removed if its bundling dep - // is removed as well. in which case, we don't have to delete it! - if (!node.inDepBundle) - this[_addNodeToTrashList](node) - } + // we don't have to add 'removed' folders to the trashlist, because + // they'll be moved aside to a retirement folder, and then the retired + // folder will be deleted at the end. This is important when we have + // a folder like FOO being "removed" in favor of a folder like "foo", + // because if we remove node_modules/FOO on case-insensitive systems, + // it will remove the dep that we *want* at node_modules/foo. + process.emit('timeEnd', 'reify:diffTrees') } @@ -429,19 +439,40 @@ module.exports = cls => class Reifier extends cls { process.emit('time', 'reify:createSparse') // if we call this fn again, we look for the previous list // so that we can avoid making the same directory multiple times - const dirs = this.diff.leaves + const leaves = this.diff.leaves .filter(diff => { return (diff.action === 'ADD' || diff.action === 'CHANGE') && !this[_sparseTreeDirs].has(diff.ideal.path) && !diff.ideal.isLink }) - .map(diff => diff.ideal.path) - - return promiseAllRejectLate(dirs.map(d => mkdirp(d))) - .then(made => { - made.forEach(made => this[_sparseTreeRoots].add(made)) - dirs.forEach(dir => this[_sparseTreeDirs].add(dir)) - }) + .map(diff => diff.ideal) + + // we check this in parallel, so guard against multiple attempts to + // retire the same path at the same time. + const dirsChecked = new Set() + return promiseAllRejectLate(leaves.map(async node => { + for (const d of walkUp(node.path)) { + if (d === node.top.path) + break + if (dirsChecked.has(d)) + continue + dirsChecked.add(d) + const st = await lstat(d).catch(er => null) + // this can happen if we have a link to a package with a name + // that the filesystem treats as if it is the same thing. + // would be nice to have conditional istanbul ignores here... + /* istanbul ignore next - defense in depth */ + if (st && !st.isDirectory()) { + const retired = retirePath(d) + this[_retiredPaths][d] = retired + this[_trashList].add(retired) + await this[_renamePath](d, retired) + } + } + const made = await mkdirp(node.path) + this[_sparseTreeDirs].add(node.path) + this[_sparseTreeRoots].add(made) + })) .then(() => process.emit('timeEnd', 'reify:createSparse')) } @@ -536,7 +567,20 @@ module.exports = cls => class Reifier extends cls { }) } - [_extractOrLink] (node) { + // do not allow node_modules to be a symlink + async [_validateNodeModules] (nm) { + if (this[_force] || this[_nmValidated].has(nm)) + return + const st = await lstat(nm).catch(() => null) + if (!st || st.isDirectory()) { + this[_nmValidated].add(nm) + return + } + this.log.warn('reify', 'Removing non-directory', nm) + await rimraf(nm) + } + + async [_extractOrLink] (node) { // in normal cases, node.resolved should *always* be set by now. // however, it is possible when a lockfile is damaged, or very old, // or in some other race condition bugs in npm v6, that a previously @@ -563,13 +607,29 @@ module.exports = cls => class Reifier extends cls { return } - return node.isLink - ? rimraf(node.path).then(() => this[_symlink](node)) - : pacote.extract(res, node.path, { + const nm = resolve(node.parent.path, 'node_modules') + await this[_validateNodeModules](nm) + + if (node.isLink) { + await rimraf(node.path) + await this[_symlink](node) + } else { + await debug(async () => { + const st = await lstat(node.path).catch(e => null) + if (st && !st.isDirectory()) { + debug.log('unpacking into a non-directory', node) + throw Object.assign(new Error('ENOTDIR: not a directory'), { + code: 'ENOTDIR', + path: node.path, + }) + } + }) + await pacote.extract(res, node.path, { ...this.options, resolved: node.resolved, integrity: node.integrity, }) + } } async [_symlink] (node) { diff --git a/node_modules/@npmcli/arborist/lib/case-insensitive-map.js b/node_modules/@npmcli/arborist/lib/case-insensitive-map.js new file mode 100644 index 0000000000000..8254c3f7a55e9 --- /dev/null +++ b/node_modules/@npmcli/arborist/lib/case-insensitive-map.js @@ -0,0 +1,48 @@ +// package children are represented with a Map object, but many file systems +// are case-insensitive and unicode-normalizing, so we need to treat +// node.children.get('FOO') and node.children.get('foo') as the same thing. + +const _keys = Symbol('keys') +const _normKey = Symbol('normKey') +const normalize = s => s.normalize('NFKD').toLowerCase() +const OGMap = Map +module.exports = class Map extends OGMap { + constructor (items = []) { + super() + this[_keys] = new OGMap() + for (const [key, val] of items) + this.set(key, val) + } + + [_normKey] (key) { + return typeof key === 'string' ? normalize(key) : key + } + + get (key) { + const normKey = this[_normKey](key) + return this[_keys].has(normKey) ? super.get(this[_keys].get(normKey)) + : undefined + } + + set (key, val) { + const normKey = this[_normKey](key) + if (this[_keys].has(normKey)) + super.delete(this[_keys].get(normKey)) + this[_keys].set(normKey, key) + return super.set(key, val) + } + + delete (key) { + const normKey = this[_normKey](key) + if (this[_keys].has(normKey)) { + const prevKey = this[_keys].get(normKey) + this[_keys].delete(normKey) + return super.delete(prevKey) + } + } + + has (key) { + const normKey = this[_normKey](key) + return this[_keys].has(normKey) && super.has(this[_keys].get(normKey)) + } +} diff --git a/node_modules/@npmcli/arborist/lib/deepest-nesting-target.js b/node_modules/@npmcli/arborist/lib/deepest-nesting-target.js index cbaa396f3f251..9c433a7652da2 100644 --- a/node_modules/@npmcli/arborist/lib/deepest-nesting-target.js +++ b/node_modules/@npmcli/arborist/lib/deepest-nesting-target.js @@ -5,7 +5,7 @@ const deepestNestingTarget = (start, name) => { for (const target of start.ancestry()) { // note: this will skip past the first target if edge is peer - if (target.isProjectRoot || !target.resolveParent) + if (target.isProjectRoot || !target.resolveParent || target.globalTop) return target const targetEdge = target.edgesOut.get(name) if (!targetEdge || !targetEdge.peer) diff --git a/node_modules/@npmcli/arborist/lib/edge.js b/node_modules/@npmcli/arborist/lib/edge.js index 0bd9021d56a70..9d5ece40e5fae 100644 --- a/node_modules/@npmcli/arborist/lib/edge.js +++ b/node_modules/@npmcli/arborist/lib/edge.js @@ -77,7 +77,7 @@ class Edge { } satisfiedBy (node) { - return depValid(node, this.spec, this.accept, this.from) + return node.name === this.name && depValid(node, this.spec, this.accept, this.from) } explain (seen = []) { @@ -167,7 +167,7 @@ class Edge { [_loadError] () { return !this[_to] ? (this.optional ? null : 'MISSING') : this.peer && this.from === this.to.parent && !this.from.isTop ? 'PEER LOCAL' - : !depValid(this.to, this.spec, this.accept, this.from) ? 'INVALID' + : !this.satisfiedBy(this.to) ? 'INVALID' : 'OK' } diff --git a/node_modules/@npmcli/arborist/lib/node.js b/node_modules/@npmcli/arborist/lib/node.js index d77b18355ff31..5616019dd9cc2 100644 --- a/node_modules/@npmcli/arborist/lib/node.js +++ b/node_modules/@npmcli/arborist/lib/node.js @@ -66,6 +66,7 @@ const relpath = require('./relpath.js') const consistentResolve = require('./consistent-resolve.js') const printableTree = require('./printable.js') +const CaseInsensitiveMap = require('./case-insensitive-map.js') class Node { constructor (options) { @@ -148,7 +149,7 @@ class Node { this.hasShrinkwrap = hasShrinkwrap || pkg._hasShrinkwrap || false this.legacyPeerDeps = legacyPeerDeps - this.children = new Map() + this.children = new CaseInsensitiveMap() this.fsChildren = new Set() this.inventory = new Inventory({}) this.tops = new Set() @@ -181,7 +182,7 @@ class Node { } this.edgesIn = new Set() - this.edgesOut = new Map() + this.edgesOut = new CaseInsensitiveMap() // have to set the internal package ref before assigning the parent, // because this.package is read when adding to inventory @@ -248,7 +249,7 @@ class Node { // true for packages installed directly in the global node_modules folder get globalTop () { - return this.global && this.parent.isProjectRoot + return this.global && this.parent && this.parent.isProjectRoot } get workspaces () { @@ -478,6 +479,9 @@ class Node { } get isProjectRoot () { + // only treat as project root if it's the actual link that is the root, + // or the target of the root link, but NOT if it's another link to the + // same root that happens to be somewhere else. return this === this.root || this === this.root.target } @@ -772,9 +776,15 @@ class Node { this[_loadDepType](this.package.dependencies, 'prod') this[_loadDepType](this.package.optionalDependencies, 'optional') - const { isTop, path, sourceReference } = this - const { isTop: srcTop, path: srcPath } = sourceReference || {} - if (isTop && path && (!sourceReference || srcTop && srcPath)) + const { globalTop, isTop, path, sourceReference } = this + const { + globalTop: srcGlobalTop, + isTop: srcTop, + path: srcPath, + } = sourceReference || {} + const thisDev = isTop && !globalTop && path + const srcDev = !sourceReference || srcTop && !srcGlobalTop && srcPath + if (thisDev && srcDev) this[_loadDepType](this.package.devDependencies, 'dev') } @@ -1014,8 +1024,20 @@ class Node { replace (node) { this[_delistFromMeta]() - this.path = node.path - this.name = node.name + + // if the name matches, but is not identical, we are intending to clobber + // something case-insensitively, so merely setting name and path won't + // have the desired effect. just set the path so it'll collide in the + // parent's children map, and leave it at that. + const nameMatch = node.parent && + node.parent.children.get(this.name) === node + if (nameMatch) + this.path = resolve(node.parent.path, 'node_modules', this.name) + else { + this.path = node.path + this.name = node.name + } + if (!this.isLink) this.realpath = this.path this[_refreshLocation]() @@ -1210,7 +1232,7 @@ class Node { } get isTop () { - return !this.parent + return !this.parent || this.globalTop } get top () { diff --git a/node_modules/@npmcli/arborist/package.json b/node_modules/@npmcli/arborist/package.json index 01f018a629339..42ec2eca3b310 100644 --- a/node_modules/@npmcli/arborist/package.json +++ b/node_modules/@npmcli/arborist/package.json @@ -1,6 +1,6 @@ { "name": "@npmcli/arborist", - "version": "2.8.1", + "version": "2.8.2", "description": "Manage node_modules trees", "dependencies": { "@npmcli/installed-package-contents": "^1.0.7", @@ -32,7 +32,6 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "ssri": "^8.0.1", - "tar": "^6.1.0", "treeverse": "^1.0.4", "walk-up-path": "^1.0.0" }, diff --git a/package-lock.json b/package-lock.json index e872f3cf5cb84..e55c9732d35c6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -83,7 +83,7 @@ "packages/*" ], "dependencies": { - "@npmcli/arborist": "^2.8.1", + "@npmcli/arborist": "^2.8.2", "@npmcli/ci-detect": "^1.2.0", "@npmcli/config": "^2.2.0", "@npmcli/map-workspaces": "^1.0.4", @@ -756,9 +756,9 @@ } }, "node_modules/@npmcli/arborist": { - "version": "2.8.1", - "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.8.1.tgz", - "integrity": "sha512-kbBWllN4CcdeN032Rw6b+TIsyoxWcv4YNN5gzkMCe8cCu0llwlq5P7uAD2oyL24QdmGlrlg/Yp0L1JF+HD8g9Q==", + "version": "2.8.2", + "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.8.2.tgz", + "integrity": "sha512-6E1XJ0YXBaI9J+25gcTF110MGNx3jv6npr4Rz1U0UAqkuVV7bbDznVJvNqi6F0p8vgrE+Smf9jDTn1DR+7uBjQ==", "inBundle": true, "dependencies": { "@npmcli/installed-package-contents": "^1.0.7", @@ -790,7 +790,6 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "ssri": "^8.0.1", - "tar": "^6.1.0", "treeverse": "^1.0.4", "walk-up-path": "^1.0.0" }, @@ -10993,9 +10992,9 @@ "dev": true }, "@npmcli/arborist": { - "version": "2.8.1", - "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.8.1.tgz", - "integrity": "sha512-kbBWllN4CcdeN032Rw6b+TIsyoxWcv4YNN5gzkMCe8cCu0llwlq5P7uAD2oyL24QdmGlrlg/Yp0L1JF+HD8g9Q==", + "version": "2.8.2", + "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.8.2.tgz", + "integrity": "sha512-6E1XJ0YXBaI9J+25gcTF110MGNx3jv6npr4Rz1U0UAqkuVV7bbDznVJvNqi6F0p8vgrE+Smf9jDTn1DR+7uBjQ==", "requires": { "@npmcli/installed-package-contents": "^1.0.7", "@npmcli/map-workspaces": "^1.0.2", @@ -11026,7 +11025,6 @@ "rimraf": "^3.0.2", "semver": "^7.3.5", "ssri": "^8.0.1", - "tar": "^6.1.0", "treeverse": "^1.0.4", "walk-up-path": "^1.0.0" } diff --git a/package.json b/package.json index b64ed7e0d4ba5..f6db0adab20c0 100644 --- a/package.json +++ b/package.json @@ -53,7 +53,7 @@ "./package.json": "./package.json" }, "dependencies": { - "@npmcli/arborist": "^2.8.1", + "@npmcli/arborist": "^2.8.2", "@npmcli/ci-detect": "^1.2.0", "@npmcli/config": "^2.2.0", "@npmcli/map-workspaces": "^1.0.4", diff --git a/test/lib/utils/completion/installed-deep.js b/test/lib/utils/completion/installed-deep.js index 21e77a568bd8a..aa0d85ec10a57 100644 --- a/test/lib/utils/completion/installed-deep.js +++ b/test/lib/utils/completion/installed-deep.js @@ -219,6 +219,8 @@ t.test('limit depth', async t => { [ ['bar', '-g'], ['foo', '-g'], + // XXX https://github.com/npm/statusboard/issues/380 + ['a-bar', '-g'], 'a', 'b', 'c', 'ch', 'd', 'e', @@ -248,6 +250,8 @@ t.test('limit depth as global', async t => { [ 'bar', 'foo', + // https://github.com/npm/statusboard/issues/380 + 'a-bar', ], 'should reorder so that packages above that level depth goes last' )