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

Quietly handle unknown HTTP methods in Action Dispatch SSL middleware #40930

Merged

Conversation

agrobbin
Copy link
Contributor

@agrobbin agrobbin commented Dec 25, 2020

Because ActionDispatch::SSL is included higher up in the middleware stack than ActionDispatch::ShowExceptions, it should ideally not be raising any exceptions.

In this case, ActionDispatch::Request#{get,head}? are called, which check if the HTTP method is valid. If it isn't, ActionController::UnknownHttpMethod is raised. Instead of calling the Rack-provided predicate methods, we leverage raw_request_method.

This is somewhat related to #40834.

@rails-bot rails-bot bot added the actionpack label Dec 25, 2020
@agrobbin agrobbin force-pushed the action-dispatch-ssl-raw-request-method branch from a40ae3d to 1dd53a9 Compare December 25, 2020 03:37
Because `ActionDispatch::SSL` is included higher up in the middleware stack than `ActionDispatch::ShowExceptions`, it should ideally not be raising any exceptions.

In this case, `ActionDispatch::Request#{get,head}?` are called, which check if the HTTP method is valid. If it isn't, `ActionController::UnknownHttpMethod` is raised. Instead of calling the Rack-provided predicate methods, we leverage `raw_request_method`.
@agrobbin agrobbin force-pushed the action-dispatch-ssl-raw-request-method branch from 1dd53a9 to ea40dd3 Compare December 28, 2020 12:28
@rafaelfranca rafaelfranca merged commit 2fdc985 into rails:master Dec 28, 2020
rafaelfranca added a commit that referenced this pull request Dec 28, 2020
…st-method

Quietly handle unknown HTTP methods in Action Dispatch SSL middleware
@agrobbin
Copy link
Contributor Author

Thanks @rafaelfranca! Is it possible to also get this backported to 6-1-stable?

@agrobbin
Copy link
Contributor Author

Wow nevermind, you already did that, thanks again!

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

2 participants