From 5b389022652abeb0e1c278a152550eb95bc6c452 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 1 Aug 2019 16:48:19 -0700 Subject: [PATCH] Handle unhandledRejections, help with cache eacces Suggested by @godmar in https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5 Incidentally, this turned up that we're catching uncaughtExceptions in the main npm functions, but not unhandledRejections! Tracing this through, it seems like node-fetch-npm's use of cacache is particularly brittle. Any throw that comes from cacache is not caught properly, since node-fetch-npm is all streams and callbacks. The naive approach (just adding a catch and failing the callback) doesn't work, because then make-fetch-happen and npm-registry-fetch interpret the failure as an invalid response, when actually it was a local cache error. So, a bit more love and polish is definitely still needed in the guts of npm's fetching and caching code paths. In the meantime, though, handling any unhandledRejection at the top level prevents at least the worst and most useless type of error message. PR-URL: https://github.com/npm/cli/pull/227 Credit: @isaacs Close: #227 Reviewed-by: @isaacs --- bin/npm-cli.js | 1 + lib/utils/error-handler.js | 7 ++-- lib/utils/error-message.js | 49 +++++++++++++++++++------- test/tap/cache-eacces-error-message.js | 38 ++++++++++++++++++++ 4 files changed, 80 insertions(+), 15 deletions(-) create mode 100644 test/tap/cache-eacces-error-message.js diff --git a/bin/npm-cli.js b/bin/npm-cli.js index 705aa472e7e5..93eddc7a3c89 100755 --- a/bin/npm-cli.js +++ b/bin/npm-cli.js @@ -62,6 +62,7 @@ log.info('using', 'node@%s', process.version) process.on('uncaughtException', errorHandler) + process.on('unhandledRejection', errorHandler) if (conf.usage && npm.command !== 'help') { npm.argv.unshift(npm.command) diff --git a/lib/utils/error-handler.js b/lib/utils/error-handler.js index 7cb43be2900e..39e0035c2728 100644 --- a/lib/utils/error-handler.js +++ b/lib/utils/error-handler.js @@ -187,11 +187,12 @@ function errorHandler (er) { log.verbose('npm ', 'v' + npm.version) ;[ + 'code', + 'syscall', 'file', 'path', - 'code', - 'errno', - 'syscall' + 'dest', + 'errno' ].forEach(function (k) { var v = er[k] if (v) log.error(k, v) diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index bf5d65c0df3c..ea8b05938c10 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -2,6 +2,7 @@ var npm = require('../npm.js') var util = require('util') var nameValidator = require('validate-npm-package-name') +var npmlog = require('npmlog') module.exports = errorMessage @@ -33,18 +34,42 @@ function errorMessage (er) { case 'EACCES': case 'EPERM': - short.push(['', er]) - detail.push([ - '', - [ - '\nThe operation was rejected by your operating system.', - (process.platform === 'win32' - ? 'It\'s possible that the file was already in use (by a text editor or antivirus),\nor that you lack permissions to access it.' - : 'It is likely you do not have the permissions to access this file as the current user'), - '\nIf you believe this might be a permissions issue, please double-check the', - 'permissions of the file and its containing directories, or try running', - 'the command again as root/Administrator (though this is not recommended).' - ].join('\n')]) + const isCachePath = typeof er.path === 'string' && + er.path.startsWith(npm.config.get('cache')) + const isCacheDest = typeof er.dest === 'string' && + er.dest.startsWith(npm.config.get('cache')) + + const isWindows = process.platform === 'win32' + + if (!isWindows && (isCachePath || isCacheDest)) { + // user probably doesn't need this, but still add it to the debug log + npmlog.verbose(er.stack) + short.push([ + '', + [ + '', + 'Your cache folder contains root-owned files, due to a bug in', + 'previous versions of npm which has since been addressed.', + '', + 'To permanently fix this problem, please run:', + ` sudo chown -R ${process.getuid()}:${process.getgid()} ${JSON.stringify(npm.config.get('cache'))}` + ].join('\n') + ]) + } else { + short.push(['', er]) + detail.push([ + '', + [ + '\nThe operation was rejected by your operating system.', + (process.platform === 'win32' + ? 'It\'s possible that the file was already in use (by a text editor or antivirus),\n' + + 'or that you lack permissions to access it.' + : 'It is likely you do not have the permissions to access this file as the current user'), + '\nIf you believe this might be a permissions issue, please double-check the', + 'permissions of the file and its containing directories, or try running', + 'the command again as root/Administrator.' + ].join('\n')]) + } break case 'ELIFECYCLE': diff --git a/test/tap/cache-eacces-error-message.js b/test/tap/cache-eacces-error-message.js new file mode 100644 index 000000000000..aa112eba4392 --- /dev/null +++ b/test/tap/cache-eacces-error-message.js @@ -0,0 +1,38 @@ +const npm = require('../../lib/npm.js') +const t = require('tap') + +if (process.platform === 'win32') { + t.plan(0, 'this is a unix-only thing') + process.exit(0) +} + +const errorMessage = require('../../lib/utils/error-message.js') + +const common = require('../common-tap.js') + +t.plan(1) + +npm.load({ cache: common.cache }, () => { + npm.config.set('cache', common.cache) + const er = new Error('access is e, i am afraid') + er.code = 'EACCES' + er.errno = -13 + er.path = common.cache + '/src' + er.dest = common.cache + '/to' + + t.match(errorMessage(er), { + summary: [ + [ + '', + new RegExp('\n' + + 'Your cache folder contains root-owned files, due to a bug in\n' + + 'previous versions of npm which has since been addressed.\n' + + '\n' + + 'To permanently fix this problem, please run:\n' + + ' sudo chown -R [0-9]+:[0-9]+ ".*npm_cache_cache-eacces-error-message"' + ) + ] + ], + detail: [] + }, 'get the helpful error message') +})