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