Skip to content

fix(auth): bound OTP emails per recipient - #39

Open
johnhooks wants to merge 1 commit into
mainfrom
refactor/rate-limiter-2
Open

fix(auth): bound OTP emails per recipient#39
johnhooks wants to merge 1 commit into
mainfrom
refactor/rate-limiter-2

Conversation

@johnhooks

@johnhooks johnhooks commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

An account that signs up and abandons email verification leaves a working credential behind. Posting those credentials to POST /auth/password/authenticate made WorkOS mail a fresh verification code every time, so anyone holding them could drive unlimited mail at the address they registered. The only email bound on that route was 5 per minute, sustainable indefinitely, and the transient counter behind it was non-atomic, so a concurrent burst passed straight through. This branch caps mail by who receives it, not just by who asks.

Changes

  • Sends are charged against the recipient. WorkOS mails the code as a side effect of authenticating and only reports it in the response, so sign-in reserves a slot before the call and refunds it when the outcome shows nothing went out. Failed passwords and completed logins cost a recipient nothing.
  • Magic-code send and password-reset start share one recipient bucket, so rotating between endpoints buys no extra mail. Sign-in keeps its own bucket so heavy magic-code use can't lock an account out of password login.
  • Send limits are layered over 5 minute, 1 hour, and 8 hour windows. A single fixed window is burstable across its boundary by construction; the longer tiers are what cap a day's volume. Request limits stay at one 60 second window, since a daily cap on sign-in attempts would lock out users who mistype a password.
  • Rate limiting moves out of the AuthKit package into WorkOS\RateLimit: a single-window interface with transient and object-cache implementations, plus a wrapper that stacks windows into a policy. Sites with a persistent object cache get atomic counters, which closes the concurrent-burst bypass; sites without keep the read-modify-write path, bounded by the longer tiers.
  • Counters are keyed by window number rather than a stored start time, so expiry is the storage TTL and retry_after is arithmetic.

Behavior changes to note

  • The change_email_rate_limit_* options are removed. Limits are class constants applied to every path that mails a caller-chosen address, so one policy governs sign-in, magic code, password reset, and email change. Previously stored values are inert. Change-email initiate moves from 3 per hour to the shared send policy.
  • Send buckets are charged before the "should we actually send?" branch, so 429 behavior is identical for known and unknown addresses and bucket state can't be used to enumerate accounts.
  • Bucket keys changed shape, so counters reset once on deploy. Windows are now aligned to the clock rather than starting at a caller's first attempt.

Signup's verification mail deliberately stays off the send policy: a duplicate create_user fails at WorkOS before the send is reached, so it isn't repeatable per address. Details in docs/rate-limiting.md.

This is an alternative to refactor/rate-limiter, which answers the same report with a larger rewrite (separate requester and recipient limiters over a pluggable counter store, an injected clock, and reservation objects). This branch reuses the existing limiter shape and stays closer to what is already deployed.

@johnhooks johnhooks self-assigned this Aug 4, 2026
@johnhooks
johnhooks requested review from bordoni and redscar August 4, 2026 20:33
@johnhooks
johnhooks marked this pull request as ready for review August 4, 2026 20:33
An account that signs up and abandons email verification leaves a working
credential behind. Posting those credentials to the sign-in endpoint made
WorkOS mail a fresh verification code every time, so anyone holding them
could drive unlimited mail at the address they registered. The only email
bound on that route was 5 per minute, which is sustainable indefinitely,
and the transient counter behind it was non-atomic, so a concurrent burst
passed straight through.

Sends are now charged against the recipient rather than only the
requester. WorkOS mails the code as a side effect of authenticating and
reports it afterwards, so the sign-in path reserves a slot before the
call and refunds it when the outcome shows nothing went out; failed
passwords and completed logins cost a recipient nothing. Magic-code send
and password-reset start share one recipient bucket, so rotating between
endpoints buys no extra mail.

Send limits are layered over 5 minute, 1 hour, and 8 hour windows. A
single fixed window is burstable across its boundary by construction, and
the longer tiers are what cap a day's volume. Request limits stay at one
60 second window, since a daily cap on sign-in attempts would lock out
users who mistype a password.

Rate limiting also moves out of the AuthKit package into its own domain:
a single-window interface with transient and object-cache
implementations, plus a wrapper that stacks windows into a policy. Sites
with a persistent object cache now get atomic counters, which closes the
concurrent-burst bypass; sites without keep the read-modify-write path,
bounded by the longer tiers.

The change_email_rate_limit_* options are removed. Limits are class
constants applied to every path that mails a caller-chosen address, so
one policy now governs sign-in, magic code, password reset, and email
change. Previously stored values are inert.

Adds docs/rate-limiting.md covering the model, the per-endpoint rules,
and what to do when adding a route that sends email.

@redscar redscar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw the other Stacked Pr's - I take back what I previously said. This looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants