Merge pull request #22826 from timrogers/actiondispatch-ssl-config

Configurable redirect and secure cookies for ActionDispatch::SSL

4 files changed, 51 insertions, 25 deletions

diff --git a/actionpack/lib/action_dispatch/middleware/ssl.rb b/actionpack/lib/action_dispatch/middleware/ssl.rb
index 8f8f1bab8b..735b5939dd 100644
--- a/actionpack/lib/action_dispatch/middleware/ssl.rb
+++ b/actionpack/lib/action_dispatch/middleware/ssl.rb
@@ -4,16 +4,18 @@ module ActionDispatch
   # requests:
   #
   #   1. TLS redirect: Permanently redirects http:// requests to https://
-  #      with the same URL host, path, etc. This is always enabled. Set
-  #      `config.ssl_options` to modify the destination URL
-  #      (e.g. `redirect: { host: "secure.widgets.com", port: 8080 }`)
+  #      with the same URL host, path, etc. Enabled by default. Set `config.ssl_options`
+  #      to modify the destination URL
+  #      (e.g. `redirect: { host: "secure.widgets.com", port: 8080 }`), or set
+  #      `redirect: false` to disable this feature.
   #
   #   2. Secure cookies: Sets the `secure` flag on cookies to tell browsers they
-  #      mustn't be sent along with http:// requests. This is always enabled.
+  #      mustn't be sent along with http:// requests. Enabled by default. Set
+  #      `config.ssl_options` with `secure_cookies: false` to disable this feature.
   #
   #   3. HTTP Strict Transport Security (HSTS): Tells the browser to remember
   #      this site as TLS-only and automatically redirect non-TLS requests.
-  #      Enabled by default. Pass `hsts: false` to disable.
+  #      Enabled by default. Configure `config.ssl_options` with `hsts: false` to disable.
   #
   # Set `config.ssl_options` with `hsts: { … }` to configure HSTS:
   #   * `expires`: How long, in seconds, these settings will stick. Defaults to
@@ -41,7 +43,7 @@ module ActionDispatch
       { expires: HSTS_EXPIRES_IN, subdomains: false, preload: false }
     end
 
-    def initialize(app, redirect: {}, hsts: {}, **options)
+    def initialize(app, redirect: {}, hsts: {}, secure_cookies: true, **options)
       @app = app
 
       if options[:host] || options[:port]
@@ -54,6 +56,7 @@ module ActionDispatch
         @redirect = redirect
       end
 
+      @secure_cookies = secure_cookies
       @hsts_header = build_hsts_header(normalize_hsts_options(hsts))
     end
 
@@ -63,10 +66,11 @@ module ActionDispatch
       if request.ssl?
         @app.call(env).tap do |status, headers, body|
           set_hsts_header! headers
-          flag_cookies_as_secure! headers
+          flag_cookies_as_secure! headers if @secure_cookies
         end
       else
-        redirect_to_https request
+        return redirect_to_https request if @redirect
+        @app.call(env)
       end
     end
 
diff --git a/actionpack/test/dispatch/ssl_test.rb b/actionpack/test/dispatch/ssl_test.rb
index 7a5b8393dc..11251c86e7 100644
--- a/actionpack/test/dispatch/ssl_test.rb
+++ b/actionpack/test/dispatch/ssl_test.rb
@@ -12,25 +12,31 @@ class SSLTest < ActionDispatch::IntegrationTest
 end
 
 class RedirectSSLTest < SSLTest
-  def assert_not_redirected(url, headers: {})
-    self.app = build_app
+
+  def assert_not_redirected(url, headers: {}, redirect: {}, deprecated_host: nil,
+    deprecated_port: nil)
+
+    self.app = build_app ssl_options: { redirect: redirect,
+      host: deprecated_host, port: deprecated_port
+    }
+
     get url, headers: headers
     assert_response :ok
   end
 
