From e24bb80d1396c2d35907204d82ee25ad74951432 Mon Sep 17 00:00:00 2001 From: nlf Date: Wed, 5 Oct 2022 14:42:38 -0700 Subject: [PATCH] feat: throw errors for invalid auth configuration BREAKING CHANGE: unscoped auth configuration is no longer automatically scoped to a registry. the `validate` method is no longer called automatically. the `_auth` configuration key is no longer split into `username` and `_password`. errors will be thrown by `validate()` if problems are found. --- README.md | 13 ++++ lib/errors.js | 22 ++++++ lib/index.js | 188 ++++++++++++++++++++++++++------------------------ test/index.js | 76 +++++++++++++------- 4 files changed, 182 insertions(+), 117 deletions(-) create mode 100644 lib/errors.js diff --git a/README.md b/README.md index 95cbb39..3241838 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,7 @@ process.on('log', (level, ...args) => { // returns a promise that fails if config loading fails, and // resolves when the config object is ready for action conf.load().then(() => { + conf.validate() console.log('loaded ok! some-key = ' + conf.get('some-key')) }).catch(er => { console.error('error loading configs!', er) @@ -210,6 +211,10 @@ Delete the configuration key from the specified level in the config stack. Verify that all known configuration options are set to valid values, and log a warning if they are invalid. +Invalid auth options will cause this method to throw an error with a `code` +property of `ERR_INVALID_AUTH`, and a `problems` property listing the specific +concerns with the current configuration. + If `where` is not set, then all config objects are validated. Returns `true` if all configs are valid. @@ -218,6 +223,14 @@ Note that it's usually enough (and more efficient) to just check `config.valid`, since each data object is marked for re-evaluation on every `config.set()` operation. +### `config.repair(problems)` + +Accept an optional array of problems (as thrown by `config.validate()`) and +perform the necessary steps to resolve them. If no problems are provided, +this method will call `config.validate()` internally to retrieve them. + +Note that you must `await config.save('user')` in order to persist the changes. + ### `config.isDefault(key)` Returns `true` if the value is coming directly from the diff --git a/lib/errors.js b/lib/errors.js new file mode 100644 index 0000000..4af1a9c --- /dev/null +++ b/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` + } else if (problem.action === 'rename') { + return `\`${problem.from}\` must be renamed to \`${problem.to}\`` + } + }).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/lib/index.js b/lib/index.js index fe5cfd2..8ee72a9 100644 --- a/lib/index.js +++ b/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,7 @@ class Config { validate (where) { if (!where) { let valid = true - for (const [entryWhere] of this.data.entries()) { + 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') { @@ -419,10 +407,77 @@ class Config { nopt.clean(obj.data, this.types, this.typeDefs) nopt.invalidHandler = null + + if (where === 'user') { + // 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 + const authProblems = [] + + // first, keys that should be removed + for (const key of ['_authtoken', '-authtoken']) { + if (this.get(key, 'user')) { + authProblems.push({ action: 'delete', key }) + } + } + + 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, 'user')) { + if (key === 'username' && !this.get('_password', 'user')) { + authProblems.push({ action: 'delete', key }) + } else if (key === '_password' && !this.get('username', 'user')) { + authProblems.push({ action: 'delete', key }) + } else { + authProblems.push({ action: 'rename', from: key, to: `${nerfedReg}:${key}` }) + } + } + } + + if (authProblems.length) { + throw new ErrInvalidAuth(authProblems) + } + } + return obj[_valid] } } + // 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, 'user') + } else if (problem.action === 'rename') { + const old = this.get(problem.from, 'user') + this.set(problem.to, old, 'user') + this.delete(problem.from, 'user') + } + } + } + // 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 +699,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 +739,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 +762,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 +802,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 +824,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 +850,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/test/index.js b/test/index.js index e9149e8..ba9569c 100644 --- a/test/index.js +++ b/test/index.js @@ -341,6 +341,8 @@ loglevel = yolo npm_config_builtin_config: 'true', }, 'set env values') + // warn logs are emitted as a side effect of validate + config.validate() t.strictSame(logs, [ ['warn', 'invalid config', 'registry="hello"', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'full url with "http://"'], @@ -467,6 +469,7 @@ loglevel = yolo t.rejects(() => config.save('yolo'), { message: 'invalid config location param: yolo', }) + config.validate() t.equal(config.valid, false, 'config should not be valid') logs.length = 0 }) @@ -552,7 +555,7 @@ t.test('cafile loads as ca (and some saving tests)', async t => { const cafile = resolve(__dirname, 'fixtures', 'cafile') const dir = t.testdir({ '.npmrc': `cafile = ${cafile} -_authToken = deadbeefcafebadfoobarbaz42069 +//registry.npmjs.org/:_authToken = deadbeefcafebadfoobarbaz42069 `, }) const expect = `cafile=${cafile} @@ -733,6 +736,29 @@ always-auth = true`, }) } + // the def_ and none_ prefixed cases have unscoped auth values and should throw + if (testCase.startsWith('def_') || + testCase === 'none_authToken' || + testCase === 'none_lcAuthToken') { + try { + c.validate() + // validate should throw, fail the test here if it doesn't + t.fail('validate should have thrown') + } catch (err) { + if (err.code !== 'ERR_INVALID_AUTH') { + throw err + } + + // we got our expected invalid auth error, so now repair it + c.repair(err.problems) + t.ok(c.valid, 'config is valid') + } + } else { + // validate won't throw for these ones, so let's prove it and repair are no-ops + c.validate() + c.repair() + } + const d = c.getCredentialsByURI(defReg) const o = c.getCredentialsByURI(otherReg) @@ -914,9 +940,12 @@ t.test('setting basic auth creds and email', async t => { t.strictSame(d.getCredentialsByURI(registry), { email: 'name@example.com' }) d.set('_auth', _auth, 'user') t.equal(d.get('_auth', 'user'), _auth, '_auth was set') + d.repair() await d.save('user') - t.equal(d.get('_auth', 'user'), undefined, 'un-nerfed _auth deleted') - t.strictSame(d.getCredentialsByURI(registry), { + const e = new Config(opts) + await e.load() + t.equal(e.get('_auth', 'user'), undefined, 'un-nerfed _auth deleted') + t.strictSame(e.getCredentialsByURI(registry), { email: 'name@example.com', username: 'admin', password: 'admin', @@ -944,17 +973,9 @@ t.test('setting username/password/email individually', async t => { c.set('_password', Buffer.from('admin').toString('base64'), 'user') t.equal(c.get('_password'), Buffer.from('admin').toString('base64')) t.equal(c.get('_auth'), undefined) + c.repair() await c.save('user') - t.equal(c.get('email'), 'name@example.com') - t.equal(c.get('username'), undefined) - t.equal(c.get('_password'), undefined) - t.equal(c.get('_auth'), undefined) - t.strictSame(c.getCredentialsByURI(registry), { - email: 'name@example.com', - username: 'admin', - password: 'admin', - auth: Buffer.from('admin:admin').toString('base64'), - }) + const d = new Config(opts) await d.load() t.equal(d.get('email'), 'name@example.com') @@ -977,15 +998,13 @@ t.test('nerfdart auths set at the top level into the registry', async t => { const email = 'i@izs.me' const _authToken = 'deadbeefblahblah' - // name: [ini, expect] + // name: [ini, expect, wontThrow] const cases = { '_auth only, no email': [`_auth=${_auth}`, { - '//registry.npmjs.org/:username': username, - '//registry.npmjs.org/:_password': _password, + '//registry.npmjs.org/:_auth': _auth, }], '_auth with email': [`_auth=${_auth}\nemail=${email}`, { - '//registry.npmjs.org/:username': username, - '//registry.npmjs.org/:_password': _password, + '//registry.npmjs.org/:_auth': _auth, email, }], '_authToken alone': [`_authToken=${_authToken}`, { @@ -1005,8 +1024,10 @@ t.test('nerfdart auths set at the top level into the registry', async t => { email, }], // handled invalid/legacy cases - 'username, no _password': [`username=${username}`, {}, true], - '_password, no username': [`_password=${_password}`, {}, true], + 'username, no _password': [`username=${username}`, {}], + '_password, no username': [`_password=${_password}`, {}], + '_authtoken instead of _authToken': [`_authtoken=${_authToken}`, {}], + '-authtoken instead of _authToken': [`-authtoken=${_authToken}`, {}], // de-nerfdart the email, if present in that way 'nerf-darted email': [`//registry.npmjs.org/:email=${email}`, { email, @@ -1020,7 +1041,7 @@ t.test('nerfdart auths set at the top level into the registry', async t => { process.removeListener('log', logHandler) }) const cwd = process.cwd() - for (const [name, [ini, expect, noWarn]] of Object.entries(cases)) { + for (const [name, [ini, expect, wontThrow]] of Object.entries(cases)) { t.test(name, async t => { t.teardown(() => { process.chdir(cwd) @@ -1047,13 +1068,18 @@ t.test('nerfdart auths set at the top level into the registry', async t => { }, npmPath: process.cwd(), } + const c = new Config(opts) await c.load() - t.same(c.list[3], expect) - if (!noWarn) { - t.equal(logs.length, 1, 'logged 1 message') - t.match(logs[0], /must be scoped to a registry/, 'logged auth warning') + + if (!wontThrow) { + t.throws(() => c.validate(), { code: 'ERR_INVALID_AUTH' }) } + + // now we go ahead and do the repair, and save + c.repair() + await c.save('user') + t.same(c.list[3], expect) }) } })