Merge pull request rails#53918 from riseshia/ensure-gen-unique-sid-when-cache-store

Add an option to ensure uniqueness of newly generated session IDs in `Session::CacheStore`

Author: byroot

actionpack/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Add `check_collisions` option to `ActionDispatch::Session::CacheStore`.
+
+    Newly generated session ids use 128 bits of randomness, which is more than
+    enough to ensure collisions can't happen, but if you need to harden sessions
+    even more, you can enable this option to check in the session store that the id
+    is indeed free you can enable that option. This however incurs an extra write
+    on session creation.
+
+    *Shia*
+
 *   In ExceptionWrapper, match backtrace lines with built templates more often,
     allowing improved highlighting of errors within do-end blocks in templates.
     Fix for Ruby 3.4 to match new method labels in backtrace.

actionpack/lib/action_dispatch/middleware/session/cache_store.rb

@@ -18,11 +18,16 @@ module Session
     # *   `expire_after`  - The length of time a session will be stored before
     #     automatically expiring. By default, the `:expires_in` option of the cache
     #     is used.
+    # *   `check_collisions`  - Check if newly generated session ids aren't already in use.
+    #     If for some reason 128 bits of randomness aren't considered secure enough to avoid
+    #     collisions, this option can be enabled to ensure newly generated ids aren't in use.
+    #     By default, it is set to `false` to avoid additional cache write operations.
     #
     class CacheStore < AbstractSecureStore
       def initialize(app, options = {})
         @cache = options[:cache] || Rails.cache
         options[:expire_after] ||= @cache.options[:expires_in]
+        @check_collisions = options[:check_collisions] || false
         super
       end
 
@@ -61,6 +66,18 @@ def cache_key(id)
         def get_session_with_fallback(sid)
           @cache.read(cache_key(sid.private_id)) || @cache.read(cache_key(sid.public_id))
         end
+
+        def generate_sid
+          if @check_collisions
+            loop do
+              sid = super
+              key = cache_key(sid.private_id)
+              break sid if @cache.write(key, {}, unless_exist: true, expires_in: default_options[:expire_after])
+            end
+          else
+            super
+          end
+        end
     end
   end
 end

actionpack/test/dispatch/session/cache_store_test.rb

@@ -203,11 +203,50 @@ def test_drop_session_in_the_legacy_id_as_well
     end
   end
 
+  def test_doesnt_generate_same_sid_with_check_collisions
+    cache_store_options({ check_collisions: true })
+
+    expected_values = [
+      +"eed45f3da20b5c4da3bc3b160f996315",
+      +"eed45f3da20b5c4da3bc3b160f996315",
+      +"eed45f3da20b5c4da3bc3b160f996316",
+    ]
+    counter = -1
+    hex_generator = proc do
+      counter += 1
+      expected_values[counter]
+    end
+
+    SecureRandom.stub(:hex, hex_generator) do
+      with_test_route_set do
+        get "/set_session_value"
+        assert_response :success
+        assert cookies["_session_id"]
+
+        first_sid = Rack::Session::SessionId.new(cookies["_session_id"])
+        assert_equal expected_values[0], first_sid.public_id
+
+        cookies.delete("_session_id")
+
+        get "/set_session_value"
+        assert_response :success
+        assert cookies["_session_id"]
+
+        second_sid = Rack::Session::SessionId.new(cookies["_session_id"])
+        assert_equal expected_values[2], second_sid.public_id
+      end
+    end
+  end
+
   private
+    def cache_store_options(options = {})
+      @cache_store_options ||= options
+    end
+
     def app
       @app ||= self.class.build_app do |middleware|
         @cache = ActiveSupport::Cache::MemoryStore.new
-        middleware.use ActionDispatch::Session::CacheStore, key: "_session_id", cache: @cache
+        middleware.use ActionDispatch::Session::CacheStore, key: "_session_id", cache: @cache, **cache_store_options
         middleware.delete ActionDispatch::ShowExceptions
       end
     end