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

Raise exception when building invalid mime type #35604

Merged
merged 1 commit into from Mar 19, 2019

Conversation

jhawthorn
Copy link
Member

This only allows mime types to be in the form text/html, text/*, or */*.

This required a few minor test/code changes where previously nil was used as a mime string.

Thanks @pixeltrix and @georgeclaghorn for help with the regex ❤️

cc @tenderlove @eileencodes

@jhawthorn jhawthorn force-pushed the validate_mime_types branch 2 times, most recently from 04d0f0c to 5cad0e6 Compare March 13, 2019 21:40
This allows mime types in the form text/html, text/*, or */*

This required a few minor test/code changes where previously nil was
used as a mime string.
@eileencodes eileencodes merged commit 7fe3c69 into rails:master Mar 19, 2019
@eileencodes
Copy link
Member

Oh you know what after merging it occurred to me that maybe this warrants a CHANGELOG entry. Thoughts?

@eileencodes eileencodes added this to the 6.0.0 milestone Mar 19, 2019
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Mar 25, 2019
…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.
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Mar 25, 2019
…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.
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Mar 26, 2019
…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.
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Mar 26, 2019
…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.
@dbackeus
Copy link

@jhawthorn @eileencodes perhaps not too late to add to CHANGELOG and perhaps Rails 6 upgrade guides? This was a breaking change and is currently confusing quite a few Rails users, eg. #37620 (comment)

As more devs upgrade their old Rails app to 6.x confronting this error might become an increasingly common occurrence.

@eileencodes
Copy link
Member

perhaps not too late to add to CHANGELOG and perhaps Rails 6 upgrade guides

Do you want to open a PR @dbackeus?

@hakusaro
Copy link
Contributor

hakusaro commented Aug 13, 2020

I'd be more than happy to send a PR here, but I'm not sure what the rails team guidance is for this. We're seeing relatively modern user agents sending this set of invalid headers, so I'm not sure if they're bots that have bad headers imitating modern browsers, or legitimate users in some weird edge case. Are most rails 6 apps just dropping these requests?

Edit: I don't want to provide the exact headers for anonymity purposes, but at least one browser claims to be Safari 11 on macOS 10.13.

@jrochkind
Copy link
Contributor

Is there any context available as to why this change was made to raise here? Is it a security issue?

Getting an error in my error logs every time a user-agent sends a malformed Content-type (or in some cases Accept), which results in an unrescued exception, is annoying me. I have to figure out how to filter them out from my error logs, as it's not actually an application error or something I can fix. I preferred the behavior before this PR.

But before I try to fix/undo it, it would be helpful to know the context of why the change was made, if anyone can supply it, perhaps with a link to an issue or discussion that prompted this PR?

@hakusaro
Copy link
Contributor

hakusaro commented Oct 1, 2020

Hi! Long time no see! Again, I'd be more than happy to submit a pull request to add this to the changelog. But I don't want to do that without giving someone advice on what to do when invalid requests inevitably come in. Is the official rails guidance to simply ignore these errors and filter them out of their error logs? @jhawthorn + @eileencodes do you have any input?

@dave105010
Copy link

Hi, I agree with @jrochkind .
Is there any context available as to why this change was made to raise here? Is it a security issue?

I see a request made from Firefox 78.0 which generates an error.

Relatively old considering the newest version is 88.0 but still...

I also agree with @hakusaro .
Is the official rails guidance to simply ignore these errors and filter them out of their error logs?

If someone can enlighten us, that would be great.

@pixeltrix
Copy link
Contributor

@dave105010 @hakusaro if you're seeing ActionDispatch::Http::MimeNegotiation::InvalidType then yes, it's fine to ignore these errors because they're coming from the Accept header of the user agent. If however you're seeing Mime::Type::InvalidMimeType errors then that's a different codepath so you'll probably still want to log those. We initially added the latter error but realised we needed to differentiate the two types of error and @jrochkind kindly added it in #40353.

If you're still on 6.0.x then the more specific error hasn't been released yet for that branch but I believe it will be released fairly soon.

@dave105010
Copy link

dave105010 commented May 5, 2021

@dave105010 @hakusaro if you're seeing ActionDispatch::Http::MimeNegotiation::InvalidType then yes, it's fine to ignore these errors because they're coming from the Accept header of the user agent. If however you're seeing Mime::Type::InvalidMimeType errors then that's a different codepath so you'll probably still want to log those. We initially added the latter error but realised we needed to differentiate the two types of error and @jrochkind kindly added it in #40353.

If you're still on 6.0.x then the more specific error hasn't been released yet for that branch but I believe it will be released fairly soon.

Thank you for the clarification @pixeltrix . I really appreciate it.

We are on Rails 6.1.3.1 right now. So, we are definitely seeing ActionDispatch::Http::MimeNegotiation::InvalidType not the other one.

The error is "/" is not a valid MIME type
And the failing Accept header is this:
HTTP_ACCEPT : text/html,applicaton/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8

@hakusaro
Copy link
Contributor

hakusaro commented May 6, 2021

@pixeltrix thank you so much for your thoughtful reply! Upon further inspection, we were seeing both bubble up on 6.1.3.1. We investigated and verified that every single occurrence was some atypical/anomalous client passing weird data to us. We ended up filtering out both exception types to rollbar. It's my understanding that we can't just rescue_from the ActionDispatch::Http::MimeNegotiation::InvalidType at the ActionController level, but if my understanding is incorrect, I'd love to be enlightened. Right now, we're just silencing the error since it doesn't impact us.

