Change request method to a GET when passing failed requests to config.exceptions_app

[agrobbin]

Similar to #38998 (fixed in #40246), HTTP method validation occurring whenever methods are called on ActionDispatch::Request can cause some weird unintended consequences. For example, if config.exceptions_app = self.routes, you get an exception raised via the ActionDispatch::ShowExceptions middleware failsafe:

Started TEST "/" for 127.0.0.1 at 2020-11-05 15:40:31 -0500
   (1.0ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
TEST, accepted HTTP methods are OPTIONS, GET, HEAD, POST, PUT, DELETE, TRACE, CONNECT, PROPFIND, PROPPATCH, MKCOL, COPY, MOVE, LOCK, UNLOCK, VERSION-CONTROL, REPORT, CHECKOUT, CHECKIN, UNCHECKOUT, MKWORKSPACE, UPDATE, LABEL, MERGE, BASELINE-CONTROL, MKACTIVITY, ORDERPATCH, ACL, SEARCH, MKCALENDAR, and PATCH excluded from capture: DSN not set

ActionController::UnknownHttpMethod (TEST, accepted HTTP methods are OPTIONS, GET, HEAD, POST, PUT, DELETE, TRACE, CONNECT, PROPFIND, PROPPATCH, MKCOL, COPY, MOVE, LOCK, UNLOCK, VERSION-CONTROL, REPORT, CHECKOUT, CHECKIN, UNCHECKOUT, MKWORKSPACE, UPDATE, LABEL, MERGE, BASELINE-CONTROL, MKACTIVITY, ORDERPATCH, ACL, SEARCH, MKCALENDAR, and PATCH):

actionpack (6.0.3.4) lib/action_dispatch/http/request.rb:431:in `check_method'
actionpack (6.0.3.4) lib/action_dispatch/http/request.rb:143:in `request_method'
rack (2.2.3) lib/rack/request.rb:187:in `head?'
actionpack (6.0.3.4) lib/action_dispatch/journey/router.rb:113:in `find_routes'
actionpack (6.0.3.4) lib/action_dispatch/journey/router.rb:32:in `serve'
actionpack (6.0.3.4) lib/action_dispatch/routing/route_set.rb:834:in `call'
Error during failsafe response: TEST, accepted HTTP methods are OPTIONS, GET, HEAD, POST, PUT, DELETE, TRACE, CONNECT, PROPFIND, PROPPATCH, MKCOL, COPY, MOVE, LOCK, UNLOCK, VERSION-CONTROL, REPORT, CHECKOUT, CHECKIN, UNCHECKOUT, MKWORKSPACE, UPDATE, LABEL, MERGE, BASELINE-CONTROL, MKACTIVITY, ORDERPATCH, ACL, SEARCH, MKCALENDAR, and PATCH
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/http/request.rb:431:in `check_method'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/http/request.rb:143:in `request_method'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rack-2.2.3/lib/rack/request.rb:187:in `head?'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/journey/router.rb:113:in `find_routes'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/journey/router.rb:32:in `serve'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/routing/route_set.rb:834:in `call'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/middleware/show_exceptions.rb:50:in `render_exception'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/middleware/show_exceptions.rb:36:in `rescue in call'
  /usr/local/var/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/actionpack-6.0.3.4/lib/action_dispatch/middleware/show_exceptions.rb:31:in `call'
  # ...

Now, to prevent the redundant exception, we overwrite request_method before passing env down to config.exceptions_app. action_dispatch.original_request_method is set to keep the original request method available for inspection.

[agrobbin]

@eugeneius it'd be awesome to get your review of this, as you're the one who reviewed #40246.

[rafaelfranca]

Can we do this without adding a new middleware? We try to not grow the list of middleware that the framework includes by default.

[rafaelfranca]

It looks like the ShowExceptions should just be able to handle this kind of exception instead of raising it.

[agrobbin]

@rafaelfranca hmm, interesting! What do you think about leveraging ActionDispatch::Callbacks? Looking at the default middleware stack, that appears to be a place that makes sense logically and shouldn't be terribly complicated to hook into. it also should allow us to keep ShowExceptions ignorant of the kind of exception raised, which would be great!

[rafaelfranca]

After a second thought, this is responsibility of the exception app. If it is calling methods that can raise it should know how to deal with the exception. I understand that it does means that you can't pass directly the rails routes as the exception application, but in my opinion this makes sense. The rails routes already raised this same exception and now we are trying to recover from it, if you pass the request to the same application that first raised the exception you should expect the same exception to happen again.

What we can do that I think it makes more sense is always use GET as the method of the request sent to the exception application. This is the behavior most users expect anyway.

[agrobbin]

@rafaelfranca I was trying to prevent routes from raising this exception at all, to not require anything special within ShowExceptions (or exceptions_app), letting everything work cleanly for those who are doing config.exceptions_app = self.routes. Without doing this in a middleware, though, that's pretty difficult (I did a quick spike on using ActionDispatch::Callbacks, and that was a no-go since env is not available in the callback).

Are you thinking of something like this in ShowExceptions?

request.set_header "action_dispatch.original_request_method", request.raw_request_method
request.request_method = "GET"

[rafaelfranca]

Are you thinking of something like this in ShowExceptions

Yes. With that, the original method is stored, so the exception app can use "action_dispatch.original_request_method" is they want to do something specific for a method.

[agrobbin]

@rafaelfranca thanks for the feedback. Just pushed an update!

[agrobbin]

Thanks @rafaelfranca!! Is this something that can/should be backported to 6-0-stable, to resolve the bug with config.exceptions_app = self.routes?

[rafaelfranca]

I don't feel confident on backporting this change to any stable release since this is a change in behavior. I'm not sure if anyone will notice though, so I'll backport to 6-1-stable. 6-0-stable is already in maintenance mode so I don't think it would be good to backport to it.

[agrobbin]

That makes sense, thanks again!

[jpwhitaker]

I think this change borked my app, we have an API and a web view, and previously a PATCH responding with 404. We have a line in routes.rb that picks up '/404' and sends it to method that redirects to dashboard. So now instead of a 404 it gets changed to GET, redirects and returns 302.

What should I do to handle this correctly?

[ghiculescu]

@jpwhitaker could you show a bit more detail on how to replicate? If a controller responds with a 404 status, I don't think it should go through ShowExceptions so I'm surprised your /404 route is picking it up.

In terms of how much detail is needed to help fix the bug, I found the replication steps in #43094 super helpful.