From 99bff3cb9759716bad56a852861de6f2bba6c64c Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Thu, 27 Nov 2025 16:33:48 +1100 Subject: [PATCH] Check user passwords against HIBP on login --- Gemfile | 1 + Gemfile.lock | 1 + .../compromised_passwords_controller.rb | 22 +++ app/controllers/sessions_controller.rb | 39 +++++- app/helpers/users_helper.rb | 25 ++++ app/models/events/user_event.rb | 5 + .../password/compromised_component.rb | 9 ++ app/views/compromised_passwords/show.html.erb | 57 ++++++++ config/initializers/rack_attack.rb | 18 +-- config/locales/en.yml | 27 ++++ config/routes.rb | 2 + lib/password_breach_checker.rb | 17 +++ .../compromised_passwords_controller_test.rb | 77 +++++++++++ test/functional/sessions_controller_test.rb | 127 ++++++++++++++++++ test/lib/password_breach_checker_test.rb | 39 ++++++ test/test_helper.rb | 1 + test/unit/helpers/users_helper_test.rb | 41 ++++++ 17 files changed, 498 insertions(+), 10 deletions(-) create mode 100644 app/controllers/compromised_passwords_controller.rb create mode 100644 app/views/components/events/user_event/password/compromised_component.rb create mode 100644 app/views/compromised_passwords/show.html.erb create mode 100644 lib/password_breach_checker.rb create mode 100644 test/functional/compromised_passwords_controller_test.rb create mode 100644 test/lib/password_breach_checker_test.rb create mode 100644 test/unit/helpers/users_helper_test.rb diff --git a/Gemfile b/Gemfile index 789a522cdbb..38b7fd683ab 100644 --- a/Gemfile +++ b/Gemfile @@ -52,6 +52,7 @@ gem "rack-attack", "~> 6.8" gem "rqrcode", "~> 3.1" gem "rotp", "~> 6.2" gem "unpwn", "~> 1.0" +gem "pwned", "~> 2.4" gem "webauthn", "~> 3.4" gem "browser", "~> 6.2" gem "bcrypt", "~> 3.1" diff --git a/Gemfile.lock b/Gemfile.lock index e3b1610c0c7..945e174b894 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1003,6 +1003,7 @@ DEPENDENCIES puma (~> 6.6) puma-plugin-statsd (~> 2.7) pundit (~> 2.5) + pwned (~> 2.4) rack (~> 3.2) rack-attack (~> 6.8) rack-sanitizer (~> 2.0) diff --git a/app/controllers/compromised_passwords_controller.rb b/app/controllers/compromised_passwords_controller.rb new file mode 100644 index 00000000000..ea862064cb5 --- /dev/null +++ b/app/controllers/compromised_passwords_controller.rb @@ -0,0 +1,22 @@ +class CompromisedPasswordsController < ApplicationController + layout "hammy" + + before_action :validate_session + + def show + @user = User.find_by(id: session[:compromised_password_user_id]) + return redirect_to sign_in_path unless @user + + # Send password reset email if not already sent + return if session[:compromised_password_email_sent] + @user.forgot_password! + PasswordMailer.change_password(@user).deliver_later + session[:compromised_password_email_sent] = true + end + + private + + def validate_session + redirect_to sign_in_path if session[:compromised_password_user_id].blank? + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index ab9a1199e3a..3c7a0f74011 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -11,6 +11,7 @@ class SessionsController < Clearance::SessionsController before_action :ensure_not_blocked, only: %i[create] before_action :find_user, only: %i[create] + before_action :check_password_compromised, only: %i[create] before_action :require_mfa, only: %i[create] before_action :find_mfa_user, only: %i[webauthn_create otp_create] before_action :validate_otp, only: %i[otp_create] @@ -151,7 +152,10 @@ def set_login_flash end def url_after_create - if current_user.mfa_recommended_not_yet_enabled? + if session.delete(:password_compromised) + flash[:alert] = t("sessions.create.password_compromised_warning") + new_password_path + elsif current_user.mfa_recommended_not_yet_enabled? new_totp_path elsif current_user.mfa_recommended_weak_level_enabled? edit_settings_path @@ -160,6 +164,39 @@ def url_after_create end end + def check_password_compromised + return unless @user + + checker = PasswordBreachChecker.new(params.dig(:session, :password)) + return unless checker.breached? + + StatsD.increment "login.password_compromised" + + if @user.mfa_enabled? + handle_compromised_password_with_mfa + else + handle_compromised_password_without_mfa + end + end + + def handle_compromised_password_with_mfa + @user.record_event!(Events::UserEvent::PASSWORD_COMPROMISED, + request:, mfa_enabled: true, action_taken: "warning_shown") + session[:password_compromised] = true + flash[:alert] = t(".password_compromised_warning") + end + + def handle_compromised_password_without_mfa + @user.record_event!(Events::UserEvent::PASSWORD_COMPROMISED, + request:, mfa_enabled: false, action_taken: "email_reset_required") + + reset_session + + session[:compromised_password_user_id] = @user.id + + redirect_to compromised_password_path + end + def ensure_not_blocked user = User.find_by_blocked(who) return unless user&.blocked_email diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 2525181b134..21d6251b6ac 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -10,4 +10,29 @@ def twitter_url(user) def show_policies_acknowledge_banner?(user) user.present? && !user.policies_acknowledged? end + + def obfuscate_email(email) + return email if email.blank? + + local, domain = email.split("@", 2) + return email unless domain + + domain_name, tld = domain.split(".", 2) + return email unless tld + + obfuscated_local = obfuscate_part(local, 1) + obfuscated_domain = obfuscate_part(domain_name, 1) + + "#{obfuscated_local}@#{obfuscated_domain}.#{tld}" + end + + private + + def obfuscate_part(str, visible_chars) + return str if str.length <= visible_chars + 1 + + visible = str[0, visible_chars] + hidden_length = str.length - visible_chars + "#{visible}#{'*' * hidden_length}" + end end diff --git a/app/models/events/user_event.rb b/app/models/events/user_event.rb index d65119fd907..9eff3c49b6d 100644 --- a/app/models/events/user_event.rb +++ b/app/models/events/user_event.rb @@ -45,5 +45,10 @@ class Events::UserEvent < ApplicationRecord PASSWORD_CHANGED = define_event "user:password:changed" + PASSWORD_COMPROMISED = define_event "user:password:compromised" do + attribute :mfa_enabled, :boolean + attribute :action_taken, :string + end + POLICIES_ACKNOWLEDGED = define_event "user:policies:acknowledged" end diff --git a/app/views/components/events/user_event/password/compromised_component.rb b/app/views/components/events/user_event/password/compromised_component.rb new file mode 100644 index 00000000000..d57bb99c909 --- /dev/null +++ b/app/views/components/events/user_event/password/compromised_component.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class Events::UserEvent::Password::CompromisedComponent < Events::TableDetailsComponent + def view_template + plain t(".mfa_status", status: additional.mfa_enabled ? t(".mfa_enabled") : t(".mfa_disabled")) + br + plain t(".action_taken", action: additional.action_taken.humanize) + end +end diff --git a/app/views/compromised_passwords/show.html.erb b/app/views/compromised_passwords/show.html.erb new file mode 100644 index 00000000000..e54a01f6ffe --- /dev/null +++ b/app/views/compromised_passwords/show.html.erb @@ -0,0 +1,57 @@ +<% @title = t('.title') %> +<% add_breadcrumb t("sign_in"), sign_in_path %> +<% add_breadcrumb t('.title') %> + +
+
+ <%= render AlertComponent.new(style: :warning) do %> + <%= t('.heading') %> + <% end %> + +

