From 0121d3d3014a58de9032bc2c75b70cd50a33acd7 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 3 Sep 2026 15:26:49 -0400 Subject: [PATCH 01/14] feat(otp): add config feature flag to EnableOTTAsSourceOfTruth --- example.env | 4 ++++ internal/conf/configuration.go | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/example.env b/example.env index 39c207ea34..58745a06fe 100644 --- a/example.env +++ b/example.env @@ -286,3 +286,7 @@ GOTRUE_MFA_RECOVERY_CODES_LOCKOUT_DURATION="15m" # between 1m and 24h # account that didn't have one (e.g. a user who signed up with an external # provider and later sets a password). GOTRUE_EXPERIMENTAL_CREATE_EMAIL_IDENTITY_ON_PASSWORD_SET_ENABLED="false" + +# Reads one-time tokens from the one_time_tokens table instead of the users +# table when verifying typed OTPs. A miss is rejected as an expired or invalid token. +GOTRUE_EXPERIMENTAL_ENABLE_OTT_AS_SOURCE_OF_TRUTH="false" diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go index 620bc11a46..b43d8710a5 100644 --- a/internal/conf/configuration.go +++ b/internal/conf/configuration.go @@ -412,6 +412,13 @@ type ExperimentalConfiguration struct { // one (e.g. a user who signed up with an external provider and later sets a password). // Env: GOTRUE_EXPERIMENTAL_CREATE_EMAIL_IDENTITY_ON_PASSWORD_SET_ENABLED=true CreateEmailIdentityOnPasswordSetEnabled bool `split_words:"true" default:"false"` + + // EnableOTTAsSourceOfTruth makes the typed-OTP verification path read the + // challenge from the one_time_tokens table instead of the users.*_token + // columns. A lookup miss is rejected as an expired or invalid token; there is + // no fallback to the users columns. + // Env: GOTRUE_EXPERIMENTAL_ENABLE_OTT_AS_SOURCE_OF_TRUTH=true + EnableOTTAsSourceOfTruth bool `split_words:"true" default:"false"` } // ReloadingConfiguration holds the configuration values for runtime From 301ec4118f5ef76f2b04786404332aaaf2993e8b Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 3 Sep 2026 21:42:08 -0400 Subject: [PATCH 02/14] feat(otp): use one_time_tokens table as source of truth for verifyUserAndToken --- internal/api/verify.go | 160 +++++++++++++++++++++++++++++++---------- 1 file changed, 124 insertions(+), 36 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index 728ce3ff9d..6b1a983a50 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -730,57 +730,145 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, var isValid bool - smsProvider, _ := sms_provider.GetSmsProvider(*config) - switch params.Type { - case mail.EmailOTPVerification: - // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns - if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { - isValid = true - params.Type = mail.SignupVerification - } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { - isValid = true - params.Type = mail.MagicLinkVerification - } else { - isValid = false - } - case mail.SignupVerification, mail.InviteVerification: - isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) - case mail.RecoveryVerification, mail.MagicLinkVerification: - isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) - case mail.EmailChangeVerification: - isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || - isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) - case phoneChangeVerification, smsVerification: - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { - if params.Token == testOTP { + if config.Experimental.EnableOTTAsSourceOfTruth { + return a.verifyOneTimeToken(conn, user, params) + } else { + smsProvider, _ := sms_provider.GetSmsProvider(*config) + switch params.Type { + case mail.EmailOTPVerification: + // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns + if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { + isValid = true + params.Type = mail.SignupVerification + } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { + isValid = true + params.Type = mail.MagicLinkVerification + } else { + isValid = false + } + case mail.SignupVerification, mail.InviteVerification: + isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) + case mail.RecoveryVerification, mail.MagicLinkVerification: + isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) + case mail.EmailChangeVerification: + isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || + isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) + case phoneChangeVerification, smsVerification: + // Check if test OP, if so skip validation and return user + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { + if params.Token == testOTP { + return user, nil + } + } + + phone := params.Phone + sentAt := user.ConfirmationSentAt + expectedToken := user.ConfirmationToken + if params.Type == phoneChangeVerification { + phone = user.PhoneChange + sentAt = user.PhoneChangeSentAt + expectedToken = user.PhoneChangeToken + } + + if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { + if err := smsProvider.(*sms_provider.TwilioVerifyProvider).VerifyOTP(phone, params.Token); err != nil { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } return user, nil } + isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) } - - phone := params.Phone - sentAt := user.ConfirmationSentAt - expectedToken := user.ConfirmationToken - if params.Type == phoneChangeVerification { - phone = user.PhoneChange - sentAt = user.PhoneChangeSentAt - expectedToken = user.PhoneChangeToken + if !isValid { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") } + } + return user, nil +} +func (a *API) verifyOneTimeToken(conn *storage.Connection, user *models.User, params *VerifyParams) (*models.User, error) { + config := a.config + + if params.Type == smsVerification || params.Type == phoneChangeVerification { + // Test OTPs and Twilio Verify don't have a local challenge to compare against, so we skip the local validation + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { + return user, nil + } if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - if err := smsProvider.(*sms_provider.TwilioVerifyProvider).VerifyOTP(phone, params.Token); err != nil { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + if err := a.verifyOTPWithTwilio(user, params); err != nil { + return nil, err } return user, nil } - isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) } - if !isValid { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") + tokenTypes := verifyTypeToTokenTypes(params.Type) + if len(tokenTypes) == 0 { + return nil, apierrors.NewBadRequestError(apierrors.ErrorCodeValidationFailed, "Invalid verification type") + } + + ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, user.ID, params.TokenHash, tokenTypes...) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token not found") + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) + } + + if ott.IsExpired() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token has expired") } + + // The generic email type needs to match to the flow that issues the token, so the caller runs the right post-verify step + if params.Type == mail.EmailOTPVerification { + switch ott.TokenType { + case models.ConfirmationToken: + params.Type = mail.SignupVerification + case models.RecoveryToken: + params.Type = mail.MagicLinkVerification + } + } + return user, nil } +// check config.Sms.IsTwilioVerifyProvider() before calling this function +func (a *API) verifyOTPWithTwilio(user *models.User, params *VerifyParams) error { + smsProvider, err := sms_provider.GetSmsProvider(*a.config) + if err != nil { + return apierrors.NewInternalServerError("Failed to get SMS provider").WithInternalError(err) + } + phone := params.Phone + if params.Type == phoneChangeVerification { + phone = user.PhoneChange + } + twilioVerify, ok := smsProvider.(*sms_provider.TwilioVerifyProvider) + if !ok { + return apierrors.NewInternalServerError("SMS provider is not Twilio Verify") + } + if err := twilioVerify.VerifyOTP(phone, params.Token); err != nil { + return apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return nil +} + +func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { + switch verifyType { + case mail.EmailOTPVerification: + return []models.OneTimeTokenType{models.ConfirmationToken, models.RecoveryToken} + case mail.SignupVerification, mail.InviteVerification: + return []models.OneTimeTokenType{models.ConfirmationToken} + case mail.RecoveryVerification, mail.MagicLinkVerification: + return []models.OneTimeTokenType{models.RecoveryToken} + case mail.EmailChangeVerification: + return []models.OneTimeTokenType{models.EmailChangeTokenCurrent, models.EmailChangeTokenNew} + case phoneChangeVerification: + return []models.OneTimeTokenType{models.PhoneChangeToken} + case smsVerification: + return []models.OneTimeTokenType{models.ConfirmationToken} + default: + return nil + } +} + // isOtpValid checks the actual otp sent against the expected otp and ensures that it's within the valid window func isOtpValid(actual, expected string, sentAt *time.Time, otpExp uint) bool { if expected == "" || sentAt == nil { From f42bd0616d71747db4954b23c020e0f9c4e9983f Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 3 Sep 2026 22:04:28 -0400 Subject: [PATCH 03/14] feat(otp): clean up and add tess --- internal/api/verify.go | 20 +- internal/api/verify_ott_parity_test.go | 501 +++++++++++++++++++++++++ 2 files changed, 510 insertions(+), 11 deletions(-) create mode 100644 internal/api/verify_ott_parity_test.go diff --git a/internal/api/verify.go b/internal/api/verify.go index 6b1a983a50..3b4ce803b0 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -787,14 +787,15 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, func (a *API) verifyOneTimeToken(conn *storage.Connection, user *models.User, params *VerifyParams) (*models.User, error) { config := a.config - + // Twilio Verify and test OTPs are verified without a local challenge if params.Type == smsVerification || params.Type == phoneChangeVerification { - // Test OTPs and Twilio Verify don't have a local challenge to compare against, so we skip the local validation if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { return user, nil } if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - if err := a.verifyOTPWithTwilio(user, params); err != nil { + // For a phone change, params.Phone is the persisted phone_change + // number, because that is how the user was found. + if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { return nil, err } return user, nil @@ -803,7 +804,7 @@ func (a *API) verifyOneTimeToken(conn *storage.Connection, user *models.User, pa tokenTypes := verifyTypeToTokenTypes(params.Type) if len(tokenTypes) == 0 { - return nil, apierrors.NewBadRequestError(apierrors.ErrorCodeValidationFailed, "Invalid verification type") + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("unknown verification type") } ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, user.ID, params.TokenHash, tokenTypes...) @@ -830,21 +831,18 @@ func (a *API) verifyOneTimeToken(conn *storage.Connection, user *models.User, pa return user, nil } -// check config.Sms.IsTwilioVerifyProvider() before calling this function -func (a *API) verifyOTPWithTwilio(user *models.User, params *VerifyParams) error { +// verifyOTPWithTwilio asks Twilio Verify to check the code. Twilio generates +// and delivers its own code, so there is no local challenge to compare. +func (a *API) verifyOTPWithTwilio(phone, code string) error { smsProvider, err := sms_provider.GetSmsProvider(*a.config) if err != nil { return apierrors.NewInternalServerError("Failed to get SMS provider").WithInternalError(err) } - phone := params.Phone - if params.Type == phoneChangeVerification { - phone = user.PhoneChange - } twilioVerify, ok := smsProvider.(*sms_provider.TwilioVerifyProvider) if !ok { return apierrors.NewInternalServerError("SMS provider is not Twilio Verify") } - if err := twilioVerify.VerifyOTP(phone, params.Token); err != nil { + if err := twilioVerify.VerifyOTP(phone, code); err != nil { return apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) } return nil diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go new file mode 100644 index 0000000000..87ae37e907 --- /dev/null +++ b/internal/api/verify_ott_parity_test.go @@ -0,0 +1,501 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "time" + + "github.com/gofrs/uuid" + "github.com/stretchr/testify/require" + "gopkg.in/h2non/gock.v1" + + "github.com/supabase/auth/internal/api/apierrors" + "github.com/supabase/auth/internal/api/sms_provider" + "github.com/supabase/auth/internal/conf" + "github.com/supabase/auth/internal/crypto" + mail "github.com/supabase/auth/internal/mailer" + "github.com/supabase/auth/internal/models" +) + +// The typed-OTP verify path can read the challenge from either the legacy +// users.*_token columns or, with EnableOTTAsSourceOfTruth, from the +// one_time_tokens table. These tests run every flow once per store and require +// the observable outcome to be identical. Both stores are seeded the way the +// send paths seed them, so a failure here is a divergence in verify logic, not +// in the fixtures. + +const ( + parityOTP = "123456" + parityEmail = "test@example.com" + parityPhone = "12345678" + parityNewEmail = "new@example.com" + parityNewPhone = "1234567890" + parityForbidden = "Token has expired or is invalid" + twilioServiceSid = "VA-parity-test" +) + +// otpParityOutcome is everything a client or an operator can observe after a +// POST /verify: the HTTP result, the user state it left, and the audit action +// it recorded. +type otpParityOutcome struct { + Status int + ErrorCode string + Msg string + Action string + EmailConfirmed bool + PhoneConfirmed bool + Email string + Phone string +} + +type otpParityCase struct { + desc string + // seed writes the challenge to both stores and returns the request body. + // It receives a freshly created, unconfirmed user. + seed func(u *models.User) map[string]interface{} + // configure applies per-case config and returns a function that undoes + // it. It runs once per store, so consumable mocks are re-armed each time. + configure func() func() + expected otpParityOutcome +} + +func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { + now := time.Now() + expired := now.Add(-48 * time.Hour) + emailHash := crypto.GenerateTokenHash(parityEmail, parityOTP) + + baseline := otpParityOutcome{Email: parityEmail, Phone: parityPhone} + forbidden := baseline + forbidden.Status = http.StatusForbidden + forbidden.ErrorCode = apierrors.ErrorCodeOTPExpired + forbidden.Msg = parityForbidden + + signedUp := baseline + signedUp.Status = http.StatusOK + signedUp.Action = string(models.UserSignedUpAction) + signedUp.EmailConfirmed = true + + loggedIn := baseline + loggedIn.Status = http.StatusOK + loggedIn.Action = string(models.LoginAction) + loggedIn.EmailConfirmed = true + + cases := []otpParityCase{ + { + desc: "signup with a valid code confirms the user", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: signedUp, + }, + { + desc: "signup with an expired code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, emailHash, expired, -time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: forbidden, + }, + { + desc: "signup with the wrong code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, crypto.GenerateTokenHash(parityEmail, "999999"), now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: forbidden, + }, + { + desc: "invite with a valid code confirms the user", + seed: func(u *models.User) map[string]interface{} { + u.InvitedAt = &now + ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + return emailOTPBody(mail.InviteVerification, parityEmail) + }, + expected: signedUp, + }, + { + desc: "magiclink with a valid code signs a confirmed user in", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, emailHash, now, time.Hour) + return emailOTPBody(mail.MagicLinkVerification, parityEmail) + }, + expected: loggedIn, + }, + { + desc: "recovery with a valid code signs a confirmed user in", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, emailHash, now, time.Hour) + return emailOTPBody(mail.RecoveryVerification, parityEmail) + }, + expected: loggedIn, + }, + { + // Tokens issued through the PKCE flow are stored with a pkce_ + // prefix. A plain code must still match them. + desc: "magiclink with a pkce_ prefixed stored hash accepts the plain code", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, PKCEPrefix+emailHash, now, time.Hour) + return emailOTPBody(mail.MagicLinkVerification, parityEmail) + }, + expected: loggedIn, + }, + { + // The generic "email" type must resolve to the signup flow when the + // stored challenge is a confirmation token. + desc: "email type with a confirmation token runs the signup flow", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + return emailOTPBody(mail.EmailOTPVerification, parityEmail) + }, + expected: signedUp, + }, + { + // The generic "email" type must resolve to the magiclink flow when + // the stored challenge is a recovery token. + desc: "email type with a recovery token runs the magiclink flow", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, emailHash, now, time.Hour) + return emailOTPBody(mail.EmailOTPVerification, parityEmail) + }, + expected: loggedIn, + }, + { + desc: "email type with no matching challenge is rejected", + seed: func(u *models.User) map[string]interface{} { + return emailOTPBody(mail.EmailOTPVerification, parityEmail) + }, + expected: forbidden, + }, + { + desc: "email change with a valid code moves the user to the new address", + configure: func() func() { + previous := ts.Config.Mailer.SecureEmailChangeEnabled + ts.Config.Mailer.SecureEmailChangeEnabled = false + return func() { ts.Config.Mailer.SecureEmailChangeEnabled = previous } + }, + seed: func(u *models.User) map[string]interface{} { + u.EmailChange = parityNewEmail + ts.seedChallenge(u, models.EmailChangeTokenNew, crypto.GenerateTokenHash(parityNewEmail, parityOTP), now, time.Hour) + return emailOTPBody(mail.EmailChangeVerification, parityNewEmail) + }, + expected: otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserModifiedAction), + EmailConfirmed: true, + Email: parityNewEmail, + Phone: parityPhone, + }, + }, + { + desc: "a banned user is rejected before the challenge is checked", + seed: func(u *models.User) map[string]interface{} { + bannedUntil := now.Add(time.Hour) + u.BannedUntil = &bannedUntil + ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: otpParityOutcome{ + Status: http.StatusForbidden, + ErrorCode: apierrors.ErrorCodeUserBanned, + Msg: "User is banned", + Email: parityEmail, + Phone: parityPhone, + }, + }, + { + desc: "an unknown verification type is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + return emailOTPBody("bogus", parityEmail) + }, + expected: forbidden, + }, + } + + ts.runOTPParityCases(cases) +} + +func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { + now := time.Now() + expired := now.Add(-48 * time.Hour) + phoneHash := crypto.GenerateTokenHash(parityPhone, parityOTP) + + baseline := otpParityOutcome{Email: parityEmail, Phone: parityPhone} + forbidden := baseline + forbidden.Status = http.StatusForbidden + forbidden.ErrorCode = apierrors.ErrorCodeOTPExpired + forbidden.Msg = parityForbidden + + phoneSignedUp := baseline + phoneSignedUp.Status = http.StatusOK + phoneSignedUp.Action = string(models.UserSignedUpAction) + phoneSignedUp.PhoneConfirmed = true + + phoneChanged := otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserModifiedAction), + PhoneConfirmed: true, + Email: parityEmail, + Phone: parityNewPhone, + } + + cases := []otpParityCase{ + { + desc: "sms with a valid code confirms the phone", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, phoneHash, now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: phoneSignedUp, + }, + { + desc: "sms with an expired code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, phoneHash, expired, -time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + { + desc: "sms with the wrong code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + { + desc: "phone change with a valid code moves the user to the new number", + seed: func(u *models.User) map[string]interface{} { + u.PhoneChange = parityNewPhone + ts.seedChallenge(u, models.PhoneChangeToken, crypto.GenerateTokenHash(parityNewPhone, parityOTP), now, time.Hour) + return phoneOTPBody(phoneChangeVerification, parityNewPhone) + }, + expected: phoneChanged, + }, + { + // A test OTP is accepted without any stored challenge. This is the + // path app store reviewers and CI rely on. + desc: "sms with a test OTP succeeds with no stored challenge", + configure: func() func() { + return ts.configureTestOTP(parityPhone, parityOTP) + }, + seed: func(u *models.User) map[string]interface{} { + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: phoneSignedUp, + }, + { + desc: "sms with a wrong code falls through the test OTP check and is rejected", + configure: func() func() { + return ts.configureTestOTP(parityPhone, "000000") + }, + seed: func(u *models.User) map[string]interface{} { + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + { + // Twilio Verify generates and delivers its own code, so the locally + // stored hash never matches what the user types. Twilio's answer + // is the only thing that counts. + desc: "sms with Twilio Verify accepts a code Twilio approves", + configure: func() func() { + return ts.configureTwilioVerify(map[string]interface{}{"status": "approved", "valid": true}) + }, + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: phoneSignedUp, + }, + { + desc: "sms with Twilio Verify rejects a code Twilio does not approve", + configure: func() func() { + return ts.configureTwilioVerify(map[string]interface{}{"status": "pending", "valid": false}) + }, + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, phoneHash, now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + } + + ts.runOTPParityCases(cases) +} + +// runOTPParityCases runs each case against both stores and asserts that both +// produce the expected outcome and agree with each other. +func (ts *VerifyTestSuite) runOTPParityCases(cases []otpParityCase) { + originalFlag := ts.Config.Experimental.EnableOTTAsSourceOfTruth + defer func() { ts.Config.Experimental.EnableOTTAsSourceOfTruth = originalFlag }() + + modes := []struct { + name string + flag bool + }{ + {name: "legacy users columns", flag: false}, + {name: "one_time_tokens", flag: true}, + } + + for _, caseItem := range cases { + c := caseItem + ts.Run(c.desc, func() { + outcomes := make(map[string]otpParityOutcome, len(modes)) + + for _, mode := range modes { + m := mode + ts.Run(m.name, func() { + ts.SetupTest() + ts.Config.Experimental.EnableOTTAsSourceOfTruth = m.flag + if c.configure != nil { + restore := c.configure() + defer restore() + } + + u, err := models.FindUserByEmailAndAudience(ts.API.db, parityEmail, ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + body := c.seed(u) + + since := time.Now() + w := ts.postVerify(body) + outcome := ts.observeOutcome(w, u.ID, since) + require.Equal(ts.T(), c.expected, outcome) + outcomes[m.name] = outcome + }) + } + + require.Equal(ts.T(), outcomes[modes[0].name], outcomes[modes[1].name], + "legacy and one_time_tokens paths must produce the same outcome") + }) + } +} + +// seedChallenge stores hash in the users column and the one_time_tokens row +// for tokenType, mirroring what the send paths write. Any other pending +// change on u is persisted at the same time. +func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, hash string, sentAt time.Time, validity time.Duration) { + switch tokenType { + case models.ConfirmationToken: + u.ConfirmationToken = hash + u.ConfirmationSentAt = &sentAt + case models.RecoveryToken: + u.RecoveryToken = hash + u.RecoverySentAt = &sentAt + case models.EmailChangeTokenNew: + u.EmailChangeTokenNew = hash + u.EmailChangeSentAt = &sentAt + case models.PhoneChangeToken: + u.PhoneChangeToken = hash + u.PhoneChangeSentAt = &sentAt + default: + ts.T().Fatalf("seedChallenge does not support token type %s", tokenType) + } + + require.NoError(ts.T(), ts.API.db.Update(u)) + require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", hash, tokenType, validity)) +} + +func (ts *VerifyTestSuite) postVerify(body map[string]interface{}) *httptest.ResponseRecorder { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(body)) + + req := httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + return w +} + +// observeOutcome collects the response and the resulting user state. Only +// audit entries written after since are considered, so earlier entries in the +// same test run cannot leak into the result. +func (ts *VerifyTestSuite) observeOutcome(w *httptest.ResponseRecorder, userID uuid.UUID, since time.Time) otpParityOutcome { + outcome := otpParityOutcome{Status: w.Code} + + if w.Code != http.StatusOK { + var body struct { + ErrorCode string `json:"error_code"` + Msg string `json:"msg"` + } + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&body)) + outcome.ErrorCode = body.ErrorCode + outcome.Msg = body.Msg + } + + u, err := models.FindUserByID(ts.API.db, userID) + require.NoError(ts.T(), err) + outcome.EmailConfirmed = u.EmailConfirmedAt != nil + outcome.PhoneConfirmed = u.PhoneConfirmedAt != nil + outcome.Email = u.GetEmail() + outcome.Phone = u.GetPhone() + + logs, err := models.FindAuditLogEntries(ts.API.db, nil, "", nil) + require.NoError(ts.T(), err) + if len(logs) > 0 && !logs[0].CreatedAt.Before(since) { + outcome.Action, _ = logs[0].Payload["action"].(string) + } + + return outcome +} + +func (ts *VerifyTestSuite) configureTestOTP(phone, otp string) func() { + previous := ts.Config.Sms.TestOTP + ts.Config.Sms.TestOTP = map[string]string{phone: otp} + return func() { ts.Config.Sms.TestOTP = previous } +} + +// configureTwilioVerify switches the SMS provider to Twilio Verify and arms a +// single mocked VerificationCheck response. +func (ts *VerifyTestSuite) configureTwilioVerify(response map[string]interface{}) func() { + previousProvider := ts.Config.Sms.Provider + previousTwilio := ts.Config.Sms.TwilioVerify + previousMock := sms_provider.MockProvider + + ts.Config.Sms.Provider = "twilio_verify" + ts.Config.Sms.TwilioVerify = conf.TwilioVerifyProviderConfiguration{ + AccountSid: "AC-parity-test", + AuthToken: "parity-test-token", + MessageServiceSid: twilioServiceSid, + } + // The mock provider would short-circuit GetSmsProvider and never reach + // the Twilio Verify type assertion. + sms_provider.MockProvider = nil + + gock.New("https://verify.twilio.com/v2/Services/" + twilioServiceSid + "/VerificationCheck"). + Post(""). + Reply(http.StatusOK). + JSON(response) + + return func() { + gock.OffAll() + sms_provider.MockProvider = previousMock + ts.Config.Sms.TwilioVerify = previousTwilio + ts.Config.Sms.Provider = previousProvider + } +} + +func emailOTPBody(verifyType, email string) map[string]interface{} { + return map[string]interface{}{ + "type": verifyType, + "token": parityOTP, + "email": email, + } +} + +func phoneOTPBody(verifyType, phone string) map[string]interface{} { + return map[string]interface{}{ + "type": verifyType, + "token": parityOTP, + "phone": phone, + } +} From d7423a4b22ef314d1c8a120b86399ef898278a55 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Fri, 4 Sep 2026 15:50:40 -0400 Subject: [PATCH 04/14] fix(otp): resolve verifying a user from the one_time_tokens row --- internal/api/verify.go | 150 +++++++++++++++++++------ internal/api/verify_ott_parity_test.go | 45 ++++---- internal/models/one_time_token.go | 5 + 3 files changed, 142 insertions(+), 58 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index 3b4ce803b0..a98e16a032 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -696,43 +696,137 @@ func (a *API) verifyTokenHash(conn *storage.Connection, params *VerifyParams) (* return user, nil } -// verifyUserAndToken verifies the token associated to the user based on the verify type -func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - config := a.config +func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + tokenType := models.ConfirmationToken + if params.Type == phoneChangeVerification { + tokenType = models.PhoneChangeToken + } + + ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) + } + + user, err := models.FindUserByID(conn, ott.UserID) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + pendingPhone := user.GetPhone() + if params.Type == phoneChangeVerification { + pendingPhone = user.PhoneChange // Should we use GetPhoneChange? + } + if pendingPhone != params.Phone { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") + } + if user.Aud != aud { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") + } + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return user, nil +} +// findUserForTestOTP resolves the user for a phone verification whose code +// matched a configured test OTP. A test OTP has no local challenge. +func (a *API) findUserForTestOTP(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { var user *models.User var err error - tokenHash := params.TokenHash switch params.Type { case phoneChangeVerification: user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) case smsVerification: user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) - case mail.EmailChangeVerification: - // Since the email change could be trigger via the implicit or PKCE flow, - // the query used has to also check if the token saved in the db contains the pkce_ prefix - user, err = models.FindUserForEmailChange(conn, params.Email, tokenHash, aud, config.Mailer.SecureEmailChangeEnabled) default: - user, err = models.FindUserByEmailAndAudience(conn, params.Email, aud) + // The caller only routes phone types here, so in practice this should never happen. + return nil, apierrors.NewInternalServerError("Test OTP lookup called for non-phone verification type %q", params.Type) } - - if err != nil { - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } if user.IsBanned() { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") } + return user, nil +} - var isValid bool +// verifyUserAndToken verifies the token associated to the user based on the verify type +func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + config := a.config if config.Experimental.EnableOTTAsSourceOfTruth { - return a.verifyOneTimeToken(conn, user, params) + // Twilio Verify and test OTPs are verified without a local challenge + if params.Type == smsVerification || params.Type == phoneChangeVerification { + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { + return a.findUserForTestOTP(conn, params, aud) + } + if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { + return a.verifyPhoneWithTwilio(conn, params, aud) + } + } + + ott, err := a.verifyOneTimeToken(conn, params) + if err != nil { + return nil, err + } + + user, err := models.FindUserByID(conn, ott.UserID) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + if user.Aud != aud { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") + } + + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + return user, nil } else { + var user *models.User + var err error + tokenHash := params.TokenHash + + var isValid bool + switch params.Type { + case phoneChangeVerification: + user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) + case smsVerification: + user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) + case mail.EmailChangeVerification: + // Since the email change could be trigger via the implicit or PKCE flow, + // the query used has to also check if the token saved in the db contains the pkce_ prefix + user, err = models.FindUserForEmailChange(conn, params.Email, tokenHash, aud, config.Mailer.SecureEmailChangeEnabled) + default: + user, err = models.FindUserByEmailAndAudience(conn, params.Email, aud) + } + + if err != nil { + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + smsProvider, _ := sms_provider.GetSmsProvider(*config) switch params.Type { case mail.EmailOTPVerification: @@ -781,33 +875,17 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, if !isValid { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") } + return user, nil } - return user, nil } -func (a *API) verifyOneTimeToken(conn *storage.Connection, user *models.User, params *VerifyParams) (*models.User, error) { - config := a.config - // Twilio Verify and test OTPs are verified without a local challenge - if params.Type == smsVerification || params.Type == phoneChangeVerification { - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { - return user, nil - } - if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - // For a phone change, params.Phone is the persisted phone_change - // number, because that is how the user was found. - if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { - return nil, err - } - return user, nil - } - } - +func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { tokenTypes := verifyTypeToTokenTypes(params.Type) if len(tokenTypes) == 0 { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("unknown verification type") } - ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, user.ID, params.TokenHash, tokenTypes...) + ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, params.TokenHash, tokenTypes...) if models.IsNotFoundError(err) { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token not found") } else if err != nil { @@ -828,7 +906,7 @@ func (a *API) verifyOneTimeToken(conn *storage.Connection, user *models.User, pa } } - return user, nil + return ott, nil } // verifyOTPWithTwilio asks Twilio Verify to check the code. Twilio generates diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 87ae37e907..5811cbd414 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -86,7 +86,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { { desc: "signup with a valid code confirms the user", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.SignupVerification, parityEmail) }, expected: signedUp, @@ -94,7 +94,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { { desc: "signup with an expired code is rejected", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, emailHash, expired, -time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, expired, -time.Hour) return emailOTPBody(mail.SignupVerification, parityEmail) }, expected: forbidden, @@ -102,7 +102,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { { desc: "signup with the wrong code is rejected", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, crypto.GenerateTokenHash(parityEmail, "999999"), now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, crypto.GenerateTokenHash(parityEmail, "999999"), now, time.Hour) return emailOTPBody(mail.SignupVerification, parityEmail) }, expected: forbidden, @@ -111,7 +111,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { desc: "invite with a valid code confirms the user", seed: func(u *models.User) map[string]interface{} { u.InvitedAt = &now - ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.InviteVerification, parityEmail) }, expected: signedUp, @@ -120,7 +120,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { desc: "magiclink with a valid code signs a confirmed user in", seed: func(u *models.User) map[string]interface{} { u.EmailConfirmedAt = &now - ts.seedChallenge(u, models.RecoveryToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.MagicLinkVerification, parityEmail) }, expected: loggedIn, @@ -129,7 +129,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { desc: "recovery with a valid code signs a confirmed user in", seed: func(u *models.User) map[string]interface{} { u.EmailConfirmedAt = &now - ts.seedChallenge(u, models.RecoveryToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.RecoveryVerification, parityEmail) }, expected: loggedIn, @@ -140,7 +140,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { desc: "magiclink with a pkce_ prefixed stored hash accepts the plain code", seed: func(u *models.User) map[string]interface{} { u.EmailConfirmedAt = &now - ts.seedChallenge(u, models.RecoveryToken, PKCEPrefix+emailHash, now, time.Hour) + ts.seedChallenge(u, models.RecoveryToken, parityEmail, PKCEPrefix+emailHash, now, time.Hour) return emailOTPBody(mail.MagicLinkVerification, parityEmail) }, expected: loggedIn, @@ -150,7 +150,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { // stored challenge is a confirmation token. desc: "email type with a confirmation token runs the signup flow", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.EmailOTPVerification, parityEmail) }, expected: signedUp, @@ -161,7 +161,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { desc: "email type with a recovery token runs the magiclink flow", seed: func(u *models.User) map[string]interface{} { u.EmailConfirmedAt = &now - ts.seedChallenge(u, models.RecoveryToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.EmailOTPVerification, parityEmail) }, expected: loggedIn, @@ -182,7 +182,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { }, seed: func(u *models.User) map[string]interface{} { u.EmailChange = parityNewEmail - ts.seedChallenge(u, models.EmailChangeTokenNew, crypto.GenerateTokenHash(parityNewEmail, parityOTP), now, time.Hour) + ts.seedChallenge(u, models.EmailChangeTokenNew, parityNewEmail, crypto.GenerateTokenHash(parityNewEmail, parityOTP), now, time.Hour) return emailOTPBody(mail.EmailChangeVerification, parityNewEmail) }, expected: otpParityOutcome{ @@ -198,7 +198,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { seed: func(u *models.User) map[string]interface{} { bannedUntil := now.Add(time.Hour) u.BannedUntil = &bannedUntil - ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody(mail.SignupVerification, parityEmail) }, expected: otpParityOutcome{ @@ -212,7 +212,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { { desc: "an unknown verification type is rejected", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, emailHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) return emailOTPBody("bogus", parityEmail) }, expected: forbidden, @@ -250,7 +250,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { { desc: "sms with a valid code confirms the phone", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, phoneHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) return phoneOTPBody(smsVerification, parityPhone) }, expected: phoneSignedUp, @@ -258,7 +258,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { { desc: "sms with an expired code is rejected", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, phoneHash, expired, -time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, expired, -time.Hour) return phoneOTPBody(smsVerification, parityPhone) }, expected: forbidden, @@ -266,7 +266,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { { desc: "sms with the wrong code is rejected", seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) return phoneOTPBody(smsVerification, parityPhone) }, expected: forbidden, @@ -275,7 +275,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { desc: "phone change with a valid code moves the user to the new number", seed: func(u *models.User) map[string]interface{} { u.PhoneChange = parityNewPhone - ts.seedChallenge(u, models.PhoneChangeToken, crypto.GenerateTokenHash(parityNewPhone, parityOTP), now, time.Hour) + ts.seedChallenge(u, models.PhoneChangeToken, parityNewPhone, crypto.GenerateTokenHash(parityNewPhone, parityOTP), now, time.Hour) return phoneOTPBody(phoneChangeVerification, parityNewPhone) }, expected: phoneChanged, @@ -311,7 +311,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { return ts.configureTwilioVerify(map[string]interface{}{"status": "approved", "valid": true}) }, seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) return phoneOTPBody(smsVerification, parityPhone) }, expected: phoneSignedUp, @@ -322,7 +322,7 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { return ts.configureTwilioVerify(map[string]interface{}{"status": "pending", "valid": false}) }, seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, phoneHash, now, time.Hour) + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) return phoneOTPBody(smsVerification, parityPhone) }, expected: forbidden, @@ -380,9 +380,10 @@ func (ts *VerifyTestSuite) runOTPParityCases(cases []otpParityCase) { } // seedChallenge stores hash in the users column and the one_time_tokens row -// for tokenType, mirroring what the send paths write. Any other pending -// change on u is persisted at the same time. -func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, hash string, sentAt time.Time, validity time.Duration) { +// for tokenType, mirroring what the send paths write. relatesTo is the address +// or number the code was sent to; the Twilio Verify path finds the row by it. +// Any other pending change on u is persisted at the same time. +func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, relatesTo, hash string, sentAt time.Time, validity time.Duration) { switch tokenType { case models.ConfirmationToken: u.ConfirmationToken = hash @@ -401,7 +402,7 @@ func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTim } require.NoError(ts.T(), ts.API.db.Update(u)) - require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, "relates_to not used", hash, tokenType, validity)) + require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, relatesTo, hash, tokenType, validity)) } func (ts *VerifyTestSuite) postVerify(body map[string]interface{}) *httptest.ResponseRecorder { diff --git a/internal/models/one_time_token.go b/internal/models/one_time_token.go index 03affc60d9..09badf1ffc 100644 --- a/internal/models/one_time_token.go +++ b/internal/models/one_time_token.go @@ -120,6 +120,11 @@ type OneTimeToken struct { ExpiresAt *time.Time `json:"expires_at" db:"expires_at"` } +// 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) +} + func (OneTimeToken) TableName() string { return "one_time_tokens" } From 4caedac50ed8e4e1b711b978b1278b13760182ef Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Fri, 4 Sep 2026 15:55:38 -0400 Subject: [PATCH 05/14] fix: reorder functions to make diff more readable --- internal/api/verify.go | 132 ++++++++++++++++++++--------------------- 1 file changed, 66 insertions(+), 66 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index a98e16a032..85f6d9fcc0 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -696,72 +696,6 @@ func (a *API) verifyTokenHash(conn *storage.Connection, params *VerifyParams) (* return user, nil } -func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - tokenType := models.ConfirmationToken - if params.Type == phoneChangeVerification { - tokenType = models.PhoneChangeToken - } - - ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType) - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) - } - - user, err := models.FindUserByID(conn, ott.UserID) - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) - } - - pendingPhone := user.GetPhone() - if params.Type == phoneChangeVerification { - pendingPhone = user.PhoneChange // Should we use GetPhoneChange? - } - if pendingPhone != params.Phone { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") - } - if user.Aud != aud { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") - } - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") - } - if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } - return user, nil -} - -// findUserForTestOTP resolves the user for a phone verification whose code -// matched a configured test OTP. A test OTP has no local challenge. -func (a *API) findUserForTestOTP(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - var user *models.User - var err error - - switch params.Type { - case phoneChangeVerification: - user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) - case smsVerification: - user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) - default: - // The caller only routes phone types here, so in practice this should never happen. - return nil, apierrors.NewInternalServerError("Test OTP lookup called for non-phone verification type %q", params.Type) - } - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) - } - - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") - } - return user, nil -} - // verifyUserAndToken verifies the token associated to the user based on the verify type func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { config := a.config @@ -879,6 +813,72 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, } } +func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + tokenType := models.ConfirmationToken + if params.Type == phoneChangeVerification { + tokenType = models.PhoneChangeToken + } + + ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) + } + + user, err := models.FindUserByID(conn, ott.UserID) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + pendingPhone := user.GetPhone() + if params.Type == phoneChangeVerification { + pendingPhone = user.PhoneChange // Should we use GetPhoneChange? + } + if pendingPhone != params.Phone { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") + } + if user.Aud != aud { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") + } + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return user, nil +} + +// findUserForTestOTP resolves the user for a phone verification whose code +// matched a configured test OTP. A test OTP has no local challenge. +func (a *API) findUserForTestOTP(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + var user *models.User + var err error + + switch params.Type { + case phoneChangeVerification: + user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) + case smsVerification: + user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) + default: + // The caller only routes phone types here, so in practice this should never happen. + return nil, apierrors.NewInternalServerError("Test OTP lookup called for non-phone verification type %q", params.Type) + } + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + return user, nil +} + func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { tokenTypes := verifyTypeToTokenTypes(params.Type) if len(tokenTypes) == 0 { From ebdd96eb09acd0a5a1c55ae0e8cb412ea8d7866d Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Wed, 9 Sep 2026 09:51:10 -0400 Subject: [PATCH 06/14] feat: validate user for ott --- internal/api/verify.go | 43 +++++++++++++++- internal/api/verify_ott_parity_test.go | 68 ++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index 85f6d9fcc0..d5259d7060 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -723,8 +723,8 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } - if user.Aud != aud { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") + if err := validateUserForOTT(params, ott, user, aud); err != nil { + return nil, err } if user.IsBanned() { @@ -945,6 +945,45 @@ func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { } } +// validateUserForOTT checks that the user found from a one_time_tokens row is +// the one the request is entitled to act on. The legacy path gets these +// guarantees for free from its identifier-keyed lookups (which also filter on +// aud and is_sso_user), so a mismatch there is a not-found. The one_time_tokens +// path finds the user by token hash, so it has to check the binding itself. +func validateUserForOTT(params *VerifyParams, ott *models.OneTimeToken, user *models.User, aud string) error { + mismatch := apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid") + + if user.IsSSOUser { + return mismatch.WithInternalMessage("SSO users cannot be verified with one time tokens") + } + + if user.Aud != aud { + return mismatch.WithInternalMessage("user audience does not match") + } + + // Pick the identifier on the user record that this verify type is bound to, + // then compare it against the identifier in the request. + var expected, actual, field string + switch params.Type { + case smsVerification: + expected, actual, field = user.GetPhone(), params.Phone, "phone" + case phoneChangeVerification: + expected, actual, field = user.PhoneChange, params.Phone, "phone" + case mail.EmailChangeVerification: + expected, actual, field = user.EmailChange, params.Email, "email" + if ott.TokenType == models.EmailChangeTokenCurrent { + expected = user.GetEmail() + } + default: // Signup, Invite, Recovery, MagicLink + expected, actual, field = user.GetEmail(), params.Email, "email" + } + + if actual == "" || !strings.EqualFold(expected, actual) { + return mismatch.WithInternalMessage("user %s does not match", field) + } + return nil +} + // isOtpValid checks the actual otp sent against the expected otp and ensures that it's within the valid window func isOtpValid(actual, expected string, sentAt *time.Time, otpExp uint) bool { if expected == "" || sentAt == nil { diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 5811cbd414..97f3f264c7 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -500,3 +500,71 @@ func phoneOTPBody(verifyType, phone string) map[string]interface{} { "phone": phone, } } + +// TestVerifyOTPParityIdentifierBinding covers the guarantee that the +// identifier in the request is the one the challenge was issued for. Legacy +// gets this from its identifier-keyed user lookups: the email or phone in the +// body is the lookup key, so a mismatch is a not-found. The one_time_tokens +// path finds the user by token hash, so it has to check the binding itself. +// Every case here is rejected by legacy; the one_time_tokens path must agree. +func (ts *VerifyTestSuite) TestVerifyOTPParityIdentifierBinding() { + now := time.Now() + emailHash := crypto.GenerateTokenHash(parityEmail, parityOTP) + phoneHash := crypto.GenerateTokenHash(parityPhone, parityOTP) + + forbidden := otpParityOutcome{ + Status: http.StatusForbidden, + ErrorCode: apierrors.ErrorCodeOTPExpired, + Msg: parityForbidden, + Email: parityEmail, + Phone: parityPhone, + } + + cases := []otpParityCase{ + { + // The client owns the phone and has an unconfirmed email on the + // same account. Presenting the SMS code as an email signup must + // not confirm the email, because no email was ever delivered. + desc: "a phone code posted as signup does not confirm the email", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) + return phoneOTPBody(mail.SignupVerification, parityPhone) + }, + expected: forbidden, + }, + { + // supabase-js sends the generic "email" type for every email OTP. + // The generic type must not let a phone code through either. + desc: "a phone code posted as the generic email type does not confirm the email", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) + return phoneOTPBody(mail.EmailOTPVerification, parityPhone) + }, + expected: forbidden, + }, + { + // The mirror image: an email confirmation code presented as an + // SMS code must not confirm the phone. + desc: "an email code posted as sms does not confirm the phone", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(smsVerification, parityEmail) + }, + expected: forbidden, + }, + { + // Every legacy user lookup filters is_sso_user = false, so an + // SSO-managed account can never be signed in with a typed OTP. + // A challenge row for one must not change that. + desc: "an SSO user cannot verify a typed OTP", + seed: func(u *models.User) map[string]interface{} { + u.IsSSOUser = true + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: forbidden, + }, + } + + ts.runOTPParityCases(cases) +} From 0820b2b8bf176c56629e8f02dd028c6e47bfe98a Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Wed, 9 Sep 2026 09:55:46 -0400 Subject: [PATCH 07/14] chore: refactor --- internal/api/verify.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index d5259d7060..ce8b235711 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -961,25 +961,27 @@ func validateUserForOTT(params *VerifyParams, ott *models.OneTimeToken, user *mo return mismatch.WithInternalMessage("user audience does not match") } - // Pick the identifier on the user record that this verify type is bound to, - // then compare it against the identifier in the request. - var expected, actual, field string switch params.Type { case smsVerification: - expected, actual, field = user.GetPhone(), params.Phone, "phone" + if params.Phone == "" || user.GetPhone() != params.Phone { + return mismatch.WithInternalMessage("user phone does not match") + } case phoneChangeVerification: - expected, actual, field = user.PhoneChange, params.Phone, "phone" + if params.Phone == "" || user.PhoneChange != params.Phone { + return mismatch.WithInternalMessage("user phone does not match") + } case mail.EmailChangeVerification: - expected, actual, field = user.EmailChange, params.Email, "email" + expected := user.EmailChange if ott.TokenType == models.EmailChangeTokenCurrent { expected = user.GetEmail() } + if params.Email == "" || !strings.EqualFold(expected, params.Email) { + return mismatch.WithInternalMessage("user email does not match") + } default: // Signup, Invite, Recovery, MagicLink - expected, actual, field = user.GetEmail(), params.Email, "email" - } - - if actual == "" || !strings.EqualFold(expected, actual) { - return mismatch.WithInternalMessage("user %s does not match", field) + if params.Email == "" || !strings.EqualFold(user.GetEmail(), params.Email) { + return mismatch.WithInternalMessage("user email does not match") + } } return nil } From 869d96164ec7815616c557cc20e2e06ccfcf0ffc Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Wed, 9 Sep 2026 15:35:50 -0400 Subject: [PATCH 08/14] chore(otp): change flagging logic to improve readability --- internal/api/verify.go | 194 ++++++++++++++++++++++------------------- 1 file changed, 103 insertions(+), 91 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index ce8b235711..f9caa94a2c 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -701,116 +701,128 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, config := a.config if config.Experimental.EnableOTTAsSourceOfTruth { - // Twilio Verify and test OTPs are verified without a local challenge - if params.Type == smsVerification || params.Type == phoneChangeVerification { - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { - return a.findUserForTestOTP(conn, params, aud) - } - if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - return a.verifyPhoneWithTwilio(conn, params, aud) - } - } + return a.verifyUserAndTokenFromOTT(conn, params, aud) + } - ott, err := a.verifyOneTimeToken(conn, params) - if err != nil { - return nil, err - } + var user *models.User + var err error + tokenHash := params.TokenHash - user, err := models.FindUserByID(conn, ott.UserID) + switch params.Type { + case phoneChangeVerification: + user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) + case smsVerification: + user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) + case mail.EmailChangeVerification: + // Since the email change could be trigger via the implicit or PKCE flow, + // the query used has to also check if the token saved in the db contains the pkce_ prefix + user, err = models.FindUserForEmailChange(conn, params.Email, tokenHash, aud, config.Mailer.SecureEmailChangeEnabled) + default: + user, err = models.FindUserByEmailAndAudience(conn, params.Email, aud) + } + + if err != nil { if models.IsNotFoundError(err) { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } - if err := validateUserForOTT(params, ott, user, aud); err != nil { - return nil, err - } + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") - } - return user, nil - } else { - var user *models.User - var err error - tokenHash := params.TokenHash + var isValid bool - var isValid bool - switch params.Type { - case phoneChangeVerification: - user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) - case smsVerification: - user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) - case mail.EmailChangeVerification: - // Since the email change could be trigger via the implicit or PKCE flow, - // the query used has to also check if the token saved in the db contains the pkce_ prefix - user, err = models.FindUserForEmailChange(conn, params.Email, tokenHash, aud, config.Mailer.SecureEmailChangeEnabled) - default: - user, err = models.FindUserByEmailAndAudience(conn, params.Email, aud) + smsProvider, _ := sms_provider.GetSmsProvider(*config) + switch params.Type { + case mail.EmailOTPVerification: + // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns + if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { + isValid = true + params.Type = mail.SignupVerification + } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { + isValid = true + params.Type = mail.MagicLinkVerification + } else { + isValid = false } - - if err != nil { - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + case mail.SignupVerification, mail.InviteVerification: + isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) + case mail.RecoveryVerification, mail.MagicLinkVerification: + isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) + case mail.EmailChangeVerification: + isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || + isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) + case phoneChangeVerification, smsVerification: + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { + if params.Token == testOTP { + return user, nil } - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + phone := params.Phone + sentAt := user.ConfirmationSentAt + expectedToken := user.ConfirmationToken + if params.Type == phoneChangeVerification { + phone = user.PhoneChange + sentAt = user.PhoneChangeSentAt + expectedToken = user.PhoneChangeToken } - smsProvider, _ := sms_provider.GetSmsProvider(*config) - switch params.Type { - case mail.EmailOTPVerification: - // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns - if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { - isValid = true - params.Type = mail.SignupVerification - } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { - isValid = true - params.Type = mail.MagicLinkVerification - } else { - isValid = false - } - case mail.SignupVerification, mail.InviteVerification: - isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) - case mail.RecoveryVerification, mail.MagicLinkVerification: - isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) - case mail.EmailChangeVerification: - isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || - isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) - case phoneChangeVerification, smsVerification: - // Check if test OP, if so skip validation and return user - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { - if params.Token == testOTP { - return user, nil - } + if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { + if err := smsProvider.(*sms_provider.TwilioVerifyProvider).VerifyOTP(phone, params.Token); err != nil { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) } + return user, nil + } + isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) + } - phone := params.Phone - sentAt := user.ConfirmationSentAt - expectedToken := user.ConfirmationToken - if params.Type == phoneChangeVerification { - phone = user.PhoneChange - sentAt = user.PhoneChangeSentAt - expectedToken = user.PhoneChangeToken - } + if !isValid { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") + } + return user, nil +} - if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - if err := smsProvider.(*sms_provider.TwilioVerifyProvider).VerifyOTP(phone, params.Token); err != nil { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } - return user, nil - } - isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) +// verifyUserAndTokenFromOTT is the EnableOTTAsSourceOfTruth path. It finds the +// challenge in the one_time_tokens table and derives the user from that row, +// instead of finding the user by identifier and comparing the users.*_token +// columns. A lookup miss is rejected as an expired or invalid token; there is +// no fallback to the users columns. +func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + config := a.config + + // Twilio Verify and test OTPs are verified without a local challenge + if params.Type == smsVerification || params.Type == phoneChangeVerification { + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { + return a.findUserForTestOTP(conn, params, aud) } - if !isValid { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") + if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { + return a.verifyPhoneWithTwilio(conn, params, aud) } - return user, nil } + + ott, err := a.verifyOneTimeToken(conn, params) + if err != nil { + return nil, err + } + + user, err := models.FindUserByID(conn, ott.UserID) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + if err := validateUserForOTT(params, ott, user, aud); err != nil { + return nil, err + } + + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + return user, nil } func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { @@ -835,7 +847,7 @@ func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyPara pendingPhone := user.GetPhone() if params.Type == phoneChangeVerification { - pendingPhone = user.PhoneChange // Should we use GetPhoneChange? + pendingPhone = user.PhoneChange } if pendingPhone != params.Phone { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") From 6d15a97055c3a5d92f4ee2a4b5d266d3b7092604 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 10:38:46 -0400 Subject: [PATCH 09/14] chore(otp): defer test OTP and Twilio Verify handling to a follow-up PR 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. --- internal/api/verify.go | 98 +----------------------- internal/api/verify_ott_parity_test.go | 101 ++----------------------- 2 files changed, 9 insertions(+), 190 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index f9caa94a2c..a523b45cfa 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -789,20 +789,9 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, // challenge in the one_time_tokens table and derives the user from that row, // instead of finding the user by identifier and comparing the users.*_token // columns. A lookup miss is rejected as an expired or invalid token; there is -// no fallback to the users columns. +// no fallback to the users columns. Test OTPs and Twilio Verify are not handled +// yet on this path; a follow-up PR adds them. func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - config := a.config - - // Twilio Verify and test OTPs are verified without a local challenge - if params.Type == smsVerification || params.Type == phoneChangeVerification { - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { - return a.findUserForTestOTP(conn, params, aud) - } - if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - return a.verifyPhoneWithTwilio(conn, params, aud) - } - } - ott, err := a.verifyOneTimeToken(conn, params) if err != nil { return nil, err @@ -825,72 +814,6 @@ func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *Verify return user, nil } -func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - tokenType := models.ConfirmationToken - if params.Type == phoneChangeVerification { - tokenType = models.PhoneChangeToken - } - - ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType) - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) - } - - user, err := models.FindUserByID(conn, ott.UserID) - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) - } - - pendingPhone := user.GetPhone() - if params.Type == phoneChangeVerification { - pendingPhone = user.PhoneChange - } - if pendingPhone != params.Phone { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") - } - if user.Aud != aud { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") - } - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") - } - if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } - return user, nil -} - -// findUserForTestOTP resolves the user for a phone verification whose code -// matched a configured test OTP. A test OTP has no local challenge. -func (a *API) findUserForTestOTP(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - var user *models.User - var err error - - switch params.Type { - case phoneChangeVerification: - user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) - case smsVerification: - user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) - default: - // The caller only routes phone types here, so in practice this should never happen. - return nil, apierrors.NewInternalServerError("Test OTP lookup called for non-phone verification type %q", params.Type) - } - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) - } - - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") - } - return user, nil -} - func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { tokenTypes := verifyTypeToTokenTypes(params.Type) if len(tokenTypes) == 0 { @@ -921,23 +844,6 @@ func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) return ott, nil } -// verifyOTPWithTwilio asks Twilio Verify to check the code. Twilio generates -// and delivers its own code, so there is no local challenge to compare. -func (a *API) verifyOTPWithTwilio(phone, code string) error { - smsProvider, err := sms_provider.GetSmsProvider(*a.config) - if err != nil { - return apierrors.NewInternalServerError("Failed to get SMS provider").WithInternalError(err) - } - twilioVerify, ok := smsProvider.(*sms_provider.TwilioVerifyProvider) - if !ok { - return apierrors.NewInternalServerError("SMS provider is not Twilio Verify") - } - if err := twilioVerify.VerifyOTP(phone, code); err != nil { - return apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } - return nil -} - func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { switch verifyType { case mail.EmailOTPVerification: diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 97f3f264c7..90a7613351 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -9,11 +9,8 @@ import ( "github.com/gofrs/uuid" "github.com/stretchr/testify/require" - "gopkg.in/h2non/gock.v1" "github.com/supabase/auth/internal/api/apierrors" - "github.com/supabase/auth/internal/api/sms_provider" - "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/crypto" mail "github.com/supabase/auth/internal/mailer" "github.com/supabase/auth/internal/models" @@ -27,13 +24,12 @@ import ( // in the fixtures. const ( - parityOTP = "123456" - parityEmail = "test@example.com" - parityPhone = "12345678" - parityNewEmail = "new@example.com" - parityNewPhone = "1234567890" - parityForbidden = "Token has expired or is invalid" - twilioServiceSid = "VA-parity-test" + parityOTP = "123456" + parityEmail = "test@example.com" + parityPhone = "12345678" + parityNewEmail = "new@example.com" + parityNewPhone = "1234567890" + parityForbidden = "Token has expired or is invalid" ) // otpParityOutcome is everything a client or an operator can observe after a @@ -280,53 +276,6 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { }, expected: phoneChanged, }, - { - // A test OTP is accepted without any stored challenge. This is the - // path app store reviewers and CI rely on. - desc: "sms with a test OTP succeeds with no stored challenge", - configure: func() func() { - return ts.configureTestOTP(parityPhone, parityOTP) - }, - seed: func(u *models.User) map[string]interface{} { - return phoneOTPBody(smsVerification, parityPhone) - }, - expected: phoneSignedUp, - }, - { - desc: "sms with a wrong code falls through the test OTP check and is rejected", - configure: func() func() { - return ts.configureTestOTP(parityPhone, "000000") - }, - seed: func(u *models.User) map[string]interface{} { - return phoneOTPBody(smsVerification, parityPhone) - }, - expected: forbidden, - }, - { - // Twilio Verify generates and delivers its own code, so the locally - // stored hash never matches what the user types. Twilio's answer - // is the only thing that counts. - desc: "sms with Twilio Verify accepts a code Twilio approves", - configure: func() func() { - return ts.configureTwilioVerify(map[string]interface{}{"status": "approved", "valid": true}) - }, - seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) - return phoneOTPBody(smsVerification, parityPhone) - }, - expected: phoneSignedUp, - }, - { - desc: "sms with Twilio Verify rejects a code Twilio does not approve", - configure: func() func() { - return ts.configureTwilioVerify(map[string]interface{}{"status": "pending", "valid": false}) - }, - seed: func(u *models.User) map[string]interface{} { - ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) - return phoneOTPBody(smsVerification, parityPhone) - }, - expected: forbidden, - }, } ts.runOTPParityCases(cases) @@ -381,7 +330,7 @@ func (ts *VerifyTestSuite) runOTPParityCases(cases []otpParityCase) { // seedChallenge stores hash in the users column and the one_time_tokens row // for tokenType, mirroring what the send paths write. relatesTo is the address -// or number the code was sent to; the Twilio Verify path finds the row by it. +// or number the code was sent to. // Any other pending change on u is persisted at the same time. func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, relatesTo, hash string, sentAt time.Time, validity time.Duration) { switch tokenType { @@ -449,42 +398,6 @@ func (ts *VerifyTestSuite) observeOutcome(w *httptest.ResponseRecorder, userID u return outcome } -func (ts *VerifyTestSuite) configureTestOTP(phone, otp string) func() { - previous := ts.Config.Sms.TestOTP - ts.Config.Sms.TestOTP = map[string]string{phone: otp} - return func() { ts.Config.Sms.TestOTP = previous } -} - -// configureTwilioVerify switches the SMS provider to Twilio Verify and arms a -// single mocked VerificationCheck response. -func (ts *VerifyTestSuite) configureTwilioVerify(response map[string]interface{}) func() { - previousProvider := ts.Config.Sms.Provider - previousTwilio := ts.Config.Sms.TwilioVerify - previousMock := sms_provider.MockProvider - - ts.Config.Sms.Provider = "twilio_verify" - ts.Config.Sms.TwilioVerify = conf.TwilioVerifyProviderConfiguration{ - AccountSid: "AC-parity-test", - AuthToken: "parity-test-token", - MessageServiceSid: twilioServiceSid, - } - // The mock provider would short-circuit GetSmsProvider and never reach - // the Twilio Verify type assertion. - sms_provider.MockProvider = nil - - gock.New("https://verify.twilio.com/v2/Services/" + twilioServiceSid + "/VerificationCheck"). - Post(""). - Reply(http.StatusOK). - JSON(response) - - return func() { - gock.OffAll() - sms_provider.MockProvider = previousMock - ts.Config.Sms.TwilioVerify = previousTwilio - ts.Config.Sms.Provider = previousProvider - } -} - func emailOTPBody(verifyType, email string) map[string]interface{} { return map[string]interface{}{ "type": verifyType, From a313e082faad53a6d8f7e789902ba3015c22d322 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 14:18:15 -0400 Subject: [PATCH 10/14] docs: clarify --- internal/api/verify.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index a523b45cfa..f53941841a 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -789,9 +789,12 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, // challenge in the one_time_tokens table and derives the user from that row, // instead of finding the user by identifier and comparing the users.*_token // columns. A lookup miss is rejected as an expired or invalid token; there is -// no fallback to the users columns. Test OTPs and Twilio Verify are not handled -// yet on this path; a follow-up PR adds them. +// no fallback to the users columns. +// +// NOTE: Test OTPs and Twilio Verify are not handled yet on this path; a follow-up PR will add them. func (a *API) 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. ott, err := a.verifyOneTimeToken(conn, params) if err != nil { return nil, err From 324860d4b4f8ea3116075c4e5a6dc852627536f3 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 15:35:12 -0400 Subject: [PATCH 11/14] chore: refactor tests --- internal/api/verify_ott_parity_test.go | 399 ++++++++++++------------- 1 file changed, 186 insertions(+), 213 deletions(-) diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 90a7613351..88fa7da579 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -16,13 +16,10 @@ import ( "github.com/supabase/auth/internal/models" ) -// The typed-OTP verify path can read the challenge from either the legacy -// users.*_token columns or, with EnableOTTAsSourceOfTruth, from the -// one_time_tokens table. These tests run every flow once per store and require -// the observable outcome to be identical. Both stores are seeded the way the -// send paths seed them, so a failure here is a divergence in verify logic, not -// in the fixtures. - +// The typed-OTP verification path can read the challenge from either the legacy +// users.*_token columns or, with EnableOTTAsSourceOfTruth, from the one_time_tokens table. +// +// These tests run every flow once per store with identical seeding and ensure the outcome is equal. const ( parityOTP = "123456" parityEmail = "test@example.com" @@ -33,8 +30,7 @@ const ( ) // otpParityOutcome is everything a client or an operator can observe after a -// POST /verify: the HTTP result, the user state it left, and the audit action -// it recorded. +// POST /verify: the HTTP result, the user state it left, and the audit action it recorded. type otpParityOutcome struct { Status int ErrorCode string @@ -47,12 +43,11 @@ type otpParityOutcome struct { } type otpParityCase struct { - desc string - // seed writes the challenge to both stores and returns the request body. - // It receives a freshly created, unconfirmed user. - seed func(u *models.User) map[string]interface{} - // configure applies per-case config and returns a function that undoes - // it. It runs once per store, so consumable mocks are re-armed each time. + // seed receives a user freshly created by the test suite's SetupTest fn, creates an OTT challenge + // and modifies the user to include the relevant OTT values, given our dual-write approach today. + seed func(u *models.User) + requestBody map[string]interface{} + // configure applies per-case config, and runs once per store. It returns a restore function that will be called after the test. configure func() func() expected otpParityOutcome } @@ -62,125 +57,115 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { expired := now.Add(-48 * time.Hour) emailHash := crypto.GenerateTokenHash(parityEmail, parityOTP) - baseline := otpParityOutcome{Email: parityEmail, Phone: parityPhone} - forbidden := baseline - forbidden.Status = http.StatusForbidden - forbidden.ErrorCode = apierrors.ErrorCodeOTPExpired - forbidden.Msg = parityForbidden - - signedUp := baseline - signedUp.Status = http.StatusOK - signedUp.Action = string(models.UserSignedUpAction) - signedUp.EmailConfirmed = true - - loggedIn := baseline - loggedIn.Status = http.StatusOK - loggedIn.Action = string(models.LoginAction) - loggedIn.EmailConfirmed = true - - cases := []otpParityCase{ - { - desc: "signup with a valid code confirms the user", - seed: func(u *models.User) map[string]interface{} { + forbidden := otpParityOutcome{ + Status: http.StatusForbidden, + ErrorCode: apierrors.ErrorCodeOTPExpired, + Msg: parityForbidden, + Email: parityEmail, + Phone: parityPhone, + } + + signedUp := otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserSignedUpAction), + EmailConfirmed: true, + Email: parityEmail, + Phone: parityPhone, + } + + loggedIn := otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.LoginAction), + EmailConfirmed: true, + Email: parityEmail, + Phone: parityPhone, + } + + cases := map[string]otpParityCase{ + "signup with a valid code confirms the user": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.SignupVerification, parityEmail) }, - expected: signedUp, + requestBody: emailOTPBody(mail.SignupVerification, parityEmail), + expected: signedUp, }, - { - desc: "signup with an expired code is rejected", - seed: func(u *models.User) map[string]interface{} { + "signup with an expired code is rejected": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, expired, -time.Hour) - return emailOTPBody(mail.SignupVerification, parityEmail) }, - expected: forbidden, + requestBody: emailOTPBody(mail.SignupVerification, parityEmail), + expected: forbidden, }, - { - desc: "signup with the wrong code is rejected", - seed: func(u *models.User) map[string]interface{} { + "signup with the wrong code is rejected": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityEmail, crypto.GenerateTokenHash(parityEmail, "999999"), now, time.Hour) - return emailOTPBody(mail.SignupVerification, parityEmail) }, - expected: forbidden, + requestBody: emailOTPBody(mail.SignupVerification, parityEmail), + expected: forbidden, }, - { - desc: "invite with a valid code confirms the user", - seed: func(u *models.User) map[string]interface{} { + "invite with a valid code confirms the user": { + seed: func(u *models.User) { u.InvitedAt = &now ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.InviteVerification, parityEmail) }, - expected: signedUp, + requestBody: emailOTPBody(mail.InviteVerification, parityEmail), + expected: signedUp, }, - { - desc: "magiclink with a valid code signs a confirmed user in", - seed: func(u *models.User) map[string]interface{} { + "magiclink with a valid code signs a confirmed user in": { + seed: func(u *models.User) { u.EmailConfirmedAt = &now ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.MagicLinkVerification, parityEmail) }, - expected: loggedIn, + requestBody: emailOTPBody(mail.MagicLinkVerification, parityEmail), + expected: loggedIn, }, - { - desc: "recovery with a valid code signs a confirmed user in", - seed: func(u *models.User) map[string]interface{} { + "magiclink with a pkce_ prefixed stored hash accepts the plain code": { + seed: func(u *models.User) { u.EmailConfirmedAt = &now - ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.RecoveryVerification, parityEmail) + ts.seedChallenge(u, models.RecoveryToken, parityEmail, PKCEPrefix+emailHash, now, time.Hour) }, - expected: loggedIn, + requestBody: emailOTPBody(mail.MagicLinkVerification, parityEmail), + expected: loggedIn, }, - { - // Tokens issued through the PKCE flow are stored with a pkce_ - // prefix. A plain code must still match them. - desc: "magiclink with a pkce_ prefixed stored hash accepts the plain code", - seed: func(u *models.User) map[string]interface{} { + "recovery with a valid code signs a confirmed user in": { + seed: func(u *models.User) { u.EmailConfirmedAt = &now - ts.seedChallenge(u, models.RecoveryToken, parityEmail, PKCEPrefix+emailHash, now, time.Hour) - return emailOTPBody(mail.MagicLinkVerification, parityEmail) + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) }, - expected: loggedIn, + requestBody: emailOTPBody(mail.RecoveryVerification, parityEmail), + expected: loggedIn, }, - { - // The generic "email" type must resolve to the signup flow when the - // stored challenge is a confirmation token. - desc: "email type with a confirmation token runs the signup flow", - seed: func(u *models.User) map[string]interface{} { + "email type with a confirmation token runs the signup flow": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.EmailOTPVerification, parityEmail) }, - expected: signedUp, + requestBody: emailOTPBody(mail.EmailOTPVerification, parityEmail), + expected: signedUp, }, - { - // The generic "email" type must resolve to the magiclink flow when - // the stored challenge is a recovery token. - desc: "email type with a recovery token runs the magiclink flow", - seed: func(u *models.User) map[string]interface{} { + "email type with a recovery token runs the magiclink flow": { + seed: func(u *models.User) { u.EmailConfirmedAt = &now ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.EmailOTPVerification, parityEmail) }, - expected: loggedIn, + requestBody: emailOTPBody(mail.EmailOTPVerification, parityEmail), + expected: loggedIn, }, - { - desc: "email type with no matching challenge is rejected", - seed: func(u *models.User) map[string]interface{} { - return emailOTPBody(mail.EmailOTPVerification, parityEmail) - }, - expected: forbidden, + "email type with no matching challenge is rejected": { + requestBody: emailOTPBody(mail.EmailOTPVerification, parityEmail), + expected: forbidden, }, - { - desc: "email change with a valid code moves the user to the new address", + "email change with a valid code moves the user to the new address": { + // Secure email change defaults to on, which needs a OTP for the old email address too. Turn it off and then revert. configure: func() func() { previous := ts.Config.Mailer.SecureEmailChangeEnabled ts.Config.Mailer.SecureEmailChangeEnabled = false return func() { ts.Config.Mailer.SecureEmailChangeEnabled = previous } }, - seed: func(u *models.User) map[string]interface{} { + seed: func(u *models.User) { u.EmailChange = parityNewEmail ts.seedChallenge(u, models.EmailChangeTokenNew, parityNewEmail, crypto.GenerateTokenHash(parityNewEmail, parityOTP), now, time.Hour) - return emailOTPBody(mail.EmailChangeVerification, parityNewEmail) }, + requestBody: emailOTPBody(mail.EmailChangeVerification, parityNewEmail), expected: otpParityOutcome{ Status: http.StatusOK, Action: string(models.UserModifiedAction), @@ -189,14 +174,13 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { Phone: parityPhone, }, }, - { - desc: "a banned user is rejected before the challenge is checked", - seed: func(u *models.User) map[string]interface{} { + "a banned user is rejected": { + seed: func(u *models.User) { bannedUntil := now.Add(time.Hour) u.BannedUntil = &bannedUntil ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.SignupVerification, parityEmail) }, + requestBody: emailOTPBody(mail.SignupVerification, parityEmail), expected: otpParityOutcome{ Status: http.StatusForbidden, ErrorCode: apierrors.ErrorCodeUserBanned, @@ -205,13 +189,12 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { Phone: parityPhone, }, }, - { - desc: "an unknown verification type is rejected", - seed: func(u *models.User) map[string]interface{} { + "an unknown verification type is rejected": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody("bogus", parityEmail) }, - expected: forbidden, + requestBody: emailOTPBody("bogus", parityEmail), + expected: forbidden, }, } @@ -223,16 +206,21 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { expired := now.Add(-48 * time.Hour) phoneHash := crypto.GenerateTokenHash(parityPhone, parityOTP) - baseline := otpParityOutcome{Email: parityEmail, Phone: parityPhone} - forbidden := baseline - forbidden.Status = http.StatusForbidden - forbidden.ErrorCode = apierrors.ErrorCodeOTPExpired - forbidden.Msg = parityForbidden + forbidden := otpParityOutcome{ + Status: http.StatusForbidden, + ErrorCode: apierrors.ErrorCodeOTPExpired, + Msg: parityForbidden, + Email: parityEmail, + Phone: parityPhone, + } - phoneSignedUp := baseline - phoneSignedUp.Status = http.StatusOK - phoneSignedUp.Action = string(models.UserSignedUpAction) - phoneSignedUp.PhoneConfirmed = true + phoneSignedUp := otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserSignedUpAction), + PhoneConfirmed: true, + Email: parityEmail, + Phone: parityPhone, + } phoneChanged := otpParityOutcome{ Status: http.StatusOK, @@ -242,39 +230,35 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { Phone: parityNewPhone, } - cases := []otpParityCase{ - { - desc: "sms with a valid code confirms the phone", - seed: func(u *models.User) map[string]interface{} { + cases := map[string]otpParityCase{ + "sms with a valid code confirms the phone": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) - return phoneOTPBody(smsVerification, parityPhone) }, - expected: phoneSignedUp, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: phoneSignedUp, }, - { - desc: "sms with an expired code is rejected", - seed: func(u *models.User) map[string]interface{} { + "sms with an expired code is rejected": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, expired, -time.Hour) - return phoneOTPBody(smsVerification, parityPhone) }, - expected: forbidden, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: forbidden, }, - { - desc: "sms with the wrong code is rejected", - seed: func(u *models.User) map[string]interface{} { + "sms with the wrong code is rejected": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) - return phoneOTPBody(smsVerification, parityPhone) }, - expected: forbidden, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: forbidden, }, - { - desc: "phone change with a valid code moves the user to the new number", - seed: func(u *models.User) map[string]interface{} { + "phone change with a valid code moves the user to the new number": { + seed: func(u *models.User) { u.PhoneChange = parityNewPhone ts.seedChallenge(u, models.PhoneChangeToken, parityNewPhone, crypto.GenerateTokenHash(parityNewPhone, parityOTP), now, time.Hour) - return phoneOTPBody(phoneChangeVerification, parityNewPhone) }, - expected: phoneChanged, + requestBody: phoneOTPBody(phoneChangeVerification, parityNewPhone), + expected: phoneChanged, }, } @@ -283,55 +267,61 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { // runOTPParityCases runs each case against both stores and asserts that both // produce the expected outcome and agree with each other. -func (ts *VerifyTestSuite) runOTPParityCases(cases []otpParityCase) { +func (ts *VerifyTestSuite) runOTPParityCases(testCases map[string]otpParityCase) { originalFlag := ts.Config.Experimental.EnableOTTAsSourceOfTruth defer func() { ts.Config.Experimental.EnableOTTAsSourceOfTruth = originalFlag }() - modes := []struct { - name string - flag bool - }{ - {name: "legacy users columns", flag: false}, - {name: "one_time_tokens", flag: true}, - } + for name, tc := range testCases { + ts.Run(name, func() { + var legacyOutcome, ottOutcome otpParityOutcome + + ts.Run("legacy users columns", func() { + legacyOutcome = ts.runOTPParityCase(tc, false) + }) + ts.Run("one_time_tokens", func() { + ottOutcome = ts.runOTPParityCase(tc, true) + }) - for _, caseItem := range cases { - c := caseItem - ts.Run(c.desc, func() { - outcomes := make(map[string]otpParityOutcome, len(modes)) - - for _, mode := range modes { - m := mode - ts.Run(m.name, func() { - ts.SetupTest() - ts.Config.Experimental.EnableOTTAsSourceOfTruth = m.flag - if c.configure != nil { - restore := c.configure() - defer restore() - } - - u, err := models.FindUserByEmailAndAudience(ts.API.db, parityEmail, ts.Config.JWT.Aud) - require.NoError(ts.T(), err) - body := c.seed(u) - - since := time.Now() - w := ts.postVerify(body) - outcome := ts.observeOutcome(w, u.ID, since) - require.Equal(ts.T(), c.expected, outcome) - outcomes[m.name] = outcome - }) - } - - require.Equal(ts.T(), outcomes[modes[0].name], outcomes[modes[1].name], + require.Equal(ts.T(), legacyOutcome, ottOutcome, "legacy and one_time_tokens paths must produce the same outcome") }) } } +// runOTPParityCase runs one case against one store and returns the outcome it +// observed. enableOTT selects the store. It also asserts the outcome matches +// c.expected, so a failure names the store that diverged. +func (ts *VerifyTestSuite) runOTPParityCase(c otpParityCase, enableOTT bool) otpParityOutcome { + ts.SetupTest() + ts.Config.Experimental.EnableOTTAsSourceOfTruth = enableOTT + if c.configure != nil { + restore := c.configure() + defer restore() + } + + u, err := models.FindUserByEmailAndAudience(ts.API.db, parityEmail, ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + if c.seed != nil { + c.seed(u) + } + + since := time.Now() + w := ts.postVerify(c.requestBody) + outcome := ts.observeOutcome(w, u.ID, since) + require.Equal(ts.T(), c.expected, outcome) + return outcome +} + +// saveUser persists every pending change on u. Call it from a seed that +// changes u but stores no challenge. seedChallenge calls it already. +func (ts *VerifyTestSuite) saveUser(u *models.User) { + require.NoError(ts.T(), ts.API.db.Update(u)) +} + // seedChallenge stores hash in the users column and the one_time_tokens row // for tokenType, mirroring what the send paths write. relatesTo is the address -// or number the code was sent to. -// Any other pending change on u is persisted at the same time. +// or number the code was sent to. It persists u, so it also saves any other +// change the case made. func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, relatesTo, hash string, sentAt time.Time, validity time.Duration) { switch tokenType { case models.ConfirmationToken: @@ -350,7 +340,7 @@ func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTim ts.T().Fatalf("seedChallenge does not support token type %s", tokenType) } - require.NoError(ts.T(), ts.API.db.Update(u)) + ts.saveUser(u) require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, relatesTo, hash, tokenType, validity)) } @@ -366,9 +356,6 @@ func (ts *VerifyTestSuite) postVerify(body map[string]interface{}) *httptest.Res return w } -// observeOutcome collects the response and the resulting user state. Only -// audit entries written after since are considered, so earlier entries in the -// same test run cannot leak into the result. func (ts *VerifyTestSuite) observeOutcome(w *httptest.ResponseRecorder, userID uuid.UUID, since time.Time) otpParityOutcome { outcome := otpParityOutcome{Status: w.Code} @@ -415,11 +402,11 @@ func phoneOTPBody(verifyType, phone string) map[string]interface{} { } // TestVerifyOTPParityIdentifierBinding covers the guarantee that the -// identifier in the request is the one the challenge was issued for. Legacy -// gets this from its identifier-keyed user lookups: the email or phone in the -// body is the lookup key, so a mismatch is a not-found. The one_time_tokens -// path finds the user by token hash, so it has to check the binding itself. -// Every case here is rejected by legacy; the one_time_tokens path must agree. +// identifier in the request is the one the challenge was issued for. +// +// The legacy path gets this from its identifier-keyed user lookups: the email or phone in the +// body is the lookup key, so a mismatch is a not-found. The one_time_tokens path finds the user +// by token hash, so it has to check the binding itself. func (ts *VerifyTestSuite) TestVerifyOTPParityIdentifierBinding() { now := time.Now() emailHash := crypto.GenerateTokenHash(parityEmail, parityOTP) @@ -433,51 +420,37 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityIdentifierBinding() { Phone: parityPhone, } - cases := []otpParityCase{ - { - // The client owns the phone and has an unconfirmed email on the - // same account. Presenting the SMS code as an email signup must - // not confirm the email, because no email was ever delivered. - desc: "a phone code posted as signup does not confirm the email", - seed: func(u *models.User) map[string]interface{} { + testCases := map[string]otpParityCase{ + "a phone code posted as signup does not confirm the email": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) - return phoneOTPBody(mail.SignupVerification, parityPhone) }, - expected: forbidden, + requestBody: phoneOTPBody(mail.SignupVerification, parityPhone), + expected: forbidden, }, - { - // supabase-js sends the generic "email" type for every email OTP. - // The generic type must not let a phone code through either. - desc: "a phone code posted as the generic email type does not confirm the email", - seed: func(u *models.User) map[string]interface{} { + "a phone code posted as the generic email type does not confirm the email": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) - return phoneOTPBody(mail.EmailOTPVerification, parityPhone) }, - expected: forbidden, + requestBody: phoneOTPBody(mail.EmailOTPVerification, parityPhone), + expected: forbidden, }, - { - // The mirror image: an email confirmation code presented as an - // SMS code must not confirm the phone. - desc: "an email code posted as sms does not confirm the phone", - seed: func(u *models.User) map[string]interface{} { + "an email code posted as sms does not confirm the phone": { + seed: func(u *models.User) { ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(smsVerification, parityEmail) }, - expected: forbidden, + requestBody: emailOTPBody(smsVerification, parityEmail), + expected: forbidden, }, - { - // Every legacy user lookup filters is_sso_user = false, so an - // SSO-managed account can never be signed in with a typed OTP. - // A challenge row for one must not change that. - desc: "an SSO user cannot verify a typed OTP", - seed: func(u *models.User) map[string]interface{} { + "an SSO user cannot verify a typed OTP": { + seed: func(u *models.User) { u.IsSSOUser = true ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) - return emailOTPBody(mail.SignupVerification, parityEmail) }, - expected: forbidden, + requestBody: emailOTPBody(mail.SignupVerification, parityEmail), + expected: forbidden, }, } - ts.runOTPParityCases(cases) + ts.runOTPParityCases(testCases) } From d4cfac698b1cb1dbfdcf58964e24afd6b260a3f4 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 15:46:13 -0400 Subject: [PATCH 12/14] chore: clean up tests and move things around --- internal/api/verify.go | 124 +--------------------------------- internal/api/verify_ott.go | 133 +++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 123 deletions(-) create mode 100644 internal/api/verify_ott.go diff --git a/internal/api/verify.go b/internal/api/verify.go index f53941841a..1009384d6f 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -701,7 +701,7 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, config := a.config if config.Experimental.EnableOTTAsSourceOfTruth { - return a.verifyUserAndTokenFromOTT(conn, params, aud) + return verifyUserAndTokenFromOTT(conn, params, aud) } var user *models.User @@ -785,128 +785,6 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, return user, nil } -// verifyUserAndTokenFromOTT is the EnableOTTAsSourceOfTruth path. It finds the -// challenge in the one_time_tokens table and derives the user from that row, -// instead of finding the user by identifier and comparing the users.*_token -// columns. A lookup miss is rejected as an expired or invalid token; there is -// no fallback to the users columns. -// -// NOTE: Test OTPs and Twilio Verify are not handled yet on this path; a follow-up PR will add them. -func (a *API) 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. - ott, err := a.verifyOneTimeToken(conn, params) - if err != nil { - return nil, err - } - - user, err := models.FindUserByID(conn, ott.UserID) - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) - } - - if err := validateUserForOTT(params, ott, user, aud); err != nil { - return nil, err - } - - if user.IsBanned() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") - } - return user, nil -} - -func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { - tokenTypes := verifyTypeToTokenTypes(params.Type) - if len(tokenTypes) == 0 { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("unknown verification type") - } - - ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, params.TokenHash, tokenTypes...) - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token not found") - } else if err != nil { - return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) - } - - if ott.IsExpired() { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token has expired") - } - - // The generic email type needs to match to the flow that issues the token, so the caller runs the right post-verify step - if params.Type == mail.EmailOTPVerification { - switch ott.TokenType { - case models.ConfirmationToken: - params.Type = mail.SignupVerification - case models.RecoveryToken: - params.Type = mail.MagicLinkVerification - } - } - - return ott, nil -} - -func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { - switch verifyType { - case mail.EmailOTPVerification: - return []models.OneTimeTokenType{models.ConfirmationToken, models.RecoveryToken} - case mail.SignupVerification, mail.InviteVerification: - return []models.OneTimeTokenType{models.ConfirmationToken} - case mail.RecoveryVerification, mail.MagicLinkVerification: - return []models.OneTimeTokenType{models.RecoveryToken} - case mail.EmailChangeVerification: - return []models.OneTimeTokenType{models.EmailChangeTokenCurrent, models.EmailChangeTokenNew} - case phoneChangeVerification: - return []models.OneTimeTokenType{models.PhoneChangeToken} - case smsVerification: - return []models.OneTimeTokenType{models.ConfirmationToken} - default: - return nil - } -} - -// validateUserForOTT checks that the user found from a one_time_tokens row is -// the one the request is entitled to act on. The legacy path gets these -// guarantees for free from its identifier-keyed lookups (which also filter on -// aud and is_sso_user), so a mismatch there is a not-found. The one_time_tokens -// path finds the user by token hash, so it has to check the binding itself. -func validateUserForOTT(params *VerifyParams, ott *models.OneTimeToken, user *models.User, aud string) error { - mismatch := apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid") - - if user.IsSSOUser { - return mismatch.WithInternalMessage("SSO users cannot be verified with one time tokens") - } - - if user.Aud != aud { - return mismatch.WithInternalMessage("user audience does not match") - } - - switch params.Type { - case smsVerification: - if params.Phone == "" || user.GetPhone() != params.Phone { - return mismatch.WithInternalMessage("user phone does not match") - } - case phoneChangeVerification: - if params.Phone == "" || user.PhoneChange != params.Phone { - return mismatch.WithInternalMessage("user phone does not match") - } - case mail.EmailChangeVerification: - expected := user.EmailChange - if ott.TokenType == models.EmailChangeTokenCurrent { - expected = user.GetEmail() - } - if params.Email == "" || !strings.EqualFold(expected, params.Email) { - return mismatch.WithInternalMessage("user email does not match") - } - default: // Signup, Invite, Recovery, MagicLink - if params.Email == "" || !strings.EqualFold(user.GetEmail(), params.Email) { - return mismatch.WithInternalMessage("user email does not match") - } - } - return nil -} - // isOtpValid checks the actual otp sent against the expected otp and ensures that it's within the valid window func isOtpValid(actual, expected string, sentAt *time.Time, otpExp uint) bool { if expected == "" || sentAt == nil { diff --git a/internal/api/verify_ott.go b/internal/api/verify_ott.go new file mode 100644 index 0000000000..d4eb7d4cc1 --- /dev/null +++ b/internal/api/verify_ott.go @@ -0,0 +1,133 @@ +package api + +import ( + "strings" + + "github.com/supabase/auth/internal/api/apierrors" + mail "github.com/supabase/auth/internal/mailer" + "github.com/supabase/auth/internal/models" + "github.com/supabase/auth/internal/storage" +) + +// verifyUserAndTokenFromOTT is the EnableOTTAsSourceOfTruth path. It finds the +// challenge in the one_time_tokens table and derives the user from that row, +// instead of finding the user by identifier and comparing the users.*_token +// 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) { + + // TODO AUTH-1553: Add support for test OTPs and Twilio Verify on this path. + ott, err := verifyOneTimeToken(conn, params) + if err != nil { + return nil, err + } + + user, err := models.FindUserByID(conn, ott.UserID) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + if err := validateUserForOTT(params, ott, user, aud); err != nil { + return nil, err + } + + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + return user, nil +} + +func verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { + tokenTypes := verifyTypeToTokenTypes(params.Type) + if len(tokenTypes) == 0 { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("unknown verification type") + } + + ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, params.TokenHash, tokenTypes...) + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token not found") + } else if err != nil { + return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) + } + + if ott.IsExpired() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token has expired") + } + + // The generic email type needs to match to the flow that issues the token, so the caller runs the right post-verify step + if params.Type == mail.EmailOTPVerification { + switch ott.TokenType { + case models.ConfirmationToken: + params.Type = mail.SignupVerification + case models.RecoveryToken: + params.Type = mail.MagicLinkVerification + } + } + + return ott, nil +} + +func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { + switch verifyType { + case mail.EmailOTPVerification: + return []models.OneTimeTokenType{models.ConfirmationToken, models.RecoveryToken} + case mail.SignupVerification, mail.InviteVerification: + return []models.OneTimeTokenType{models.ConfirmationToken} + case mail.RecoveryVerification, mail.MagicLinkVerification: + return []models.OneTimeTokenType{models.RecoveryToken} + case mail.EmailChangeVerification: + return []models.OneTimeTokenType{models.EmailChangeTokenCurrent, models.EmailChangeTokenNew} + case phoneChangeVerification: + return []models.OneTimeTokenType{models.PhoneChangeToken} + case smsVerification: + return []models.OneTimeTokenType{models.ConfirmationToken} + default: + return nil + } +} + +// validateUserForOTT checks that the user found from a one_time_tokens row is +// the one the request is entitled to act on. +// +// The legacy path gets these guarantees for free from its identifier-keyed lookups (which also filter on +// aud and is_sso_user), so a mismatch there is a not-found. +// +// The one_time_tokens path finds the user by token hash, so it has to check the binding itself. +func validateUserForOTT(params *VerifyParams, ott *models.OneTimeToken, user *models.User, aud string) error { + mismatch := apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid") + + if user.IsSSOUser { + return mismatch.WithInternalMessage("SSO users cannot be verified with one time tokens") + } + + if user.Aud != aud { + return mismatch.WithInternalMessage("user audience does not match") + } + + switch params.Type { + case smsVerification: + if params.Phone == "" || user.GetPhone() != params.Phone { + return mismatch.WithInternalMessage("user phone does not match") + } + case phoneChangeVerification: + if params.Phone == "" || user.PhoneChange != params.Phone { + return mismatch.WithInternalMessage("user phone does not match") + } + case mail.EmailChangeVerification: + expected := user.EmailChange + if ott.TokenType == models.EmailChangeTokenCurrent { + expected = user.GetEmail() + } + if params.Email == "" || !strings.EqualFold(expected, params.Email) { + return mismatch.WithInternalMessage("user email does not match") + } + default: // Signup, Invite, Recovery, MagicLink + if params.Email == "" || !strings.EqualFold(user.GetEmail(), params.Email) { + return mismatch.WithInternalMessage("user email does not match") + } + } + return nil +} From 7cdc4285adc83864f387f5db8a082b66f2a1e013 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 16:03:48 -0400 Subject: [PATCH 13/14] chore: refactor to make clearer --- internal/api/verify_ott.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/internal/api/verify_ott.go b/internal/api/verify_ott.go index d4eb7d4cc1..c5383366ed 100644 --- a/internal/api/verify_ott.go +++ b/internal/api/verify_ott.go @@ -23,6 +23,9 @@ func verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, a return nil, err } + // Resolve the generic email type to the flow that issued the token, so the caller runs the correct post-verification step. + params.Type = resolveEmailOTPType(params.Type, ott.TokenType) + user, err := models.FindUserByID(conn, ott.UserID) if models.IsNotFoundError(err) { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) @@ -57,16 +60,6 @@ func verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token has expired") } - // The generic email type needs to match to the flow that issues the token, so the caller runs the right post-verify step - if params.Type == mail.EmailOTPVerification { - switch ott.TokenType { - case models.ConfirmationToken: - params.Type = mail.SignupVerification - case models.RecoveryToken: - params.Type = mail.MagicLinkVerification - } - } - return ott, nil } @@ -89,6 +82,18 @@ func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { } } +func resolveEmailOTPType(verifyType string, tokenType models.OneTimeTokenType) string { + if verifyType == mail.EmailOTPVerification { + switch tokenType { + case models.ConfirmationToken: + return mail.SignupVerification + case models.RecoveryToken: + return mail.MagicLinkVerification + } + } + return verifyType +} + // validateUserForOTT checks that the user found from a one_time_tokens row is // the one the request is entitled to act on. // From 7cea4ca8746d7366e1fd7425b6036a81b78d6b54 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 16:29:01 -0400 Subject: [PATCH 14/14] chore: improve test coverage --- internal/api/verify_ott_parity_test.go | 157 ++++++++++++++++++++++--- 1 file changed, 139 insertions(+), 18 deletions(-) diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 88fa7da579..07d63e55eb 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -268,18 +268,11 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { // runOTPParityCases runs each case against both stores and asserts that both // produce the expected outcome and agree with each other. func (ts *VerifyTestSuite) runOTPParityCases(testCases map[string]otpParityCase) { - originalFlag := ts.Config.Experimental.EnableOTTAsSourceOfTruth - defer func() { ts.Config.Experimental.EnableOTTAsSourceOfTruth = originalFlag }() - + // Run order does not matter: every store run truncates and re-seeds. for name, tc := range testCases { ts.Run(name, func() { - var legacyOutcome, ottOutcome otpParityOutcome - - ts.Run("legacy users columns", func() { - legacyOutcome = ts.runOTPParityCase(tc, false) - }) - ts.Run("one_time_tokens", func() { - ottOutcome = ts.runOTPParityCase(tc, true) + legacyOutcome, ottOutcome := ts.runPerStore(func() otpParityOutcome { + return ts.runOTPParityCase(tc) }) require.Equal(ts.T(), legacyOutcome, ottOutcome, @@ -288,19 +281,35 @@ func (ts *VerifyTestSuite) runOTPParityCases(testCases map[string]otpParityCase) } } -// runOTPParityCase runs one case against one store and returns the outcome it -// observed. enableOTT selects the store. It also asserts the outcome matches -// c.expected, so a failure names the store that diverged. -func (ts *VerifyTestSuite) runOTPParityCase(c otpParityCase, enableOTT bool) otpParityOutcome { - ts.SetupTest() - ts.Config.Experimental.EnableOTTAsSourceOfTruth = enableOTT +// runPerStore runs run once per store, each in its own subtest with a truncated +// database, and returns what each store produced. Tests that need more than one +// request per store use this directly instead of the case table. +func (ts *VerifyTestSuite) runPerStore(run func() otpParityOutcome) (legacy, ott otpParityOutcome) { + originalFlag := ts.Config.Experimental.EnableOTTAsSourceOfTruth + defer func() { ts.Config.Experimental.EnableOTTAsSourceOfTruth = originalFlag }() + + ts.Run("legacy users columns", func() { + ts.SetupTest() + ts.Config.Experimental.EnableOTTAsSourceOfTruth = false + legacy = run() + }) + ts.Run("one_time_tokens", func() { + ts.SetupTest() + ts.Config.Experimental.EnableOTTAsSourceOfTruth = true + ott = run() + }) + return legacy, ott +} + +// runOTPParityCase arranges one case, sends its request, and asserts the +// outcome matches c.expected, so a failure names the store that diverged. +func (ts *VerifyTestSuite) runOTPParityCase(c otpParityCase) otpParityOutcome { if c.configure != nil { restore := c.configure() defer restore() } - u, err := models.FindUserByEmailAndAudience(ts.API.db, parityEmail, ts.Config.JWT.Aud) - require.NoError(ts.T(), err) + u := ts.parityUser() if c.seed != nil { c.seed(u) } @@ -312,6 +321,13 @@ func (ts *VerifyTestSuite) runOTPParityCase(c otpParityCase, enableOTT bool) otp return outcome } +// parityUser returns the fixture user SetupTest created. +func (ts *VerifyTestSuite) parityUser() *models.User { + u, err := models.FindUserByEmailAndAudience(ts.API.db, parityEmail, ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + return u +} + // saveUser persists every pending change on u. Call it from a seed that // changes u but stores no challenge. seedChallenge calls it already. func (ts *VerifyTestSuite) saveUser(u *models.User) { @@ -333,6 +349,9 @@ func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTim case models.EmailChangeTokenNew: u.EmailChangeTokenNew = hash u.EmailChangeSentAt = &sentAt + case models.EmailChangeTokenCurrent: + u.EmailChangeTokenCurrent = hash + u.EmailChangeSentAt = &sentAt case models.PhoneChangeToken: u.PhoneChangeToken = hash u.PhoneChangeSentAt = &sentAt @@ -385,6 +404,90 @@ func (ts *VerifyTestSuite) observeOutcome(w *httptest.ResponseRecorder, userID u return outcome } +func (ts *VerifyTestSuite) TestVerifyOTPParityCodeIsSingleUse() { + now := time.Now() + emailHash := crypto.GenerateTokenHash(parityEmail, parityOTP) + requestBody := emailOTPBody(mail.SignupVerification, parityEmail) + + legacyOutcome, ottOutcome := ts.runPerStore(func() otpParityOutcome { + u := ts.parityUser() + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + + first := ts.observeOutcome(ts.postVerify(requestBody), u.ID, now) + require.Equal(ts.T(), http.StatusOK, first.Status, "the first use must succeed") + require.True(ts.T(), first.EmailConfirmed) + + // The row must be gone/ + _, err := models.FindOneTimeToken(ts.API.db, emailHash, models.ConfirmationToken) + require.True(ts.T(), models.IsNotFoundError(err), + "the challenge row must be deleted on success, got %v", err) + + since := time.Now() + return ts.observeOutcome(ts.postVerify(requestBody), u.ID, since) + }) + + replayed := otpParityOutcome{ + Status: http.StatusForbidden, + ErrorCode: apierrors.ErrorCodeOTPExpired, + Msg: parityForbidden, + EmailConfirmed: true, + Email: parityEmail, + Phone: parityPhone, + } + require.Equal(ts.T(), replayed, legacyOutcome) + require.Equal(ts.T(), replayed, ottOutcome) +} + +// TestVerifyOTPParitySecureEmailChange covers the dual-confirmation flow +// where a code goes to both the old and the new address and both tokens must be redeemed in order to be fully verified. +func (ts *VerifyTestSuite) TestVerifyOTPParitySecureEmailChange() { + now := time.Now() + newHash := crypto.GenerateTokenHash(parityNewEmail, parityOTP) + currentHash := crypto.GenerateTokenHash(parityEmail, parityOTP) + + legacyOutcome, ottOutcome := ts.runPerStore(func() otpParityOutcome { + require.True(ts.T(), ts.Config.Mailer.SecureEmailChangeEnabled, + "this test covers the secure flow, which is the default") + + u := ts.parityUser() + u.EmailChange = parityNewEmail + ts.seedChallenge(u, models.EmailChangeTokenNew, parityNewEmail, newHash, now, time.Hour) + ts.seedChallenge(u, models.EmailChangeTokenCurrent, parityEmail, currentHash, now, time.Hour) + + w := ts.postVerify(emailOTPBody(mail.EmailChangeVerification, parityNewEmail)) + require.Equal(ts.T(), http.StatusOK, w.Code, "the new address code must be accepted") + require.Equal(ts.T(), singleConfirmationAccepted, ts.responseMsg(w)) + + pending := ts.parityUser() + require.Equal(ts.T(), singleConfirmation, pending.EmailChangeConfirmStatus) + require.Equal(ts.T(), parityEmail, pending.GetEmail(), + "one code must not be enough to move the address") + + since := time.Now() + return ts.observeOutcome( + ts.postVerify(emailOTPBody(mail.EmailChangeVerification, parityEmail)), u.ID, since) + }) + + changed := otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserModifiedAction), + EmailConfirmed: true, + Email: parityNewEmail, + Phone: parityPhone, + } + require.Equal(ts.T(), changed, legacyOutcome) + require.Equal(ts.T(), changed, ottOutcome) +} + +// responseMsg reads the msg field out of a response body. +func (ts *VerifyTestSuite) responseMsg(w *httptest.ResponseRecorder) string { + var body struct { + Msg string `json:"msg"` + } + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&body)) + return body.Msg +} + func emailOTPBody(verifyType, email string) map[string]interface{} { return map[string]interface{}{ "type": verifyType, @@ -450,6 +553,24 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityIdentifierBinding() { requestBody: emailOTPBody(mail.SignupVerification, parityEmail), expected: forbidden, }, + "a user in another audience cannot verify a typed OTP": { + seed: func(u *models.User) { + u.Aud = "other-audience" + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + }, + requestBody: emailOTPBody(mail.SignupVerification, parityEmail), + expected: forbidden, + }, + // The challenge exists, but the user has no pending change to that + // number, so nothing entitles the request to move the phone. + "a phone change code is rejected when the user has no pending change": { + seed: func(u *models.User) { + ts.seedChallenge(u, models.PhoneChangeToken, parityNewPhone, + crypto.GenerateTokenHash(parityNewPhone, parityOTP), now, time.Hour) + }, + requestBody: phoneOTPBody(phoneChangeVerification, parityNewPhone), + expected: forbidden, + }, } ts.runOTPParityCases(testCases)