rails icon indicating copy to clipboard operation
rails copied to clipboard

Add `redirect_code_for_unsafe_http_methods` config

Open jonathanhefner opened this issue 1 year ago • 2 comments

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

jonathanhefner avatar Jun 17 '22 22:06 jonathanhefner

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?

matthewd avatar Jun 21 '22 06:06 matthewd

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.

jonathanhefner avatar Jun 21 '22 17:06 jonathanhefner

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?

ghiculescu avatar Feb 26 '24 06:02 ghiculescu