[8.8] [Lens] Quote csv values when contain separator char (#155905)

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 {

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', ' ', '|']);

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({
+        separator: ',',
+        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({
+        separator: ',',
+        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({
+          separator: ',',
+          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({
+          separator: ',',
+          quoteValues: true,
+          escapeFormulaValues: false,
+        });
       });
 
       ['@', '+', '-', '='].forEach((badChar) => {
@@ -86,4 +102,64 @@ describe('escapeValue', function () {
       });
     });
   });
+
+  describe('csvSeparator', () => {
+    it('should escape when text contains the separator char with quotes enabled', () => {
+      const escapeValue = createEscapeValue({
+        separator: ';',
+        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({
+        separator: ';',
+        quoteValues: false,
+        escapeFormulaValues: false,
+      });
+      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');
+    });
+  });
 });

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.
@@ -21,17 +28,27 @@ 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({
+  separator,
+  quoteValues,
+  escapeFormulaValues,
+}: {
+  separator: 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, '""')}"`;
+        }
       }
     }
-    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 && shouldBeQuoted(stringVal, separator) ? `"${stringVal}"` : stringVal;
   };
 }

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');
+  });
 });

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(quoteValues, escapeFormulaValues);
+  const escapeValues = createEscapeValue({
+    separator: csvSeparator,
+    quoteValues,
+    escapeFormulaValues,
+  });
   // Build the header row by its names
   const header = columns.map((col) => escapeValues(col.name));
 

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);