From 320ac9aeeafd11bb693c53b31148b8d10c4165e8 Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Tue, 10 Dec 2019 23:38:55 -0800
Subject: [PATCH] Do not remove global bin/man links inappropriately

Prevent a global install from overwriting bins and manpages if they are
not links/shims that npm controls, or if then are links/shims to
packages other than the one being installed.

Changes error message output on EEXIST errors to be more helpful.

Related:

- https://github.com/npm/bin-links/pull/12
- https://github.com/npm/gentle-fs/pull/7

Note: this does NOT prevent packages from overwriting one another's bins
in non-global package installs, because doing so would introduce a
[dependency hell](https://en.wikipedia.org/wiki/Dependency_hell) that
npm 6 is not capable of avoiding without significant refactoring.  The
collision detection in npm v7's tree building will enable us to explore
such an option, by never placing dependencies in the same place if they
would write the same bin script.  (It's fundamentally similar to
peerDependency resolution, but much simpler.)

Since users have not complained about this potential foot-gun in the
last 5 years, its unlikely that it is a significant issue, and
introducing additional dependency nesting (or worse, failing installs
for unresolveable trees) is likely an even worse hazard.  If we do
prevent non-global-top installs from overwriting one another's bins, it
ought to be done only as best-effort (ie, allow the collision if both
deps need to be placed in the same node_modules folder) and perhaps
opt-in with a config flag.
---
 lib/utils/error-message.js  |   5 +-
 test/tap/bin-overwriting.js | 107 ++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 2 deletions(-)
 create mode 100644 test/tap/bin-overwriting.js

diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js
index 5ddfb37682a2..12f304d1e894 100644
--- a/lib/utils/error-message.js
+++ b/lib/utils/error-message.js
@@ -280,8 +280,9 @@ function errorMessage (er) {
 
     case 'EEXIST':
       short.push(['', er.message])
-      short.push(['', 'File exists: ' + er.path])
-      detail.push(['', 'Move it away, and try again.'])
+      short.push(['', 'File exists: ' + (er.dest || er.path)])
+      detail.push(['', 'Remove the existing file and try again, or run npm'])
+      detail.push(['', 'with --force to overwrite files recklessly.'])
       break
 
     case 'ENEEDAUTH':
diff --git a/test/tap/bin-overwriting.js b/test/tap/bin-overwriting.js
new file mode 100644
index 000000000000..155d4abf5205
--- /dev/null
+++ b/test/tap/bin-overwriting.js
@@ -0,0 +1,107 @@
+const t = require('tap')
+const common = require('../common-tap.js')
+const pkg = common.pkg
+
+const { writeFileSync, readFileSync, readlink } = require('fs')
+const readCmdShim = require('read-cmd-shim')
+const path = require('path')
+const readBinCb = process.platform === 'win32' ? readCmdShim : readlink
+const readBin = bin => new Promise((resolve, reject) => {
+  readBinCb(bin, (er, target) => {
+    if (er)
+      reject(er)
+    else
+      resolve(path.resolve(pkg + '/global/bin', target))
+  })
+})
+
+// verify that we can't overwrite bins that we shouldn't be able to
+
+const mkdirp = require('mkdirp').sync
+
+process.env.npm_config_prefix = pkg + '/global'
+process.env.npm_config_global = true
+
+const globalBin = process.platform === 'win32'
+  ? path.resolve(pkg, 'global')
+  : path.resolve(pkg, 'global/bin')
+
+const globalDir = process.platform === 'win32'
+  ? path.resolve(pkg, 'global/node_modules')
+  : path.resolve(pkg, 'global/lib/node_modules')
+
+const beep = path.resolve(globalBin, 'beep')
+const firstBin = path.resolve(globalDir, 'first/first.js')
+const secondBin = path.resolve(globalDir, 'second/second.js')
+
+t.test('setup', { bail: true }, t => {
+  // set up non-link bin in place
+  mkdirp(globalBin)
+  writeFileSync(beep, 'beep boop')
+
+  // create first package
+  mkdirp(pkg + '/first')
+  writeFileSync(pkg + '/first/package.json', JSON.stringify({
+    name: 'first',
+    version: '1.0.0',
+    bin: { beep: 'first.js' }
+  }))
+  writeFileSync(pkg + '/first/first.js', `#!/usr/bin/env node
+    console.log('first')`)
+
+  // create second package
+  mkdirp(pkg + '/second')
+  writeFileSync(pkg + '/second/package.json', JSON.stringify({
+    name: 'second',
+    version: '1.0.0',
+    bin: { beep: 'second.js' }
+  }))
+  writeFileSync(pkg + '/second/second.js', `#!/usr/bin/env node
+    console.log('second')`)
+
+  // pack both to install globally
+  return common.npm(['pack'], { cwd: pkg + '/first' })
+    .then(() => common.npm(['pack'], { cwd: pkg + '/second' }))
+})
+
+t.test('installing first fails, because pre-existing bin in place', t => {
+  return common.npm([
+    'install',
+    pkg + '/first/first-1.0.0.tgz'
+  ]).then(([code, stdout, stderr]) => {
+    t.notEqual(code, 0)
+    t.match(stderr, 'EEXIST')
+    t.equal(readFileSync(beep, 'utf8'), 'beep boop')
+  })
+})
+
+t.test('installing first with --force succeeds', t => {
+  return common.npm([
+    'install',
+    pkg + '/first/first-1.0.0.tgz',
+    '--force'
+  ]).then(() => {
+    return t.resolveMatch(readBin(beep), firstBin, 'bin written to first.js')
+  })
+})
+
+t.test('installing second fails, because bin links to other package', t => {
+  return common.npm([
+    'install',
+    pkg + '/second/second-1.0.0.tgz'
+  ]).then(([code, stdout, stderr]) => {
+    t.notEqual(code, 0)
+    t.match(stderr, 'EEXIST')
+    return t.resolveMatch(readBin(beep), firstBin, 'bin still linked to first')
+  })
+})
+
+t.test('installing second with --force succeeds', t => {
+  return common.npm([
+    'install',
+    pkg + '/second/second-1.0.0.tgz',
+    '--force'
+  ]).then(() => {
+    return t.resolveMatch(readBin(beep), secondBin, 'bin written to second.js')
+  })
+})