CL-6521: Sign-in rate limiter no longer locks victims out indefinitely

apps/hub/src/config.ts

@@ -77,10 +77,10 @@ const HubEnv = type({
     "the maximum sign-ups a single IP may make per window, e.g. 5",
   ),
   "SIGNIN_RATE_LIMIT_WINDOW_SECONDS?": type(/^[1-9]\d*$/).describe(
-    "the per-IP sign-in rate-limit window, in seconds, e.g. 60; overrides better-auth's built-in 10-second/3-attempt default, which is too tight for a person retyping a password",
+    "the per-account sign-in rate-limit window, in seconds, e.g. 60; overrides better-auth's built-in 10-second/3-attempt default, which is too tight for a person retyping a password",
   ),
   "SIGNIN_RATE_LIMIT_MAX?": type(/^[1-9]\d*$/).describe(
-    "the maximum sign-in attempts a single IP may make per window, e.g. 10",
+    "the maximum failed sign-in attempts a single account may accrue per window before further failures are rejected, e.g. 10 — keyed on the target email, not client IP (see sign-in-rate-limit.ts); a correct password always succeeds regardless of this budget",
   ),
   "WORKBENCH_SIGNUP?": type("'open' | 'closed'").describe(
     "open = self-serve email signup allowed; closed (default) = owner adds users or copy-link invite only",

apps/hub/src/index.ts

@@ -332,6 +332,7 @@ import {
   createDrizzleCredentialExpirySweepStore,
 } from "./credential-expiry-sweep";
 
+import { type } from "arktype";
 import { betterAuth } from "better-auth";
 import { createBenchSessionMinter } from "./bench-session";
 import { createSignInAttemptLimiter } from "./sign-in-rate-limit";
@@ -381,6 +382,7 @@ const MAX_TARBALL_BYTES = 10 * 1024 * 1024;
 const TENANT_PREFIX = "/api/tenants/:tenantId";
 const SIGN_UP_EMAIL_PATH = "/sign-up/email";
 const SIGN_IN_EMAIL_PATH = "/sign-in/email";
+const SignInEmailBody = type({ email: "string" });
 // Chat residents carry a real hub-driven idle-reap again (reversing
 // CL-5477's removal): the sidecar's own park/wake scheme it was meant to
 // replace has itself been retired in favor of a simpler reap-and-relaunch
@@ -1034,36 +1036,40 @@ export async function createHub(config: HubConfig) {
           }
         }
       }
-      // Account-keyed sign-in brute-force protection (CL-6494) — see
-      // `sign-in-rate-limit.ts` for why this fully replaces better-auth's
-      // own IP-keyed enforcement for this path instead of running beside
-      // it.
+      // Account-keyed sign-in brute-force protection (CL-6494, hardened
+      // CL-6521) — see `sign-in-rate-limit.ts` for why this fully replaces
+      // better-auth's own IP-keyed enforcement for this path instead of
+      // running beside it, and for why only failures ever consume budget.
       if (c.req.method === "POST" && c.req.path.endsWith(SIGN_IN_EMAIL_PATH)) {
-        let email = "";
+        let email: string | undefined;
         try {
           const body: unknown = await c.req.raw.clone().json();
-          if (
-            body !== null &&
-            typeof body === "object" &&
-            "email" in body &&
-            typeof (body as { email: unknown }).email === "string"
-          ) {
-            email = (body as { email: string }).email;
-          }
+          const parsed = SignInEmailBody(body);
+          if (!(parsed instanceof type.errors)) email = parsed.email;
         } catch {
-          email = "";
+          email = undefined;
+        }
+        // A body that doesn't parse to `{ email: string }` never touches
+        // the limiter at all — there is no account to key a bucket on,
+        // and better-auth will reject the request on its own terms.
+        const response = await auth.handler(c.req.raw);
+        if (email === undefined) return response;
+        if (response.status >= 200 && response.status < 300) {
+          signInAttemptLimiter.recordSuccess(email);
+          return response;
         }
-        const decision = signInAttemptLimiter.consume(email);
+        const decision = signInAttemptLimiter.recordFailure(email);
         if (!decision.allowed) {
           return c.json(
             {
               error: "rate_limited",
               message: `Too many sign-in attempts. Try again in ${decision.retryAfterSeconds} second${decision.retryAfterSeconds === 1 ? "" : "s"}.`,
             },
             429,
-            { "X-Retry-After": decision.retryAfterSeconds.toString() },
+            { "Retry-After": decision.retryAfterSeconds.toString() },
           );
         }
+        return response;
       }
       return auth.handler(c.req.raw);
     },

