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

Add content-type wildcard support to validation #93

Merged
merged 2 commits into from
May 10, 2019

Conversation

kconwayatlassian
Copy link
Contributor

This implements the cascading wildcard behavior described in the OpenAPI
specification for request body and response body validation.

https://swagger.io/docs/specification/describing-request-body/

Closes #91

This implements the cascading wildcard behavior described in the OpenAPI
specification for request body and response body validation.

https://swagger.io/docs/specification/describing-request-body/
Copy link
Collaborator

@fenollp fenollp left a comment

Choose a reason for hiding this comment

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

I'm thinking we either should return nil or panic or even not handle that case.
What do you think?

// try the x/* pattern.
i = strings.IndexByte(mime, '/')
if i < 0 {
return content["*/*"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can there be mimes without / in them? Should this case really be considered at all?

@kconwayatlassian
Copy link
Contributor Author

@fenollp I don't believe mime types are valid without the subtype. However, since they are arbitrary string input from a client I'd prefer if we degraded to the wildcard rather than panic. Alternatively, we can return nil for this case instead of the wildcard if that would be preferable. I'm not sure how the OpenAPI specification intends for this case to be handled other than this blerb from the docs:

*/* represents all types and is functionally equivalent to application/octet-stream

This seems to suggest that */* should be used in all cases where a better match is not found.

My concern is that if I'm using the request/response validation in production then a panic on bad input might be an avenue for denial of service if a client is sending invalid mime types. I'd prefer the response to be "400 Bad Request" wherever possible for faulty input.

@fenollp
Copy link
Collaborator

fenollp commented May 9, 2019

Agreed wrt panics.
I’d like ya to return nil as this would at least not go unnoticed. I’m sure there’s a nonzero amount of things out there that send an incorrect content type mime type but I very much prefer strictness and to discuss an optional behavior when someone has a use case.
What do you think?

Rather than allow accidental fallthrough of invalid mime types we will
return nil.
@kconwayatlassian
Copy link
Contributor Author

@fenollp I've updated the PR to return nil for cases of invalid mime types.

Adding the test for this actually exposed a bug in my initial patch where any mime type that did not have metadata such as ;encoding=utf-8 would return the double wildcard result rather than the single wildcard result if there was no direct match. I've increased the test coverage to account for this and patched the logic to behave as expected.

@fenollp
Copy link
Collaborator

fenollp commented May 10, 2019

Amazing! Thanks!

@fenollp fenollp merged commit 90a7df0 into getkin:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request validation doesn't support content-type wildcards
2 participants