From 4fb943894e42608eedca60e9ba22eeb17dea532d Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 20:01:44 +0530 Subject: [PATCH] security(verify-email): one decision core, reject empty-subject token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-targets PR #752 onto main. That PR merged into security/2.4.0-audit-part-2 four minutes AFTER that branch had already merged to main, so it landed in a dead end: GitHub shows it merged, but its merge commit is not an ancestor of main and none of its content ever shipped. The GraphQL and REST verify-email paths had drifted apart twice, each time because the same decision was implemented in two places. Both now route through ConsumeEmailVerificationToken, which owns token lookup, JWT validation, purpose binding, subject handling, user lookup, the revoked check, and the email_verified_at write — the write happens before any caller gate, which is what the earlier bug got wrong. Also rejects a verification token with an empty subject. ValidateJWTClaims passes on an empty sub because User.ID is "" at that call site, so the token resolved to no principal instead of failing. Cherry-picked with one conflict, in the signup matrix test: main gained TestTOTPEnrollmentWorksForPhoneOnlyAccounts from #753 while this branch added the expired-link and empty-subject tests. All three are kept. --- docs/email-verification-contract.md | 38 ++++ internal/http_handlers/verify_email.go | 105 ++-------- .../signup_verification_matrix_test.go | 187 ++++++++++++++++++ internal/service/provider.go | 5 + internal/service/verify_email.go | 84 ++------ internal/service/verify_email_core.go | 162 +++++++++++++++ 6 files changed, 419 insertions(+), 162 deletions(-) create mode 100644 internal/service/verify_email_core.go diff --git a/docs/email-verification-contract.md b/docs/email-verification-contract.md index dda99b6ad..de8f5c00b 100644 --- a/docs/email-verification-contract.md +++ b/docs/email-verification-contract.md @@ -236,6 +236,44 @@ Two fixes make the table above actually hold: applied further down, so a call setting only `email_verified` was rejected unless padded with an unrelated field. +### How long a verification link is valid + +**30 minutes.** `CreateVerificationToken` mints the JWT with `exp` 30 minutes out +(`internal/token/verification_token.go`), and the stored row carries a matching +`expires_at`. + +Expiry is enforced by **JWT validation**, not by the row: redemption parses and +validates the token before anything else, so an expired link is refused even if +the row is still present. The `expires_at` column is used elsewhere — the login +path reads it to decide whether a *pending* verification should block a sign-in +or be cleared and replaced. + +A link also stops working **before** its 30 minutes if a newer one is issued: +each request rotates the nonce, and redemption checks the token's nonce against +the stored row. So the most recent link is always the only valid one — requesting +a fresh link invalidates the previous one immediately. + +### The link expired — what now + +Nothing here needs an administrator. + +1. **Request a new link.** `resend_verify_email` with the same `identifier` + (`basic_auth_signup` for a normal signup) mints a fresh request and mails it. + This works whether or not a pending request still exists — it will create one + if the old row is gone. +2. **Or just log in with your password.** An unverified account's password login + emails a one-time code instead of a session; entering that code verifies the + address (`verify_otp.go`). This is why an expired link is rarely noticed by + password users. +3. **Operator fallback.** An admin can force-verify from the dashboard + (**Mark Email Verified**) or via `_update_user { email_verified: true }`. + Prefer **Resend Verification Email** where possible — it has the user prove + control rather than asserting it on their behalf. + +The response to a resend is deliberately generic ("if a verification is pending +…") and identical whether or not the address exists, so the endpoint cannot be +used to test which addresses are registered. + ### Hard requirement: email verification needs a working email service `--enable-email-verification=true` with no SMTP configured is now a **fatal diff --git a/internal/http_handlers/verify_email.go b/internal/http_handlers/verify_email.go index 249afad14..792057394 100644 --- a/internal/http_handlers/verify_email.go +++ b/internal/http_handlers/verify_email.go @@ -47,70 +47,28 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { return } - verificationRequest, err := h.StorageProvider.GetVerificationRequestByToken(c, tokenInQuery) + // Decision logic is shared with the GraphQL/gRPC mutation via + // service.ConsumeEmailVerificationToken — token validation, purpose + // binding, user lookup, the revoked check, and the email_verified_at + // write, in that order. This handler previously reimplemented all of it + // and drifted twice; see that function's comment. Only presentation + // stays here (redirect vs AuthResponse). + verified, err := h.ServiceProvider.ConsumeEmailVerificationToken(c, hostname, tokenInQuery) if err != nil { - log.Debug().Err(err).Msg("Error getting verification request") + log.Debug().Err(err).Msg("Failed to consume verification token") errorRes["error"] = err.Error() utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) return } - - // verify if token exists in db - claim, err := h.TokenProvider.ParseJWTToken(tokenInQuery) - if err != nil { - log.Debug().Err(err).Msg("Error parsing jwt token") - errorRes["error"] = err.Error() - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } - // // hostname, verificationRequest.Nonce, verificationRequest.Email - if ok, err := h.TokenProvider.ValidateJWTClaims(claim, &token.AuthTokenConfig{ - HostName: hostname, - Nonce: verificationRequest.Nonce, - User: &schemas.User{ - Email: refs.NewStringRef(verificationRequest.Email), - }, - }); !ok || err != nil { - log.Debug().Err(err).Msg("Error validating jwt token") - errorRes["error"] = err.Error() - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } - - // Purpose binding: only the email-verification family may complete here. - // Every flow's token lives in one `verification_requests` table keyed by - // the token string alone, so without this a forgot-password token — the - // one credential this endpoint was never meant to see — redeems for a - // full session AND marks the address verified. Same reason the GraphQL - // mutation gates it (service.VerifyEmail); this handler is a separate - // implementation of the same flow and needs the same gate. Generic error - // so it is not an oracle for which flow a leaked token belongs to. - if !service.IsVerifyEmailPurpose(verificationRequest, claim) { - log.Debug().Str("identifier", verificationRequest.Identifier).Msg("Verification token used for the wrong purpose") - errorRes["error"] = "invalid verification token" - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } - - user, err := h.StorageProvider.GetUserByEmail(c, verificationRequest.Email) - if err != nil { - log.Debug().Err(err).Msg("Error getting user by email") - errorRes["error"] = err.Error() - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } - - if user.RevokedTimestamp != nil { - log.Debug().Msg("User access has been revoked") - errorRes["error"] = "user access has been revoked" - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } + user := verified.User + verificationRequest := verified.Request + isSignUp := verified.IsSignUp + loginMethod := verified.LoginMethod // Resolved once, early: needed both for the MFA-gate-withheld redirect // below and the success redirect further down. if redirectURL == "" { - redirectURL = claimString(claim, "redirect_uri") + redirectURL = verified.RedirectURI } if !validators.IsValidRedirectURI(redirectURL, h.Config.AllowedOrigins, hostname) { log.Debug().Msg("Invalid redirect URI in token claim") @@ -118,37 +76,6 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { return } - // Record the address as verified HERE, BEFORE the MFA gate below. - // - // The gate's withheld branch redirects to MFA setup and returns, so the - // write that used to sit after it never ran for the overwhelmingly - // common case: a fresh signup clicking its verification link, with MFA - // on by default. The user did everything right and their address stayed - // unverified forever. - // - // Only passkey login surfaces it — webauthn.go refuses outright on - // email_verified_at == nil, with an error the user cannot act on. - // Password, TOTP and email/SMS-OTP logins never check the column, so - // they appear to "work" while the account is in exactly the same broken - // state. That asymmetry is why this hid. - // - // Clicking the link IS the proof of mailbox control; whether MFA then - // interrupts token issuance is a separate question and must not discard - // it. Mirrors service.VerifyEmail, which is the GraphQL twin of this - // handler — the two implementations have to agree. - emailJustVerified := user.EmailVerifiedAt == nil - if emailJustVerified { - now := time.Now().Unix() - user.EmailVerifiedAt = &now - user, err = h.StorageProvider.UpdateUser(c, user) - if err != nil { - log.Debug().Err(err).Msg("Error updating user") - errorRes["error"] = err.Error() - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } - } - // MFA gate: this REST endpoint is what the emailed verification/magic // link literally points to, so it must enforce the same gate every // other login entry point does (login.go/signup.go/oauth_callback.go). @@ -177,8 +104,6 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { return } - // Set above, before the MFA gate — see the comment there. - isSignUp := emailJustVerified // delete from verification table if err := h.StorageProvider.DeleteVerificationRequest(c, verificationRequest); err != nil { log.Debug().Err(err).Msg("Error deleting verification request") @@ -207,10 +132,6 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { } else { scope = strings.Split(scopeString, " ") } - loginMethod := constants.AuthRecipeMethodBasicAuth - if verificationRequest.Identifier == constants.VerificationTypeMagicLinkLogin { - loginMethod = constants.AuthRecipeMethodMagicLinkLogin - } code := "" // Not required as /oauth/token cannot be resumed from other tab diff --git a/internal/integration_tests/signup_verification_matrix_test.go b/internal/integration_tests/signup_verification_matrix_test.go index 0c961fee8..481f7640e 100644 --- a/internal/integration_tests/signup_verification_matrix_test.go +++ b/internal/integration_tests/signup_verification_matrix_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -16,6 +17,7 @@ import ( "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/storage/schemas" + "github.com/authorizerdev/authorizer/internal/token" ) // Signup × email-verification matrix. @@ -469,3 +471,188 @@ func TestTOTPEnrollmentWorksForPhoneOnlyAccounts(t *testing.T) { }) } } + +// TestExpiredVerificationLinkIsRefusedAndRecoverable covers the case a real user +// hits most often: they did not click the link within 30 minutes. +// +// Two things must both hold, and only the pair is useful. The stale link must be +// refused — it is a capability, and an expired one that still works is just a +// long-lived one. And the user must be able to get a fresh link WITHOUT an +// admin, or "your link expired" is a dead end. +// +// The 30 minutes comes from CreateVerificationToken's `exp` claim +// (internal/token/verification_token.go); expiry is enforced by JWT validation +// inside ConsumeEmailVerificationToken, not by the row's expires_at column. +func TestExpiredVerificationLinkIsRefusedAndRecoverable(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + cfg.DisableMFA = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "expired_link_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + original, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + // Mint a token that is already past its exp, and swap it onto the stored + // row so the row and the token agree — otherwise the lookup fails first and + // the test would pass without ever exercising expiry. + expiredToken, err := ts.TokenProvider.CreateVerificationToken(&token.AuthTokenConfig{ + User: &schemas.User{Email: &email}, + Nonce: original.Nonce, + HostName: testAuthorizerHost(ts), + LoginMethod: constants.AuthRecipeMethodBasicAuth, + }, original.RedirectURI, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + expiredRow := *original + expiredRow.Token = expiredToken + expiredRow.ExpiresAt = time.Now().Add(-1 * time.Hour).Unix() + require.NoError(t, ts.StorageProvider.DeleteVerificationRequest(ctx, original)) + _, err = ts.StorageProvider.AddVerificationRequest(ctx, &expiredRow) + require.NoError(t, err) + + t.Run("an expired link is refused", func(t *testing.T) { + // A genuinely expired token, signed with exp in the past. Built from + // claims directly rather than CreateVerificationToken, which hardcodes + // exp to +30m — there is no way to test expiry through that helper + // without actually waiting, and a test that waits 30 minutes is a test + // nobody runs. + expiredClaims := jwt.MapClaims{ + "iss": testAuthorizerHost(ts), + "aud": ts.Config.ClientID, + "sub": email, + "exp": time.Now().Add(-1 * time.Minute).Unix(), + "iat": time.Now().Add(-31 * time.Minute).Unix(), + "token_type": constants.VerificationTypeBasicAuthSignup, + "nonce": expiredRow.Nonce, + "redirect_uri": expiredRow.RedirectURI, + } + signed, sErr := ts.TokenProvider.SignJWTToken(expiredClaims) + require.NoError(t, sErr) + + // Put it on the stored row so the lookup succeeds and the request + // genuinely reaches the expiry check, rather than failing earlier as an + // unknown token — which would pass for the wrong reason. + row := expiredRow + row.Token = signed + require.NoError(t, ts.StorageProvider.DeleteVerificationRequest(ctx, &expiredRow)) + _, aErr := ts.StorageProvider.AddVerificationRequest(ctx, &row) + require.NoError(t, aErr) + expiredRow = row + + _, err := ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: signed}) + require.Error(t, err, "an expired verification link must not be redeemable — it is a capability, and an expired one that still works is just a long-lived one") + assert.Contains(t, err.Error(), "invalid verification token") + + user, uErr := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, uErr) + assert.Nil(t, user.EmailVerifiedAt, "a refused link must not verify the address") + }) + + t.Run("a stale link whose nonce no longer matches is refused", func(t *testing.T) { + // This is what an expired-then-resent link looks like in practice: the + // resend rotates the nonce, so the OLD link stops validating even before + // its exp. Same refusal path, and testable without waiting 30 minutes. + _, err := ts.GraphQLProvider.ResendVerifyEmail(ctx, &model.ResendVerifyEmailRequest{ + Email: email, + Identifier: constants.VerificationTypeBasicAuthSignup, + }) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: expiredRow.Token}) + require.Error(t, err, "the superseded link must stop working once a new one is issued") + assert.Contains(t, err.Error(), "invalid verification token") + + user, uErr := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, uErr) + assert.Nil(t, user.EmailVerifiedAt, "a refused link must not verify the address") + }) + + t.Run("the resent link works, so the user is never stuck", func(t *testing.T) { + fresh, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + require.NotEqual(t, expiredRow.Token, fresh.Token) + + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: fresh.Token}) + require.NoError(t, err) + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.NotNil(t, user.EmailVerifiedAt, "the recovery path must actually complete verification") + }) +} + +// TestVerificationTokenWithEmptySubjectIsRefused pins a guard that does not +// depend on anything else being right. +// +// ValidateJWTClaims checks `sub` as +// +// claims["sub"] != cfg.User.ID && claims["sub"] != cfg.User.Email +// +// and the verification path sets only Email, leaving User.ID as "". A token +// whose `sub` is the empty STRING therefore satisfies the first comparison and +// passes that check. Whether it then does damage depends on whether the storage +// backend keeps a missing email as NULL or as "" — SQL keeps it NULL, so the +// lookup finds nobody, but that is an accident of one backend's representation +// across six implementations, not a declared property. +// +// So the subject is rejected outright. If this ever fails, an empty-subject +// token is reaching account selection and the only thing standing between it +// and a real account is a per-backend NULL convention. +func TestVerificationTokenWithEmptySubjectIsRefused(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "empty_sub_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + row, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + // Everything valid except the subject, which is empty. + forged, err := ts.TokenProvider.SignJWTToken(jwt.MapClaims{ + "iss": testAuthorizerHost(ts), + "aud": ts.Config.ClientID, + "sub": "", + "exp": time.Now().Add(10 * time.Minute).Unix(), + "iat": time.Now().Unix(), + "token_type": constants.VerificationTypeBasicAuthSignup, + "nonce": row.Nonce, + "redirect_uri": row.RedirectURI, + }) + require.NoError(t, err) + + // Attach it to the stored row so the lookup succeeds and the request really + // reaches subject handling instead of failing earlier as an unknown token. + forgedRow := *row + forgedRow.Token = forged + require.NoError(t, ts.StorageProvider.DeleteVerificationRequest(ctx, row)) + _, err = ts.StorageProvider.AddVerificationRequest(ctx, &forgedRow) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: forged}) + require.Error(t, err, "an empty subject must never be used to select an account") + assert.Contains(t, err.Error(), "invalid verification token") + + // And nothing was verified as a side effect. + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.Nil(t, user.EmailVerifiedAt) +} diff --git a/internal/service/provider.go b/internal/service/provider.go index 49d7486c0..519510f4d 100644 --- a/internal/service/provider.go +++ b/internal/service/provider.go @@ -170,6 +170,11 @@ type Provider interface { // VerifyEmail completes email verification and logs the user in. Browser // callers get a session cookie via side-effects. Public. VerifyEmail(ctx context.Context, meta RequestMetadata, params *model.VerifyEmailRequest) (*model.AuthResponse, *ResponseSideEffects, error) + // ConsumeEmailVerificationToken is the shared decision core behind both + // implementations of email verification (the GraphQL/gRPC mutation and the + // REST handler behind GET /verify_email). See verify_email_core.go for why + // it is shared rather than duplicated. + ConsumeEmailVerificationToken(ctx context.Context, hostname, rawToken string) (*EmailVerification, error) // VerifyOTP validates an email/SMS OTP or TOTP/recovery code and logs the // user in. Browser callers get a session cookie via side-effects. Public. diff --git a/internal/service/verify_email.go b/internal/service/verify_email.go index 7858972af..ad0fdacf4 100644 --- a/internal/service/verify_email.go +++ b/internal/service/verify_email.go @@ -29,50 +29,21 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params log := p.Log.With().Str("func", "VerifyEmail").Logger() side := &ResponseSideEffects{} - verificationRequest, err := p.StorageProvider.GetVerificationRequestByToken(ctx, params.Token) - if err != nil { - log.Debug().Err(err).Msg("failed GetVerificationRequestByToken") - return nil, nil, InvalidArgument(`invalid verification token`) - } - - // verify if token exists in db + // Decision logic lives in ConsumeEmailVerificationToken so the REST handler + // behind GET /verify_email runs exactly the same checks — see + // verify_email_core.go for the two drifts that motivated sharing it. Only + // the shared PREFIX moved: every MFA branch below stays here, because the + // two callers diverge from this point on (this one returns AuthResponse + // screens, the handler redirects). hostname := meta.HostURL - claim, err := p.TokenProvider.ParseJWTToken(params.Token) - if err != nil { - log.Debug().Err(err).Msg("Failed to parse jwt token") - return nil, nil, InvalidArgument(`invalid verification token`) - } - - if ok, err := p.TokenProvider.ValidateJWTClaims(claim, &token.AuthTokenConfig{ - HostName: hostname, - Nonce: verificationRequest.Nonce, - User: &schemas.User{ - Email: &verificationRequest.Email, - }, - }); !ok || err != nil { - log.Debug().Err(err).Msg("Failed to validate jwt claims") - return nil, nil, InvalidArgument(`invalid verification token`) - } - - // Purpose binding: only the email-verification family may complete here. A - // forgot-password token must not be redeemable for a session. - if !IsVerifyEmailPurpose(verificationRequest, claim) { - log.Debug().Str("identifier", verificationRequest.Identifier).Msg("Verification token used for the wrong purpose") - return nil, nil, InvalidArgument(`invalid verification token`) - } - - email := claim["sub"].(string) - log.Debug().Str("email", email).Msg("Email verified successfully") - user, err := p.StorageProvider.GetUserByEmail(ctx, email) + verified, err := p.ConsumeEmailVerificationToken(ctx, hostname, params.Token) if err != nil { - log.Debug().Err(err).Msg("failed GetUserByEmail") return nil, nil, err } - - if user.RevokedTimestamp != nil { - log.Debug().Msg("User access has been revoked") - return nil, nil, FailedPrecondition("user access has been revoked") - } + user := verified.User + verificationRequest := verified.Request + loginMethod := verified.LoginMethod + emailJustVerified := verified.IsSignUp // A single check protecting every MFA branch below, mirroring login.go — // lockout is set only by explicit user action (lock_mfa), never inferred @@ -83,11 +54,6 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params return nil, nil, FailedPrecondition("your account's multi-factor authentication is locked; contact your administrator to regain access") } - loginMethod := constants.AuthRecipeMethodBasicAuth - if verificationRequest.Identifier == constants.VerificationTypeMagicLinkLogin { - loginMethod = constants.AuthRecipeMethodMagicLinkLogin - } - isTOTPLoginEnabled := p.Config.EnableTOTPLogin isMFAEnabled := p.Config.EnableMFA @@ -154,31 +120,9 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params }, side, nil } - // Record the email as verified HERE, before the MFA gate below, because - // every branch of that gate can return early — and MFA is on by default - // (TOTP needs no external provider, so config.Finalize derives - // EnableMFA=true). A fresh signup clicking its verification link therefore - // lands on the MFA setup screen and used to return with email_verified - // still nil: the user completed setup, got a session, and their address was - // never marked verified. Anything gating on it later refused them - // permanently — passkey login says "email is not verified. please verify - // your email before signing in with a passkey" to a user who did exactly - // that, with no way to fix it. - // - // Clicking the link IS the proof of mailbox control. Whether MFA then - // interrupts session issuance is a separate question and must not discard - // the proof. The verification request itself is still consumed below, on - // the path that completes. - emailJustVerified := user.EmailVerifiedAt == nil - if emailJustVerified { - now := time.Now().Unix() - user.EmailVerifiedAt = &now - user, err = p.StorageProvider.UpdateUser(ctx, user) - if err != nil { - log.Debug().Err(err).Msg("failed UpdateUser") - return nil, nil, err - } - } + // The address was already marked verified by ConsumeEmailVerificationToken, + // before any of the MFA branches above could return early — see the + // ORDER IS LOAD-BEARING note there. // Gate runs whenever MFA applies at all, exactly like login.go/signup.go — // this used to be an ad-hoc TOTP-only check (refs.BoolValue(user.IsMultiFactorAuthEnabled) diff --git a/internal/service/verify_email_core.go b/internal/service/verify_email_core.go new file mode 100644 index 000000000..3db16baa1 --- /dev/null +++ b/internal/service/verify_email_core.go @@ -0,0 +1,162 @@ +package service + +import ( + "context" + "strings" + "time" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/refs" + "github.com/authorizerdev/authorizer/internal/storage/schemas" + "github.com/authorizerdev/authorizer/internal/token" +) + +// EmailVerification is the outcome of redeeming a verification token: who the +// principal is, which flow the token belonged to, and whether this redemption is +// the one that verified the address. +type EmailVerification struct { + User *schemas.User + // Request is the consumed verification row. The caller deletes it once it + // has finished with it — deletion is deliberately NOT done here, because the + // two callers finish at different points. + Request *schemas.VerificationRequest + // LoginMethod is basic_auth, or magic_link_login when the token came from a + // magic link. + LoginMethod string + // IsSignUp reports whether THIS redemption flipped the address to verified, + // which is what the callers key their signup-vs-login webhook on. + IsSignUp bool + // RedirectURI is the `redirect_uri` claim the token was minted with. The + // REST handler falls back to it when the query string carries none; it is + // returned here so the claim never has to leave this function. + RedirectURI string +} + +// ConsumeEmailVerificationToken is the single source of truth for what it means +// to redeem an email-verification token. +// +// It exists because there are two implementations of this flow over the same +// token table, and they have now drifted twice: +// +// - the MFA gate was added to the GraphQL mutation and missed the REST +// handler, so the emailed link bypassed MFA entirely (noted in that +// handler's own comment); +// - the email_verified_at write was moved above the MFA gate in the mutation +// and missed the REST handler, so users who clicked the emailed button were +// never marked verified — surfacing only on passkey login, because that is +// the sole login path that checks the column. Password login checks it too +// but silently self-heals via an email-OTP detour, which is why TOTP and +// email-OTP users appeared unaffected. +// +// GET /verify_email is what the button in the email literally points to, so the +// REST handler is the path essentially every real user takes — and it was the +// copy that kept missing fixes. GraphQL and gRPC both already delegate to the +// service; this pulls the REST handler onto the same decision logic so a third +// divergence cannot happen. +// +// What stays with the callers is presentation, which legitimately differs: the +// mutation returns an AuthResponse, the handler redirects with tokens in the +// query string. What lives here is every decision that must be identical. +// +// ORDER IS LOAD-BEARING. The address is marked verified BEFORE this returns, so +// that a caller which then withholds tokens — the MFA gate redirecting to setup +// — cannot lose the fact that the user proved control of their mailbox. +// Clicking the link is the proof; whether MFA interrupts token issuance +// afterwards is a separate question. +func (p *provider) ConsumeEmailVerificationToken(ctx context.Context, hostname, rawToken string) (*EmailVerification, error) { + log := p.Log.With().Str("func", "ConsumeEmailVerificationToken").Logger() + + verificationRequest, err := p.StorageProvider.GetVerificationRequestByToken(ctx, rawToken) + if err != nil { + log.Debug().Err(err).Msg("failed GetVerificationRequestByToken") + return nil, InvalidArgument(`invalid verification token`) + } + + claim, err := p.TokenProvider.ParseJWTToken(rawToken) + if err != nil { + log.Debug().Err(err).Msg("Failed to parse jwt token") + return nil, InvalidArgument(`invalid verification token`) + } + + if ok, err := p.TokenProvider.ValidateJWTClaims(claim, &token.AuthTokenConfig{ + HostName: hostname, + Nonce: verificationRequest.Nonce, + User: &schemas.User{ + Email: &verificationRequest.Email, + }, + }); !ok || err != nil { + log.Debug().Err(err).Msg("Failed to validate jwt claims") + return nil, InvalidArgument(`invalid verification token`) + } + + // Purpose binding: only the email-verification family completes here. A + // forgot-password token redeemed at this endpoint would otherwise hand out a + // full session AND mark the address verified. Generic error so it is not an + // oracle for which flow a leaked token belongs to. + if !IsVerifyEmailPurpose(verificationRequest, claim) { + log.Debug().Str("identifier", verificationRequest.Identifier).Msg("Verification token used for the wrong purpose") + return nil, InvalidArgument(`invalid verification token`) + } + + // `sub` must be a non-empty string before it is used to select an account. + // + // ValidateJWTClaims above does NOT guarantee that. Its subject check is + // + // claims["sub"] != cfg.User.ID && claims["sub"] != cfg.User.Email + // + // and this call site sets only Email, leaving User.ID as "". A token whose + // `sub` is the empty STRING therefore satisfies the first comparison and the + // whole check passes. The lookup would then run as GetUserByEmail(ctx, ""), + // whose result depends entirely on whether a given backend stores a missing + // email as NULL or as "" — SQL keeps it NULL today, so nothing matches, but + // that is an accident of one backend's representation and not a property + // anyone declared. Six storage backends is too many places for a security + // boundary to be implicit. + // + // Rejected explicitly so this does not depend on the empty-User.ID quirk + // above, nor on NULL-vs-empty-string behaviour below. + email, _ := claim["sub"].(string) + if strings.TrimSpace(email) == "" { + log.Debug().Msg("verification token has no subject") + return nil, InvalidArgument(`invalid verification token`) + } + + user, err := p.StorageProvider.GetUserByEmail(ctx, email) + if err != nil { + log.Debug().Err(err).Msg("failed GetUserByEmail") + return nil, err + } + + if user.RevokedTimestamp != nil { + log.Debug().Msg("User access has been revoked") + return nil, FailedPrecondition("user access has been revoked") + } + + loginMethod := constants.AuthRecipeMethodBasicAuth + if verificationRequest.Identifier == constants.VerificationTypeMagicLinkLogin { + loginMethod = constants.AuthRecipeMethodMagicLinkLogin + } + + // See the ORDER IS LOAD-BEARING note above. + isSignUp := user.EmailVerifiedAt == nil + if isSignUp { + now := time.Now().Unix() + user.EmailVerifiedAt = &now + user, err = p.StorageProvider.UpdateUser(ctx, user) + if err != nil { + log.Debug().Err(err).Msg("failed UpdateUser") + return nil, err + } + } + + redirectURI, _ := claim["redirect_uri"].(string) + + log.Debug().Str("email", refs.StringValue(user.Email)).Msg("Email verified successfully") + return &EmailVerification{ + User: user, + Request: verificationRequest, + LoginMethod: loginMethod, + IsSignUp: isSignUp, + RedirectURI: redirectURI, + }, nil +}