reify: debug crash when extracting into symlink

We should never be doing this.  Any time we're extracting contents, it
should be into an honest to goodness directory, and not any other kind
of thing.

Commits to follow will make it impossible to reach this condition
without some very deep hacking, but the check is here to make it easier
to debug if anything like this is reported again.

Related-to: npm/cli#3457

Author: isaacs

lib/arborist/reify.js

@@ -614,6 +614,16 @@ module.exports = cls => class Reifier extends cls {
       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,

test/arborist/reify.js

@@ -18,6 +18,8 @@ rimrafMock.sync = (...args) => {
     throw new Error('rimraf fail')
 }
 const fs = require('fs')
+const { promisify } = require('util')
+const symlink = promisify(fs.symlink)
 let failRename = null
 let failRenameOnce = null
 let failMkdir = null
@@ -77,10 +79,23 @@ const warningTracker = () => {
   }
 }
 
+const debugLogTracker = () => {
+  const list = []
+  mockDebug.log = (...msg) => list.push(msg)
+  return () => {
+    mockDebug.log = () => {}
+    return list
+  }
+}
+const mockDebug = Object.assign(fn => fn(), { log: () => {} })
+
 const Arborist = t.mock('../../lib/index.js', {
   ...mocks,
   // need to not mock this one, so we still can swap the process object
   '../../lib/signal-handling.js': require('../../lib/signal-handling.js'),
+
+  // mock the debug so we can test these even when not enabled
+  '../../lib/debug.js': mockDebug,
 })
 
 const { Node, Link, Shrinkwrap } = Arborist
@@ -2304,3 +2319,40 @@ t.test('node_modules may not be a symlink', async t => {
   t.matchSnapshot(tree)
   t.matchSnapshot(warnings())
 })
+
+t.test('never unpack into anything other than a real directory', async t => {
+  const kUnpack = Symbol.for('unpackNewModules')
+  const unpackNewModules = Arborist.prototype[kUnpack]
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      dependencies: {
+        once: '',
+        wrappy: 'file:target',
+      },
+    }),
+    target: {
+      donotdeleteme: 'please do not delete this',
+      'package.json': JSON.stringify({
+        name: 'donotclobberme',
+        version: '1.2.3-please-do-not-clobber',
+      }),
+    },
+  })
+  const arb = newArb({ path })
+  const logs = debugLogTracker()
+  const wrappy = resolve(path, 'node_modules/once/node_modules/wrappy')
+  arb[kUnpack] = async () => {
+    // will have already created it
+    realRimraf.sync(wrappy)
+    const target = resolve(path, 'target')
+    await symlink(target, wrappy, 'junction')
+    arb[kUnpack] = unpackNewModules
+    return arb[kUnpack]()
+  }
+  await t.rejects(arb.reify({ path }), {
+    message: 'ENOTDIR: not a directory',
+    code: 'ENOTDIR',
+    path: wrappy,
+  })
+  t.match(logs(), [['unpacking into a non-directory', { path: wrappy }]])
+})