Make fetching insecure session fallback configurable

This change allows the disabling of fallback used to access old,
insecure sessions, and rewrite them as secure sessions. The fallback was
originally added as part of the mitigation of CVE-2019-25025 several
years back.

However, this fallback mechanism was added over 5 years ago. In many
cases, or at least in our case, the expiry on old, insecure, sessions
has long since passed. We'd like the ability to disable the fallback
entirely as it will never be a valid path for us.

See: rails#151

Also, we had to improve our patch for
`ActionDispatch::Assertions::RoutingAssertions::WithIntegrationRouting`
to handle middleware correctly. This is the same implementation as was
added in Rails 8.0.

See: rails/rails#54705

Author: tmandke

README.md

@@ -38,6 +38,13 @@ been updated in the last 30 days. The 30 days cutoff can be changed using the
 Configuration
 --------------
 
+Disable fallback to use insecure session by providing the option
+`secure_session_only` when setting up the session store.
+
+```ruby
+Rails.application.config.session_store :active_record_store, key: '_my_app_session', secure_session_only: true
+```
+
 The default assumes a `sessions` table with columns:
 
 *  `id` (numeric primary key),

lib/action_dispatch/session/active_record_store.rb

@@ -66,6 +66,11 @@ def initialize(app, options = {})
         super
       end
 
+      def initialize(app, options = {})
+        @secure_session_only = options.delete(:secure_session_only) { false }
+        super(app, options)
+      end
+
     private
       def get_session(request, sid)
         logger.silence do
@@ -142,7 +147,7 @@ def get_session_with_fallback(sid)
         if sid && !self.class.private_session_id?(sid.public_id)
           if (secure_session = session_class.find_by_session_id(sid.private_id))
             secure_session
-          elsif (insecure_session = session_class.find_by_session_id(sid.public_id))
+          elsif !@secure_session_only && (insecure_session = session_class.find_by_session_id(sid.public_id))
             insecure_session.session_id = sid.private_id # this causes the session to be secured
             insecure_session
           end

test/action_controller_test.rb

@@ -330,6 +330,30 @@ def test_session_store_with_all_domains
       end
     end
 
+    define_method :"test_unsecured_sessions_are_ignored_when_insecure_fallback_is_disabled_#{class_name}" do
+      with_store(class_name) do
+        session_options(secure_session_only: true)
+        with_test_route_set do
+          get '/set_session_value', params: { foo: 'baz' }
+          assert_response :success
+          public_session_id = cookies['_session_id']
+
+          session = ActiveRecord::SessionStore::Session.last
+          session.data # otherwise we cannot save
+          session.session_id = public_session_id
+          session.save!
+
+          get '/get_session_value'
+          assert_response :success
+
+          session.reload
+          new_session = ActiveRecord::SessionStore::Session.last
+          assert_not_equal public_session_id, new_session.session_id
+          assert_not_equal session.session_id, new_session.session_id
+        end
+      end
+    end
+
     # to avoid a different kind of timing attack
     define_method :"test_sessions_cannot_be_retrieved_by_their_private_session_id_for_#{class_name}" do
       with_store(class_name) do

test/helper.rb

@@ -102,12 +102,14 @@ def with_store(class_name)
       ActionDispatch::Session::ActiveRecordStore.session_class = session_class
     end
 
-  # Patch in support for with_routing for integration tests, which was introduced in Rails 7.2
-  if !defined?(ActionDispatch::Assertions::RoutingAssertions::WithIntegrationRouting)
-    require_relative 'with_integration_routing_patch'
+    # Patch in support for with_routing for integration tests, which was
+    # introduced in Rails 7.2, but improved in Rails 8.0 to better handle
+    # middleware. See: https://github.com/rails/rails/pull/54705
+    if Gem.loaded_specs["actionpack"].version <= Gem::Version.new("8.0")
+      require_relative "with_integration_routing_patch"
 
-    include WithIntegrationRoutingPatch
-  end
+      include WithIntegrationRoutingPatch
+    end
 end
 
 ActiveSupport::TestCase.test_order = :random

test/with_integration_routing_patch.rb

@@ -7,39 +7,45 @@ module WithIntegrationRoutingPatch # :nodoc:
   module ClassMethods
     def with_routing(&block)
       old_routes = nil
+      old_routes_call_method = nil
       old_integration_session = nil
 
       setup do
         old_routes = app.routes
+        old_routes_call_method = old_routes.method(:call)
         old_integration_session = integration_session
         create_routes(&block)
       end
 
       teardown do
-        reset_routes(old_routes, old_integration_session)
+        reset_routes(old_routes, old_routes_call_method, old_integration_session)
       end
     end
   end
 
   def with_routing(&block)
     old_routes = app.routes
+    old_routes_call_method = old_routes.method(:call)
     old_integration_session = integration_session
     create_routes(&block)
   ensure
-    reset_routes(old_routes, old_integration_session)
+    reset_routes(old_routes, old_routes_call_method, old_integration_session)
   end
 
   private
 
   def create_routes
     app = self.app
     routes = ActionDispatch::Routing::RouteSet.new
-    rack_app = app.config.middleware.build(routes)
+
+    @original_routes ||= app.routes
+    @original_routes.singleton_class.redefine_method(:call, &routes.method(:call))
+
     https = integration_session.https?
     host = integration_session.host
 
     app.instance_variable_set(:@routes, routes)
-    app.instance_variable_set(:@app, rack_app)
+
     @integration_session = Class.new(ActionDispatch::Integration::Session) do
       include app.routes.url_helpers
       include app.routes.mounted_helpers
@@ -51,11 +57,9 @@ def create_routes
     yield routes
   end
 
-  def reset_routes(old_routes, old_integration_session)
-    old_rack_app = app.config.middleware.build(old_routes)
-
+  def reset_routes(old_routes, old_routes_call_method, old_integration_session)
     app.instance_variable_set(:@routes, old_routes)
-    app.instance_variable_set(:@app, old_rack_app)
+    @original_routes.singleton_class.redefine_method(:call, &old_routes_call_method)
     @integration_session = old_integration_session
     @routes = old_routes
   end