@pixeltrix
Copy link
Contributor

pixeltrix commented May 8, 2021

@hakusaro I'm pretty sure you can't rescue it but even if you could I'd recommend against it - it's the same kind of error as if someone requested a .json response to an action that only supports HTML responses. Quietly rescuing and redirecting these errors obscures the fact that something is broken from the end user and the recommendation to ignore them is based on the assumption that the only person who can fix it is the end user.

@hakusaro
Copy link
Contributor

hakusaro commented May 8, 2021

@pixeltrix at least in our experience, by anomalous clients, I mean it’s web crawlers with varying degrees of odd user agents (Java 6 HTTP clients, old unsupported browsers, etc). As far as we’re concerned the errors don’t actually match humans or users that we actually serve anyway (e.g., we aren’t going to build a site that somehow parses correctly in all of the esoteric UAs we get). A good portion of the clients that send this are trying to run SQL injection attacks and other automated fuzzing scan techniques. It doesn’t seem right to me that a clientside decision can create a serverside uncatchable exception in a world where we very explicitly teach people to not trust user input. It’s a MIME negotiation. If the other side isn’t sending compliant headers that’s kinda not something we can fix, right?

@pixeltrix
Copy link
Contributor

@hakusaro it's always catchable somewhere, just not in a controller since the mime parsing is triggered by params parsing (we need to know the content type to trigger the correct parser). You could add a custom middleware that catches the exception and does what you need it to do.

You're right that it's not something you can fix which is why we suggest ignoring it for exception monitoring services like Appsignal, Sentry and Bugsnag but it's still logged so you can go back and analyse things if necessary.

mysociety-pusher pushed a commit to mysociety/alaveteli that referenced this pull request Oct 8, 2021
This expcetion was added between Rails 5.2 and 6.0 [1] in #35604 [2]
and #40353.

Under Rails 6, in development a 406 HTTP status is returned:

    > curl -I -H "Content-Type: *" http://localhost:3000
    HTTP/1.1 406 Not Acceptable

In production, we also get a 406:

    > curl -I -H "Content-Type: *" https://www.whatdotheyknow.com/cy/request/merthy_road_whitchurch_cardiff
    HTTP/2 406

This appears to be the correct behaviour and in the PRs above the Rails
core team suggest ignoring these exceptions [4].

> You're right that it's not something you can fix which is why we
> suggest ignoring it for exception monitoring services like Appsignal,
> Sentry and Bugsnag but it's still logged so you can go back and
> analyse things if necessary.

Fixes #6553.

Hat tip to @gbp for the investigation and explanation included in this
commit message.

[1] https://github.com/rails/rails/blob/1b5658b562b29f8aab7e80cbab8adbd2ddea5447/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb#L15
[2] rails/rails#35604
[3] rails/rails#40353
[4] rails/rails#35604 (comment)
@fjl82
Copy link

fjl82 commented Feb 27, 2023

But why should we have to jump through hoops to filter out "error" messages that aren't actually errors? No one likes a monitoring system crying wolf. I don't see the need for this change at all. If a client is sending an invalid type, ignore it and move on. Set it to nil or some sane default, whatever.
I started getting these errors recently and ended up in this thread. It takes up my valuable time on effort to silence an "error" that is not even fixable. I don't want to write my own middleware or make complex configurations for something trivial like this. Errors on invalid mime types in a header we don't even use should be optional behaviour and be allowed to enabled/disabled with a simple configuration item (environments/*.rb?).

@fjl82
Copy link

fjl82 commented Feb 27, 2023

Another small addition: just found this: https://edgeguides.rubyonrails.org/configuring.html#config-exceptions-app
Apparently the situation is so bad that now the official rails guides recommend adding a custom exceptions_app to work around this specific "problem". I think the behaviour that they are suggesting should be the default behaviour: return an http error status and be quiet about it.

@jrochkind
Copy link
Contributor

jrochkind commented Feb 27, 2023

@fjl82 personally I would have liked to be able to ignore this completely too; making it it's own exception class was at least the first pre-requisite for making that possible. Currently via a custom exceptions_app or rescue_from, I guess are the methods I can think of.

I agree that a configuration might possibly be a further advance (I am not a rails committer and have no sway).

But: Compare to what happens when a user accesses a route that doesn't exist. This also is something that is an "error that isn't actually an error that you can't do anything about", right? But it still results in an ActionController::RoutingError exception. Which by default results in an http 404 status code being returned. I'm not sure how it shows up in the logs, but I think it might show up as an exception too? One that most have learned to ignore, perhaps?

This case seems very analogous to the ActionController::RoutingError -- I am not certain if they are handled completely analogously or not; I think they might be? If there is some way the "non-existing route" case is handled differently than the "invalid mime type" case with regard to how the error propagates and is handled, I think there'd be a good argument for a PR to make ActionDispatch::Http::MimeNegotiation::InvalidType be handled more similarly to ActionController::RoutingError; it seems they should be the same but for the by-default http status code in the response. I am not sure if they are or not.

@fjl82
Copy link

fjl82 commented Feb 27, 2023

I had actually forgotten about the RoutingError. This also annoyed me a lot when first starting out with rails (it was always a user error and nothing I could help). But the difference with the RoutingError is that I was able to get rid of these with a small snippet in the routes.rb that responds to all unknown routes with a 404. This is part of the rails system and fixable within your application. This InvalidType error happens outside of your rails application, hence the need for rack middleware "magic".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants