fix(auth): hash MFA backup codes (#1629)

* fix(auth): hash MFA backup codes

* fix(auth): lock and filter backup code verification

* test(auth): assert consumed backup code digest

* fix(auth): strengthen backup code handling

* fix(auth): require otp secret before mfa enable

* test(auth): assert backup code digest consumption

* fix(auth): rehash legacy MFA backup codes

* fix(auth): narrow legacy backup code migration

app/controllers/mfa_controller.rb

@@ -11,8 +11,7 @@ class MfaController < ApplicationController
 
   def create
     if Current.user.verify_otp?(params[:code])
-      Current.user.enable_mfa!
-      @backup_codes = Current.user.otp_backup_codes
+      @backup_codes = Current.user.enable_mfa!
       render :backup_codes
     else
       Current.user.disable_mfa!

app/models/user.rb

@@ -9,9 +9,6 @@ class User < ApplicationRecord
   if encryption_ready?
     # MFA secrets
     encrypts :otp_secret, deterministic: true
-    # Note: otp_backup_codes is a PostgreSQL array column which doesn't support
-    # AR encryption. To encrypt it, a migration would be needed to change the
-    # column type from array to text/jsonb.
 
     # PII - emails (deterministic for lookups, downcase for case-insensitive)
     encrypts :email, deterministic: true, downcase: true
@@ -40,6 +37,8 @@ class User < ApplicationRecord
   has_many :shared_accounts, through: :account_shares, source: :account
   accepts_nested_attributes_for :family, update_only: true
 
+  MFA_BACKUP_CODE_COUNT = 8
+
   validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP }
   validate :ensure_valid_profile_image
   validates :default_period, inclusion: { in: Period::PERIODS.keys }
@@ -221,10 +220,17 @@ class User < ApplicationRecord
   end
 
   def enable_mfa!
+    raise ArgumentError, "OTP secret must be set before enabling MFA" if otp_secret.blank?
+
+    backup_codes = generate_backup_codes
+
+    # Store bcrypt digests only; this Postgres array cannot use AR encryption.
     update!(
       otp_required: true,
-      otp_backup_codes: generate_backup_codes
+      otp_backup_codes: backup_codes.map { |code| digest_backup_code(code) }
     )
+
+    backup_codes
   end
 
   def disable_mfa!
@@ -240,8 +246,13 @@ class User < ApplicationRecord
 
   def verify_otp?(code)
     return false if otp_secret.blank?
-    return true if verify_backup_code?(code)
-    totp.verify(code, drift_behind: 15)
+
+    normalized_code = normalize_mfa_code(code)
+    return false if normalized_code.blank?
+    return true if totp.verify(normalized_code, drift_behind: 15)
+    return false unless backup_code_input?(normalized_code)
+
+    consume_backup_code!(normalized_code)
   end
 
   def provisioning_uri
@@ -464,21 +475,72 @@ class User < ApplicationRecord
       ROTP::TOTP.new(otp_secret, issuer: "Sure Finances")
     end
 
-    def verify_backup_code?(code)
-      return false if otp_backup_codes.blank?
+    def consume_backup_code!(normalized_code)
+      consumed = false
 
-      # Find and remove the used backup code
-      if (index = otp_backup_codes.index(code))
-        remaining_codes = otp_backup_codes.dup
-        remaining_codes.delete_at(index)
-        update!(otp_backup_codes: remaining_codes)
-        true
-      else
-        false
+      transaction do
+        lock!
+
+        if otp_backup_codes.present?
+          matching_index = otp_backup_codes.index do |stored_code|
+            backup_code_matches?(stored_code, normalized_code)
+          end
+
+          if matching_index
+            remaining_codes = otp_backup_codes.dup
+            remaining_codes.delete_at(matching_index)
+            update!(otp_backup_codes: remaining_codes)
+            consumed = true
+          end
+        end
       end
+
+      consumed
     end
 
     def generate_backup_codes
