diff --git a/README.md b/README.md index 1ed08c7..5b2e7b2 100644 --- a/README.md +++ b/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), diff --git a/lib/action_dispatch/session/active_record_store.rb b/lib/action_dispatch/session/active_record_store.rb index ae21d70..85e4424 100644 --- a/lib/action_dispatch/session/active_record_store.rb +++ b/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 diff --git a/test/action_controller_test.rb b/test/action_controller_test.rb index 16c555d..4988fb0 100644 --- a/test/action_controller_test.rb +++ b/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 diff --git a/test/helper.rb b/test/helper.rb index 5d632e8..4a9ad7c 100644 --- a/test/helper.rb +++ b/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 diff --git a/test/with_integration_routing_patch.rb b/test/with_integration_routing_patch.rb index efa39e2..36e26f2 100644 --- a/test/with_integration_routing_patch.rb +++ b/test/with_integration_routing_patch.rb @@ -7,26 +7,29 @@ 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 @@ -34,12 +37,15 @@ def with_routing(&block) 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