From b6ef27cd2d3cb982b2fee0d7682aa7c48a7fae0d Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 03:19:35 +0800 Subject: [PATCH] fix(auth): enforce the verified-email uniqueness that email login assumes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/api/api_test.go | 7 ++++ internal/api/errors.go | 3 +- internal/api/handlers_email_otp.go | 12 ++++-- internal/api/handlers_email_otp_test.go | 14 +++++++ internal/api/pgrepo.go | 27 +++++++++++- internal/api/repo.go | 7 +++- internal/pgint/pgint_test.go | 42 +++++++++++++++++++ .../migrations/0020_verified_email_unique.sql | 15 +++++++ 8 files changed, 120 insertions(+), 7 deletions(-) create mode 100644 internal/store/migrations/0020_verified_email_unique.sql diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 4c0b27a..4927820 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -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 { diff --git a/internal/api/errors.go b/internal/api/errors.go index a79a0f3..20b81ce 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -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 diff --git a/internal/api/handlers_email_otp.go b/internal/api/handlers_email_otp.go index f1571c9..dfc2fbd 100644 --- a/internal/api/handlers_email_otp.go +++ b/internal/api/handlers_email_otp.go @@ -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 diff --git a/internal/api/handlers_email_otp_test.go b/internal/api/handlers_email_otp_test.go index 891da6f..1d8fc1d 100644 --- a/internal/api/handlers_email_otp_test.go +++ b/internal/api/handlers_email_otp_test.go @@ -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: "player@example.net", 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: diff --git a/internal/api/pgrepo.go b/internal/api/pgrepo.go index 22c54b9..add5506 100644 --- a/internal/api/pgrepo.go +++ b/internal/api/pgrepo.go @@ -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 { diff --git a/internal/api/repo.go b/internal/api/repo.go index c70e3db..bd45c08 100644 --- a/internal/api/repo.go +++ b/internal/api/repo.go @@ -309,7 +309,10 @@ type Repo interface { // mismatch increments attempts and returns ErrOTPInvalid WITHOUT consuming the // code (so a typo does not burn it). On a match the code is consumed and the // user row is flipped to email=, email_verified=true; the - // proven email is returned. now is the API clock so expiry is testable. + // proven email is returned — unless a DIFFERENT account has already proven the + // same address, which is ErrEmailTaken with the code left unconsumed (the + // address, not the code, is the problem). now is the API clock so expiry is + // testable. // // This is the ONBOARDING primitive: verifying the code is the moment the address // becomes proven, so the write is load-bearing. The pre-session LOGIN door must @@ -490,7 +493,7 @@ type Repo interface { // (email_verified true), so a merely-asserted or unverified address never // resolves to a session-mintable identity — an attacker cannot claim someone // else's login by typing their email. Matching is on lower(email) to align with - // the users_verified_email_unique partial index (migration 0010), which + // the users_verified_email_unique partial index (migration 0020), which // guarantees at most one verified row per normalized address, so the result is // unambiguous. A player (role='user') row resolves too — email-first login is // passwordless and role-agnostic here; the door that consumes this result decides diff --git a/internal/pgint/pgint_test.go b/internal/pgint/pgint_test.go index 58dc64f..550e036 100644 --- a/internal/pgint/pgint_test.go +++ b/internal/pgint/pgint_test.go @@ -231,6 +231,48 @@ func TestOnboardingEmailOTPContract(t *testing.T) { assertConsumed(t, u.ID, purpose, false) } +// The verified-email uniqueness guard: two accounts cannot prove the same +// address (the login door resolves accounts BY verified email, so a duplicate +// would make identity ambiguous). This is the guard ConsumeLoginEmailOTP +// deliberately skips. +func TestOnboardingEmailOTPRejectsTakenEmail(t *testing.T) { + ctx := context.Background() + a, b := newUser(t, "user", "take-a"), newUser(t, "user", "take-b") + addr := "shared-" + suffix(t) + "@example.net" + now := mustNow() + purpose := "onboard_email" + + if err := repo.CreateEmailOTP(ctx, "tka-"+suffix(t), a.ID, addr, "h-a", purpose, now.Add(5*time.Minute)); err != nil { + t.Fatalf("CreateEmailOTP(a): %v", err) + } + if _, err := repo.VerifyEmailOTP(ctx, a.ID, purpose, "h-a", now); err != nil { + t.Fatalf("verify a: %v", err) + } + if err := repo.CreateEmailOTP(ctx, "tkb-"+suffix(t), b.ID, addr, "h-b", purpose, now.Add(5*time.Minute)); err != nil { + t.Fatalf("CreateEmailOTP(b): %v", err) + } + if _, err := repo.VerifyEmailOTP(ctx, b.ID, purpose, "h-b", now); !errors.Is(err, api.ErrEmailTaken) { + t.Fatalf("second account proving a taken email = %v, want ErrEmailTaken", err) + } + // The address, not the code, was the problem: b's code stays live. + assertConsumed(t, b.ID, purpose, false) + // The invariant is also enforced by the database, not only the app guard: a + // direct write that bypasses VerifyEmailOTP still loses (uppercased to prove + // the index keys on lower(email)). + if _, err := db.ExecContext(ctx, + `UPDATE users SET email = $2, email_verified = true WHERE id = $1`, b.ID, strings.ToUpper(addr)); err == nil { + t.Fatal("a direct duplicate verified-email write succeeded; users_verified_email_unique is missing") + } + // And the refusal does not wedge b: its OWN address still verifies fine. + own := "own-" + suffix(t) + "@example.net" + if err := repo.CreateEmailOTP(ctx, "tkb2-"+suffix(t), b.ID, own, "h-b2", purpose, now.Add(5*time.Minute)); err != nil { + t.Fatalf("CreateEmailOTP(b, own): %v", err) + } + if _, err := repo.VerifyEmailOTP(ctx, b.ID, purpose, "h-b2", now); err != nil { + t.Fatalf("b must still be able to prove its own address: %v", err) + } +} + // ---- email OTP: pre-session login primitive (regression: the #16 drift) -------- func TestConsumeLoginEmailOTPContract(t *testing.T) { diff --git a/internal/store/migrations/0020_verified_email_unique.sql b/internal/store/migrations/0020_verified_email_unique.sql new file mode 100644 index 0000000..29a3ab0 --- /dev/null +++ b/internal/store/migrations/0020_verified_email_unique.sql @@ -0,0 +1,15 @@ +-- The verified-email uniqueness that the email-first login design has claimed +-- since 0010 but no migration ever actually created: at most one account may +-- hold a PROVEN address (email_verified), compared case-insensitively, so the +-- pre-session login door (UserByEmail on lower(email)) resolves at most one +-- identity. VerifyEmailOTP's ErrEmailTaken guard is the application-level +-- counterpart; this index is the database-level backstop behind it, so a +-- cross-user race that passes the guard still cannot write a second verified +-- row (it loses to a unique violation instead). +-- +-- If an upgrade meets a database where the missing guard already produced two +-- verified rows for one address, CREATE UNIQUE INDEX refuses to build and names +-- the duplicated key in DETAIL. That is deliberate — no silent "unverify the +-- loser" surgery: only a human can say which holder keeps the address. +CREATE UNIQUE INDEX users_verified_email_unique ON users (lower(email)) + WHERE email_verified;