From 5c50c0a5ac953dc2e8ac357261221d05183264fa Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Fri, 11 Sep 2020 22:54:05 -0400 Subject: [PATCH 1/2] fix: follow symlinks on build-ideal-tree add Symlinks were being tracked as invalid when loading actual trees since their realpath wouldn't match, given that build-ideal-tree was just preserving the fetchSpec read from npm-package-arg when adding new packages to the ideal tree. This change fixes it by properly parsing and following symlinks realpaths at the moment they're added to the ideal tree. --- lib/add-rm-pkg-deps.js | 2 +- lib/arborist/build-ideal-tree.js | 63 +++++++++++++++---- ...t-arborist-build-ideal-tree.js-TAP.test.js | 35 +++++++++++ test/arborist/build-ideal-tree.js | 63 ++++++++++++++++--- 4 files changed, 142 insertions(+), 21 deletions(-) diff --git a/lib/add-rm-pkg-deps.js b/lib/add-rm-pkg-deps.js index 4479328f9..9ec7456b0 100644 --- a/lib/add-rm-pkg-deps.js +++ b/lib/add-rm-pkg-deps.js @@ -55,7 +55,7 @@ const addSingle = ({pkg, spec, saveBundle, saveType, path}) => { pkg[type] = pkg[type] || {} if (rawSpec !== '' || pkg[type][name] === undefined) { // if we're in global mode, file specs are based on cwd, not arb path - pkg[type][name] = specType === 'file' || specType === 'directory' + pkg[type][name] = specType === 'file' ? `file:${relpath(path, fetchSpec)}` : (rawSpec || '*') } diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index d0a476c1d..5f147fa44 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -8,6 +8,7 @@ const pickManifest = require('npm-pick-manifest') const mapWorkspaces = require('@npmcli/map-workspaces') const promiseCallLimit = require('promise-call-limit') const getPeerSet = require('../peer-set.js') +const realpath = require('../../lib/realpath.js') const fromPath = require('../from-path.js') const calcDepFlags = require('../calc-dep-flags.js') @@ -79,6 +80,10 @@ const _globalStyle = Symbol('globalStyle') const _globalRootNode = Symbol('globalRootNode') const _isVulnerable = Symbol.for('isVulnerable') const _usePackageLock = Symbol.for('usePackageLock') +const _rpcache = Symbol('realpathCache') +const _stcache = Symbol('statCache') +const _followSymlinkPath = Symbol('followSymlinkPath') +const _retrieveSpecName = Symbol('retrieveSpecName') // used for the ERESOLVE error to show the last peer conflict encountered const _peerConflict = Symbol('peerConflict') @@ -130,6 +135,12 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_linkNodes] = new Set() this[_manifests] = new Map() this[_peerConflict] = null + + // caches for cached realpath calls + const cwd = process.cwd() + // assume that the cwd is real enough for our purposes + this[_rpcache] = new Map([[ cwd, cwd ]]) + this[_stcache] = new Map() } get explicitRequests () { @@ -326,18 +337,11 @@ module.exports = cls => class IdealTreeBuilder extends cls { return Promise.all(add.map(s => { // in global mode, `npm i foo.tgz` needs to be resolved from // the current working dir, NOT /usr/local/lib! - const spec = npa(s, this[_global] ? process.cwd() : this.path) - // if it's just @'' then we reload whatever's there, or get latest - // if it's an explicit tag, we need to install that specific tag version - const isTag = spec.rawSpec && spec.type === 'tag' - return spec.name && !isTag ? spec : pacote.manifest(spec).then(mani => { - // if it's a tag type, then we need to run it down to an actual version - if (isTag) - return npa(`${mani.name}@${mani.version}`) - - spec.name = mani.name - return spec - }) + const currpath = this[_global] ? process.cwd() : this.path + const spec = npa(s, currpath) + + return this[_retrieveSpecName](spec) + .then(add => this[_followSymlinkPath](add, currpath)) })).then(add => { this[_resolvedAdd] = add // now add is a list of spec objects with names. @@ -355,6 +359,41 @@ module.exports = cls => class IdealTreeBuilder extends cls { }) } + async [_retrieveSpecName] (spec) { + // if it's just @'' then we reload whatever's there, or get latest + // if it's an explicit tag, we need to install that specific tag version + const isTag = spec.rawSpec && spec.type === 'tag' + + if (spec.name && !isTag) + return spec + + const mani = await pacote.manifest(spec) + // if it's a tag type, then we need to run it down to an actual version + if (isTag) + return npa(`${mani.name}@${mani.version}`) + + spec.name = mani.name + return spec + } + + async [_followSymlinkPath] (spec, currpath) { + if(spec.type === 'directory') { + const real = await ( + realpath(spec.fetchSpec, this[_rpcache], this[_stcache]) + // TODO: create synthetic test case to simulate realpath failure + .catch(/* istanbul ignore next */() => null) + ) + + /* istanbul ignore else - same as the ignored catch above */ + if (real) { + const { name } = spec + spec = npa(`file:${relpath(this.path, real)}`, this.path) + spec.name = name + } + } + return spec + } + // TODO: provide a way to fix bundled deps by exposing metadata about // what's in the bundle at each published manifest. Without that, we // can't possibly fix bundled deps without breaking a ton of other stuff, diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index 0837112ac..588f0d3e2 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -223,6 +223,41 @@ Node { } ` +exports[`test/arborist/build-ideal-tree.js TAP add symlink that points to a symlink > should follow symlinks to find final realpath destination 1`] = ` +Node { + "children": Map { + "a" => Link { + "edgesIn": Set { + Edge { + "from": "", + "name": "a", + "spec": "file:../linked-pkg", + "type": "prod", + }, + }, + "location": "node_modules/a", + "name": "a", + "resolved": "file:../../linked-pkg", + "target": Object { + "name": "a", + "parent": null, + }, + }, + }, + "edgesOut": Map { + "a" => Edge { + "name": "a", + "spec": "file:../linked-pkg", + "to": "node_modules/a", + "type": "prod", + }, + }, + "location": "", + "name": "my-project", + "resolved": null, +} +` + exports[`test/arborist/build-ideal-tree.js TAP bad shrinkwrap file > bad shrinkwrap 1`] = ` Node { "children": Map { diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index c1eaee93b..192f2ab94 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -1321,6 +1321,42 @@ t.test('workspaces', t => { t.end() }) +t.test('add symlink that points to a symlink', t => { + const fixt = t.testdir({ + 'global-prefix': { + lib: { + node_modules: { + a: t.fixture('symlink', '../../../linked-pkg') + } + } + }, + 'linked-pkg': { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.0.0' + }) + }, + 'my-project': { + 'package.json': JSON.stringify({}) + } + }) + const path = resolve(fixt, 'my-project') + const arb = new Arborist({ + path, + ...OPT, + }) + return arb.buildIdealTree({ + add: [ + 'file:../global-prefix/lib/node_modules/a' + ] + }).then(tree => + t.matchSnapshot( + printTree(tree), + 'should follow symlinks to find final realpath destination' + ) + ) +}) + // if we get this wrong, it'll spin forever and use up all the memory t.test('pathologically nested dependency cycle', t => t.resolveMatchSnapshot(printIdeal( @@ -1349,24 +1385,35 @@ t.test('resolve files from cwd in global mode, Arb path in local mode', t => { }) t.only('resolve links in global mode', t => { - const cwd = process.cwd() - t.teardown(() => process.chdir(cwd)) const path = t.testdir({ - global: {} + global: {}, + lib: { + 'my-project': {} + }, + 'linked-dep': { + 'package.json': JSON.stringify({ + name: 'linked-dep', + version: '1.0.0' + }) + } }) - const fixturedir = resolve(fixtures, 'root-bundler') + const fixturedir = resolve(path, 'lib', 'my-project') + + const cwd = process.cwd() + t.teardown(() => process.chdir(cwd)) process.chdir(fixturedir) + const arb = new Arborist({ + ...OPT, global: true, path: resolve(path, 'global'), - ...OPT, }) return arb.buildIdealTree({ - add: ['file:../sax'], + add: ['file:../../linked-dep'], global: true, }).then(tree => { - const resolved = 'file:../../../../fixtures/sax' - t.equal(tree.children.get('sax').resolved, resolved) + const resolved = 'file:../../linked-dep' + t.equal(tree.children.get('linked-dep').resolved, resolved) }) }) From 6ae40a79e9943d24bee50ce159cc8f6e0a9d5c6a Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Tue, 15 Sep 2020 13:04:17 -0400 Subject: [PATCH 2/2] fixup! addressed CR --- lib/add-rm-pkg-deps.js | 3 ++- lib/arborist/build-ideal-tree.js | 10 +++++----- lib/arborist/load-actual.js | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/add-rm-pkg-deps.js b/lib/add-rm-pkg-deps.js index 9ec7456b0..d8afc0445 100644 --- a/lib/add-rm-pkg-deps.js +++ b/lib/add-rm-pkg-deps.js @@ -54,7 +54,8 @@ const addSingle = ({pkg, spec, saveBundle, saveType, path}) => { pkg[type] = pkg[type] || {} if (rawSpec !== '' || pkg[type][name] === undefined) { - // if we're in global mode, file specs are based on cwd, not arb path + // file specs are based on arb path, directories specs + // are normalized during build-ideal-tree instead pkg[type][name] = specType === 'file' ? `file:${relpath(path, fetchSpec)}` : (rawSpec || '*') diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index 5f147fa44..70c3b032b 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -80,8 +80,8 @@ const _globalStyle = Symbol('globalStyle') const _globalRootNode = Symbol('globalRootNode') const _isVulnerable = Symbol.for('isVulnerable') const _usePackageLock = Symbol.for('usePackageLock') -const _rpcache = Symbol('realpathCache') -const _stcache = Symbol('statCache') +const _rpcache = Symbol.for('realpathCache') +const _stcache = Symbol.for('statCache') const _followSymlinkPath = Symbol('followSymlinkPath') const _retrieveSpecName = Symbol('retrieveSpecName') @@ -139,8 +139,8 @@ module.exports = cls => class IdealTreeBuilder extends cls { // caches for cached realpath calls const cwd = process.cwd() // assume that the cwd is real enough for our purposes - this[_rpcache] = new Map([[ cwd, cwd ]]) - this[_stcache] = new Map() + this[_rpcache] = this[_rpcache] + this[_stcache] = this[_stcache] } get explicitRequests () { @@ -377,7 +377,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { } async [_followSymlinkPath] (spec, currpath) { - if(spec.type === 'directory') { + if (spec.type === 'directory') { const real = await ( realpath(spec.fetchSpec, this[_rpcache], this[_stcache]) // TODO: create synthetic test case to simulate realpath failure diff --git a/lib/arborist/load-actual.js b/lib/arborist/load-actual.js index 0fe82ba4a..ed57f4eee 100644 --- a/lib/arborist/load-actual.js +++ b/lib/arborist/load-actual.js @@ -24,8 +24,8 @@ const _findMissingEdges = Symbol('findMissingEdges') const _findFSParents = Symbol('findFSParents') const _actualTreeLoaded = Symbol('actualTreeLoaded') -const _rpcache = Symbol('realpathCache') -const _stcache = Symbol('statCache') +const _rpcache = Symbol.for('realpathCache') +const _stcache = Symbol.for('statCache') const _topNodes = Symbol('linkTargets') const _cache = Symbol('nodeLoadingCache') const _loadActual = Symbol('loadActual')