diff --git a/docs/rate-limiting.md b/docs/rate-limiting.md index 1fd5007..8be35f4 100644 --- a/docs/rate-limiting.md +++ b/docs/rate-limiting.md @@ -73,12 +73,12 @@ Every public auth route is throttled by requester. A subset also counts as a sen | Endpoint | Request subject | Request policy | Send policy | |---|---|---|---| -| `POST /auth/password/authenticate` | email | shared | yes, reserved — see [Reserve and refund](#reserve-and-refund) | -| `POST /auth/password/reset/start` | email | shared | yes (`otp_send`) | +| `POST /auth/password/authenticate` | email, exact | shared | yes, reserved — see [Reserve and refund](#reserve-and-refund) | +| `POST /auth/password/reset/start` | mailbox | shared | yes (`otp_send`) | | `POST /auth/password/reset/confirm` | — | shared (IP only) | no | -| `POST /auth/magic/send` | email | shared | yes (`otp_send`) | -| `POST /auth/magic/verify` | email | shared | no | -| `POST /auth/signup/create` | email | `Signup` constants | no — see below | +| `POST /auth/magic/send` | mailbox | shared | yes (`otp_send`) | +| `POST /auth/magic/verify` | email, exact | shared | no | +| `POST /auth/signup/create` | mailbox | `Signup` constants | no — see below | | `POST /auth/signup/verify` | — | `Signup` constants (IP only) | no | | `POST /auth/mfa/challenge` | factor ID | shared | no | | `POST /auth/mfa/verify` | — | shared (IP only) | no | @@ -88,7 +88,7 @@ Every public auth route is throttled by requester. A subset also counts as a sen | `POST /users/{id}/email-change` | user ID | — | yes | | `POST /admin/users/{id}/password-reset` | user ID | shared | no | -"Shared" means `REQUEST_IP` plus, where there is a subject, `REQUEST_SUBJECT`. +"Shared" means `REQUEST_IP` plus, where there is a subject, `REQUEST_SUBJECT`. "Exact" and "mailbox" are the two email keyings described under [Which mailbox](#which-mailbox). `magic_send` and `pw_reset` share the `otp_send` bucket prefix, so rotating between those two endpoints buys an attacker nothing. `password` uses its own `password_otp` prefix: it reserves before it knows whether a send happens, and sharing would let heavy magic-code use lock an account out of password sign-in. @@ -105,10 +105,11 @@ WorkOS decides: authenticating an account whose email is unverified makes it mai So that path claims the slot before the call and hands it back when the outcome shows nothing was sent: ```php +$mailbox = $this->canonicalizer->canonical( $email ); $reserved = $this->rate_limit( [ [ self::BUCKET_PASSWORD_OTP_IP, $ip, TieredRateLimiter::SEND_IP ], - [ self::BUCKET_PASSWORD_OTP_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ], + [ self::BUCKET_PASSWORD_OTP_RECIPIENT, $mailbox, TieredRateLimiter::SEND_RECIPIENT ], ] ); if ( is_wp_error( $reserved ) ) { @@ -119,7 +120,7 @@ $workos_response = workos()->api()->authenticate_with_password( /* … */ ); if ( ! $this->triggered_verification_email( $workos_response ) ) { $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_IP, $ip, TieredRateLimiter::SEND_IP ); - $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ); + $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_RECIPIENT, $mailbox, TieredRateLimiter::SEND_RECIPIENT ); } ``` @@ -153,6 +154,34 @@ A real environment variable of the same name works too. When the header holds a When nothing usable is found, resolution returns `0.0.0.0` so a bucket still has a stable subject. +### Which mailbox + +A per-recipient limit is only as good as the key it counts against. `alice@gmail.com`, `alice+1@gmail.com`, and `a.l.i.c.e@gmail.com` are three strings and one inbox, so email subjects are reduced to a mailbox by `WorkOS\Email\AddressCanonicalizer` before they become a bucket key. The canonical form is a key only; the address passed to WorkOS is always what the caller typed. + +Both rewrites are opt-in per provider, and the reason is the asymmetry of guessing wrong: + +- Collapsing two real mailboxes into one bucket lets either exhaust the other's allowance. On a corporate or self-hosted domain `business+brian@corp.com` is routinely its own mailbox with its own owner, so a blanket rule turns a spam control into a way to lock a colleague out. +- Splitting one real mailbox across several buckets lets a caller earn an allowance per tag, but the per-IP send limits still cap the total mail one caller can cause. + +The second is recoverable; the first denies service to someone who did nothing wrong. So `+` is stripped only on domains known to treat it as a tag (Google, Outlook and its consumer domains, Fastmail), and dots are ignored only on Google. This is not over-caution: [RFC 5233](https://www.rfc-editor.org/rfc/rfc5233.html) states that the encoding of detailed addresses is "site and/or implementation specific", and RFC 5321 leaves local-part semantics to the destination host. Among big providers the convention is genuinely not universal, with Yahoo and iCloud offering disposable addresses instead of tags and Proton using a different separator. + +#### Mailbox or account + +Two different questions get keyed on an address, and they don't want the same answer: + +- **Guessing limits** (`password_email`, `magic_verify_email`) bound attempts to guess a password or a code for one account. The right subject is the account, and WorkOS keys accounts on the exact address. +- **Mail-causing limits** (both `*_recipient` buckets, plus `magic_send_email`, `pw_reset_email`, and `signup_email`) bound how much mail an inbox receives. The right subject is the mailbox, so the canonical form is correct. + +The split is not "request limits versus send limits". `pw_reset_email`, `magic_send_email`, and `signup_email` are per-request limits on routes whose whole effect is mail, so they belong with the mailbox group even though they sit next to the guessing limits in code. + +The two groups therefore use different subjects, even where they sit side by side in the same `rate_limit()` call. Nothing stops someone registering `alice@gmail.com` and `alice+work@gmail.com` as separate accounts, since WorkOS treats them as distinct users, while Gmail delivers both to one inbox. Keying guessing on the exact address means an attacker hammering one cannot spend the other's sign-in budget; keying mail on the mailbox means they cannot double the volume aimed at that single inbox. + +Guessing limits use the address as typed, which needs no account lookup, because the exact address is already the identifier WorkOS resolves. If you touch this, the neighbouring buckets look like the same kind of thing and are not: `signup_email` would let `alice+1@`, `alice+2@` and `alice+3@` create three accounts that all mail one inbox, and `pw_reset_email` and `magic_send_email` sit on routes that exist to send. All three stay canonical. + +Conditioning on whether the address is already known to us was considered and rejected. It would cost a user lookup on every unauthenticated request, ahead of the limit that exists to avoid work; it would make the bucket key depend on whether an account exists; and pre-authentication we can only see WordPress users, so an account that exists only in WorkOS would key differently depending on sync state. Recognising an address as registered also says nothing about whether its inbox is shared, which is the only thing the send limit cares about. + +The lists are fixed, not configurable. A self-hosted server that does treat `+` as a tag is invisible to us and will get a counter per tag, which is the milder of the two failures and is bounded by the per-IP send limits. Adding a domain is a code change, in `AddressCanonicalizer`. + ### Storage `ObjectCacheRateLimiter` is used when WordPress reports a persistent object cache. On a real backend `wp_cache_incr` is an atomic in-place increment, so two concurrent requests get distinct counts and a limit cannot be overspent. @@ -165,9 +194,11 @@ To check which one a site is using: wp eval 'var_dump( wp_using_ext_object_cache() );' ``` +This is worth checking per environment rather than assuming. A site can ship an object-cache plugin and still run without one: Object Cache Pro is inert unless the `object-cache.php` drop-in is in place and `WP_REDIS_DISABLED` is not set. When it is inert, the transient path is what runs, and the concurrent-burst gap described above is open. + ## Adding a send path -1. Take the slot before sending, with `SEND_IP` and `SEND_RECIPIENT`. +1. Take the slot before sending, with `SEND_IP` and `SEND_RECIPIENT`, keyed on the canonical mailbox rather than the address as typed. 2. Reuse `BaseEndpoint::BUCKET_OTP_SEND_IP` / `_RECIPIENT` unless the path has a reason not to share the allowance. Bucket names are constants so the sharing survives a rename. 3. Charge it before any "should we actually send?" branch, so a `429` never depends on whether the account exists. 4. If you only learn afterwards that nothing went out, refund both buckets. @@ -206,5 +237,6 @@ That is what was removed. One policy, one place, applied to every path that mail - `src/WorkOS/RateLimit/` — the module. - `src/WorkOS/REST/Auth/BaseEndpoint.php` — the `rate_limit()` helper endpoints call. - `src/WorkOS/Http/ClientIp.php` — caller identity, shared with the logs. +- `src/WorkOS/Email/AddressCanonicalizer.php` — which strings mean one mailbox. - [`docs/change-email.md`](change-email.md) — the change-email flow's use of the send policy. - [`docs/password-reset.md`](password-reset.md) — the reset endpoints. diff --git a/src/WorkOS/Controller.php b/src/WorkOS/Controller.php index 42f638b..d46772b 100644 --- a/src/WorkOS/Controller.php +++ b/src/WorkOS/Controller.php @@ -19,6 +19,7 @@ use WorkOS\REST\Controller as RESTController; use WorkOS\Webhook\Controller as WebhookController; use WorkOS\Sync\Controller as SyncController; +use WorkOS\Email\Controller as EmailController; use WorkOS\Http\Controller as HttpController; use WorkOS\Organization\Controller as OrganizationController; use WorkOS\RateLimit\Controller as RateLimitController; @@ -39,6 +40,7 @@ class Controller extends BaseController { protected function doRegister(): void { // Ahead of the features that resolve these at construction. $this->container->register( HttpController::class ); + $this->container->register( EmailController::class ); $this->container->register( RateLimitController::class ); $this->container->register( AdminController::class ); diff --git a/src/WorkOS/Email/AddressCanonicalizer.php b/src/WorkOS/Email/AddressCanonicalizer.php new file mode 100644 index 0000000..cb2e4bc --- /dev/null +++ b/src/WorkOS/Email/AddressCanonicalizer.php @@ -0,0 +1,118 @@ + 'gmail.com', + 'googlemail.com' => 'gmail.com', + ]; + + /** + * Providers known to treat `+` as a tag separator and deliver to the + * address on its left. + * + * Deliberately not "everyone". `+` is a legal local-part character, and + * on corporate or self-hosted mail `business+brian@corp.com` is + * routinely a real mailbox with its own owner. Consumer providers also + * disagree: Yahoo and iCloud offer disposable addresses instead of + * tags, and Proton uses a different separator entirely. + */ + private const PLUS_TAG_DOMAINS = [ + 'gmail.com', + 'googlemail.com', + 'outlook.com', + 'hotmail.com', + 'live.com', + 'msn.com', + 'fastmail.com', + 'fastmail.fm', + ]; + + /** + * Lowercase and trim an address. + * + * @param string $email Raw email string. + * + * @return string Empty when the input isn't an address. + */ + public function normalize( string $email ): string { + $email = strtolower( trim( $email ) ); + + return is_email( $email ) ? $email : ''; + } + + /** + * Reduce an address to its mailbox. + * + * Use this for anything keyed per-mailbox. Never use it as the address + * to send to: the canonical form is a bucket key, not a destination. + * + * @param string $email Raw email string. + * + * @return string Canonical form, or empty when the input isn't an address. + */ + public function canonical( string $email ): string { + $email = $this->normalize( $email ); + if ( '' === $email ) { + return ''; + } + + $at = strrpos( $email, '@' ); + if ( false === $at ) { + return $email; + } + + $local = substr( $email, 0, $at ); + $domain = substr( $email, $at + 1 ); + + if ( in_array( $domain, self::PLUS_TAG_DOMAINS, true ) ) { + $plus = strpos( $local, '+' ); + if ( false !== $plus ) { + $local = substr( $local, 0, $plus ); + } + } + + if ( isset( self::DOT_INSENSITIVE_DOMAINS[ $domain ] ) ) { + $local = str_replace( '.', '', $local ); + $domain = self::DOT_INSENSITIVE_DOMAINS[ $domain ]; + } + + // A local part that was only a tag collapses to empty, which would + // merge unrelated addresses into one mailbox. + return '' !== $local ? $local . '@' . $domain : $email; + } +} diff --git a/src/WorkOS/Email/Controller.php b/src/WorkOS/Email/Controller.php new file mode 100644 index 0000000..64091ec --- /dev/null +++ b/src/WorkOS/Email/Controller.php @@ -0,0 +1,37 @@ +container->singleton( AddressCanonicalizer::class ); + } + + /** + * Nothing to unhook: this module only provides container bindings. + * + * @return void + */ + protected function doUnregister(): void { + } +} diff --git a/src/WorkOS/REST/Auth/BaseEndpoint.php b/src/WorkOS/REST/Auth/BaseEndpoint.php index a32a651..5e12d58 100644 --- a/src/WorkOS/REST/Auth/BaseEndpoint.php +++ b/src/WorkOS/REST/Auth/BaseEndpoint.php @@ -12,6 +12,7 @@ use WorkOS\Auth\AuthKit\Profile; use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; +use WorkOS\Email\AddressCanonicalizer; use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WP_Error; @@ -79,6 +80,13 @@ abstract class BaseEndpoint { */ protected ClientIp $client_ip; + /** + * Mailbox canonicalizer for rate-limit subjects. + * + * @var AddressCanonicalizer + */ + protected AddressCanonicalizer $canonicalizer; + /** * Login completer. * @@ -89,12 +97,13 @@ abstract class BaseEndpoint { /** * Constructor. * - * @param ProfileRepository $profiles Profile repository. - * @param Nonce $nonce Nonce helper. - * @param Radar $radar Radar helper. - * @param TieredRateLimiter $rate_limiter Rate limiter. - * @param ClientIp $client_ip Client IP resolver. - * @param LoginCompleter $login_completer Login completer. + * @param ProfileRepository $profiles Profile repository. + * @param Nonce $nonce Nonce helper. + * @param Radar $radar Radar helper. + * @param TieredRateLimiter $rate_limiter Rate limiter. + * @param ClientIp $client_ip Client IP resolver. + * @param AddressCanonicalizer $canonicalizer Mailbox canonicalizer. + * @param LoginCompleter $login_completer Login completer. */ public function __construct( ProfileRepository $profiles, @@ -102,6 +111,7 @@ public function __construct( Radar $radar, TieredRateLimiter $rate_limiter, ClientIp $client_ip, + AddressCanonicalizer $canonicalizer, LoginCompleter $login_completer ) { $this->profiles = $profiles; @@ -109,6 +119,7 @@ public function __construct( $this->radar = $radar; $this->rate_limiter = $rate_limiter; $this->client_ip = $client_ip; + $this->canonicalizer = $canonicalizer; $this->login_completer = $login_completer; } diff --git a/src/WorkOS/REST/Auth/MagicCode.php b/src/WorkOS/REST/Auth/MagicCode.php index d640f79..91c3900 100644 --- a/src/WorkOS/REST/Auth/MagicCode.php +++ b/src/WorkOS/REST/Auth/MagicCode.php @@ -83,11 +83,15 @@ public function send( WP_REST_Request $request ) { ); } + // Count against the mailbox, not the string typed: tag and dot + // variants of one inbox would otherwise each get an allowance. + $mailbox = $this->canonicalizer->canonical( $email ); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'magic_send_ip', $ip, TieredRateLimiter::REQUEST_IP ], - [ 'magic_send_email', $email, TieredRateLimiter::REQUEST_SUBJECT ], + [ 'magic_send_email', $mailbox, TieredRateLimiter::REQUEST_SUBJECT ], ] ); if ( is_wp_error( $rate_ok ) ) { @@ -99,7 +103,7 @@ public function send( WP_REST_Request $request ) { $send_ok = $this->rate_limit( [ [ self::BUCKET_OTP_SEND_IP, $ip, TieredRateLimiter::SEND_IP ], - [ self::BUCKET_OTP_SEND_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ], + [ self::BUCKET_OTP_SEND_RECIPIENT, $mailbox, TieredRateLimiter::SEND_RECIPIENT ], ] ); if ( is_wp_error( $send_ok ) ) { @@ -157,10 +161,10 @@ public function verify( WP_REST_Request $request ) { $code = (string) $request->get_param( 'code' ); $pending_auth_token = (string) $request->get_param( 'pending_authentication_token' ); - if ( '' === $email || '' === $code ) { + if ( '' === $email || ! is_email( $email ) || '' === $code ) { return new WP_Error( 'workos_authkit_invalid_input', - __( 'An email and code are required.', 'integration-workos' ), + __( 'A valid email and code are required.', 'integration-workos' ), [ 'status' => 400 ] ); } @@ -169,6 +173,10 @@ public function verify( WP_REST_Request $request ) { $rate_ok = $this->rate_limit( [ [ 'magic_verify_ip', $ip, TieredRateLimiter::REQUEST_IP ], + // Code guessing is counted against the address as typed, not + // the mailbox: this route sends nothing, and the thing being + // guessed belongs to whichever account WorkOS resolves, which + // it keys on the exact address. [ 'magic_verify_email', $email, TieredRateLimiter::REQUEST_SUBJECT ], ] ); diff --git a/src/WorkOS/REST/Auth/Password.php b/src/WorkOS/REST/Auth/Password.php index faf4c67..e7962cf 100644 --- a/src/WorkOS/REST/Auth/Password.php +++ b/src/WorkOS/REST/Auth/Password.php @@ -120,6 +120,14 @@ public function authenticate( WP_REST_Request $request ) { ); } + // Two subjects, deliberately different. Mail is counted against the + // mailbox, since tag and dot variants of one inbox would otherwise + // each get an allowance. Password guessing is counted against the + // address as typed, because that is what WorkOS keys an account on: + // `alice@` and `alice+work@` can both be registered, and merging + // them would let an attacker on one spend the other's budget. + $mailbox = $this->canonicalizer->canonical( $email ); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ @@ -138,7 +146,7 @@ public function authenticate( WP_REST_Request $request ) { $reserved = $this->rate_limit( [ [ self::BUCKET_PASSWORD_OTP_IP, $ip, TieredRateLimiter::SEND_IP ], - [ self::BUCKET_PASSWORD_OTP_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ], + [ self::BUCKET_PASSWORD_OTP_RECIPIENT, $mailbox, TieredRateLimiter::SEND_RECIPIENT ], ] ); if ( is_wp_error( $reserved ) ) { @@ -194,7 +202,7 @@ public function authenticate( WP_REST_Request $request ) { // so the reserved OTP slot goes back. if ( ! $this->triggered_verification_email( $workos_response ) ) { $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_IP, $ip, TieredRateLimiter::SEND_IP ); - $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ); + $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_RECIPIENT, $mailbox, TieredRateLimiter::SEND_RECIPIENT ); } // `complete()` handles the `organization_selection_required` error @@ -248,11 +256,13 @@ public function reset_start( WP_REST_Request $request ) { ); } + $mailbox = $this->canonicalizer->canonical( $email ); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'pw_reset_ip', $ip, TieredRateLimiter::REQUEST_IP ], - [ 'pw_reset_email', $email, TieredRateLimiter::REQUEST_SUBJECT ], + [ 'pw_reset_email', $mailbox, TieredRateLimiter::REQUEST_SUBJECT ], ] ); if ( is_wp_error( $rate_ok ) ) { @@ -264,7 +274,7 @@ public function reset_start( WP_REST_Request $request ) { $send_ok = $this->rate_limit( [ [ self::BUCKET_OTP_SEND_IP, $ip, TieredRateLimiter::SEND_IP ], - [ self::BUCKET_OTP_SEND_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ], + [ self::BUCKET_OTP_SEND_RECIPIENT, $mailbox, TieredRateLimiter::SEND_RECIPIENT ], ] ); if ( is_wp_error( $send_ok ) ) { diff --git a/src/WorkOS/REST/Auth/Signup.php b/src/WorkOS/REST/Auth/Signup.php index ceb3244..ef70dee 100644 --- a/src/WorkOS/REST/Auth/Signup.php +++ b/src/WorkOS/REST/Auth/Signup.php @@ -108,11 +108,16 @@ public function create( WP_REST_Request $request ) { ); } + // Counted against the mailbox, not the address, even though this + // limit guards account creation rather than a send. Creating an + // account mails it, so `alice+1@`, `alice+2@` and `alice+3@` would + // otherwise be three accounts' worth of verification mail landing in + // one inbox. $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'signup_ip', $ip, self::RATE_LIMIT_CREATE_IP ], - [ 'signup_email', $email, self::RATE_LIMIT_CREATE_EMAIL ], + [ 'signup_email', $this->canonicalizer->canonical( $email ), self::RATE_LIMIT_CREATE_EMAIL ], ] ); if ( is_wp_error( $rate_ok ) ) { diff --git a/tests/wpunit/AddressCanonicalizerTest.php b/tests/wpunit/AddressCanonicalizerTest.php new file mode 100644 index 0000000..b5fcde5 --- /dev/null +++ b/tests/wpunit/AddressCanonicalizerTest.php @@ -0,0 +1,163 @@ +canonicalizer = new AddressCanonicalizer(); + } + + /** + * Addresses that reduce to the same mailbox. + * + * @param string $email Input address. + * @param string $expected Canonical form. + */ + #[DataProvider( 'collapses_provider' )] + public function test_collapses_variants_of_one_mailbox( string $email, string $expected ): void { + $this->assertSame( $expected, $this->canonicalizer->canonical( $email ) ); + } + + /** + * Addresses that must stay distinct, because on these domains the + * character is part of the mailbox name rather than a tag. + * + * @param string $email Input address. + */ + #[DataProvider( 'stays_distinct_provider' )] + public function test_leaves_unknown_providers_alone( string $email ): void { + $this->assertSame( $email, $this->canonicalizer->canonical( $email ) ); + } + + /** + * Two people on a corporate domain keep separate allowances. This is + * the case the plus-stripping allowlist exists for. + */ + public function test_corporate_plus_addresses_do_not_share_a_bucket(): void { + $shared = $this->canonicalizer->canonical( 'business+brian@corp.com' ); + $personal = $this->canonicalizer->canonical( 'business@corp.com' ); + + $this->assertNotSame( $shared, $personal ); + } + + /** + * Dots are only inert on the providers that ignore them. + */ + public function test_dots_only_collapse_for_known_providers(): void { + $this->assertSame( 'alice@gmail.com', $this->canonicalizer->canonical( 'a.l.i.c.e@gmail.com' ) ); + $this->assertSame( 'a.lice@outlook.com', $this->canonicalizer->canonical( 'a.lice@outlook.com' ) ); + } + + /** + * An address that is nothing but a tag would collapse to an empty + * local part, which would merge unrelated senders; it is left alone. + */ + public function test_tag_only_local_part_is_left_intact(): void { + $this->assertSame( '+tag@gmail.com', $this->canonicalizer->canonical( '+tag@gmail.com' ) ); + } + + /** + * Non-addresses yield empty rather than a bucket key. + * + * @param string $input Input string. + */ + #[DataProvider( 'not_an_address_provider' )] + public function test_rejects_non_addresses( string $input ): void { + $this->assertSame( '', $this->canonicalizer->canonical( $input ) ); + } + + /** + * Normalization lowercases and trims without touching the mailbox. + */ + public function test_normalize_lowercases_and_trims(): void { + $this->assertSame( 'alice+tag@corp.com', $this->canonicalizer->normalize( ' Alice+Tag@Corp.com ' ) ); + $this->assertSame( '', $this->canonicalizer->normalize( 'nope' ) ); + } + + // ------------------------------------------------------------------------- + // Providers + // ------------------------------------------------------------------------- + + /** + * Variants that are genuinely one inbox. + * + * @return array + */ + public function collapses_provider(): array { + return [ + 'gmail tag' => [ 'alice+newsletter@gmail.com', 'alice@gmail.com' ], + 'gmail dots' => [ 'a.l.i.c.e@gmail.com', 'alice@gmail.com' ], + 'gmail dots and tag' => [ 'a.lice+x@gmail.com', 'alice@gmail.com' ], + 'googlemail alias' => [ 'alice@googlemail.com', 'alice@gmail.com' ], + 'outlook tag' => [ 'alice+x@outlook.com', 'alice@outlook.com' ], + 'hotmail tag' => [ 'alice+x@hotmail.com', 'alice@hotmail.com' ], + 'fastmail tag' => [ 'alice+x@fastmail.com', 'alice@fastmail.com' ], + 'case and whitespace' => [ ' Alice@Gmail.com ', 'alice@gmail.com' ], + 'multiple tag markers' => [ 'alice+a+b@gmail.com', 'alice@gmail.com' ], + ]; + } + + /** + * Addresses that must survive untouched. + * + * Yahoo and iCloud offer disposable addresses rather than tags, and a + * self-hosted or corporate server commonly treats `+` as an ordinary + * character, so on all of these the tag form is a different mailbox. + * + * @return array + */ + public function stays_distinct_provider(): array { + return [ + 'corporate tag' => [ 'business+brian@corp.com' ], + 'self-hosted tag' => [ 'ops+alerts@example.org' ], + 'yahoo tag' => [ 'alice+x@yahoo.com' ], + 'icloud tag' => [ 'alice+x@icloud.com' ], + 'proton tag' => [ 'alice+x@proton.me' ], + 'corporate dots' => [ 'a.lice@corp.com' ], + 'outlook dots' => [ 'a.lice@outlook.com' ], + 'plain corporate' => [ 'alice@corp.com' ], + ]; + } + + /** + * Strings that are not addresses. + * + * @return array + */ + public function not_an_address_provider(): array { + return [ + 'empty' => [ '' ], + 'no at sign' => [ 'alice' ], + 'no domain' => [ 'alice@' ], + 'spaces only' => [ ' ' ], + ]; + } +} diff --git a/tests/wpunit/AuthKitRestMagicSessionTest.php b/tests/wpunit/AuthKitRestMagicSessionTest.php index 7a7a54b..cbd30a3 100644 --- a/tests/wpunit/AuthKitRestMagicSessionTest.php +++ b/tests/wpunit/AuthKitRestMagicSessionTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Email\AddressCanonicalizer; use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\MagicCode; @@ -105,8 +106,8 @@ public function setUp(): void { $rate_limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); $completer = new LoginCompleter(); - $magic = new MagicCode( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); - $session = new Session( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); + $magic = new MagicCode( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), new AddressCanonicalizer(), $completer ); + $session = new Session( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), new AddressCanonicalizer(), $completer ); add_action( 'rest_api_init', [ $magic, 'register_routes' ] ); add_action( 'rest_api_init', [ $session, 'register_routes' ] ); @@ -429,6 +430,30 @@ public function test_legacy_block_does_not_affect_default_form(): void { // ------------------------------- /auth/magic/verify ------------------------ + /** + * A malformed address is rejected before rate limiting rather than + * reaching it. The per-email bucket is keyed on the canonical mailbox, + * which is empty for a non-address, and an empty subject is skipped + * rather than counted, so accepting one here would leave that bucket + * silently unenforced. + */ + public function test_magic_verify_rejects_malformed_email(): void { + $response = $this->dispatch_with_nonce( + 'POST', + '/workos/v1/auth/magic/verify', + [ 'profile' => 'members', 'email' => 'not-an-email', 'code' => '123456' ] + ); + + $this->assertSame( 400, $response->get_status() ); + $this->assertSame( 'workos_authkit_invalid_input', $response->get_data()['code'] ); + + $verify_calls = array_filter( + $this->captured, + static fn( array $c ) => str_contains( $c['url'], '/user_management/authenticate' ) + ); + $this->assertEmpty( $verify_calls, 'A malformed address must not reach WorkOS.' ); + } + public function test_magic_verify_completes_login_on_success(): void { self::factory()->user->create( [ 'user_email' => 'alice@example.com' ] ); diff --git a/tests/wpunit/AuthKitRestMfaTest.php b/tests/wpunit/AuthKitRestMfaTest.php index aa5c0fa..82b8a19 100644 --- a/tests/wpunit/AuthKitRestMfaTest.php +++ b/tests/wpunit/AuthKitRestMfaTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Email\AddressCanonicalizer; use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\Mfa; @@ -95,6 +96,7 @@ public function setUp(): void { new Radar(), new TieredRateLimiter( new DatabaseRateLimiter() ), new ClientIp(), + new AddressCanonicalizer(), new LoginCompleter() ); add_action( 'rest_api_init', [ $mfa, 'register_routes' ] ); diff --git a/tests/wpunit/AuthKitRestPasswordTest.php b/tests/wpunit/AuthKitRestPasswordTest.php index 0c7b3c6..266479b 100644 --- a/tests/wpunit/AuthKitRestPasswordTest.php +++ b/tests/wpunit/AuthKitRestPasswordTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Email\AddressCanonicalizer; use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\Password; @@ -123,6 +124,7 @@ public function setUp(): void { new Radar(), new TieredRateLimiter( new DatabaseRateLimiter() ), new ClientIp(), + new AddressCanonicalizer(), new LoginCompleter() ); @@ -1401,4 +1403,48 @@ public function test_authenticate_rate_limits_by_ip(): void { $this->assertSame( 429, $response->get_status() ); } + + /** + * The two subjects on this route diverge on purpose. + * + * Password guessing is counted against the address as typed, so two + * addresses that are separate WorkOS accounts keep separate budgets and + * an attacker on one can't lock the other out. Mail is counted against + * the mailbox, so tag variants of one inbox share a send allowance. + */ + public function test_guessing_counts_per_address_while_sends_count_per_mailbox(): void { + $limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); + $canon = ( new AddressCanonicalizer() )->canonical( 'alice+work@gmail.com' ); + + // The guessing bucket is keyed on the exact address, so the tag + // variant is untouched by attempts against the bare address. + for ( $i = 0; $i < 5; $i++ ) { + $this->assertTrue( + $limiter->attempt( 'password_email', 'alice@gmail.com', TieredRateLimiter::REQUEST_SUBJECT ), + "Attempt {$i} on the bare address should be allowed." + ); + } + $this->assertInstanceOf( + \WP_Error::class, + $limiter->attempt( 'password_email', 'alice@gmail.com', TieredRateLimiter::REQUEST_SUBJECT ) + ); + $this->assertTrue( + $limiter->attempt( 'password_email', 'alice+work@gmail.com', TieredRateLimiter::REQUEST_SUBJECT ), + 'A separate account must keep its own guessing budget.' + ); + + // The send bucket is keyed on the mailbox, so both variants spend it. + for ( $i = 0; $i < 3; $i++ ) { + $this->assertTrue( $limiter->attempt( 'password_otp_recipient', $canon, TieredRateLimiter::SEND_RECIPIENT ) ); + } + $this->assertSame( + $canon, + ( new AddressCanonicalizer() )->canonical( 'alice@gmail.com' ), + 'Both variants must reduce to the same mailbox.' + ); + $this->assertInstanceOf( + \WP_Error::class, + $limiter->attempt( 'password_otp_recipient', $canon, TieredRateLimiter::SEND_RECIPIENT ) + ); + } } diff --git a/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php b/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php index ca427e8..1c93972 100644 --- a/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php +++ b/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Email\AddressCanonicalizer; use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\Invitation; @@ -111,9 +112,9 @@ public function setUp(): void { $rate_limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); $completer = new LoginCompleter(); - $signup = new Signup( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); - $invitation = new Invitation( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); - $oauth = new OAuth( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); + $signup = new Signup( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), new AddressCanonicalizer(), $completer ); + $invitation = new Invitation( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), new AddressCanonicalizer(), $completer ); + $oauth = new OAuth( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), new AddressCanonicalizer(), $completer ); add_action( 'rest_api_init', [ $signup, 'register_routes' ] ); add_action( 'rest_api_init', [ $invitation, 'register_routes' ] ); diff --git a/tests/wpunit/ChangeEmailRestApiTest.php b/tests/wpunit/ChangeEmailRestApiTest.php index 87da5a7..edc484c 100644 --- a/tests/wpunit/ChangeEmailRestApiTest.php +++ b/tests/wpunit/ChangeEmailRestApiTest.php @@ -69,8 +69,6 @@ public function setUp(): void { ); \WorkOS\App::container()->get( \WorkOS\Options\Production::class )->reset(); - $this->reset_rate_limit_buckets(); - $tokens = new TokenFactory(); $pending = new PendingChange( $tokens ); $mailer = new Mailer(); @@ -686,14 +684,4 @@ public function test_cancel_without_token_or_cap_is_403(): void { // ----------------------------------------------------------------- helpers - private function reset_rate_limit_buckets(): void { - global $wpdb; - $wpdb->query( - "DELETE FROM {$wpdb->options} WHERE option_name LIKE '_transient_workos_rl_%' OR option_name LIKE '_transient_timeout_workos_rl_%'" - ); - - if ( wp_using_ext_object_cache() ) { - wp_cache_flush(); - } - } } diff --git a/tests/wpunit/RateLimiterTest.php b/tests/wpunit/RateLimiterTest.php index db17545..d27e7b5 100644 --- a/tests/wpunit/RateLimiterTest.php +++ b/tests/wpunit/RateLimiterTest.php @@ -25,20 +25,19 @@ class RateLimiterTest extends WPTestCase { /** - * Build a limiter and clear the bucket this test uses. + * Build a limiter. * - * @param string $class Limiter implementation. - * @param int $window Window length the test will use. + * No bucket clearing needed: the suite flushes the object cache and + * opens a transaction before each test, then rolls back, so counters + * cannot survive in either store. + * + * @param string $class Limiter implementation. * * @return RateLimiter */ - private function limiter( string $class, int $window = 60 ): RateLimiter { + private function limiter( string $class ): RateLimiter { /** @var RateLimiter $limiter */ - $limiter = new $class(); - $limiter->reset( 'test_bucket', '192.168.0.1', $window ); - $limiter->reset( 'test_bucket', '10.0.0.1', $window ); - - return $limiter; + return new $class(); } /**