Redact protected config values in argv logs

lib/commands/config.js

@@ -8,21 +8,7 @@ const { defaults, definitions, nerfDarts, proxyEnv } = require('@npmcli/config/l
 const { log, output, input } = require('proc-log')
 const BaseCommand = require('../base-cmd.js')
 const { redact } = require('@npmcli/redact')
-
-// These are the config values to swap with "protected".
-// It does not catch every single sensitive thing a user may put in the npmrc file but it gets the common ones.
-// This is distinct from nerfDarts because that is used to validate valid configs during "npm config set", and folks may have old invalid entries lying around in a config file that we still want to protect when running "npm config list"
-// This is a more general list of values to consider protected.
-// You cannot "npm config get" them, and they will not display during "npm config list"
-const protected = [
-  'auth',
-  'authToken',
-  'certfile',
-  'email',
-  'keyfile',
-  'password',
-  'username',
-]
+const { isProtected } = require('../utils/protected-config.js')
 
 // take an array of `[key, value, k2=v2, k3, v3, ...]` and turn into { key: value, k2: v2, k3: v3 }
 const keyValues = args => {
@@ -38,29 +24,6 @@ const keyValues = args => {
   return kv
 }
 
-const isProtected = (k) => {
-  // _password
-  if (k.startsWith('_')) {
-    return true
-  }
-  if (protected.includes(k)) {
-    return true
-  }
-  // //localhost:8080/:_password
-  if (k.startsWith('//')) {
-    if (k.includes(':_')) {
-      return true
-    }
-    // //registry:_authToken or //registry:authToken
-    for (const p of protected) {
-      if (k.endsWith(`:${p}`) || k.endsWith(`:_${p}`)) {
-        return true
-      }
-    }
-  }
-  return false
-}
-
 // Private fields are either protected or they can redacted info
 const isPrivate = (k, v) => isProtected(k) || redact(v) !== v
 

lib/npm.js

@@ -10,6 +10,7 @@ const { log, time, output, META } = require('proc-log')
 const { redactLog: replaceInfo } = require('@npmcli/redact')
 const pkg = require('../package.json')
 const { deref } = require('./utils/cmd-list.js')
+const { cleanArgv } = require('./utils/clean-argv.js')
 const { jsonError, outputError } = require('./utils/output-error.js')
 
 class Npm {
@@ -142,8 +143,7 @@ class Npm {
       process.title = this.#title
       // The cooked argv is also logged separately for debugging purposes.
       // It is cleaned as a best effort by replacing known secrets like basic auth password and strings that look like npm tokens.
-      // XXX: for this to be safer the config should create a sanitized version of the argv as it has the full context of what each option contains.
-      this.#argvClean = replaceInfo(cooked)
+      this.#argvClean = cleanArgv(cooked)
       log.verbose('title', this.title)
       log.verbose('argv', this.#argvClean.map(JSON.stringify).join(' '))
     })

lib/utils/clean-argv.js

@@ -0,0 +1,35 @@
+const { redactLog: replaceInfo } = require('@npmcli/redact')
+const { isProtected } = require('./protected-config.js')
+
+const REDACTED = '***'
+const isProtectedArg = key => isProtected(key) || key === 'otp'
+
+const cleanArgv = (argv) => {
+  const cleaned = replaceInfo(argv)
+  for (let i = 0; i < cleaned.length; i++) {
+    const arg = cleaned[i]
+    if (typeof arg !== 'string' || !arg.startsWith('--')) {
+      continue
+    }
+
+    const raw = arg.slice(2)
+    const equalsIndex = raw.indexOf('=')
+    const key = equalsIndex === -1 ? raw : raw.slice(0, equalsIndex)
+    if (!isProtectedArg(key)) {
+      continue
+    }
+
+    if (equalsIndex === -1) {
+      if (i + 1 < cleaned.length) {
+        cleaned[i + 1] = REDACTED
+      }
+    } else {
+      cleaned[i] = `${arg.slice(0, equalsIndex + 3)}${REDACTED}`
+    }
+  }
+  return cleaned
+}
+
+module.exports = {
+  cleanArgv,
+}

lib/utils/protected-config.js

@@ -0,0 +1,39 @@
+// These are config values whose contents should not be displayed.
+// This is intentionally broader than nerfDarts because users may have old or
+// invalid auth entries that still need to be protected when npm reports config.
+const protected = [
+  'auth',
+  'authToken',
+  'certfile',
+  'email',
+  'keyfile',
+  'password',
+  'username',
+]
+
+const isProtected = (key) => {
+  // _password
+  if (key.startsWith('_')) {
+    return true
+  }
+  if (protected.includes(key)) {
+    return true
+  }
+  // //localhost:8080/:_password
+  if (key.startsWith('//')) {
+    if (key.includes(':_')) {
+      return true
+    }
+    // //registry:authToken or //registry:_authToken
+    for (const protectedKey of protected) {
+      if (key.endsWith(`:${protectedKey}`) || key.endsWith(`:_${protectedKey}`)) {
+        return true
+      }
+    }
+  }
+  return false
+}
+
+module.exports = {
+  isProtected,
+}

tap-snapshots/test/lib/utils/error-message.js.test.cjs

@@ -195,6 +195,8 @@ Object {
       "g",
       "s",
       "https://evil:***@npmjs.org",
+      "--//registry.npmjs.org/:_authToken",
+      "plain-secret",
     ],
     Array [
       "",

test/lib/cli/entry.js

@@ -105,6 +105,31 @@ t.test('logged argv is sanitized with equals', async t => {
   t.match(logs.verbose.byTitle('argv'), ['argv "version" "--registry" "https://u:***@npmjs.org"'])
 })
 
+t.test('logged argv redacts protected config values', async t => {
+  const { logs, cli } = await cliMock(t, {
+    globals: {
+      'process.argv': [
+        'node',
+        'npm',
+        'version',
+        '--//registry.npmjs.org/:_authToken=plain-secret',
+        '--_password',
+        'hunter2',
+        '--otp=123456',
+      ],
+    },
+  })
+  await cli(process)
+
+  const argv = logs.verbose.byTitle('argv')[0]
+  t.notMatch(argv, 'plain-secret')
+  t.notMatch(argv, 'hunter2')
+  t.notMatch(argv, '123456')
+  t.match(argv, '"--//registry.npmjs.org/:_authToken" "***"')
+  t.match(argv, '"--_password" "***"')
+  t.match(argv, '"--otp" "***"')
+})
+
 t.test('print usage if no params provided', async t => {
   const { cli, outputs, exitHandlerCalled, exitHandlerNpm } = await cliMock(t, {
     globals: {

test/lib/utils/clean-argv.js

@@ -0,0 +1,53 @@
+const t = require('tap')
+const { cleanArgv } = require('../../../lib/utils/clean-argv.js')
+
+t.test('redacts protected config values', t => {
+  t.strictSame(cleanArgv([
+    '--password=secret',
+    '--otp',
+    '123456',
+    '--//registry.npmjs.org/:_authToken',
+    'plain-secret',
+    '--registry',
+    'https://user:pass@registry.npmjs.org',
+  ]), [
+    '--password=***',
+    '--otp',
+    '***',
+    '--//registry.npmjs.org/:_authToken',
+    '***',
+    '--registry',
+    'https://user:***@registry.npmjs.org',
+  ])
+  t.end()
+})
+
+t.test('redacts protected config values with equals', t => {
+  t.strictSame(cleanArgv([
+    '--otp=123456',
+    '--_authToken=plain-secret',
+    '--//registry.npmjs.org/:_password=hunter2',
+  ]), [
+    '--otp=***',
+    '--_authToken=***',
+    '--//registry.npmjs.org/:_password=***',
+  ])
+  t.end()
+})
+
+t.test('leaves non-protected config values intact', t => {
+  t.strictSame(cleanArgv([
+    '--registry',
+    'https://registry.npmjs.org',
+    '--auth-type',
+    'legacy',
+    '--scope=@npmcli',
+  ]), [
+    '--registry',
+    'https://registry.npmjs.org',
+    '--auth-type',
+    'legacy',
+    '--scope=@npmcli',
+  ])
+  t.end()
+})

test/lib/utils/error-message.js

@@ -167,12 +167,42 @@ t.test('args are cleaned', async t => {
   t.matchSnapshot(errorMessage(Object.assign(new Error('cmd err'), {
     cmd: 'some command',
     signal: 'SIGYOLO',
-    args: ['a', 'r', 'g', 's', 'https://evil:password@npmjs.org'],
+    args: [
+      'a',
+      'r',
+      'g',
+      's',
+      'https://evil:password@npmjs.org',
+      '--//registry.npmjs.org/:_authToken',
+      'plain-secret',
+    ],
     stdout: 'stdout',
     stderr: 'stderr',
   })))
 })
 
+t.test('child process args keep protected-looking flags', async t => {
+  const { errorMessage } = await loadMockNpm(t)
+  const msg = errorMessage(Object.assign(new Error('cmd err'), {
+    cmd: 'some command',
+    args: [
+      '--password',
+      'hunter2',
+      '--registry',
+      'https://evil:password@npmjs.org',
+    ],
+  }))
+
+  t.strictSame(msg.detail, [[
+    'command',
+    'some command',
+    '--password',
+    'hunter2',
+    '--registry',
+    'https://evil:***@npmjs.org',
+  ]])
+})
+
 t.test('eacces/eperm', async t => {
   const runTest = (windows, loaded, cachePath, cacheDest) => async t => {
     const { errorMessage, logs, cache } = await loadMockNpm(t, {