+ <%= t('.subheading') %> +

+ +

+ <%= t('.explanation_html') %> +

+ +
+

+ <%= t('.email_sent') %> +

+

+ <%= obfuscate_email(@user.email) %> +

+
+ +

+ <%= t('.next_steps') %> +

+ +
    +
  1. <%= t('.step_1') %>
    2. +
  2. <%= t('.step_2') %>
    3. +
  3. <%= t('.step_3') %>
    4. +
  4. <%= t('.step_4_html', link: link_to(t('.step_4_link_text'), new_totp_path, class: "text-orange-500 dark:text-orange-400 hover:underline")) %>
    5. +
+ +
+

+ <%= t('.learn_more') %> +

+
    +
  • <%= t('.learn_more_hibp_html', link: link_to(t('.learn_more_hibp_link_text'), "https://haveibeenpwned.com", target: "_blank", rel: "noopener", class: "text-orange-500 dark:text-orange-400 hover:underline")) %>
    • +
  • <%= t('.learn_more_mfa_html', link: link_to(t('.learn_more_mfa_link_text'), "https://guides.rubygems.org/setting-up-multifactor-authentication/", target: "_blank", rel: "noopener", class: "text-orange-500 dark:text-orange-400 hover:underline")) %>
    • +
+
+ +
+ <%= render ButtonComponent.new(t('.back_to_sign_in'), sign_in_path, type: :link, color: :secondary) %> +
+ +

