Skip to content

Commit

Permalink
fix: Better error on invalid --user/--group
Browse files Browse the repository at this point in the history
This addresses the issue when people fail to install binary packages on
Docker and other environments where there is no 'nobody' user.

The Config.loadUid() method was failing in these cases, but not in a way
that was particularly informative.  Furthermore, the uid and gid
resolved in that way were just ignored and never stored anywhere.

However, until npm-lifecycle@3.1.3, an error from uid-number was _also_
ignored, so if we didn't crash somewhere, then it would run scripts as
root when provided with an invalid user.  This is arguably fine, but it
is a violation of the contract that the npm CLI presents.

As of npm-lifecycle@3.1.3, these errors are handled properly in
npm-lifecycle, so the additional uninformative crash is no longer doing
anything.  This commit removes that uninformative crash.  This also
means that we won't fail _until_ an invalid user config is actually
relevant; if someone never runs an install script (or runs with
--ignore-scripts), then it's not relevant, so we can move forward
anyway.
  • Loading branch information
isaacs committed Aug 12, 2019
1 parent a4e2795 commit 769d2e0
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 21 deletions.
8 changes: 2 additions & 6 deletions lib/config/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,6 @@ function Conf (base) {

Conf.prototype.loadPrefix = require('./load-prefix.js')
Conf.prototype.loadCAFile = require('./load-cafile.js')
Conf.prototype.loadUid = require('./load-uid.js')
Conf.prototype.setUser = require('./set-user.js')
Conf.prototype.getCredentialsByURI = require('./get-credentials-by-uri.js')
Conf.prototype.setCredentialsByURI = require('./set-credentials-by-uri.js')
Expand All @@ -227,11 +226,8 @@ Conf.prototype.clearCredentialsByURI = require('./clear-credentials-by-uri.js')
Conf.prototype.loadExtras = function (cb) {
this.setUser(function (er) {
if (er) return cb(er)
this.loadUid(function (er) {
if (er) return cb(er)
// Without prefix, nothing will ever work
mkdirp(this.prefix, cb)
}.bind(this))
// Without prefix, nothing will ever work
mkdirp(this.prefix, cb)
}.bind(this))
}

Expand Down
15 changes: 0 additions & 15 deletions lib/config/load-uid.js

This file was deleted.

14 changes: 14 additions & 0 deletions lib/utils/error-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,20 @@ function errorMessage (er) {
}
break

case 'EUIDLOOKUP':
short.push(['lifecycle', er.message])
detail.push([
'',
[
'',
'Failed to look up the user/group for running scripts.',
'',
'Try again with a different --user or --group settings, or',
'run with --unsafe-perm to execute scripts as root.'
].join('\n')
])
break

case 'ELIFECYCLE':
short.push(['', er.message])
detail.push([
Expand Down

0 comments on commit 769d2e0

Please sign in to comment.