Prevent TOTP code replay within the drift window
verify_totp called ROTP without `after:`, so a captured 6-digit code stayed valid for the full ~90s drift window and could be replayed in a separate sign-in. Add a last_otp_at column, pass it as ROTP's `after:`, and persist the matched timestep on success so a code (or any earlier one) cannot be reused. Also fixes a latent bug surfaced by the new replay path: enable_totp! did `self.backup_codes = generate_backup_codes`, reassigning backup_codes to the plaintext return value (generate_backup_codes already stores the BCrypt hashes internally). That stored backup codes in plaintext and broke verification. enable_totp! is test-only today, but it is public and backup_codes is not encrypted, so this is a real footgun. Now it just calls generate_backup_codes. Rewrites the mislabeled "TOTP code cannot be reused" test to actually assert that replaying an accepted code is rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -63,7 +63,10 @@ class User < ApplicationRecord
|
|||||||
def enable_totp!
|
def enable_totp!
|
||||||
require "rotp"
|
require "rotp"
|
||||||
self.totp_secret = ROTP::Base32.random
|
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!
|
save!
|
||||||
end
|
end
|
||||||
|
|
||||||
@@ -86,7 +89,13 @@ class User < ApplicationRecord
|
|||||||
|
|
||||||
require "rotp"
|
require "rotp"
|
||||||
totp = ROTP::TOTP.new(totp_secret)
|
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
|
end
|
||||||
|
|
||||||
# Console/debug helper: get current TOTP code
|
# Console/debug helper: get current TOTP code
|
||||||
|
|||||||
7
db/migrate/20260611000001_add_last_otp_at_to_users.rb
Normal file
7
db/migrate/20260611000001_add_last_otp_at_to_users.rb
Normal file
@@ -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
|
||||||
3
db/schema.rb
generated
3
db/schema.rb
generated
@@ -10,7 +10,7 @@
|
|||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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|
|
create_table "active_storage_attachments", force: :cascade do |t|
|
||||||
t.bigint "blob_id", null: false
|
t.bigint "blob_id", null: false
|
||||||
t.datetime "created_at", 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.datetime "created_at", null: false
|
||||||
t.json "custom_claims", default: {}, null: false
|
t.json "custom_claims", default: {}, null: false
|
||||||
t.string "email_address", null: false
|
t.string "email_address", null: false
|
||||||
|
t.integer "last_otp_at"
|
||||||
t.datetime "last_sign_in_at"
|
t.datetime "last_sign_in_at"
|
||||||
t.string "name"
|
t.string "name"
|
||||||
t.string "password_digest", null: false
|
t.string "password_digest", null: false
|
||||||
|
|||||||
@@ -19,16 +19,21 @@ class TotpSecurityTest < ActionDispatch::IntegrationTest
|
|||||||
|
|
||||||
# First use of the code should succeed
|
# First use of the code should succeed
|
||||||
post totp_verification_path, params: {code: valid_code}
|
post totp_verification_path, params: {code: valid_code}
|
||||||
assert_response :redirect
|
|
||||||
assert_redirected_to root_path
|
assert_redirected_to root_path
|
||||||
|
|
||||||
# Sign out
|
# Sign out
|
||||||
delete session_path
|
delete session_path
|
||||||
assert_response :redirect
|
assert_response :redirect
|
||||||
|
|
||||||
# Note: In the current implementation, TOTP codes CAN be reused within the 60-second time window
|
# Replay the SAME code in a fresh sign-in attempt. Because verify_totp records
|
||||||
# This is standard TOTP behavior. For enhanced security, you could implement used code tracking.
|
# the accepted timestep (ROTP `after:`), the code is now rejected even though
|
||||||
# This test documents the current behavior - codes work within their time window
|
# 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.sessions.delete_all
|
||||||
user.destroy
|
user.destroy
|
||||||
|
|||||||
Reference in New Issue
Block a user