From 19319a1cffcf8fd87d3d973ec190e50a3cf2f8cf Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 20 Oct 2026 08:37:07 -0700 Subject: [PATCH] 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. --- 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 4a1d70c..2457ad9 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 12dff8f..aff85ab 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 1ac2bfd..3a03946 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 c70b6fa..bc65f90 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 18322a8..262fe7f 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 4b20dd9..ed1f05b 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 0000000..53cf5ac --- /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 db29028..4bd1bef 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")