From d7b5fcd6c2ddaf06243acbb78a079b4a3d58248b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 20:10:12 +0200 Subject: [PATCH] fix(auth): cap failed password sign-ins per account, not only per address Password sign-in was limited per client address only, so guesses spread over many addresses reached one account without end. Each account now has its own ceiling of ten failed attempts in fifteen minutes, whichever addresses they come from. The attempt is counted before the password is hashed and given back when the password is right, so the owner signing in does not use it up. An identifier that matches no account is counted under its hash with the same ceiling, so the refusal does not reveal whether the account exists. Passkey and single sign-on remain open while the ceiling is reached. --- .../api/auth/login/mfa-branch.test.ts | 58 +++++++++++++++++++ src/app/api/auth/login/route.ts | 55 +++++++++++++++++- 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/__tests__/api/auth/login/mfa-branch.test.ts b/src/__tests__/api/auth/login/mfa-branch.test.ts index 79de4e666..3210a5c44 100644 --- a/src/__tests__/api/auth/login/mfa-branch.test.ts +++ b/src/__tests__/api/auth/login/mfa-branch.test.ts @@ -29,6 +29,8 @@ vi.mock("@/lib/rate-limit", () => ({ resetAt: Date.now() + 1e6, ip: "203.0.113.7", }), + checkRateLimit: vi.fn(), + refundRateLimit: vi.fn(), rateLimitHeaders: vi.fn(() => ({})), })); vi.mock("@/lib/auth/login-response", () => ({ @@ -61,6 +63,7 @@ import { POST } from "@/app/api/auth/login/route"; import { prisma } from "@/lib/db"; import { finishLogin } from "@/lib/auth/login-response"; import { createMfaChallenge } from "@/lib/auth/mfa/challenge"; +import { checkRateLimit, refundRateLimit } from "@/lib/rate-limit"; function loginRequest() { return new Request("http://localhost/api/auth/login", { @@ -85,6 +88,12 @@ beforeEach(() => { vi.clearAllMocks(); // Default: no registered security key. Individual tests override as needed. vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + limit: 10, + remaining: 9, + resetAt: Date.now() + 1e6, + }); }); describe("login MFA branch", () => { @@ -155,3 +164,52 @@ describe("login MFA branch", () => { expect(finishLogin).not.toHaveBeenCalled(); }); }); + +describe("per-account login ceiling", () => { + it("refuses a throttled account before any password is verified", async () => { + const { verifyPasswordOrDummy } = await import("@/lib/auth/password"); + vi.mocked(prisma.user.findFirst).mockResolvedValue({ + ...BASE_USER, + totpConfirmedAt: null, + } as never); + vi.mocked(checkRateLimit).mockResolvedValueOnce({ + allowed: false, + limit: 10, + remaining: 0, + resetAt: Date.now() + 1e6, + }); + + const res = await POST(loginRequest()); + expect(res.status).toBe(429); + expect(verifyPasswordOrDummy).not.toHaveBeenCalled(); + expect(finishLogin).not.toHaveBeenCalled(); + expect(vi.mocked(checkRateLimit).mock.calls[0][0]).toBe( + "auth:login:account:u:user-1", + ); + }); + + it("keys an unknown identifier on its hash, so a lock says nothing about existence", async () => { + vi.mocked(prisma.user.findFirst).mockResolvedValue(null as never); + const res = await POST(loginRequest()); + expect(res.status).toBe(401); + expect(vi.mocked(checkRateLimit).mock.calls[0][0]).toMatch( + /^auth:login:account:i:/, + ); + }); + + it("refunds a successful sign-in and keeps a failed one counted", async () => { + const { verifyPasswordOrDummy } = await import("@/lib/auth/password"); + vi.mocked(prisma.user.findFirst).mockResolvedValue({ + ...BASE_USER, + totpConfirmedAt: null, + } as never); + + await POST(loginRequest()); + expect(refundRateLimit).toHaveBeenCalledWith("auth:login:account:u:user-1"); + + vi.mocked(refundRateLimit).mockClear(); + vi.mocked(verifyPasswordOrDummy).mockResolvedValueOnce(false); + await POST(loginRequest()); + expect(refundRateLimit).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/auth/login/route.ts b/src/app/api/auth/login/route.ts index 63e285c6a..2c07466cb 100644 --- a/src/app/api/auth/login/route.ts +++ b/src/app/api/auth/login/route.ts @@ -9,7 +9,12 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +import { + checkAuthSurfaceRateLimit, + checkRateLimit, + rateLimitHeaders, + refundRateLimit, +} from "@/lib/rate-limit"; import { ensureDbCompatibility } from "@/lib/db-compat"; import { NextRequest, NextResponse } from "next/server"; import { apiHandler } from "@/lib/api-handler"; @@ -21,6 +26,15 @@ import { syncMfaEnrollCookie } from "@/lib/auth/mfa-enrollment"; import { isOidcOnly } from "@/lib/auth/oidc"; import { readPendingLink } from "@/lib/auth/oidc-pending-link"; +/** + * Failed password sign-ins one account takes per window, from any number of + * addresses. Twice the per-IP ceiling, so the owner mistyping from one network + * meets that one first, and small enough that guessing is hopeless. + */ +const LOGIN_ACCOUNT_BUCKET = "auth:login:account"; +const LOGIN_ACCOUNT_LIMIT = 10; +const LOGIN_ACCOUNT_WINDOW_MS = 15 * 60 * 1000; + export const POST = apiHandler(async (request: NextRequest) => { // OIDC_ONLY means password login must be a dead end, not just a hidden // button — otherwise a pre-existing password (or a leaked/reused one) @@ -91,6 +105,42 @@ export const POST = apiHandler(async (request: NextRequest) => { }, }); + // A per-account ceiling beside the per-IP one. The IP bucket alone is only + // as good as the address it is keyed on, and a guesser with many addresses + // (or a proxy that forwards whatever address the caller claims) could try + // one account's password without end. This bucket follows the account + // wherever the guesses come from. + // + // Keyed on the account when the identifier resolves and on the identifier's + // hash when it does not, with the same ceiling, so a locked answer says + // nothing about whether the account exists. Charged before the Argon2id + // verification (a locked account costs no hashing) and refunded on a + // successful sign-in, so it counts failures: the owner signing in does not + // use it up. While it is spent, a passkey or single sign-on still works. + const accountBucket = user + ? `${LOGIN_ACCOUNT_BUCKET}:u:${user.id}` + : `${LOGIN_ACCOUNT_BUCKET}:i:${hashToken(identifier.toLowerCase())}`; + const accountRl = await checkRateLimit( + accountBucket, + LOGIN_ACCOUNT_LIMIT, + LOGIN_ACCOUNT_WINDOW_MS, + ); + if (!accountRl.allowed) { + await auditLog("auth.login.account_throttled", { + userId: user?.id, + ipAddress: ip, + details: { identifierHash }, + }); + annotate({ action: { name: "auth.login.account_throttled" } }); + return NextResponse.json( + { + data: null, + error: "Too many login attempts. Please try again later.", + }, + { status: 429, headers: rateLimitHeaders(accountRl) }, + ); + } + // One verification for every outcome. An unknown identifier and an // account carrying no password hash (a passkey-only account) used to // return here before any hashing, so the three answers cost one indexed @@ -132,6 +182,9 @@ export const POST = apiHandler(async (request: NextRequest) => { }); } + // The password was right: this attempt was not a failed guess. + await refundRateLimit(accountBucket); + const ua = request.headers.get("user-agent"); await auditLog("auth.login.password", {