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 1 commit
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
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
1 change: 1 addition & 0 deletions x-pack/legacy/plugins/reporting/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export async function config(Joi: any) {
useByteOrderMarkEncoding: Joi.boolean().default(false),
checkForFormulas: Joi.boolean().default(true),
enablePanelActionDownload: Joi.boolean().default(true),
escapeFormulaValues: Joi.boolean().default(false),
maxSizeBytes: Joi.number()
.integer()
.default(1024 * 1024 * 10), // bytes in a kB * kB in a mB * 10
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,50 @@ describe('CSV Execute Job', function() {
});
});

describe('Formula values', () => {
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 @@ -136,6 +136,7 @@ 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'),
},
});

Expand Down
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,23 @@
*/

import { RawValue } from './types';
import { CSV_FORMULA_CHARS } from '../../../../common/constants';

const nonAlphaNumRE = /[^a-zA-Z0-9]/;
const allDoubleQuoteRE = /"/g;

export function createEscapeValue(quoteValues: boolean): (val: RawValue) => string {
const valHasFormulas = (val: string) =>
CSV_FORMULA_CHARS.some(formulaChar => val.startsWith(formulaChar));

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 && valHasFormulas(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 @@ -26,7 +26,7 @@ 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`;
if (!builder.tryAppend(header)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,5 +109,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/legacy/plugins/reporting/server/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export interface ReportingConfigType {
checkForFormulas: boolean;
maxSizeBytes: number;
useByteOrderMarkEncoding: boolean;
escapeFormulaValues: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

The changes here and in the config.ts file will conflict with #62500 and I'm mere minutes away from merging that. Sorry! But I think we should merge the older PR first.

When those conflicts are resolved, the new setting added here will be defined in the New Platform Reporting plugin :)

};
encryptionKey: string;
kibanaServer: any;
Expand Down