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

Decouple request validation from @kbn/config-schema #50179

Closed
joshdover opened this issue Nov 11, 2019 · 6 comments · Fixed by #51919
Closed

Decouple request validation from @kbn/config-schema #50179

joshdover opened this issue Nov 11, 2019 · 6 comments · Fixed by #51919
Assignees
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Contributor

joshdover commented Nov 11, 2019

We made the decision last week to decouple the request validation requirement from the @kbn/config-schema package. Reasons for this decision:

  • The @kbn/config-schema package was designed explicitly for config schema. While it could be developed to be compatible with the request validation use-case, we do not have the time to do this right now.
  • This package does not currently work in the browser, due to its dependency on Joi. This is a commonly requested feature. Newer versions of Joi do support the browser environment, but we do not have the time at this moment to do this upgrade.
  • Other teams are already using other packages, such as io-ts, for validation. While there are some advantages of using the same package (eg. consistent validation error formats), we do not know what the recommended package for this use-case will be yet. In the meantime, we shouldn't stand in the way of what teams need to accomplish today.

We should be able to replace this mechanism with a simple abstraction that is compatible with @kbn/config-schema in a non-breaking way and could be made compatible with other packages (eg. io-ts) with a lightweight wrapper.

Related: #45074

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Nov 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@kobelb
Copy link
Contributor

kobelb commented Nov 11, 2019

Requiring strict validation on user-input is our primary defense against prototype-pollution. The lack of a standardized approach to validating request data potentially makes this significantly harder. Are there any additional details for exactly how this will be implemented, as it's possible I'm mis-understanding the implementation and this isn't such a cause for concern.

@joshdover
Copy link
Contributor Author

@kobelb would it not be possible to use the prototype pollution validation added in (#48753) separately from schema validation? Could we execute that validation first, prior to excuting the schema validation?

@kobelb
Copy link
Contributor

kobelb commented Nov 18, 2019

We could, but we don't always want to do the prototype pollution validation which was added in #48753. There are various routes in Kibana where we need to disable this validation, for example any routes which are essentially "proxies" to Elasticsearch where users should be able to do things like a terms aggregation on a field named __proto__.

Additionally, this validation only catches the low-hanging fruit. The stricter that we are with the route validation, the more assurance we have that the input is "safe".

@joshdover
Copy link
Contributor Author

We could, but we don't always want to do the prototype pollution validation which was added in #48753. There are various routes in Kibana where we need to disable this validation, for example any routes which are essentially "proxies" to Elasticsearch where users should be able to do things like a terms aggregation on a field named __proto__.

For the proxies case, we can skip the prototype pollution validation when the parse: false option added in #50783 is set.

Additionally, this validation only catches the low-hanging fruit. The stricter that we are with the route validation, the more assurance we have that the input is "safe".

I agree with this, which is one reason this proposed change is specified as a temporary stop-gap rather than a long-term solution. Alternatively, we could ask plugins to use schema.object({}, { allowUnknowns: true }) and then use their own validation logic in their request handlers. However, I don't think that would be significantly different than what is being proposed here?

@kobelb
Copy link
Contributor

kobelb commented Nov 18, 2019

I agree with this, which is one reason this proposed change is specified as a temporary stop-gap rather than a long-term solution. Alternatively, we could ask plugins to use schema.object({}, { allowUnknowns: true }) and then use their own validation logic in their request handlers. However, I don't think that would be significantly different than what is being proposed here?

Not significantly, but it would perhaps encourage other developers without their own validation framework to use the default validation, which is better than nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants