Skip to content

Commit

Permalink
Do not remove global bin/man links inappropriately
Browse files Browse the repository at this point in the history
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:

- npm/bin-links#12
- npm/gentle-fs#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.
  • Loading branch information
isaacs committed Dec 11, 2019
1 parent d06f5c0 commit 320ac9a
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 2 deletions.
5 changes: 3 additions & 2 deletions lib/utils/error-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down
107 changes: 107 additions & 0 deletions test/tap/bin-overwriting.js
Original file line number Diff line number Diff line change
@@ -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')
})
})

0 comments on commit 320ac9a

Please # to comment.