From ebd0edefce0450181ad45e9fe1e724658fea0d0a Mon Sep 17 00:00:00 2001 From: Gar Date: Fri, 8 Dec 2023 08:39:31 -0800 Subject: [PATCH] fix: filter C0 and C1 control characters from logs and cli output --- lib/commands/view.js | 14 +- lib/npm.js | 4 +- lib/utils/display.js | 95 +++++++++- lib/utils/log-file.js | 2 + lib/utils/reify-output.js | 2 +- .../test/lib/commands/view.js.test.cjs | 176 +++++++++--------- test/lib/npm.js | 13 +- test/lib/utils/display.js | 81 +++++++- 8 files changed, 278 insertions(+), 109 deletions(-) diff --git a/lib/commands/view.js b/lib/commands/view.js index f118184124db9..214a45e92611c 100644 --- a/lib/commands/view.js +++ b/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('') diff --git a/lib/npm.js b/lib/npm.js index 3322131d8df44..0a023f4ac8a30 100644 --- a/lib/npm.js +++ b/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() } } diff --git a/lib/utils/display.js b/lib/utils/display.js index a41bf903e9a8f..c5e5ca2b5b874 100644 --- a/lib/utils/display.js +++ b/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)) diff --git a/lib/utils/log-file.js b/lib/utils/log-file.js index 84f86983639ce..8c06f5647e761 100644 --- a/lib/utils/log-file.js +++ b/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, '' diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js index 22036dc8110cf..3b79fc2be1898 100644 --- a/lib/utils/reify-output.js +++ b/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) diff --git a/tap-snapshots/test/lib/commands/view.js.test.cjs b/tap-snapshots/test/lib/commands/view.js.test.cjs index 5248d439afad9..1c37e26db7f82 100644 --- a/tap-snapshots/test/lib/commands/view.js.test.cjs +++ b/tap-snapshots/test/lib/commands/view.js.test.cjs @@ -12,23 +12,23 @@ green is a very important color DEPRECATED!! - true -keywords:,colors, green, crayola +keywords: colors, green, crayola -bin:,green +bin: green dist -.tarball:,http://hm.green.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1.0 GB +.tarball: http://hm.green.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1.0 GB dependencies: red: 1.0.0 yellow: 1.0.0 maintainers: --,claudia <c@yellow.com> --,isaacs <i@yellow.com> +- claudia <c@yellow.com> +- isaacs <i@yellow.com> dist-tags: latest: 1.0.0 @@ -41,23 +41,23 @@ green is a very important color DEPRECATED ⚠️ - true -keywords:,colors, green, crayola +keywords: colors, green, crayola -bin:,green +bin: green dist -.tarball:,http://hm.green.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1.0 GB +.tarball: http://hm.green.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1.0 GB dependencies: red: 1.0.0 yellow: 1.0.0 maintainers: --,claudia <c@yellow.com> --,isaacs <i@yellow.com> +- claudia <c@yellow.com> +- isaacs <i@yellow.com> dist-tags: latest: 1.0.0 @@ -70,23 +70,23 @@ green is a very important color DEPRECATED!! - true -keywords:,colors, green, crayola +keywords: colors, green, crayola -bin:,green +bin: green dist -.tarball:,http://hm.green.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1.0 GB +.tarball: http://hm.green.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1.0 GB dependencies: red: 1.0.0 yellow: 1.0.0 maintainers: --,claudia <c@yellow.com> --,isaacs <i@yellow.com> +- claudia <c@yellow.com> +- isaacs <i@yellow.com> dist-tags: latest: 1.0.0 @@ -97,10 +97,10 @@ exports[`test/lib/commands/view.js TAP package in cwd directory > must match sna blue@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.blue.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.blue.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -113,10 +113,10 @@ exports[`test/lib/commands/view.js TAP package in cwd non-specific version > mus blue@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.blue.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.blue.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -129,10 +129,10 @@ exports[`test/lib/commands/view.js TAP package in cwd specific version > must ma blue@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.blue.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.blue.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -181,10 +181,10 @@ exports[`test/lib/commands/view.js TAP package with homepage > must match snapsh http://hm.orange.com dist -.tarball:,http://hm.orange.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.orange.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -195,10 +195,10 @@ exports[`test/lib/commands/view.js TAP package with maintainers info as object > pink@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.pink.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.pink.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -209,10 +209,10 @@ exports[`test/lib/commands/view.js TAP package with more than 25 deps > must mat black@1.0.0 | Proprietary | deps: 25 | versions: 2 dist -.tarball:,http://hm.black.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.black.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dependencies: 0: 1.0.0 @@ -250,10 +250,10 @@ exports[`test/lib/commands/view.js TAP package with no modified time > must matc cyan@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.cyan.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1.0 MB +.tarball: http://hm.cyan.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1.0 MB dist-tags: latest: 1.0.0 @@ -266,10 +266,10 @@ exports[`test/lib/commands/view.js TAP package with no repo or homepage > must m blue@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.blue.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.blue.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -282,10 +282,10 @@ exports[`test/lib/commands/view.js TAP package with semver range > must match sn blue@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.blue.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.blue.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -295,10 +295,10 @@ published {TIME} ago blue@1.0.1 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.blue.com/1.0.1.tgz -.shasum:,124 -.integrity:,--- -.unpackedSize:,1.0 kB +.tarball: http://hm.blue.com/1.0.1.tgz +.shasum: 124 +.integrity: --- +.unpackedSize: 1.0 kB dist-tags: latest: 1.0.0 @@ -436,23 +436,23 @@ green is a very important color DEPRECATED!! - true -keywords:,colors, green, crayola +keywords: colors, green, crayola -bin:,green +bin: green dist -.tarball:,http://hm.green.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1.0 GB +.tarball: http://hm.green.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1.0 GB dependencies: red: 1.0.0 yellow: 1.0.0 maintainers: --,claudia <c@yellow.com> --,isaacs <i@yellow.com> +- claudia <c@yellow.com> +- isaacs <i@yellow.com> dist-tags: latest: 1.0.0 @@ -461,10 +461,10 @@ dist-tags: http://hm.orange.com dist -.tarball:,http://hm.orange.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.orange.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 @@ -500,23 +500,23 @@ green is a very important color DEPRECATED!! - true -keywords:,colors, green, crayola +keywords: colors, green, crayola -bin:,green +bin: green dist -.tarball:,http://hm.green.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1.0 GB +.tarball: http://hm.green.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1.0 GB dependencies: red: 1.0.0 yellow: 1.0.0 maintainers: --,claudia <c@yellow.com> --,isaacs <i@yellow.com> +- claudia <c@yellow.com> +- isaacs <i@yellow.com> dist-tags: latest: 1.0.0 @@ -527,10 +527,10 @@ exports[`test/lib/commands/view.js TAP workspaces remote package name > must mat pink@1.0.0 | Proprietary | deps: none | versions: 2 dist -.tarball:,http://hm.pink.com/1.0.0.tgz -.shasum:,123 -.integrity:,--- -.unpackedSize:,1 B +.tarball: http://hm.pink.com/1.0.0.tgz +.shasum: 123 +.integrity: --- +.unpackedSize: 1 B dist-tags: latest: 1.0.0 diff --git a/test/lib/npm.js b/test/lib/npm.js index 162e8c83ca4a4..e9300ecfa6bd1 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -388,15 +388,18 @@ t.test('debug log', async t => { const log1 = ['silly', 'test', 'before load'] const log2 = ['silly', 'test', 'after load'] + const log3 = ['silly', 'test', 'hello\x00world'] process.emit('log', ...log1) await npm.load() process.emit('log', ...log2) + process.emit('log', ...log3) const debug = await debugFile() t.equal(npm.logFiles.length, 1, 'one debug file') t.match(debug, log1.join(' '), 'before load appears') t.match(debug, log2.join(' '), 'after load log appears') + t.match(debug, 'hello^@world') }) t.test('can load with bad dir', async t => { @@ -517,7 +520,7 @@ t.test('timings', async t => { } }) -t.test('output clears progress and console.logs the message', async t => { +t.test('output clears progress and console.logs cleaned messages', async t => { t.plan(4) let showingProgress = true const logs = [] @@ -541,11 +544,11 @@ t.test('output clears progress and console.logs the message', async t => { }, }, }) - npm.originalOutput('hello') - npm.originalOutputError('error') + npm.originalOutput('hello\x00world') + npm.originalOutputError('error\x00world') - t.match(logs, [['hello']]) - t.match(errors, [['error']]) + t.match(logs, [['hello^@world']]) + t.match(errors, [['error^@world']]) }) t.test('aliases and typos', async t => { diff --git a/test/lib/utils/display.js b/test/lib/utils/display.js index b8f047668bfe4..2b9db0e672510 100644 --- a/test/lib/utils/display.js +++ b/test/lib/utils/display.js @@ -3,6 +3,7 @@ const log = require('../../../lib/utils/log-shim') const mockLogs = require('../../fixtures/mock-logs') const mockGlobals = require('@npmcli/mock-globals') const tmock = require('../../fixtures/tmock') +const util = require('util') const mockDisplay = (t, mocks) => { const { logs, logMocks } = mockLogs(mocks) @@ -38,7 +39,7 @@ t.test('setup', async (t) => { t.equal(log.progressEnabled, true) }) -t.test('can log', async (t) => { +t.test('can log cleanly', async (t) => { const explains = [] const { display, logs } = mockDisplay(t, { npmlog: { @@ -53,8 +54,8 @@ t.test('can log', async (t) => { }, }) - display.log('error', 'test') - t.match(logs.error, [['test']]) + display.log('error', 'test\x00message') + t.match(logs.error, [['test^@message']]) display.log('warn', 'ERESOLVE', 'hello', { some: 'object' }) t.match(logs.warn, [['ERESOLVE', 'hello']]) @@ -84,3 +85,77 @@ t.test('handles log throwing', async (t) => { [/attempt to log .* crashed/, Error('explain'), Error('verbose')], ]) }) + +class CustomObj { + [util.inspect.custom] () { + return this.inspected + } +} + +t.test('Display.clean', async (t) => { + const Display = require('../../../lib/utils/display') + const customNaN = new CustomObj() + const customNull = new CustomObj() + const customNumber = new CustomObj() + const customObject = new CustomObj() + const customString = new CustomObj() + const customUndefined = new CustomObj() + customNaN.inspected = NaN + customNull.inspected = null + customNumber.inspected = 477 + customObject.inspected = { custom: 'rend\x00ering' } + customString.inspected = 'custom\x00rendering' + customUndefined.inspected = undefined + t.test('strings', async (t) => { + const tests = [ + [477, '477'], + [null, 'null'], + [NaN, 'NaN'], + [true, 'true'], + [undefined, 'undefined'], + ['🚀', '🚀'], + // Cover the bounds of each range and a few characters from inside each range + // \x00 through \x1f + ['hello\x00world', 'hello^@world'], + ['hello\x07world', 'hello^Gworld'], + ['hello\x1bworld', 'hello^[world'], + ['hello\x1eworld', 'hello^^world'], + ['hello\x1fworld', 'hello^_world'], + // \x7f is C0 + ['hello\x7fworld', 'hello^?world'], + // \x80 through \x9f + ['hello\x80world', 'hello^@world'], + ['hello\x87world', 'hello^Gworld'], + ['hello\x9eworld', 'hello^^world'], + ['hello\x9fworld', 'hello^_world'], + // Allowed C0 + ['hello\tworld', 'hello\tworld'], + ['hello\nworld', 'hello\nworld'], + ['hello\vworld', 'hello\vworld'], + ['hello\rworld', 'hello\rworld'], + // Allowed SGR + ['hello\x1b[38;5;254mworld', 'hello\x1b[38;5;254mworld'], + ['hello\x1b[mworld', 'hello\x1b[mworld'], + // Unallowed CSI / OSC + ['hello\x1b[2Aworld', 'hello^[[2Aworld'], + ['hello\x9b[2Aworld', 'hello^[[2Aworld'], + ['hello\x9decho goodbye\x9cworld', 'hello^]echo goodbye^\\world'], + // This is done twice to ensure we define inspect.custom as writable + [{ test: 'object' }, "{ test: 'object' }"], + [{ test: 'object' }, "{ test: 'object' }"], + // Make sure custom util.inspect doesn't bypass our cleaning + [customNaN, 'NaN'], + [customNull, 'null'], + [customNumber, '477'], + [customObject, "{ custom: 'rend\\x00ering' }"], + [customString, 'custom^@rendering'], + [customUndefined, 'undefined'], + // UTF-16 form of 8-bit C1 + ['hello\xc2\x9bworld', 'hello\xc2^[world'], + ] + for (const [dirty, clean] of tests) { + const cleaned = Display.clean(dirty) + t.equal(util.format(cleaned), clean) + } + }) +})