Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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 = () =>
Expand All @@ -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,
Expand Down Expand Up @@ -87,6 +92,7 @@ beforeEach(async () => {
getSessionMock.mockReset();
getSessionMock.mockResolvedValue(null);
signInEmailMock.mockReset();
authBaseURL.current = undefined;
});

describe('IdentityService - rate limiting (real Redis)', () => {
Expand Down Expand Up @@ -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)', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -54,6 +55,7 @@ const {
changePasswordMock,
capturedAuthOptions,
authTx,
authBaseURL,
} = vi.hoisted(() => ({
signInEmailMock: vi.fn(),
verifyTotpMock: vi.fn(),
Expand All @@ -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
| {
Expand All @@ -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,
Expand Down Expand Up @@ -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`,
);
Expand Down Expand Up @@ -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' });
Expand Down
15 changes: 10 additions & 5 deletions packages/core/src/pam/identity/service/identity.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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 })
Expand All @@ -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);
Expand Down
Loading