apps/hub/src/sign-in-rate-limit.test.ts

@@ -2,21 +2,21 @@ import { describe, expect, test } from "bun:test";
 import { createSignInAttemptLimiter } from "./sign-in-rate-limit.ts";
 
 describe("createSignInAttemptLimiter", () => {
-  test("the Nth attempt against one account past the configured max is rejected", () => {
+  test("the Nth failure against one account past the configured max is rejected", () => {
     const limiter = createSignInAttemptLimiter(60, 2);
 
-    expect(limiter.consume("victim@example.com").allowed).toBe(true);
-    expect(limiter.consume("victim@example.com").allowed).toBe(true);
-    const throttled = limiter.consume("victim@example.com");
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    const throttled = limiter.recordFailure("victim@example.com");
 
     expect(throttled.allowed).toBe(false);
   });
 
-  test("a rejected attempt reports how many seconds remain in the window", () => {
+  test("a rejected failure reports how many seconds remain in the window", () => {
     const limiter = createSignInAttemptLimiter(60, 1);
 
-    limiter.consume("victim@example.com");
-    const throttled = limiter.consume("victim@example.com");
+    limiter.recordFailure("victim@example.com");
+    const throttled = limiter.recordFailure("victim@example.com");
 
     expect(throttled.allowed).toBe(false);
     if (!throttled.allowed) {
@@ -33,27 +33,49 @@ describe("createSignInAttemptLimiter", () => {
     // email — nothing about a rotated header changes it.
     const limiter = createSignInAttemptLimiter(60, 3);
 
-    expect(limiter.consume("victim@example.com").allowed).toBe(true);
-    expect(limiter.consume("victim@example.com").allowed).toBe(true);
-    expect(limiter.consume("victim@example.com").allowed).toBe(true);
-    expect(limiter.consume("victim@example.com").allowed).toBe(false);
-    expect(limiter.consume("victim@example.com").allowed).toBe(false);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(false);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(false);
   });
 
   test("two distinct accounts get independent budgets", () => {
     const limiter = createSignInAttemptLimiter(60, 1);
 
-    expect(limiter.consume("alice@example.com").allowed).toBe(true);
-    expect(limiter.consume("alice@example.com").allowed).toBe(false);
+    expect(limiter.recordFailure("alice@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("alice@example.com").allowed).toBe(false);
 
     // Bob's own budget is untouched by Alice's exhausted one.
-    expect(limiter.consume("bob@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("bob@example.com").allowed).toBe(true);
   });
 
   test("email matching is case- and whitespace-insensitive, so it can't be sidestepped by casing/padding", () => {
     const limiter = createSignInAttemptLimiter(60, 1);
 
-    expect(limiter.consume("Victim@Example.com").allowed).toBe(true);
-    expect(limiter.consume(" victim@example.com ").allowed).toBe(false);
+    expect(limiter.recordFailure("Victim@Example.com").allowed).toBe(true);
+    expect(limiter.recordFailure(" victim@example.com ").allowed).toBe(false);
+  });
+
+  test("a successful sign-in clears the account's budget, so a prior run of failures never carries over", () => {
+    const limiter = createSignInAttemptLimiter(60, 1);
+
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(false);
+
+    limiter.recordSuccess("victim@example.com");
+
+    // The next failure is treated as a fresh first attempt, not a
+    // continuation of the exhausted budget from before the success.
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+  });
+
+  test("recordSuccess is case- and whitespace-insensitive, matching the key recordFailure uses", () => {
+    const limiter = createSignInAttemptLimiter(60, 1);
+
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
+    limiter.recordSuccess(" Victim@Example.com ");
+
+    expect(limiter.recordFailure("victim@example.com").allowed).toBe(true);
   });
 });

apps/hub/src/sign-in-rate-limit.ts

@@ -1,4 +1,4 @@
-// Account-keyed sign-in attempt limiter (CL-6494).
+// Account-keyed sign-in attempt limiter (CL-6494, hardened CL-6521).
 //
 // better-auth's own rate limiter keys solely on client IP (falling back to
 // one shared bucket when no IP resolves), configured via
@@ -42,12 +42,23 @@
 // *secondary* per-source budget could be layered on top of this one, to
 // also blunt one source spraying many different accounts.
 //
-// Guards against the account key itself becoming a way to lock a known
-// user out of their own account: the window is short and the count is
-// generous (60s / 10 by default — `config.signInRateLimit`), so a lockout
-// an attacker forces self-heals within the window and never compounds
-// across windows, while a real user mistyping a password a few times in a
-// row is never affected.
+// Only failed attempts consume budget, and a successful sign-in clears the
+// bucket outright. This is deliberate, not incidental: an account-keyed
+// limiter that also counted (or blocked) successful attempts would let an
+// attacker who never learns the password still deny the real owner access
+// indefinitely — send a handful of wrong-password guesses against a known
+// email every window, forever, at near-zero cost, and the genuine holder
+// of the account is locked out by the same mechanism meant to protect
+// them. Counting only failures closes that: the caller in `index.ts`
+// always lets a sign-in attempt reach `auth.handler` and inspects the
+// real outcome before touching this limiter, so a correct password is
+// never rejected on account of someone else's prior wrong guesses,
+// however many there were. A distributed password spray against one
+// account — many source IPs, one target email — is still bounded: once
+// `max` wrong guesses have been recorded inside `windowSeconds`, every
+// further failure in that window is turned into a generic 429 instead of
+// a distinguishing auth failure, regardless of which IP it came from,
+// because the key is the email, never the source.
 
 const MAX_TRACKED_EMAILS = 50_000;
 
@@ -56,7 +67,21 @@ export type SignInAttemptDecision =
   | { readonly allowed: false; readonly retryAfterSeconds: number };
 
 export type SignInAttemptLimiter = {
-  consume(email: string): SignInAttemptDecision;
+  /**
+   * Records one failed sign-in attempt against `email`'s budget. Returns
+   * `allowed: false` once that account has already exhausted its budget
+   * for the current window — the caller should surface that as a 429
+   * instead of the underlying auth failure. Never called for an attempt
+   * that succeeded.
+   */
+  recordFailure(email: string): SignInAttemptDecision;
+  /**
+   * Clears `email`'s bucket entirely. Called after a successful sign-in
+   * so a prior run of wrong guesses — an attacker's or the account
+   * owner's own mistyping — never carries over to block the next
+   * legitimate attempt.
+   */
+  recordSuccess(email: string): void;
 };
 
 function normalizeEmail(email: string): string {
@@ -84,7 +109,7 @@ export function createSignInAttemptLimiter(
   }
 
   return {
-    consume(email: string): SignInAttemptDecision {
+    recordFailure(email: string): SignInAttemptDecision {
       const now = Date.now();
       pruneExpiredAndOverflow(now);
       const key = normalizeEmail(email);
@@ -104,5 +129,8 @@ export function createSignInAttemptLimiter(
       bucket.count += 1;
       return { allowed: true };
     },
+    recordSuccess(email: string): void {
+      buckets.delete(normalizeEmail(email));
+    },
   };
 }

apps/hub/test/composition.test.ts

@@ -184,17 +184,31 @@ describeIfDb(
       hub: Awaited<ReturnType<typeof createHub>>,
       email: string,
       forgedIp: string,
+      password = "wrong-password",
     ) {
       return hub.app.request("/api/auth/sign-in/email", {
         method: "POST",
         headers: {
           "content-type": "application/json",
           "x-forwarded-for": forgedIp,
         },
-        body: JSON.stringify({ email, password: "wrong-password" }),
+        body: JSON.stringify({ email, password }),
       });
     }
 
+    async function signUpUser(
+      hub: Awaited<ReturnType<typeof createHub>>,
+      email: string,
+      password: string,
+    ) {
+      const signUp = await hub.app.request("/api/auth/sign-up/email", {
+        method: "POST",
+        headers: { "content-type": "application/json" },
+        body: JSON.stringify({ email, password, name: "Test User" }),
+      });
+      expect(signUp.status).toBe(200);
+    }
+
     test("a forged, rotating IP header per attempt cannot exceed the per-account budget", async () => {
       const hub = await createHub({
         ...config,
@@ -254,10 +268,79 @@ describeIfDb(
       );
 
       expect(throttled.status).toBe(429);
-      expect(throttled.headers.get("x-retry-after")).not.toBeNull();
+      expect(throttled.headers.get("retry-after")).not.toBeNull();
       const body = (await throttled.json()) as { message: string };
       expect(body.message).toMatch(/\S/);
     });
+
+    test("an attacker exhausting the account's budget with wrong guesses never blocks the owner's correct password, and a successful sign-in resets the budget", async () => {
+      const hub = await createHub({
+        ...config,
+        signupMode: "open",
+        signInRateLimit: { windowSeconds: 60, max: 2 },
+      });
+      closers.push(hub.close);
+
+      const email = `owner-${crypto.randomUUID()}@example.com`;
+      const password = "correct-horse-battery";
+      await signUpUser(hub, email, password);
+
+      // Two wrong guesses exhaust the account's failure budget — exactly
+      // what an attacker who doesn't know the password sends.
+      await signInAttempt(hub, email, "203.0.113.70");
+      await signInAttempt(hub, email, "203.0.113.71");
+      const thirdWrongGuess = await signInAttempt(hub, email, "203.0.113.72");
+      expect(thirdWrongGuess.status).toBe(429);
+
+      // The account owner's correct password still succeeds: only
+      // failures ever consume budget, so a correct attempt is never
+      // gated on how many wrong guesses came before it.
+      const genuineSignIn = await signInAttempt(
+        hub,
+        email,
+        "203.0.113.73",
+        password,
+      );
+      expect(genuineSignIn.status).toBe(200);
+
+      // That success cleared the bucket: the very next wrong guess is a
+      // fresh first failure, not an immediate 429 carried over from the
+      // attacker's earlier attempts.
+      const freshFailureAfterSuccess = await signInAttempt(
+        hub,
+        email,
+        "203.0.113.74",
+      );
+      expect(freshFailureAfterSuccess.status).not.toBe(429);
+    });
+
+    test("malformed sign-in bodies are never rate-limited together and never block a real account", async () => {
+      const hub = await createHub({
+        ...config,
+        signInRateLimit: { windowSeconds: 60, max: 1 },
+      });
+      closers.push(hub.close);
+
+      // No `email` field at all: doesn't parse, so it never touches the
+      // limiter — unrelated malformed requests must not share one bucket.
+      for (let attempt = 0; attempt < 5; attempt += 1) {
+        const malformed = await hub.app.request("/api/auth/sign-in/email", {
+          method: "POST",
+          headers: { "content-type": "application/json" },
+          body: JSON.stringify({ password: "whatever" }),
+        });
+        expect(malformed.status).not.toBe(429);
+      }
+
+      // A real account's first attempt is untouched by the malformed
+      // traffic above.
+      const fresh = await signInAttempt(
+        hub,
+        "never-touched@example.com",
+        "203.0.113.90",
+      );
+      expect(fresh.status).not.toBe(429);
+    });
   },
 );