From 6c3999a06769cc9517ea9281cf3eeae4d2392656 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 30 Jun 2026 20:04:23 +0900 Subject: [PATCH] fix(api): rate-limit email-OTP sends to close the email-bomb vector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleEmailOTPStart minted and mailed a code on every call, so an authenticated caller could drive unbounded mail to any address they typed — an email-bomb primitive against arbitrary mailboxes. Add a separate otpLimiter (its own sync.Once and map, distinct from the wake limiter) and throttle each send on two keys before anything is minted: the caller (user:) and the recipient (email:). A refused send mints no code and mails nothing; both cooldowns are recorded only after delivery succeeds, mirroring the wake path so a failed mint or delivery never consumes the throttle. The two-key design stops both one account fanning out across addresses and many accounts converging on one mailbox. --- internal/api/api.go | 14 +++++ internal/api/handlers_email_otp.go | 20 ++++++ internal/api/handlers_email_otp_test.go | 81 +++++++++++++++++++++++++ 3 files changed, 115 insertions(+) diff --git a/internal/api/api.go b/internal/api/api.go index 38778fa..92ac72d 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -95,6 +95,9 @@ type API struct { cooldownOnce sync.Once cooldown *cooldownLimiter + + otpCooldownOnce sync.Once + otpCooldown *cooldownLimiter } // now returns the current time using the injected clock. @@ -113,6 +116,17 @@ func (a *API) limiter() *cooldownLimiter { return a.cooldown } +// otpLimiter lazily builds a SEPARATE cooldown limiter for email-OTP sends, so an +// OTP resend throttle never shares state with the wake throttle. Keyed by +// principal and by recipient (see handleEmailOTPStart), it bounds how often a code +// may be mailed and closes the email-bomb vector. +func (a *API) otpLimiter() *cooldownLimiter { + a.otpCooldownOnce.Do(func() { + a.otpCooldown = &cooldownLimiter{now: a.now, last: map[string]time.Time{}} + }) + return a.otpCooldown +} + // apiRoute is one served HTTP route. Each face exposes its routes as a single // table (internalAPIRoutes / externalAPIRoutes) so that one declaration drives // BOTH handler construction here AND the OpenAPI parity test (openapi_test.go): diff --git a/internal/api/handlers_email_otp.go b/internal/api/handlers_email_otp.go index 0218360..26ddb6e 100644 --- a/internal/api/handlers_email_otp.go +++ b/internal/api/handlers_email_otp.go @@ -41,6 +41,12 @@ const ( // a value in [0, otpCodeBound) zero-pads to exactly otpCodeDigits digits. otpCodeDigits = 6 otpCodeBound = 1_000_000 + // otpResendCooldown is the minimum spacing between OTP sends. Without it, + // handleEmailOTPStart is an email-bomb primitive: an authenticated caller could + // drive unbounded mail to any address they type. The cooldown is enforced on two + // keys (principal and recipient) so neither one account fanning out across many + // addresses, nor many accounts converging on one address, can flood a mailbox. + otpResendCooldown = 60 * time.Second ) // OTPMailer delivers a one-time code to an email address. It is a seam, not a @@ -115,6 +121,16 @@ func (a *API) handleEmailOTPStart(w http.ResponseWriter, r *http.Request) { writeError(w, r, newError(http.StatusBadRequest, "bad_request", "a valid email is required")) return } + // Throttle sends on both the caller and the recipient before minting anything, + // so a refused request mints no code and mails nothing. The recipient key is + // lower-cased so case variants of one address can't sidestep the per-mailbox cap. + userKey, emailKey := "user:"+p.UserID, "email:"+strings.ToLower(email) + lim := a.otpLimiter() + if !lim.allowed(userKey, otpResendCooldown) || !lim.allowed(emailKey, otpResendCooldown) { + writeError(w, r, newError(http.StatusTooManyRequests, "otp_resend_cooldown", + "a code was sent recently; wait a moment before requesting another")) + return + } code, err := newEmailOTP() if err != nil { writeError(w, r, err) @@ -134,6 +150,10 @@ func (a *API) handleEmailOTPStart(w http.ResponseWriter, r *http.Request) { writeError(w, r, err) return } + // Start both cooldowns only after a code was actually sent: a failed mint or + // delivery above must not consume the throttle, mirroring the wake path. + lim.record(userKey) + lim.record(emailKey) a.audit(r, auditActor(p), "account.email.otp_sent", "") writeJSON(w, http.StatusAccepted, map[string]any{ "sent": true, diff --git a/internal/api/handlers_email_otp_test.go b/internal/api/handlers_email_otp_test.go index c56e7fb..8477ca0 100644 --- a/internal/api/handlers_email_otp_test.go +++ b/internal/api/handlers_email_otp_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "net/http/httptest" "testing" "time" ) @@ -150,6 +151,81 @@ func TestEmailOTPStartValidation(t *testing.T) { }) } +// TestEmailOTPStartRateLimited closes the email-bomb vector: handleEmailOTPStart is +// an authenticated primitive that mails arbitrary addresses, so a resend cooldown +// bounds it on both the caller (a fan-out across many addresses) and the recipient +// (a convergence of many accounts on one mailbox). +func TestEmailOTPStartRateLimited(t *testing.T) { + const emailA, emailB = "a@example.net", "b@example.net" + u1 := &Principal{UserID: "u1", Email: "u1@example.net", Role: "user"} + u2 := &Principal{UserID: "u2", Email: "u2@example.net", Role: "user"} + start := func(eh http.Handler, email string) *httptest.ResponseRecorder { + return do(eh, "POST", "/api/v1/account/email/start", `{"email":"`+email+`"}`, nil) + } + + t.Run("same caller and recipient is throttled, then recovers after the cooldown", func(t *testing.T) { + repo := newFakeRepo() + mailer := &captureMailer{} + api := newTestAPI(repo, newFakeCluster()) + api.External = staticExternal{p: u1} + api.Mailer = mailer + clock := time.Unix(1_700_000_000, 0) + api.Now = func() time.Time { return clock } + eh := api.ExternalHandler() + + if w := start(eh, emailA); w.Code != http.StatusAccepted { + t.Fatalf("first send: code = %d, want 202 (%s)", w.Code, w.Body.String()) + } + // An immediate resend is refused with 429 — and mints/mails nothing. + if w := start(eh, emailA); w.Code != http.StatusTooManyRequests || decodeErr(t, w) != "otp_resend_cooldown" { + t.Fatalf("immediate resend: code = %d body %s, want 429 otp_resend_cooldown", w.Code, w.Body.String()) + } + if mailer.calls != 1 { + t.Errorf("mailer calls = %d, want 1 (the throttled resend must not mail)", mailer.calls) + } + if len(repo.otps) != 1 { + t.Errorf("persisted codes = %d, want 1 (the throttled resend must not mint)", len(repo.otps)) + } + // Once the cooldown elapses the same address may be mailed again. + clock = clock.Add(otpResendCooldown + time.Second) + if w := start(eh, emailA); w.Code != http.StatusAccepted { + t.Fatalf("post-cooldown send: code = %d, want 202 (%s)", w.Code, w.Body.String()) + } + }) + + t.Run("one caller cannot fan out across addresses", func(t *testing.T) { + api := newTestAPI(newFakeRepo(), newFakeCluster()) + api.External = staticExternal{p: u1} + api.Mailer = &captureMailer{} + eh := api.ExternalHandler() + + if w := start(eh, emailA); w.Code != http.StatusAccepted { + t.Fatalf("send to A: code = %d, want 202", w.Code) + } + // A different recipient, same caller, same instant: the per-caller key throttles. + if w := start(eh, emailB); w.Code != http.StatusTooManyRequests { + t.Fatalf("fan-out to B: code = %d, want 429", w.Code) + } + }) + + t.Run("many callers cannot converge on one recipient", func(t *testing.T) { + api := newTestAPI(newFakeRepo(), newFakeCluster()) + api.External = staticExternal{p: u1} + api.Mailer = &captureMailer{} + eh := api.ExternalHandler() + + if w := start(eh, emailA); w.Code != http.StatusAccepted { + t.Fatalf("u1 send to A: code = %d, want 202", w.Code) + } + // A different caller targeting the same address, same instant: the per-recipient + // key throttles even though u2 has never sent before. + api.External = staticExternal{p: u2} + if w := start(eh, emailA); w.Code != http.StatusTooManyRequests { + t.Fatalf("u2 converge on A: code = %d, want 429", w.Code) + } + }) +} + // TestEmailOTPVerifyRejections is the redeem-side failure matrix. The expired and // locked cases plant rows directly: the test clock is frozen, so an already-expired // or already-exhausted row is the only way to reach those branches deterministically. @@ -275,10 +351,15 @@ func TestEmailOTPSupersede(t *testing.T) { api := newTestAPI(repo, newFakeCluster()) api.External = staticExternal{p: user} api.Mailer = mailer + // A re-request is a fresh send, so it must clear the resend cooldown: advance the + // clock past it between the two starts (supersede is orthogonal to the throttle). + clock := time.Unix(1_700_000_000, 0) + api.Now = func() time.Time { return clock } eh := api.ExternalHandler() do(eh, "POST", "/api/v1/account/email/start", `{"email":"player@example.net"}`, nil) first := mailer.code + clock = clock.Add(otpResendCooldown + time.Second) do(eh, "POST", "/api/v1/account/email/start", `{"email":"player@example.net"}`, nil) second := mailer.code