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

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

Merged
merged 1 commit into from
Apr 21, 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 = ['=', '+', '-', '@'];

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

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 }),
enablePanelActionDownload: schema.boolean({ defaultValue: true }),
maxSizeBytes: schema.number({
defaultValue: 1024 * 1024 * 10, // 10MB
Expand Down