From 0e59fdafe7f0b36d4de0d1e1a294df603ef1dceb Mon Sep 17 00:00:00 2001 From: nlf Date: Thu, 6 Oct 2022 12:38:02 -0700 Subject: [PATCH 1/3] deps: @npmcli/config@5.0.0 --- node_modules/@npmcli/config/lib/errors.js | 22 +++ node_modules/@npmcli/config/lib/index.js | 198 ++++++++++++---------- node_modules/@npmcli/config/package.json | 17 +- package-lock.json | 9 +- package.json | 2 +- 5 files changed, 143 insertions(+), 105 deletions(-) create mode 100644 node_modules/@npmcli/config/lib/errors.js diff --git a/node_modules/@npmcli/config/lib/errors.js b/node_modules/@npmcli/config/lib/errors.js new file mode 100644 index 0000000000000..fa3e20798542a --- /dev/null +++ b/node_modules/@npmcli/config/lib/errors.js @@ -0,0 +1,22 @@ +'use strict' + +class ErrInvalidAuth extends Error { + constructor (problems) { + let message = 'Invalid auth configuration found: ' + message += problems.map((problem) => { + if (problem.action === 'delete') { + return `\`${problem.key}\` is not allowed in ${problem.where} config` + } else if (problem.action === 'rename') { + return `\`${problem.from}\` must be renamed to \`${problem.to}\` in ${problem.where} config` + } + }).join(', ') + message += '\nPlease run `npm config fix` to repair your configuration.`' + super(message) + this.code = 'ERR_INVALID_AUTH' + this.problems = problems + } +} + +module.exports = { + ErrInvalidAuth, +} diff --git a/node_modules/@npmcli/config/lib/index.js b/node_modules/@npmcli/config/lib/index.js index fe5cfd2aa9ed5..572f5fe00a9b6 100644 --- a/node_modules/@npmcli/config/lib/index.js +++ b/node_modules/@npmcli/config/lib/index.js @@ -51,6 +51,10 @@ const parseField = require('./parse-field.js') const typeDescription = require('./type-description.js') const setEnvs = require('./set-envs.js') +const { + ErrInvalidAuth, +} = require('./errors.js') + // types that can be saved back to const confFileTypes = new Set([ 'global', @@ -280,26 +284,10 @@ class Config { await this.loadGlobalConfig() process.emit('timeEnd', 'config:load:global') - // warn if anything is not valid - process.emit('time', 'config:load:validate') - this.validate() - process.emit('timeEnd', 'config:load:validate') - // set this before calling setEnvs, so that we don't have to share // symbols, as that module also does a bunch of get operations this[_loaded] = true - process.emit('time', 'config:load:credentials') - const reg = this.get('registry') - const creds = this.getCredentialsByURI(reg) - // ignore this error because a failed set will strip out anything that - // might be a security hazard, which was the intention. - try { - this.setCredentialsByURI(reg, creds) - // eslint-disable-next-line no-empty - } catch (_) {} - process.emit('timeEnd', 'config:load:credentials') - // set proper globalPrefix now that everything is loaded this.globalPrefix = this.get('prefix') @@ -399,7 +387,9 @@ class Config { validate (where) { if (!where) { let valid = true - for (const [entryWhere] of this.data.entries()) { + const authProblems = [] + + for (const entryWhere of this.data.keys()) { // no need to validate our defaults, we know they're fine // cli was already validated when parsed the first time if (entryWhere === 'default' || entryWhere === 'builtin' || entryWhere === 'cli') { @@ -407,7 +397,48 @@ class Config { } const ret = this.validate(entryWhere) valid = valid && ret + + if (['global', 'user', 'project'].includes(entryWhere)) { + // after validating everything else, we look for old auth configs we no longer support + // if these keys are found, we build up a list of them and the appropriate action and + // attach it as context on the thrown error + + // first, keys that should be removed + for (const key of ['_authtoken', '-authtoken']) { + if (this.get(key, entryWhere)) { + authProblems.push({ action: 'delete', key, where: entryWhere }) + } + } + + // NOTE we pull registry without restricting to the current 'where' because we want to + // suggest scoping things to the registry they would be applied to, which is the default + // regardless of where it was defined + const nerfedReg = nerfDart(this.get('registry')) + // keys that should be nerfed but currently are not + for (const key of ['_auth', '_authToken', 'username', '_password']) { + if (this.get(key, entryWhere)) { + // username and _password must both exist in the same file to be recognized correctly + if (key === 'username' && !this.get('_password', entryWhere)) { + authProblems.push({ action: 'delete', key, where: entryWhere }) + } else if (key === '_password' && !this.get('username', entryWhere)) { + authProblems.push({ action: 'delete', key, where: entryWhere }) + } else { + authProblems.push({ + action: 'rename', + from: key, + to: `${nerfedReg}:${key}`, + where: entryWhere, + }) + } + } + } + } } + + if (authProblems.length) { + throw new ErrInvalidAuth(authProblems) + } + return valid } else { const obj = this.data.get(where) @@ -423,6 +454,40 @@ class Config { } } + // fixes problems identified by validate(), accepts the 'problems' property from a thrown + // ErrInvalidAuth to avoid having to check everything again + repair (problems) { + if (!problems) { + try { + this.validate() + } catch (err) { + // coverage skipped here because we don't need to test re-throwing an error + // istanbul ignore next + if (err.code !== 'ERR_INVALID_AUTH') { + throw err + } + + problems = err.problems + } finally { + if (!problems) { + problems = [] + } + } + } + + for (const problem of problems) { + // coverage disabled for else branch because it doesn't do anything and shouldn't + // istanbul ignore else + if (problem.action === 'delete') { + this.delete(problem.key, problem.where) + } else if (problem.action === 'rename') { + const old = this.get(problem.from, problem.where) + this.set(problem.to, old, problem.where) + this.delete(problem.from, problem.where) + } + } + } + // Returns true if the value is coming directly from the source defined // in default definitions, if the current value for the key config is // coming from any other different source, returns false @@ -644,21 +709,19 @@ class Config { if (!confFileTypes.has(where)) { throw new Error('invalid config location param: ' + where) } + const conf = this.data.get(where) conf[_raw] = { ...conf.data } conf[_loadError] = null - // upgrade auth configs to more secure variants before saving if (where === 'user') { - const reg = this.get('registry') - const creds = this.getCredentialsByURI(reg) - // we ignore this error because the failed set already removed - // anything that might be a security hazard, and it won't be - // saved back to the .npmrc file, so we're good. - try { - this.setCredentialsByURI(reg, creds) - // eslint-disable-next-line no-empty - } catch (_) {} + // if email is nerfed, then we want to de-nerf it + const nerfed = nerfDart(this.get('registry')) + const email = this.get(`${nerfed}:email`, 'user') + if (email) { + this.delete(`${nerfed}:email`, 'user') + this.set('email', email, 'user') + } } const iniData = ini.stringify(conf.data).trim() + '\n' @@ -686,14 +749,17 @@ class Config { const nerfed = nerfDart(uri) const def = nerfDart(this.get('registry')) if (def === nerfed) { - // do not delete email, that shouldn't be nerfed any more. - // just delete the nerfed copy, if one exists. this.delete(`-authtoken`, 'user') this.delete(`_authToken`, 'user') this.delete(`_authtoken`, 'user') this.delete(`_auth`, 'user') this.delete(`_password`, 'user') this.delete(`username`, 'user') + // de-nerf email if it's nerfed to the default registry + const email = this.get(`${nerfed}:email`, 'user') + if (email) { + this.set('email', email, 'user') + } } this.delete(`${nerfed}:_authToken`, 'user') this.delete(`${nerfed}:_auth`, 'user') @@ -706,28 +772,9 @@ class Config { setCredentialsByURI (uri, { token, username, password, email, certfile, keyfile }) { const nerfed = nerfDart(uri) - const def = nerfDart(this.get('registry')) - if (def === nerfed) { - // remove old style auth info not limited to a single registry - this.delete('_password', 'user') - this.delete('username', 'user') - this.delete('_auth', 'user') - this.delete('_authtoken', 'user') - this.delete('-authtoken', 'user') - this.delete('_authToken', 'user') - } - - // email used to be nerfed always. if we're using the default - // registry, de-nerf it. - if (nerfed === def) { - email = email || - this.get('email', 'user') || - this.get(`${nerfed}:email`, 'user') - if (email) { - this.set('email', email, 'user') - } - } + // email is either provided, a top level key, or nothing + email = email || this.get('email', 'user') // field that hasn't been used as documented for a LONG time, // and as of npm 7.10.0, isn't used at all. We just always @@ -765,15 +812,17 @@ class Config { // this has to be a bit more complicated to support legacy data of all forms getCredentialsByURI (uri) { const nerfed = nerfDart(uri) + const def = nerfDart(this.get('registry')) const creds = {} - const deprecatedAuthWarning = [ - '`_auth`, `_authToken`, `username` and `_password` must be scoped to a registry.', - 'see `npm help npmrc` for more information.', - ].join(' ') - + // email is handled differently, it used to always be nerfed and now it never should be + // if it's set nerfed to the default registry, then we copy it to the unnerfed key + // TODO: evaluate removing 'email' from the credentials object returned here const email = this.get(`${nerfed}:email`) || this.get('email') if (email) { + if (nerfed === def) { + this.set('email', email, 'user') + } creds.email = email } @@ -785,13 +834,8 @@ class Config { // cert/key may be used in conjunction with other credentials, thus no `return` } - const defaultToken = nerfDart(this.get('registry')) && this.get('_authToken') - const tokenReg = this.get(`${nerfed}:_authToken`) || defaultToken - + const tokenReg = this.get(`${nerfed}:_authToken`) if (tokenReg) { - if (tokenReg === defaultToken) { - log.warn('config', deprecatedAuthWarning) - } creds.token = tokenReg return creds } @@ -816,37 +860,7 @@ class Config { return creds } - // at this point, we can only use the values if the URI is the - // default registry. - const defaultNerf = nerfDart(this.get('registry')) - if (nerfed !== defaultNerf) { - return creds - } - - const userDef = this.get('username') - const passDef = this.get('_password') - if (userDef && passDef) { - log.warn('config', deprecatedAuthWarning) - creds.username = userDef - creds.password = Buffer.from(passDef, 'base64').toString('utf8') - const auth = `${creds.username}:${creds.password}` - creds.auth = Buffer.from(auth, 'utf8').toString('base64') - return creds - } - - // Handle the old-style _auth= style for the default - // registry, if set. - const auth = this.get('_auth') - if (!auth) { - return creds - } - - log.warn('config', deprecatedAuthWarning) - const authDecode = Buffer.from(auth, 'base64').toString('utf8') - const authSplit = authDecode.split(':') - creds.username = authSplit.shift() - creds.password = authSplit.join(':') - creds.auth = auth + // at this point, nothing else is usable so just return what we do have return creds } diff --git a/node_modules/@npmcli/config/package.json b/node_modules/@npmcli/config/package.json index 81c36228c6b4a..0d224611ac204 100644 --- a/node_modules/@npmcli/config/package.json +++ b/node_modules/@npmcli/config/package.json @@ -1,6 +1,6 @@ { "name": "@npmcli/config", - "version": "4.2.2", + "version": "5.0.0", "files": [ "bin/", "lib/" @@ -16,9 +16,6 @@ "scripts": { "test": "tap", "snap": "tap", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "lint": "eslint \"**/*.js\"", "postlint": "template-oss-check", "lintfix": "npm run lint -- --fix", @@ -27,11 +24,15 @@ }, "tap": { "check-coverage": true, - "coverage-map": "map.js" + "coverage-map": "map.js", + "nyc-arg": [ + "--exclude", + "tap-snapshots/**" + ] }, "devDependencies": { "@npmcli/eslint-config": "^3.0.1", - "@npmcli/template-oss": "3.6.0", + "@npmcli/template-oss": "4.5.0", "tap": "^16.0.1" }, "dependencies": { @@ -45,10 +46,10 @@ "walk-up-path": "^1.0.0" }, "engines": { - "node": "^12.13.0 || ^14.15.0 || >=16.0.0" + "node": "^14.17.0 || ^16.13.0 || >=18.0.0" }, "templateOSS": { "//@npmcli/template-oss": "This file is partially managed by @npmcli/template-oss. Edits may be overwritten.", - "version": "3.6.0" + "version": "4.5.0" } } diff --git a/package-lock.json b/package-lock.json index f36c790095c30..c6db183c82fff 100644 --- a/package-lock.json +++ b/package-lock.json @@ -92,7 +92,7 @@ "@isaacs/string-locale-compare": "^1.1.0", "@npmcli/arborist": "^6.0.0-pre.4", "@npmcli/ci-detect": "^3.0.0", - "@npmcli/config": "^4.2.1", + "@npmcli/config": "^5.0.0", "@npmcli/fs": "^2.1.0", "@npmcli/map-workspaces": "^2.0.3", "@npmcli/package-json": "^2.0.0", @@ -2164,9 +2164,10 @@ } }, "node_modules/@npmcli/config": { - "version": "4.2.2", + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/@npmcli/config/-/config-5.0.0.tgz", + "integrity": "sha512-TfJ3IRw5eKtzvzCxWbmy74KfO1ikKoWr2oPzpugo3RqSneAF/PNFZuSAuubvyv5qKjAj0hU4BC7VI2o3eOAT2A==", "inBundle": true, - "license": "ISC", "dependencies": { "@npmcli/map-workspaces": "^2.0.2", "ini": "^3.0.0", @@ -2178,7 +2179,7 @@ "walk-up-path": "^1.0.0" }, "engines": { - "node": "^12.13.0 || ^14.15.0 || >=16.0.0" + "node": "^14.17.0 || ^16.13.0 || >=18.0.0" } }, "node_modules/@npmcli/disparity-colors": { diff --git a/package.json b/package.json index b952e64c4fe3e..e8f27f20da441 100644 --- a/package.json +++ b/package.json @@ -58,7 +58,7 @@ "@isaacs/string-locale-compare": "^1.1.0", "@npmcli/arborist": "^6.0.0-pre.4", "@npmcli/ci-detect": "^3.0.0", - "@npmcli/config": "^4.2.1", + "@npmcli/config": "^5.0.0", "@npmcli/fs": "^2.1.0", "@npmcli/map-workspaces": "^2.0.3", "@npmcli/package-json": "^2.0.0", From 5fa1f7933bf4af5dbf9798c5c3db5a6c755d9a39 Mon Sep 17 00:00:00 2001 From: nlf Date: Thu, 6 Oct 2022 12:48:39 -0700 Subject: [PATCH 2/3] feat: explicitly validate config within the cli BREAKING CHANGE: the presence of auth related settings that are not scoped to a specific registry found in a config file is no longer supported and will throw errors --- lib/base-command.js | 8 +++ lib/commands/config.js | 2 + .../test/lib/utils/exit-handler.js.test.cjs | 56 +++++++++---------- test/fixtures/mock-npm.js | 1 + test/index.js | 2 +- test/lib/arborist-cmd.js | 6 +- test/lib/commands/adduser.js | 8 --- test/lib/commands/bugs.js | 2 +- test/lib/commands/explain.js | 3 + test/lib/commands/explore.js | 3 + test/lib/commands/help.js | 1 + test/lib/commands/install-ci-test.js | 3 + test/lib/commands/install-test.js | 3 + test/lib/commands/login.js | 8 --- test/lib/commands/publish.js | 6 +- test/lib/commands/set.js | 3 + test/lib/commands/stars.js | 2 +- test/lib/commands/token.js | 2 + test/lib/lifecycle-cmd.js | 3 + 19 files changed, 68 insertions(+), 54 deletions(-) diff --git a/lib/base-command.js b/lib/base-command.js index 3ab800adbfd98..b57b7474a5efb 100644 --- a/lib/base-command.js +++ b/lib/base-command.js @@ -11,6 +11,10 @@ class BaseCommand { constructor (npm) { this.wrapWidth = 80 this.npm = npm + + if (!this.skipConfigValidation) { + this.npm.config.validate() + } } get name () { @@ -25,6 +29,10 @@ class BaseCommand { return this.constructor.ignoreImplicitWorkspace } + get skipConfigValidation () { + return this.constructor.skipConfigValidation + } + get usage () { const usage = [ `${this.constructor.description}`, diff --git a/lib/commands/config.js b/lib/commands/config.js index 96dd4abcaf4af..8d74b8d2741e7 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -63,6 +63,8 @@ class Config extends BaseCommand { static ignoreImplicitWorkspace = false + static skipConfigValidation = true + async completion (opts) { const argv = opts.conf.argv.remain if (argv[1] !== 'config') { diff --git a/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs b/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs index 4d52c02d97dd3..1c0bb1c38c811 100644 --- a/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs +++ b/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs @@ -7,34 +7,34 @@ 'use strict' exports[`test/lib/utils/exit-handler.js TAP handles unknown error with logs and debug file > debug file contents 1`] = ` 0 timing npm:load:whichnode Completed in {TIME}ms -15 timing config:load Completed in {TIME}ms -16 timing npm:load:configload Completed in {TIME}ms -17 timing npm:load:mkdirpcache Completed in {TIME}ms -18 timing npm:load:mkdirplogs Completed in {TIME}ms -19 verbose title npm -20 verbose argv -21 timing npm:load:setTitle Completed in {TIME}ms -23 timing npm:load:display Completed in {TIME}ms -24 verbose logfile logs-max:10 dir:{CWD}/test/lib/utils/tap-testdir-exit-handler-handles-unknown-error-with-logs-and-debug-file/cache/_logs/{DATE}- -25 verbose logfile {CWD}/test/lib/utils/tap-testdir-exit-handler-handles-unknown-error-with-logs-and-debug-file/cache/_logs/{DATE}-debug-0.log -26 timing npm:load:logFile Completed in {TIME}ms -27 timing npm:load:timers Completed in {TIME}ms -28 timing npm:load:configScope Completed in {TIME}ms -29 timing npm:load Completed in {TIME}ms -30 silly logfile done cleaning log files -31 verbose stack Error: Unknown error -32 verbose cwd {CWD} -33 verbose Foo 1.0.0 -34 verbose node v1.0.0 -35 verbose npm v1.0.0 -36 error code ECODE -37 error ERR SUMMARY Unknown error -38 error ERR DETAIL Unknown error -39 verbose exit 1 -41 timing npm Completed in {TIME}ms -42 verbose code 1 -43 error A complete log of this run can be found in: -43 error {CWD}/test/lib/utils/tap-testdir-exit-handler-handles-unknown-error-with-logs-and-debug-file/cache/_logs/{DATE}-debug-0.log +13 timing config:load Completed in {TIME}ms +14 timing npm:load:configload Completed in {TIME}ms +15 timing npm:load:mkdirpcache Completed in {TIME}ms +16 timing npm:load:mkdirplogs Completed in {TIME}ms +17 verbose title npm +18 verbose argv +19 timing npm:load:setTitle Completed in {TIME}ms +21 timing npm:load:display Completed in {TIME}ms +22 verbose logfile logs-max:10 dir:{CWD}/test/lib/utils/tap-testdir-exit-handler-handles-unknown-error-with-logs-and-debug-file/cache/_logs/{DATE}- +23 verbose logfile {CWD}/test/lib/utils/tap-testdir-exit-handler-handles-unknown-error-with-logs-and-debug-file/cache/_logs/{DATE}-debug-0.log +24 timing npm:load:logFile Completed in {TIME}ms +25 timing npm:load:timers Completed in {TIME}ms +26 timing npm:load:configScope Completed in {TIME}ms +27 timing npm:load Completed in {TIME}ms +28 silly logfile done cleaning log files +29 verbose stack Error: Unknown error +30 verbose cwd {CWD} +31 verbose Foo 1.0.0 +32 verbose node v1.0.0 +33 verbose npm v1.0.0 +34 error code ECODE +35 error ERR SUMMARY Unknown error +36 error ERR DETAIL Unknown error +37 verbose exit 1 +39 timing npm Completed in {TIME}ms +40 verbose code 1 +41 error A complete log of this run can be found in: +41 error {CWD}/test/lib/utils/tap-testdir-exit-handler-handles-unknown-error-with-logs-and-debug-file/cache/_logs/{DATE}-debug-0.log ` exports[`test/lib/utils/exit-handler.js TAP handles unknown error with logs and debug file > logs 1`] = ` diff --git a/test/fixtures/mock-npm.js b/test/fixtures/mock-npm.js index 72cfa4b0c4beb..0318eadebafe6 100644 --- a/test/fixtures/mock-npm.js +++ b/test/fixtures/mock-npm.js @@ -216,6 +216,7 @@ class MockNpm { } }, list: [{ ...realConfig.defaults, ...config }], + validate: () => {}, } if (t && config.loglevel) { diff --git a/test/index.js b/test/index.js index 081c89cee9c70..5075597b5d93c 100644 --- a/test/index.js +++ b/test/index.js @@ -11,7 +11,7 @@ t.test('loading as main module will load the cli', t => { const cwd = t.testdir() const { spawn } = require('child_process') const LS = require('../lib/commands/ls.js') - const ls = new LS({}) + const ls = new LS({ config: { validate: () => {} } }) const p = spawn(process.execPath, [index, 'ls', '-h', '--cache', cwd]) const out = [] p.stdout.on('data', c => out.push(c)) diff --git a/test/lib/arborist-cmd.js b/test/lib/arborist-cmd.js index 91d8a7b333bf9..d2497efe129dc 100644 --- a/test/lib/arborist-cmd.js +++ b/test/lib/arborist-cmd.js @@ -44,8 +44,7 @@ t.test('arborist-cmd', async t => { class TestCmd extends ArboristCmd {} - const cmd = new TestCmd() - cmd.npm = { localPrefix: path } + const cmd = new TestCmd({ localPrefix: path, config: { validate: () => {} } }) // check filtering for a single workspace name cmd.exec = async function (args) { @@ -97,8 +96,7 @@ t.test('handle getWorkspaces raising an error', async t => { }, }) class TestCmd extends ArboristCmd {} - const cmd = new TestCmd() - cmd.npm = { localPrefix: t.testdir() } + const cmd = new TestCmd({ localPrefix: t.testdir(), config: { validate: () => {} } }) await t.rejects( cmd.execWorkspaces(['foo'], ['a']), diff --git a/test/lib/commands/adduser.js b/test/lib/commands/adduser.js index 04fe8dc8c3147..73e446309ef3b 100644 --- a/test/lib/commands/adduser.js +++ b/test/lib/commands/adduser.js @@ -27,15 +27,7 @@ t.test('legacy', async t => { const { npm, home } = await loadMockNpm(t, { config: { 'auth-type': 'legacy' }, homeDir: { - // These all get cleaned up by config.setCredentialsByURI '.npmrc': [ - '_token=user', - '_password=user', - 'username=user', - '_auth=user', - '_authtoken=user', - '-authtoken=user', - '_authToken=user', '//registry.npmjs.org/:_authToken=user', '//registry.npmjs.org/:always-auth=user', '//registry.npmjs.org/:email=test-email-old@npmjs.org', diff --git a/test/lib/commands/bugs.js b/test/lib/commands/bugs.js index dbd618b0848a4..91d144b6bdc97 100644 --- a/test/lib/commands/bugs.js +++ b/test/lib/commands/bugs.js @@ -62,7 +62,7 @@ const Bugs = t.mock('../../../lib/commands/bugs.js', { '../../../lib/utils/open-url.js': openUrl, }) -const bugs = new Bugs({ flatOptions: {} }) +const bugs = new Bugs({ flatOptions: {}, config: { validate: () => {} } }) t.test('usage', (t) => { t.match(bugs.usage, 'bugs', 'usage has command name in it') diff --git a/test/lib/commands/explain.js b/test/lib/commands/explain.js index 63deb8bc78a4c..c92732e904e60 100644 --- a/test/lib/commands/explain.js +++ b/test/lib/commands/explain.js @@ -6,6 +6,9 @@ const npm = { output: (...args) => { OUTPUT.push(args) }, + config: { + validate: () => {}, + }, } const { resolve } = require('path') diff --git a/test/lib/commands/explore.js b/test/lib/commands/explore.js index 5bb211e4503c3..af6f4df908677 100644 --- a/test/lib/commands/explore.js +++ b/test/lib/commands/explore.js @@ -67,6 +67,9 @@ const getExplore = (windows) => { output: out => { output.push(out) }, + config: { + validate: () => {}, + }, } return new Explore(npm) } diff --git a/test/lib/commands/help.js b/test/lib/commands/help.js index f84f94ad2f8d9..1e623dab9386e 100644 --- a/test/lib/commands/help.js +++ b/test/lib/commands/help.js @@ -19,6 +19,7 @@ const npm = { parsedArgv: { cooked: [], }, + validate: () => {}, }, exec: async (cmd, args) => { if (cmd === 'help-search') { diff --git a/test/lib/commands/install-ci-test.js b/test/lib/commands/install-ci-test.js index 0828d2b24ed97..68b86be43a30f 100644 --- a/test/lib/commands/install-ci-test.js +++ b/test/lib/commands/install-ci-test.js @@ -23,6 +23,9 @@ const installCITest = new InstallCITest({ testCalled = true } }, + config: { + validate: () => {}, + }, }) t.test('the install-ci-test command', t => { diff --git a/test/lib/commands/install-test.js b/test/lib/commands/install-test.js index 223bbe106aec7..0e0cf47521c4e 100644 --- a/test/lib/commands/install-test.js +++ b/test/lib/commands/install-test.js @@ -23,6 +23,9 @@ const installTest = new InstallTest({ testCalled = true } }, + config: { + validate: () => {}, + }, }) t.test('the install-test command', t => { diff --git a/test/lib/commands/login.js b/test/lib/commands/login.js index 8d27421313406..6c1d40c0d6edb 100644 --- a/test/lib/commands/login.js +++ b/test/lib/commands/login.js @@ -26,15 +26,7 @@ t.test('legacy', t => { const { npm, home } = await loadMockNpm(t, { config: { 'auth-type': 'legacy' }, homeDir: { - // These all get cleaned up by config.setCredentialsByURI '.npmrc': [ - '_token=user', - '_password=user', - 'username=user', - '_auth=user', - '_authtoken=user', - '-authtoken=user', - '_authToken=user', '//registry.npmjs.org/:_authToken=user', '//registry.npmjs.org/:always-auth=user', '//registry.npmjs.org/:email=test-email-old@npmjs.org', diff --git a/test/lib/commands/publish.js b/test/lib/commands/publish.js index 00fba9ef218e0..dfcc7204daca5 100644 --- a/test/lib/commands/publish.js +++ b/test/lib/commands/publish.js @@ -190,7 +190,7 @@ t.test('dry-run', async t => { t.test('shows usage with wrong set of arguments', async t => { t.plan(1) const Publish = t.mock('../../../lib/commands/publish.js') - const publish = new Publish({}) + const publish = new Publish({ config: { validate: () => {} } }) await t.rejects(publish.exec(['a', 'b', 'c']), publish.usage) }) @@ -637,7 +637,7 @@ t.test('ignore-scripts', async t => { t.test('_auth config default registry', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { config: { - _auth: basic, + '//registry.npmjs.org/:_auth': basic, }, prefixDir: { 'package.json': JSON.stringify(pkgJson), @@ -661,7 +661,7 @@ t.test('bare _auth and registry config', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { config: { registry: alternateRegistry, - _auth: basic, + '//other.registry.npmjs.org/:_auth': basic, }, prefixDir: { 'package.json': JSON.stringify({ diff --git a/test/lib/commands/set.js b/test/lib/commands/set.js index feeb901571768..9a68eaf32d457 100644 --- a/test/lib/commands/set.js +++ b/test/lib/commands/set.js @@ -40,6 +40,9 @@ const npm = { configArgs = args } }, + config: { + validate: () => {}, + }, } const Set = t.mock('../../../lib/commands/set.js') diff --git a/test/lib/commands/stars.js b/test/lib/commands/stars.js index 094b530d8c610..44de6ba1fb960 100644 --- a/test/lib/commands/stars.js +++ b/test/lib/commands/stars.js @@ -4,7 +4,7 @@ let result = '' const noop = () => null const npm = { - config: { get () {} }, + config: { get () {}, validate: () => {} }, flatOptions: {}, output: (...msg) => { result = [result, ...msg].join('\n') diff --git a/test/lib/commands/token.js b/test/lib/commands/token.js index c32c0b74a96f4..af53f49a130f5 100644 --- a/test/lib/commands/token.js +++ b/test/lib/commands/token.js @@ -7,6 +7,7 @@ const mocks = { } const npm = { output: (...args) => mocks.output(...args), + config: { validate: () => {} }, } const mockToken = (otherMocks) => t.mock('../../../lib/commands/token.js', { @@ -21,6 +22,7 @@ const tokenWithMocks = (options = {}) => { for (const mod in mockRequests) { if (mod === 'npm') { mockRequests.npm = { ...npm, ...mockRequests.npm } + mockRequests.npm.config.validate = () => {} } else { if (typeof mockRequests[mod] === 'function') { mocks[mod] = mockRequests[mod] diff --git a/test/lib/lifecycle-cmd.js b/test/lib/lifecycle-cmd.js index eb03f19270be0..22011197ead54 100644 --- a/test/lib/lifecycle-cmd.js +++ b/test/lib/lifecycle-cmd.js @@ -8,6 +8,9 @@ const npm = { return 'called the right thing' } }, + config: { + validate: () => {}, + }, } t.test('create a lifecycle command', async t => { t.plan(5) From 5c1315e35e44a405eee1685beb3b576e58633abb Mon Sep 17 00:00:00 2001 From: nlf Date: Thu, 6 Oct 2022 12:50:36 -0700 Subject: [PATCH 3/3] feat: introduce the `npm config fix` command --- lib/commands/config.js | 50 ++++++++++++- tap-snapshots/test/lib/docs.js.test.cjs | 2 + test/lib/commands/config.js | 99 ++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 2 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 8d74b8d2741e7..e6c0ba79d294f 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -51,6 +51,7 @@ class Config extends BaseCommand { 'delete [ ...]', 'list [--json]', 'edit', + 'fix', ] static params = [ @@ -72,7 +73,7 @@ class Config extends BaseCommand { } if (argv.length === 2) { - const cmds = ['get', 'set', 'delete', 'ls', 'rm', 'edit'] + const cmds = ['get', 'set', 'delete', 'ls', 'rm', 'edit', 'fix'] if (opts.partialWord !== 'l') { cmds.push('list') } @@ -97,6 +98,7 @@ class Config extends BaseCommand { case 'edit': case 'list': case 'ls': + case 'fix': default: return [] } @@ -129,6 +131,9 @@ class Config extends BaseCommand { case 'edit': await this.edit() break + case 'fix': + await this.fix() + break default: throw this.usageError() } @@ -240,6 +245,49 @@ ${defData} }) } + async fix () { + let problems + + try { + this.npm.config.validate() + return // if validate doesn't throw we have nothing to do + } catch (err) { + // coverage skipped because we don't need to test rethrowing errors + // istanbul ignore next + if (err.code !== 'ERR_INVALID_AUTH') { + throw err + } + + problems = err.problems + } + + if (!this.npm.config.isDefault('location')) { + problems = problems.filter((problem) => { + return problem.where === this.npm.config.get('location') + }) + } + + this.npm.config.repair(problems) + const locations = [] + + this.npm.output('The following configuration problems have been repaired:\n') + const summary = problems.map(({ action, from, to, key, where }) => { + // coverage disabled for else branch because it is intentionally omitted + // istanbul ignore else + if (action === 'rename') { + // we keep track of which configs were modified here so we know what to save later + locations.push(where) + return `~ \`${from}\` renamed to \`${to}\` in ${where} config` + } else if (action === 'delete') { + locations.push(where) + return `- \`${key}\` deleted from ${where} config` + } + }).join('\n') + this.npm.output(summary) + + return await Promise.all(locations.map((location) => this.npm.config.save(location))) + } + async list () { const msg = [] // long does not have a flattener diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index 4ce9a30ef2817..62040363b50b4 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -2668,6 +2668,7 @@ npm config get [ [ ...]] npm config delete [ ...] npm config list [--json] npm config edit +npm config fix Options: [--json] [-g|--global] [--editor ] [-L|--location ] @@ -2683,6 +2684,7 @@ npm config get [ [ ...]] npm config delete [ ...] npm config list [--json] npm config edit +npm config fix alias: c \`\`\` diff --git a/test/lib/commands/config.js b/test/lib/commands/config.js index 42df8b52d8e57..0d71d32a823c2 100644 --- a/test/lib/commands/config.js +++ b/test/lib/commands/config.js @@ -411,6 +411,102 @@ t.test('config edit - editor exits non-0', async t => { ) }) +t.test('config fix', (t) => { + t.test('no problems', async (t) => { + const home = t.testdir({ + '.npmrc': '', + }) + + const sandbox = new Sandbox(t, { home }) + await sandbox.run('config', ['fix']) + t.equal(sandbox.output, '', 'printed nothing') + }) + + t.test('repairs all configs by default', async (t) => { + const root = t.testdir({ + global: { + npmrc: '_authtoken=notatoken\n_authToken=afaketoken', + }, + home: { + '.npmrc': '_authtoken=thisisinvalid\n_auth=beef', + }, + }) + const registry = `//registry.npmjs.org/` + + const sandbox = new Sandbox(t, { + global: join(root, 'global'), + home: join(root, 'home'), + }) + await sandbox.run('config', ['fix']) + + // global config fixes + t.match(sandbox.output, '`_authtoken` deleted from global config', + 'output has deleted global _authtoken') + t.match(sandbox.output, `\`_authToken\` renamed to \`${registry}:_authToken\` in global config`, + 'output has renamed global _authToken') + t.not(sandbox.config.get('_authtoken', 'global'), '_authtoken is not set globally') + t.not(sandbox.config.get('_authToken', 'global'), '_authToken is not set globally') + t.equal(sandbox.config.get(`${registry}:_authToken`, 'global'), 'afaketoken', + 'global _authToken was scoped') + const globalConfig = await readFile(join(root, 'global', 'npmrc'), { encoding: 'utf8' }) + t.equal(globalConfig, `${registry}:_authToken=afaketoken\n`, 'global config was written') + + // user config fixes + t.match(sandbox.output, '`_authtoken` deleted from user config', + 'output has deleted user _authtoken') + t.match(sandbox.output, `\`_auth\` renamed to \`${registry}:_auth\` in user config`, + 'output has renamed user _auth') + t.not(sandbox.config.get('_authtoken', 'user'), '_authtoken is not set in user config') + t.not(sandbox.config.get('_auth'), '_auth is not set in user config') + t.equal(sandbox.config.get(`${registry}:_auth`, 'user'), 'beef', 'user _auth was scoped') + const userConfig = await readFile(join(root, 'home', '.npmrc'), { encoding: 'utf8' }) + t.equal(userConfig, `${registry}:_auth=beef\n`, 'user config was written') + }) + + t.test('repairs only the config specified by --location if asked', async (t) => { + const root = t.testdir({ + global: { + npmrc: '_authtoken=notatoken\n_authToken=afaketoken', + }, + home: { + '.npmrc': '_authtoken=thisisinvalid\n_auth=beef', + }, + }) + const registry = `//registry.npmjs.org/` + + const sandbox = new Sandbox(t, { + global: join(root, 'global'), + home: join(root, 'home'), + }) + await sandbox.run('config', ['fix', '--location=user']) + + // global config should be untouched + t.notMatch(sandbox.output, '`_authtoken` deleted from global', + 'output has deleted global _authtoken') + t.notMatch(sandbox.output, `\`_authToken\` renamed to \`${registry}:_authToken\` in global`, + 'output has renamed global _authToken') + t.equal(sandbox.config.get('_authtoken', 'global'), 'notatoken', 'global _authtoken untouched') + t.equal(sandbox.config.get('_authToken', 'global'), 'afaketoken', 'global _authToken untouched') + t.not(sandbox.config.get(`${registry}:_authToken`, 'global'), 'global _authToken not scoped') + const globalConfig = await readFile(join(root, 'global', 'npmrc'), { encoding: 'utf8' }) + t.equal(globalConfig, '_authtoken=notatoken\n_authToken=afaketoken', + 'global config was not written') + + // user config fixes + t.match(sandbox.output, '`_authtoken` deleted from user', + 'output has deleted user _authtoken') + t.match(sandbox.output, `\`_auth\` renamed to \`${registry}:_auth\` in user`, + 'output has renamed user _auth') + t.not(sandbox.config.get('_authtoken', 'user'), '_authtoken is not set in user config') + t.not(sandbox.config.get('_auth', 'user'), '_auth is not set in user config') + t.equal(sandbox.config.get(`${registry}:_auth`, 'user'), 'beef', 'user _auth was scoped') + const userConfig = await readFile(join(root, 'home', '.npmrc'), { encoding: 'utf8' }) + t.equal(userConfig, `${registry}:_auth=beef\n`, 'user config was written') + }) + + t.end() +}) + t.test('completion', async t => { const sandbox = new Sandbox(t) @@ -423,13 +519,14 @@ t.test('completion', async t => { sandbox.reset() } - await testComp([], ['get', 'set', 'delete', 'ls', 'rm', 'edit', 'list']) + await testComp([], ['get', 'set', 'delete', 'ls', 'rm', 'edit', 'fix', 'list']) await testComp(['set', 'foo'], []) await testComp(['get'], allKeys) await testComp(['set'], allKeys) await testComp(['delete'], allKeys) await testComp(['rm'], allKeys) await testComp(['edit'], []) + await testComp(['fix'], []) await testComp(['list'], []) await testComp(['ls'], [])