+ <%= t('.need_help_html', email: mail_to("support@rubygems.org", "support@rubygems.org", class: "text-orange-500 dark:text-orange-400 hover:underline")) %> +

+
+
diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 5fce0e26e4f..3c5eff22830 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -27,15 +27,15 @@ class Rack::Attack # Key: "rack::attack:#{Time.now.to_i/:period}:logins/ip:#{req.ip}" protected_ui_actions = [ - { controller: "sessions", action: "create" }, - { controller: "users", action: "create" }, - { controller: "passwords", action: "edit" }, - { controller: "sessions", action: "authenticate" }, - { controller: "passwords", action: "create" }, - { controller: "profiles", action: "update" }, - { controller: "profiles", action: "destroy" }, - { controller: "email_confirmations", action: "create" }, - { controller: "reverse_dependencies", action: "index" } + { controller: "sessions", action: "create" }, + { controller: "users", action: "create" }, + { controller: "passwords", action: "edit" }, + { controller: "sessions", action: "authenticate" }, + { controller: "passwords", action: "create" }, + { controller: "profiles", action: "update" }, + { controller: "profiles", action: "destroy" }, + { controller: "email_confirmations", action: "create" }, + { controller: "reverse_dependencies", action: "index" } ] otp_create_action = { controller: "sessions", action: "otp_create" } diff --git a/config/locales/en.yml b/config/locales/en.yml index 494bc3f656a..5cce6d7bd91 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -842,6 +842,27 @@ en: notice: Please confirm your password to continue. create: account_blocked: Your account was blocked by rubygems team. Please email support@rubygems.org to recover your account. + password_compromised_warning: "Your password was found in a data breach. Reset it now to keep your account secure." + compromised_passwords: + show: + title: Password Reset Required + heading: Password found in data breach + subheading: Your RubyGems.org account remains secure + explanation_html: The password you entered has been found in a public data breach from another website. This doesn't mean your RubyGems.org account was compromised, but because passwords are often reused, we require a reset to keep your account secure. + email_sent: "We've sent a password reset link to:" + next_steps: "Here's what to do next:" + step_1: Check your email inbox (and spam folder) for the password reset link + step_2: Click the link and create a new, unique password you don't use elsewhere + step_3: Sign in with your new password + step_4_html: Consider %{link} for additional security + step_4_link_text: enabling multi-factor authentication + learn_more: Learn more + learn_more_hibp_html: Check if your email appears in other breaches at %{link} + learn_more_hibp_link_text: Have I Been Pwned + learn_more_mfa_html: Protect your account with %{link} + learn_more_mfa_link_text: multi-factor authentication + back_to_sign_in: Back to Sign In + need_help_html: "Need help? Contact %{email}" stats: index: title: Stats @@ -1035,3 +1056,9 @@ en: api_key_gem_html: "Gem: %{gem}" api_key_mfa: "MFA: %{mfa}" not_required: "Not required" + password: + password_compromised: "Password Compromised" + mfa_status: "MFA Status: %{status}" + mfa_enabled: "Enabled" + mfa_disabled: "Disabled" + action_taken: "Action: %{action}" diff --git a/config/routes.rb b/config/routes.rb index 867501f64d3..8cb4a94a635 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -262,6 +262,8 @@ post 'webauthn_edit', to: 'passwords#webauthn_edit', as: :webauthn_edit end + resource :compromised_password, only: %i[show] + resource :session, only: %i[create destroy] do post 'otp_create', to: 'sessions#otp_create', as: :otp_create post 'webauthn_create', to: 'sessions#webauthn_create', as: :webauthn_create diff --git a/lib/password_breach_checker.rb b/lib/password_breach_checker.rb new file mode 100644 index 00000000000..e91a8a838c7 --- /dev/null +++ b/lib/password_breach_checker.rb @@ -0,0 +1,17 @@ +class PasswordBreachChecker + def initialize(password) + @password = Pwned::Password.new(password.to_s) + end + + def breached? + @password.pwned? + rescue Pwned::TimeoutError, Pwned::Error => e + Rails.logger.warn "HIBP check failed: #{e.class}" + StatsD.increment "login.hibp_check.error" + false + end + + def inspect + "#" + end +end diff --git a/test/functional/compromised_passwords_controller_test.rb b/test/functional/compromised_passwords_controller_test.rb new file mode 100644 index 00000000000..bf2fd4519b7 --- /dev/null +++ b/test/functional/compromised_passwords_controller_test.rb @@ -0,0 +1,77 @@ +require "test_helper" + +class CompromisedPasswordsControllerTest < ActionController::TestCase + include ActionMailer::TestHelper + + setup do + @user = create(:user) + end + + context "on GET to show" do + context "with valid session" do + setup do + @controller.session[:compromised_password_user_id] = @user.id + end + + should "display password reset info page" do + get :show + + assert_response :success + assert_select "span.font-semibold", I18n.t("compromised_passwords.show.heading") + assert_select "a[href=?]", sign_in_path + end + + should "enqueue password reset email on first visit" do + assert_enqueued_emails 1 do + get :show + end + + assert_response :success + end + + should "not re-send password reset email on page refresh" do + @controller.session[:compromised_password_email_sent] = true + + assert_enqueued_emails 0 do + get :show + end + + assert_response :success + end + + should "update user confirmation_token for password reset" do + original_token = @user.confirmation_token + + get :show + + assert_not_equal original_token, @user.reload.confirmation_token + end + + should "set email_sent flag in session after sending" do + refute @controller.session[:compromised_password_email_sent] + + get :show + + assert @controller.session[:compromised_password_email_sent] + end + end + + context "without valid session" do + should "redirect to sign in when session is missing" do + get :show + + assert_redirected_to sign_in_path + end + + should "redirect to sign in when user no longer exists" do + deleted_user_id = @user.id + @user.destroy + @controller.session[:compromised_password_user_id] = deleted_user_id + + get :show + + assert_redirected_to sign_in_path + end + end + end +end diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb index 7d27638d56c..8c8ff69514d 100644 --- a/test/functional/sessions_controller_test.rb +++ b/test/functional/sessions_controller_test.rb @@ -818,4 +818,131 @@ def verify_challenge format: :html ) end + + context "when password is compromised" do + setup do + @user = create(:user, email_confirmed: true, handle: "compromised_user") + PasswordBreachChecker.any_instance.stubs(:breached?).returns(true) + end + + context "when user has no MFA" do + context "on POST to create" do + setup do + post :create, params: { session: { who: "compromised_user", password: PasswordHelpers::SECURE_TEST_PASSWORD } } + end + + should redirect_to("the compromised password page") { compromised_password_path } + + should "store user id in session for password reset" do + assert_equal @user.id, @controller.session[:compromised_password_user_id] + end + + should "not sign in the user" do + refute_predicate @controller.request.env[:clearance], :signed_in? + end + + should "record compromised password event" do + event = @user.events.find_by(tag: Events::UserEvent::PASSWORD_COMPROMISED) + + assert_predicate event, :present? + refute event.additional["mfa_enabled"] + assert_equal "email_reset_required", event.additional["action_taken"] + end + end + end + + context "when user has MFA enabled" do + setup do + @user.enable_totp!(ROTP::Base32.random_base32, :ui_only) + end + + context "on POST to create" do + setup do + post :create, params: { session: { who: "compromised_user", password: PasswordHelpers::SECURE_TEST_PASSWORD } } + end + + should respond_with :success + + should "save user id in session for MFA" do + assert_equal @user.id, @controller.session[:mfa_user] + end + + should "set password_compromised flag in session" do + assert @controller.session[:password_compromised] + end + + should "show MFA prompt" do + assert page.has_content? "Multi-factor authentication" + end + + should "set flash alert with warning" do + assert_equal I18n.t("sessions.create.password_compromised_warning"), flash[:alert] + end + + should "record compromised password event" do + event = @user.events.find_by(tag: Events::UserEvent::PASSWORD_COMPROMISED) + + assert_predicate event, :present? + assert_equal "warning_shown", event.additional["action_taken"] + assert event.additional["mfa_enabled"] + end + end + + context "after successful MFA" do + setup do + post :create, params: { session: { who: "compromised_user", password: PasswordHelpers::SECURE_TEST_PASSWORD } } + @controller.session[:mfa_user] = @user.id + post :otp_create, params: { otp: ROTP::TOTP.new(@user.totp_seed).now } + end + + should redirect_to("the password reset page") { new_password_path } + + should "clear password_compromised flag from session" do + refute @controller.session[:password_compromised] + end + + should "make user logged in" do + assert_predicate @controller.request.env[:clearance], :signed_in? + end + end + end + end + + context "when HIBP API fails" do + setup do + @user = create(:user, email_confirmed: true, handle: "safe_user") + PasswordBreachChecker.any_instance.stubs(:breached?).returns(false) + end + + context "on POST to create" do + setup do + post :create, params: { session: { who: "safe_user", password: PasswordHelpers::SECURE_TEST_PASSWORD } } + end + + should redirect_to("the dashboard") { dashboard_path } + + should "sign in the user" do + assert_predicate @controller.request.env[:clearance], :signed_in? + end + end + end + + context "when credentials are wrong" do + setup do + @user = create(:user, email_confirmed: true, handle: "wrong_password_user") + PasswordBreachChecker.any_instance.expects(:breached?).never + end + + context "on POST to create with wrong password" do + setup do + post :create, params: { session: { who: "wrong_password_user", password: "wrong_password" } } + end + + should respond_with :unauthorized + + should "not sign in the user" do + refute_predicate @controller.request.env[:clearance], :signed_in? + end + end + end end diff --git a/test/lib/password_breach_checker_test.rb b/test/lib/password_breach_checker_test.rb new file mode 100644 index 00000000000..94b5695f107 --- /dev/null +++ b/test/lib/password_breach_checker_test.rb @@ -0,0 +1,39 @@ +require "test_helper" + +class PasswordBreachCheckerTest < ActiveSupport::TestCase + context "#breached?" do + should "return true for compromised passwords" do + Unpwn.any_instance.stubs(:acceptable?).returns(false) + + assert_predicate PasswordBreachChecker.new("password123"), :breached? + end + + should "return false for safe passwords" do + Unpwn.any_instance.stubs(:pwned?).returns(false) + + refute_predicate PasswordBreachChecker.new("very-secure-password"), :breached? + end + + should "return true for nil password" do + assert_predicate PasswordBreachChecker.new(nil), :breached? + end + + should "return true for empty password" do + assert_predicate PasswordBreachChecker.new(""), :breached? + end + + should "return false and increment counter on timeout" do + Pwned::Password.any_instance.stubs(:pwned?).raises(Pwned::TimeoutError) + StatsD.expects(:increment).with("login.hibp_check.error") + + refute_predicate PasswordBreachChecker.new("password123"), :breached? + end + + should "return false and increment counter on API error" do + Pwned::Password.any_instance.stubs(:pwned?).raises(Pwned::Error) + StatsD.expects(:increment).with("login.hibp_check.error") + + refute_predicate PasswordBreachChecker.new("password123"), :breached? + end + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 3603cd9e334..427352bec8f 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -46,6 +46,7 @@ WebMock.disable_net_connect!( allow_localhost: true, allow: [ + "api.pwnedpasswords.com", "chromedriver.storage.googleapis.com", "search", # DevContainer services "selenium", diff --git a/test/unit/helpers/users_helper_test.rb b/test/unit/helpers/users_helper_test.rb new file mode 100644 index 00000000000..ac4430dbce2 --- /dev/null +++ b/test/unit/helpers/users_helper_test.rb @@ -0,0 +1,41 @@ +require "test_helper" + +class UsersHelperTest < ActionView::TestCase + context "obfuscate_email" do + should "obfuscate standard email" do + assert_equal "g*******@e******.com", obfuscate_email("gem-user@example.com") + end + + should "obfuscate email with short local part" do + assert_equal "j**@g****.com", obfuscate_email("joe@gmail.com") + end + + should "handle very short email parts" do + assert_equal "j@x.io", obfuscate_email("j@x.io") + end + + should "handle single character local part" do + assert_equal "a@e******.com", obfuscate_email("a@example.com") + end + + should "handle subdomain in TLD" do + assert_equal "u***@m***.co.uk", obfuscate_email("user@mail.co.uk") + end + + should "return nil for nil input" do + assert_nil obfuscate_email(nil) + end + + should "return empty string for empty input" do + assert_equal "", obfuscate_email("") + end + + should "return original for invalid email without @" do + assert_equal "notanemail", obfuscate_email("notanemail") + end + + should "return original for invalid email without domain" do + assert_equal "user@", obfuscate_email("user@") + end + end +end