Unverified Commit b6ef27cd authored by Lemon-miaow's avatar Lemon-miaow
Browse files

fix(auth): enforce the verified-email uniqueness that email login assumes

The design has claimed since migration 0010 that at most one account can hold
a PROVEN email address, with ErrEmailTaken as the 409 a second verifier sees.
Neither half ever shipped: no migration created users_verified_email_unique,
and VerifyEmailOTP had no guard at all — the sentinel was defined but never
returned, so two accounts could both verify one address. The damage is not
cosmetic: the pre-session login resolves accounts BY verified email, so the
duplicate decided which identity a mailed sign-in code belonged to.

- Migration 0020 creates the partial unique index (lower(email) WHERE
  email_verified) the comments have been citing — the database-level backstop.
- VerifyEmailOTP now refuses the take-over with ErrEmailTaken BEFORE consuming
  the code (the address, not the code, is the problem), charges no attempt,
  and maps a lost cross-user race (unique violation) to the same answer.
- The verify handler answers 409 email_taken instead of a generic 500.

Covered by the pgint suite (sequential double-verify refused with the code
still live, a direct duplicate write still loses to the index, the refused
account can still prove its own address) and a hermetic 409 case.
parent 2a55a0d2
Loading
Loading
Loading
Loading
+7 −0
Changes for internal/api/api_test.go: 7 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -354,6 +354,13 @@ func (f *fakeRepo) VerifyEmailOTP(_ context.Context, userID, purpose, codeHash s
		live.attempts++ // a typo costs an attempt but does not consume the code
		return "", ErrOTPInvalid
	}
	// A DIFFERENT verified holder of the same address → ErrEmailTaken, code left
	// live — mirrors PGRepo's guard + the users_verified_email_unique index.
	for _, u := range f.staff {
		if u.ID != userID && u.EmailVerified && strings.EqualFold(u.Email, live.email) {
			return "", ErrEmailTaken
		}
	}
	live.consumed = true
	for _, u := range f.staff { // flip the user row verified (UPDATE users ...)
		if u.ID == userID {
+2 −1
Changes for internal/api/errors.go: 2 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -51,7 +51,8 @@ var (
	// the handler answers 403 (wrong door) rather than 409 (already linked).
	ErrPlayerBindForbidden = errors.New("bind code belongs to a staff account")
	// ErrEmailTaken means a verified email would collide with another account's
	// already-verified address (spec §B email-first login foundation, migration 0010).
	// already-verified address (spec §B email-first login foundation; the
	// users_verified_email_unique index ships in migration 0020).
	// VerifyEmailOTP returns it — WITHOUT consuming the code, since the address, not
	// the code, is the problem — when a DIFFERENT user has already proven the same
	// address case-insensitively. It is the clean, application-level counterpart of
+9 −3
Changes for internal/api/handlers_email_otp.go: 9 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -187,9 +187,11 @@ type emailOTPVerifyRequest struct {

// handleEmailOTPVerify redeems a code for the caller (spec §B2, external app face).
// Outcomes mirror the link-verify shape: an invalid/expired/mismatched code → 400
// invalid_code, a locked code (too many wrong guesses) → 429 otp_locked, and on
// success the user's email is written and email_verified flips true. The verified
// address is echoed so the panel can render it.
// invalid_code, a locked code (too many wrong guesses) → 429 otp_locked, an address
// another account already proved → 409 email_taken (the code stays live — the
// address, not the code, is the problem), and on success the user's email is
// written and email_verified flips true. The verified address is echoed so the
// panel can render it.
func (a *API) handleEmailOTPVerify(w http.ResponseWriter, r *http.Request) {
	p := principalFromContext(r.Context())
	var req emailOTPVerifyRequest
@@ -211,6 +213,10 @@ func (a *API) handleEmailOTPVerify(w http.ResponseWriter, r *http.Request) {
	case errors.Is(err, ErrOTPInvalid):
		writeError(w, r, newError(http.StatusBadRequest, "invalid_code", "email code is invalid or expired"))
		return
	case errors.Is(err, ErrEmailTaken):
		writeError(w, r, newError(http.StatusConflict, "email_taken",
			"that email is already verified on another account; sign in with it or use another address"))
		return
	case err != nil:
		writeError(w, r, err)
		return
+14 −0
Changes for internal/api/handlers_email_otp_test.go: 14 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -339,6 +339,20 @@ func TestEmailOTPVerifyRejections(t *testing.T) {
			t.Fatalf("code = %d body %s, want 429 otp_locked", w.Code, w.Body.String())
		}
	})
	t.Run("address proven elsewhere -> 409 email_taken, not consumed", func(t *testing.T) {
		repo := newFakeRepo()
		live(repo, "tk", otpCodeHash("123456"), time.Unix(1_700_000_600, 0), 0)
		// Another account already proved the same address: login resolves accounts
		// BY verified email, so the second proof must be refused.
		repo.staff["other"] = &StaffUser{ID: "u2", Email: "[email protected]", EmailVerified: true}
		w := do(mk(repo), "POST", "/api/v1/account/email/verify", `{"code":"123456"}`, nil)
		if w.Code != http.StatusConflict || decodeErr(t, w) != "email_taken" {
			t.Fatalf("code = %d body %s, want 409 email_taken", w.Code, w.Body.String())
		}
		if repo.otps["tk"].consumed {
			t.Error("a taken address must not consume the code")
		}
	})
}

// TestEmailOTPBruteForceLockout drives the lockout end-to-end through the handler:
+26 −1
Changes for internal/api/pgrepo.go: 26 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -695,7 +695,9 @@ func (p *PGRepo) CreateEmailOTP(ctx context.Context, id, userID, email, codeHash
// never silently accepted, and a hash mismatch costs an attempt (UPDATE attempts+1)
// without consuming the code — a typo must not burn a still-valid code. On a match
// the code is consumed and the user row is flipped verified, returning the proven
// address. ErrOTPInvalid / ErrOTPLocked are the only domain errors.
// address — unless a DIFFERENT account already proved the same address, which is
// ErrEmailTaken with the code left unconsumed (the address, not the guess, is the
// problem). ErrOTPInvalid / ErrOTPLocked / ErrEmailTaken are the only domain errors.
func (p *PGRepo) VerifyEmailOTP(ctx context.Context, userID, purpose, codeHash string, now time.Time) (string, error) {
	tx, err := p.db.BeginTx(ctx, nil)
	if err != nil {
@@ -738,12 +740,35 @@ func (p *PGRepo) VerifyEmailOTP(ctx context.Context, userID, purpose, codeHash s
		return "", ErrOTPInvalid
	}

	// A DIFFERENT account may not also prove this address: the pre-session login
	// door resolves accounts BY verified email (UserByEmail), so a second verified
	// holder would make the identity ambiguous. The code is NOT consumed and no
	// attempt is charged — the address, not the guess, is the problem. The check is
	// the application-level counterpart of the users_verified_email_unique index
	// (migration 0020), which catches a cross-user race that passes this SELECT.
	var taken bool
	if err := tx.QueryRowContext(ctx,
		`SELECT EXISTS (SELECT 1 FROM users
		 WHERE lower(email) = lower($1) AND email_verified = true AND id <> $2)`,
		email, userID).Scan(&taken); err != nil {
		return "", fmt.Errorf("check verified-email uniqueness: %w", err)
	}
	if taken {
		return "", ErrEmailTaken
	}

	if _, err := tx.ExecContext(ctx,
		`UPDATE email_otps SET consumed_at = $2 WHERE id = $1`, id, now); err != nil {
		return "", fmt.Errorf("consume otp: %w", err)
	}
	if _, err := tx.ExecContext(ctx,
		`UPDATE users SET email = $2, email_verified = true WHERE id = $1`, userID, email); err != nil {
		// Lost the race the guarded SELECT above cannot serialise: the index rejects
		// the second write, and it reads as the same answer the sequential path gives.
		// The rollback undoes the OTP consumption with it, so the code stays live.
		if isUniqueViolation(err) {
			return "", ErrEmailTaken
		}
		return "", fmt.Errorf("mark email verified: %w", err)
	}
	if err := tx.Commit(); err != nil {
Loading