feat(otp): switch one_time_tokens table to source of truth - #2788
Conversation
2ed1e5b to
0665647
Compare
0665647 to
8bf17f6
Compare
7577774 to
fc7f405
Compare
8bf17f6 to
0820b2b
Compare
Remove the phone-provider branches from the one_time_tokens verify path so this PR covers only the row-based flow. In OTT mode a test OTP or Twilio Verify code now misses the hash lookup and is rejected. A follow-up PR stacked on this branch restores both.
31b4e74 to
6d15a97
Compare
| // NOTE: Test OTPs and Twilio Verify are not handled yet on this path; a follow-up PR will add them. | ||
| func verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { | ||
|
|
||
| // TODO AUTH-1553: Add support for test OTPs and Twilio Verify on this path. |
There was a problem hiding this comment.
Separating this out to make it easier to review, but they'll be merged together.
|
|
||
| // IsExpired treats nil ExpiresAt as expired. This is a security measure to avoid accidentally treating a token with no expiration as valid. | ||
| func (o OneTimeToken) IsExpired() bool { | ||
| return o.ExpiresAt == nil || time.Now().After(*o.ExpiresAt) |
There was a problem hiding this comment.
We have two options here:
- Treat a nil
expiresAtas expired. - Treat a nil
expiresAtas non-expired.
expiresAt will be nil for all OTTs created before expiresAt was written to.
We can reduce the likelihood by staggering the release of these changes + the write-path changes so the OTTs without expiries age out, but eventually we plan to make this path on by default. So, if a self-hosted customer doesn't upgrade until we've made it the default, it would cause all of their recently issued tokens to expire.
I went this direction because having to re-request a OTT seems like a minor inconvenience, but I wanted to point that out so we can discuss if there are any concerns.
| // columns. A lookup miss is rejected as an expired or invalid token. | ||
| // | ||
| // NOTE: Test OTPs and Twilio Verify are not handled yet on this path; a follow-up PR will add them. | ||
| func verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { |
There was a problem hiding this comment.
The summary is that we:
- Verify the OTT by looking it up and checking if it's expired
- Resolve the generic
type=emailrequest param type to the actual token type for post-verification steps - Find the user by the OTT's user ID
- Ensure that the user associated with the OTT is eligible to verify it, and validate identifier binding
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("unknown verification type") | ||
| } | ||
|
|
||
| ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, params.TokenHash, tokenTypes...) |
There was a problem hiding this comment.
🟠 Severity: HIGH
The new fallback accepts a plain OTP whose stored email-change hash is pkce_-prefixed, but emailChangeVerify cannot match or clear that prefixed row. Replaying the same attacker-supplied OTP then passes with status already single and reaches the email-change commit, bypassing the required second confirmation.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: After the successful OTT lookup at line 52, normalize params.TokenHash to match the actual stored token hash before returning. When FindOneTimeTokenWithPKCEFallback resolves a PKCE-prefixed row (e.g. stored as pkce_<hash> but looked up by plain <hash>), the returned ott.TokenHash differs from params.TokenHash. The downstream emailChangeVerify function in verify.go then calls FindOneTimeToken(tx, params.TokenHash, ...) with the original plain hash, which finds no row — so neither currentOTT nor newOTT is set, neither ClearOneTimeTokenForUser branch fires, and the PKCE-prefixed token row remains in the database. EmailChangeConfirmStatus is still promoted to singleConfirmation, so a replay of the same OTP bypasses the zeroConfirmation guard and commits the email change without a second confirmation.
Fix: In verifyOneTimeToken (verify_ott.go), insert the following normalization block immediately after the error checks for FindOneTimeTokenWithPKCEFallback (after line 57, before the ott.IsExpired() check):
// If the token was matched via its PKCE-prefixed hash, normalise params.TokenHash
// to the actual stored value so that downstream operations (e.g. emailChangeVerify)
// can find and clear the correct row from the one_time_tokens table.
if ott.TokenHash != params.TokenHash {
params.TokenHash = ott.TokenHash
}Because params is passed as a pointer, this update propagates to emailChangeVerify, which will then call FindOneTimeToken(tx, "pkce_<hash>", ...), find the correct row, enter the clearing branch, and delete the token — preventing the replay attack.
xlgmokha
left a comment
xlgmokha
left a comment
There was a problem hiding this comment.
Nicely done! Nothing stood out for me as blockers. I left one question but it's not a blocker.
I'm not as familiar with this part of the codebase so you might choose to wait for another review from someone else or not. I trust your discretion.
| return mismatch.WithInternalMessage("user email does not match") | ||
| } | ||
| default: // Signup, Invite, Recovery, MagicLink | ||
| if params.Email == "" || !strings.EqualFold(user.GetEmail(), params.Email) { |
There was a problem hiding this comment.
question: is user.GetEmail() a safe fallback?
2 participants
What kind of change does this PR introduce?
Feat
What is the current behavior?
We dual-write token data to the
one_time_tokensanduserstable, but read token data from them inconsistently.What is the new behavior?
When a flag
GOTRUE_EXPERIMENTAL_ENABLE_OTT_AS_SOURCE_OF_TRUTHis enabled, we treatone_time_tokensas the source of truth for token data, and only lookup the user to validate eligibility and confirm identifier binding.Additional context
I've intentionally split out the Twilio Verify flow + Test OTP handling into a separate PR, but will wait until both are merged to release.
Closes AUTH-1553.