Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens][Visualize][Inspector][Reporting] Unified check for CSV cells for known formula characters (and value escaping more in general) #105221

Merged
merged 9 commits into from
Jul 16, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@
exporters: {
datatableToCSV: typeof datatableToCSV;
CSV_MIME_TYPE: string;
cellHasFormulas: (val: string) => boolean;
tableHasFormulas: (columns: import("../../expressions").DatatableColumn[], rows: Record<string, any>[]) => boolean;
}
```
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ describe('Export CSV action', () => {
| Record<string, { content: string; type: string }>;
expect(result).toEqual({
'Hello Kibana.csv': {
content: `First Name,Last Name${LINE_FEED_CHARACTER}Kibana,undefined${LINE_FEED_CHARACTER}`,
content: `First Name,Last Name${LINE_FEED_CHARACTER}Kibana,${LINE_FEED_CHARACTER}`,
type: 'text/plain;charset=utf-8',
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export class ExportCSVAction implements Action<ExportContext> {
csvSeparator: this.params.core.uiSettings.get('csv:separator', ','),
quoteValues: this.params.core.uiSettings.get('csv:quoteValues', true),
formatFactory,
escapeFormulaValues: false,
}),
type: exporters.CSV_MIME_TYPE,
};
Expand Down
11 changes: 11 additions & 0 deletions src/plugins/data/common/exports/constants.ts
Original file line number Diff line number Diff line change
@@ -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
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export const CSV_FORMULA_CHARS = ['=', '+', '-', '@'];
export const nonAlphaNumRE = /[^a-zA-Z0-9]/;
export const allDoubleQuoteRE = /"/g;
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import expect from '@kbn/expect';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* 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 { cellHasFormulas } from './formula_checks';

import { RawValue } from '../types';
import { cellHasFormulas } from './cell_has_formula';

const nonAlphaNumRE = /[^a-zA-Z0-9]/;
const allDoubleQuoteRE = /"/g;
type RawValue = string | object | null | undefined;

export function createEscapeValue(
quoteValues: boolean,
Expand All @@ -22,7 +21,6 @@ export function createEscapeValue(
return `"${formulasEscaped.replace(allDoubleQuoteRE, '""')}"`;
}
}

return val == null ? '' : val.toString();
};
}
13 changes: 13 additions & 0 deletions src/plugins/data/common/exports/export_csv.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ function getDefaultOptions() {
csvSeparator: ',',
quoteValues: true,
formatFactory,
escapeFormulaValues: false,
};
}

Expand Down Expand Up @@ -71,4 +72,16 @@ describe('CSV exporter', () => {
'columnOne\r\n"Formatted_""value"""\r\n'
);
});

test('should escape formulas', () => {
const datatable = getDataTable();
datatable.rows[0].col1 = '=1';
expect(
datatableToCSV(datatable, {
...getDefaultOptions(),
escapeFormulaValues: true,
formatFactory: () => ({ convert: (v: unknown) => v } as FieldFormat),
})
).toMatch('columnOne\r\n"\'=1"\r\n');
});
});
26 changes: 6 additions & 20 deletions src/plugins/data/common/exports/export_csv.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,40 +10,26 @@

import { FormatFactory } from 'src/plugins/data/common/field_formats/utils';
import { Datatable } from 'src/plugins/expressions';
import { createEscapeValue } from './escape_value';

export const LINE_FEED_CHARACTER = '\r\n';
const nonAlphaNumRE = /[^a-zA-Z0-9]/;
const allDoubleQuoteRE = /"/g;
export const CSV_MIME_TYPE = 'text/plain;charset=utf-8';

// TODO: enhance this later on
function escape(val: object | string, quoteValues: boolean) {
if (val != null && typeof val === 'object') {
val = val.valueOf();
}

val = String(val);

if (quoteValues && nonAlphaNumRE.test(val)) {
val = `"${val.replace(allDoubleQuoteRE, '""')}"`;
}

return val;
}

interface CSVOptions {
csvSeparator: string;
quoteValues: boolean;
escapeFormulaValues: boolean;
formatFactory: FormatFactory;
raw?: boolean;
}

export function datatableToCSV(
{ columns, rows }: Datatable,
{ csvSeparator, quoteValues, formatFactory, raw }: CSVOptions
{ csvSeparator, quoteValues, formatFactory, raw, escapeFormulaValues }: CSVOptions
) {
const escapeValues = createEscapeValue(quoteValues, escapeFormulaValues);
// Build the header row by its names
const header = columns.map((col) => escape(col.name, quoteValues));
const header = columns.map((col) => escapeValues(col.name));

const formatters = columns.reduce<Record<string, ReturnType<FormatFactory>>>(
(memo, { id, meta }) => {
Expand All @@ -56,7 +42,7 @@ export function datatableToCSV(
// Convert the array of row objects to an array of row arrays
const csvRows = rows.map((row) => {
return columns.map((column) =>
escape(raw ? row[column.id] : formatters[column.id].convert(row[column.id]), quoteValues)
escapeValues(raw ? row[column.id] : formatters[column.id].convert(row[column.id]))
);
});

Expand Down
21 changes: 21 additions & 0 deletions src/plugins/data/common/exports/formula_checks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { startsWith } from 'lodash';
import { Datatable } from 'src/plugins/expressions';
import { CSV_FORMULA_CHARS } from './constants';

export const cellHasFormulas = (val: string) =>
CSV_FORMULA_CHARS.some((formulaChar) => startsWith(val, formulaChar));

export const tableHasFormulas = (columns: Datatable['columns'], rows: Datatable['rows']) => {
return (
columns.some(({ name }) => cellHasFormulas(name)) ||
rows.some((row) => columns.some(({ id }) => cellHasFormulas(row[id])))
);
};
3 changes: 3 additions & 0 deletions src/plugins/data/common/exports/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@
*/

export { datatableToCSV, CSV_MIME_TYPE } from './export_csv';
export { createEscapeValue } from './escape_value';
export { CSV_FORMULA_CHARS } from './constants';
export { cellHasFormulas, tableHasFormulas } from './formula_checks';
4 changes: 3 additions & 1 deletion src/plugins/data/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,12 @@ export {
* Exporters (CSV)
*/

import { datatableToCSV, CSV_MIME_TYPE } from '../common';
import { datatableToCSV, CSV_MIME_TYPE, cellHasFormulas, tableHasFormulas } from '../common';
export const exporters = {
datatableToCSV,
CSV_MIME_TYPE,
cellHasFormulas,
tableHasFormulas,
};

/*
Expand Down
40 changes: 21 additions & 19 deletions src/plugins/data/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,8 @@ export type ExistsFilter = Filter & {
export const exporters: {
datatableToCSV: typeof datatableToCSV;
CSV_MIME_TYPE: string;
cellHasFormulas: (val: string) => boolean;
tableHasFormulas: (columns: import("../../expressions").DatatableColumn[], rows: Record<string, any>[]) => boolean;
};

// Warning: (ae-missing-release-tag) "ExpressionFunctionKibana" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down Expand Up @@ -2773,25 +2775,25 @@ export interface WaitUntilNextSessionCompletesOptions {
// src/plugins/data/public/index.ts:170:26 - (ae-forgotten-export) The symbol "TruncateFormat" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:170:26 - (ae-forgotten-export) The symbol "HistogramFormat" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:213:23 - (ae-forgotten-export) The symbol "datatableToCSV" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:238:27 - (ae-forgotten-export) The symbol "isFilterable" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:238:27 - (ae-forgotten-export) The symbol "isNestedField" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:238:27 - (ae-forgotten-export) The symbol "validateIndexPattern" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:238:27 - (ae-forgotten-export) The symbol "flattenHitWrapper" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:238:27 - (ae-forgotten-export) The symbol "formatHitProvider" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:410:20 - (ae-forgotten-export) The symbol "getResponseInspectorStats" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:410:20 - (ae-forgotten-export) The symbol "tabifyAggResponse" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:410:20 - (ae-forgotten-export) The symbol "tabifyGetColumns" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:412:1 - (ae-forgotten-export) The symbol "CidrMask" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:413:1 - (ae-forgotten-export) The symbol "dateHistogramInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:422:1 - (ae-forgotten-export) The symbol "InvalidEsCalendarIntervalError" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:423:1 - (ae-forgotten-export) The symbol "InvalidEsIntervalFormatError" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:424:1 - (ae-forgotten-export) The symbol "IpAddress" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:425:1 - (ae-forgotten-export) The symbol "isDateHistogramBucketAggConfig" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:429:1 - (ae-forgotten-export) The symbol "isValidEsInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:430:1 - (ae-forgotten-export) The symbol "isValidInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:433:1 - (ae-forgotten-export) The symbol "parseInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:434:1 - (ae-forgotten-export) The symbol "propFilter" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:437:1 - (ae-forgotten-export) The symbol "toAbsoluteDates" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:240:27 - (ae-forgotten-export) The symbol "isFilterable" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:240:27 - (ae-forgotten-export) The symbol "isNestedField" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:240:27 - (ae-forgotten-export) The symbol "validateIndexPattern" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:240:27 - (ae-forgotten-export) The symbol "flattenHitWrapper" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:240:27 - (ae-forgotten-export) The symbol "formatHitProvider" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:412:20 - (ae-forgotten-export) The symbol "getResponseInspectorStats" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:412:20 - (ae-forgotten-export) The symbol "tabifyAggResponse" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:412:20 - (ae-forgotten-export) The symbol "tabifyGetColumns" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:414:1 - (ae-forgotten-export) The symbol "CidrMask" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:415:1 - (ae-forgotten-export) The symbol "dateHistogramInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:424:1 - (ae-forgotten-export) The symbol "InvalidEsCalendarIntervalError" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:425:1 - (ae-forgotten-export) The symbol "InvalidEsIntervalFormatError" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:426:1 - (ae-forgotten-export) The symbol "IpAddress" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:427:1 - (ae-forgotten-export) The symbol "isDateHistogramBucketAggConfig" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:431:1 - (ae-forgotten-export) The symbol "isValidEsInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:432:1 - (ae-forgotten-export) The symbol "isValidInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:435:1 - (ae-forgotten-export) The symbol "parseInterval" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:436:1 - (ae-forgotten-export) The symbol "propFilter" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/index.ts:439:1 - (ae-forgotten-export) The symbol "toAbsoluteDates" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/query/state_sync/connect_to_query_state.ts:34:5 - (ae-forgotten-export) The symbol "FilterStateStore" needs to be exported by the entry point index.d.ts
// src/plugins/data/public/search/session/session_service.ts:62:5 - (ae-forgotten-export) The symbol "UrlGeneratorStateMapping" needs to be exported by the entry point index.d.ts

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ jest.mock('../../../../../share/public', () => ({
}));
jest.mock('../../../../common', () => ({
datatableToCSV: jest.fn().mockReturnValue('csv'),
tableHasFormulas: jest.fn().mockReturnValue(false),
}));

describe('Inspector Data View', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,14 @@ import PropTypes from 'prop-types';
import { FormattedMessage } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';

import { EuiButton, EuiContextMenuItem, EuiContextMenuPanel, EuiPopover } from '@elastic/eui';
import { CSV_MIME_TYPE, datatableToCSV } from '../../../../common';
import {
EuiButton,
EuiContextMenuItem,
EuiContextMenuPanel,
EuiPopover,
EuiToolTip,
} from '@elastic/eui';
import { CSV_MIME_TYPE, datatableToCSV, tableHasFormulas } from '../../../../common';
import { Datatable } from '../../../../../expressions';
import { downloadMultipleAs } from '../../../../../share/public';
import { FieldFormatsStart } from '../../../field_formats';
Expand Down Expand Up @@ -74,6 +80,7 @@ class DataDownloadOptions extends Component<DataDownloadOptionsProps, DataDownlo
quoteValues: this.props.uiSettings.get('csv:quoteValues', true),
raw: !isFormatted,
formatFactory: this.props.fieldFormats.deserialize,
escapeFormulaValues: false,
}),
type: CSV_MIME_TYPE,
};
Expand All @@ -96,6 +103,9 @@ class DataDownloadOptions extends Component<DataDownloadOptionsProps, DataDownlo
};

renderFormattedDownloads() {
const detectedFormulasInTables = this.props.datatables.some(({ columns, rows }) =>
tableHasFormulas(columns, rows)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to memoize the result of this check and only re-run it when the datatables prop has changed. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did add the memoization on this specific instance. Or perhaps you meant something more generic like on the tableHasFormulas function?

const button = (
<EuiButton iconType="arrowDown" iconSide="right" size="s" onClick={this.onTogglePopover}>
<FormattedMessage
Expand All @@ -104,6 +114,20 @@ class DataDownloadOptions extends Component<DataDownloadOptionsProps, DataDownlo
/>
</EuiButton>
);
const downloadButton = detectedFormulasInTables ? (
<EuiToolTip
position="top"
content={i18n.translate('data.inspector.table.exportButtonFormulasWarning', {
defaultMessage:
'Your CSV contains characters which spreadsheet applications can interpret as formulas',
})}
>
{button}
</EuiToolTip>
) : (
button
);

const items = [
<EuiContextMenuItem
key="csv"
Expand Down Expand Up @@ -139,7 +163,7 @@ class DataDownloadOptions extends Component<DataDownloadOptionsProps, DataDownlo
return (
<EuiPopover
id="inspectorDownloadData"
button={button}
button={downloadButton}
isOpen={this.state.isPopoverOpen}
closePopover={this.closePopover}
panelPaddingSize="none"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@
*/

import React, { memo, useState, useCallback } from 'react';
import { EuiButtonEmpty, EuiContextMenuItem, EuiContextMenuPanel, EuiPopover } from '@elastic/eui';
import {
EuiButtonEmpty,
EuiContextMenuItem,
EuiContextMenuPanel,
EuiPopover,
EuiToolTip,
} from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';

Expand Down Expand Up @@ -39,6 +45,8 @@ export const TableVisControls = memo(
services: { uiSettings },
} = useKibana<CoreStart>();

const detectedFormulasInTable = exporters.tableHasFormulas(columns, rows);

const onClickExport = useCallback(
(formatted: boolean) => {
const csvSeparator = uiSettings.get(CSV_SEPARATOR_SETTING);
Expand All @@ -55,6 +63,7 @@ export const TableVisControls = memo(
quoteValues,
formatFactory: getFormatService().deserialize,
raw: !formatted,
escapeFormulaValues: false,
}
);
downloadFileAs(`${filename || 'unsaved'}.csv`, { content, type: exporters.CSV_MIME_TYPE });
Expand Down Expand Up @@ -85,6 +94,20 @@ export const TableVisControls = memo(
</EuiButtonEmpty>
);

const downloadButton = detectedFormulasInTable ? (
<EuiToolTip
position="top"
content={i18n.translate('visTypeTable.vis.controls.exportButtonFormulasWarning', {
defaultMessage:
'Your CSV contains characters which spreadsheet applications can interpret as formulas',
})}
>
{button}
</EuiToolTip>
) : (
button
);

const items = [
<EuiContextMenuItem key="rawCsv" onClick={() => onClickExport(false)}>
<FormattedMessage id="visTypeTable.vis.controls.rawCSVButtonLabel" defaultMessage="Raw" />
Expand All @@ -100,7 +123,7 @@ export const TableVisControls = memo(
return (
<EuiPopover
id="dataTableExportData"
button={button}
button={downloadButton}
isOpen={isPopoverOpen}
closePopover={closePopover}
panelPaddingSize="none"
Expand Down
Loading