NP Router handler doesn't provide to request body on validate: false

[jfsiii]

Kibana version: master / 0150c8e
Elasticsearch version: 8.0.0-SNAPSHOT

Problem

When using the NP router for POST routes, the request handler doesn't appear to have access to the request body when validate: false is supplied

Steps to reproduce

const router = createRouter('/np_example');
router.post(
  { path: '/b/{some_param}', validate: false },
  async (context, request, response) => {
    console.log({ request });
    return response.ok({ body: 'return value from /b' });
  }
);

curl --user elastic:changeme \
  -X POST -H 'kbn-xsrf: true' \
  -H "Content-Type: application/json" \
  -d '[{"key":"value one"},{"key":"value two"}]' \
  http://localhost:5601/np_example/b/with_post?foo=bar

Observed

logs return value from /b to terminal making request and

{ request:
   KibanaRequest {
     params: {},
     query: {},
     body: {},
     withoutSecretHeaders: true,
     url:
      Url {
        protocol: null,
        slashes: null,
        auth: null,
        host: null,
        port: null,
        hostname: null,
        hash: null,
        search: '?foo=bar',
        query: [Object],
        pathname: '/np_example/b/with_post',
        path: '/np_example/b/with_post?foo=bar',
        href: '/np_example/b/with_post?foo=bar' },
     route:
      { path: '/np_example/b/with_post',
        method: 'post',
        options: [Object] },
     headers:
      { host: 'localhost:5601',
        authorization: 'Basic ZWxhc3RpYzpjaGFuZ2VtZQ==',
        'user-agent': 'curl/7.54.0',
        accept: '*/*',
        'kbn-xsrf': 'true',
        'content-type': 'application/json',
        'content-length': '41' },
     socket:
      KibanaSocket {
        socket: [Socket],
        authorized: undefined,
        authorizationError: undefined } } }

in server logs (no request body that I can find)

Expected

Some way to access the request body. Whether to validate independently or simply to use it as-is.

Further discussion

Reading the notes

kibana/src/core/server/http/router/route.ts

Lines 65 to 87 in d394ded

	/**
	* A schema created with `@kbn/config-schema` that every request will be validated against.
	* You *must* specify a validation schema to be able to read:
	* - url path segments
	* - request query
	* - request body
	* To opt out of validating the request, specify `false`.
	* @example
	* ```ts
	* import { schema } from '@kbn/config-schema';
	* router.get({
	* path: 'path/{id}'
	* validate: {
	* params: schema.object({
	* id: schema.string(),
	* }),
	* query: schema.object({...}),
	* body: schema.object({...}),
	* },
	* })
	* ```
	*/
	validate: RouteSchemas<P, Q, B> | false;

make it appear this is intentional, but I believe it's needlessly limiting to authors.

There are many reasons why one might set validate: false. For example, if they already have a validation library like io-ts or joi and they want the NP router/handler without also having convert to their models to @kbn/config-schema. Setting validate: false and moving the validation to the handler allows for an incremental move to NP, dropping hapi or KbnServer without also having to give up what they like about their validation library.

In my opinion, it's reasonable to say, if you want these additional features (e.g. validation and request.{query,params,body} properties) you must use this pattern, but I don't think it's helpful to prevent all other ways to accomplish the task (processing the POST) request.

For example, in GET request handlers, the request.{query,params,body} parameters are also empty objects {}, but there is a WhatWG URL instance that can be used. It's present in the POST request above as well. This way, developers can opt-out of validation, but still achieve their goal.

There are a number of possible solutions to this approach, but I think the most important thing is to provide some way to access the request body when not using @kbn/config-schema

[simianhacker]

Whether or not you use validation shouldn't affect how you access the request objects (query, params, body). It should only control whether or not you validated the request objects. Setting validate: false should just perform a noop and pass everything through "as is".

[elasticmachine]

Pinging @elastic/kibana-platform

[joshdover]

Validating incoming data provides us pretty great protection against prototype pollution attacks. I'm not sure if was the rationale for requiring it or not, but it seems like a pretty good reason to me.

I agree there should be a way to opt out and false should allow that, but I also think this should only be used while developing to enable rapid prototyping & experimentation.

There are many reasons why one might set validate: false. For example, if they already have a validation library like io-ts or joi and they want the NP router/handler without also having convert to their models to @kbn/config-schema.

It should be noted that @kbn/config-schema really is Joi under the hood, but designed in a way that can be type safe. Converting between Joi and @kbn/config-schema should be fairly painless.

[kobelb]

