From 6726319ee0c0be442b9cf060751d00c15045ec73 Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Tue, 10 Nov 2020 15:40:30 -0800
Subject: [PATCH] Set process.title a bit more usefully

This includes all positional args (ie, not anything that is a config),
except for the third positional arg when running `npm token revoke ...`,
since that is also a secret.

Makes the `ps` entry a bit more useful than just "npm", so users can at
least get some clue what npm is doing, while minimizing the degree to
which we leak anything secret.

Closes: #1927
Fixes: https://npm.community/t/8400
Related: tmux-plugins/tmux-resurrect#201

Credit: @isaacs
Close: #2154
Reviewed-by: @ruyadorno
---
 lib/cli.js      |  2 ++
 lib/npm.js      | 20 +++++++++++
 test/lib/npm.js | 93 ++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/lib/cli.js b/lib/cli.js
index f06abcd186f96..910b674eaa790 100644
--- a/lib/cli.js
+++ b/lib/cli.js
@@ -1,5 +1,7 @@
 // Separated out for easier unit testing
 module.exports = (process) => {
+  // set it here so that regardless of what happens later, we don't
+  // leak any private CLI configs to other programs
   process.title = 'npm'
 
   const {
diff --git a/lib/npm.js b/lib/npm.js
index d6eab9674fe18..4430b80539d03 100644
--- a/lib/npm.js
+++ b/lib/npm.js
@@ -54,6 +54,7 @@ const _runCmd = Symbol('_runCmd')
 const _load = Symbol('_load')
 const _flatOptions = Symbol('_flatOptions')
 const _tmpFolder = Symbol('_tmpFolder')
+const _title = Symbol('_title')
 const npm = module.exports = new class extends EventEmitter {
   constructor () {
     super()
@@ -75,6 +76,7 @@ const npm = module.exports = new class extends EventEmitter {
       defaults,
       shorthands,
     })
+    this[_title] = process.title
     this.updateNotification = null
   }
 
@@ -156,6 +158,15 @@ const npm = module.exports = new class extends EventEmitter {
     return this.config.loaded
   }
 
+  get title () {
+    return this[_title]
+  }
+
+  set title (t) {
+    process.title = t
+    this[_title] = t
+  }
+
   async [_load] () {
     const node = await which(process.argv[0]).catch(er => null)
     if (node && node.toUpperCase() !== process.execPath.toUpperCase()) {
@@ -166,6 +177,15 @@ const npm = module.exports = new class extends EventEmitter {
 
     await this.config.load()
     this.argv = this.config.parsedArgv.remain
+    // note: this MUST be shorter than the actual argv length, because it
+    // uses the same memory, so node will truncate it if it's too long.
+    // if it's a token revocation, then the argv contains a secret, so
+    // don't show that.  (Regrettable historical choice to put it there.)
+    // Any other secrets are configs only, so showing only the positional
+    // args keeps those from being leaked.
+    const tokrev = deref(this.argv[0]) === 'token' && this.argv[1] === 'revoke'
+    this.title = tokrev ? 'npm token revoke' + (this.argv[2] ? ' ***' : '')
+      : ['npm', ...this.argv].join(' ')
 
     this.color = setupLog(this.config, this)
     process.env.COLOR = this.color ? '1' : '0'
diff --git a/test/lib/npm.js b/test/lib/npm.js
index b2be5543fa1ea..f6a13b90fa5e3 100644
--- a/test/lib/npm.js
+++ b/test/lib/npm.js
@@ -256,7 +256,10 @@ t.test('npm.load', t => {
       '--prefix', dir,
       '--userconfig', `${dir}/.npmrc`,
       '--usage',
-      '--scope=foo'
+      '--scope=foo',
+      'token',
+      'revoke',
+      'blergggg',
     ]
 
     freshConfig()
@@ -353,3 +356,91 @@ t.test('loading as main will load the cli', t => {
     t.end()
   })
 })
+
+t.test('set process.title', t => {
+  const { execPath, argv: processArgv } = process
+  const { log } = console
+  const titleDesc = Object.getOwnPropertyDescriptor(process, 'title')
+  Object.defineProperty(process, 'title', {
+    value: '',
+    settable: true,
+    enumerable: true,
+    configurable: true,
+  })
+  const consoleLogs = []
+  console.log = (...msg) => consoleLogs.push(msg)
+
+  t.teardown(() => {
+    console.log = log
+    process.argv = processArgv
+    Object.defineProperty(process, 'title', titleDesc)
+    freshConfig()
+  })
+
+  t.afterEach(cb => {
+    consoleLogs.length = 0
+    cb()
+  })
+
+  t.test('basic title setting', async t => {
+    freshConfig({
+      argv: [
+        process.execPath,
+        process.argv[1],
+        '--metrics-registry', 'http://example.com',
+        '--usage',
+        '--scope=foo',
+        'ls',
+      ],
+    })
+    await npm.load(er => {
+      if (er)
+        throw er
+      t.equal(npm.title, 'npm ls')
+      t.equal(process.title, 'npm ls')
+    })
+  })
+
+  t.test('do not expose token being revoked', async t => {
+    freshConfig({
+      argv: [
+        process.execPath,
+        process.argv[1],
+        '--metrics-registry', 'http://example.com',
+        '--usage',
+        '--scope=foo',
+        'token',
+        'revoke',
+        'deadbeefcafebad',
+      ],
+    })
+    await npm.load(er => {
+      if (er)
+        throw er
+      t.equal(npm.title, 'npm token revoke ***')
+      t.equal(process.title, 'npm token revoke ***')
+    })
+  })
+
+  t.test('do show *** unless a token is actually being revoked', async t => {
+    freshConfig({
+      argv: [
+        process.execPath,
+        process.argv[1],
+        '--metrics-registry', 'http://example.com',
+        '--usage',
+        '--scope=foo',
+        'token',
+        'revoke',
+      ],
+    })
+    await npm.load(er => {
+      if (er)
+        throw er
+      t.equal(npm.title, 'npm token revoke')
+      t.equal(process.title, 'npm token revoke')
+    })
+  })
+
+  t.end()
+})