From 3d2235949a23d39fd5d3f846f0f5d55fc30606ee Mon Sep 17 00:00:00 2001
From: Pierre Gayvallet <pierre.gayvallet@elastic.co>
Date: Wed, 8 Nov 2023 15:12:01 +0100
Subject: [PATCH] [logging] always escape control chars (#170826)

## Summary

Follow-up of https://github.com/elastic/kibana/pull/169773

Escape message even if it's not a string
---
 .../src/layouts/conversions/message.test.ts   | 48 +++++++++++++++++++
 .../src/layouts/conversions/message.ts        | 24 +++++-----
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.test.ts b/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.test.ts
index cf16b57c927823..2c84635d6711c8 100644
--- a/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.test.ts
+++ b/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.test.ts
@@ -62,4 +62,52 @@ describe('MessageConversion', () => {
       '\\u001b\\u0000[31mESC-INJECTION-LFUNICODE:\\u001b[32mSUCCESSFUL\\u001b[0m\\u0007\n\nInjecting 10.000 lols 😂\\u001b[10000;b\\u0007'
     );
   });
+
+  test('it should encode/escape ANSI chars lines from the message when not a string', () => {
+    expect(
+      MessageConversion.convert(
+        {
+          ...baseRecord,
+          // @ts-expect-error message is supposed to be a string
+          message: {
+            toString: () => 'toString...\u001b[5;7;6mThis is Fine\u001b[27m',
+          },
+        },
+        false
+      )
+    ).toEqual('toString...\\u001b[5;7;6mThis is Fine\\u001b[27m');
+  });
+
+  test('it should encode/escape ANSI chars lines from the error stack', () => {
+    const error = new Error('Something went bad');
+    error.stack = 'stack...\u001b[5;7;6mThis is Fine\u001b[27m';
+    expect(
+      MessageConversion.convert(
+        {
+          ...baseRecord,
+          message: 'Some message that will be ignored',
+          error,
+        },
+        false
+      )
+    ).toEqual('stack...\\u001b[5;7;6mThis is Fine\\u001b[27m');
+  });
+
+  test('it should encode/escape ANSI chars lines from the error stack when not a string', () => {
+    expect(
+      MessageConversion.convert(
+        {
+          ...baseRecord,
+          message: 'Some message that will be ignored',
+          error: {
+            // @ts-expect-error message is supposed to be a string
+            stack: {
+              toString: () => 'stackToString...\u001b[5;7;6mThis is Fine\u001b[27m',
+            },
+          },
+        },
+        false
+      )
+    ).toEqual('stackToString...\\u001b[5;7;6mThis is Fine\\u001b[27m');
+  });
 });
diff --git a/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.ts b/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.ts
index 706aa478d992b1..1d0dd15373aafc 100644
--- a/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.ts
+++ b/packages/core/logging/core-logging-common-internal/src/layouts/conversions/message.ts
@@ -17,17 +17,19 @@ export const MessageConversion: Conversion = {
   pattern: /%message/g,
   convert(record: LogRecord) {
     // Error stack is much more useful than just the message.
-    const str = record.error?.stack || record.message;
+    let str = record.error?.stack || record.message;
+    // typings may be wrong, there's scenarios where the message is not a plain string (e.g error stacks from the ES client)
+    if (typeof str !== 'string') {
+      str = String(str);
+    }
 
-    return typeof str === 'string' // We need to validate it's a string because, despite types, there are use case where it's not a string :/
-      ? str.replace(
-          CONTROL_CHAR_REGEXP,
-          // Escaping control chars via JSON.stringify to maintain consistency with `meta` and the JSON layout.
-          // This way, post analysis of the logs is easier as we can search the same patterns.
-          // Our benchmark didn't show a big difference in performance between custom-escaping vs. JSON.stringify one.
-          // The slice is removing the double-quotes.
-          (substr) => JSON.stringify(substr).slice(1, -1)
-        )
-      : str;
+    return str.replace(
+      CONTROL_CHAR_REGEXP,
+      // Escaping control chars via JSON.stringify to maintain consistency with `meta` and the JSON layout.
+      // This way, post analysis of the logs is easier as we can search the same patterns.
+      // Our benchmark didn't show a big difference in performance between custom-escaping vs. JSON.stringify one.
+      // The slice is removing the double-quotes.
+      (substr) => JSON.stringify(substr).slice(1, -1)
+    );
   },
 };