-      8.times.map { SecureRandom.hex(4) }
+      MFA_BACKUP_CODE_COUNT.times.map { SecureRandom.hex(8) }
+    end
+
+    def digest_backup_code(code)
+      BCrypt::Password.create(normalize_mfa_code(code), cost: backup_code_digest_cost).to_s
+    end
+
+    def backup_code_matches?(stored_code, normalized_code)
+      if backup_code_digest?(stored_code)
+        return false unless backup_code_input?(normalized_code)
+
+        BCrypt::Password.new(stored_code).is_password?(normalized_code)
+      else
+        # Legacy plaintext codes are accepted once so existing MFA users are
+        # not locked out after backup-code hashing ships.
+        ActiveSupport::SecurityUtils.secure_compare(stored_code.to_s, normalized_code)
+      end
+    rescue BCrypt::Errors::InvalidHash
+      false
+    end
+
+    def backup_code_digest?(stored_code)
+      stored_code.to_s.start_with?("$2a$", "$2b$", "$2y$")
+    end
+
+    def normalize_mfa_code(code)
+      code.to_s.strip.downcase
+    end
+
+    def backup_code_input?(code)
+      backup_code_candidate?(code) || legacy_plaintext_backup_code_candidate?(code)
+    end
+
+    def backup_code_candidate?(code)
+      code.to_s.match?(/\A[0-9a-f]{16}\z/)
+    end
+
+    def legacy_plaintext_backup_code_candidate?(code)
+      code.to_s.match?(/\A[0-9a-f]{8}\z/)
+    end
+
+    def backup_code_digest_cost
+      ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost
     end
 end

app/views/mfa/verify.html.erb

@@ -31,7 +31,7 @@
     required: true,
     autofocus: true,
     autocomplete: "one-time-code",
-    type: "number",
+    type: "text",
     label: t(".page_title") %>
 
   <%= form.submit t(".verify_button") %>

db/migrate/20260503180000_rehash_plaintext_mfa_backup_codes.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+class RehashPlaintextMfaBackupCodes < ActiveRecord::Migration[7.2]
+  class MigrationUser < ActiveRecord::Base
+    self.table_name = "users"
+  end
+
+  BCRYPT_PREFIXES = %w[$2a$ $2b$ $2y$].freeze
+  PLAINTEXT_BACKUP_CODE_PATTERN = /\A[0-9a-f]{8}\z/
+
+  def up
+    require "bcrypt"
+
+    say_with_time "Rehashing plaintext MFA backup codes" do
+      rehashed_users_count = 0
+
+      MigrationUser.where(otp_required: true).find_each do |user|
+        backup_codes = Array(user.otp_backup_codes)
+        next if backup_codes.blank?
+        next unless backup_codes.any? { |code| plaintext_backup_code?(code) }
+
+        rehashed_codes = backup_codes.map do |code|
+          plaintext_backup_code?(code) ? BCrypt::Password.create(normalize_backup_code(code), cost: bcrypt_cost).to_s : code
+        end
+
+        user.update_columns(otp_backup_codes: rehashed_codes, updated_at: Time.current)
+        rehashed_users_count += 1
+      end
+
+      rehashed_users_count
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+
+  private
+    def plaintext_backup_code?(code)
+      normalized_code = normalize_backup_code(code)
+      normalized_code.match?(PLAINTEXT_BACKUP_CODE_PATTERN) && BCRYPT_PREFIXES.none? { |prefix| normalized_code.start_with?(prefix) }
+    end
+
+    def normalize_backup_code(code)
+      code.to_s.strip.downcase
+    end
+
+    def bcrypt_cost
+      ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost
+    end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2026_05_02_120000) do
+ActiveRecord::Schema[7.2].define(version: 2026_05_03_180000) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"

test/controllers/mfa_controller_test.rb

@@ -39,7 +39,12 @@ class MfaControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert @user.reload.otp_required?
     assert_equal 8, @user.otp_backup_codes.length
+    assert @user.otp_backup_codes.all? { |code| code.start_with?("$2") }
     assert_select "div.grid-cols-2" # Check for backup codes grid
+    rendered_codes = css_select("div.grid-cols-2 div").map { |node| node.text.strip }
+    assert_equal 8, rendered_codes.length
+    assert rendered_codes.all? { |code| code.match?(/\A[0-9a-f]{16}\z/) }
+    assert_empty rendered_codes & @user.otp_backup_codes
   end
 
   test "does not enable MFA with invalid code" do
@@ -96,17 +101,19 @@ class MfaControllerTest < ActionDispatch::IntegrationTest
 
   test "verify_code authenticates with valid backup code" do
     @user.setup_mfa!
-    @user.enable_mfa!
+    backup_code = @user.enable_mfa!.first
+    matching_digest = @user.otp_backup_codes.find { |digest| BCrypt::Password.new(digest).is_password?(backup_code) }
+    assert_not_nil matching_digest
     sign_out
 
     post sessions_path, params: { email: @user.email, password: user_password_test }
-    backup_code = @user.otp_backup_codes.first
 
     post verify_mfa_path, params: { code: backup_code }
 
     assert_redirected_to root_path
     assert Session.exists?(user_id: @user.id)
-    assert_not @user.reload.otp_backup_codes.include?(backup_code)
+    assert_equal 7, @user.reload.otp_backup_codes.size
+    assert_not_includes @user.otp_backup_codes, matching_digest
   end
 
   test "verify_code rejects invalid codes" do

