Feature: per recipient email limits - #38
Closed
johnhooks wants to merge 8 commits into
Closed
Conversation
Forwarding headers gain one entry per hop, each appending the address it received from, so the left-most entry is whatever the client sent. Reading it let any caller behind a configured proxy mint a fresh identity per request and walk past every per-IP limit. The scan now runs right to left, skipping private and reserved ranges so an internal load balancer does not become everyone's bucket. The activity log shares this resolution instead of accepting any forwarding header present, so audit rows record an address the site can vouch for.
Three places worked out who was calling, and they disagreed. The WorkOS audit log saw only REMOTE_ADDR, so every event behind a CDN recorded the edge node, and the activity log walked forwarding headers with no trust configuration at all. ClientIp owns it now and is injected where it is used, so nothing borrows the rate limiter to answer a question about the request. Login no longer holds a limiter it never counted with.
Locks decide on cooldowns measured in hours, so exercising a rollover meant either waiting or reaching for a global filter. Clock is injected instead: Registry hands one to every lock it builds, and the garbage collector, profile panel and CLI take their own. Replaces the workos_lock_current_time filter, which shipped in no release. The workos_locks filter now also passes the clock, since a third-party lock needs one to construct.
The limiter tracked two cache keys per subject, a first-seen stamp and a count, measured its window from first use, and carried a recovery branch for when one key outlived the other. Its transient fallback stored an array, so even the non-atomic path was a read-modify-write over a structure. Counting now happens against one key with the window epoch built into it, so expiry is arithmetic and a stale counter can only reach a window that never comes round again. The value is a plain integer, which is what lets the object-cache store use wp_cache_incr and be genuinely atomic rather than atomic-ish. Storage sits behind CounterStore, and the clock is injected, so window rollover is testable without waiting for one. attempt() keeps its signature, so all nineteen call sites are unchanged and the old class goes. Reservations arrive with the core and are wired to the delivery paths next.
The initiate route had burst limits but no durable lock. Self-service requests now check and record one user-backed otp_send attempt per initiation, however many messages it sends or addresses it names, so a signed-in account cannot spray recipients through our sending domain. The capability-gated admin path keeps its bypass.
for_email() now keys unowned addresses on their canonical form, so re-spelling an inbox (alice+2@, dotted gmail) no longer restarts any counter on any lock. Addresses owned by a WP user still resolve to the account; the subject value stays canonical either way, so counters recorded before the account existed are found and folded in whichever spelling the user registered with. Locks now take an AddressCanonicalizer, and the workos_locks filter passes it as a fourth argument for constructing custom locks.
reserve()/release()/consume(), the Reservation handle and the CounterStore decrement primitive were ported for a delivery ceiling that shipped as attempt locks instead, leaving them with tests but no production caller. Docblocks referencing the never-built OutboundEmailRateLimiter now point at the locks that actually sit behind the limiter. Also reframes the lock Store contract's durability as the implementation's defining trade rather than a hard rule — a cache-backed store swapped in via workos_lock_store is a legitimate operator choice where flushes and eviction are controlled — and syncs the docs with the branch: the workos_locks filter's new fourth argument, and change-email's durable initiate cap.
Initiations drew on otp_send, whose label and refusal message describe one-time codes and whose budget is shared with sign-in verification sends — a user who burned strikes there was barred from changing their email, and the 429 talked about codes that were never involved. A dedicated email_change lock now caps self-service initiations at 3 per account with the standard ladder. Keyed per requesting account, never per address, since the destination is whatever the caller typed. The admin-direct path keeps its bypass. Lock tests import classes instead of spelling out FQCNs inline.
johnhooks
commented
Aug 4, 2026
| new MfaChallengeLock( $this->store, $this->clock, $this->addresses ), | ||
| new SignupLock( $this->store, $this->clock, $this->addresses ), | ||
| new TokenGuessLock( $this->store, $this->clock, $this->addresses ), | ||
| new EmailChangeLock( $this->store, $this->clock, $this->addresses ), |
Collaborator
Author
There was a problem hiding this comment.
Not a huge fan of this. When I added the clock and email canonicalizer, I was only thinking they would be DI injected, not manually wired like this.
Collaborator
Author
There was a problem hiding this comment.
Do we really need these filterable? I don't think so.
Also, I don't see the reason we need the Registry for this. We should just DI resolve the locks, there is a filter to replace them when called already.
redscar
reviewed
Aug 4, 2026
Comment on lines
+69
to
+72
| $plus = strpos( $local, '+' ); | ||
| if ( false !== $plus ) { | ||
| $local = substr( $local, 0, $plus ); | ||
| } |
Collaborator
There was a problem hiding this comment.
I don't think we can strip + everywhere. It's a legal character in the local part and not every provider treats it as a tag - on a lot of corporate and self-hosted mail business+brian@corp.com is just a different mailbox with a different person behind it. Stripping it means those two end up sharing a lock ladder, so one of them can burn through the other's OTP and reset allowance.
The dot handling right below this already does the careful version — only collapsing for the providers known to ignore them. I'd do the same for +, or at least put it behind a filter so we can turn it off for domains where it isn't a tag.
This scenario may not happen out in the open as it's pretty specific. I just wanted to call it out.
Collaborator
Author
|
close in favor of #39 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on
feature/reusable-attempt-locks. A signed-in account could trigger unlimited change-email verification mail to any address it typed, and the identities our limits count against were easy to fake — email aliases split ledgers, and a spoofed forwarding header minted a fresh IP per request. This branch caps the sending durably and makes both identities trustworthy.Changes
email_changelock so the cap and its refusal message are independent of the sign-in code budget. Admin-of-other keeps its bypass.REMOTE_ADDRunless the operator names the header their proxy overwrites, read so that caller-supplied entries carry no weight. Rate limits, the activity log, and the audit log now agree on who the caller is.Enumeration-safe routes keep their uniform responses; refused sends are skipped silently. Details in
docs/locks.mdanddocs/change-email.md.