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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Apr 20, 2020
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ab046e
Adds a new xpack.reporting.csv.escapeFormulaValues boolean to auto-es…
Apr 15, 2020
8ac2a32
Merge remote-tracking branch 'upstream/master' into reporting/securit…
Apr 15, 2020
8ca9b05
Merge remote-tracking branch 'upstream/master' into reporting/securit…
Apr 15, 2020
b49c39c
Merge remote-tracking branch 'upstream/master' into reporting/securit…
Apr 16, 2020
c45b8a6
Treat csvs with formulas differently than those that aren't escaped
Apr 16, 2020
39bf9f3
Merge branch 'master' into reporting/security-escape-formulas
elasticmachine Apr 20, 2020
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
View file
Open in desktop
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
44 changes: 44 additions & 0 deletions x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.test.ts
View file
Open in desktop
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
1 change: 1 addition & 0 deletions x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.ts
View file
Open in desktop
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
34 changes: 32 additions & 2 deletions x-pack/legacy/plugins/reporting/export_types/csv/server/lib/escape_value.test.ts
View file
Open in desktop
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"`);
});
});
});
});
});
14 changes: 11 additions & 3 deletions x-pack/legacy/plugins/reporting/export_types/csv/server/lib/escape_value.ts
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/reporting/export_types/csv/server/lib/generate_csv.ts
View file
Open in desktop
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
1 change: 1 addition & 0 deletions x-pack/legacy/plugins/reporting/export_types/csv/types.d.ts
View file
Open in desktop
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;
};
}
1 change: 1 addition & 0 deletions ...acy/plugins/reporting/export_types/csv_from_savedobject/server/lib/generate_csv_search.ts
View file
Open in desktop
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
View file
Open in desktop
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