Skip to content

Conversation

@gitraiwaqas
Copy link

Fix: OTP Leading Zeros Bug Causing Phone Verification Failures

Fixes #40797

Problem

The GenerateOtp() function in internal/crypto/crypto.go loses leading zeros when converting big.Int to string. This causes:

  • SMS receives: "123" (3 digits)
  • Token hash stored from: "000123" (6 digits)
  • Result: Hash mismatch → 403: Token has expired or is invalid

Root Cause

big.Int.String() strips leading zeros (e.g., 000123"123"), but the OTP should always be exactly digits characters long.

Solution

Added zero-padding to ensure the OTP string always matches the requested digit count:

   if len(s) < digits {
       s = strings.Repeat("0", digits-len(s)) + s
   }

Impact

  • ✅ Fixes phone change OTP verification (issue #40797)
  • ✅ Ensures SMS code matches token hash
  • ✅ Affects: auth.updateUser({ phone }) flow

Files Changed

  • internal/crypto/crypto.go (lines 22-29)

Testing

CI will validate this fix runs correctly with existing test suite.

@gitraiwaqas gitraiwaqas requested a review from a team as a code owner November 29, 2025 05:29
@cstockton
Copy link
Contributor

I could be wrong here, but I don't see any issue with the zero padding or uniformity of the current GenerateOtp code. The formatting is correctly zero padded and does not reduce entropy. The only thing that has just caught my eye is the math.Pow10 usage, for >18 digits it would need to be changed to something like:

upper := new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(digits)), nil)
val := must(rand.Int(r, upper))

But since we do not allow >10 digits today the current implementation is safe. If you discover any other code that appears to be invalid in the OTP verification process tag me and I'll be happy to review.

@cstockton cstockton added do not merge working-as-intended This feature is working as intended labels Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge working-as-intended This feature is working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

phone_change_token OTP code from SMS doesn't match stored token hash

2 participants