From c78c3fd7efada9d64d2bb6b3b3b31d1b845266d0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 10:28:06 -0700 Subject: [PATCH 01/10] Sign out the account's other sessions when its password changes Changing the password is how someone shuts out whoever else is signed in to their account, but every session opened with the old password stayed valid. Now the others end and their live connections close, as on sign-out. The session the password was changed from stays signed in. Changing the password outside a request, as from a console, ends every session. --- app/controllers/users/profiles_controller.rb | 2 + app/models/user.rb | 7 +++ test/controllers/cached_responses_test.rb | 23 ++++++++++ .../users/profiles_controller_test.rb | 46 +++++++++++++++++++ test/models/user_test.rb | 22 +++++++++ 5 files changed, 100 insertions(+) diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 96015d55..9f3726a7 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 + disconnect_remote_connections 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 df2fee19..449ab02b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,6 +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? scope :ordered, -> { order("LOWER(name)") } scope :filtered_by, ->(query) { where("name like ?", "%#{query}%") } @@ -50,6 +51,12 @@ 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 + end + # One statement, which SQLite runs under the write lock, so no room is closed between reading the open ones and # inserting. A room that Room#destroy_later closes is either closed before, and skipped, or after, and takes this # membership away with the others. A room that granted itself to every user when it was created is skipped too, diff --git a/test/controllers/cached_responses_test.rb b/test/controllers/cached_responses_test.rb index 888994d0..df4102c4 100644 --- a/test/controllers/cached_responses_test.rb +++ b/test/controllers/cached_responses_test.rb @@ -136,6 +136,29 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest assert_redirected_to new_session_url end + test "a session signed out by a password change isn't served from a cached snapshot or page" do + other_device = open_session { |session| session.host! "once.campfire.test" } + other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" } + other_device.get user_sidebar_url(:me) + other_device.assert_response :success + + Session.expects(:find_by).never + ResponseCache.instance.expects(:write).never + other_device.get user_sidebar_url(:me) + other_device.assert_response :success + Session.unstub(:find_by) + ResponseCache.instance.unstub(:write) + + put user_profile_url, params: { user: { password: "a new secret123456" } }, + headers: { "Sec-Fetch-Site" => "same-origin", "Origin" => "http://once.campfire.test" } + assert_redirected_to user_profile_url + + other_device.get user_sidebar_url(:me) + other_device.assert_redirected_to new_session_url + get user_sidebar_url(:me) + assert_response :success + end + test "conditional requests keep native validators" do get room_messages_url(@room) etag = response.headers["ETag"] diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 0331aafb..c5f95614 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -27,6 +27,52 @@ 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 + other_device = open_session + other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" } + other_device.get user_profile_url + other_device.assert_response :success + + put user_profile_url, params: { user: { password: "a new secret123456" } } + assert_redirected_to user_profile_url + + other_device.get user_profile_url + other_device.assert_redirected_to new_session_url + get user_profile_url + assert_response :success + assert_equal 1, users(:david).sessions.count + end + + 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) }) + + put user_profile_url, params: { user: { password: "a new secret123456" } } + + assert_redirected_to user_profile_url + end + + test "changing the password still signs out other sessions when the realtime service is unreachable" do + other_session = users(:david).sessions.first + ActionCable.server.stubs(:remote_connections).raises(RuntimeError.new("cable down")) + + put user_profile_url, params: { user: { password: "a new secret123456" } } + + assert_redirected_to user_profile_url + assert_not Session.exists?(other_session.id) + end + + test "updating the profile without a new password leaves other sessions signed in" do + ActionCable.server.expects(:remote_connections).never + + assert_no_difference -> { users(:david).sessions.count } do + put user_profile_url, params: { user: { name: "David", password: "" } } + end + + assert_redirected_to user_profile_url + end + test "updates are limited to the current user" do put user_profile_url(users(:jason)), params: { user: { name: "John Doe" } } diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 61bb1c39..0926adaa 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -42,6 +42,28 @@ 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") + + assert_changes -> { users(:david).sessions.count }, to: 0 do + users(:david).update!(password: "a new secret123456") + end + 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 + assert_no_changes -> { users(:david).sessions.count } do + users(:david).update!(name: "Dave", email_address: "dave@37signals.com") + end + end + private def create_new_user User.create!(name: "User", email_address: "user@example.com", password: "secret123456") From c6e01054916e7f7f68304eaee2c4762c12358722 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 10:33:46 -0700 Subject: [PATCH 02/10] Close live connections from the model on any password change The sessions end wherever the password changes, but only the profile page closed the live connections, so a change from a console left a signed-out device receiving messages until it dropped. Close them after commit from User, logging rather than failing if the realtime service can't be reached, since the password is saved by then. --- app/controllers/users/profiles_controller.rb | 2 -- app/models/user.rb | 9 +++++++++ test/models/user_test.rb | 10 ++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 9f3726a7..96015d55 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -8,8 +8,6 @@ class Users::ProfilesController < ApplicationController def update @user.update user_params - disconnect_remote_connections 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 449ab02b..2df933ef 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,6 +21,7 @@ class User < ApplicationRecord after_create_commit :grant_membership_to_open_rooms after_update :sign_out_other_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)") } scope :filtered_by, ->(query) { where("name like ?", "%#{query}%") } @@ -57,6 +58,14 @@ class User < ApplicationRecord sessions.excluding(Current.session).delete_all end + # After commit, so a connection that drops can't come back on a session that's still there. The password is + # saved by then, so a realtime service that can't be reached mustn't fail the change. + def disconnect_signed_out_sessions + reset_remote_connections + rescue => error + Rails.logger.warn "Could not disconnect remote connections after a password change: #{error.class}" + end + # One statement, which SQLite runs under the write lock, so no room is closed between reading the open ones and # inserting. A room that Room#destroy_later closes is either closed before, and skipped, or after, and takes this # membership away with the others. A room that granted itself to every user when it was created is skipped too, diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 0926adaa..22b8109e 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -50,6 +50,14 @@ class UserTest < ActiveSupport::TestCase end end + test "changing the password outside a request 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) }) + + 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") @@ -59,6 +67,8 @@ class UserTest < ActiveSupport::TestCase end test "changing other details keeps every session" do + ActionCable.server.expects(:remote_connections).never + assert_no_changes -> { users(:david).sessions.count } do users(:david).update!(name: "Dave", email_address: "dave@37signals.com") end From b01bb7a0a76d4dd21dba0018c27cdc0aefed3647 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 10:45:34 -0700 Subject: [PATCH 03/10] 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. --- 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 From e158388f3d006a7b01163f5fee95f62a252fd517 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 10:57:14 -0700 Subject: [PATCH 04/10] Close live connections on the transaction that changed the password An after_commit condition reads the record's last save, so another save of the user in the same transaction skipped the disconnect even though the sessions were gone. The callback that deletes them now registers the disconnect on the transaction itself, which also leaves a rolled back change alone. A password changed along with the avatar is covered too: the attachment's touch doesn't hide the change. --- app/models/user.rb | 10 ++++----- .../users/profiles_controller_test.rb | 13 +++++++++++ test/models/user_test.rb | 22 +++++++++++++++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index c9ffee57..de25de1b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,7 +21,6 @@ class User < ApplicationRecord after_create_commit :grant_membership_to_open_rooms 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)") } scope :filtered_by, ->(query) { where("name like ?", "%#{query}%") } @@ -52,14 +51,15 @@ class User < ApplicationRecord end private - # 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. + # A new password is how someone shuts out whoever else got into their account, so it ends every session, copies + # of the changing browser's cookie included. Connections close once that commits, registered on the transaction + # because a later save in it would replace the saved change an after_commit condition reads. def sign_out_all_sessions sessions.delete_all + self.class.current_transaction.after_commit { disconnect_signed_out_sessions } end - # After commit, so a connection that drops can't come back on a session that's still there. The password is - # saved by then, so a realtime service that can't be reached mustn't fail the change. + # The password is saved by now, so a realtime service that can't be reached mustn't fail the change. def disconnect_signed_out_sessions reset_remote_connections rescue => error diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 6d017260..835166e6 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -55,6 +55,19 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest copy.assert_redirected_to new_session_url end + test "changing the password along with the avatar still signs this browser back in and closes 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) }) + + put user_profile_url, params: { user: { password: "a new secret123456", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } } + assert_redirected_to user_profile_url + assert users(:david).reload.avatar.attached? + + get user_profile_url + assert_response :success + 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 2262dcac..8dbe0ea4 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -58,6 +58,28 @@ class UserTest < ActiveSupport::TestCase users(:david).update!(password: "a new secret123456") end + test "changing the password closes live connections even when the user is saved again before commit" 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) }) + + User.transaction do + users(:david).update!(password: "a new secret123456") + users(:david).update!(name: "Dave") + end + end + + test "a password change that rolls back keeps every session and connection" do + ActionCable.server.expects(:remote_connections).never + + assert_no_changes -> { users(:david).sessions.count } do + User.transaction do + users(:david).update!(password: "a new secret123456") + raise ActiveRecord::Rollback + end + end + end + test "changing other details keeps every session" do ActionCable.server.expects(:remote_connections).never From 160ba295b5e24fc77a1b8d9c192b7f8f3f8bc66b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 11:36:16 -0700 Subject: [PATCH 05/10] End sign-in links made before a password change A transfer link signed only the user id, so one copied before the password changed still started a session for up to four hours after. Bind it to the password salt with generates_token_for, so a new password ends those links too. Links made before this upgrade stop working; they last four hours at most. --- app/models/user/transferable.rb | 11 ++++- .../sessions/transfers_controller_test.rb | 11 +++++ test/models/user/transferable_test.rb | 40 +++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 test/models/user/transferable_test.rb diff --git a/app/models/user/transferable.rb b/app/models/user/transferable.rb index 096913aa..3a9ace04 100644 --- a/app/models/user/transferable.rb +++ b/app/models/user/transferable.rb @@ -3,13 +3,20 @@ module User::Transferable TRANSFER_LINK_EXPIRY_DURATION = 4.hours + included do + # Bound to the password, so changing it also ends sign-in links made before. + generates_token_for :transfer, expires_in: TRANSFER_LINK_EXPIRY_DURATION do + password_salt&.last(10) + end + end + class_methods do def find_by_transfer_id(id) - find_signed(id, purpose: :transfer) + find_by_token_for(:transfer, id) end end def transfer_id - signed_id(purpose: :transfer, expires_in: TRANSFER_LINK_EXPIRY_DURATION) + generate_token_for(:transfer) end end diff --git a/test/controllers/sessions/transfers_controller_test.rb b/test/controllers/sessions/transfers_controller_test.rb index 6efb37f4..18322a88 100644 --- a/test/controllers/sessions/transfers_controller_test.rb +++ b/test/controllers/sessions/transfers_controller_test.rb @@ -19,4 +19,15 @@ class Sessions::TransfersControllerTest < ActionDispatch::IntegrationTest assert_redirected_to root_url assert parsed_cookies.signed[:session_token] end + + test "update rejects a link made before the password changed" do + user = users(:david) + transfer_id = user.transfer_id + user.update!(password: "a new secret123456") + + put session_transfer_url(transfer_id) + + assert_response :bad_request + assert_nil parsed_cookies.signed[:session_token] + end end diff --git a/test/models/user/transferable_test.rb b/test/models/user/transferable_test.rb new file mode 100644 index 00000000..00ce337e --- /dev/null +++ b/test/models/user/transferable_test.rb @@ -0,0 +1,40 @@ +require "test_helper" + +class User::TransferableTest < ActiveSupport::TestCase + test "a transfer id finds its user" do + assert_equal users(:david), User.find_by_transfer_id(users(:david).transfer_id) + end + + test "a transfer id expires" do + transfer_id = users(:david).transfer_id + + travel User::TRANSFER_LINK_EXPIRY_DURATION + 1.second + + assert_nil User.find_by_transfer_id(transfer_id) + end + + test "a transfer id made before a password change stops working" do + transfer_id = users(:david).transfer_id + + users(:david).update!(password: "a new secret123456") + + assert_nil User.find_by_transfer_id(transfer_id) + assert_equal users(:david), User.find_by_transfer_id(users(:david).transfer_id) + end + + test "other changes keep a transfer id working" do + transfer_id = users(:david).transfer_id + + users(:david).update!(name: "Dave", email_address: "dave@37signals.com") + + assert_equal users(:david), User.find_by_transfer_id(transfer_id) + end + + test "a transfer id finds only users in scope" do + transfer_id = users(:david).transfer_id + + users(:david).deactivate + + assert_nil User.active.find_by_transfer_id(transfer_id) + end +end From 44a92ff6c6645c71c1106dc0638ecb8b76b8d8d5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 12:17:14 -0700 Subject: [PATCH 06/10] Ask for the current password to change it Changing the password now ends every other session, so whoever holds one could shut the owner out of their own account with a new password. The profile asks for the current password whenever the password changes; without it, or with a wrong one, nothing is saved and the page says why. Other profile edits don't need it, and the admin reset-password script still works without it. Attempts are rate limited like signing in, 10 in 3 minutes per address, so the current password can't be guessed here instead. --- app/controllers/users/profiles_controller.rb | 36 ++++++++-- app/helpers/translations_helper.rb | 1 + app/models/user.rb | 8 +++ app/views/users/profiles/show.html.erb | 10 +++ test/controllers/cached_responses_test.rb | 2 +- .../users/profiles_controller_test.rb | 66 +++++++++++++++++-- test/script/admin/reset_password_test.rb | 21 ++++++ 7 files changed, 134 insertions(+), 10 deletions(-) create mode 100644 test/script/admin/reset_password_test.rb diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index bdf13c47..a6acc994 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -1,16 +1,23 @@ class Users::ProfilesController < ApplicationController before_action :set_user + # Limited like signing in, so the current password can't be guessed here instead. + rate_limit to: 10, within: 3.minutes, only: :update, if: :changing_password?, + with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." } + def show @direct_memberships, @shared_memberships = Current.user.memberships.with_ordered_room.partition { |m| m.room.direct? } end def update - @user.update user_params - start_new_session_for @user if @user.saved_change_to_password_digest? + if @user.update user_params + start_new_session_for @user if @user.saved_change_to_password_digest? - redirect_to user_profile_url, notice: update_notice + redirect_to user_profile_url, notice: update_notice + else + redirect_to user_profile_url, alert: update_alert + end end private @@ -19,10 +26,31 @@ class Users::ProfilesController < ApplicationController end def user_params - params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact + permitted = params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact + + if changing_password? + permitted.merge(password_challenge: params[:user][:password_challenge].to_s) + else + permitted + end + end + + # As has_secure_password reads it: an empty password leaves the current one alone. + def changing_password? + !params.dig(:user, :password).to_s.empty? end def update_notice params[:user][:avatar] ? "It may take up to 30 minutes to change everywhere." : "✓" end + + def update_alert + if !@user.errors.include?(:password_challenge) + @user.errors.full_messages.to_sentence + elsif params[:user][:password_challenge].blank? + "Enter your current password to change it." + else + "That isn't your current password." + end + end end diff --git a/app/helpers/translations_helper.rb b/app/helpers/translations_helper.rb index 6142ab49..6c269ab3 100644 --- a/app/helpers/translations_helper.rb +++ b/app/helpers/translations_helper.rb @@ -2,6 +2,7 @@ module TranslationsHelper TRANSLATIONS = { email_address: { "🇺🇸": "Enter your email address", "🇪🇸": "Introduce tu correo electrónico", "🇫🇷": "Entrez votre adresse courriel", "🇮🇳": "अपना ईमेल पता दर्ज करें", "🇩🇪": "Geben Sie Ihre E-Mail-Adresse ein", "🇧🇷": "Insira seu endereço de email", "🇯🇵": "メールアドレスを入力してください" }, password: { "🇺🇸": "Enter your password", "🇪🇸": "Introduce tu contraseña", "🇫🇷": "Saisissez votre mot de passe", "🇮🇳": "अपना पासवर्ड दर्ज करें", "🇩🇪": "Geben Sie Ihr Passwort ein", "🇧🇷": "Insira sua senha", "🇯🇵": "パスワードを入力してください" }, + current_password: { "🇺🇸": "Enter your current password", "🇪🇸": "Introduce tu contraseña actual", "🇫🇷": "Saisissez votre mot de passe actuel", "🇮🇳": "अपना वर्तमान पासवर्ड दर्ज करें", "🇩🇪": "Geben Sie Ihr aktuelles Passwort ein", "🇧🇷": "Insira sua senha atual", "🇯🇵": "現在のパスワードを入力してください" }, update_password: { "🇺🇸": "Change password", "🇪🇸": "Cambiar contraseña", "🇫🇷": "Changer le mot de passe", "🇮🇳": "पासवर्ड बदलें", "🇩🇪": "Passwort ändern", "🇧🇷": "Alterar senha", "🇯🇵": "パスワードを変更" }, user_name: { "🇺🇸": "Enter your name", "🇪🇸": "Introduce tu nombre", "🇫🇷": "Entrez votre nom", "🇮🇳": "अपना नाम दर्ज करें", "🇩🇪": "Geben Sie Ihren Namen ein", "🇧🇷": "Insira seu nome", "🇯🇵": "お名前を入力してください" }, account_name: { "🇺🇸": "Name this account", "🇪🇸": "Nombre de esta cuenta", "🇫🇷": "Nommez ce compte", "🇮🇳": "इस खाते का नाम दें", "🇩🇪": "Benennen Sie dieses Konto", "🇧🇷": "Dê um nome a essa conta", "🇯🇵": "アカウントに名前を付ける" }, diff --git a/app/models/user.rb b/app/models/user.rb index de25de1b..716088e8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -18,6 +18,7 @@ class User < ApplicationRecord enum :status, %i[ active deactivated banned ], default: :active has_secure_password validations: false + validate :password_challenge_matches, unless: -> { password_challenge.nil? } after_create_commit :grant_membership_to_open_rooms after_update :sign_out_all_sessions, if: :saved_change_to_password_digest? @@ -51,6 +52,13 @@ class User < ApplicationRecord end private + # The profile sets password_challenge whenever it changes the password; scripts that reset one don't. + def password_challenge_matches + unless password_digest_was.present? && BCrypt::Password.new(password_digest_was).is_password?(password_challenge) + errors.add :password_challenge, :invalid + end + end + # A new password is how someone shuts out whoever else got into their account, so it ends every session, copies # of the changing browser's cookie included. Connections close once that commits, registered on the transaction # because a later save in it would replace the saved change an after_commit condition reads. diff --git a/app/views/users/profiles/show.html.erb b/app/views/users/profiles/show.html.erb index 1fe919da..368853d8 100644 --- a/app/views/users/profiles/show.html.erb +++ b/app/views/users/profiles/show.html.erb @@ -69,6 +69,16 @@ +
+ <%= translation_button(:current_password) %> + + +
+
<%= translation_button(:update_password) %> diff --git a/test/controllers/cached_responses_test.rb b/test/controllers/cached_responses_test.rb index df4102c4..9168f3bf 100644 --- a/test/controllers/cached_responses_test.rb +++ b/test/controllers/cached_responses_test.rb @@ -149,7 +149,7 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest Session.unstub(:find_by) ResponseCache.instance.unstub(:write) - put user_profile_url, params: { user: { password: "a new secret123456" } }, + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }, headers: { "Sec-Fetch-Site" => "same-origin", "Origin" => "http://once.campfire.test" } assert_redirected_to user_profile_url diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 835166e6..4e93520c 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -33,7 +33,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest other_device.get user_profile_url other_device.assert_response :success - put user_profile_url, params: { user: { password: "a new secret123456" } } + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } assert_redirected_to user_profile_url other_device.get user_profile_url @@ -46,7 +46,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest 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" } } + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } assert_not_equal copied_cookie, cookies[:session_token] copy = open_session @@ -60,7 +60,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest 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) }) - put user_profile_url, params: { user: { password: "a new secret123456", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } } + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } } assert_redirected_to user_profile_url assert users(:david).reload.avatar.attached? @@ -73,7 +73,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest 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) }) - put user_profile_url, params: { user: { password: "a new secret123456" } } + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } assert_redirected_to user_profile_url end @@ -82,7 +82,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest other_session = users(:david).sessions.first ActionCable.server.stubs(:remote_connections).raises(RuntimeError.new("cable down")) - put user_profile_url, params: { user: { password: "a new secret123456" } } + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } assert_redirected_to user_profile_url assert_not Session.exists?(other_session.id) @@ -98,9 +98,65 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_redirected_to user_profile_url end + test "changing the password with the current one" do + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } + + assert_redirected_to user_profile_url + assert_equal "✓", flash[:notice] + assert users(:david).reload.authenticate("a new secret123456") + end + + test "changing the password without the current one changes nothing" do + assert_password_change_refused "Enter your current password to change it.", + user: { name: "Someone else", password: "a new secret123456" } + end + + test "changing the password with a wrong current one changes nothing" do + assert_password_change_refused "That isn't your current password.", + user: { name: "Someone else", password: "a new secret123456", password_challenge: "not my password" } + end + + test "editing the profile without changing the password doesn't need the current one" do + put user_profile_url, params: { user: { name: "Dave", bio: "Acrobat", password: "" } } + + assert_redirected_to user_profile_url + assert_equal "Dave", users(:david).reload.name + assert users(:david).authenticate("secret123456") + end + + test "attempts to change the password are rate limited like signing in" do + Users::ProfilesController.cache_store.stubs(:increment).returns(11) + + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } } + assert_redirected_to user_profile_url + assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert] + assert users(:david).reload.authenticate("secret123456") + + put user_profile_url, params: { user: { name: "Dave" } } + assert_equal "Dave", users(:david).reload.name + end + test "updates are limited to the current user" do put user_profile_url(users(:jason)), params: { user: { name: "John Doe" } } assert_equal "Jason", users(:jason).reload.name end + + private + def assert_password_change_refused(alert, params) + other_device = open_session + other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" } + ActionCable.server.expects(:remote_connections).never + + assert_no_difference -> { Session.count } do + put user_profile_url, params: params + end + + assert_redirected_to user_profile_url + assert_equal alert, flash[:alert] + assert users(:david).reload.authenticate("secret123456") + assert_equal "David", users(:david).name + other_device.get user_profile_url + other_device.assert_response :success + end end diff --git a/test/script/admin/reset_password_test.rb b/test/script/admin/reset_password_test.rb new file mode 100644 index 00000000..637bb950 --- /dev/null +++ b/test/script/admin/reset_password_test.rb @@ -0,0 +1,21 @@ +require "test_helper" + +class ResetPasswordScriptTest < ActiveSupport::TestCase + test "resets a password without the current one and signs out every session" do + assert_output "Password has been reset\n" do + run_script "david@37signals.com", "a new secret123456" + end + + assert users(:david).reload.authenticate("a new secret123456") + assert_empty users(:david).sessions + end + + private + def run_script(*arguments) + original_arguments = ARGV.dup + ARGV.replace(arguments) + load Rails.root.join("script/admin/reset-password").to_s + ensure + ARGV.replace(original_arguments) + end +end From 6cab9240978ec6d305d5f043bacda1da800d0d12 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 12:39:03 -0700 Subject: [PATCH 07/10] Remove push subscriptions on a password change, and post them again A push subscription belongs to the user, not a session, so a device signed out by a password change kept getting notifications. Changing the password now removes every subscription along with the sessions, in the same transaction. A browser still signed in keeps its own subscription in its service worker, so the notifications controller posts it once per session: the bell names the session by a digest of its id, never its token, and the browser remembers the last one it posted for. That post never unsubscribes the browser when the server refuses it. A request that authenticated just before the change could put a subscription back after it. The create action now checks the session still exists and saves in one immediate transaction, after validating the endpoint outside it. --- .../users/push_subscriptions_controller.rb | 27 ++++--- .../users/push_subscriptions_helper.rb | 7 ++ .../controllers/notifications_controller.js | 21 +++++- app/models/user.rb | 6 +- app/views/rooms/involvements/_bell.html.erb | 3 +- .../users/profiles_controller_test.rb | 14 +++- .../push_subscriptions_controller_test.rb | 48 +++++++++++++ test/models/user_test.rb | 20 +++++- test/system/push_subscription_resync_test.rb | 71 +++++++++++++++++++ 9 files changed, 201 insertions(+), 16 deletions(-) create mode 100644 app/helpers/users/push_subscriptions_helper.rb create mode 100644 test/system/push_subscription_resync_test.rb diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index 8e263b23..e4e46524 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -5,17 +5,14 @@ class Users::PushSubscriptionsController < ApplicationController end def create - if subscription = @push_subscriptions.find_by(push_subscription_params) - # Existing endpoints must pass current validations - if subscription.valid? - subscription.touch - head :ok - else - head :unprocessable_entity - end + subscription = @push_subscriptions.find_or_initialize_by(push_subscription_params) + subscription.user_agent = request.user_agent if subscription.new_record? + + # Existing endpoints must pass current validations too + if subscription.valid? + head save_while_signed_in(subscription) ? :ok : :unauthorized else - subscription = @push_subscriptions.create push_subscription_params.merge(user_agent: request.user_agent) - head subscription.persisted? ? :ok : :unprocessable_entity + head :unprocessable_entity end end @@ -29,6 +26,16 @@ class Users::PushSubscriptionsController < ApplicationController @push_subscriptions = Current.user.push_subscriptions end + # A password change ends every session and subscription. A request that authenticated just before it mustn't + # put one back, so the session is checked under the same write lock as the save. Validation, which looks up + # the endpoint's address, runs before taking the lock. + def save_while_signed_in(subscription) + Push::Subscription.transaction do + Session.exists?(Current.session.id) && + (subscription.new_record? ? subscription.save!(validate: false) : subscription.touch) + end + end + def push_subscription_params params.require(:push_subscription).permit(:endpoint, :p256dh_key, :auth_key) end diff --git a/app/helpers/users/push_subscriptions_helper.rb b/app/helpers/users/push_subscriptions_helper.rb new file mode 100644 index 00000000..57c681f9 --- /dev/null +++ b/app/helpers/users/push_subscriptions_helper.rb @@ -0,0 +1,7 @@ +module Users::PushSubscriptionsHelper + # Names the session without revealing its token, so the browser can tell when it's in a new one. The creation + # time keeps an id reused by a new database from looking like the same session. + def push_subscription_session_marker + Digest::SHA256.hexdigest("push-subscription-session-#{Current.session.id}-#{Current.session.created_at.to_f}") if Current.session + end +end diff --git a/app/javascript/controllers/notifications_controller.js b/app/javascript/controllers/notifications_controller.js index 3ce53a5c..b3698a31 100644 --- a/app/javascript/controllers/notifications_controller.js +++ b/app/javascript/controllers/notifications_controller.js @@ -4,8 +4,10 @@ import { pageIsTurboPreview } from "helpers/turbo_helpers" import { onNextEventLoopTick } from "helpers/timing_helpers" import { getCookie, setCookie } from "lib/cookie" +const PUSH_SUBSCRIPTION_SYNCED_KEY = "push-subscription-synced-session" + export default class extends Controller { - static values = { subscriptionsUrl: String } + static values = { subscriptionsUrl: String, session: String } static targets = [ "notAllowedNotice", "bell", "details" ] static classes = [ "attention" ] @@ -25,6 +27,8 @@ export default class extends Controller { this.#showBellAlert() } } + + this.#resyncPushSubscriptionOncePerSession() } } @@ -109,6 +113,21 @@ export default class extends Controller { if (!response.ok) subscription.unsubscribe() } + // A password change removes the account's subscriptions, the one this browser holds included. Post it again + // once per session, so a browser that's still signed in keeps getting notifications. A failure here leaves the + // browser's own subscription alone: the next session tries again. + async #resyncPushSubscriptionOncePerSession() { + if (this.#allowed && this.sessionValue && localStorage.getItem(PUSH_SUBSCRIPTION_SYNCED_KEY) !== this.sessionValue) { + const registration = await this.#serviceWorkerRegistration + const subscription = await registration?.pushManager?.getSubscription() + + if (Notification.permission === "granted" && subscription) { + await post(this.subscriptionsUrlValue, { body: this.#extractJsonPayloadAsString(subscription), responseKind: "turbo-stream" }) + localStorage.setItem(PUSH_SUBSCRIPTION_SYNCED_KEY, this.sessionValue) + } + } + } + async #requestPermissionAndSubscribe(registration) { const permission = await Notification.requestPermission() if (permission === "granted") this.#subscribe(registration) diff --git a/app/models/user.rb b/app/models/user.rb index 716088e8..07f4bd03 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -60,10 +60,12 @@ class User < ApplicationRecord end # A new password is how someone shuts out whoever else got into their account, so it ends every session, copies - # of the changing browser's cookie included. Connections close once that commits, registered on the transaction - # because a later save in it would replace the saved change an after_commit condition reads. + # of the changing browser's cookie included, and every push subscription; browsers still signed in post theirs + # again. Connections close once that commits, registered on the transaction because a later save in it would + # replace the saved change an after_commit condition reads. def sign_out_all_sessions sessions.delete_all + push_subscriptions.delete_all self.class.current_transaction.after_commit { disconnect_signed_out_sessions } end diff --git a/app/views/rooms/involvements/_bell.html.erb b/app/views/rooms/involvements/_bell.html.erb index d9fa32c5..266bdfb0 100644 --- a/app/views/rooms/involvements/_bell.html.erb +++ b/app/views/rooms/involvements/_bell.html.erb @@ -1,6 +1,7 @@ + data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path %>" + data-notifications-session-value="<%= push_subscription_session_marker %>" data-notifications-attention-class="btn--pulsing"> <%= turbo_frame_tag dom_id(room, :involvement), data: { controller: "turbo-frame", action: "notifications:ready@window->turbo-frame#load", turbo_frame_url_param: room_involvement_path(room) } do %>