fix(replay): Fix console formatter handling of literal "%" (#45167)

Use `util.format` (from [browser implementation of node's `util`](https://github.com/browserify/node-util/)) instead of sprintf as it is exactly what we need to parse console.log messages. We do not need the full sprintf formatting support and it also handles literal `%` poorly.


fixes JAVASCRIPT-2AKF
fixes JAVASCRIPT-2EQJ
fixes JAVASCRIPT-2EQK

package.json

@@ -159,6 +159,7 @@
     "typescript": "^4.9.4",
     "u2f-api": "1.0.10",
     "url-loader": "^4.1.0",
+    "util": "^0.12.5",
     "webpack": "5.75.0",
     "webpack-cli": "5.0.1",
     "webpack-remove-empty-scripts": "^1.0.1",

static/app/views/replays/detail/console/messageFormatter.spec.tsx

@@ -20,7 +20,7 @@ const breadcrumbs: Extract<Crumb, BreadcrumbTypeDefault>[] = [
     level: BreadcrumbLevelType.LOG,
     message: 'This is a %s test',
     timestamp: '2022-06-22T20:00:39.959Z',
-    id: 1,
+    id: 0,
     color: 'purple300',
     description: 'Debug',
   },
@@ -34,7 +34,7 @@ const breadcrumbs: Extract<Crumb, BreadcrumbTypeDefault>[] = [
     level: BreadcrumbLevelType.LOG,
     message: 'test 1 false [object Object]',
     timestamp: '2022-06-22T16:49:11.198Z',
-    id: 2,
+    id: 1,
     color: 'purple300',
     description: 'Debug',
   },
@@ -99,6 +99,48 @@ const breadcrumbs: Extract<Crumb, BreadcrumbTypeDefault>[] = [
     color: 'purple300',
     description: 'Debug',
   },
+  {
+    type: BreadcrumbType.DEBUG,
+    category: 'console',
+    data: {
+      arguments: ['This is a literal 100%'],
+      logger: 'console',
+    },
+    level: BreadcrumbLevelType.LOG,
+    message: 'This is a literal 100%',
+    timestamp: '2022-06-22T20:00:39.959Z',
+    id: 6,
+    color: 'purple300',
+    description: 'Debug',
+  },
+  {
+    type: BreadcrumbType.DEBUG,
+    category: 'console',
+    data: {
+      arguments: ['Unbound placeholder %s'],
+      logger: 'console',
+    },
+    level: BreadcrumbLevelType.LOG,
+    message: 'Unbound placeholder %s',
+    timestamp: '2022-06-22T20:00:39.959Z',
+    id: 7,
+    color: 'purple300',
+    description: 'Debug',
+  },
+  {
+    type: BreadcrumbType.DEBUG,
+    category: 'console',
+    data: {
+      arguments: ['Placeholder %s with 100%', 'myPlaceholder'],
+      logger: 'console',
+    },
+    level: BreadcrumbLevelType.LOG,
+    message: 'Placeholder %s with 100%',
+    timestamp: '2022-06-22T20:00:39.959Z',
+    id: 8,
+    color: 'purple300',
+    description: 'Debug',
+  },
 ];
 
 describe('MessageFormatter', () => {
@@ -129,12 +171,30 @@ describe('MessageFormatter', () => {
   it('Should ignore the "%c" placheholder and print the console message correctly', () => {
     render(<MessageFormatter breadcrumb={breadcrumbs[4]} />);
 
-    expect(screen.getByText('prev state {"cart":[]}')).toBeInTheDocument();
+    expect(screen.getByText('prev state { cart: [] }')).toBeInTheDocument();
   });
 
   it('Should print arrays correctly', () => {
     render(<MessageFormatter breadcrumb={breadcrumbs[5]} />);
 
-    expect(screen.getByText('test ["foo","bar"]')).toBeInTheDocument();
+    expect(screen.getByText("test [ 'foo', 'bar' ]")).toBeInTheDocument();
+  });
+
+  it('Should print literal %', () => {
+    render(<MessageFormatter breadcrumb={breadcrumbs[6]} />);
+
+    expect(screen.getByText('This is a literal 100%')).toBeInTheDocument();
+  });
+
+  it('Should print unbound %s placeholder', () => {
+    render(<MessageFormatter breadcrumb={breadcrumbs[7]} />);
+
+    expect(screen.getByText('Unbound placeholder %s')).toBeInTheDocument();
+  });
+
+  it('Should print placeholder with literal %', () => {
+    render(<MessageFormatter breadcrumb={breadcrumbs[8]} />);
+
+    expect(screen.getByText('Placeholder myPlaceholder with 100%')).toBeInTheDocument();
   });
 });

static/app/views/replays/detail/console/messageFormatter.tsx

@@ -1,9 +1,11 @@
-import isObject from 'lodash/isObject';
-import {sprintf, vsprintf} from 'sprintf-js';
+// webpack fallback handles this
+// eslint-disable-next-line
+import {format} from 'util';
 
 import {AnnotatedText} from 'sentry/components/events/meta/annotatedText';
 import {getMeta} from 'sentry/components/events/meta/metaProxy';
 import {BreadcrumbTypeDefault, Crumb} from 'sentry/types/breadcrumbs';
+import isObject from 'lodash/isObject';
 import {objectIsEmpty} from 'sentry/utils';
 
 interface Props {
@@ -25,61 +27,19 @@ function MessageFormatter({breadcrumb}: Props) {
     return <AnnotatedText meta={getMeta(breadcrumb, 'message')} value={logMessage} />;
   }
 
-  // Browser's console formatter only works on the first arg
-  const [message, ...args] = breadcrumb.data?.arguments;
-
-  const isMessageString = typeof message === 'string';
-
-  const placeholders = isMessageString
-    ? sprintf.parse(message).filter(parsed => Array.isArray(parsed))
-    : [];
-
-  // Placeholders can only occur in the first argument and only if it is a string.
-  // We can skip the below code and avoid using `sprintf` if there are no placeholders.
-  if (placeholders.length) {
-    // TODO `%c` is console specific, it applies colors to messages
-    // for now we are stripping it as this is potentially risky to implement due to xss
-    const consoleColorPlaceholderIndexes = placeholders
-      .filter(([placeholder]) => placeholder === '%c')
-      .map((_, i) => i);
-
-    // Retrieve message formatting args
-    const messageArgs = args.slice(0, placeholders.length);
-
-    // Filter out args that were for %c
-    for (const colorIndex of consoleColorPlaceholderIndexes) {
-      messageArgs.splice(colorIndex, 1);
-    }
-
-    // Attempt to stringify the rest of the args
-    const restArgs = args.slice(placeholders.length).map(renderString);
-
-    const formattedMessage = isMessageString
-      ? vsprintf(message.replaceAll('%c', ''), messageArgs)
-      : renderString(message);
-
-    logMessage = [formattedMessage, ...restArgs].join(' ').trim();
-  } else if (
+  // There is a special case where `console.error()` is called with an Error object.
+  // The SDK uses the Error's `message` property as the breadcrumb message, but we lose the Error type,
+  // resulting in an empty object in the breadcrumb arguments. In this case, we
+  // only want to use `breadcrumb.message`.
+  if (
+    breadcrumb.message &&
     breadcrumb.data?.arguments.length === 1 &&
-    isObject(message) &&
-    objectIsEmpty(message)
+    isObject(breadcrumb.data.arguments[0]) &&
+    objectIsEmpty(breadcrumb.data.arguments[0])
   ) {
-    // There is a special case where `console.error()` is called with an Error object.
-    // The SDK uses the Error's `message` property as the breadcrumb message, but we lose the Error type,
-    // resulting in an empty object in the breadcrumb arguments. In this case, we
-    // only want to use `breadcrumb.message`.
-    logMessage = breadcrumb.message || JSON.stringify(message);
+    logMessage = breadcrumb.message;
   } else {
-    // If the string `[object Object]` is found in message, it means the SDK attempted to stringify an object,
-    // but the actual object should be captured in the arguments.
-    //
-    // Likewise if arrays are found e.g. [test,test] the SDK will serialize it to 'test, test'.
-    //
-    // In those cases, we'll want to use our pretty print in every argument that was passed to the logger instead of using
-    // the SDK's serialization.
-    const argValues = breadcrumb.data?.arguments.map(renderString);
-
-    logMessage = argValues.join(' ').trim();
+    logMessage = format(...breadcrumb.data.arguments);
   }
 
   // TODO(replays): Add better support for AnnotatedText (e.g. we use message
@@ -87,19 +47,4 @@ function MessageFormatter({breadcrumb}: Props) {
   return <AnnotatedText meta={getMeta(breadcrumb, 'message')} value={logMessage} />;
 }
 
-/**
- * Attempt to stringify
- */
-function renderString(arg: string | number | boolean | object) {
-  if (typeof arg !== 'object') {
-    return arg;
-  }
-
-  try {
-    return JSON.stringify(arg);
-  } catch {
-    return arg.toString();
-  }
-}
-
 export default MessageFormatter;

webpack.config.ts

@@ -437,6 +437,7 @@ const appConfig: Configuration = {
       vm: false,
       stream: false,
       crypto: require.resolve('crypto-browserify'),
+      util: require.resolve('util'),
       // `yarn why` says this is only needed in dev deps
       string_decoder: false,
     },

yarn.lock

@@ -3640,6 +3640,11 @@ autosize@^4.0.2:
   resolved "https://registry.yarnpkg.com/autosize/-/autosize-4.0.2.tgz#073cfd07c8bf45da4b9fd153437f5bafbba1e4c9"
   integrity sha512-jnSyH2d+qdfPGpWlcuhGiHmqBJ6g3X+8T+iRwFrHPLVcdoGJE/x6Qicm6aDHfTsbgZKxyV8UU/YB2p4cjKDRRA==
 
+available-typed-arrays@^1.0.5:
+  version "1.0.5"
+  resolved "https://registry.yarnpkg.com/available-typed-arrays/-/available-typed-arrays-1.0.5.tgz#92f95616501069d07d10edb2fc37d3e1c65123b7"
+  integrity sha512-DMD0KiN46eipeziST1LPP/STfDU0sufISXmjSgvVsoU2tqxctQeASejWcfNtxYKqETM1UxQ8sp2OrSBWpHY6sw==
+
 babel-gettext-extractor@^4.1.3:
   version "4.1.3"
   resolved "https://registry.yarnpkg.com/babel-gettext-extractor/-/babel-gettext-extractor-4.1.3.tgz#7d3343b9c9875cf6d25f1526f9de05598bd61f83"
@@ -5839,6 +5844,13 @@ follow-redirects@^1.0.0:
   resolved "https://registry.yarnpkg.com/follow-redirects/-/follow-redirects-1.14.8.tgz#016996fb9a11a100566398b1c6839337d7bfa8fc"
   integrity sha512-1x0S9UVJHsQprFcEC/qnNzBLcIxsjAV905f/UkQxbclCsoTWlacCNOpQa/anodLl2uaEKFhfWOvM2Qg77+15zA==
 
+for-each@^0.3.3:
+  version "0.3.3"
+  resolved "https://registry.yarnpkg.com/for-each/-/for-each-0.3.3.tgz#69b447e88a0a5d32c3e7084f3f1710034b21376e"
+  integrity sha512-jqYfLp7mo9vIyQf8ykW2v7A+2N4QjeCeI5+Dz9XraiO1ign81wjiH7Fb9vSOWvQfNtmSa4H2RoQTrrXivdUZmw==
+  dependencies:
+    is-callable "^1.1.3"
+
 fork-ts-checker-webpack-plugin@^7.3.0:
   version "7.3.0"
   resolved "https://registry.yarnpkg.com/fork-ts-checker-webpack-plugin/-/fork-ts-checker-webpack-plugin-7.3.0.tgz#a9c984a018493962360d7c7e77a67b44a2d5f3aa"
@@ -6139,6 +6151,13 @@ globjoin@^0.1.4:
   resolved "https://registry.yarnpkg.com/globjoin/-/globjoin-0.1.4.tgz#2f4494ac8919e3767c5cbb691e9f463324285d43"
   integrity sha1-L0SUrIkZ43Z8XLtpHp9GMyQoXUM=
 
+gopd@^1.0.1:
+  version "1.0.1"
+  resolved "https://registry.yarnpkg.com/gopd/-/gopd-1.0.1.tgz#29ff76de69dac7489b7c0918a5788e56477c332c"
+  integrity sha512-d65bNlIadxvpb/A2abVdlqKqV563juRnZ1Wtk6s1sIR8uNsXR70xqIzVqxVf1eTqDunwT2MkczEeaezCKTZhwA==
+  dependencies:
+    get-intrinsic "^1.1.3"
+
 graceful-fs@^4.1.2, graceful-fs@^4.1.6, graceful-fs@^4.2.0, graceful-fs@^4.2.4, graceful-fs@^4.2.6, graceful-fs@^4.2.9:
   version "4.2.9"
   resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.2.9.tgz#041b05df45755e587a24942279b9d113146e1c96"
@@ -6606,6 +6625,14 @@ ipaddr.js@^2.0.1:
   resolved "https://registry.yarnpkg.com/ipaddr.js/-/ipaddr.js-2.0.1.tgz#eca256a7a877e917aeb368b0a7497ddf42ef81c0"
   integrity sha512-1qTgH9NG+IIJ4yfKs2e6Pp1bZg8wbDbKHT21HrLIeYBTRLgMYKnMTPAuI3Lcs61nfx5h1xlXnbJtH1kX5/d/ng==
 
+is-arguments@^1.0.4:
+  version "1.1.1"
+  resolved "https://registry.yarnpkg.com/is-arguments/-/is-arguments-1.1.1.tgz#15b3f88fda01f2a97fec84ca761a560f123efa9b"
+  integrity sha512-8Q7EARjzEnKpt/PCD7e1cgUS0a6X8u5tdSiMqXhojOdoV9TsMsiO+9VLC5vAmO8N7/GmXn7yjR8qnA6bVAEzfA==
+  dependencies:
+    call-bind "^1.0.2"
+    has-tostringtag "^1.0.0"
+
 is-arrayish@^0.2.1:
   version "0.2.1"
   resolved "https://registry.yarnpkg.com/is-arrayish/-/is-arrayish-0.2.1.tgz#77c99840527aa8ecb1a8ba697b80645a7a926a9d"
@@ -6635,7 +6662,7 @@ is-boolean-object@^1.1.0:
   dependencies:
     call-bind "^1.0.0"
 
-is-callable@^1.1.4, is-callable@^1.2.7:
+is-callable@^1.1.3, is-callable@^1.1.4, is-callable@^1.2.7:
   version "1.2.7"
   resolved "https://registry.yarnpkg.com/is-callable/-/is-callable-1.2.7.tgz#3bc2a85ea742d9e36205dcacdd72ca1fdc51b055"
   integrity sha512-1BC0BVFhS/p0qtw6enp8e+8OD0UrK0oFLztSjNzhcKA3WDuJxxAPXzPuPtKkjEY9UUoEWlX/8fgKeu2S8i9JTA==
@@ -6672,6 +6699,13 @@ is-generator-fn@^2.0.0:
   resolved "https://registry.yarnpkg.com/is-generator-fn/-/is-generator-fn-2.1.0.tgz#7d140adc389aaf3011a8f2a2a4cfa6faadffb118"
   integrity sha512-cTIB4yPYL/Grw0EaSzASzg6bBy9gqCofvWN8okThAYIxKJZC+udlRAmGbM0XLeniEJSs8uEgHPGuHSe1XsOLSQ==
 
+is-generator-function@^1.0.7:
+  version "1.0.10"
+  resolved "https://registry.yarnpkg.com/is-generator-function/-/is-generator-function-1.0.10.tgz#f1558baf1ac17e0deea7c0415c438351ff2b3c72"
+  integrity sha512-jsEjy9l3yiXEQ+PsXdmBwEPcOxaXWLspKdplFUVI9vq1iZgIekeC0L167qeu86czQaxed3q/Uzuw0swL0irL8A==
+  dependencies:
+    has-tostringtag "^1.0.0"
+
 is-glob@^4.0.0, is-glob@^4.0.1, is-glob@^4.0.3, is-glob@~4.0.1:
   version "4.0.3"
   resolved "https://registry.yarnpkg.com/is-glob/-/is-glob-4.0.3.tgz#64f61e42cbbb2eec2071a9dac0b28ba1e65d5084"
@@ -6775,6 +6809,17 @@ is-symbol@^1.0.2, is-symbol@^1.0.3:
   dependencies:
     has-symbols "^1.0.2"
 
+is-typed-array@^1.1.10, is-typed-array@^1.1.3:
+  version "1.1.10"
+  resolved "https://registry.yarnpkg.com/is-typed-array/-/is-typed-array-1.1.10.tgz#36a5b5cb4189b575d1a3e4b08536bfb485801e3f"
+  integrity sha512-PJqgEHiWZvMpaFZ3uTc8kHPM4+4ADTlDniuQL7cU/UDA0Ql7F70yGfHph3cLNe+c9toaigv+DFzTJKhc2CtO6A==
+  dependencies:
+    available-typed-arrays "^1.0.5"
+    call-bind "^1.0.2"
+    for-each "^0.3.3"
+    gopd "^1.0.1"
+    has-tostringtag "^1.0.0"
+
 is-weakref@^1.0.2:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/is-weakref/-/is-weakref-1.0.2.tgz#9529f383a9338205e89765e0392efc2f100f06f2"
@@ -10704,6 +10749,17 @@ util-deprecate@^1.0.1, util-deprecate@^1.0.2, util-deprecate@~1.0.1:
   resolved "https://registry.yarnpkg.com/util-deprecate/-/util-deprecate-1.0.2.tgz#450d4dc9fa70de732762fbd2d4a28981419a0ccf"
   integrity sha1-RQ1Nyfpw3nMnYvvS1KKJgUGaDM8=
 
+util@^0.12.5:
+  version "0.12.5"
+  resolved "https://registry.yarnpkg.com/util/-/util-0.12.5.tgz#5f17a6059b73db61a875668781a1c2b136bd6fbc"
+  integrity sha512-kZf/K6hEIrWHI6XqOFUiiMa+79wE/D8Q+NCNAWclkyg3b4d2k7s0QGepNjiABc+aR3N1PAyHL7p6UcLY6LmrnA==
+  dependencies:
+    inherits "^2.0.3"
+    is-arguments "^1.0.4"
+    is-generator-function "^1.0.7"
+    is-typed-array "^1.1.3"
+    which-typed-array "^1.1.2"
+
 utila@~0.4:
   version "0.4.0"
   resolved "https://registry.yarnpkg.com/utila/-/utila-0.4.0.tgz#8a16a05d445657a3aea5eecc5b12a4fa5379772c"
@@ -11024,6 +11080,18 @@ which-boxed-primitive@^1.0.2:
     is-string "^1.0.5"
     is-symbol "^1.0.3"
 
+which-typed-array@^1.1.2:
+  version "1.1.9"
+  resolved "https://registry.yarnpkg.com/which-typed-array/-/which-typed-array-1.1.9.tgz#307cf898025848cf995e795e8423c7f337efbde6"
+  integrity sha512-w9c4xkx6mPidwp7180ckYWfMmvxpjlZuIudNtDf4N/tTAUB8VJbX25qZoAsrtGuYNnGw3pa0AXgbGKRB8/EceA==
+  dependencies:
+    available-typed-arrays "^1.0.5"
+    call-bind "^1.0.2"
+    for-each "^0.3.3"
+    gopd "^1.0.1"
+    has-tostringtag "^1.0.0"
+    is-typed-array "^1.1.10"
+
 which@^1.3.1:
   version "1.3.1"
   resolved "https://registry.yarnpkg.com/which/-/which-1.3.1.tgz#a45043d54f5805316da8d62f9f50918d3da70b0a"