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", 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.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/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)); + }, }; } 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); + }); }, );