[Reporting] Config flag to escape formula CSV values by joelgriffith · Pull Request #63645 · elastic/kibana

joelgriffith · 2020-04-15T20:42:04Z

Adds a new xpack.reporting.csv.escapeFormulaValues boolean to have kibana append ' to cells that are potentially formulas (https://owasp.org/www-community/attacks/CSV_Injection).

This includes CSV exported by Discover (via Share), and by the Table Aggregation vis in Dashboard.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…cape potentially bad cells

tsullivan · 2020-04-15T20:55:05Z

x-pack/legacy/plugins/reporting/server/config/index.ts

    checkForFormulas: boolean;
    maxSizeBytes: number;
    useByteOrderMarkEncoding: boolean;
+    escapeFormulaValues: boolean;


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 :)

…y-escape-formulas

legrego

Functionally this seems to be working well, just a couple of questions/nits.

Now that we can escape formulas, should we also update the toast warning?
If I export a CSV with dangerous characters, and I've chosen to escape formulas, then I still see this warning:

I know that nobody reads these (heck, I didn't the first time), but maybe another sentence explaining that the report detected possible formulas and disabled/escaped them?

legrego · 2020-04-16T13:50:03Z

x-pack/plugins/reporting/server/config/schema.ts


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


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

👍

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

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

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

legrego · 2020-04-16T13:56:21Z

x-pack/legacy/plugins/reporting/common/constants.ts

 export const CONTENT_TYPE_CSV = 'text/csv';
 export const CSV_REPORTING_ACTION = 'downloadCsvReport';
 export const CSV_BOM_CHARS = '\ufeff';
+export const CSV_FORMULA_CHARS = ['=', '+', '-', '@'];


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?

++, will consolidate

Consolidated!

…y-escape-formulas

joelgriffith · 2020-04-16T22:57:15Z

So, CSVs now exports with formula's escaped now complete with warnings. Toasts show up as successful, but show warnings when in the report list saying that the csv had potential formula's that were escaped.

legrego · 2020-04-17T12:31:07Z

x-pack/legacy/plugins/reporting/export_types/csv/server/lib/generate_csv.ts

+    if (csvContainsFormulas && settings.escapeFormulaValues) {
+      warnings.push(
+        i18n.translate('xpack.reporting.exportTypes.csv.generateCsv.escapedFormulaValues', {
+          defaultMessage: 'CSV may contain formulas whose values have been escaped',


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

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.

We'll follow up with this in another PR

joelgriffith · 2020-04-17T15:49:29Z

@elastic/kibana-operations @legrego @tsullivan this is ready for final review

legrego

LGTM - thanks @joelgriffith!

spalger

docker config flag change LGTM

joelgriffith · 2020-04-20T15:58:55Z

@elasticmachine merge upstream

kibanamachine · 2020-04-20T17:56:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 39bf9f3

History

💚 Build #41414 succeeded c45b8a6
💚 Build #41107 succeeded 8ca9b05
💔 Build #41091 failed 2ab046e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* master: (30 commits) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) [TSVB] Use default Kibana palette for split series (elastic#62241) Ingest overview page (elastic#63214) ...

…t-node-pipelines * 'master' of github.com:elastic/kibana: (32 commits) Move authz lib out of snapshot restore (#63947) Migrate vis_type_table to kibana/new platform (#63105) Enable include/exclude in Terms agg for numeric fields (#59425) follow conventions for saved object definitions (#63571) [Docs]Adds saved object key setting to load balancing kib instances (#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (#63838) Fix page layouts, clean up unused code (#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (#63859) [Maps] Do not fetch geo_shape from docvalues (#63997) Vega doc changes (#63889) [Metrics UI] Reorganize file layout for Metrics UI (#60049) Add sub-menus to Resolver node (for 75% zoom) (#63476) [FTR] Add test suite metrics tracking/output (#62515) [ML] Fixing single metric viewer page padding (#63839) [Discover] Allow user to generate a report after saving a modified saved search (#63623) [Reporting] Config flag to escape formula CSV values (#63645) [Metrics UI] Remove remaining field filtering (#63398) [Maps] fix date labels (#63909) [TSVB] Use default Kibana palette for split series (#62241) ...

…bana into ingest-node-pipelines/privileges * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits) Move authz lib out of snapshot restore (elastic#63947) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [Ingest pipelines] Delete pipeline (elastic#63635) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) ... # Conflicts: # x-pack/legacy/plugins/uptime/public/components/monitor/ml/index.ts # x-pack/legacy/plugins/uptime/public/components/overview/index.ts # x-pack/plugins/ingest_pipelines/public/application/index.tsx # x-pack/plugins/ingest_pipelines/server/routes/api/index.ts # x-pack/plugins/ingest_pipelines/server/routes/index.ts

* Adds a new xpack.reporting.csv.escapeFormulaValues boolean to auto-escape potentially bad cells * Treat csvs with formulas differently than those that aren't escaped Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…bana into pipeline-editor-part-mvp-2 * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits) [Ingest Node Pipelines] Clone Pipeline (elastic#64049) Move authz lib out of snapshot restore (elastic#63947) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [Ingest pipelines] Delete pipeline (elastic#63635) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) ...

Adds a new xpack.reporting.csv.escapeFormulaValues boolean to auto-es…

2ab046e

…cape potentially bad cells

joelgriffith added zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement v7.8.0 v8.0.0 labels Apr 15, 2020

joelgriffith requested review from a team and tsullivan April 15, 2020 20:42

alexfrancoeur mentioned this pull request Apr 15, 2020

Data Table - CSV export formulas warning #56081

Closed

tsullivan reviewed Apr 15, 2020

View reviewed changes

Joel Griffith added 2 commits April 15, 2020 14:06

Merge remote-tracking branch 'upstream/master' into reporting/securit…

8ac2a32

…y-escape-formulas

Merge remote-tracking branch 'upstream/master' into reporting/securit…

8ca9b05

…y-escape-formulas

legrego reviewed Apr 16, 2020

View reviewed changes

Joel Griffith added 2 commits April 16, 2020 12:54

Merge remote-tracking branch 'upstream/master' into reporting/securit…

b49c39c

…y-escape-formulas

Treat csvs with formulas differently than those that aren't escaped

c45b8a6

joelgriffith requested a review from a team as a code owner April 16, 2020 22:22

legrego self-requested a review April 16, 2020 23:00

legrego reviewed Apr 17, 2020

View reviewed changes

legrego approved these changes Apr 17, 2020

View reviewed changes

spalger approved these changes Apr 17, 2020

View reviewed changes

Merge branch 'master' into reporting/security-escape-formulas

39bf9f3

joelgriffith merged commit 86922e8 into elastic:master Apr 20, 2020

joelgriffith mentioned this pull request Apr 20, 2020

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

Merged

legrego mentioned this pull request Sep 21, 2020

[Lens] CSV export #74509

Closed

tsullivan mentioned this pull request Dec 8, 2021

[Reporting] "xpack.reporting.csv.escapeFormulaValues" is undocumented #120851

Closed

Conversation

joelgriffith commented Apr 15, 2020

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelgriffith commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelgriffith commented Apr 17, 2020

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

joelgriffith commented Apr 20, 2020

Uh oh!

kibanamachine commented Apr 20, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants