fix: filter C0 and C1 control characters from logs and cli output (#7113)

Author: wraithgar

lib/commands/view.js

@@ -392,20 +392,20 @@ class View extends BaseCommand {
 
     if (info.keywords.length) {
       this.npm.output('')
-      this.npm.output('keywords:', chalk.yellow(info.keywords.join(', ')))
+      this.npm.output(`keywords: ${chalk.yellow(info.keywords.join(', '))}`)
     }
 
     if (info.bins.length) {
       this.npm.output('')
-      this.npm.output('bin:', chalk.yellow(info.bins.join(', ')))
+      this.npm.output(`bin: ${chalk.yellow(info.bins.join(', '))}`)
     }
 
     this.npm.output('')
     this.npm.output('dist')
-    this.npm.output('.tarball:', info.tarball)
-    this.npm.output('.shasum:', info.shasum)
-    info.integrity && this.npm.output('.integrity:', info.integrity)
-    info.unpackedSize && this.npm.output('.unpackedSize:', info.unpackedSize)
+    this.npm.output(`.tarball: ${info.tarball}`)
+    this.npm.output(`.shasum: ${info.shasum}`)
+    info.integrity && this.npm.output(`.integrity: ${info.integrity}`)
+    info.unpackedSize && this.npm.output(`.unpackedSize: ${info.unpackedSize}`)
 
     const maxDeps = 24
     if (info.deps.length) {
@@ -420,7 +420,7 @@ class View extends BaseCommand {
     if (info.maintainers && info.maintainers.length) {
       this.npm.output('')
       this.npm.output('maintainers:')
-      info.maintainers.forEach((u) => this.npm.output('-', u))
+      info.maintainers.forEach((u) => this.npm.output(`- ${u}`))
     }
 
     this.npm.output('')

lib/npm.js

@@ -440,7 +440,7 @@ class Npm {
   output (...msg) {
     log.clearProgress()
     // eslint-disable-next-line no-console
-    console.log(...msg)
+    console.log(...msg.map(Display.clean))
     log.showProgress()
   }
 
@@ -478,7 +478,7 @@ class Npm {
   outputError (...msg) {
     log.clearProgress()
     // eslint-disable-next-line no-console
-    console.error(...msg)
+    console.error(...msg.map(Display.clean))
     log.showProgress()
   }
 }

lib/utils/display.js

@@ -3,6 +3,44 @@ const npmlog = require('npmlog')
 const log = require('./log-shim.js')
 const { explain } = require('./explain-eresolve.js')
 
+const originalCustomInspect = Symbol('npm.display.original.util.inspect.custom')
+
+// These are most assuredly not a mistake
+// https://eslint.org/docs/latest/rules/no-control-regex
+/* eslint-disable no-control-regex */
+// \x00 through \x1f, \x7f through \x9f, not including \x09 \x0a \x0b \x0d
+const hasC01 = /[\x00-\x08\x0c\x0e-\x1f\x7f-\x9f]/
+// Allows everything up to '[38;5;255m' in 8 bit notation
+const allowedSGR = /^\[[0-9;]{0,8}m/
+// '[38;5;255m'.length
+const sgrMaxLen = 10
+
+// Strips all ANSI C0 and C1 control characters (except for SGR up to 8 bit)
+function stripC01 (str) {
+  if (!hasC01.test(str)) {
+    return str
+  }
+  let result = ''
+  for (let i = 0; i < str.length; i++) {
+    const char = str[i]
+    const code = char.charCodeAt(0)
+    if (!hasC01.test(char)) {
+      // Most characters are in this set so continue early if we can
+      result = `${result}${char}`
+    } else if (code === 27 && allowedSGR.test(str.slice(i + 1, i + sgrMaxLen + 1))) {
+      // \x1b with allowed SGR
+      result = `${result}\x1b`
+    } else if (code <= 31) {
+      // escape all other C0 control characters besides \x7f
+      result = `${result}^${String.fromCharCode(code + 64)}`
+    } else {
+      // hasC01 ensures this is now a C1 control character or \x7f
+      result = `${result}^${String.fromCharCode(code - 64)}`
+    }
+  }
+  return result
+}
+
 class Display {
   #chalk = null
 
@@ -12,6 +50,57 @@ class Display {
     log.pause()
   }
 
+  static clean (output) {
+    if (typeof output === 'string') {
+      // Strings are cleaned inline
+      return stripC01(output)
+    }
+    if (!output || typeof output !== 'object') {
+      // Numbers, booleans, null all end up here and don't need cleaning
+      return output
+    }
+    // output && typeof output === 'object'
+    // We can't use hasOwn et al for detecting the original but we can use it
+    // for detecting the properties we set via defineProperty
+    if (
+      output[inspect.custom] &&
+      (!Object.hasOwn(output, originalCustomInspect))
+    ) {
+      // Save the old one if we didn't already do it.
+      Object.defineProperty(output, originalCustomInspect, {
+        value: output[inspect.custom],
+        writable: true,
+      })
+    }
+    if (!Object.hasOwn(output, originalCustomInspect)) {
+      // Put a dummy one in for when we run multiple times on the same object
+      Object.defineProperty(output, originalCustomInspect, {
+        value: function () {
+          return this
+        },
+        writable: true,
+      })
+    }
+    // Set the custom inspect to our own function
+    Object.defineProperty(output, inspect.custom, {
+      value: function () {
+        const toClean = this[originalCustomInspect]()
+        // Custom inspect can return things other than objects, check type again
+        if (typeof toClean === 'string') {
+          // Strings are cleaned inline
+          return stripC01(toClean)
+        }
+        if (!toClean || typeof toClean !== 'object') {
+          // Numbers, booleans, null all end up here and don't need cleaning
+          return toClean
+        }
+        return stripC01(inspect(toClean, { customInspect: false }))
+      },
+      writable: true,
+    })
+    return output
+  }
+
   on () {
     process.on('log', this.#logHandler)
   }
@@ -103,7 +192,7 @@ class Display {
   // Explicitly call these on npmlog and not log shim
   // This is the final place we should call npmlog before removing it.
   #npmlog (level, ...args) {
-    npmlog[level](...args)
+    npmlog[level](...args.map(Display.clean))
   }
 
   // Also (and this is a really inexcusable kludge), we patch the
@@ -112,8 +201,8 @@ class Display {
   // highly abbreviated explanation of what's being overridden.
   #eresolveWarn (level, heading, message, expl) {
     if (level === 'warn' &&
-        heading === 'ERESOLVE' &&
-        expl && typeof expl === 'object'
+      heading === 'ERESOLVE' &&
+      expl && typeof expl === 'object'
     ) {
       this.#npmlog(level, heading, message)
       this.#npmlog(level, '', explain(expl, this.#chalk, 2))

lib/utils/log-file.js

@@ -6,6 +6,7 @@ const { Minipass } = require('minipass')
 const fsMiniPass = require('fs-minipass')
 const fs = require('fs/promises')
 const log = require('./log-shim')
+const Display = require('./display')
 
 const padZero = (n, length) => n.toString().padStart(length.toString().length, '0')
 const globify = pattern => pattern.split('\\').join('/')
@@ -49,6 +50,7 @@ class LogFiles {
 
     return format(...args)
       .split(/\r?\n/)
+      .map(Display.clean)
       .reduce((lines, line) =>
         lines += prefix + (line ? ' ' : '') + line + os.EOL,
       ''

lib/utils/reify-output.js

@@ -76,7 +76,7 @@ const reifyOutput = (npm, arb) => {
       summary.audit = npm.command === 'audit' ? auditReport
         : auditReport.toJSON().metadata
     }
-    npm.output(JSON.stringify(summary, 0, 2))
+    npm.output(JSON.stringify(summary, null, 2))
   } else {
     packagesChangedMessage(npm, summary)
     packagesFundingMessage(npm, summary)