From 4dd89fa5040ca60a1ffcfd88dc355697f31cd2b4 Mon Sep 17 00:00:00 2001 From: Matheus Richard Date: Tue, 10 Mar 2026 22:42:55 -0300 Subject: [PATCH] Fix race condition in test helpers where background requests steal login_as callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `login_as` and `logout` used `Warden.on_next_request`, a global FIFO queue consumed by the next non-asset request through the Warden middleware. This meant any background request (Turbo Frame fetches, ActionCable `/cable` reconnects) could race against the intended `visit` and consume the authentication callback, leaving the user silently unauthenticated. The `asset_paths` mechanism (introduced in #45 for #44) partially addressed this by skipping asset requests, but it cannot cover application-level background requests like `/cable` or Turbo Frame endpoints — these are normal HTTP requests that go through the full middleware stack. Users had to work around this by extending `asset_paths` to match `/cable` and other paths (#182), which is fragile and requires knowledge of Warden internals. This commit replaces the one-shot queue mechanism in `login_as` with a persistent `_test_users` hash keyed by scope. On each request, the `on_request` hook checks whether the scope already has a user stored in the Rack session before applying the test user. This approach fixes the race condition because: - Background requests from an established session have `stored?=true`, so they are skipped — there is no callback to steal. - The intended browser's first request has `stored?=false` and gets the test user applied via `set_user`, which writes to the Rack session. - Subsequent requests from the same browser load the user from session normally — the test user is not "consumed" like `on_next_request` was. - `on_next_request` is preserved for backward compatibility and is still used by `logout` to clear the Rack session on the next request. Fixes #163, fixes #182. --- lib/warden.rb | 7 +++ lib/warden/test/helpers.rb | 11 +++-- lib/warden/test/warden_helpers.rb | 9 ++++ spec/warden/test/helpers_spec.rb | 73 ++++++++++++++++++++++++++++++ spec/warden/test/test_mode_spec.rb | 7 +++ 5 files changed, 103 insertions(+), 4 deletions(-) diff --git a/lib/warden.rb b/lib/warden.rb index 777e0e2..5919fb7 100644 --- a/lib/warden.rb +++ b/lib/warden.rb @@ -39,6 +39,13 @@ def self.test_mode! while blk = Warden._on_next_request.shift blk.call(proxy) end + Warden._test_users.each_value do |(user, opts)| + opts = opts.dup + scope = opts[:scope] || proxy.config.default_scope + unless proxy.session_serializer.stored?(scope) + proxy.set_user(user, opts) + end + end end end end diff --git a/lib/warden/test/helpers.rb b/lib/warden/test/helpers.rb index cecbc1b..a14312e 100644 --- a/lib/warden/test/helpers.rb +++ b/lib/warden/test/helpers.rb @@ -16,10 +16,8 @@ def self.included(_base) # @see Warden::Proxy#set_user # @api public def login_as(user, opts = {}) - Warden.on_next_request do |proxy| - opts[:event] ||= :authentication - proxy.set_user(user, opts) - end + opts[:event] ||= :authentication + Warden._test_users[opts[:scope]] = [user, opts] end # Logs out a user from the session. @@ -28,6 +26,11 @@ def login_as(user, opts = {}) # @see Warden::Proxy#logout # @api public def logout(*scopes) + if scopes.empty? + Warden._test_users.clear + else + scopes.each { |s| Warden._test_users.delete(s) } + end Warden.on_next_request do |proxy| proxy.logout(*scopes) end diff --git a/lib/warden/test/warden_helpers.rb b/lib/warden/test/warden_helpers.rb index 9fa46ec..076376d 100644 --- a/lib/warden/test/warden_helpers.rb +++ b/lib/warden/test/warden_helpers.rb @@ -31,6 +31,7 @@ def on_next_request(&blk) # @api public def test_reset! _on_next_request.clear + _test_users.clear end # A container for the on_next_request items. @@ -39,6 +40,14 @@ def _on_next_request @_on_next_request ||= [] @_on_next_request end + + # A persistent store of test users keyed by scope. + # Unlike _on_next_request, these survive across multiple requests, + # preventing race conditions with background requests. + # @api private + def _test_users + @_test_users ||= {} + end end end end diff --git a/spec/warden/test/helpers_spec.rb b/spec/warden/test/helpers_spec.rb index 187bbd3..913027e 100644 --- a/spec/warden/test/helpers_spec.rb +++ b/spec/warden/test/helpers_spec.rb @@ -84,6 +84,79 @@ expect($captures).to eq([:run]) end + it "should persist authentication across multiple requests" do + user = "A User" + session = {} + login_as user + app = setup_rack(lambda { |e| + $captures << e['warden'].user + valid_response + }) + app.call(env_with_params("/", {}, "rack.session" => session)) + app.call(env_with_params("/", {}, "rack.session" => session)) + expect($captures).to eq(["A User", "A User"]) + end + + it "should not lose authentication when another session makes a concurrent request" do + alice = "Alice" + bob = "Bob" + + alice_session = {} + bob_session = {} + + app = setup_rack(lambda { |e| + $captures << e['warden'].user + valid_response + }) + + # 1. Alice logs in and visits a page — her session is established + login_as alice + app.call(env_with_params("/", {}, "rack.session" => alice_session)) + + # 2. Bob logs in + login_as bob + + # 3. A background request from Alice's session (e.g. Turbo Frame fetch) + app.call(env_with_params("/turbo-frame", {}, "rack.session" => alice_session)) + + # 4. Bob's session makes its first request + app.call(env_with_params("/", {}, "rack.session" => bob_session)) + + expect($captures).to eq(["Alice", "Alice", "Bob"]) + end + + it "should allow switching users with logout in between" do + session = {} + app = setup_rack(lambda { |e| + $captures << e['warden'].user + valid_response + }) + + login_as "Alice" + app.call(env_with_params("/", {}, "rack.session" => session)) + + logout + login_as "Bob" + app.call(env_with_params("/", {}, "rack.session" => session)) + + expect($captures).to eq(["Alice", "Bob"]) + end + + it "should clear persistent state on logout" do + user = "A User" + login_as user + expect(Warden._test_users).not_to be_empty + logout + expect(Warden._test_users).to be_empty + end + + it "should clear only the specified scope on logout" do + login_as "Default User" + login_as "Foo User", scope: :foo + logout :foo + expect(Warden._test_users.keys).to eq([nil]) + end + describe "#asset_paths" do it "should default asset_paths to anything asset path regex" do expect(Warden.asset_paths).to eq([/^\/assets\//] ) diff --git a/spec/warden/test/test_mode_spec.rb b/spec/warden/test/test_mode_spec.rb index ccb4471..3ebef37 100644 --- a/spec/warden/test/test_mode_spec.rb +++ b/spec/warden/test/test_mode_spec.rb @@ -63,6 +63,13 @@ expect($captures).to eq([]) end + it "should clear _test_users when test is reset" do + Warden._test_users[:default] = ["A User", { event: :authentication }] + expect(Warden._test_users).not_to be_empty + Warden.test_reset! + expect(Warden._test_users).to be_empty + end + context "asset requests" do it "should not execute on_next_request blocks if this is an asset request" do app = setup_rack(@app)