From b1122c7d6da3c6b040f5c256cd0d2494af2951af Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 10:45:34 -0700 Subject: [PATCH] Give the changing browser a new session rather than keeping its own Keeping the session the password was changed from also kept any copy of its cookie working, and a copied cookie is one of the ways someone else gets into an account. Every session ends now, and the profile page signs the browser that changed the password back in with a new one. From #371. (cherry picked from commit b01bb7a0a76d4dd21dba0018c27cdc0aefed3647) --- app/controllers/users/profiles_controller.rb | 2 ++ app/models/user.rb | 10 +++++----- test/controllers/users/profiles_controller_test.rb | 14 +++++++++++++- test/models/user_test.rb | 14 +++----------- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 96015d55..bdf13c47 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -8,6 +8,8 @@ class Users::ProfilesController < ApplicationController def update @user.update user_params + start_new_session_for @user if @user.saved_change_to_password_digest? + redirect_to user_profile_url, notice: update_notice end diff --git a/app/models/user.rb b/app/models/user.rb index 2df933ef..c9ffee57 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,7 +20,7 @@ class User < ApplicationRecord has_secure_password validations: false after_create_commit :grant_membership_to_open_rooms - after_update :sign_out_other_sessions, if: :saved_change_to_password_digest? + after_update :sign_out_all_sessions, if: :saved_change_to_password_digest? after_update_commit :disconnect_signed_out_sessions, if: :saved_change_to_password_digest? scope :ordered, -> { order("LOWER(name)") } @@ -52,10 +52,10 @@ class User < ApplicationRecord end private - # A new password is how someone shuts out whoever else got into their account, so it ends the sessions opened - # with the old one. The session it was changed from, if any, stays. - def sign_out_other_sessions - sessions.excluding(Current.session).delete_all + # A new password is how someone shuts out whoever else got into their account, so it ends every session opened + # with the old one, copies of the changing browser's cookie included. The profile page signs that browser back in. + def sign_out_all_sessions + sessions.delete_all end # After commit, so a connection that drops can't come back on a session that's still there. The password is diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index c5f95614..6d017260 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -27,7 +27,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_equal "david@37signals.com", users(:david).email_address end - test "changing the password signs out the account's other sessions but not this one" do + test "changing the password signs out the account's other devices but not this browser" do other_device = open_session other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" } other_device.get user_profile_url @@ -43,6 +43,18 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_equal 1, users(:david).sessions.count 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] + + put user_profile_url, params: { user: { password: "a new secret123456" } } + assert_not_equal copied_cookie, cookies[:session_token] + + copy = open_session + copy.cookies[:session_token] = copied_cookie + copy.get user_profile_url + copy.assert_redirected_to new_session_url + end + test "changing the password closes the account's live connections" do remote_connections = mock remote_connections.expects(:disconnect).with(reconnect: true) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 22b8109e..2262dcac 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -42,15 +42,15 @@ class UserTest < ActiveSupport::TestCase end end - test "changing the password outside a request signs out every session" do - users(:david).sessions.start!(user_agent: "Other device", ip_address: "127.0.0.1") + 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") assert_changes -> { users(:david).sessions.count }, to: 0 do users(:david).update!(password: "a new secret123456") end end - test "changing the password outside a request closes the account's live connections" do + test "changing the password closes the account's live connections" do remote_connections = mock remote_connections.expects(:disconnect).with(reconnect: true) ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) }) @@ -58,14 +58,6 @@ class UserTest < ActiveSupport::TestCase users(:david).update!(password: "a new secret123456") end - test "changing the password keeps the session it was changed from" do - Current.session = users(:david).sessions.start!(user_agent: "This device", ip_address: "127.0.0.1") - - users(:david).update!(password: "a new secret123456") - - assert_equal [ Current.session ], users(:david).sessions.reload - end - test "changing other details keeps every session" do ActionCable.server.expects(:remote_connections).never