test/models/user_test.rb

@@ -91,11 +91,24 @@ class UserTest < ActiveSupport::TestCase
   test "enable_mfa! enables MFA and generates backup codes" do
     user = users(:family_member)
     user.setup_mfa!
-    user.enable_mfa!
+    backup_codes = user.enable_mfa!
 
     assert user.otp_required?
+    assert_equal 8, backup_codes.length
+    assert backup_codes.all? { |code| code.match?(/\A[0-9a-f]{16}\z/) }
     assert_equal 8, user.otp_backup_codes.length
-    assert user.otp_backup_codes.all? { |code| code.length == 8 }
+    assert user.otp_backup_codes.all? { |code| code.start_with?("$2") }
+    assert_empty backup_codes & user.otp_backup_codes
+  end
+
+  test "enable_mfa! requires an OTP secret" do
+    user = users(:family_member)
+    user.setup_mfa!
+    user.update_column(:otp_secret, nil)
+
+    assert_raises(ArgumentError) { user.enable_mfa! }
+    assert_not user.reload.otp_required?
+    assert_empty user.otp_backup_codes
   end
 
   test "disable_mfa! removes all MFA data" do
@@ -155,22 +168,91 @@ class UserTest < ActiveSupport::TestCase
     assert_not user.verify_otp?("123456")
   end
 
-  test "verify_otp? accepts backup codes" do
+  test "verify_otp? does not check backup code digests for normal TOTP input" do
+    user = users(:family_member)
+    user.setup_mfa!
+    user.enable_mfa!
+    valid_code = ROTP::TOTP.new(user.otp_secret, issuer: "Sure Finances").now
+
+    BCrypt::Password.expects(:new).never
+
+    assert user.verify_otp?(valid_code)
+  end
+
+  test "verify_otp? fast rejects non-backup-code input before digest checks" do
     user = users(:family_member)
     user.setup_mfa!
     user.enable_mfa!
 
-    backup_code = user.otp_backup_codes.first
+    BCrypt::Password.expects(:new).never
+
+    assert_not user.verify_otp?("not-a-backup-code")
+  end
+
+  test "verify_otp? rejects unmatched legacy-shaped backup input" do
+    user = users(:family_member)
+    user.setup_mfa!
+    user.enable_mfa!
+
+    assert_not user.verify_otp?("deadbeef")
+  end
+
+  test "verify_otp? accepts backup codes" do
+    user = users(:family_member)
+    user.setup_mfa!
+    backup_codes = user.enable_mfa!
+
+    backup_code = backup_codes.first
+    matching_digest = user.otp_backup_codes.find { |digest| BCrypt::Password.new(digest).is_password?(backup_code) }
+    assert_not_nil matching_digest
+
     assert user.verify_otp?(backup_code)
 
     # Backup code should be consumed
-    assert_not user.otp_backup_codes.include?(backup_code)
     assert_equal 7, user.otp_backup_codes.length
+    assert_not_includes user.otp_backup_codes, matching_digest
 
     # Used backup code should not work again
     assert_not user.verify_otp?(backup_code)
   end
 
+  test "verify_otp? reloads backup codes while consuming under lock" do
+    user = users(:family_member)
+    user.setup_mfa!
+    backup_code = user.enable_mfa!.first
+    stale_user = User.find(user.id)
+
+    user.update!(otp_backup_codes: [])
+
+    assert_not stale_user.verify_otp?(backup_code)
+    assert_empty stale_user.reload.otp_backup_codes
+  end
+
+  test "verify_otp? accepts and consumes legacy plaintext backup codes once" do
+    user = users(:family_member)
+    user.setup_mfa!
+    user.update!(otp_required: true, otp_backup_codes: [ "deadbeef" ])
+
+    assert user.verify_otp?("deadbeef")
+
+    assert_empty user.reload.otp_backup_codes
+    assert_not user.verify_otp?("deadbeef")
+  end
+
+  test "verify_otp? accepts and consumes migrated legacy backup code digests" do
+    user = users(:family_member)
+    user.setup_mfa!
+    user.update!(
+      otp_required: true,
+      otp_backup_codes: [ BCrypt::Password.create("deadbeef", cost: BCrypt::Engine::MIN_COST).to_s ]
+    )
+
+    assert user.verify_otp?("deadbeef")
+
+    assert_empty user.reload.otp_backup_codes
+    assert_not user.verify_otp?("deadbeef")
+  end
+
   test "provisioning_uri generates correct URI" do
     user = users(:family_member)
     user.setup_mfa!