fix(api): make OTP-start throttle atomic to close concurrent-burst bypass
The email-OTP resend cooldown checked the window with a peek (allowed) and only recorded it after delivery. For OTP that throttle is the sole defense and each admitted send is a real, non-idempotent email, so a burst of truly concurrent starts all passed the peek before any recorded and every one mailed: N concurrent starts bombed a mailbox with N codes. Add an atomic reserve/release pair to cooldownLimiter: reserve checks and records the window in one critical section under the mutex, so a concurrent burst yields exactly one winner; release rolls a reservation back only if it is still the current one, so a slow failing caller never clobbers a newer holder. handleEmailOTPStart now reserves both the principal and the recipient key up front and defers a rollback that frees both windows on any mint, create, or delivery error — preserving the old "a failed send does not consume the cooldown" property, now race-free. The wake path keeps allowed→record: its real gate is the running cap and its side effect (SetDesiredState) is idempotent, so the peek gap is harmless there. Tests: a frozen-clock gate-mailer fires 8 concurrent starts for one victim from one principal and asserts exactly one mail and one 202; a flaky-mailer test proves a failed delivery releases the window so an immediate retry in the same instant is admitted.
This commit is contained in:
3 files changed
+182
-8
No files matched your search
@@ -121,16 +121,36 @@ 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.
|
||||
// Atomically reserve the cooldown on both the caller and the recipient BEFORE
|
||||
// minting, so a burst of truly concurrent starts yields exactly one winner. Here
|
||||
// the throttle is the sole defense and each admitted send is a real, non-idempotent
|
||||
// email, so an allowed→record peek would let N goroutines slip past together and
|
||||
// bomb a mailbox. The recipient key is lower-cased so case variants of one address
|
||||
// can't sidestep the per-mailbox cap. If any later step fails the deferred rollback
|
||||
// frees both windows, so a failed mint or delivery never consumes the cooldown —
|
||||
// the same property the old record-after-send gave, now race-free.
|
||||
userKey, emailKey := "user:"+p.UserID, "email:"+strings.ToLower(email)
|
||||
lim := a.otpLimiter()
|
||||
if !lim.allowed(userKey, otpResendCooldown) || !lim.allowed(emailKey, otpResendCooldown) {
|
||||
userAt, ok := lim.reserve(userKey, otpResendCooldown)
|
||||
if !ok {
|
||||
writeError(w, r, newError(http.StatusTooManyRequests, "otp_resend_cooldown",
|
||||
"a code was sent recently; wait a moment before requesting another"))
|
||||
return
|
||||
}
|
||||
emailAt, ok := lim.reserve(emailKey, otpResendCooldown)
|
||||
if !ok {
|
||||
lim.release(userKey, userAt)
|
||||
writeError(w, r, newError(http.StatusTooManyRequests, "otp_resend_cooldown",
|
||||
"a code was sent recently; wait a moment before requesting another"))
|
||||
return
|
||||
}
|
||||
committed := false
|
||||
defer func() {
|
||||
if !committed {
|
||||
lim.release(userKey, userAt)
|
||||
lim.release(emailKey, emailAt)
|
||||
}
|
||||
}()
|
||||
code, err := newEmailOTP()
|
||||
if err != nil {
|
||||
writeError(w, r, err)
|
||||
@@ -150,10 +170,9 @@ 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)
|
||||
// The send succeeded: keep both reservations (the deferred rollback becomes a
|
||||
// no-op) so the cooldown windows stand.
|
||||
committed = true
|
||||
a.audit(r, auditActor(p), "account.email.otp_sent", "")
|
||||
writeJSON(w, http.StatusAccepted, map[string]any{
|
||||
"sent": true,
|
||||
|
||||
Reference in new issue
Block a user