[Reporting] Config flag to escape formula CSV values (#63645) (#63987)

* Adds a new xpack.reporting.csv.escapeFormulaValues boolean to auto-escape potentially bad cells * Treat csvs with formulas differently than those that aren't escaped Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
elastic · Apr 21, 2020 · 58aa4da · 58aa4da
1 parent 90cd648
commit 58aa4da
Show file tree

Hide file tree

Showing 12 changed files with 152 additions and 13 deletions.
diff --git a/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker b/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker
@@ -195,6 +195,7 @@ kibana_vars=(
     xpack.reporting.capture.viewport.width
     xpack.reporting.capture.zoom
     xpack.reporting.csv.checkForFormulas
+    xpack.reporting.csv.escapeFormulaValues
     xpack.reporting.csv.enablePanelActionDownload
     xpack.reporting.csv.maxSizeBytes
     xpack.reporting.csv.scroll.duration

diff --git a/x-pack/legacy/plugins/reporting/common/constants.ts b/x-pack/legacy/plugins/reporting/common/constants.ts
@@ -20,6 +20,7 @@ export const API_GENERATE_IMMEDIATE = `${API_BASE_URL_V1}/generate/immediate/csv
 export const CONTENT_TYPE_CSV = 'text/csv';
 export const CSV_REPORTING_ACTION = 'downloadCsvReport';
 export const CSV_BOM_CHARS = '\ufeff';
+export const CSV_FORMULA_CHARS = ['=', '+', '-', '@'];
 
 export const WHITELISTED_JOB_CONTENT_TYPES = [
   'application/json',

diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.test.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.test.ts
@@ -300,7 +300,7 @@ describe('CSV Execute Job', function() {
     });
   });
 
-  describe('Cells with formula values', () => {
+  describe('Warning when cells have formulas', () => {
     it('returns `csv_contains_formulas` when cells contain formulas', async function() {
       configGetStub.withArgs('csv', 'checkForFormulas').returns(true);
       callAsCurrentUserStub.onFirstCall().returns({
@@ -353,6 +353,7 @@ describe('CSV Execute Job', function() {
 
     it('returns no warnings when cells have no formulas', async function() {
       configGetStub.withArgs('csv', 'checkForFormulas').returns(true);
+      configGetStub.withArgs('csv', 'escapeFormulaValues').returns(false);
       callAsCurrentUserStub.onFirstCall().returns({
         hits: {
           hits: [{ _source: { one: 'foo', two: 'bar' } }],
@@ -376,6 +377,33 @@ describe('CSV Execute Job', function() {
       expect(csvContainsFormulas).toEqual(false);
     });
 
+    it('returns no warnings when cells have formulas but are escaped', async function() {
+      configGetStub.withArgs('csv', 'checkForFormulas').returns(true);
+      configGetStub.withArgs('csv', 'escapeFormulaValues').returns(true);
+      callAsCurrentUserStub.onFirstCall().returns({
+        hits: {
+          hits: [{ _source: { '=SUM(A1:A2)': 'foo', two: 'bar' } }],
+        },
+        _scroll_id: 'scrollId',
+      });
+
+      const executeJob = await executeJobFactory(mockReportingPlugin, mockLogger);
+      const jobParams = getJobDocPayload({
+        headers: encryptedHeaders,
+        fields: ['=SUM(A1:A2)', 'two'],
+        conflictedTypesFields: [],
+        searchRequest: { index: null, body: null },
+      });
+
+      const { csv_contains_formulas: csvContainsFormulas } = await executeJob(
+        'job123',
+        jobParams,
+        cancellationToken
+      );
+
+      expect(csvContainsFormulas).toEqual(false);
+    });
+
     it('returns no warnings when configured not to', async () => {
       configGetStub.withArgs('csv', 'checkForFormulas').returns(false);
       callAsCurrentUserStub.onFirstCall().returns({
@@ -446,6 +474,50 @@ describe('CSV Execute Job', function() {
     });
   });
 
+  describe('Escaping cells with formulas', () => {
+    it('escapes values with formulas', async () => {
+      configGetStub.withArgs('csv', 'escapeFormulaValues').returns(true);
+      callAsCurrentUserStub.onFirstCall().returns({
+        hits: {
+          hits: [{ _source: { one: `=cmd|' /C calc'!A0`, two: 'bar' } }],
+        },
+        _scroll_id: 'scrollId',
+      });
+
+      const executeJob = await executeJobFactory(mockReportingPlugin, mockLogger);
+      const jobParams = getJobDocPayload({
+        headers: encryptedHeaders,
+        fields: ['one', 'two'],
+        conflictedTypesFields: [],
+        searchRequest: { index: null, body: null },
+      });
+      const { content } = await executeJob('job123', jobParams, cancellationToken);
+
+      expect(content).toEqual("one,two\n\"'=cmd|' /C calc'!A0\",bar\n");
+    });
+
+    it('does not escapes values with formulas', async () => {
+      configGetStub.withArgs('csv', 'escapeFormulaValues').returns(false);
+      callAsCurrentUserStub.onFirstCall().returns({
+        hits: {
+          hits: [{ _source: { one: `=cmd|' /C calc'!A0`, two: 'bar' } }],
+        },
+        _scroll_id: 'scrollId',
+      });
+
+      const executeJob = await executeJobFactory(mockReportingPlugin, mockLogger);
+      const jobParams = getJobDocPayload({
+        headers: encryptedHeaders,
+        fields: ['one', 'two'],
+        conflictedTypesFields: [],
+        searchRequest: { index: null, body: null },
+      });
+      const { content } = await executeJob('job123', jobParams, cancellationToken);
+
+      expect(content).toEqual('one,two\n"=cmd|\' /C calc\'!A0",bar\n');
+    });
+  });
+
   describe('Elasticsearch call errors', function() {
     it('should reject Promise if search call errors out', async function() {
       callAsCurrentUserStub.rejects(new Error());

diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.ts
@@ -123,7 +123,7 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
     const generateCsv = createGenerateCsv(jobLogger);
     const bom = config.get('csv', 'useByteOrderMarkEncoding') ? CSV_BOM_CHARS : '';
 
-    const { content, maxSizeReached, size, csvContainsFormulas } = await generateCsv({
+    const { content, maxSizeReached, size, csvContainsFormulas, warnings } = await generateCsv({
       searchRequest,
       fields,
       metaFields,
@@ -136,15 +136,18 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
         checkForFormulas: config.get('csv', 'checkForFormulas'),
         maxSizeBytes: config.get('csv', 'maxSizeBytes'),
         scroll: config.get('csv', 'scroll'),
+        escapeFormulaValues: config.get('csv', 'escapeFormulaValues'),
       },
     });
 
+    // @TODO: Consolidate these one-off warnings into the warnings array (max-size reached and csv contains formulas)
     return {
       content_type: 'text/csv',
       content: bom + content,
       max_size_reached: maxSizeReached,
       size,
       csv_contains_formulas: csvContainsFormulas,
+      warnings,
     };
   };
 };
diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/cell_has_formula.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/cell_has_formula.ts
@@ -0,0 +1,11 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { startsWith } from 'lodash';
+import { CSV_FORMULA_CHARS } from '../../../../common/constants';
+
+export const cellHasFormulas = (val: string) =>
+  CSV_FORMULA_CHARS.some(formulaChar => startsWith(val, formulaChar));
diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/check_cells_for_formulas.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/check_cells_for_formulas.ts
@@ -5,16 +5,15 @@
  */
 
 import * as _ from 'lodash';
-
-const formulaValues = ['=', '+', '-', '@'];
+import { cellHasFormulas } from './cell_has_formula';
 
 interface IFlattened {
   [header: string]: string;
 }
 
 export const checkIfRowsHaveFormulas = (flattened: IFlattened, fields: string[]) => {
   const pruned = _.pick(flattened, fields);
-  const csvValues = [..._.keys(pruned), ...(_.values(pruned) as string[])];
+  const cells = [..._.keys(pruned), ...(_.values(pruned) as string[])];
 
-  return _.some(csvValues, cell => _.some(formulaValues, char => _.startsWith(cell, char)));
+  return _.some(cells, cell => cellHasFormulas(cell));
 };
diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/escape_value.test.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/escape_value.test.ts
@@ -11,7 +11,7 @@ describe('escapeValue', function() {
   describe('quoteValues is true', function() {
     let escapeValue: (val: string) => string;
     beforeEach(function() {
-      escapeValue = createEscapeValue(true);
+      escapeValue = createEscapeValue(true, false);
     });
 
     it('should escape value with spaces', function() {
@@ -46,12 +46,42 @@ describe('escapeValue', function() {
   describe('quoteValues is false', function() {
     let escapeValue: (val: string) => string;
     beforeEach(function() {
-      escapeValue = createEscapeValue(false);
+      escapeValue = createEscapeValue(false, false);
     });
 
     it('should return the value unescaped', function() {
       const value = '"foo, bar & baz-qux"';
       expect(escapeValue(value)).to.be(value);
     });
   });
+
+  describe('escapeValues', () => {
+    describe('when true', () => {
+      let escapeValue: (val: string) => string;
+      beforeEach(function() {
+        escapeValue = createEscapeValue(true, true);
+      });
+
+      ['@', '+', '-', '='].forEach(badChar => {
+        it(`should escape ${badChar} injection values`, function() {
+          expect(escapeValue(`${badChar}cmd|' /C calc'!A0`)).to.be(
+            `"'${badChar}cmd|' /C calc'!A0"`
+          );
+        });
+      });
+    });
+
+    describe('when false', () => {
+      let escapeValue: (val: string) => string;
+      beforeEach(function() {
+        escapeValue = createEscapeValue(true, false);
+      });
+
+      ['@', '+', '-', '='].forEach(badChar => {
+        it(`should not escape ${badChar} injection values`, function() {
+          expect(escapeValue(`${badChar}cmd|' /C calc'!A0`)).to.be(`"${badChar}cmd|' /C calc'!A0"`);
+        });
+      });
+    });
+  });
 });
diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/escape_value.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/escape_value.ts
@@ -5,15 +5,20 @@
  */
 
 import { RawValue } from './types';
+import { cellHasFormulas } from './cell_has_formula';
 
 const nonAlphaNumRE = /[^a-zA-Z0-9]/;
 const allDoubleQuoteRE = /"/g;
 
-export function createEscapeValue(quoteValues: boolean): (val: RawValue) => string {
+export function createEscapeValue(
+  quoteValues: boolean,
+  escapeFormulas: boolean
+): (val: RawValue) => string {
   return function escapeValue(val: RawValue) {
     if (val && typeof val === 'string') {
-      if (quoteValues && nonAlphaNumRE.test(val)) {
-        return `"${val.replace(allDoubleQuoteRE, '""')}"`;
+      const formulasEscaped = escapeFormulas && cellHasFormulas(val) ? "'" + val : val;
+      if (quoteValues && nonAlphaNumRE.test(formulasEscaped)) {
+        return `"${formulasEscaped.replace(allDoubleQuoteRE, '""')}"`;
       }
     }
 

diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/generate_csv.ts b/x-pack/legacy/plugins/reporting/export_types/csv/server/lib/generate_csv.ts
@@ -4,6 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
+import { i18n } from '@kbn/i18n';
 import { Logger } from '../../../../types';
 import { GenerateCsvParams, SavedSearchGeneratorResult } from '../../types';
 import { createFlattenHit } from './flatten_hit';
@@ -26,14 +27,17 @@ export function createGenerateCsv(logger: Logger) {
     cancellationToken,
     settings,
   }: GenerateCsvParams): Promise<SavedSearchGeneratorResult> {
-    const escapeValue = createEscapeValue(settings.quoteValues);
+    const escapeValue = createEscapeValue(settings.quoteValues, settings.escapeFormulaValues);
     const builder = new MaxSizeStringBuilder(settings.maxSizeBytes);
     const header = `${fields.map(escapeValue).join(settings.separator)}\n`;
+    const warnings: string[] = [];
+
     if (!builder.tryAppend(header)) {
       return {
         size: 0,
         content: '',
         maxSizeReached: true,
+        warnings: [],
       };
     }
 
@@ -82,11 +86,20 @@ export function createGenerateCsv(logger: Logger) {
     const size = builder.getSizeInBytes();
     logger.debug(`finished generating, total size in bytes: ${size}`);
 
+    if (csvContainsFormulas && settings.escapeFormulaValues) {
+      warnings.push(
+        i18n.translate('xpack.reporting.exportTypes.csv.generateCsv.escapedFormulaValues', {
+          defaultMessage: 'CSV may contain formulas whose values have been escaped',
+        })
+      );
+    }
+
     return {
       content: builder.getString(),
-      csvContainsFormulas,
+      csvContainsFormulas: csvContainsFormulas && !settings.escapeFormulaValues,
       maxSizeReached,
       size,
+      warnings,
     };
   };
 }
diff --git a/x-pack/legacy/plugins/reporting/export_types/csv/types.d.ts b/x-pack/legacy/plugins/reporting/export_types/csv/types.d.ts
@@ -87,6 +87,7 @@ export interface SavedSearchGeneratorResult {
   size: number;
   maxSizeReached: boolean;
   csvContainsFormulas?: boolean;
+  warnings: string[];
 }
 
 export interface CsvResultFromSearch {
@@ -109,5 +110,6 @@ export interface GenerateCsvParams {
     maxSizeBytes: number;
     scroll: ScrollConfig;
     checkForFormulas?: boolean;
+    escapeFormulaValues: boolean;
   };
 }
diff --git a/...acy/plugins/reporting/export_types/csv_from_savedobject/server/lib/generate_csv_search.ts b/...acy/plugins/reporting/export_types/csv_from_savedobject/server/lib/generate_csv_search.ts
@@ -173,6 +173,7 @@ export async function generateCsvSearch(
       ...uiSettings,
       maxSizeBytes: config.get('csv', 'maxSizeBytes'),
       scroll: config.get('csv', 'scroll'),
+      escapeFormulaValues: config.get('csv', 'escapeFormulaValues'),
       timezone,
     },
   };

diff --git a/x-pack/plugins/reporting/server/config/schema.ts b/x-pack/plugins/reporting/server/config/schema.ts
@@ -114,6 +114,7 @@ const CaptureSchema = schema.object({
 
 const CsvSchema = schema.object({
   checkForFormulas: schema.boolean({ defaultValue: true }),
+  escapeFormulaValues: schema.boolean({ defaultValue: false }),
   enablePanelActionDownload: schema.boolean({ defaultValue: true }),
   maxSizeBytes: schema.number({
     defaultValue: 1024 * 1024 * 10, // 10MB