The current implementation was done at my request, for the reasons which @joshdover articulated. One of the best defenses against prototype pollution is requiring the user's input to match a strict schema. We've historically had routes which allowed free-form user input which was then used with a vulnerable recursive algorithm like _.deepMerge in old versions of lodash which led to the Object prototype being polluted.

We really shouldn't be using the user's input "as-is" without validation. If there's a specific use-case this enables which I just haven't thought of, please do bring it up.

It certainly makes my life easier if we standardize on a validation library as it makes linting/static-analysis for "non-strict" validation on routes easier to detect. Are there specific features which io-ts provides which @kbn/config-schema doesn't? Or is it primarily that you all have your validation written already using io-ts and would prefer to not have to rewrite all of this?

If we do end up allowing route definitions to skip the validation, I'd prefer we name the setting something painfully obvious like "dangerouslySkipValidation". This makes it more obvious to the author that this is a dangerous operation, and makes it easier for us to "grep" for usages.

[jfsiii]

We really shouldn't be using the user's input "as-is" without validation.

Aren't GET requests effectively exempt from this since they have the url instance? I realize it's not parsed path segments, but the unvalidated user input is still available.

e.g. my POST example could be converted to a GET like

curl --user elastic:changeme localhost:5601/np_example/a/b?body='%5B%7B%22key%22%3A%22value%20one%22%7D%2C%7B%22key%22%3A%22value%20two%22%7D%5D'

and the handler would get this in request.url

{
     protocol: null,
     slashes: null,
     auth: null,
     host: null,
     port: null,
     hostname: null,
     hash: null,
     search:
      '?body=%5B%7B%22key%22%3A%22value%20one%22%7D%2C%7B%22key%22%3A%22value%20two%22%7D%5D',
     query:
      [Object: null prototype] { body: '[{"key":"value one"},{"key":"value two"}]' },
     pathname: '/np_example/a/b',
     path:
      '/np_example/a/b?body=%5B%7B%22key%22%3A%22value%20one%22%7D%2C%7B%22key%22%3A%22value%20two%22%7D%5D',
     href:
      '/np_example/a/b?body=%5B%7B%22key%22%3A%22value%20one%22%7D%2C%7B%22key%22%3A%22value%20two%22%7D%5D' 
}

Are there specific features which io-ts provides which @kbn/config-schema doesn't?

io-ts provides runtime checks. Does @kbn/config-schema? I've seen it used in some code & examples, but cannot find any docs or discussions about it. Where can I learn more about @kbn/config-schema?

is it primarily that you all have your validation written already using io-ts and would prefer to not have to rewrite all of this?

The plugin I'm writing is new and still rather small, so the conversion isn't a concern for me. However, the @elastic/infra-logs-ui team has some io-ts already and is exploring more. @elastic/apm-ui recently migrated some routes to it as well.

My goal is to describe the validation in one place and use that description as (directly or as part of some pipeline) a) validation schema b) TypeScript type/interface c) JSON Schema and/or OpenAPI spec.

I had some discussion about this in #36674 (comment) and did some exploration in #40041. I see a way to do that with both joi and io-ts but I don't know how @kbn/config-schema affects those. However, as I said,

"I there are a few references, especially #40623, for incorporating OpenAPI definitions

so hopefully @kbn/config-schema fits into those.

[kobelb]

Aren't GET requests effectively exempt from this since they have the url instance? I realize it's not parsed path segments, but the unvalidated user input is still available.

That's a good point. I'm not super excited to restrict access to request.url as that seems rather awkward though.

[sorenlouv]

I don't have all of the context but: On APM we've ditched joi and are using io-ts for runtime and compile time checks. Works great 👍 I don't assume this is going to be a problem in NP?

[rudolf]

I agree that we should be requiring some sort of validation. The most flexible API would be to just require a validation function, but that is open to "abuse" with something like validate: (body) => body. Strictly limiting validation to a specific validation library gives us stronger guarantees, but you can also "abuse" that by specifying a very loose schema. So it might be better to rely on a culture of building secure HTTP endpoints rather than strictly enforcing this and I think requiring some function is a good hint to a developer so that validation isn't simply forgotten.

As far as I can tell @kbn/config-schema is very similar to io-ts in providing runtime validation with type information. However, being based on Joi I assume @kbn/config-schema doesn't have browser support, which is a significant downside. Having browser-side validation makes it easier to build snappy UI which can inform the user of invalid input without having to make a network request.

[mshustov]

I think requiring some function is a good hint to a developer so that validation isn't simply forgotten.

