CVE-2024-21654: Improper Authentication (CWE-287)
value: Rubygems.org is the Ruby community's gem hosting service. Rubygems.org users with MFA enabled would normally be protected from account takeover in the case of email account takeover. However, a workaround on the forgotten password form allows an attacker to bypass the MFA requirement and takeover the account. This vulnerability has been patched in commit 0b3272a.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
CVSS v3 base score: 9.8
Published: 2024-01-12T21:15Z
Repository: https://github.com/rubygems/rubygems.org
Language: YAML
Files changed: app/controllers/adoptions_controller.rb, app/controllers/application_controller.rb, app/controllers/concerns/session_verifiable.rb, app/controllers/passwords_controller.rb, app/controllers/sessions_controller.rb, app/views/passwords/edit.html.erb, config/locales/de.yml, config/locales/en.yml, config/locales/es.yml, config/locales/fr.yml, config/locales/ja.yml, config/locales/nl.yml, config/locales/pt-BR.yml, config/locales/zh-CN.yml, config/locales/zh-TW.yml, test/functional/passwords_controller_test.rb, test/integration/password_reset_test.rb
Commit date: 2024-01-08 03:42:57 +0100
Commit message: Protect forgotten password changes from MFA bypass.

It was previously possible to submit the password update form directly
with only the confirmation token, bypassing MFA protections.

Diff: diff --git a/app/controllers/adoptions_controller.rb b/app/controllers/adoptions_controller.rb
index 1cc0b4858..5c3cdb58a 100644
--- a/app/controllers/adoptions_controller.rb
+++ b/app/controllers/adoptions_controller.rb
@@ -1,11 +1,11 @@
 class AdoptionsController < ApplicationController
   include SessionVerifiable
 
   before_action :find_rubygem
   before_action :verify_ownership_requestable
-  before_action :redirect_to_verify, if: -> { current_user_is_owner? && !password_session_active? }
+  before_action :redirect_to_verify, if: -> { current_user_is_owner? && !verified_session_active? }
 
   def index
     @ownership_call     = @rubygem.ownership_call
     @user_request       = @rubygem.ownership_requests.find_by(user: current_user)
     @ownership_requests = @rubygem.ownership_requests.includes(:user)
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index d0be78968..92aed8dd6 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -151,14 +151,10 @@ class ApplicationController < ActionController::Base
     response.headers["Cache-Control"] = "no-cache, no-store"
     response.headers["Pragma"] = "no-cache"
     response.headers["Expires"] = "Fri, 01 Jan 1990 00:00:00 GMT"
   end
 
