Malformed content-type header allows to bypass request validation

[fredplante]

Hello,

I recently noticed some requests for which it seemed the schema validation was not performed. Upon investigation, I noticed these requests had in common malformed Content-Type headers, for example: multipart/form-data, application/json.

Committee is extracting the media type from the header in 2 place:

First, it's done in https://github.com/interagent/committee/blob/v5.6.0/lib/committee/schema_validator/open_api_3/request_validator.rb#L15 to check if the request content type is defined in the schema. It's using ActionDispatch::Http::MimeNegotiation#content_mime_type. Here, a malformed content-type like multipart/form-data, application/json is interpreted as multipart/form-data.

Assuming the schema handles this mime type, the process continues. Later, Committee extract the content type again, but this time by splitting using ; as a delimiter in https://github.com/interagent/committee/blob/v5.6.0/lib/committee/schema_validator/open_api_3/operation_wrapper.rb#L114 and pass this content-type to the request validation handled by openapi_parser.

Splitting on ; means that in this case we are passing multipart/form-data, application/json to the openapi_parser gem and it does not manage to extract the media type, returns nil and effectively just bypass the validation in https://github.com/ota42y/openapi_parser/blob/5cbe0772d9b2a4f8345c6c2ded1a05dee1b7b7d1/lib/openapi_parser/schemas/request_body.rb#L22.

I'm not sure what should be the fix, but at least I guess the content type extraction should be consistent.

[geemus]

@fredplante Thanks for the detailed report, let me dig in.

It looks to me like the request_validator is deferring to the media_type as determined by Rack, rather than ActionDispatch, but I may be missing something. Could you point me toward where you found that?

Splitting on ; definitely seems problematic there. My inclination would probably be to change that to match the way it's being done in the request_validator, which I think would be to use this: https://github.com/rack/rack/blob/d8d50702429265c28b84968e43caafc4bffd82ef/lib/rack/media_type.rb

If we made that update it would at least be consistent, then we can certainly discuss more what the behavior should be in the case that you mentioned. I think basically the case is where the schema doesn't provide anything for the specified content-type, is that right? Behavior there may relate to how strict it is set or not (I think if things are set to strict and nothing matches it should error, whereas otherwise a non-match is allowed).

[fredplante]

@geemus

It looks to me like the request_validator is deferring to the media_type as determined by Rack, rather than ActionDispatch, but I may be missing something. Could you point me toward where you found that?

In a Rails context, in https://github.com/interagent/committee/blob/v5.6.0/lib/committee/schema_validator.rb#L7 the request object is a ActionDispatch::Request, thus #media_type is slightly different implementation, but both implementation provides same result for the bogus content type multipart/form-data, application/json.

Splitting on ; definitely seems problematic there. My inclination would probably be to change that to match the way it's being done in the request_validator

Indeed, we should be consistent here.

I think basically the case is where the schema doesn't provide anything for the specified content-type, is that right? Behavior there may relate to how strict it is set or not (I think if things are set to strict and nothing matches it should error, whereas otherwise a non-match is allowed).

If the schema doesn't provide anything for the specified content-type it's working, as committee handle this well: the request has multipart/form-data, application/json as content type, committee check in the schema using multipart/form-data so it works there.

My issue is after that: because of the inconsistency in determining the content type, just before handing it over to openapi_parser, when it splits using ; openapi_parser is unable to validate against the schema, and no exception is raised.

Therefore, the request is not validated at all and committee happily let the request pass. That's the main issue here for me: we noticed sometimes very weird exception that should not happen in theory because Committee is supposed to not let pass such bogus requests.

It looks like to me that by only enforcing consistency this problem would be solved. On our end, we solved this by normalizing bogus content-type header when they are invalid.

[geemus]

@fredplante Could you check out #455? I think it at least fixes the immediate/obvious inconsistency bug.

[fredplante]

@geemus I just tested it and I can confirm it fixes the issue I reported 🙌

[fredplante]

Note that the fix in #455 still means that we have potential inconsistencies as a different implementation is used in Committee::SchemaValidator for the same task.

[geemus]

@fredplante thanks for confirming the fix. I'll look at making the two consistent and see if I can add some tests and get a release out.

[geemus]

@fredplante - I have attempted to update so that both places would be more consistent, and I think it should work as expected, but I don't think the tests do rails vs rack as well as they could (and perhaps should). Could you check again and ensure I haven't broken anything in my attempt to align things?

[fredplante]

@geemus I can confirm it still works 🙂

[geemus]

@fredplante Thanks for the detailed report, your help and patience. I just released the fix in v5.6.1.