Skip to content

Commit

Permalink
change request method to a GET when passing failed requests to `con…
Browse files Browse the repository at this point in the history
…fig.exceptions_app`

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.
  • Loading branch information
agrobbin committed Dec 17, 2020
1 parent faa27dd commit cb23580
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 0 deletions.
4 changes: 4 additions & 0 deletions actionpack/CHANGELOG.md
@@ -1,5 +1,9 @@
## Unreleased

* Change the request method to a `GET` when passing failed requests down to `config.exceptions_app`.

*Alex Robbin*

* Add `redirect_back_or_to(fallback_location, **)` as a more aesthetically pleasing version of `redirect_back fallback_location:, **`.
The old method name is retained without explicit deprecation.

Expand Down
2 changes: 2 additions & 0 deletions actionpack/lib/action_dispatch/middleware/show_exceptions.rb
Expand Up @@ -46,7 +46,9 @@ def render_exception(request, exception)
status = wrapper.status_code
request.set_header "action_dispatch.exception", wrapper.unwrapped_exception
request.set_header "action_dispatch.original_path", request.path_info
request.set_header "action_dispatch.original_request_method", request.raw_request_method
request.path_info = "/#{status}"
request.request_method = "GET"
response = @exceptions_app.call(request.env)
response[1]["X-Cascade"] == "pass" ? pass_response(status) : response
rescue Exception => failsafe_error
Expand Down
14 changes: 14 additions & 0 deletions railties/test/application/middleware/exceptions_test.rb
Expand Up @@ -51,6 +51,20 @@ def index
assert_equal 405, last_response.status
end

test "renders unknown http methods as 405 when routes are used as the custom exceptions app" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
end
RUBY

add_to_config "config.exceptions_app = self.routes"

app.config.action_dispatch.show_exceptions = true

request "/", { "REQUEST_METHOD" => "NOT_AN_HTTP_METHOD" }
assert_equal 405, last_response.status
end

test "uses custom exceptions app" do
add_to_config <<-RUBY
config.exceptions_app = lambda do |env|
Expand Down

0 comments on commit cb23580

Please sign in to comment.