From c8f9470fccdc2b17f5a218bb4ee5884a9442ee46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oleksandr=20Kapitu=C5=82a?= Date: Thu, 5 Nov 2026 17:57:56 +0100 Subject: [PATCH] fix(identity): find the pending 2fa cookie by better-auth's own cookie name Over HTTPS better-auth names the pending challenge cookie `__Secure-better-auth.two_factor`, which the exact match on `better-auth.two_factor` never found. Verify then rejected a code better-auth had already accepted, skipped the lockout count, and keyed every pending attempt on the shared anonymous rate-limit bucket. Derive the name from `createAuthCookie('two_factor')` on the auth context so the cookie prefix and secure prefix match what the two-factor plugin sets, keeping the exact-name match against look-alike cookies. --- .../__tests__/identity.rate-limit.int.test.ts | 29 ++++++++++++++++++- .../__tests__/identity.service.int.test.ts | 29 ++++++++++++++++++- .../pam/identity/service/identity.service.ts | 15 ++++++---- 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/packages/core/src/pam/identity/__tests__/identity.rate-limit.int.test.ts b/packages/core/src/pam/identity/__tests__/identity.rate-limit.int.test.ts index 80de64c72..72cacb9cd 100644 --- a/packages/core/src/pam/identity/__tests__/identity.rate-limit.int.test.ts +++ b/packages/core/src/pam/identity/__tests__/identity.rate-limit.int.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; +import { createCookieGetter } from 'better-auth/cookies'; import { RedisRateLimiter } from '@openora/core/server'; import { createTestDb, createTestRedis, type TestDb, type TestRedis } from '@openora/core/testing'; import type { @@ -34,9 +35,10 @@ function withTemplateRenderer( }); } -const { getSessionMock, signInEmailMock } = vi.hoisted(() => ({ +const { getSessionMock, signInEmailMock, authBaseURL } = vi.hoisted(() => ({ getSessionMock: vi.fn().mockResolvedValue(null), signInEmailMock: vi.fn(), + authBaseURL: { current: undefined as string | undefined }, })); const rejectedOtpResponse = () => @@ -52,6 +54,9 @@ vi.mock('@openora/core/server', async (importOriginal) => { return { ...actual, createAuth: vi.fn(() => ({ + $context: Promise.resolve({ + createAuthCookie: createCookieGetter({ baseURL: authBaseURL.current }), + }), options: { session: {} }, api: { getSession: getSessionMock, @@ -87,6 +92,7 @@ beforeEach(async () => { getSessionMock.mockReset(); getSessionMock.mockResolvedValue(null); signInEmailMock.mockReset(); + authBaseURL.current = undefined; }); describe('IdentityService - rate limiting (real Redis)', () => { @@ -291,6 +297,27 @@ describe('IdentityService - verify2fa rate-limit key stability (ABC-208 finding ).rejects.toMatchObject({ code: 'TOO_MANY_REQUESTS' }); } }); + + it('keys on the __Secure- two_factor cookie value when better-auth runs over HTTPS', async () => { + authBaseURL.current = 'https://api.example.test'; + const limiter = makeLimiter(); + const twoFactorIdentifier = 'pending-2fa-identifier-secure'; + for (let i = 0; i < 5; i++) { + await limiter.consume(`verify2fa:${twoFactorIdentifier}`, { + limit: 5, + windowMs: 5 * 60 * 1000, + }); + } + const svc = withTemplateRenderer({ drizzle, events, limiter }); + + await expect( + svc.verifyTwoFactor( + { code: '123456', method: 'totp', trustDevice: false }, + { cookie: `__Secure-better-auth.two_factor=${twoFactorIdentifier}` }, + new Headers(), + ), + ).rejects.toMatchObject({ code: 'TOO_MANY_REQUESTS' }); + }); }); describe('IdentityService - rate limiting on secret-guessing routes (ABC-208 finding #6)', () => { diff --git a/packages/core/src/pam/identity/__tests__/identity.service.int.test.ts b/packages/core/src/pam/identity/__tests__/identity.service.int.test.ts index d38fb495b..bde2e56e5 100644 --- a/packages/core/src/pam/identity/__tests__/identity.service.int.test.ts +++ b/packages/core/src/pam/identity/__tests__/identity.service.int.test.ts @@ -1,6 +1,7 @@ import { randomUUID } from 'node:crypto'; import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; import { eq, sql } from 'drizzle-orm'; +import { createCookieGetter } from 'better-auth/cookies'; import { ORPCError } from '@orpc/server'; import type { DrizzleTx } from '@openora/core/server'; import { @@ -54,6 +55,7 @@ const { changePasswordMock, capturedAuthOptions, authTx, + authBaseURL, } = vi.hoisted(() => ({ signInEmailMock: vi.fn(), verifyTotpMock: vi.fn(), @@ -73,6 +75,7 @@ const { confirmEmailChangeMock: vi.fn(), changePasswordMock: vi.fn(), authTx: { current: undefined as DrizzleTx | undefined }, + authBaseURL: { current: undefined as string | undefined }, capturedAuthOptions: { current: undefined as | { @@ -92,7 +95,10 @@ vi.mock('@openora/core/server', async (importOriginal) => ({ createAuth: vi.fn((options) => { capturedAuthOptions.current = options; return { - $context: Promise.resolve({ password: { verify: passwordVerifyMock } }), + $context: Promise.resolve({ + password: { verify: passwordVerifyMock }, + createAuthCookie: createCookieGetter({ baseURL: authBaseURL.current }), + }), options: { session: { expiresIn: SESSION_DURATION_IN_SECONDS, @@ -210,6 +216,7 @@ beforeEach(async () => { requestEmailChangeMock.mockReset(); confirmEmailChangeMock.mockReset(); changePasswordMock.mockReset(); + authBaseURL.current = undefined; await db.drizzle.db.execute( sql`TRUNCATE ${user}, ${session}, ${player} RESTART IDENTITY CASCADE`, ); @@ -1506,6 +1513,26 @@ describe('IdentityService.verifyTwoFactor', () => { ); }); + it('binds an HTTPS challenge to the account behind the __Secure- pending cookie', async () => { + authBaseURL.current = 'https://api.example.test'; + const account = await seedUser(); + const { cookieValue, lockout } = await realPendingChallenge(account.id); + verifyTotpMock.mockResolvedValue(okResponse(account.id)); + const events = makeEventBus(); + + const result = await buildService({ events, twoFactorLockout: lockout }).verifyTwoFactor( + { code: '123456', method: 'totp', trustDevice: false }, + { cookie: `a.two_factor=junk; __Secure-better-auth.two_factor=${cookieValue}` }, + new Headers(), + ); + + expect(result).toEqual({ success: true, trustGranted: false }); + expect(events.emit).toHaveBeenCalledWith( + 'identity.2fa.verified', + expect.objectContaining({ userId: account.id }), + ); + }); + it('revokes a minted session when Better Auth verifies a different account than the pending challenge', async () => { const challengedAccount = await seedUser(); const otherAccount = await seedUser({ email: 'other@b.dev' }); diff --git a/packages/core/src/pam/identity/service/identity.service.ts b/packages/core/src/pam/identity/service/identity.service.ts index 060402574..0cf882af9 100644 --- a/packages/core/src/pam/identity/service/identity.service.ts +++ b/packages/core/src/pam/identity/service/identity.service.ts @@ -100,7 +100,6 @@ import { assertFreshReauthentication, } from './fresh-reauthentication.service.js'; -const TWO_FACTOR_PENDING_COOKIE_NAME = 'better-auth.two_factor'; const TwoFactorVerificationResponseSchema = z.object({ token: z.string(), user: z.object({ id: z.string() }).optional(), @@ -183,13 +182,13 @@ function makeLoginIpRateLimitKey(ip: string): `login-ip:${string}` { // Read only Better Auth's pending-cookie VALUE, never a similarly suffixed cookie or the // raw Cookie header: unrelated cookie pairs must not change the challenge identity. -function twoFactorPendingCookieValue(headers: Headers): string | undefined { +function twoFactorPendingCookieValue(headers: Headers, cookieName: string): string | undefined { const cookieHeader = headers.get('cookie'); if (!cookieHeader) { return undefined; } for (const [name, value] of parseCookies(cookieHeader)) { - if (name === TWO_FACTOR_PENDING_COOKIE_NAME) { + if (name === cookieName) { return value; } } @@ -1654,7 +1653,7 @@ export class IdentityService { */ async sendTwoFactorOtp(reqHeaders: NodeHeaders, resHeaders: Headers) { const headers = nodeHeadersToHeaders(reqHeaders); - const pendingCookie = twoFactorPendingCookieValue(headers); + const pendingCookie = await this.twoFactorPendingCookie(headers); const userId = (await this.currentUserId(headers)) ?? (await this.twoFactorLockout?.resolvePendingUserId(pendingCookie)); @@ -1822,6 +1821,12 @@ export class IdentityService { return row?.requireTwoFactorOnLogin ?? false; } + private async twoFactorPendingCookie(headers: Headers) { + // The two-factor plugin's own cookie key; it does not export the constant. + const { name } = (await this.auth.$context).createAuthCookie('two_factor'); + return twoFactorPendingCookieValue(headers, name); + } + private async accountRole(userId: User['id']) { const [row] = await this.drizzle.db .select({ role: user.role }) @@ -1834,7 +1839,7 @@ export class IdentityService { async verifyTwoFactor(input: Verify2faInput, reqHeaders: NodeHeaders, resHeaders: Headers) { const { ip, userAgent } = extractClientMeta(reqHeaders); const headers = nodeHeadersToHeaders(reqHeaders); - const pendingCookie = twoFactorPendingCookieValue(headers); + const pendingCookie = await this.twoFactorPendingCookie(headers); const sessionUserId = await this.currentUserId(headers); const twoFactorKey = sessionUserId ?? pendingCookie ?? 'anonymous'; await assertRateLimit(this.limiter, `verify2fa:${twoFactorKey}`, VERIFY_2FA_RATE_LIMIT);