From 52549f7b3a2bd016171e7d7cd87769dc2b2c47da Mon Sep 17 00:00:00 2001
From: Lemon-miaow <admin@lemonmiaow.xyz>
Date: Tue, 22 Sep 2026 19:51:04 +0800
Subject: [PATCH] =?UTF-8?q?fix(api):=20ConsumeLoginEmailOTP=20honesty=20?=
 =?UTF-8?q?=E2=80=94=20wrong/expired/consumed=20codes=20are=20ErrOTPInvali?=
 =?UTF-8?q?d=20400,=20not=20a=20500?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The PG implementation was a single UPDATE ... WHERE code_hash that returned
ErrNotFound on zero rows: every wrong, expired, replayed or superseded code on
the pre-session email-login door (and the op-login finish / migration confirm
doors) fell through to writeError's unmapped-error 500, and attempts were never
charged so otpMaxAttempts/ErrOTPLocked could not trigger. The fake repo and the
Repo interface ("SAME code lifecycle as VerifyEmailOTP") already documented the
intended contract; only the PG side had drifted.

Mirror VerifyEmailOTP's transaction without its users write: SELECT ... FOR
UPDATE the newest live row, expiry + attempt cap before the hash compare,
mismatch charges one attempt and returns ErrOTPInvalid without consuming,
match consumes and commits. Verified live on the VM: 5 wrong guesses return
400 and stop at attempts=5 (correct code then also refused, unconsumed);
fresh code redeems; replay returns 400.
---
 internal/api/pgrepo.go | 64 +++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 16 deletions(-)

diff --git a/internal/api/pgrepo.go b/internal/api/pgrepo.go
index 4eb83ad..25960ed 100644
--- a/internal/api/pgrepo.go
+++ b/internal/api/pgrepo.go
@@ -1878,29 +1878,61 @@ func (p *PGRepo) UserByEmail(ctx context.Context, email string) (*StaffUser, err
 	return &u, nil
 }
 
-// ConsumeLoginEmailOTP redeems a live code for the PRE-SESSION email login door.
-// Unlike VerifyEmailOTP it has no identity side-effects: it neither writes
-// users.email nor runs the verified-email uniqueness guard — login already
-// resolved the userID via UserByEmail, which requires email_verified, so the
-// address is settled. Zero rows affected (no live code, expired, consumed, or
-// hash mismatch) → ErrNotFound.
+// ConsumeLoginEmailOTP redeems the live code for the PRE-SESSION email login door
+// with the SAME lifecycle as VerifyEmailOTP (FOR UPDATE, expiry + attempt cap
+// before the hash compare, a mismatch charges one attempt without consuming) but
+// with NO identity side-effects: it neither writes users.email nor runs the
+// verified-email uniqueness guard — login already resolved the userID via
+// UserByEmail, which requires email_verified, so the address is settled. Errors
+// are exactly ErrOTPInvalid / ErrOTPLocked (ErrEmailTaken is structurally
+// impossible here).
 func (p *PGRepo) ConsumeLoginEmailOTP(ctx context.Context, userID, purpose, codeHash string, now time.Time) error {
-	res, err := p.db.ExecContext(ctx,
-		`UPDATE email_otps SET consumed_at = $4
-		 WHERE user_id = $1 AND purpose = $2 AND code_hash = $3
-		   AND consumed_at IS NULL AND expires_at > $4`,
-		userID, purpose, codeHash, now)
+	tx, err := p.db.BeginTx(ctx, nil)
 	if err != nil {
 		return err
 	}
-	n, err := res.RowsAffected()
-	if err != nil {
+	defer tx.Rollback() //nolint:errcheck // no-op after commit
+
+	var (
+		id         string
+		storedHash string
+		attempts   int
+		expiresAt  time.Time
+	)
+	switch err := tx.QueryRowContext(ctx,
+		`SELECT id, code_hash, attempts, expires_at FROM email_otps
+		 WHERE user_id = $1 AND purpose = $2 AND consumed_at IS NULL
+		 ORDER BY created_at DESC LIMIT 1 FOR UPDATE`,
+		userID, purpose).Scan(&id, &storedHash, &attempts, &expiresAt); {
+	case errors.Is(err, sql.ErrNoRows):
+		// Nothing live: never minted, already consumed, or superseded.
+		return ErrOTPInvalid
+	case err != nil:
 		return err
 	}
-	if n == 0 {
-		return ErrNotFound
+
+	if !expiresAt.After(now) {
+		return ErrOTPInvalid
 	}
-	return nil
+	if attempts >= otpMaxAttempts {
+		return ErrOTPLocked
+	}
+	if storedHash != codeHash {
+		if _, err := tx.ExecContext(ctx,
+			`UPDATE email_otps SET attempts = attempts + 1 WHERE id = $1`, id); err != nil {
+			return fmt.Errorf("record otp attempt: %w", err)
+		}
+		if err := tx.Commit(); err != nil {
+			return err
+		}
+		return ErrOTPInvalid
+	}
+
+	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)
+	}
+	return tx.Commit()
 }
 
 // ---- op.console staff login: in-game approval state machine (spec §B op-login) ----