Np migration tsvb route validation #51850

TinaHeiligers · 2019-11-28T00:27:41Z

Summary

Relates to https://github.com/elastic/kibana-team/issues/166.
Adds payload validation using Joi to the TSVB post route /api/metrics/vis/data.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-11-28T00:27:43Z

Pinging @elastic/kibana-app (Team:KibanaApp)

elasticmachine · 2019-11-28T01:43:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: ffd1de4

…TSVB_route_validation

flash1293 · 2019-12-04T14:59:26Z

Jenkins, test this.

elasticmachine · 2019-12-04T16:14:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 489219a

…_route_validation

elasticmachine · 2019-12-06T12:15:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 338783c

flash1293 · 2019-12-06T12:23:47Z

I clicked through all of the tabs and played around with all of the settings, but still not sure whether all cases are covered. This should be reviewed by as many eyes as possible.

@legrego @timroes was thinking to run this validation in a non-blocking mode for a minor and report failures via telemetry to make sure we didn't forget to cover some cases. That would mean however that we wouldn't have in-depth validation for 7.6. What do you think?

TinaHeiligers · 2019-12-06T15:18:25Z

I clicked through all of the tabs and played around with all of the settings, but still not sure whether all cases are covered. This should be reviewed by as many eyes as possible.

@legrego @timroes was thinking to run this validation in a non-blocking mode for a minor and report failures via telemetry to make sure we didn't forget to cover some cases. That would mean however that we wouldn't have in-depth validation for 7.6. What do you think?

@flash1293 @timroes @legrego I was thinking that maybe we could do the 'soft' validation by allowing the default payload validation that checks for prototype pollution to run and moving the custom validator into the handler with a soft error. That way, we still guard against malicious payloads but retain the custom one for final migration efforts.

flash1293 · 2019-12-06T15:28:19Z

@TinaHeiligers What exactly do you mean by soft error? Just logging the validation failure (and maybe reporting it as I suggested above)?

legrego · 2019-12-06T15:31:57Z

src/legacy/core_plugins/vis_type_timeseries/server/routes/post_vis_schema.js

+    .items(queryObject)
+    .allow(null)
+    .required(),
+  state: Joi.object({}).required(),


I did a quick search of the code, and it looks like the only expected property here is sort:

kibana/src/legacy/core_plugins/vis_type_timeseries/server/lib/vis_data/request_processors/table/pivot.js

Line 28 in 5887526

const { sort } = req.payload.state;

Are you aware of others? If not, we should be explicit about the set of allowed keys.

Thanks @legrego , it was a huge payload to unpack and some of these items may have fallen through the cracks.

Added it to the validation.

@TinaHeiligers it was a massive payload -- thanks so much for taking care of this!

@flash1293 thanks!

TinaHeiligers · 2019-12-06T21:05:34Z

@flash1293 what I meant was the same as you suggested.

src/legacy/core_plugins/vis_type_timeseries/public/request_handler.js

alexwizp · 2019-12-19T13:37:01Z

src/legacy/core_plugins/vis_type_timeseries/server/plugin.ts

@@ -28,6 +28,8 @@ import { visDataRoutes } from './routes/vis';
 import { SearchStrategiesRegister } from './lib/search_strategies/search_strategies_register';
 // @ts-ignore
 import { getVisData } from './lib/get_vis_data';
