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

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

merged 6 commits into from
Apr 20, 2020

Conversation

joelgriffith
Copy link
Contributor

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

@joelgriffith joelgriffith added (Deprecated) 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 joelgriffith requested review from tsullivan and a team April 15, 2020 20:42
@@ -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 :)

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

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:

image

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?

@@ -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

@@ -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!

@joelgriffith joelgriffith requested a review from a team as a code owner April 16, 2020 22:22
@joelgriffith
Copy link
Contributor Author

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 legrego self-requested a review April 16, 2020 23:00
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

@joelgriffith
Copy link
Contributor Author

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

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @joelgriffith!

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

docker config flag change LGTM

@joelgriffith
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@joelgriffith joelgriffith merged commit 86922e8 into elastic:master Apr 20, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 21, 2020
* 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)
  ...
jloleysens added a commit that referenced this pull request Apr 21, 2020
…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)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 21, 2020
…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
joelgriffith pushed a commit that referenced this pull request Apr 21, 2020
* 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 <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 22, 2020
…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)
  ...
@legrego legrego mentioned this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants