fix: node_modules must be a directory

This adds a check to the extraction process to ensure that the
node_modules into which a node is being extracted is a true directory,
and not a symbolic link.

This is performed at every level, so that the symlink cannot be hidden
within a workspace, or created via some other creative means.

If `--force` is set, then the safety measure is disabled.

lib/arborist/reify.js

@@ -21,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')
@@ -78,6 +79,8 @@ 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.for('reifyPackages')
 
@@ -121,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
@@ -161,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] () {
@@ -560,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
@@ -587,13 +607,19 @@ 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 pacote.extract(res, node.path, {
         ...this.options,
         resolved: node.resolved,
         integrity: node.integrity,
       })
+    }
   }
 
   async [_symlink] (node) {

tap-snapshots/test/arborist/reify.js.test.cjs

@@ -4660,6 +4660,40 @@ ArboristNode {
 }
 `
 
+exports[`test/arborist/reify.js TAP move aside symlink clutter > must match snapshot 2`] = `
+ArboristNode {
+  "children": Map {
+    "abbrev" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "abbrev",
+          "spec": "latest",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/abbrev",
+      "name": "abbrev",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-move-aside-symlink-clutter/node_modules/abbrev",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "version": "1.1.1",
+    },
+  },
+  "edgesOut": Map {
+    "abbrev" => EdgeOut {
+      "name": "abbrev",
+      "spec": "latest",
+      "to": "node_modules/abbrev",
+      "type": "prod",
+    },
+  },
+  "isProjectRoot": true,
+  "location": "",
+  "name": "tap-testdir-reify-move-aside-symlink-clutter",
+  "path": "{CWD}/test/arborist/tap-testdir-reify-move-aside-symlink-clutter",
+}
+`
+
 exports[`test/arborist/reify.js TAP multiple bundles at the same level > must match snapshot 1`] = `
 ArboristNode {
   "children": Map {
@@ -16699,6 +16733,51 @@ exports[`test/arborist/reify.js TAP no saveType: prod w/ peer > must match snaps
 {"dependencies":{"abbrev":"^1.1.1"}}
 `
 
+exports[`test/arborist/reify.js TAP node_modules may not be a symlink > must match snapshot 1`] = `
+ArboristNode {
+  "children": Map {
+    "abbrev" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "abbrev",
+          "spec": "*",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/abbrev",
+      "name": "abbrev",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-node_modules-may-not-be-a-symlink/node_modules/abbrev",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "version": "1.1.1",
+    },
+  },
+  "edgesOut": Map {
+    "abbrev" => EdgeOut {
+      "name": "abbrev",
+      "spec": "*",
+      "to": "node_modules/abbrev",
+      "type": "prod",
+    },
+  },
+  "isProjectRoot": true,
+  "location": "",
+  "name": "tap-testdir-reify-node_modules-may-not-be-a-symlink",
+  "path": "{CWD}/test/arborist/tap-testdir-reify-node_modules-may-not-be-a-symlink",
+}
+`
+
+exports[`test/arborist/reify.js TAP node_modules may not be a symlink > must match snapshot 2`] = `
+Array [
+  Array [
+    "warn",
+    "reify",
+    "Removing non-directory",
+    "{CWD}/test/arborist/tap-testdir-reify-node_modules-may-not-be-a-symlink/node_modules",
+  ],
+]
+`
+
 exports[`test/arborist/reify.js TAP omit peer deps > finished timers 1`] = `
 Array [
   "arborist:ctor",

test/arborist/reify.js

@@ -1,4 +1,3 @@
-process.env.ARBORIST_DEBUG = '1'
 const {resolve, basename} = require('path')
 const t = require('tap')
 const runScript = require('@npmcli/run-script')
@@ -1014,17 +1013,18 @@ t.test('saving the ideal tree', t => {
   t.end()
 })
 
-t.test('scoped registries', t => {
+t.test('scoped registries', async t => {
   const path = t.testdir()
 
   // this is a very artifical test that is setting a lot of internal things
   // up so that we assert that the intended behavior of sending right
   // resolved data for pacote.extract is working as intended, alternatively
   // we might prefer to replace this with a proper parallel alternative
   // registry server so that we can have more of an integration test instead
-  t.plan(1)
+  let sawPacoteExtract = false
   const pacote = {
     extract: res => {
+      sawPacoteExtract = true
       t.matchSnapshot(
         res,
         'should preserve original resolved value'
@@ -1043,14 +1043,17 @@ t.test('scoped registries', t => {
     registry,
   })
   const kReify = Symbol.for('reifyNode')
+  a.idealTree = new Node({ path })
 
   const node = new Node({
     name: '@ruyadorno/theoretically-private-pkg',
     resolved: 'https://npm.pkg.github.com/@ruyadorno/' +
       'theoretically-private-pkg/-/theoretically-private-pkg-1.2.3.tgz',
     pkg: { version: '1.2.3', name: '@ruyadorno/theoretically-private-pkg' },
+    parent: a.idealTree,
   })
-  a[kReify](node)
+  await a[kReify](node)
+  t.equal(sawPacoteExtract, true, 'saw pacote extraction')
 })
 
 t.test('bin links adding and removing', t => {
@@ -2230,3 +2233,74 @@ t.test('collide case-variant dep names', async t => {
     version: '1.1.1',
   })
 })
+
+t.test('move aside symlink clutter', async t => {
+  // have to make the clutter manually, because we collide packages based
+  // on case-insensitive names, so the ABBREV folder would be removed.
+  // not sure how this would ever happen, but defense in depth.
+  const kReifyPackages = Symbol.for('reifyPackages')
+  const reifyPackages = Arborist.prototype[kReifyPackages]
+  Arborist.prototype[kReifyPackages] = async function () {
+    fs.mkdirSync(path + '/node_modules')
+    fs.symlinkSync('../target', path + '/node_modules/ABBREV')
+    Arborist.prototype[kReifyPackages] = reifyPackages
+    return this[kReifyPackages]()
+  }
+
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      dependencies: {
+        abbrev: 'latest',
+      },
+    }),
+    target: {
+      file: 'do not delete me please',
+      'package.json': JSON.stringify({ name: 'ABBREV', version: '1.0.0' }),
+    },
+    'sensitivity-test': t.fixture('symlink', './target'),
+  })
+
+  // check to see if we're on a case-insensitive fs
+  try {
+    const st = fs.lstatSync(path + '/SENSITIVITY-TEST')
+    t.equal(st.isSymbolicLink(), true, 'fs is case insensitive')
+  } catch (er) {
+    t.plan(0, 'case sensitive file system, test not relevant')
+    return
+  }
+
+  const tree = await printReified(path)
+  const st = fs.lstatSync(path + '/node_modules/abbrev')
+  t.equal(st.isSymbolicLink(), false)
+  t.equal(st.isDirectory(), true)
+  t.equal(fs.readFileSync(path + '/target/file', 'utf8'),
+    'do not delete me please')
+  const linkPJ = fs.readFileSync(path + '/target/package.json', 'utf8')
+  t.strictSame(JSON.parse(linkPJ), {
+    name: 'ABBREV',
+    version: '1.0.0',
+  })
+  const abbrevPJ = fs.readFileSync(path + '/node_modules/abbrev/package.json', 'utf8')
+  t.match(JSON.parse(abbrevPJ), {
+    name: 'abbrev',
+    version: '1.1.1',
+  })
+
+  t.matchSnapshot(tree)
+})
+
+t.test('node_modules may not be a symlink', async t => {
+  const path = t.testdir({
+    target: {},
+    node_modules: t.fixture('symlink', 'target'),
+    'package.json': JSON.stringify({
+      dependencies: {
+        abbrev: '',
+      },
+    }),
+  })
+  const warnings = warningTracker()
+  const tree = await printReified(path)
+  t.matchSnapshot(tree)
+  t.matchSnapshot(warnings())
+})