+import { UsageCollectionSetup } from '../../../../plugins/usage_collection/server';
+import { ValidationTelemetryService } from './validation_telemetry/validation_telemetry_service';

 // TODO: Remove as CoreSetup is completed.
 export interface CustomCoreSetup {


Related to one of previous comment. Not sure that we need it

alexwizp

Please see comments above

…_route_validation

elasticmachine · 2019-12-20T14:37:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: fdd7a74

History

💚 Build #16028 succeeded f419ec5
💔 Build #15764 failed 897c947
💚 Build #15344 succeeded e6316ef
💚 Build #15212 succeeded 0f19a66
💚 Build #14251 succeeded 19b9765

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

…_route_validation

flash1293 · 2020-01-10T09:13:26Z

Updated behavior: No config flag, but extend the error message in the logs with the saved object id of the visualization that caused the error:

cc @timroes

timroes · 2020-01-10T10:51:28Z

src/plugins/vis_type_timeseries/server/validation_telemetry/validation_telemetry_service.ts

+                ignore: [404],
+              });
+              return {
+                failed_validations: get(


Optional chainging and nullish coallescing for the win :-)

timroes

Left one small code comment. Tested on Chrome Linux, checked that a modified request will show the warning and also trigger the counter to go up, checked in telemetry summary in advanced settings that counter is set properly. LGTM

timroes · 2020-01-10T13:02:50Z

src/legacy/core_plugins/visualizations/public/np_ready/public/legacy/build_pipeline.ts

+  visState: VisState,
+  schemas: Schemas,
+  uiState: any,
+  meta?: { savedObjectId?: string }


Maybe we want to make a TODO here or directly create an issue, that we remove this again once we enable hard validation?

timroes · 2020-01-13T10:17:41Z

@elasticmachine merge upstream

kibanamachine · 2020-01-13T11:31:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e0dec2f

History

💚 Build #19285 succeeded 77f5cf8
💔 Build #19223 failed ffa3df4
💔 Build #19208 failed 4731b94
💔 Build #19197 failed c65129a
💔 Build #16896 failed fdd7a74

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

streamich

AppArch code changes LGTM.

* upstream/master: (26 commits) Take page offset into account too (elastic#54567) [APM] Support error.{log,exception}.stacktrace.classname (elastic#54577) Np migration tsvb route validation (elastic#51850) [ML] MML calculator enhancements for multi-metric job wizard (elastic#54573) [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (elastic#54223) Fix chromeless NP apps not using full page width (elastic#54550) Remove extraneous public import to prevent failing Kibana startup (elastic#54676) [Uptime] Temporarily skip flakey tests (elastic#54675) Skip failing uptime tests Create UI for alerting and actions plugin (elastic#48959) [dev/build/sass] build stylesheets for disabled plugins too (elastic#54654) [SIEM] Use bulk actions API when updating or deleting rules (elastic#54521) Support "Deprecated" label in advanced settings (elastic#54539) [Maps] add text halo color and width style properties (elastic#53827) Service Map Data API at Runtime (elastic#54027) [SIEM] Detection Engine Create Rule Design Review #1 (elastic#54442) Skip flaky test [Canvas] Enable Embeddable maps (elastic#53971) [SIEM][Detection Engine] Increases the number or rules you can view on a single page (elastic#54628) uiSettings - use validation field for image field maxSize (elastic#54522) ...

TinaHeiligers added 2 commits November 27, 2019 16:36

[Route audit] adds initial Joi validation schema to payload

b485d56

Adds more keys to the payload validation

ffd1de4

TinaHeiligers added Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 28, 2019

TinaHeiligers added 8 commits November 29, 2019 12:18

Merge branch 'master' of github.com:elastic/kibana into NP_migration_…

1487ebe

…TSVB_route_validation

Merge branch 'master' of github.com:elastic/kibana into NP_migration_…

da5c949

…TSVB_route_validation

Merge branch 'master' of github.com:elastic/kibana into NP_migration_…

cde75b4

…TSVB_route_validation

Adds more validation items to the TSVB payload

0108b50

Merge branch 'master' of github.com:elastic/kibana into NP_migration_…

81751fa

…TSVB_route_validation

Starts extracting common subschemas for reuse

e106c65

Adds more payload validation items

a8e9e34

extracts inner objects

489219a

flash1293 added 2 commits December 6, 2019 10:34

Merge remote-tracking branch 'upstream/master' into NP_migration_TSVB…

4079b45

…_route_validation

fix schema

338783c

flash1293 marked this pull request as ready for review December 6, 2019 12:20

flash1293 requested review from timroes, legrego and alexwizp December 6, 2019 12:20

legrego reviewed Dec 6, 2019

View reviewed changes

alexwizp reviewed Dec 19, 2019

View reviewed changes

src/legacy/core_plugins/vis_type_timeseries/public/request_handler.js Outdated Show resolved Hide resolved

alexwizp reviewed Dec 19, 2019

View reviewed changes

alexwizp approved these changes Dec 19, 2019

View reviewed changes

alexwizp reviewed Dec 19, 2019

View reviewed changes

alexwizp self-requested a review December 19, 2019 13:41

Merge remote-tracking branch 'upstream/master' into NP_migration_TSVB…

fdd7a74

…_route_validation

flash1293 added 2 commits January 9, 2020 14:59

Merge remote-tracking branch 'upstream/master' into NP_migration_TSVB…

ba8c8e6

…_route_validation

remove setting and improve log message

c65129a

TinaHeiligers requested a review from a team as a code owner January 10, 2020 09:08

flash1293 added 2 commits January 10, 2020 10:49

fix eslint error

4731b94

fix type error

ffa3df4

timroes reviewed Jan 10, 2020

View reviewed changes

timroes approved these changes Jan 10, 2020

View reviewed changes

timroes reviewed Jan 10, 2020

View reviewed changes

flash1293 mentioned this pull request Jan 10, 2020

Remove saved object id in TSVB params #54456

Closed

remove lodash call and fix test

77f5cf8

Merge branch 'master' into NP_migration_TSVB_route_validation

e0dec2f

timroes added the v8.0.0 label Jan 13, 2020

streamich approved these changes Jan 13, 2020

View reviewed changes

flash1293 merged commit 9380b64 into elastic:master Jan 14, 2020

flash1293 mentioned this pull request Jan 14, 2020

[7.x] Np migration tsvb route validation (#51850) (9380b640) #54688

Merged

flash1293 pushed a commit to flash1293/kibana that referenced this pull request Jan 14, 2020

Np migration tsvb route validation (elastic#51850)

fc4fa8b

flash1293 added a commit that referenced this pull request Jan 14, 2020

Np migration tsvb route validation (#51850) (#54688)

0a2c83d

jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020

Np migration tsvb route validation (elastic#51850)

d005b90

TinaHeiligers deleted the NP_migration_TSVB_route_validation branch October 14, 2020 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Np migration tsvb route validation #51850

Np migration tsvb route validation #51850

TinaHeiligers commented Nov 28, 2019 •

edited

Loading

elasticmachine commented Nov 28, 2019

elasticmachine commented Nov 28, 2019

flash1293 commented Dec 4, 2019

elasticmachine commented Dec 4, 2019

elasticmachine commented Dec 6, 2019

flash1293 commented Dec 6, 2019

TinaHeiligers commented Dec 6, 2019

flash1293 commented Dec 6, 2019

legrego Dec 6, 2019

TinaHeiligers Dec 6, 2019

flash1293 Dec 9, 2019

legrego Dec 9, 2019

TinaHeiligers commented Dec 6, 2019

alexwizp Dec 19, 2019

alexwizp left a comment

elasticmachine commented Dec 20, 2019

flash1293 commented Jan 10, 2020 •

edited

Loading

timroes Jan 10, 2020

timroes left a comment

timroes Jan 10, 2020

flash1293 Jan 10, 2020

timroes commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

streamich left a comment

Np migration tsvb route validation #51850

Np migration tsvb route validation #51850

Conversation

TinaHeiligers commented Nov 28, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 28, 2019

elasticmachine commented Nov 28, 2019

💔 Build Failed

flash1293 commented Dec 4, 2019

elasticmachine commented Dec 4, 2019

💔 Build Failed

elasticmachine commented Dec 6, 2019

💚 Build Succeeded

flash1293 commented Dec 6, 2019

TinaHeiligers commented Dec 6, 2019

flash1293 commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Dec 6, 2019

Choose a reason for hiding this comment

alexwizp left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 20, 2019

💔 Build Failed

History

flash1293 commented Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

💚 Build Succeeded

History

streamich left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Nov 28, 2019 •

edited

Loading

flash1293 commented Jan 10, 2020 •

edited

Loading