From 4a580302a55f1deb00b91665d7f444454979dc3a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 14:56:00 -0700 Subject: [PATCH] Check the current password whenever the saved password changes The profile asked for the current password when the request's password field was filled in. The check now follows the save itself: the model checks the challenge whenever the password digest is about to change, and the profile always hands it the challenge. Editing a name, avatar or email without a new password still needs none, and scripts that reset a password, which set no challenge, are unaffected. Only a string counts as a challenge, and the rate limit counts every non-empty one, so what's limited is exactly what gets checked. --- app/controllers/users/profiles_controller.rb | 27 +++++++++------- app/models/user.rb | 5 +-- .../users/profiles_controller_test.rb | 32 +++++++++++++++++++ test/models/user_test.rb | 8 +++++ 4 files changed, 58 insertions(+), 14 deletions(-) diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index a6acc99..6b30b74 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -2,7 +2,7 @@ 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?, + rate_limit to: 10, within: 3.minutes, only: :update, if: :answering_password_challenge?, with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." } def show @@ -25,19 +25,22 @@ class Users::ProfilesController < ApplicationController @user = Current.user end + # Always with the challenge, so the model checks it whenever the save would change the password, however the + # request spelled the new one. def user_params - 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 + params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact + .merge(password_challenge: password_challenge) 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? + # Only a string is a challenge, so the rate limit counts exactly what the model checks. + def password_challenge + challenge = params.dig(:user, :password_challenge) + challenge.is_a?(String) ? challenge : "" + end + + # Guessing the current password means sending one, so this is what the rate limit counts. + def answering_password_challenge? + !password_challenge.empty? end def update_notice @@ -47,7 +50,7 @@ class Users::ProfilesController < ApplicationController def update_alert if !@user.errors.include?(:password_challenge) @user.errors.full_messages.to_sentence - elsif params[:user][:password_challenge].blank? + elsif password_challenge.empty? "Enter your current password to change it." else "That isn't your current password." diff --git a/app/models/user.rb b/app/models/user.rb index 07f4bd0..6f01f6b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -18,7 +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? } + validate :password_challenge_matches, if: :will_save_change_to_password_digest?, unless: -> { password_challenge.nil? } after_create_commit :grant_membership_to_open_rooms after_update :sign_out_all_sessions, if: :saved_change_to_password_digest? @@ -52,7 +52,8 @@ class User < ApplicationRecord end private - # The profile sets password_challenge whenever it changes the password; scripts that reset one don't. + # The profile always sets password_challenge, and it's checked whenever the save would change the password; scripts + # that reset one don't set it. def password_challenge_matches unless password_digest_was.present? && BCrypt::Password.new(password_digest_was).is_password?(password_challenge) errors.add :password_challenge, :invalid diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 3d82162..49fcb3d 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -116,6 +116,18 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest user: { name: "Someone else", password: "a new secret123456", password_challenge: "not my password" } end + test "a new password needs the current one however the form spells it" do + assert_password_change_refused "Enter your current password to change it.", + user: { name: "Someone else", "password(1)" => "a new secret123456" } + end + + test "editing the profile with an empty current password doesn't need it" do + put user_profile_url, params: { user: { name: "Dave", password: "", password_challenge: "" } } + + assert_redirected_to user_profile_url + assert_equal "Dave", users(:david).reload.name + 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: "" } } @@ -132,10 +144,30 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest 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: { password: "a new secret123456" } } + assert_equal "Enter your current password to change it.", flash[:alert] + put user_profile_url, params: { user: { name: "Dave" } } assert_equal "Dave", users(:david).reload.name end + test "any current password sent counts toward the rate limit, however blank it looks" do + Users::ProfilesController.cache_store.stubs(:increment).returns(11) + + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: " " } } + assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert] + end + + test "a current password that isn't text is no current password" do + [ false, [], {}, 0 ].each do |challenge| + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: challenge } }, as: :json + + assert_redirected_to user_profile_url + assert_equal "Enter your current password to change it.", flash[:alert] + assert users(:david).reload.authenticate("secret123456") + end + end + test "changing the password removes the other devices' push subscriptions, and this browser can register its own again" do stub_web_push_dns_resolution endpoint = push_subscriptions(:david_chrome).attributes.slice("endpoint", "p256dh_key", "auth_key") diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 5cd445a..db29028 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -6,6 +6,14 @@ class UserTest < ActiveSupport::TestCase assert users(:david).valid? end + test "a password challenge is checked whenever the password would change" do + assert_not User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "") + assert_not User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "not my password") + assert User.find(users(:david).id).update(name: "Dave", password_challenge: "") + assert User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "secret123456") + assert User.find(users(:david).id).update(password: "another secret123456") + end + test "creating users grants membership to the open rooms" do assert_difference -> { Membership.count }, +Rooms::Open.count do create_new_user