Change default HTTP status to 308 for ActionDispatch::SSL.

308 status code introduced in https://tools.ietf.org/html/rfc7538
preserves the request method unlike 301 status code which would convert
POST requests to GET.

actionpack/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Change default redirection status code for non-GET/HEAD requests to 308 Permanent Redirect for `ActionDispatch::SSL`.
+
+    *Alan Tan*, *Oz Ben-David*
+
 *   Fix `follow_redirect!` to follow redirection with same HTTP verb when following
     a 308 redirection.
 

actionpack/lib/action_dispatch/middleware/ssl.rb

@@ -56,7 +56,7 @@ module ActionDispatch
       { expires: HSTS_EXPIRES_IN, subdomains: true, preload: false }
     end
 
-    def initialize(app, redirect: {}, hsts: {}, secure_cookies: true)
+    def initialize(app, redirect: {}, hsts: {}, secure_cookies: true, ssl_default_redirect_status: nil)
       @app = app
 
       @redirect = redirect
@@ -65,6 +65,7 @@ module ActionDispatch
       @secure_cookies = secure_cookies
 
       @hsts_header = build_hsts_header(normalize_hsts_options(hsts))
+      @ssl_default_redirect_status = ssl_default_redirect_status
     end
 
     def call(env)
@@ -132,6 +133,8 @@ module ActionDispatch
       def redirection_status(request)
         if request.get? || request.head?
           301 # Issue a permanent redirect via a GET request.
+        elsif @ssl_default_redirect_status
+          @ssl_default_redirect_status
         else
           307 # Issue a fresh request redirect to preserve the HTTP method.
         end

actionpack/test/dispatch/ssl_test.rb

@@ -21,7 +21,7 @@ class RedirectSSLTest < SSLTest
   end
 
   def assert_redirected(redirect: {}, from: "http://a/b?c=d", to: from.sub("http", "https"))
-    redirect = { status: 301, body: [] }.merge(redirect)
+    redirect = { body: [] }.merge(redirect)
 
     self.app = build_app ssl_options: { redirect: redirect }
 
@@ -64,8 +64,22 @@ class RedirectSSLTest < SSLTest
     assert_post_redirected
   end
 
-  test "redirect with non-301 status" do
-    assert_redirected redirect: { status: 307 }
+  test "redirect with custom status" do
+    assert_redirected redirect: { status: 308 }
+  end
+
+  test "redirect with ssl_default_redirect_status" do
+    self.app = build_app(ssl_options: { ssl_default_redirect_status: 308 })
+
+    get "http://a/b?c=d"
+
+    assert_response 301
+    assert_redirected_to "https://a/b?c=d"
+
+    post "http://a/b?c=d"
+
+    assert_response 308
+    assert_redirected_to "https://a/b?c=d"
   end
 
   test "redirect with custom body" do

guides/source/configuring.md

@@ -628,6 +628,11 @@ Defaults to `'signed cookie'`.
   value of the `SameSite` attribute when setting cookies. Defaults to `nil`,
   which means the `SameSite` attribute is not added.
 
+* `config.action_dispatch.ssl_default_redirect_status` configures the default
+  HTTP status code used when redirecting non-GET/HEAD requests from HTTP to HTTPS
+  in the `ActionDispatch::SSL` middleware. Defaults to `308` as defined in
+  https://tools.ietf.org/html/rfc7538.
+
 * `ActionDispatch::Callbacks.before` takes a block of code to run before the request.
 
 * `ActionDispatch::Callbacks.after` takes a block of code to run after the request.

guides/source/upgrading_ruby_on_rails.md

@@ -176,6 +176,10 @@ This change is backwards compatible for the majority of applications, in which c
 
 Technically, however, controllers could configure `helpers_path` to point to a directoy in `$LOAD_PATH` that was not in the autoload paths. That use case is no longer supported out of the box. If the helper module is not autoloadable, the application is responsible for loading it before calling `helper`.
 
+### Redirection to HTTPS from HTTP will now use the 308 HTTP status code
+
+The default HTTP status code used in `ActionDispatch::SSL` when redirecting non-GET/HEAD requests from HTTP to HTTPS has been changed to `308` as defined in https://tools.ietf.org/html/rfc7538.
+
 Upgrading from Rails 5.2 to Rails 6.0
 -------------------------------------
 

railties/lib/rails/application/configuration.rb

@@ -175,6 +175,7 @@ module Rails
 
           if respond_to?(:action_dispatch)
             action_dispatch.cookies_same_site_protection = :lax
+            action_dispatch.ssl_default_redirect_status = 308
           end
 
           if respond_to?(:action_controller)

railties/lib/rails/application/default_middleware_stack.rb

@@ -16,7 +16,8 @@ module Rails
           middleware.use ::ActionDispatch::HostAuthorization, config.hosts, config.action_dispatch.hosts_response_app
 
           if config.force_ssl
-            middleware.use ::ActionDispatch::SSL, **config.ssl_options
+            middleware.use ::ActionDispatch::SSL, **config.ssl_options,
+              ssl_default_redirect_status: config.action_dispatch.ssl_default_redirect_status
           end
 
           middleware.use ::Rack::Sendfile, config.action_dispatch.x_sendfile_header

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_1.rb.tt

@@ -34,3 +34,7 @@
 # Specify whether `ActiveSupport::TimeZone.utc_to_local` returns a time with an
 # UTC offset or a UTC time.
 # ActiveSupport.utc_to_local_returns_utc_offset_times = true
+
+# Change default HTTP status code to `308` when redirecting non-GET/HEAD requets
+# to HTTPS in `ActionDispatch::SSL` middleware.
+# Rails.application.config.action_dispatch.ssl_default_redirect_status = 308

railties/test/application/configuration_test.rb

@@ -2347,6 +2347,28 @@ module ApplicationTests
       assert_equal :lax, Rails.application.config.action_dispatch.cookies_same_site_protection
     end
 
+    test "Rails.application.config.action_dispatch.ssl_default_redirect_status is 308 in 6.1 defaults" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config 'config.load_defaults "6.1"'
+
+      app "production"
+
+      assert_equal 308, Rails.application.config.action_dispatch.ssl_default_redirect_status
+    end
+
+    test "Rails.application.config.action_dispatch.ssl_default_redirect_status can be configured in an initializer" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config 'config.load_defaults "6.0"'
+
+      app_file "config/initializers/new_framework_defaults_6_1.rb", <<-RUBY
+        Rails.application.config.action_dispatch.ssl_default_redirect_status = 308
+      RUBY
+
+      app "production"
+
+      assert_equal 308, Rails.application.config.action_dispatch.ssl_default_redirect_status
+    end
+
     test "ActiveSupport.utc_to_local_returns_utc_offset_times is true in 6.1 defaults" do
       remove_from_config '.*config\.load_defaults.*\n'
       add_to_config 'config.load_defaults "6.1"'

railties/test/application/middleware_test.rb

@@ -145,7 +145,7 @@ module ApplicationTests
       add_to_config "config.ssl_options = { redirect: { host: 'example.com' } }"
       boot!
 
-      assert_equal [{ redirect: { host: "example.com" } }], Rails.application.middleware[2].args
+      assert_equal [{ redirect: { host: "example.com" }, ssl_default_redirect_status: 308 }], Rails.application.middleware[2].args
     end
 
     test "removing Active Record omits its middleware" do