feat: throw errors for invalid auth configuration

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

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,
+}

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,15 +387,58 @@ 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') {
           continue
         }
         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=<base64> 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
   }