From 769d2e057daf5a2cbfe0ce86f02550e59825a691 Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 12 Aug 2019 14:06:33 -0700 Subject: [PATCH] fix: Better error on invalid --user/--group 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. --- lib/config/core.js | 8 ++------ lib/config/load-uid.js | 15 --------------- lib/utils/error-message.js | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 21 deletions(-) delete mode 100644 lib/config/load-uid.js diff --git a/lib/config/core.js b/lib/config/core.js index b9851f98d0e0c..36605555ff77a 100644 --- a/lib/config/core.js +++ b/lib/config/core.js @@ -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') @@ -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)) } diff --git a/lib/config/load-uid.js b/lib/config/load-uid.js deleted file mode 100644 index 859eac7494bc7..0000000000000 --- a/lib/config/load-uid.js +++ /dev/null @@ -1,15 +0,0 @@ -module.exports = loadUid - -var getUid = require('uid-number') - -// Call in the context of a npmconf object - -function loadUid (cb) { - // if we're not in unsafe-perm mode, then figure out who - // to run stuff as. Do this first, to support `npm update npm -g` - if (!this.get('unsafe-perm')) { - getUid(this.get('user'), this.get('group'), cb) - } else { - process.nextTick(cb) - } -} diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index ea8b05938c108..7835a1cdfcfa2 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -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([