-  def assert_redirected(host: nil, port: nil, status: 301, body: [],
-    deprecated_host: nil, deprecated_port: nil,
+  def assert_redirected(redirect: {}, deprecated_host: nil, deprecated_port: nil,
     from: 'http://a/b?c=d', to: from.sub('http', 'https'))
 
-    self.app = build_app ssl_options: {
-      redirect: { host: host, port: port, status: status, body: body },
+    redirect = { status: 301, body: [] }.merge(redirect)
+
+    self.app = build_app ssl_options: { redirect: redirect,
       host: deprecated_host, port: deprecated_port
     }
 
     get from
-    assert_response status
+    assert_response redirect[:status] || 301
     assert_redirected_to to
-    assert_equal body.join, @response.body
+    assert_equal redirect[:body].join, @response.body
   end
 
   test 'https is not redirected' do
@@ -46,31 +52,31 @@ class RedirectSSLTest < SSLTest
   end
 
   test 'redirect with non-301 status' do
-    assert_redirected status: 307
+    assert_redirected redirect: { status: 307 }
   end
 
   test 'redirect with custom body' do
-    assert_redirected body: ['foo']
+    assert_redirected redirect: { body: ['foo'] }
   end
 
   test 'redirect to specific host' do
-    assert_redirected host: 'ssl', to: 'https://ssl/b?c=d'
+    assert_redirected redirect: { host: 'ssl' }, to: 'https://ssl/b?c=d'
   end
 
   test 'redirect to default port' do
-    assert_redirected port: 443
+    assert_redirected redirect: { port: 443 }
   end
 
   test 'redirect to non-default port' do
-    assert_redirected port: 8443, to: 'https://a:8443/b?c=d'
+    assert_redirected redirect: { port: 8443 }, to: 'https://a:8443/b?c=d'
   end
 
   test 'redirect to different host and non-default port' do
-    assert_redirected host: 'ssl', port: 8443, to: 'https://ssl:8443/b?c=d'
+    assert_redirected redirect: { host: 'ssl', port: 8443 }, to: 'https://ssl:8443/b?c=d'
   end
 
   test 'redirect to different host including port' do
-    assert_redirected host: 'ssl:443', to: 'https://ssl:443/b?c=d'
+    assert_redirected redirect: { host: 'ssl:443' }, to: 'https://ssl:443/b?c=d'
   end
 
   test ':host is deprecated, moved within redirect: { host: … }' do
@@ -84,6 +90,10 @@ class RedirectSSLTest < SSLTest
       assert_redirected deprecated_port: 1, to: 'https://a:1/b?c=d'
     end
   end
+
+  test 'no redirect with redirect set to false' do
+    assert_not_redirected 'http://example.org', redirect: false
+  end
 end
 
 class StrictTransportSecurityTest < SSLTest
@@ -187,6 +197,11 @@ class SecureCookiesTest < SSLTest
     assert_cookies 'problem=def; path=/; Secure; HttpOnly'
   end
 
+  def test_cookies_as_not_secure_with_secure_cookies_disabled
+    get headers: { 'Set-Cookie' => DEFAULT }, ssl_options: { secure_cookies: false }
+    assert_cookies *DEFAULT.split("\n")
+  end
+
   def test_no_cookies
     get
     assert_nil response.headers['Set-Cookie']
diff --git a/railties/lib/rails/application/default_middleware_stack.rb b/railties/lib/rails/application/default_middleware_stack.rb
index ed6a1f82d3..4f1cc0703d 100644
--- a/railties/lib/rails/application/default_middleware_stack.rb
+++ b/railties/lib/rails/application/default_middleware_stack.rb
@@ -68,7 +68,7 @@ module Rails
           middleware.use ::ActionDispatch::Cookies unless config.api_only
 
           if !config.api_only && config.session_store
-            if config.force_ssl && !config.session_options.key?(:secure)
+            if config.force_ssl && config.ssl_options.fetch(:secure_cookies, true) && !config.session_options.key?(:secure)
               config.session_options[:secure] = true
             end
             middleware.use config.session_store, config.session_options
diff --git a/railties/test/application/middleware/session_test.rb b/railties/test/application/middleware/session_test.rb
index 25eadfc387..f847e80471 100644
--- a/railties/test/application/middleware/session_test.rb
+++ b/railties/test/application/middleware/session_test.rb
@@ -20,12 +20,19 @@ module ApplicationTests
       @app ||= Rails.application
     end
 
-    test "config.force_ssl sets cookie to secure only" do
+    test "config.force_ssl sets cookie to secure only by default" do
       add_to_config "config.force_ssl = true"
       require "#{app_path}/config/environment"
       assert app.config.session_options[:secure], "Expected session to be marked as secure"
     end
 
+    test "config.force_ssl doesn't set cookie to secure only when changed from default" do
+      add_to_config "config.force_ssl = true"
+      add_to_config "config.ssl_options = { secure_cookies: false }"
+      require "#{app_path}/config/environment"
+      assert !app.config.session_options[:secure]
+    end
+
     test "session is not loaded if it's not used" do
       make_basic_app