From 48da2b4e4ebfc0bd74adb563f20fe14f8fea6309 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 20 Oct 2026 08:37:07 -0700 Subject: [PATCH 1/4] Refuse a session for a password that changed after it was checked A password change ends every session, but a sign-in or sign-in link checked against the old password just before the change could still save its session just after, and that session outlived the change. The session now checks, as it's saved, that the user's password is still the one the request loaded. The save's transaction holds SQLite's write lock from the start, so a change commits either before the check, which fails it, or after the insert, and ends this session with the others. Sessions start through the user's association so the check sees the user that was authenticated, and a refused sign-in is answered like a wrong password. (cherry picked from commit 19319a1cffcf8fd87d3d973ec190e50a3cf2f8cf) --- app/controllers/concerns/authentication.rb | 2 +- .../sessions/transfers_controller.rb | 2 ++ app/controllers/sessions_controller.rb | 2 ++ app/models/session.rb | 15 +++++++++++--- .../sessions/transfers_controller_test.rb | 15 ++++++++++++++ test/controllers/sessions_controller_test.rb | 13 ++++++++++++ test/models/session_test.rb | 20 +++++++++++++++++++ test/models/user_test.rb | 2 +- 8 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 test/models/session_test.rb diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index e609d33a..be5cf2de 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -57,7 +57,7 @@ module Authentication end def start_new_session_for(user) - user.sessions.start!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session| + user.sessions.create!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session| authenticated_as session end end diff --git a/app/controllers/sessions/transfers_controller.rb b/app/controllers/sessions/transfers_controller.rb index 12dff8f4..aff85abd 100644 --- a/app/controllers/sessions/transfers_controller.rb +++ b/app/controllers/sessions/transfers_controller.rb @@ -11,5 +11,7 @@ class Sessions::TransfersController < ApplicationController else head :bad_request end + rescue ActiveRecord::RecordInvalid + head :bad_request end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1ac2bfd6..3a039462 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -14,6 +14,8 @@ class SessionsController < ApplicationController else render_rejection :unauthorized end + rescue ActiveRecord::RecordInvalid + render_rejection :unauthorized end def destroy diff --git a/app/models/session.rb b/app/models/session.rb index c70b6fa1..bc65f90a 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -7,13 +7,22 @@ class Session < ApplicationRecord before_create { self.last_active_at ||= Time.now } - def self.start!(user_agent:, ip_address:) - create! user_agent: user_agent, ip_address: ip_address - end + validate :user_password_unchanged, on: :create def resume(user_agent:, ip_address:) if last_active_at.before?(ACTIVITY_REFRESH_RATE.ago) update! user_agent: user_agent, ip_address: ip_address, last_active_at: Time.now end end + + private + # A password change ends every session, so one can't start on a password checked before the change. Validated in + # the save's transaction, which holds the database's write lock from the start, so a change commits either before + # this check, and fails it, or after the insert, and ends this session with the others. Compared here rather than in + # the query, which logs its values. + def user_password_unchanged + unless User.where(id: user_id).pick(:password_digest) == user.password_digest + errors.add :user, :invalid + end + end end diff --git a/test/controllers/sessions/transfers_controller_test.rb b/test/controllers/sessions/transfers_controller_test.rb index 18322a88..262fe7fc 100644 --- a/test/controllers/sessions/transfers_controller_test.rb +++ b/test/controllers/sessions/transfers_controller_test.rb @@ -30,4 +30,19 @@ class Sessions::TransfersControllerTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_nil parsed_cookies.signed[:session_token] end + + test "update is refused when the password changes after the link was checked" do + user = users(:david) + transfer_id = user.transfer_id + stale = User.find(user.id) + User.find(user.id).update!(password: "a new secret123456") + User.stubs(:find_by_transfer_id).with(transfer_id).returns(stale) + + assert_no_difference -> { Session.count } do + put session_transfer_url(transfer_id) + end + + assert_response :bad_request + assert_nil parsed_cookies.signed[:session_token] + end end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 4b20dd91..ed1f05be 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -36,6 +36,19 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert parsed_cookies.signed[:session_token] end + test "create is refused when the password changes after it was checked" do + user = User.find(users(:david).id) + User.find(user.id).update!(password: "a new secret123456") + User.stubs(:authenticate_by).returns(user) + + assert_no_difference -> { Session.count } do + post session_url, params: { email_address: "david@37signals.com", password: "secret123456" } + end + + assert_response :unauthorized + assert_nil parsed_cookies.signed[:session_token] + end + test "create with invalid credentials" do post session_url, params: { email_address: "david@37signals.com", password: "wrong" } diff --git a/test/models/session_test.rb b/test/models/session_test.rb new file mode 100644 index 00000000..53cf5ac7 --- /dev/null +++ b/test/models/session_test.rb @@ -0,0 +1,20 @@ +require "test_helper" + +class SessionTest < ActiveSupport::TestCase + test "start a session" do + assert_difference -> { users(:david).sessions.count }, +1 do + users(:david).sessions.create!(user_agent: "This device", ip_address: "127.0.0.1") + end + end + + test "a session doesn't start on a password checked before it changed" do + user = User.find(users(:david).id) + User.find(user.id).update!(password: "a new secret123456") + + assert_no_difference -> { Session.count } do + assert_raises(ActiveRecord::RecordInvalid) do + user.sessions.create!(user_agent: "This device", ip_address: "127.0.0.1") + end + end + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index db290288..4bd1bef2 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -51,7 +51,7 @@ class UserTest < ActiveSupport::TestCase end test "changing the password signs out every session, the current one included" do - Current.session = users(:david).sessions.start!(user_agent: "This device", ip_address: "127.0.0.1") + Current.session = users(:david).sessions.create!(user_agent: "This device", ip_address: "127.0.0.1") assert_changes -> { users(:david).sessions.count }, to: 0 do users(:david).update!(password: "a new secret123456") From 09402c77524e21643f7208965d331d20aa848063 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 20 Oct 2026 08:47:47 -0700 Subject: [PATCH 2/4] Read the password digest past the query cache A digest cached earlier in the request would let the check pass on a password that has since changed. (cherry picked from commit c3faccb0f4d74de44677fb2e5b39374f20f7f6eb) --- app/models/session.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/session.rb b/app/models/session.rb index bc65f90a..9fa3664d 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -18,10 +18,10 @@ class Session < ApplicationRecord private # A password change ends every session, so one can't start on a password checked before the change. Validated in # the save's transaction, which holds the database's write lock from the start, so a change commits either before - # this check, and fails it, or after the insert, and ends this session with the others. Compared here rather than in - # the query, which logs its values. + # this check, and fails it, or after the insert, and ends this session with the others. Read past the query cache, + # and compared here rather than in the query, which logs its values. def user_password_unchanged - unless User.where(id: user_id).pick(:password_digest) == user.password_digest + unless User.uncached { User.where(id: user_id).pick(:password_digest) } == user.password_digest errors.add :user, :invalid end end From 9ca680de0246cfad84e7e6dba9cfb370fb1277b2 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 20 Oct 2026 08:57:40 -0700 Subject: [PATCH 3/4] Sign the browser out when its new session is refused after a password change If the password changes again between this browser's change and its new session, that session is refused, and the browser is sent to sign in instead of an error. (cherry picked from commit a476cbfa24e9863fb9e9e555cb45de6183db78e7) --- app/controllers/users/profiles_controller.rb | 3 +++ test/controllers/users/profiles_controller_test.rb | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 6b30b744..4f5ab314 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -18,6 +18,9 @@ class Users::ProfilesController < ApplicationController else redirect_to user_profile_url, alert: update_alert end + rescue ActiveRecord::RecordInvalid + # The password changed again before this browser's new session could start, and that change signed it out. + redirect_to new_session_url end private diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 49fcb3da..035318db 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -43,6 +43,16 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_equal 1, users(:david).sessions.count end + test "changing the password signs this browser out when it changes again before its new session starts" do + User.stubs(:uncached).returns(BCrypt::Password.create("someone else's secret", cost: BCrypt::Engine::MIN_COST)) + + assert_changes -> { users(:david).sessions.count }, to: 0 do + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } + end + + assert_redirected_to new_session_url + end + test "changing the password gives this browser a new session, so a copy of its cookie stops working" do copied_cookie = cookies[:session_token] From 20fbd3865c6cb7e9548dc97d331f1ac75fb2470e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 20 Oct 2026 20:42:54 -0700 Subject: [PATCH 4/4] Check a cable connection's session while it's open (#376) * Check a cable connection's session while it's open Signing out or changing the password ends sessions and then disconnects their connections. A connection that found its session just before isn't subscribed to its disconnects yet, so it missed that and stayed open. Each connection now looks its session up every minute while it's open, and closes once the session is gone, asking the browser to reconnect so one holding a current session comes back. That also covers any other way a connection misses the disconnect, such as a subscription that takes long to register, and a lookup that fails is simply tried again. * Check the session on the server's heartbeat rather than a timer of its own A connection's own timer could outlive it: closing can run before opening finishes, or a channel's teardown can fail before the connection's. The heartbeat only reaches connections that are open, so nothing is left to stop. * Skip the session check on a rejected connection A rejected connection stays registered until its socket closes, and has no session to check, so its heartbeat mustn't fail and stop the heartbeat reaching the connections after it. (cherry picked from commit ce7b5d4ea05b9908dffc2c747c53b98772ea4926) --- app/channels/application_cable/connection.rb | 27 +++++++++- .../application_cable/connection_test.rb | 49 +++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb index 749265c2..0094130e 100644 --- a/app/channels/application_cable/connection.rb +++ b/app/channels/application_cable/connection.rb @@ -2,19 +2,42 @@ module ApplicationCable class Connection < ActionCable::Connection::Base include Authentication::SessionLookup + SESSION_CHECK_INTERVAL = 1.minute + identified_by :current_user def connect self.current_user = find_verified_user + @session_checked_at = Time.now + end + + # Signing out or changing the password ends sessions and then disconnects their connections, but a connection can + # miss that: one that found its session just before isn't listening yet. So on the server's heartbeat, which + # reaches every open connection, each one looks its session up again now and then, and closes once it's gone. A + # rejected connection has no session, and stays registered until its socket closes. + def beat + super + check_session if @session_checked_at&.before?(SESSION_CHECK_INTERVAL.ago) end private def find_verified_user - if verified_session = find_session_by_cookie - verified_session.user + if @verified_session = find_session_by_cookie + @verified_session.user else reject_unauthorized_connection end end + + def check_session + @session_checked_at = Time.now + perform_work -> { close_unless_session_remains }, :call + end + + def close_unless_session_remains + unless Session.exists?(@verified_session.id) + close reason: ActionCable::INTERNAL[:disconnect_reasons][:unauthorized] + end + end end end diff --git a/test/channels/application_cable/connection_test.rb b/test/channels/application_cable/connection_test.rb index 8533ef1d..342e9a58 100644 --- a/test/channels/application_cable/connection_test.rb +++ b/test/channels/application_cable/connection_test.rb @@ -18,4 +18,53 @@ class ApplicationCable::ConnectionTest < ActionCable::Connection::TestCase assert_reject_connection { connect } end + + test "closes once its session has ended without it hearing" do + cookies.signed[:session_token] = sessions(:david_safari).token + connect + sessions(:david_safari).destroy + + travel 2.minutes + connection.beat + + assert socket.closed + assert_equal "disconnect", socket.transmissions.last[:type] + assert socket.transmissions.last[:reconnect] + end + + test "keeps checking its session while open" do + cookies.signed[:session_token] = sessions(:david_safari).token + connect + + travel 2.minutes + connection.beat + assert_not socket.closed + + sessions(:david_safari).destroy + travel 2.minutes + connection.beat + assert socket.closed + end + + test "checks its session at most once a minute" do + cookies.signed[:session_token] = sessions(:david_safari).token + connect + sessions(:david_safari).destroy + + travel 30.seconds + connection.beat + + assert_not socket.closed + end + + test "a rejected connection's heartbeat doesn't fail" do + request = ActionCable::Connection::TestSocket.build_request(ActionCable.server.config.mount_path || "/cable", cookies: cookies) + rejected = ApplicationCable::Connection.new \ + ActionCable::Connection::TestServer.new(ActionCable.server), ActionCable::Connection::TestSocket.new(request) + rejected.handle_open + + travel 2.minutes + + assert_nothing_raised { rejected.beat } + end end