From 2dc40f136b9a7b1a36d80522a4e9eb165fad01b0 Mon Sep 17 00:00:00 2001
From: dej611 <dej611@gmail.com>
Date: Wed, 26 Apr 2023 18:25:27 +0200
Subject: [PATCH 1/4] :bug: Add fix for string that contains seaprator char

---
 .../data/common/exports/escape_value.test.ts  | 46 +++++++++++++++++--
 .../data/common/exports/escape_value.ts       | 31 +++++++++----
 .../data/common/exports/export_csv.test.ts    | 12 +++++
 .../data/common/exports/export_csv.tsx        |  2 +-
 4 files changed, 77 insertions(+), 14 deletions(-)

diff --git a/src/plugins/data/common/exports/escape_value.test.ts b/src/plugins/data/common/exports/escape_value.test.ts
index 9463af644d417..aa44e7631c6e6 100644
--- a/src/plugins/data/common/exports/escape_value.test.ts
+++ b/src/plugins/data/common/exports/escape_value.test.ts
@@ -13,7 +13,11 @@ describe('escapeValue', function () {
   describe('quoteValues is true', function () {
     let escapeValue: (val: string) => string;
     beforeEach(function () {
-      escapeValue = createEscapeValue(true, false);
+      escapeValue = createEscapeValue({
+        csvSeparator: ',',
+        quoteValues: true,
+        escapeFormulaValues: false,
+      });
     });
 
     it('should escape value with spaces', function () {
@@ -48,7 +52,11 @@ describe('escapeValue', function () {
   describe('quoteValues is false', function () {
     let escapeValue: (val: string) => string;
     beforeEach(function () {
-      escapeValue = createEscapeValue(false, false);
+      escapeValue = createEscapeValue({
+        csvSeparator: ',',
+        quoteValues: false,
+        escapeFormulaValues: false,
+      });
     });
 
     it('should return the value unescaped', function () {
@@ -57,11 +65,15 @@ describe('escapeValue', function () {
     });
   });
 
-  describe('escapeValues', () => {
+  describe('escapeFormulaValues', () => {
     describe('when true', () => {
       let escapeValue: (val: string) => string;
       beforeEach(function () {
-        escapeValue = createEscapeValue(true, true);
+        escapeValue = createEscapeValue({
+          csvSeparator: ',',
+          quoteValues: true,
+          escapeFormulaValues: true,
+        });
       });
 
       ['@', '+', '-', '='].forEach((badChar) => {
@@ -76,7 +88,11 @@ describe('escapeValue', function () {
     describe('when false', () => {
       let escapeValue: (val: string) => string;
       beforeEach(function () {
-        escapeValue = createEscapeValue(true, false);
+        escapeValue = createEscapeValue({
+          csvSeparator: ',',
+          quoteValues: true,
+          escapeFormulaValues: false,
+        });
       });
 
       ['@', '+', '-', '='].forEach((badChar) => {
@@ -86,4 +102,24 @@ describe('escapeValue', function () {
       });
     });
   });
+
+  describe('csvSeparator', () => {
+    it('should escape when text contains the separator char with quotes enabled', () => {
+      const escapeValue = createEscapeValue({
+        csvSeparator: ';',
+        quoteValues: true,
+        escapeFormulaValues: false,
+      });
+      expect(escapeValue('a;b')).to.be('"a;b"');
+    });
+
+    it('should not escape when text contains the separator char if quotes are disabled', () => {
+      const escapeValue = createEscapeValue({
+        csvSeparator: ';',
+        quoteValues: false,
+        escapeFormulaValues: false,
+      });
+      expect(escapeValue('a;b')).to.be('a;b');
+    });
+  });
 });
diff --git a/src/plugins/data/common/exports/escape_value.ts b/src/plugins/data/common/exports/escape_value.ts
index 393ce6043993a..e946477addc4c 100644
--- a/src/plugins/data/common/exports/escape_value.ts
+++ b/src/plugins/data/common/exports/escape_value.ts
@@ -21,17 +21,32 @@ type RawValue = string | object | null | undefined;
  *
  * See OWASP: https://www.owasp.org/index.php/CSV_Injection.
  */
-export function createEscapeValue(
-  quoteValues: boolean,
-  escapeFormulas: boolean
-): (val: RawValue) => string {
+export function createEscapeValue({
+  csvSeparator,
+  quoteValues,
+  escapeFormulaValues,
+}: {
+  csvSeparator: string;
+  quoteValues: boolean;
+  escapeFormulaValues: boolean;
+}): (val: RawValue) => string {
   return function escapeValue(val: RawValue) {
     if (val && typeof val === 'string') {
-      const formulasEscaped = escapeFormulas && cellHasFormulas(val) ? "'" + val : val;
-      if (quoteValues && nonAlphaNumRE.test(formulasEscaped)) {
-        return `"${formulasEscaped.replace(allDoubleQuoteRE, '""')}"`;
+      const formulasEscaped = escapeFormulaValues && cellHasFormulas(val) ? "'" + val : val;
+      if (quoteValues) {
+        if (nonAlphaNumRE.test(formulasEscaped)) {
+          return `"${formulasEscaped.replace(allDoubleQuoteRE, '""')}"`;
+        }
+        // string with the separator already inside need to be wrapped in quotes
+        // i.e. string with csvSeparator char in it like free text or some number formatting (1143 => 1,143)
+        if (val.includes(csvSeparator)) {
+          return `"${val}"`;
+        }
       }
     }
-    return val == null ? '' : val.toString();
+    // raw multi-terms are stringified as T1,T2,T3 so check if the final value contains the
+    // csv separator before returning (usually for raw values)
+    const stringVal = val == null ? '' : val.toString();
+    return quoteValues && stringVal.includes(csvSeparator) ? `"${stringVal}"` : stringVal;
   };
 }
diff --git a/src/plugins/data/common/exports/export_csv.test.ts b/src/plugins/data/common/exports/export_csv.test.ts
index 7d4f1347c6cba..0c005b4bdee2e 100644
--- a/src/plugins/data/common/exports/export_csv.test.ts
+++ b/src/plugins/data/common/exports/export_csv.test.ts
@@ -84,4 +84,16 @@ describe('CSV exporter', () => {
       })
     ).toMatch('columnOne\r\n"\'=1"\r\n');
   });
+
+  test('should escape text with csvSeparator char in it', () => {
+    const datatable = getDataTable();
+    datatable.rows[0].col1 = 'a,b';
+    expect(
+      datatableToCSV(datatable, {
+        ...getDefaultOptions(),
+        escapeFormulaValues: true,
+        formatFactory: () => ({ convert: (v: unknown) => v } as FieldFormat),
+      })
+    ).toMatch('columnOne\r\n"a,b"\r\n');
+  });
 });
diff --git a/src/plugins/data/common/exports/export_csv.tsx b/src/plugins/data/common/exports/export_csv.tsx
index 94dd1b78d2737..de71021fad63b 100644
--- a/src/plugins/data/common/exports/export_csv.tsx
+++ b/src/plugins/data/common/exports/export_csv.tsx
@@ -27,7 +27,7 @@ export function datatableToCSV(
   { columns, rows }: Datatable,
   { csvSeparator, quoteValues, formatFactory, raw, escapeFormulaValues }: CSVOptions
 ) {
-  const escapeValues = createEscapeValue(quoteValues, escapeFormulaValues);
+  const escapeValues = createEscapeValue({ csvSeparator, quoteValues, escapeFormulaValues });
   // Build the header row by its names
   const header = columns.map((col) => escapeValues(col.name));
 

From 7868c1687c73e3969f144d52daaf22084c51b7f7 Mon Sep 17 00:00:00 2001
From: dej611 <dej611@gmail.com>
Date: Thu, 27 Apr 2023 09:48:24 +0200
Subject: [PATCH 2/4] :bug: Fix other dependant fns

---
 packages/kbn-generate-csv/src/get_export_settings.ts |  6 +++++-
 src/plugins/data/common/exports/escape_value.test.ts | 12 ++++++------
 src/plugins/data/common/exports/escape_value.ts      |  8 ++++----
 src/plugins/data/common/exports/export_csv.tsx       |  6 +++++-
 .../discover/public/utils/convert_value_to_string.ts | 10 ++++++++--
 5 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/packages/kbn-generate-csv/src/get_export_settings.ts b/packages/kbn-generate-csv/src/get_export_settings.ts
index d2e62f9281418..46d31afaf9e4f 100644
--- a/packages/kbn-generate-csv/src/get_export_settings.ts
+++ b/packages/kbn-generate-csv/src/get_export_settings.ts
@@ -64,7 +64,11 @@ export const getExportSettings = async (
   ]);
 
   const escapeFormulaValues = config.escapeFormulaValues;
-  const escapeValue = createEscapeValue(quoteValues, escapeFormulaValues);
+  const escapeValue = createEscapeValue({
+    separator,
+    quoteValues,
+    escapeFormulaValues,
+  });
   const bom = config.useByteOrderMarkEncoding ? CSV_BOM_CHARS : '';
 
   return {
diff --git a/src/plugins/data/common/exports/escape_value.test.ts b/src/plugins/data/common/exports/escape_value.test.ts
index aa44e7631c6e6..c8645e49b1d3e 100644
--- a/src/plugins/data/common/exports/escape_value.test.ts
+++ b/src/plugins/data/common/exports/escape_value.test.ts
@@ -14,7 +14,7 @@ describe('escapeValue', function () {
     let escapeValue: (val: string) => string;
     beforeEach(function () {
       escapeValue = createEscapeValue({
-        csvSeparator: ',',
+        separator: ',',
         quoteValues: true,
         escapeFormulaValues: false,
       });
@@ -53,7 +53,7 @@ describe('escapeValue', function () {
     let escapeValue: (val: string) => string;
     beforeEach(function () {
       escapeValue = createEscapeValue({
-        csvSeparator: ',',
+        separator: ',',
         quoteValues: false,
         escapeFormulaValues: false,
       });
@@ -70,7 +70,7 @@ describe('escapeValue', function () {
       let escapeValue: (val: string) => string;
       beforeEach(function () {
         escapeValue = createEscapeValue({
-          csvSeparator: ',',
+          separator: ',',
           quoteValues: true,
           escapeFormulaValues: true,
         });
@@ -89,7 +89,7 @@ describe('escapeValue', function () {
       let escapeValue: (val: string) => string;
       beforeEach(function () {
         escapeValue = createEscapeValue({
-          csvSeparator: ',',
+          separator: ',',
           quoteValues: true,
           escapeFormulaValues: false,
         });
@@ -106,7 +106,7 @@ describe('escapeValue', function () {
   describe('csvSeparator', () => {
     it('should escape when text contains the separator char with quotes enabled', () => {
       const escapeValue = createEscapeValue({
-        csvSeparator: ';',
+        separator: ';',
         quoteValues: true,
         escapeFormulaValues: false,
       });
@@ -115,7 +115,7 @@ describe('escapeValue', function () {
 
     it('should not escape when text contains the separator char if quotes are disabled', () => {
       const escapeValue = createEscapeValue({
-        csvSeparator: ';',
+        separator: ';',
         quoteValues: false,
         escapeFormulaValues: false,
       });
diff --git a/src/plugins/data/common/exports/escape_value.ts b/src/plugins/data/common/exports/escape_value.ts
index e946477addc4c..ab60ab54b11ea 100644
--- a/src/plugins/data/common/exports/escape_value.ts
+++ b/src/plugins/data/common/exports/escape_value.ts
@@ -22,11 +22,11 @@ type RawValue = string | object | null | undefined;
  * See OWASP: https://www.owasp.org/index.php/CSV_Injection.
  */
 export function createEscapeValue({
-  csvSeparator,
+  separator,
   quoteValues,
   escapeFormulaValues,
 }: {
-  csvSeparator: string;
+  separator: string;
   quoteValues: boolean;
   escapeFormulaValues: boolean;
 }): (val: RawValue) => string {
@@ -39,7 +39,7 @@ export function createEscapeValue({
         }
         // string with the separator already inside need to be wrapped in quotes
         // i.e. string with csvSeparator char in it like free text or some number formatting (1143 => 1,143)
-        if (val.includes(csvSeparator)) {
+        if (val.includes(separator)) {
           return `"${val}"`;
         }
       }
@@ -47,6 +47,6 @@ export function createEscapeValue({
     // raw multi-terms are stringified as T1,T2,T3 so check if the final value contains the
     // csv separator before returning (usually for raw values)
     const stringVal = val == null ? '' : val.toString();
-    return quoteValues && stringVal.includes(csvSeparator) ? `"${stringVal}"` : stringVal;
+    return quoteValues && stringVal.includes(separator) ? `"${stringVal}"` : stringVal;
   };
 }
diff --git a/src/plugins/data/common/exports/export_csv.tsx b/src/plugins/data/common/exports/export_csv.tsx
index de71021fad63b..fe7d35fe03e49 100644
--- a/src/plugins/data/common/exports/export_csv.tsx
+++ b/src/plugins/data/common/exports/export_csv.tsx
@@ -27,7 +27,11 @@ export function datatableToCSV(
   { columns, rows }: Datatable,
   { csvSeparator, quoteValues, formatFactory, raw, escapeFormulaValues }: CSVOptions
 ) {
-  const escapeValues = createEscapeValue({ csvSeparator, quoteValues, escapeFormulaValues });
+  const escapeValues = createEscapeValue({
+    separator: csvSeparator,
+    quoteValues,
+    escapeFormulaValues,
+  });
   // Build the header row by its names
   const header = columns.map((col) => escapeValues(col.name));
 
diff --git a/src/plugins/discover/public/utils/convert_value_to_string.ts b/src/plugins/discover/public/utils/convert_value_to_string.ts
index f67abe8d53dc2..f310ba35bf1c0 100644
--- a/src/plugins/discover/public/utils/convert_value_to_string.ts
+++ b/src/plugins/discover/public/utils/convert_value_to_string.ts
@@ -17,6 +17,8 @@ interface ConvertedResult {
   withFormula: boolean;
 }
 
+const separator = ',';
+
 export const convertValueToString = ({
   rowIndex,
   rows,
@@ -77,7 +79,7 @@ export const convertValueToString = ({
 
       return stringify(formattedValue, disableMultiline) || '';
     })
-    .join(', ');
+    .join(`${separator} `);
 
   return {
     formattedString: formatted,
@@ -97,7 +99,11 @@ const stringify = (val: object | string, disableMultiline: boolean) => {
   return disableMultiline ? JSON.stringify(val) : JSON.stringify(val, null, 2);
 };
 
-const escapeValueFn = createEscapeValue(true, true);
+const escapeValueFn = createEscapeValue({
+  separator,
+  quoteValues: true,
+  escapeFormulaValues: true,
+});
 
 const escapeFormattedValue = (formattedValue: string): string => {
   return escapeValueFn(formattedValue);

From 46de6b0206fee6b3af6d618eb20146d6e1220eda Mon Sep 17 00:00:00 2001
From: dej611 <dej611@gmail.com>
Date: Fri, 28 Apr 2023 10:11:29 +0200
Subject: [PATCH 3/4] :ok_hand: Improved checks

---
 src/plugins/data/common/exports/constants.ts  |  2 +
 .../data/common/exports/escape_value.test.ts  | 42 ++++++++++++++++++-
 .../data/common/exports/escape_value.ts       | 16 +++----
 3 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/src/plugins/data/common/exports/constants.ts b/src/plugins/data/common/exports/constants.ts
index 9c256d4d3ea97..94b0cad9dc7fc 100644
--- a/src/plugins/data/common/exports/constants.ts
+++ b/src/plugins/data/common/exports/constants.ts
@@ -9,3 +9,5 @@
 export const CSV_FORMULA_CHARS = ['=', '+', '-', '@'];
 export const nonAlphaNumRE = /[^a-zA-Z0-9]/;
 export const allDoubleQuoteRE = /"/g;
+// this is a non-exhaustive list of delimiters that require to be quoted
+export const commonQuotedDelimiters = new Set([',', ';', '\t', ' ', '|']);
diff --git a/src/plugins/data/common/exports/escape_value.test.ts b/src/plugins/data/common/exports/escape_value.test.ts
index c8645e49b1d3e..6388872f79503 100644
--- a/src/plugins/data/common/exports/escape_value.test.ts
+++ b/src/plugins/data/common/exports/escape_value.test.ts
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-import expect from '@kbn/expect';
+import expect from '@kbn/expect/expect';
 import { createEscapeValue } from './escape_value';
 
 describe('escapeValue', function () {
@@ -121,5 +121,45 @@ describe('escapeValue', function () {
       });
       expect(escapeValue('a;b')).to.be('a;b');
     });
+
+    it.each([', ', ' , ', ' ,'])(
+      'should handle also delimiters that contains white spaces "%p"',
+      (separator) => {
+        const escapeValue = createEscapeValue({
+          separator,
+          quoteValues: true,
+          escapeFormulaValues: false,
+        });
+        const nonStringValue = {
+          toString() {
+            return `a${separator}b`;
+          },
+        };
+        expect(escapeValue(nonStringValue)).to.be(`"a${separator}b"`);
+      }
+    );
+
+    it('should handle also non-string values (array)', () => {
+      const escapeValue = createEscapeValue({
+        separator: ',',
+        quoteValues: true,
+        escapeFormulaValues: true,
+      });
+      expect(escapeValue(['a', 'b'])).to.be('"a,b"');
+    });
+
+    it('should not quote non-string values, even if escapable, when separator is not in the quoted delimiters list', () => {
+      const escapeValue = createEscapeValue({
+        separator: ':',
+        quoteValues: true,
+        escapeFormulaValues: true,
+      });
+      const nonStringValue = {
+        toString() {
+          return 'a:b';
+        },
+      };
+      expect(escapeValue(nonStringValue)).to.be('a:b');
+    });
   });
 });
diff --git a/src/plugins/data/common/exports/escape_value.ts b/src/plugins/data/common/exports/escape_value.ts
index ab60ab54b11ea..0b036f2dae496 100644
--- a/src/plugins/data/common/exports/escape_value.ts
+++ b/src/plugins/data/common/exports/escape_value.ts
@@ -5,11 +5,18 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
-import { allDoubleQuoteRE, nonAlphaNumRE } from './constants';
+import { allDoubleQuoteRE, commonQuotedDelimiters, nonAlphaNumRE } from './constants';
 import { cellHasFormulas } from './formula_checks';
 
 type RawValue = string | object | null | undefined;
 
+// string with the delimiter/separator already inside need to be wrapped in quotes
+// i.e. string with delimiter char in it like free text or some number formatting (1143 => 1,143)
+function shouldBeQuoted(value: string, delimiter: string) {
+  const trimmedSeparator = delimiter.trim();
+  return value.includes(trimmedSeparator) && commonQuotedDelimiters.has(trimmedSeparator);
+}
+
 /**
  * Create a function that will escape CSV values like "=", "@" and "+" with a
  * "'". This will also place CSV values in "" if contain non-alphanumeric chars.
@@ -37,16 +44,11 @@ export function createEscapeValue({
         if (nonAlphaNumRE.test(formulasEscaped)) {
           return `"${formulasEscaped.replace(allDoubleQuoteRE, '""')}"`;
         }
-        // string with the separator already inside need to be wrapped in quotes
-        // i.e. string with csvSeparator char in it like free text or some number formatting (1143 => 1,143)
-        if (val.includes(separator)) {
-          return `"${val}"`;
-        }
       }
     }
     // raw multi-terms are stringified as T1,T2,T3 so check if the final value contains the
     // csv separator before returning (usually for raw values)
     const stringVal = val == null ? '' : val.toString();
-    return quoteValues && stringVal.includes(separator) ? `"${stringVal}"` : stringVal;
+    return quoteValues && shouldBeQuoted(stringVal, separator) ? `"${stringVal}"` : stringVal;
   };
 }

From 764ec1f4bf7e3492afcd89e8fccf488a6e4ebd8f Mon Sep 17 00:00:00 2001
From: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Date: Fri, 28 Apr 2023 08:16:20 +0000
Subject: [PATCH 4/4] [CI] Auto-commit changed files from 'node
 scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

---
 src/plugins/data/common/exports/escape_value.test.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/plugins/data/common/exports/escape_value.test.ts b/src/plugins/data/common/exports/escape_value.test.ts
index 6388872f79503..08247e0cd98a9 100644
--- a/src/plugins/data/common/exports/escape_value.test.ts
+++ b/src/plugins/data/common/exports/escape_value.test.ts
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-import expect from '@kbn/expect/expect';
+import expect from '@kbn/expect';
 import { createEscapeValue } from './escape_value';
 
 describe('escapeValue', function () {