Add `redirect_code_for_unsafe_http_methods` config #45393

jonathanhefner · 2022-06-17T22:36:00Z

When a client follows an HTTP 302 redirect, it will typically use the same HTTP method as the original request. This can cause issues when, for example, redirecting an XHR DELETE request after a destroy. (Note that many clients make an exception for POST, and will use GET to follow a 302 redirect in that case.)

The solution so far has been for users to specify an explicit response code in such cases. For example:

def destroy
  Post.destroy(params[:id])
  redirect_to root_path, status: 303
end

This commit adds a redirect_code_for_unsafe_http_methods configuration setting that allows users to specify a default HTTP response code to use when redirecting a request made with an unsafe HTTP method, such as POST or DELETE. For example, when set to 303, the explicit response code may be omitted:

def destroy
  Post.destroy(params[:id])
  redirect_to root_path
end

skipkayhil

I like this!

skipkayhil · 2022-06-17T23:22:03Z

actionpack/lib/action_controller/metal/redirecting.rb

@@ -85,7 +86,7 @@ def redirect_to(options = {}, response_options = {})

      allow_other_host = response_options.delete(:allow_other_host) { _allow_other_host }

-      self.status        = _extract_redirect_to_status(options, response_options)
+      self.status        = _extract_redirect_to_status(request, options, response_options)


Does request need to be a param? (Won't it always be present on a Metal?)

I wrote it this way partly to explicitly list the factors that affect the result, and partly for parity with _compute_redirect_to_location(request, options) (which I assume is written that way for the same reason). But I don't have a strong preference.

byroot · 2022-06-18T07:14:46Z

actionpack/lib/action_controller/metal/redirecting.rb

@@ -11,6 +11,7 @@ class UnsafeRedirectError < StandardError; end

    included do
      mattr_accessor :raise_on_open_redirects, default: false
+      mattr_accessor :redirect_code_for_unsafe_http_methods, default: 302


mattr_accessor is rarely the right choice.

I think here might want to use class_attribute, so that we can set the default on AC::Base, and then refine on a per controller basis if needed.

I think here might want to use class_attribute, so that we can set the default on AC::Base, and then refine on a per controller basis if needed.

Excellent point! That would make it much easier to migrate an app one piece at a time.

mattr_accessor is rarely the right choice.

For clarity, would you say that the order of preference is:

singleton_class.attr_accessor - when the scope is exactly one class

class_attribute - when the scope is a class hierarchy, allowing value inheritance

mattr_accessor - when the scope is global

Aside: do you feel like :raise_on_open_redirects would be better as class_attribute instead of mattr_accessor?

For clarity, would you say that the order of preference is

singleton_class.attr_accessor - when it's exactly one class, or more frequently, when it's single global.

class_attribute - when the scope is a class hierarchy, allowing value inheritance, and it's not meant to be changed too much at runtime (class_attributes writes are slow because it's basically a define_method).

mattr_accessor - honestly there's no reason to use it ever.

do you feel like :raise_on_open_redirects

It could make sense yes. As you mentioned it could allow to migrate on a per controller basis.

byroot · 2022-06-18T07:17:48Z

railties/lib/rails/application/configuration.rb

@@ -284,6 +284,7 @@ def load_defaults(target_version)

          if respond_to?(:action_controller)
            action_controller.allow_deprecated_parameters_hash_equality = false
+            action_controller.redirect_code_for_unsafe_http_methods = 303


Hum, we should be very careful with this new default. We need to really check what the browser support etc might be.

There might also be a ton of API clients etc checking response.status == 302. I do think it's cleaner, but I'm not sure we'll ever be able to switch the default for apps that upgrade (we could for new apps though).

We need to really check what the browser support etc might be.

If you mean support for HTTP status code 303, caniuse shows "unknown" for many older browser versions, but shows "supported" for the current version of all major browsers. I think "unknown" may be a quirk of how the data is aggregated. I found this Stack Overflow answer dated 2014 which indicates status code 303 is supported by Firefox 6+, Chrome 13+, Safari 5.1+, and — most importantly — Internet Explorer 6+.

