From fe5a3d6d3bd98e83299b54e05e8ae54acb03a49b Mon Sep 17 00:00:00 2001 From: Ilia Denisov Date: Mon, 13 Jul 2026 22:48:59 +0200 Subject: [PATCH] fix(login): stop a rate-limit from latching a phantom offline on the login screen A fresh email login that hit the per-IP email rate limit stranded the user in an unrecoverable "offline" state that survived a full PWA restart, even though the network was fine. Root cause: the gateway email class (auth.email.request + auth.email.login, keyed per IP, 5/10min burst 2) trips on the third event, which in the natural "request code -> wrong code -> correct code" sequence is the correct-code login. The gateway returns ResourceExhausted; the client mapped it to 'rate_limited', which retry.ts classified as retryable + a connection code, so exec() called reportOffline(). That pushes the net-state machine into connecting, whose recovery probe is an authenticated profile.get -- but the login screen has no session, so the probe can never succeed and the machine latches offline. The transport kill switch (assertOnline) then refuses the very login that would fix it, and because the IP's email bucket refills only 1 token / 120s, each fresh attempt after a restart is rate-limited again -> offline again. Fix (client, narrow -- the trigger): - A rate-limit is no longer treated as connectivity. retry.ts no longer marks 'rate_limited' retryable or a connection code, so exec() never reports it offline; it surfaces as the existing error.rate_limited "slow down" message. Not auto-retrying it also stops the ~20s button freeze and avoids feeding the gateway's ban tripwire with 6 extra rejections per attempt. Fix (server): - Raise the email-code burst 2 -> 4 so the honest request + a mistyped code + the correct one is not throttled mid-login. Defence-in-depth over the backend's own per-code guards (5-attempt cap + 15-min TTL + send throttle). Tests: retry classification units updated to the new semantics; a gateway regression guard asserts the honest three-event email flow passes under the default policy. gateway/README.md rate-limit note updated. The deeper gap (the net-state recovery probe is session-gated, so any real transport failure on the session-less login/confirm screens still cannot self-heal) is left as a known residual, deferred by the owner. --- gateway/README.md | 3 ++- gateway/internal/config/config.go | 7 ++++- gateway/internal/ratelimit/ratelimit_test.go | 18 +++++++++++++ ui/src/lib/retry.test.ts | 20 ++++++++++----- ui/src/lib/retry.ts | 27 ++++++++++++-------- 5 files changed, 56 insertions(+), 19 deletions(-) diff --git a/gateway/README.md b/gateway/README.md index 95bcfcd..1a1276a 100644 --- a/gateway/README.md +++ b/gateway/README.md @@ -121,7 +121,8 @@ web code exchange via `internal/vkid`, and both forward the trusted `external_id Rate-limit defaults (built-in): public 30/min·IP (burst 10), authenticated 300/min·user (burst 80, sized for multi-device play), admin 60/min·IP (burst 20, guarding the `/_gm` mount ahead of its Basic-Auth), -email-code 5/10 min·IP (burst 2). +email-code 5/10 min·IP (burst 4, covering request + a mistyped code + the +correct one; defence-in-depth over the backend's per-code 5-attempt/15-min cap). Every rejection increments `gateway_rate_limited_total{class}` (`user`/`public`/`email`/`admin`) and logs one Debug line; a reporter drains the diff --git a/gateway/internal/config/config.go b/gateway/internal/config/config.go index 1dfb104..439bf3c 100644 --- a/gateway/internal/config/config.go +++ b/gateway/internal/config/config.go @@ -165,7 +165,12 @@ func DefaultRateLimit() RateLimitConfig { // because multi-device play tripped the old 120/40. UserPerMinute: 300, UserBurst: 80, AdminPerMinute: 60, AdminBurst: 20, - EmailPer10Min: 5, EmailBurst: 2, + // Email-code path (per IP), defence-in-depth over the backend's own per-code guards + // (a 5-attempt cap + 15-min TTL + per-recipient send throttle). Burst 4 so the honest flow + // — request a code, mistype once or twice, then enter the right one — is not throttled + // mid-login: a request + wrong-code + right-code sequence exhausted the old burst of 2 and + // tripped the limit on the correct code, which the client mis-read as going offline. + EmailPer10Min: 5, EmailBurst: 4, } } diff --git a/gateway/internal/ratelimit/ratelimit_test.go b/gateway/internal/ratelimit/ratelimit_test.go index 49f50e2..d7416fe 100644 --- a/gateway/internal/ratelimit/ratelimit_test.go +++ b/gateway/internal/ratelimit/ratelimit_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "scrabble/gateway/internal/config" "scrabble/gateway/internal/ratelimit" ) @@ -44,3 +45,20 @@ func TestPerWindow(t *testing.T) { t.Fatalf("per-window burst = %v, want [true true false]", got) } } + +// TestEmailBurstAllowsHonestLoginFlow guards the default email-code burst against being +// tightened back below the honest login sequence: requesting a code, submitting one wrong +// code, then the correct one are three immediate email-class events from one IP, and all +// must pass. A burst of 2 denied the correct-code login, which the client mis-read as going +// offline and could not recover from on the session-less login screen. This is defence in +// depth over the backend's own per-code attempt cap and code TTL. +func TestEmailBurstAllowsHonestLoginFlow(t *testing.T) { + rl := config.DefaultRateLimit() + p := ratelimit.Per(rl.EmailPer10Min, 10*time.Minute, rl.EmailBurst) + l := ratelimit.New() + for i, want := range []bool{true, true, true} { // request code, wrong code, right code + if got := l.Allow("email:198.51.100.7", p); got != want { + t.Fatalf("honest email event %d allowed = %v, want %v", i+1, got, want) + } + } +} diff --git a/ui/src/lib/retry.test.ts b/ui/src/lib/retry.test.ts index 65c9057..c08aaad 100644 --- a/ui/src/lib/retry.test.ts +++ b/ui/src/lib/retry.test.ts @@ -28,10 +28,15 @@ describe('toGatewayError', () => { }); describe('retryable', () => { - it('retries any op on a rate-limit rejection (it never reached the backend)', () => { - expect(retryable('rate_limited', 'game.submit_play')).toBe(true); - expect(retryable('rate_limited', 'games.list')).toBe(true); - expect(retryable('rate_limited', 'chat.post')).toBe(true); + it('does not auto-retry a rate-limit rejection (a deliberate throttle, surfaced not spun)', () => { + // A rate-limit is the server saying "slow down": auto-retrying cannot succeed until the bucket + // refills, only freezes the calling button for the whole backoff and feeds the gateway's ban + // tripwire with more rejections. It is surfaced to the caller instead. (Regression guard: the + // old retry path also called reportOffline, which on the session-less login screen — where the + // reachability probe can never heal — latched a stuck phantom offline.) + expect(retryable('rate_limited', 'game.submit_play')).toBe(false); + expect(retryable('rate_limited', 'games.list')).toBe(false); + expect(retryable('rate_limited', 'auth.email.login')).toBe(false); }); it('retries only read-only ops on a transport unavailable (a mutation could double-apply)', () => { @@ -53,9 +58,12 @@ describe('retryable', () => { }); describe('isConnectionCode', () => { - it('flags the transport/rate-limit codes the indicator covers', () => { + it('flags only the transport connectivity code (a rate-limit is a surfaced message, not connectivity)', () => { expect(isConnectionCode('unavailable')).toBe(true); - expect(isConnectionCode('rate_limited')).toBe(true); + // A rate-limit must NOT read as connectivity: isConnectionCode gates both reportOffline (the + // offline chrome) and handleError's toast suppression, so treating it as connectivity produced + // a silent, stuck phantom offline on the login screen. It surfaces as error.rate_limited. + expect(isConnectionCode('rate_limited')).toBe(false); expect(isConnectionCode('not_your_turn')).toBe(false); expect(isConnectionCode('internal')).toBe(false); }); diff --git a/ui/src/lib/retry.ts b/ui/src/lib/retry.ts index 6c1b562..61d9ce8 100644 --- a/ui/src/lib/retry.ts +++ b/ui/src/lib/retry.ts @@ -2,11 +2,14 @@ // fails at the transport level the app retries it with capped exponential backoff while showing // the "Connecting…" indicator, instead of flashing a red toast each time. // -// Idempotency: a rate-limit rejection (ResourceExhausted) never reached the backend, so any op is -// safe to retry. A transport 'unavailable' is ambiguous for a mutation (its response could have -// been lost after the backend applied it), so only **read-only** ops are auto-retried on -// 'unavailable'; a mutation is surfaced instead (its button is disabled while offline and -// re-enables on reconnect, so the player re-issues it deliberately). +// Retry policy: a transport 'unavailable' is ambiguous for a mutation (its response could have been +// lost after the backend applied it), so only **read-only** ops are auto-retried on 'unavailable'; a +// mutation is surfaced instead (its button is disabled while offline and re-enables on reconnect, so +// the player re-issues it deliberately). A rate-limit rejection (ResourceExhausted) is NOT retried and +// is NOT a connectivity code: it is a deliberate "slow down", so auto-retrying cannot succeed until the +// bucket refills — it only freezes the calling button for the whole backoff and feeds the gateway's ban +// tripwire with more rejections. It is surfaced as its own message (error.rate_limited) and never flips +// the offline chrome, which the session-less login screen has no session-bearing probe to recover from. import { Code, ConnectError } from '@connectrpc/connect'; import { GatewayError } from './client'; @@ -65,20 +68,22 @@ export const READ_OPS: ReadonlySet = new Set([ ]); /** - * retryable reports whether a failed op should be auto-retried. A rate-limit rejection is always - * safe (the gateway rejected it before processing); a transport 'unavailable' is retried only for - * read-only ops, never a mutation; every other code (a domain rejection, not-found, …) is final. + * retryable reports whether a failed op should be auto-retried. A transport 'unavailable' is retried + * only for read-only ops, never a mutation; every other code — a rate-limit (a deliberate throttle, + * surfaced not spun), a domain rejection, not-found, … — is final. */ export function retryable(code: string, op: string): boolean { - if (code === 'rate_limited') return true; if (code === 'unavailable') return READ_OPS.has(op); return false; } /** isConnectionCode reports whether a code is a transport/connectivity failure the Connecting - * indicator covers (so the UI suppresses its red toast). */ + * indicator covers — so the UI suppresses its red toast and reportOffline may flip the offline + * chrome. A rate-limit is deliberately NOT one: it is a genuine server signal, surfaced as its own + * "slow down" message, never the offline chrome (which the session-less login screen, whose + * reachability probe needs a bearer token, cannot recover from). */ export function isConnectionCode(code: string): boolean { - return code === 'unavailable' || code === 'rate_limited'; + return code === 'unavailable'; } /** backoffMs is the delay before retry attempt n (1-based): capped exponential growth plus a