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

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

Merged
merged 6 commits into from
Apr 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions x-pack/legacy/plugins/reporting/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ['=', '+', '-', '@'];
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about consolidating this with the existing set of characters defined here, and in general consolidating that logic so we check for formulas in a consistent manner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++, will consolidate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidated!


export const WHITELISTED_JOB_CONTENT_TYPES = [
'application/json',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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' } }],
Expand All @@ -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({
Expand Down Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
};
};
};
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;
* 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));
Original file line number Diff line number Diff line change
Expand Up @@ -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));
};
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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"`);
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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, '""')}"`;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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: [],
};
}

Expand Down Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like an "Error" condition to me, but moreso an informational message

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can probably just remove that second line since we have more information in the Job details. We have a task to come back and consolidate all of these warning messages since we now have a method of sending generic messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll follow up with this in another PR

})
);
}

return {
content: builder.getString(),
csvContainsFormulas,
csvContainsFormulas: csvContainsFormulas && !settings.escapeFormulaValues,
maxSizeReached,
size,
warnings,
};
};
}
2 changes: 2 additions & 0 deletions x-pack/legacy/plugins/reporting/export_types/csv/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export interface SavedSearchGeneratorResult {
size: number;
maxSizeReached: boolean;
csvContainsFormulas?: boolean;
warnings: string[];
}

export interface CsvResultFromSearch {
Expand All @@ -109,5 +110,6 @@ export interface GenerateCsvParams {
maxSizeBytes: number;
scroll: ScrollConfig;
checkForFormulas?: boolean;
escapeFormulaValues: boolean;
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
};
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/reporting/server/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ const CaptureSchema = schema.object({

const CsvSchema = schema.object({
checkForFormulas: schema.boolean({ defaultValue: true }),
escapeFormulaValues: schema.boolean({ defaultValue: false }),
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be added to kibana-docker and our public documentation as well?

This is likely something we'll need to whitelist on Cloud as well

Copy link
Member

Choose a reason for hiding this comment

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

👍

Looks like the kibana-docker whitelisted settings are out of date for Reporting. I will file and issue and get those synced up.

For Cloud reference, here is a template PR for updating the whitelist for cloud: https://github.com/elastic/cloud/pull/55389

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Tim! I'll work on whitelisting this in cloud

Copy link
Member

Choose a reason for hiding this comment

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

related PR, since the docker stuff was out of date: #63766

enablePanelActionDownload: schema.boolean({ defaultValue: true }),
maxSizeBytes: schema.number({
defaultValue: 1024 * 1024 * 10, // 10MB
Expand Down