-
Notifications
You must be signed in to change notification settings - Fork 21.9k
Add the Mime::Type::InvalidMimeType
error in the default rescue_response:
#35753
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
Conversation
d074935
to
cc7f543
Compare
@@ -178,6 +180,10 @@ def call(env) | |||
get "/parameter_missing", headers: { "action_dispatch.show_exceptions" => true } | |||
assert_response 400 | |||
assert_match(/ActionController::ParameterMissing/, body) | |||
|
|||
get "/invalid_mimetype", headers: { "Accept" => "text/html,*", "action_dispatch.show_exceptions" => true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think *
is not an invalid mime type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, * is but */*
is not and we got an exception for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair I also thing *
is valid, or at least IE10 sends it by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm if IE10 sends it by default then we should probably support it, although the RFC doesn't say it's valid https://tools.ietf.org/html/rfc7231#page-38
I'll try to modify the regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(How did you check btw, so I know for next time).
All requests were to xml paths in very crazy pages. This looks like behavior of bots.
If the browser sends malformated query paratemers or invalid bite sequence, rack would raise and block access to the app as well, no ?
Right, but this started because of a change on the browser, where now this started because of a change in the framework that is now rejecting a request that were valid for years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look in our logs and DW, the Accept headers on IE 10 is correctly formatted and sent as */*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm @rafaelfranca , do we want to modify the regex to include the *
mime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know IE10 is not sending that, it is fine to exclude it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know IE10 is not sending that, it is fine to exclude it.
For anyone running into the same situation, I believe we are getting this error from valid IE 10 machines. Example UA below:
Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.108 Safari/537.36
I looked up the UA and it appears to be listed in https://developers.whatismybrowser.com/useragents/explore/software_name/chrome/ as well.
Maybe it would be good to allow *
?
cc7f543
to
ea61ff3
Compare
…ponse: - rails#35604 introduced a vulnerability fix to raise an error in case the `HTTP_ACCEPT` headers contains malformated mime type. This will cause applications to throw a 500 if a User Agent sends an invalid header. This PR adds the `InvalidMimeType` in the default `rescue_responses` from the ExceptionWrapper and will return a 406. I looked up the HTTP/1.1 RFC and it doesn't stand what should be returned when the UA sends malformated mime type. Decided to get 406 as it seemed to be the status the better suited for this.
ea61ff3
to
378b4fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Looks good to me
…_app: - Rescuing the InvalidMimeType error in the PublicException class wasn't a good idea because if an app has a custom exception_class it will cause the same issue. This PR move the rescue around calling the exception_app.
To be able to distinguish from other kinds of `Mime::Type::InvalidMimeType` that may be raised by user or third-party code. It's only failure in parsing client-supplied content-types in ActionDispatch::Http::MimeNegotiation that should result in special handling. This also allows third-party error handling/tracking code to specifically target the new `ActionDispatch::Http::MimeNegotiation::InvalidType` for ignoring or other special handling, separate from `Mime::Type::InvalidMimeType` This is a revision of rails#35753 in response to rails#37620 and discussion with @eugeneius
Add the
Mime::Type::InvalidMimeType
error in the default rescue_response:Raise exception when building invalid mime type #35604 introduced a vulnerability fix
to raise an error in case the
HTTP_ACCEPT
headers contains malformatedmime type.
This will cause applications to throw a 500 if a User Agent sends an
invalid header.
This PR adds the
InvalidMimeType
in the defaultrescue_responses
fromthe ExceptionWrapper and will return a 406. I looked up the HTTP/1.1
RFC and it doesn't stand what should be returned when the UA
sends malformated mime type. Decided to go for 406 as it seemed to be the
status the better suited for this.
cc/ @rafaelfranca @jhawthorn