Merge pull request #199 from docsend/optional-insecure-session-fallback

Allow use of secure session only

Author: byroot

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

@@ -60,6 +60,11 @@ class ActiveRecordStore < ActionDispatch::Session::AbstractSecureStore
       SESSION_RECORD_KEY = 'rack.session.record'
       ENV_SESSION_OPTIONS_KEY = Rack::RACK_SESSION_OPTIONS
 
+      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
@@ -136,7 +141,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

@@ -305,6 +305,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

@@ -92,12 +92,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