From df1f5cd6a4491cbb758d8bb3b8dc32e23bea9783 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 1 Nov 2026 00:05:09 +0100 Subject: [PATCH] fix: give a pair step-up attempt back only to the windows it was counted in #1066 --- app/api/auth/pair/create/route.ts | 7 ++++--- lib/__tests__/shared-auth-budgets.test.ts | 12 ++++++++++++ lib/auth/pair-attempts.ts | 24 +++++++++++++++++++---- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/app/api/auth/pair/create/route.ts b/app/api/auth/pair/create/route.ts index 79dc777f4..45b9b04ce 100644 --- a/app/api/auth/pair/create/route.ts +++ b/app/api/auth/pair/create/route.ts @@ -5,7 +5,7 @@ import { createPairingCode, isGrantAvailable, stashGrant } from '@/lib/auth/pair import { pairingOwner, readPairReauthFromStore, setPairReauthInStore } from '@/lib/auth/pair-reauth'; import { mintGrantWithPassword } from '@/lib/auth/pair-grant'; import { appAcceptsServerUrl } from '@/lib/auth/pair-bundle'; -import { beginPairStepUp, pairAttemptKey, settlePairStepUp } from '@/lib/auth/pair-attempts'; +import { pairAttemptKey, reservePairStepUp, settlePairStepUp } from '@/lib/auth/pair-attempts'; import { hasSessionSecret } from '@/lib/auth/session-secret'; import { readStalwartAuthContextFromStore } from '@/lib/stalwart/auth-context'; import { isTrustedJmapServerUrl } from '@/lib/stalwart/server-fetch'; @@ -134,7 +134,8 @@ export async function POST(request: NextRequest) { // was written under an identity address. const accountName = context.accountName ?? context.username; const attemptKey = pairAttemptKey(serverUrl, accountName); - if (!(await beginPairStepUp(attemptKey))) { + const attempt = await reservePairStepUp(attemptKey); + if (!attempt) { return NextResponse.json({ error: 'too_many_attempts' }, { status: 429 }); } const totp = typeof body?.totp === 'string' && body.totp.trim() ? body.totp.trim() : undefined; @@ -152,7 +153,7 @@ export async function POST(request: NextRequest) { }); if (!result.ok) { const wrong = result.error === 'invalid_credentials' || (result.error === 'totp_required' && !!totp); - await settlePairStepUp(attemptKey, wrong ? 'wrong_credential' : 'other').catch(settleFailed); + await settlePairStepUp(attemptKey, wrong ? 'wrong_credential' : 'other', Date.now(), attempt).catch(settleFailed); return NextResponse.json({ error: result.error }, { status: STEP_UP_ERRORS[result.error] }); } await settlePairStepUp(attemptKey, 'success').catch(settleFailed); diff --git a/lib/__tests__/shared-auth-budgets.test.ts b/lib/__tests__/shared-auth-budgets.test.ts index 0283f6026..209acb264 100644 --- a/lib/__tests__/shared-auth-budgets.test.ts +++ b/lib/__tests__/shared-auth-budgets.test.ts @@ -106,6 +106,18 @@ describe('sign-in budgets with a shared store', () => { expect(await (await attempts()).beginPairStepUp(key, T0 + 30)).toBe(false); }); + it('gives an attempt back only to the windows it was counted in', async () => { + const key = 'https://mail.example.org|carol@example.org'; + const early = await (await attempts()).reservePairStepUp(key, T0); + expect(early).not.toBeNull(); + // The 15-minute window has reset; five new attempts fill the next one. + const later = T0 + 16 * 60 * 1000; + for (let i = 0; i < 5; i++) expect(await (await attempts()).beginPairStepUp(key, later)).toBe(true); + // The early attempt turns out not to count: the new window keeps its five. + await (await attempts()).settlePairStepUp(key, 'other', later, early!); + expect(await (await attempts()).beginPairStepUp(key, later)).toBe(false); + }); + it('caps pair step-ups at 20 a day across replicas', async () => { const key = 'https://mail.example.org|bob@example.org'; let now = T0; diff --git a/lib/auth/pair-attempts.ts b/lib/auth/pair-attempts.ts index 62aa1c678..3199cbc1a 100644 --- a/lib/auth/pair-attempts.ts +++ b/lib/auth/pair-attempts.ts @@ -113,16 +113,30 @@ function recordFailureLocal(key: string, now: number): void { * attempt is counted as a failure until {@link settlePairStepUp} says better. */ export async function beginPairStepUp(key: string, now = Date.now()): Promise { + return (await reservePairStepUp(key, now)) !== null; +} + +/** A counted step-up attempt: which windows it was counted in (shared store). */ +export interface PairStepUpTicket { + resetAts?: number[]; +} + +/** + * {@link beginPairStepUp}, returning the attempt (null when locked out), so + * {@link settlePairStepUp} gives it back only to the windows it was counted in. + */ +export async function reservePairStepUp(key: string, now = Date.now()): Promise { if (isSharedMode()) { // Check and count in one step, so parallel attempts on several replicas // can't all pass before the first one is counted. - return (await getStore().counters.reserve(sharedLimits(key), now)).ok; + const result = await getStore().counters.reserve(sharedLimits(key), now); + return result.ok ? { resetAts: result.resetAts } : null; } // Check and count with nothing in between, as parallel requests would // otherwise all pass the check. - if (isLockedLocal(key, now)) return false; + if (isLockedLocal(key, now)) return null; recordFailureLocal(key, now); - return true; + return {}; } /** @@ -134,6 +148,7 @@ export async function settlePairStepUp( key: string, outcome: 'success' | 'wrong_credential' | 'other', now = Date.now(), + ticket?: PairStepUpTicket, ): Promise { if (outcome === 'success') { await clearPairStepUpFailures(key); @@ -141,7 +156,8 @@ export async function settlePairStepUp( } if (outcome === 'other') { if (isSharedMode()) { - await getStore().counters.refund(sharedLimits(key).map((limit) => ({ key: limit.key })), now); + // A window that has reset since the attempt was counted is left alone. + await getStore().counters.refund(sharedLimits(key).map((limit, i) => ({ key: limit.key, resetAt: ticket?.resetAts?.[i] })), now); return; } const entry = store().get(key);