security(verify-email): one decision core, reject empty-subject token

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

internal/http_handlers/verify_email.go

@@ -47,108 +47,35 @@ 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")
 			c.JSON(http.StatusBadRequest, gin.H{"error": "invalid redirect uri"})
 			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

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)
+}