From fc6f8f3fe5700fc59cd89dddeb21183af0129834 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 16 Mar 2022 13:47:03 +0800 Subject: [PATCH] always log errors with stack trace (#3573) but don't do it for common error types that we ourselves produce fixes #3495 --- packages/@uppy/companion/src/server/logger.js | 39 ++++++++----------- .../@uppy/companion/src/server/middlewares.js | 2 +- .../@uppy/companion/src/standalone/index.js | 5 +-- .../@uppy/companion/test/__tests__/logger.js | 5 +-- 4 files changed, 21 insertions(+), 30 deletions(-) diff --git a/packages/@uppy/companion/src/server/logger.js b/packages/@uppy/companion/src/server/logger.js index 7972ba945..d7bb05153 100644 --- a/packages/@uppy/companion/src/server/logger.js +++ b/packages/@uppy/companion/src/server/logger.js @@ -1,6 +1,7 @@ const chalk = require('chalk') const escapeStringRegexp = require('escape-string-regexp') const util = require('util') +const { ProviderApiError, ProviderAuthError } = require('./provider/error') const valuesToMask = [] /** @@ -35,37 +36,30 @@ function maskMessage (msg) { /** * message log * - * @param {string | Error} msg the message to log + * @param {string | Error} arg the message or error to log * @param {string} tag a unique tag to easily search for this message * @param {string} level error | info | debug * @param {string=} id a unique id to easily trace logs tied to a request * @param {Function=} color function to display the log in appropriate color - * @param {boolean=} shouldLogStackTrace when set to true, errors will be logged with their stack trace */ -const log = (msg, tag = '', level, id = '', color = (message) => message, shouldLogStackTrace) => { +const log = (arg, tag = '', level, id = '', color = (message) => message) => { const time = new Date().toISOString() const whitespace = tag && id ? ' ' : '' - function logMsg (msg2) { - let msgString = typeof msg2 === 'string' ? msg2 : util.inspect(msg2) - msgString = maskMessage(msgString) - // eslint-disable-next-line no-console - console.log(color(`companion: ${time} [${level}] ${id}${whitespace}${tag}`), color(msgString)) - } - - if (msg instanceof Error) { - // Not sure why it only logs the stack without the message, but this is how the code was originally - if (shouldLogStackTrace && typeof msg.stack === 'string') { - logMsg(msg.stack) - return + function msgToString () { + // We don't need to log stack trace on special errors that we ourselves have produced + // (to reduce log noise) + if ((arg instanceof ProviderApiError || arg instanceof ProviderAuthError) && typeof arg.message === 'string') { + return arg.message } - - // We don't want to log stack trace (this is how the code was originally) - logMsg(String(msg)) - return + if (typeof arg === 'string') return arg + return util.inspect(arg) } - logMsg(msg) + const msgString = msgToString() + const masked = maskMessage(msgString) + // eslint-disable-next-line no-console + console.log(color(`companion: ${time} [${level}] ${id}${whitespace}${tag}`), color(masked)) } /** @@ -97,11 +91,10 @@ exports.warn = (msg, tag, traceId) => { * @param {string | Error} msg the message to log * @param {string=} tag a unique tag to easily search for this message * @param {string=} traceId a unique id to easily trace logs tied to a request - * @param {boolean=} shouldLogStackTrace when set to true, errors will be logged with their stack trace */ -exports.error = (msg, tag, traceId, shouldLogStackTrace) => { +exports.error = (msg, tag, traceId) => { // @ts-ignore - log(msg, tag, 'error', traceId, chalk.bold.red, shouldLogStackTrace) + log(msg, tag, 'error', traceId, chalk.bold.red) } /** diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index eddb26d38..9e48be859 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/packages/@uppy/companion/src/server/middlewares.js @@ -42,7 +42,7 @@ exports.verifyToken = (req, res, next) => { const { err, payload } = tokenService.verifyEncryptedToken(token, req.companion.options.secret) if (err || !payload[providerName]) { if (err) { - logger.error(err, 'token.verify.error', req.id) + logger.error(err.message, 'token.verify.error', req.id) } return res.sendStatus(401) } diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index 918ec91f6..0f546ff3a 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -197,18 +197,17 @@ module.exports = function server (inputCompanionOptions = {}) { // @ts-ignore app.use((err, req, res, next) => { // eslint-disable-line no-unused-vars - const logStackTrace = true if (app.get('env') === 'production') { // if the error is a URIError from the requested URL we only log the error message // to avoid uneccessary error alerts if (err.status === 400 && err instanceof URIError) { logger.error(err.message, 'root.error', req.id) } else { - logger.error(err, 'root.error', req.id, logStackTrace) + logger.error(err, 'root.error', req.id) } res.status(err.status || 500).json({ message: 'Something went wrong', requestId: req.id }) } else { - logger.error(err, 'root.error', req.id, logStackTrace) + logger.error(err, 'root.error', req.id) res.status(err.status || 500).json({ message: err.message, error: err, requestId: req.id }) } }) diff --git a/packages/@uppy/companion/test/__tests__/logger.js b/packages/@uppy/companion/test/__tests__/logger.js index cdeb0d8f5..a4592ac52 100644 --- a/packages/@uppy/companion/test/__tests__/logger.js +++ b/packages/@uppy/companion/test/__tests__/logger.js @@ -57,14 +57,13 @@ describe('Test Logger secret mask', () => { const exptectedMsg = chalk.bold.red('Error: this error has ****** and ****** and case-insensitive ******') - expect(loggedMessage).toBeTruthy() - expect(loggedMessage).toBe(exptectedMsg) + expect(loggedMessage.startsWith(exptectedMsg)).toBeTruthy() }) test('masks secret values present in log.error stack trace', () => { const loggedMessage = captureConsoleLog(() => { const err = new Error('this error has ToBeMasked1 and toBeMasked2 and case-insensitive TOBEMasKED2') - logger.error(err, '', '', true) + logger.error(err, '', '') }) const exptectedMsg = chalk.bold.red('Error: this error has ****** and ****** and case-insensitive ******')