Agree. It provides a good balance between security, ease of use and speed of migration.
Although, it won't be easy to grep all Kibana plugins that opt-out of validation and preferred to implement custom ones.

As far as I can tell @kbn/config-schema is very similar to io-ts in providing runtime validation with type information.However, being based on Joi I assume @kbn/config-schema doesn't have browser support, which is a significant downside.

io-ts provides validation for TS language types out-of-the-box. Whereas Joi supports validators for compound data types such as email, ip, uri, etc. In the future, we can switch @kbn/config-schema implementation from Joi to io-ts, for example, but that will require some work.

[dgieselaar]

Just want to add my $0.02 about io-ts and Joi: IME it's relatively easy to extend io-ts with more specific types, seemingly with less code than @kbn/config-schema requires to build on top of Joi. From where we're standing, not being able to share types between server and client (at least not with the current version of Joi) is also very unfortunate. Would be great if we can opt out of @kbn/config-schema and just use io-ts while that is still the case.

io-ts also offers encoding. As an example, we have a type that encodes/decodes JSON. It does not however provide a default value, something that @kbn/config-schema does seem to offer.

[weltenwort]

Another vote from my side for leaving the option to use other validation methods in general and io-ts in particular. We use it for full validation of the elasticsearch - kibana - browser communication with the io-ts type as the single source of truth.

[azasypkin]

My 0.05₽ as well:

IME it's relatively easy to extend io-ts with more specific types, seemingly with less code than @kbn/config-schema requires to build on top of Joi

Even though it's good to know, it may not matter too much to consumers if it's done under the hood in @kbn/config-schema anyway assuming we still want to keep that additional abstraction, see below.

From where we're standing, not being able to share types between server and client (at least not with the current version of Joi) is also very unfortunate.

That's a good point and one of the reasons why we wanted to abstract away from Joi with @kbn/config-schema with a goal to eventually add client-side support to it (e.g. ditch Joi in favor of anything else).

But I'm curious how real this use case is already, do you have any immediate need in sharing schemas you define on your server-side routes and use on client-side for something else?

Would be great if we can opt out of @kbn/config-schema and just use io-ts while that is still the case.

I see your point, but I'm really afraid of fragmenting our HTTP request validation infrastructure. If we find an issue in @kbn/config-schema we'll have to go and check if alternative framework isn't suffering from this issue and fix that if so and potentially in a different manner. Even more dangerous if it's a security vulnerability. We had issues in the past when Joi validation messages leaked too much information and it's not always easy to fix, with @kbn/config-schema we can override anything and for everything if we need to. It's not ideal and I assume io-ts can do this too, but it's worth pointing out.

Another thing that may be problematic in using io-ts\whatever directly is uncontrolled API surface that is quite hard to audit and migrate away from when needed (who knew few years ago we'll be using Joi biting-the-bullet with a desire to replace it as soon as possible?).

. As an example, we have a type that encodes/decodes JSON.

That's quite easy to implement in @kbn/config-schema as well if needed, also all sort of validations can be done with custom validate function that is provided for every type.

Having said that I'd be glad if we replace Joi with a better alternative if there is a need and too much friction, but still keep it as an internal detail of our own @kbn/config-schema-like abstraction. The @kbn/config-schema doesn't have to be and won't be a universal validation library for Kibana, we use it for NP config and NP routes now and can optionally use it for anything else, but not obliged to.

---

re: handler doesn't provide to request body on validate: false - not super relevant to the discussion, but you can achieve that with validation like this I think:

validate: { body: schema.object({}, { allowUnknowns: true }) }

We can probably allow using schema.map, schema.recordOf and even schema.any as well. At least it'd be explicitly said that body\query\params will be used somewhere somehow and reviewer can dig, follow the request flow and make sure the usage isn't problematic. But if developer can't say anything at all about the shape of the request it's accepting it sounds concerning to me (with a few exceptions, e.g. pass through proxy-like code and anything along these lines that also require additional care and attention).

[jfsiii]

re: handler doesn't provide to request body on validate: false - not super relevant to the discussion, you can achieve that with validation like this I think

validate: { body: schema.object({}, { allowUnknowns: true }) }

That is the title/purpose of the original ticket, but the the discussion has grown to something larger.

I'm ok with closing this ticket with "works as intended" and letting teams use the schema.any-style workaround.

However, I think the discussion that began here around the current/future use of io-ts should be continued in another ticket. I opened #45074 to continue this discussion. Feel free to rename/edit. I just wanted a more focused place to have the discussion.