-  def password_session_active?
-    session[:verification] && session[:verification] > Time.current && session.fetch(:verified_user, "") == current_user.id
-  end
-
   def set_error_context_user
     return unless current_user
 
     Rails.error.set_context(
       user_id: current_user.id,
diff --git a/app/controllers/concerns/session_verifiable.rb b/app/controllers/concerns/session_verifiable.rb
index 2f7a6d68b..864e9c957 100644
--- a/app/controllers/concerns/session_verifiable.rb
+++ b/app/controllers/concerns/session_verifiable.rb
@@ -4,11 +4,11 @@ module SessionVerifiable
   class_methods do
     def verify_session_before(**opts)
       before_action :redirect_to_signin, **opts, unless: :signed_in?
       before_action :redirect_to_new_mfa, **opts, if: :mfa_required_not_yet_enabled?
       before_action :redirect_to_settings_strong_mfa_required, **opts, if: :mfa_required_weak_level_enabled?
-      before_action :redirect_to_verify, **opts, unless: :password_session_active?
+      before_action :redirect_to_verify, **opts, unless: :verified_session_active?
     end
   end
 
   private
 
@@ -23,7 +23,18 @@ module SessionVerifiable
 
     def redirect_to_verify
       session[:redirect_uri] = verify_session_redirect_path
       redirect_to verify_session_path
     end
+
+    def session_verified
+      session[:verified_user] = current_user.id
+      session[:verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
+    end
+
+    def verified_session_active?
+      session[:verification] &&
+        session[:verification] > Time.current &&
+        session.fetch(:verified_user, "") == current_user.id
+    end
   end
 end
diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb
index 13304eaaf..250b25789 100644
--- a/app/controllers/passwords_controller.rb
+++ b/app/controllers/passwords_controller.rb
@@ -1,42 +1,50 @@
 class PasswordsController < Clearance::PasswordsController
   include MfaExpiryMethods
   include WebauthnVerifiable
+  include SessionVerifiable
 
   before_action :validate_confirmation_token, only: %i[edit otp_edit webauthn_edit]
   after_action :delete_mfa_expiry_session, only: %i[otp_edit webauthn_edit]
 
+  # By default, clearance expects the token to be submitted with the password update.
+  # We already invalidated the token when the user became verified by token(+mfa).
+  skip_before_action :ensure_existing_user, only: %i[update]
+  # Instead of the token, we now require the user to have been verified recently.
+  verify_session_before only: %i[update]
+
   def edit
     if @user.mfa_enabled?
       @otp_verification_url = otp_edit_user_password_url(@user, token: @user.confirmation_token)
       setup_webauthn_authentication(form_url: webauthn_edit_user_password_url(token: @user.confirmation_token))
 
       create_new_mfa_expiry
 
       render template: "multifactor_auths/prompt"
     else
+      # When user doesn't have mfa, a valid token is a full "magic link" sign in.
+      verified_sign_in
       render template: "passwords/edit"
     end
   end
 
   def update
-    @user = find_user_for_update
-
-    if @user.update_password password_from_password_reset_params
-      @user.reset_api_key! if reset_params[:reset_api_key] == "true"
-      @user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true"
-      sign_in @user
+    if current_user.update_password password_from_password_reset_params
+      current_user.reset_api_key! if reset_params[:reset_api_key] == "true"
+      current_user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true"
       redirect_to url_after_update
       session[:password_reset_token] = nil
     else
       flash_failure_after_update
       render template: "passwords/edit"
     end
   end
 
   def otp_edit
     if otp_edit_conditions_met?
+      # When the user identified by the email token submits adequate totp, they are logged in
+      verified_sign_in
       render template: "passwords/edit"
     elsif !session_active?
       login_failure(t("multifactor_auths.session_expired"))
     else
       login_failure(t("multifactor_auths.incorrect_otp"))
@@ -49,15 +57,24 @@ class PasswordsController < Clearance::PasswordsController
       return
     end
 
     return login_failure(@webauthn_error) unless webauthn_credential_verified?
 
+    # When the user identified by the email token submits verified webauthn, they are logged in
+    verified_sign_in
     render template: "passwords/edit"
   end
 
   private
 
+  def verified_sign_in
+    sign_in @user
+    session_verified
+    @user.update!(confirmation_token: nil)
+    StatsD.increment "login.success"
+  end
+
   def url_after_update
     dashboard_path
   end
 
   def reset_params
@@ -79,6 +96,11 @@ class PasswordsController < Clearance::PasswordsController
 
   def login_failure(message)
     flash.now.alert = message
     render template: "multifactor_auths/prompt", status: :unauthorized
   end
+
+  def redirect_to_verify
+    session[:redirect_uri] = verify_session_redirect_path
+    redirect_to verify_session_path, alert: t("verification_expired")
+  end
 end
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index 10b354c84..d696234a4 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -1,8 +1,9 @@
 class SessionsController < Clearance::SessionsController
   include MfaExpiryMethods
   include WebauthnVerifiable
+  include SessionVerifiable
 
   before_action :redirect_to_signin, unless: :signed_in?, only: %i[verify webauthn_authenticate authenticate]
   before_action :redirect_to_new_mfa, if: :mfa_required_not_yet_enabled?, only: %i[verify webauthn_authenticate authenticate]
   before_action :redirect_to_settings_strong_mfa_required, if: :mfa_required_weak_level_enabled?, only: %i[verify webauthn_authenticate authenticate]
   before_action :ensure_not_blocked, only: %i[create webauthn_full_create]
@@ -91,12 +92,11 @@ class SessionsController < Clearance::SessionsController
   end
 
   private
 
   def mark_verified
-    session[:verified_user] = current_user.id
-    session[:verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
+    session_verified
     redirect_to session.delete(:redirect_uri) || root_path
   end
 
   def verify_user
     current_user.authenticated? verify_password_params[:password]
diff --git a/app/views/passwords/edit.html.erb b/app/views/passwords/edit.html.erb
index fa6d7fc38..dea3648b5 100644
--- a/app/views/passwords/edit.html.erb
+++ b/app/views/passwords/edit.html.erb
@@ -1,11 +1,11 @@
 <% @title = t('.title') %>
 
 <%= form_for(:password_reset,
-             :url => user_password_path(@user, :token => @user.confirmation_token),
+             :url => user_password_path(current_user),
              :html => { :method => :put }) do |form| %>
-  <%= error_messages_for @user %>
+  <%= error_messages_for current_user %>
   <div class="password_field">
     <%= form.label :password, "Password", :class => 'form__label' %>
     <%= form.password_field :password, autocomplete: 'new-password', class: 'form__input' %>
   </div>
   <div class="form__checkbox">
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 9e6df4cee..caae7be98 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -1,10 +1,11 @@
 ---
 de:
   credentials_required:
   edit: Bearbeiten
   failure_when_forbidden:
+  verification_expired:
   feed_latest: RubyGems.org | Neueste Gems
   feed_subscribed: RubyGems.org | Abonnierte Gems
   footer_about_html:
   footer_sponsors_html:
   footer_join_rt_html:
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 9e16b3176..6edb85654 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1,10 +1,11 @@
 ---
 en:
   credentials_required: Credentials required
   edit: Edit
   failure_when_forbidden: Please double check the URL or try submitting it again.
+  verification_expired: The verification has expired. Please verify again.
   feed_latest: RubyGems.org | Latest Gems
   feed_subscribed: RubyGems.org | Subscribed Gems
   footer_about_html:
     RubyGems.org is the Ruby community&rsquo;s gem hosting service.
     Instantly <a href="%{publish_docs}">publish your gems</a> and then <a href="%{install_docs}">install them</a>.
diff --git a/config/locales/es.yml b/config/locales/es.yml
index f4e3a95a7..147eb0f5f 100644
--- a/config/locales/es.yml
+++ b/config/locales/es.yml
@@ -1,10 +1,11 @@
 ---
 es:
   credentials_required: Credenciales requeridas
   edit: Editar
   failure_when_forbidden: Por favor verifica la URL o inténtalo nuevamente.
+  verification_expired:
   feed_latest: RubyGems.org | Gemas más recientes
   feed_subscribed: RubyGems.org | Suscripciones a gemas
   footer_about_html: RubyGems.org es el servicio de alojamiento de Gemas de la comunidad
     de Ruby. <a href="%{publish_docs}">Publica tus gemas</a> instantáneamente y luego
     <a href="%{install_docs}">instálalas</a>. Usa <a href="%{api_docs}">la API</a>
diff --git a/config/locales/fr.yml b/config/locales/fr.yml
index eed71b455..812d4d95e 100644
--- a/config/locales/fr.yml
+++ b/config/locales/fr.yml
@@ -1,10 +1,11 @@
 ---
 fr:
   credentials_required:
   edit: Modification
   failure_when_forbidden: Veuillez vérifier l'URL ou réessayer.
+  verification_expired:
   feed_latest: RubyGems.org | Derniers Gems
   feed_subscribed: RubyGems.org | Gems abonnés
   footer_about_html: RubyGems.org est le service d&rsquo;hébergement de la communauté
     Ruby. Publiez vos gems instantanément et utilisez-les. Utilisez l'API pour interagir
     et trouver des informations sur les gems disponibles. Contribuez et améliorez
diff --git a/config/locales/ja.yml b/config/locales/ja.yml
index 61cac4f0a..3a421f6dc 100644
--- a/config/locales/ja.yml
+++ b/config/locales/ja.yml
@@ -1,10 +1,11 @@
 ---
 ja:
   credentials_required: 認証情報が必要です
   edit: 編集
   failure_when_forbidden: URLを見返すか、再度送信してみてください。
+  verification_expired:
   feed_latest: RubyGems.org | 最新のgemの一覧
   feed_subscribed: RubyGems.org | 購読したgemの一覧
   footer_about_html: RubyGems.orgはRubyコミュニティのgemのホスティングサービスです。すぐに<a href="%{publish_docs}">gemを公開</a>して<a
     href="%{install_docs}">インストール</a>できます。<a href="%{api_docs}">API</a>を使用して<a href="%{gem_list}">利用可能なgem</a>の詳細を調べられます。ご自身が<a
     href="%{contributing_docs}">貢献者となり</a>サイトをより良くしてください。
diff --git a/config/locales/nl.yml b/config/locales/nl.yml
index b7b2207b3..8f514914b 100644
--- a/config/locales/nl.yml
+++ b/config/locales/nl.yml
@@ -1,10 +1,11 @@
 ---
 nl:
   credentials_required:
   edit: Wijzig
   failure_when_forbidden: Controleer het webadres, en probeer het opnieuw.
+  verification_expired:
   feed_latest: RubyGems.org | Nieuwste Gems
   feed_subscribed: RubyGems.org | Geabonneerde Gems
   footer_about_html: RubyGems.org is de gem hosting service van de Ruby community.
     <a href="%{publish_docs}">Publiceer</a> en <a href="%{install_docs}">installeer</a>
     je gems direct. Gebruik <a href="%{api_docs}">de API</a> om meer informatie over
diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml
index 37379ea5f..2832d81cc 100644
--- a/config/locales/pt-BR.yml
+++ b/config/locales/pt-BR.yml
@@ -1,10 +1,11 @@
 ---
 pt-BR:
   credentials_required:
   edit: Editar
   failure_when_forbidden: Por favor, confira a URL ou tente submetê-la novamente.
+  verification_expired:
   feed_latest: RubyGems.org | Últimas Gems
   feed_subscribed: RubyGems.org | Gems do seu Feed
   footer_about_html: RubyGems.org é o serviço de hospedagem de gems da comunidade
     Ruby. Publique e instale suas gems instantaneamente. Use a API para interagir
     e encontrar mais informações sobre gems disponíveis. Torne-se um contribuidor
diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml
index f1147b993..090afee28 100644
--- a/config/locales/zh-CN.yml
+++ b/config/locales/zh-CN.yml
@@ -1,10 +1,11 @@
 ---
 zh-CN:
   credentials_required: 需要凭证
   edit: 编辑
   failure_when_forbidden: 请再次确认 URL 或尝试重新提交
+  verification_expired:
   feed_latest: RubyGems.org | 最新的 Gem
   feed_subscribed: RubyGems.org | 订阅的 Gem
   footer_about_html: RubyGems.org 是 Ruby 社区的 Gem 托管服务。 立即 <a href="%{publish_docs}">发布您的
     Gem</a> 并 <a href="%{install_docs}">安装它们</a>。 使用 <a href="%{api_docs}">API</a>
     来查找更多 <a href="%{gem_list}">可用的 Gem</a>。 <a href="%{contributing_docs}">快来成为一名贡献者吧！</a>
diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml
index 0e023a49a..526a8cb8d 100644
--- a/config/locales/zh-TW.yml
+++ b/config/locales/zh-TW.yml
@@ -1,10 +1,11 @@
 ---
 zh-TW:
   credentials_required:
   edit: 編輯
   failure_when_forbidden: 請確認 URL 或再次提交
+  verification_expired:
   feed_latest: RubyGems.org | 最新 Gems
   feed_subscribed: RubyGems.org | 訂閱 Gems
   footer_about_html: RubyGems.org 是 Ruby 社群的 Gem 套件管理服務，讓你能立即地發佈及安裝你的 Gem 套件，並且利用
     API 查詢及操作可用 Gem 的詳細資訊。<br />現在就成為貢獻者，貢獻一己之力來改善本站。
   footer_sponsors_html:
diff --git a/test/functional/passwords_controller_test.rb b/test/functional/passwords_controller_test.rb
index 9328fe839..e63640901 100644
--- a/test/functional/passwords_controller_test.rb
+++ b/test/functional/passwords_controller_test.rb
@@ -33,10 +33,18 @@ class PasswordsControllerTest < ActionController::TestCase
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should respond_with :success
 
+      should "sign in the user" do
+        assert_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
+      should "invalidate the confirmation_token" do
+        assert_nil @user.reload.confirmation_token
+      end
+
       should "display edit form" do
         page.assert_text("Reset password")
         page.assert_selector("input[type=password][autocomplete=new-password]")
       end
     end
@@ -47,10 +55,14 @@ class PasswordsControllerTest < ActionController::TestCase
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should redirect_to("the home page") { root_path }
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "warn about invalid url" do
         assert_equal "Please double check the URL or try submitting it again.", flash[:alert]
       end
     end
 
@@ -60,10 +72,14 @@ class PasswordsControllerTest < ActionController::TestCase
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should respond_with :success
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "display otp form" do
         assert page.has_content?("Multi-factor authentication")
         assert page.has_content?("OTP code")
         assert page.has_button?("Authenticate")
       end
@@ -78,10 +94,14 @@ class PasswordsControllerTest < ActionController::TestCase
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should respond_with :success
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "display webauthn prompt" do
         assert page.has_button?("Authenticate with security device")
       end
 
       should "not display recovery code prompt" do
@@ -95,10 +115,14 @@ class PasswordsControllerTest < ActionController::TestCase
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should respond_with :success
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "display webauthn prompt" do
         assert page.has_button?("Authenticate with security device")
       end
 
       should "display recovery code prompt" do
@@ -113,10 +137,14 @@ class PasswordsControllerTest < ActionController::TestCase
         get :edit, params: { token: @user.confirmation_token, user_id: @user.id }
       end
 
       should respond_with :success
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "display webauthn prompt" do
         assert page.has_button?("Authenticate with security device")
       end
 
       should "display otp prompt" do
@@ -140,13 +168,22 @@ class PasswordsControllerTest < ActionController::TestCase
           post :otp_edit, params: { user_id: @user.id, token: @user.confirmation_token, otp: ROTP::TOTP.new(@user.totp_seed).now }
         end
 
         should respond_with :success
 
+        should "sign in the user" do
+          assert_predicate @controller.request.env[:clearance], :signed_in?
+        end
+
+        should "invalidate the confirmation_token" do
+          assert_nil @user.reload.confirmation_token
+        end
+
         should "display edit form" do
           page.assert_text("Reset password")
         end
+
         should "clear mfa_expires_at" do
           assert_nil @controller.session[:mfa_expires_at]
         end
       end
 
@@ -156,10 +193,14 @@ class PasswordsControllerTest < ActionController::TestCase
           post :otp_edit, params: { user_id: @user.id, token: @user.confirmation_token, otp: "eatthis" }
         end
 
         should respond_with :unauthorized
 
+        should "not sign in the user" do
+          refute_predicate @controller.request.env[:clearance], :signed_in?
+        end
+
         should "alert about otp being incorrect" do
           assert_equal "Your OTP code is incorrect.", flash[:alert]
         end
       end
 
@@ -220,10 +261,18 @@ class PasswordsControllerTest < ActionController::TestCase
         )
       end
 
       should respond_with :success
 
+      should "sign in the user" do
+        assert_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
+      should "invalidate the confirmation_token" do
+        assert_nil @user.reload.confirmation_token
+      end
+
       should "display edit form" do
         page.assert_text("Reset password")
       end
 
       should "clear mfa_expires_at" do
@@ -236,10 +285,14 @@ class PasswordsControllerTest < ActionController::TestCase
         post :webauthn_edit, params: { user_id: @user.id, token: @user.confirmation_token }, format: :html
       end
 
       should respond_with :unauthorized
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "set flash notice" do
         assert_equal "Credentials required", flash[:alert]
       end
     end
 
@@ -264,13 +317,18 @@ class PasswordsControllerTest < ActionController::TestCase
         )
       end
 
       should respond_with :unauthorized
 
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
+
       should "set flash notice" do
         assert_equal "WebAuthn::ChallengeVerificationError", flash[:alert]
       end
+
       should "still have the webauthn form url" do
         assert_not_nil page.find(".js-webauthn-session--form")[:action]
       end
     end
 
@@ -320,109 +378,165 @@ class PasswordsControllerTest < ActionController::TestCase
       @api_key = @user.api_key
       @new_api_key = create(:api_key, owner: @user)
       @old_encrypted_password = @user.encrypted_password
     end
 
-    context "with reset_api_key and invalid password" do
+    context "when not signed in" do
       setup do
         put :update, params: {
           user_id: @user.id,
           token: @user.confirmation_token,
-          password_reset: { reset_api_key: "true", password: "pass" }
+          password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
         }
       end
 
-      should respond_with :success
+      should redirect_to("the sign in page") { sign_in_path }
 
       should "not change api_key" do
         assert_equal(@user.reload.api_key, @api_key)
       end
       should "not change password" do
         assert_equal(@user.reload.encrypted_password, @old_encrypted_password)
       end
+      should "not sign in the user" do
+        refute_predicate @controller.request.env[:clearance], :signed_in?
+      end
     end
 
-    context "without reset_api_key and valid password" do
+    context "when signed in" do
       setup do
-        put :update, params: {
-          user_id: @user.id,
-          token: @user.confirmation_token,
-          password_reset: { password: PasswordHelpers::SECURE_TEST_PASSWORD }
-        }
+        sign_in_as @user
+        session[:verification] = 10.minutes.from_now
+        session[:verified_user] = @user.id
       end
 
-      should respond_with :found
-
-      should "not change api_key" do
-        assert_equal(@user.reload.api_key, @api_key)
+      teardown do
+        session[:verification] = nil
+        session[:verified_user] = nil
       end
-      should "change password" do
-        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
-      end
-    end
 
-    context "with reset_api_key false and valid password" do
-      setup do
-        put :update, params: {
-          user_id: @user.id,
-          token: @user.confirmation_token,
-          password_reset: { reset_api_key: "false", password: PasswordHelpers::SECURE_TEST_PASSWORD }
-        }
-      end
+      context "with invalid password" do
+        setup do
+          put :update, params: {
+            user_id: @user.id,
+            token: @user.confirmation_token,
+            password_reset: { reset_api_key: "true", password: "pass" }
+          }
+        end
 
-      should respond_with :found
+        should respond_with :success
 
-      should "not change api_key" do
-        assert_equal(@user.reload.api_key, @api_key)
-      end
-      should "change password" do
-        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
+        should "not change api_key" do
+          assert_equal(@user.reload.api_key, @api_key)
+        end
+        should "not change password" do
+          assert_equal(@user.reload.encrypted_password, @old_encrypted_password)
+        end
       end
-    end
 
-    context "with reset_api_key and valid password" do
-      setup do
-        put :update, params: {
-          user_id: @user.id,
-          token: @user.confirmation_token,
-          password_reset: { reset_api_key: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
-        }
-      end
+      context "with a valid password" do
+        context "when verification has expired" do
+          setup do
+            travel 16.minutes do
+              put :update, params: {
+                user_id: @user.id,
+                token: @user.confirmation_token,
+                password_reset: { password: PasswordHelpers::SECURE_TEST_PASSWORD }
+              }
+            end
+          end
 
-      should respond_with :found
+          should set_flash[:alert]
+          should redirect_to("the verification page") { verify_session_path }
 
-      should "change api_key" do
-        refute_equal(@user.reload.api_key, @api_key)
-      end
-      should "change password" do
-        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
-      end
-      should "not delete new api key" do
-        refute_predicate @new_api_key.reload, :destroyed?
-        refute_empty @user.reload.api_keys
-      end
-    end
+          should "not sign the user out" do
+            assert_predicate @controller.request.env[:clearance], :signed_in?
+          end
+        end
 
-    context "with reset_api_key and reset_api_keys and valid password" do
-      setup do
-        put :update, params: {
-          user_id: @user.id,
-          token: @user.confirmation_token,
-          password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
-        }
-      end
+        context "without reset_api_key" do
+          setup do
+            put :update, params: {
+              user_id: @user.id,
+              token: @user.confirmation_token,
+              password_reset: { password: PasswordHelpers::SECURE_TEST_PASSWORD }
+            }
+          end
 
-      should respond_with :found
+          should respond_with :found
 
-      should "change api_key" do
-        refute_equal(@user.reload.api_key, @api_key)
-      end
-      should "change password" do
-        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
-      end
-      should "expire new api key" do
-        assert_empty @user.reload.api_keys.unexpired
-        refute_empty @user.reload.api_keys.expired
+          should "not change api_key" do
+            assert_equal(@user.reload.api_key, @api_key)
+          end
+          should "change password" do
+            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
+          end
+        end
+
+        context "with reset_api_key false" do
+          setup do
+            put :update, params: {
+              user_id: @user.id,
+              token: @user.confirmation_token,
+              password_reset: { reset_api_key: "false", password: PasswordHelpers::SECURE_TEST_PASSWORD }
+            }
+          end
+
+          should respond_with :found
+
+          should "not change api_key" do
+            assert_equal(@user.reload.api_key, @api_key)
+          end
+          should "change password" do
+            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
+          end
+        end
+
+        context "with reset_api_key" do
+          setup do
+            put :update, params: {
+              user_id: @user.id,
+              token: @user.confirmation_token,
+              password_reset: { reset_api_key: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
+            }
+          end
+
+          should respond_with :found
+
+          should "change api_key" do
+            refute_equal(@user.reload.api_key, @api_key)
+          end
+          should "change password" do
+            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
+          end
+          should "not delete new api key" do
+            refute_predicate @new_api_key.reload, :destroyed?
+            refute_empty @user.reload.api_keys
+          end
+        end
+
+        context "with reset_api_key and reset_api_keys" do
+          setup do
+            put :update, params: {
+              user_id: @user.id,
+              token: @user.confirmation_token,
+              password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
+            }
+          end
+
+          should respond_with :found
+
+          should "change api_key" do
+            refute_equal(@user.reload.api_key, @api_key)
+          end
+          should "change password" do
+            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
+          end
+          should "expire new api key" do
+            assert_empty @user.reload.api_keys.unexpired
+            refute_empty @user.reload.api_keys.expired
+          end
+        end
       end
     end
   end
 end
diff --git a/test/integration/password_reset_test.rb b/test/integration/password_reset_test.rb
index 8ed199be1..d56ab2a93 100644
--- a/test/integration/password_reset_test.rb
+++ b/test/integration/password_reset_test.rb
@@ -53,17 +53,40 @@ class PasswordResetTest < SystemTest
   test "resetting a password with a blank password" do
     forgot_password_with @user.email
 
     visit password_reset_link
 
+    assert page.has_content?("Sign out")
+
     fill_in "Password", with: ""
     click_button "Save this password"
 
     assert page.has_content? "Password can't be blank."
-    assert page.has_content? "Sign in"
+    assert page.has_content? "Reset password"
+
+    # try again
+    fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
+    click_button "Save this password"
+
+    assert @user.reload.authenticated? PasswordHelpers::SECURE_TEST_PASSWORD
+  end
+
+  test "resetting a password but waiting too long after token auth" do
+    forgot_password_with @user.email
+
+    visit password_reset_link
+
+    fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
+
+    travel 16.minutes do
+      click_button "Save this password"
+
+      assert page.has_content? "verification has expired. Please verify again."
+    end
   end
 
+
   test "resetting a password when signed in" do
     visit sign_in_path
 
     fill_in "Email or Username", with: @user.email
     fill_in "Password", with: @user.password
@@ -76,10 +99,12 @@ class PasswordResetTest < SystemTest
     fill_in "Email address", with: @user.email
     perform_enqueued_jobs { click_button "Reset password" }
 
     visit password_reset_link
 
+    assert page.has_content?("Sign out")
+
     fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
     click_button "Save this password"
 
     assert @user.reload.authenticated? PasswordHelpers::SECURE_TEST_PASSWORD
   end
@@ -88,17 +113,19 @@ class PasswordResetTest < SystemTest
     @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
     forgot_password_with @user.email
 
     visit password_reset_link
 
+    refute page.has_content?("Sign out")
+
     fill_in "otp", with: ROTP::TOTP.new(@user.totp_seed).now
     click_button "Authenticate"
 
+    assert page.has_content?("Sign out")
+
     fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
     click_button "Save this password"
-
-    assert page.has_content?("Sign out")
   end
 
   test "resetting a password when mfa is enabled but mfa session is expired" do
     @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
     forgot_password_with @user.email
@@ -139,10 +166,11 @@ class PasswordResetTest < SystemTest
 
     forgot_password_with @user.email
 
     visit password_reset_link
 
+    refute page.has_content? "Sign out"
     assert page.has_content? "Multi-factor authentication"
     assert page.has_content? "Security Device"
     assert page.has_content? "Recovery code"
     assert_not_nil page.find(".js-webauthn-session--form")[:action]
 

Vulnerable code:   before_action :redirect_to_verify, if: -> { current_user_is_owner? && !password_session_active? }
  def password_session_active?
    session[:verification] && session[:verification] > Time.current && session.fetch(:verified_user, "") == current_user.id
  end

      before_action :redirect_to_verify, **opts, unless: :password_session_active?
    @user = find_user_for_update

    if @user.update_password password_from_password_reset_params
      @user.reset_api_key! if reset_params[:reset_api_key] == "true"
      @user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true"
      sign_in @user
    session[:verified_user] = current_user.id
    session[:verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
             :url => user_password_path(@user, :token => @user.confirmation_token),
  <%= error_messages_for @user %>
    context "with reset_api_key and invalid password" do
          password_reset: { reset_api_key: "true", password: "pass" }
      should respond_with :success
    context "without reset_api_key and valid password" do
        put :update, params: {
          user_id: @user.id,
          token: @user.confirmation_token,
          password_reset: { password: PasswordHelpers::SECURE_TEST_PASSWORD }
        }
      should respond_with :found

      should "not change api_key" do
        assert_equal(@user.reload.api_key, @api_key)
      should "change password" do
        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
      end
    end
    context "with reset_api_key false and valid password" do
      setup do
        put :update, params: {
          user_id: @user.id,
          token: @user.confirmation_token,
          password_reset: { reset_api_key: "false", password: PasswordHelpers::SECURE_TEST_PASSWORD }
        }
      end
      should respond_with :found
      should "not change api_key" do
        assert_equal(@user.reload.api_key, @api_key)
      end
      should "change password" do
        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
    end
    context "with reset_api_key and valid password" do
      setup do
        put :update, params: {
          user_id: @user.id,
          token: @user.confirmation_token,
          password_reset: { reset_api_key: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
        }
      end
      should respond_with :found
      should "change api_key" do
        refute_equal(@user.reload.api_key, @api_key)
      end
      should "change password" do
        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
      end
      should "not delete new api key" do
        refute_predicate @new_api_key.reload, :destroyed?
        refute_empty @user.reload.api_keys
      end
    end
    context "with reset_api_key and reset_api_keys and valid password" do
      setup do
        put :update, params: {
          user_id: @user.id,
          token: @user.confirmation_token,
          password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
        }
      end
      should respond_with :found
      should "change api_key" do
        refute_equal(@user.reload.api_key, @api_key)
      end
      should "change password" do
        refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
      end
      should "expire new api key" do
        assert_empty @user.reload.api_keys.unexpired
        refute_empty @user.reload.api_keys.expired
    assert page.has_content? "Sign in"

    assert page.has_content?("Sign out")
Fixed code:   before_action :redirect_to_verify, if: -> { current_user_is_owner? && !verified_session_active? }
      before_action :redirect_to_verify, **opts, unless: :verified_session_active?

    def session_verified
      session[:verified_user] = current_user.id
      session[:verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
    end

    def verified_session_active?
      session[:verification] &&
        session[:verification] > Time.current &&
        session.fetch(:verified_user, "") == current_user.id
    end
  include SessionVerifiable
  # By default, clearance expects the token to be submitted with the password update.
  # We already invalidated the token when the user became verified by token(+mfa).
  skip_before_action :ensure_existing_user, only: %i[update]
  # Instead of the token, we now require the user to have been verified recently.
  verify_session_before only: %i[update]

      # When user doesn't have mfa, a valid token is a full "magic link" sign in.
      verified_sign_in
    if current_user.update_password password_from_password_reset_params
      current_user.reset_api_key! if reset_params[:reset_api_key] == "true"
      current_user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true"
      # When the user identified by the email token submits adequate totp, they are logged in
      verified_sign_in
    # When the user identified by the email token submits verified webauthn, they are logged in
    verified_sign_in
  def verified_sign_in
    sign_in @user
    session_verified
    @user.update!(confirmation_token: nil)
    StatsD.increment "login.success"
  end


  def redirect_to_verify
    session[:redirect_uri] = verify_session_redirect_path
    redirect_to verify_session_path, alert: t("verification_expired")
  end
  include SessionVerifiable
    session_verified
             :url => user_password_path(current_user),
  <%= error_messages_for current_user %>
  verification_expired:
  verification_expired: The verification has expired. Please verify again.
  verification_expired:
  verification_expired:
  verification_expired:
  verification_expired:
  verification_expired:
  verification_expired:
  verification_expired:
      should "sign in the user" do
        assert_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "invalidate the confirmation_token" do
        assert_nil @user.reload.confirmation_token
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end

        should "sign in the user" do
          assert_predicate @controller.request.env[:clearance], :signed_in?
        end

        should "invalidate the confirmation_token" do
          assert_nil @user.reload.confirmation_token
        end


        should "not sign in the user" do
          refute_predicate @controller.request.env[:clearance], :signed_in?
        end

      should "sign in the user" do
        assert_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "invalidate the confirmation_token" do
        assert_nil @user.reload.confirmation_token
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end

      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end


    context "when not signed in" do
          password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
      should redirect_to("the sign in page") { sign_in_path }
      should "not sign in the user" do
        refute_predicate @controller.request.env[:clearance], :signed_in?
      end
    context "when signed in" do
        sign_in_as @user
        session[:verification] = 10.minutes.from_now
        session[:verified_user] = @user.id
      teardown do
        session[:verification] = nil
        session[:verified_user] = nil
      context "with invalid password" do
        setup do
          put :update, params: {
            user_id: @user.id,
            token: @user.confirmation_token,
            password_reset: { reset_api_key: "true", password: "pass" }
          }
        end
        should respond_with :success
        should "not change api_key" do
          assert_equal(@user.reload.api_key, @api_key)
        end
        should "not change password" do
          assert_equal(@user.reload.encrypted_password, @old_encrypted_password)
        end
      context "with a valid password" do
        context "when verification has expired" do
          setup do
            travel 16.minutes do
              put :update, params: {
                user_id: @user.id,
                token: @user.confirmation_token,
                password_reset: { password: PasswordHelpers::SECURE_TEST_PASSWORD }
              }
            end
          end
          should set_flash[:alert]
          should redirect_to("the verification page") { verify_session_path }
          should "not sign the user out" do
            assert_predicate @controller.request.env[:clearance], :signed_in?
          end
        end
        context "without reset_api_key" do
          setup do
            put :update, params: {
              user_id: @user.id,
              token: @user.confirmation_token,
              password_reset: { password: PasswordHelpers::SECURE_TEST_PASSWORD }
            }
          end
          should respond_with :found
          should "not change api_key" do
            assert_equal(@user.reload.api_key, @api_key)
          end
          should "change password" do
            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
          end
        end

        context "with reset_api_key false" do
          setup do
            put :update, params: {
              user_id: @user.id,
              token: @user.confirmation_token,
              password_reset: { reset_api_key: "false", password: PasswordHelpers::SECURE_TEST_PASSWORD }
            }
          end

          should respond_with :found

          should "not change api_key" do
            assert_equal(@user.reload.api_key, @api_key)
          end
          should "change password" do
            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
          end
        end

        context "with reset_api_key" do
          setup do
            put :update, params: {
              user_id: @user.id,
              token: @user.confirmation_token,
              password_reset: { reset_api_key: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
            }
          end

          should respond_with :found

          should "change api_key" do
            refute_equal(@user.reload.api_key, @api_key)
          end
          should "change password" do
            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
          end
          should "not delete new api key" do
            refute_predicate @new_api_key.reload, :destroyed?
            refute_empty @user.reload.api_keys
          end
        end

        context "with reset_api_key and reset_api_keys" do
          setup do
            put :update, params: {
              user_id: @user.id,
              token: @user.confirmation_token,
              password_reset: { reset_api_key: "true", reset_api_keys: "true", password: PasswordHelpers::SECURE_TEST_PASSWORD }
            }
          end

          should respond_with :found

          should "change api_key" do
            refute_equal(@user.reload.api_key, @api_key)
          end
          should "change password" do
            refute_equal(@user.reload.encrypted_password, @old_encrypted_password)
          end
          should "expire new api key" do
            assert_empty @user.reload.api_keys.unexpired
            refute_empty @user.reload.api_keys.expired
          end
        end
    assert page.has_content?("Sign out")

    assert page.has_content? "Reset password"

    # try again
    fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
    click_button "Save this password"

    assert @user.reload.authenticated? PasswordHelpers::SECURE_TEST_PASSWORD
  end

  test "resetting a password but waiting too long after token auth" do
    forgot_password_with @user.email

    visit password_reset_link

    fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD

    travel 16.minutes do
      click_button "Save this password"

      assert page.has_content? "verification has expired. Please verify again."
    end

    assert page.has_content?("Sign out")

    refute page.has_content?("Sign out")

    assert page.has_content?("Sign out")

    refute page.has_content? "Sign out"