There might also be a ton of API clients etc checking response.status == 302.

Sidenote: this PR was partly inspired by hotwired/turbo#84. I believe that checking status == 302 in the browser is not possible. hotwired/turbo#84 (comment) indicates that for the Fetch API, and another Stack Overflow answer indicates that for the XHR API. But it is indeed possible in non-browser clients.

I'm not sure we'll ever be able to switch the default for apps that upgrade (we could for new apps though).

How do you envision implementing that? Do you know of an example?

One way would be to modify the app generator templates, e.g. add a line to application_controller.rb.tt. But that feels like configuration over convention.

Another way would be for rails app:update to check if config.load_defaults < 7.1, and, if so, generate an initializer that (re)sets redirect_code_for_unsafe_http_methods = 302.

But it is indeed possible in non-browser clients.

Yeah, that's what I meant. Web scrapers as well certainly.

How do you envision implementing that?

With a template, the question is which one. I was thinking application.rb.tt, but it can be debated. I just don't think it can ever be in load_defaults as it would break too many non-browser clients.

The version-specific settings in load_defaults are specifically for new apps.

rails app:update to [..] generate an initializer that (re)sets redirect_code_for_unsafe_http_methods = 302.

That's what new_framework_defaults_*.rb is.

This PR is already doing the thing you're describing. 😄

The version-specific settings in load_defaults are specifically for new apps.

I guess I wasn't clear. Yes load_defaults is for new apps, but upgrading apps are expected to flip load_defauts after a while.

Ensuring that flipping this config won't break any client is such an herculean task that I don't think we can expect users to do it.

Ah, fair enough.

I guess I've never anticipated that the average application would bump the load_defaults line at all... but even assuming that's the case, I think it's generally true that in order to do so, you need to consider each of the generated "new defaults", and choose either to adopt them or to explicitly retain the "old" behaviour.

