From a93e934f62090154d257570a1dd6115b8fb0c7cf Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 21 Aug 2026 11:53:28 -0700 Subject: [PATCH 1/3] Add tests for outcome-based sign-in rate limiting Cover the fix for CL-6521: a correct password must always succeed regardless of prior failed attempts against the same account, a successful sign-in must clear the account's budget, repeated failures must still be bounded, and malformed sign-in bodies must never share one rate-limit bucket. --- apps/hub/src/sign-in-rate-limit.test.ts | 56 +++++++++++----- apps/hub/test/composition.test.ts | 87 ++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 19 deletions(-) diff --git a/apps/hub/src/sign-in-rate-limit.test.ts b/apps/hub/src/sign-in-rate-limit.test.ts index 3b998f6f..b634aa53 100644 --- a/apps/hub/src/sign-in-rate-limit.test.ts +++ b/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); }); }); diff --git a/apps/hub/test/composition.test.ts b/apps/hub/test/composition.test.ts index 3b4ed00a..ceb34fb7 100644 --- a/apps/hub/test/composition.test.ts +++ b/apps/hub/test/composition.test.ts @@ -184,6 +184,7 @@ describeIfDb( hub: Awaited>, email: string, forgedIp: string, + password = "wrong-password", ) { return hub.app.request("/api/auth/sign-in/email", { method: "POST", @@ -191,10 +192,23 @@ describeIfDb( "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>, + 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); + }); }, ); From 659a7d560edf9e16a0abf2136f0a2b6c8e33dd3c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 21 Aug 2026 11:53:40 -0700 Subject: [PATCH 2/3] Sign-in rate limiter: count only failures, reset on success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The account-keyed limiter consumed budget on every sign-in attempt, including successful ones, and never reset — a caller could POST a known email with garbage passwords a few times a minute and keep the real owner permanently locked out with correct credentials, since the budget never released. The limiter's own comment claimed this self-heals; it doesn't against a repeating attacker. The auth handler now always lets a sign-in attempt reach auth.handler and inspects the real response before touching the limiter: success clears the account's bucket, failure consumes it. A correct password is never rejected on account of someone else's prior wrong guesses. A distributed spray against one account is still bounded, since the key is the email, not the source. Also: the sign-in body is now parsed with an arktype schema instead of unsafe `as` casts, and a body that doesn't parse to an email never touches the limiter at all, instead of sharing one bucket with every other malformed request. The 429 response now sends the standard Retry-After header instead of X-Retry-After. --- apps/hub/src/index.ts | 38 +++++++++++++----------- apps/hub/src/sign-in-rate-limit.ts | 46 ++++++++++++++++++++++++------ 2 files changed, 59 insertions(+), 25 deletions(-) diff --git a/apps/hub/src/index.ts b/apps/hub/src/index.ts index a7352b65..b7a54568 100644 --- a/apps/hub/src/index.ts +++ b/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,26 +1036,29 @@ 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( { @@ -1061,9 +1066,10 @@ export async function createHub(config: HubConfig) { 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); }, diff --git a/apps/hub/src/sign-in-rate-limit.ts b/apps/hub/src/sign-in-rate-limit.ts index d0e1ae78..deee304c 100644 --- a/apps/hub/src/sign-in-rate-limit.ts +++ b/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)); + }, }; } From d5fe32c62eeeee2ef7b4b19df07b6ff4e6bc2051 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 21 Aug 2026 11:53:46 -0700 Subject: [PATCH 3/3] Update docs: SIGNIN_RATE_LIMIT_* is per-account, not per-IP config.ts documented these as a per-IP sign-in limit; the limiter has always been account-keyed and deliberately ignores client IP (see sign-in-rate-limit.ts). Left uncorrected, an operator would tune it against the wrong threat model. --- apps/hub/src/config.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/hub/src/config.ts b/apps/hub/src/config.ts index f6a50f79..96cba219 100644 --- a/apps/hub/src/config.ts +++ b/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",