Gem: active_model_otp 2.3.4
lib/active_model/one_time_password.rb:126 draws each backup code from its own random counter and never checks the batch for repeats:
backup_codes = Array.new(self.class.otp_backup_codes_count) do
otp.generate_otp((SecureRandom.random_number(9e5) + 1e5).to_i)
end
Two draws can hit the same counter (1 in 900,000), and two counters can produce the same 6-digit code (1 in 10^6). Over one batch that is about 1 in 7,000 at the default 12 codes, 1 in 32,000 at 6. Stub SecureRandom.random_number to return the same value twice and the duplicate falls straight out.
Why it matters
- The user is quietly one recovery code short, at the moment they are being locked into MFA.
- With
one_time_backup_codes: true, authenticate_backup_code calls backup_codes.delete(code), which removes every match — redeeming the duplicate burns both entries at once.
- Consumers that assume uniqueness break. Ours keys a Svelte
{#each} on the code, so a repeat aborted the whole page render: a blank page right after enabling MFA.
Fix: draw until the batch is distinct (backup_codes |= [...] in a while loop). The retry essentially never runs. A wider counter range would lower the collision rate but not remove it — the uniqueness check is what makes the guarantee.
Gem:
active_model_otp2.3.4lib/active_model/one_time_password.rb:126draws each backup code from its own random counter and never checks the batch for repeats:Two draws can hit the same counter (1 in 900,000), and two counters can produce the same 6-digit code (1 in 10^6). Over one batch that is about 1 in 7,000 at the default 12 codes, 1 in 32,000 at 6. Stub
SecureRandom.random_numberto return the same value twice and the duplicate falls straight out.Why it matters
one_time_backup_codes: true,authenticate_backup_codecallsbackup_codes.delete(code), which removes every match — redeeming the duplicate burns both entries at once.{#each}on the code, so a repeat aborted the whole page render: a blank page right after enabling MFA.Fix: draw until the batch is distinct (
backup_codes |= [...]in awhileloop). The retry essentially never runs. A wider counter range would lower the collision rate but not remove it — the uniqueness check is what makes the guarantee.