I definitely don't see this as a short-term transitional option that we'd remove later: broadly-cliented existing applications (👋🏻) will likely want to keep their old behaviour for the foreseeable future, as you say. But I'm not so sure that's more generally--or uniquely--true: acknowledging that any change can cause bumps for overly-particular clients (see e.g. our recent dropping of the response body in redirects, which had similar danger, but we didn't even option), I suspect that the vast majority of apps would be able to change this either without consequence, or with a handful of clients that needed to [and could] Just Catch Up.

I'm fine with even e.g. giving it an explicit call-out in the upgrading guide, if you think that might help? I guess my counterbalancing thought is just that ~all of the "new defaults" are things we've consciously decided are too radical of a change for us to drop them on existing applications, so I'm reluctant to give this so many warning lights that we implicitly suggest it's safe to flip others. 🤷🏻‍♂️

(But also, to my point in the other top-level comment... if we exclude POST, I suspect this gets a lot safer for a lot more apps to just roll with.)

I guess I've never anticipated that the average application would bump the load_defaults line at all...

I may be wrong but for me the process was always that you'd carefully uncoment new_framework_defaults, and then once it's all done you delete it and bump the load_defaults (and merge any setting you don't want into application.rb).

Otherwise you loose the consistency benefit of using a framework if all your apps have different defaults hence behave in different ways.

giving it an explicit call-out in the upgrading guide, if you think that might help?

I suppose, and a LOUD comment in the new_framework_defaults.

a LOUD comment in the new_framework_defaults.

"Note that changing this setting on a previously-deployed application could easily break any existing non-browser HTTP clients that expect the old 302 response code."?

I think that's already scarier than the wording we used when we changed the cookie serializer, for example, and I'd argue that was much more dangerous (and harder to roll back)

Yeah, but the cookie it's your own app talking to itself. Here it's potentially 3rd party things you can't see the code of. If it breaks it may take a while before you realize it, hence why I'm much more worried.

rails app:update to [..] generate an initializer that (re)sets redirect_code_for_unsafe_http_methods = 302.

That's what new_framework_defaults_*.rb is.

Well, more like the inverse.

Currently, new_framework_defaults_*.rb has # redirect_code_for_unsafe_http_methods = 303, and users must uncomment it, and eventually delete it after bumping config.load_defaults.

My suggestion was for something like legacy_framework_defaults.rb, which would have redirect_code_for_unsafe_http_methods = 302 (uncommented), and users would possibly never delete it. And it would only be generated if config.load_defaults was < 7.1 when rails app:update was run.

But I would also be happy with a loud callout in new_framework_defaults_*.rb, as you mentioned. In my opinion, either is preferable to generating extra config code for new apps.

matthewd · 2022-06-21T06:17:31Z

Note that many clients make an exception for POST, and will use GET to follow a 302 redirect in that case.

I think this is probably worth specific discussion.

The version of this change that I had pictured after seeing hotwired/turbo#84 (via #45383) had us retaining the legacy 302 status for "legacy HTTP actions", rather than safe/idempotent ones... which leaves us joining in the codification of existing non-conforming practice, but seems like it gives a decent reduction in potential surprise?

jonathanhefner · 2022-06-21T17:06:58Z

The version of this change that I had pictured after seeing hotwired/turbo#84 (via #45383) had us retaining the legacy 302 status for "legacy HTTP actions", rather than safe/idempotent ones... which leaves us joining in the codification of existing non-conforming practice, but seems like it gives a decent reduction in potential surprise?

One potential issue with excluding POST is that form helper forms always use POST. The intended HTTP method is then exposed via request_method. But since we are using method (which I believe is correct), a submitted form would get 302, but an API call or controller test would get 303.

When a client follows an HTTP 302 redirect, it will typically use the same HTTP method as the original request. This can cause issues when, for example, redirecting an XHR `DELETE` request after a `destroy`. (Note that many clients make an exception for `POST`, and will use `GET` to follow a 302 redirect in that case.) The solution so far has been for users to specify an explicit response code in such cases. For example: ```ruby def destroy Post.destroy(params[:id]) redirect_to root_path, status: 303 end ``` This commit adds a `redirect_code_for_unsafe_http_methods` configuration setting that allows users to specify a default HTTP response code to use when redirecting a request made with an [unsafe HTTP method][], such as `POST` or `DELETE`. For example, when set to 303, the explicit response code may be omitted: ```ruby def destroy Post.destroy(params[:id]) redirect_to root_path end ``` [unsafe HTTP method]: https://developer.mozilla.org/en-US/docs/Glossary/Safe/HTTP

…ode on redirect for the update action XHR requests other than GET or POST have issues when using 302 (e.g browsers trying to follow the redirect using the original request method resulting in double PATCH/PUT) This should be reverted when / if #45393 is merged

ghiculescu · 2024-02-26T06:37:24Z

I'd love to see this PR brought back to life. As part of rolling out Turbo Drive across a large app we are looking to monkey patch ApplicationController

  def redirect_to(options = {}, response_options = {})
      if options.is_a?(Hash)
        options[:status] = :see_other
      else
        response_options[:status] = :see_other
      end

    super(options, response_options)
  end

But given it's basically a requirement for using Hotwire properly, this feels like something Rails should support out of the box.

That said, if making it a default is controversial, could we just add the config and consider the default in a separate PR?

This PR checks that `render` and `redirect_to` are provided an explicit status code in response to [unsafe request methods](https://developer.mozilla.org/en-US/docs/Glossary/Safe/HTTP) (e.g. POST/PUT/PATCH/DELETE). #### `redirect_to` When redirecting a POST/PUT/PATCH/DELETE to a GET location, we should use 303, rather than a 302. A 302 means that the request method should not be altered, but: > Many web browsers implemented this code in a manner that violated this standard, changing the request type of the new request to [GET](https://en.wikipedia.org/wiki/HTTP_GET_request), regardless of the type employed in the original request (e.g. [POST](https://en.wikipedia.org/wiki/POST_(HTTP))).[[1]](https://en.wikipedia.org/wiki/HTTP_302#cite_note-1) For this reason, HTTP/1.1 (RFC [2616](https://datatracker.ietf.org/doc/html/rfc2616)) added the new status codes [303](https://en.wikipedia.org/wiki/HTTP_303) and [307](https://en.wikipedia.org/wiki/HTTP_307) to disambiguate between the two behaviours, with 303 mandating the change of request type to GET, and 307 preserving the request type as originally sent. > -- https://en.wikipedia.org/wiki/HTTP_302 Sinatra's [`redirect` method](https://github.com/sinatra/sinatra/blob/5640495babcb4cfd69ba650b293660b7446402da/lib/sinatra/base.rb#L307-L321) automatically uses 303 for non-GET requests. [This PR](rails/rails#45393) aims to do this for Rails applications. When autocorrect is enabled, `status: :see_other` will be added to redirects used in the `create`, `update`, and `delete` actions. #### `render` Calling `render` in the `create`, `update`, and `destroy` actions is almost always associated with error handling, so a 4xx status should be used. In the vast majority of cases, we should be using 422 Unprocessable Entity. ```ruby if something.save redirect_to something, status: :see_other else render :new, status: :unprocessable_entity end ``` There are exceptions, though. For example, sometimes you want to show a confirmation page that says "We've received your request". In which case, 201 or 202 might be a more appropriate choice. When autocorrect is enabled, `status: :unprocessable_entity` will be added to renders used in the `create`, `update`, and `delete` actions. This is a pretty unreasonable thing to do, but could help people update these usages en-masse. For this reason, Autocorrect is disabled by default for this rule.

rails-bot bot added actionpack docs railties labels Jun 17, 2022

skipkayhil reviewed Jun 17, 2022

View reviewed changes

byroot reviewed Jun 18, 2022

View reviewed changes

renanigt mentioned this pull request Jul 5, 2022

adds info about status code to the rails guides #45522

Closed

jonathanhefner force-pushed the add-redirect_code_for_unsafe_http_methods branch from 5df493d to badd696 Compare August 4, 2022 20:43

jonathanhefner force-pushed the add-redirect_code_for_unsafe_http_methods branch 2 times, most recently from 4e8b978 to 2147697 Compare August 12, 2022 21:49

p8 mentioned this pull request Oct 14, 2022

Scaffold destroy action returns status code 303 #45383

Merged

jonathanhefner force-pushed the add-redirect_code_for_unsafe_http_methods branch from 2147697 to 7384b65 Compare October 14, 2022 17:29

rzane mentioned this pull request Apr 9, 2024

Encourage the use of status codes for unsafe requests Betterment/betterlint#40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `redirect_code_for_unsafe_http_methods` config #45393

Add `redirect_code_for_unsafe_http_methods` config #45393

jonathanhefner commented Jun 17, 2022

skipkayhil left a comment

skipkayhil Jun 17, 2022

jonathanhefner Jun 20, 2022

byroot Jun 18, 2022

jonathanhefner Jun 20, 2022

byroot Jun 21, 2022

byroot Jun 18, 2022

jonathanhefner Jun 20, 2022 •

edited

byroot Jun 21, 2022

matthewd Jun 21, 2022

byroot Jun 21, 2022

matthewd Jun 21, 2022

byroot Jun 21, 2022

matthewd Jun 21, 2022

byroot Jun 21, 2022

jonathanhefner Jun 21, 2022

matthewd commented Jun 21, 2022

jonathanhefner commented Jun 21, 2022

ghiculescu commented Feb 26, 2024 •

edited

Add redirect_code_for_unsafe_http_methods config #45393

Are you sure you want to change the base?

Add redirect_code_for_unsafe_http_methods config #45393

Conversation

jonathanhefner commented Jun 17, 2022

skipkayhil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanhefner Jun 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewd commented Jun 21, 2022

jonathanhefner commented Jun 21, 2022

ghiculescu commented Feb 26, 2024 • edited

Add `redirect_code_for_unsafe_http_methods` config #45393

Add `redirect_code_for_unsafe_http_methods` config #45393

jonathanhefner Jun 20, 2022 •

edited

ghiculescu commented Feb 26, 2024 •

edited