Merge pull request 'fix(login): rate-limit no longer latches a phantom offline on the login screen' (#255) from feature/login-rate-limit-offline into development

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

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,
 	}
 }
 

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)
+		}
+	}
+}

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)', () => {
+  it('does not auto-retry a rate-limit rejection (a deliberate throttle, surfaced not spun)', () => {
-    expect(retryable('rate_limited', 'game.submit_play')).toBe(true);
+    // A rate-limit is the server saying "slow down": auto-retrying cannot succeed until the bucket
-    expect(retryable('rate_limited', 'games.list')).toBe(true);
+    // refills, only freezes the calling button for the whole backoff and feeds the gateway's ban
-    expect(retryable('rate_limited', 'chat.post')).toBe(true);
+    // 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);
   });

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
+// Retry policy: a transport 'unavailable' is ambiguous for a mutation (its response could have been
-// safe to retry. A transport 'unavailable' is ambiguous for a mutation (its response could have
+// lost after the backend applied it), so only **read-only** ops are auto-retried on 'unavailable'; a
-// been lost after the backend applied it), so only **read-only** ops are auto-retried on
+// mutation is surfaced instead (its button is disabled while offline and re-enables on reconnect, so
-// 'unavailable'; a mutation is surfaced instead (its button is disabled while offline and
+// the player re-issues it deliberately). A rate-limit rejection (ResourceExhausted) is NOT retried and
-// re-enables on reconnect, so the player re-issues it deliberately).
+// 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<string> = new Set([
 ]);
 
 /**
- * retryable reports whether a failed op should be auto-retried. A rate-limit rejection is always
+ * retryable reports whether a failed op should be auto-retried. A transport 'unavailable' is retried
- * safe (the gateway rejected it before processing); a transport 'unavailable' is retried only for
+ * only for read-only ops, never a mutation; every other code — a rate-limit (a deliberate throttle,
- * read-only ops, never a mutation; every other code (a domain rejection, not-found, …) is final.
+ * 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