diff --git a/app/models/user.rb b/app/models/user.rb index 5f07d71..0fe24ee 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -63,7 +63,10 @@ class User < ApplicationRecord def enable_totp! require "rotp" self.totp_secret = ROTP::Base32.random - self.backup_codes = generate_backup_codes + # generate_backup_codes assigns the BCrypt hashes to self.backup_codes and + # returns the plaintext codes for display. Do NOT reassign backup_codes to the + # return value — that would store the plaintext codes and break verification. + generate_backup_codes save! end @@ -86,7 +89,13 @@ class User < ApplicationRecord require "rotp" totp = ROTP::TOTP.new(totp_secret) - totp.verify(code, drift_behind: 30, drift_ahead: 30) + # Pass `after:` so a code can only be accepted once: ROTP rejects any timestep + # at or before the last accepted one, closing the ~90s drift-window replay. + verified_at = totp.verify(code, drift_behind: 30, drift_ahead: 30, after: last_otp_at) + return false unless verified_at + + update_column(:last_otp_at, verified_at) + true end # Console/debug helper: get current TOTP code diff --git a/db/migrate/20260611000001_add_last_otp_at_to_users.rb b/db/migrate/20260611000001_add_last_otp_at_to_users.rb new file mode 100644 index 0000000..c7856f7 --- /dev/null +++ b/db/migrate/20260611000001_add_last_otp_at_to_users.rb @@ -0,0 +1,7 @@ +class AddLastOtpAtToUsers < ActiveRecord::Migration[8.1] + def change + # Unix timestamp of the most recently accepted TOTP timestep, used to reject + # replay of a code within its drift window (passed to ROTP's `after:`). + add_column :users, :last_otp_at, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 12ae5a0..129980c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_06_07_000003) do +ActiveRecord::Schema[8.1].define(version: 2026_06_11_000001) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -233,6 +233,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_06_07_000003) do t.datetime "created_at", null: false t.json "custom_claims", default: {}, null: false t.string "email_address", null: false + t.integer "last_otp_at" t.datetime "last_sign_in_at" t.string "name" t.string "password_digest", null: false diff --git a/test/controllers/totp_security_test.rb b/test/controllers/totp_security_test.rb index 9d1fa74..94b25db 100644 --- a/test/controllers/totp_security_test.rb +++ b/test/controllers/totp_security_test.rb @@ -19,16 +19,21 @@ class TotpSecurityTest < ActionDispatch::IntegrationTest # First use of the code should succeed post totp_verification_path, params: {code: valid_code} - assert_response :redirect assert_redirected_to root_path # Sign out delete session_path assert_response :redirect - # Note: In the current implementation, TOTP codes CAN be reused within the 60-second time window - # This is standard TOTP behavior. For enhanced security, you could implement used code tracking. - # This test documents the current behavior - codes work within their time window + # Replay the SAME code in a fresh sign-in attempt. Because verify_totp records + # the accepted timestep (ROTP `after:`), the code is now rejected even though + # it is still within its drift window — so we stay on the verification step. + post signin_path, params: {email_address: "totp_replay_test@example.com", password: "password123"} + assert_redirected_to totp_verification_path + + post totp_verification_path, params: {code: valid_code} + assert_redirected_to totp_verification_path + assert_equal "Invalid verification code. Please try again.", flash[:alert] user.sessions.delete_all user.destroy