From a784bbd373ef99740b20c6d5430e2b95e4e846ad Mon Sep 17 00:00:00 2001 From: doutatsu Date: Thu, 8 Oct 2026 16:55:24 +0900 Subject: [PATCH] Do not restore a flushed refresh token during a refresh A refresh that ran at the same time as a flush of its session wrote csrf, access_uid and access_expiration to the removed refresh token again. With Redis, the key had no expiration field and no TTL, so no later flush found it. With the memory store, the flushed session stayed refreshable. The refresh also returned a new access token that stayed valid after the flush. update_refresh now writes only while the refresh token exists, and returns false otherwise. The session then removes the access token that it just created and raises Unauthorized. --- lib/jwt_sessions/session.rb | 7 ++- .../store_adapters/abstract_store_adapter.rb | 2 + .../store_adapters/memory_store_adapter.rb | 3 ++ .../store_adapters/redis_store_adapter.rb | 34 ++++++++----- .../test_memory_store_adapter.rb | 24 ++++++++- .../test_redis_store_adapter.rb | 49 +++++++++++++++++++ test/units/jwt_sessions/test_session.rb | 15 ++++++ 7 files changed, 121 insertions(+), 13 deletions(-) diff --git a/lib/jwt_sessions/session.rb b/lib/jwt_sessions/session.rb index c38a903..f3917c8 100644 --- a/lib/jwt_sessions/session.rb +++ b/lib/jwt_sessions/session.rb @@ -221,7 +221,12 @@ def issue_tokens_after_refresh end def update_refresh_token - @_refresh.update(@_access.uid, @_access.expiration, @_csrf.encoded) + updated = @_refresh.update(@_access.uid, @_access.expiration, @_csrf.encoded) + if updated == false + AccessToken.destroy(@_access.uid, store) + raise Errors::Unauthorized, "Refresh token not found" + end + @refresh_token = @_refresh.token link_access_to_refresh end diff --git a/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb b/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb index 190cb1b..c5215d6 100644 --- a/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb +++ b/lib/jwt_sessions/store_adapters/abstract_store_adapter.rb @@ -20,6 +20,8 @@ def persist_refresh(_uid:, _access_expiration:, _access_uid:, _csrf:, _expiratio raise NotImplementedError end + # Return false when the refresh token no longer exists. A refresh then + # fails instead of writing back a token that a flush has just removed. def update_refresh(_uid:, _access_expiration:, _access_uid:, _csrf:, _namespace:) raise NotImplementedError end diff --git a/lib/jwt_sessions/store_adapters/memory_store_adapter.rb b/lib/jwt_sessions/store_adapters/memory_store_adapter.rb index 6f68512..7e2b30f 100644 --- a/lib/jwt_sessions/store_adapters/memory_store_adapter.rb +++ b/lib/jwt_sessions/store_adapters/memory_store_adapter.rb @@ -46,6 +46,8 @@ def persist_refresh(uid:, access_expiration:, access_uid:, csrf:, expiration:, n end def update_refresh(uid:, access_expiration:, access_uid:, csrf:, namespace: "") + return false if value_if_not_expired(uid, "refresh", namespace.to_s).empty? + update_refresh_fields( uid, namespace.to_s, @@ -53,6 +55,7 @@ def update_refresh(uid:, access_expiration:, access_uid:, csrf:, namespace: "") access_expiration: access_expiration, access_uid: access_uid ) + true end def all_refresh_tokens(namespace) diff --git a/lib/jwt_sessions/store_adapters/redis_store_adapter.rb b/lib/jwt_sessions/store_adapters/redis_store_adapter.rb index 5cdb276..0e96108 100644 --- a/lib/jwt_sessions/store_adapters/redis_store_adapter.rb +++ b/lib/jwt_sessions/store_adapters/redis_store_adapter.rb @@ -7,6 +7,15 @@ class RedisStoreAdapter < AbstractStoreAdapter REFRESH_KEYS = %i[csrf access_uid access_expiration expiration].freeze DEFAULT_POOL_SIZE = 5 + # Updates a refresh token only while it exists, so that a refresh which + # races a flush cannot write the flushed token back. + UPDATE_REFRESH_SCRIPT = <<~LUA + if redis.call("EXISTS", KEYS[1]) == 0 then + return 0 + end + redis.call("HMSET", KEYS[1], "csrf", ARGV[1], "access_expiration", ARGV[2], "access_uid", ARGV[3]) + return 1 + LUA def initialize(token_prefix: JWTSessions.token_prefix, redis_client: nil, **options) @prefix = token_prefix @@ -51,24 +60,27 @@ def fetch_refresh(uid, namespace, first_match = false) def persist_refresh(uid:, access_expiration:, access_uid:, csrf:, expiration:, namespace: nil) key = full_refresh_key(uid, namespace) - update_refresh( - uid: uid, - access_expiration: access_expiration, - access_uid: access_uid, - csrf: csrf, - namespace: namespace + storage.call("HMSET", + key, + :csrf, csrf, + :access_expiration, access_expiration, + :access_uid, access_uid, + :expiration, expiration ) - storage.call("HSET", key, :expiration, expiration) storage.call("EXPIREAT", key, expiration) end def update_refresh(uid:, access_expiration:, access_uid:, csrf:, namespace: nil) - storage.call("HMSET", + updated = storage.call( + "EVAL", + UPDATE_REFRESH_SCRIPT, + 1, full_refresh_key(uid, namespace), - :csrf, csrf, - :access_expiration, access_expiration, - :access_uid, access_uid + csrf, + access_expiration, + access_uid ) + updated == 1 end def all_refresh_tokens(namespace) diff --git a/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb b/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb index 79fdef2..dee0d74 100644 --- a/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb +++ b/test/units/jwt_sessions/store_adapters/test_memory_store_adapter.rb @@ -60,7 +60,7 @@ def test_update_refresh expiration: expiration, namespace: "" ) - store.update_refresh( + assert store.update_refresh( uid: "uid", access_expiration: expiration, access_uid: "access_uid", @@ -71,6 +71,28 @@ def test_update_refresh assert_equal "csrf2", refresh[:csrf] end + def test_update_refresh_after_destroy + expiration = Time.now.to_i + 3600 + store.persist_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf", + expiration: expiration, + namespace: "" + ) + store.destroy_refresh("uid", "") + + refute store.update_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf2", + namespace: "" + ) + assert_equal({}, store.fetch_refresh("uid", "")) + end + def test_all_refresh_tokens expiration = Time.now.to_i + 3600 store.persist_refresh( diff --git a/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb b/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb index 757be13..f3963c8 100644 --- a/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb +++ b/test/units/jwt_sessions/store_adapters/test_redis_store_adapter.rb @@ -11,6 +11,55 @@ def teardown JWTSessions.remove_instance_variable(:@redis_url) if JWTSessions.instance_variable_defined?(:@redis_url) end + def test_update_refresh + store = JWTSessions::StoreAdapters::RedisStoreAdapter.new + expiration = Time.now.to_i + 3600 + store.persist_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf", + expiration: expiration, + namespace: "update_refresh" + ) + + assert store.update_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf2", + namespace: "update_refresh" + ) + assert_equal "csrf2", store.fetch_refresh("uid", "update_refresh")[:csrf] + ensure + store.destroy_refresh("uid", "update_refresh") + end + + def test_update_refresh_after_destroy + store = JWTSessions::StoreAdapters::RedisStoreAdapter.new + expiration = Time.now.to_i + 3600 + store.persist_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf", + expiration: expiration, + namespace: "update_refresh_after_destroy" + ) + store.destroy_refresh("uid", "update_refresh_after_destroy") + + refute store.update_refresh( + uid: "uid", + access_expiration: expiration, + access_uid: "access_uid", + csrf: "csrf2", + namespace: "update_refresh_after_destroy" + ) + assert_empty store.storage.call("KEYS", "#{store.prefix}_update_refresh_after_destroy_*") + ensure + store.destroy_refresh("uid", "update_refresh_after_destroy") + end + def test_error_on_mixed_redis_options assert_raises ArgumentError do JWTSessions::StoreAdapters::RedisStoreAdapter.new( diff --git a/test/units/jwt_sessions/test_session.rb b/test/units/jwt_sessions/test_session.rb index b60b142..fcdc5d2 100644 --- a/test/units/jwt_sessions/test_session.rb +++ b/test/units/jwt_sessions/test_session.rb @@ -336,6 +336,21 @@ def test_flush_namespaced_access_tokens_after_flush_namespaced assert_equal 0, session.flush_namespaced_access_tokens end + def test_refresh_after_concurrent_flush + refresh_token = tokens[:refresh] + racing_store = JWTSessions.token_store.dup + racing_store.define_singleton_method(:update_refresh) do |**fields| + JWTSessions::Session.new.flush_by_token(refresh_token) + super(**fields) + end + racing_session = JWTSessions::Session.new(payload: payload, store: racing_store) + + assert_raises JWTSessions::Errors::Unauthorized do + racing_session.refresh(refresh_token) + end + assert_empty RedisClient.new.call("KEYS", "jwt_*") + end + def test_flush_all refresh_token = @session.instance_variable_get(:"@_refresh") flushed_count = JWTSessions::Session.flush_all