From 44e4b7e4b145e7c5b2d03cfeb97fe0b8a7002659 Mon Sep 17 00:00:00 2001 From: Tejas Mandke Date: Mon, 27 Mar 2023 14:29:20 -0700 Subject: [PATCH] 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: https://github.com/rails/activerecord-session_store/pull/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: https://github.com/rails/rails/pull/54705 --- README.md | 7 ++++++ .../session/active_record_store.rb | 7 +++++- test/action_controller_test.rb | 24 +++++++++++++++++++ test/helper.rb | 12 ++++++---- test/with_integration_routing_patch.rb | 20 +++++++++------- 5 files changed, 56 insertions(+), 14 deletions(-) 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