fix(two-factor): backup codes shouldn't be encrypted twice (#5202)

This commit is contained in:
Bereket Engida
2025-10-09 17:55:22 -07:00
committed by GitHub
parent 3e013eea62
commit 130184f05d
3 changed files with 66 additions and 85 deletions

View File

@@ -11,7 +11,7 @@ import type {
import { APIError } from "better-call"; import { APIError } from "better-call";
import { TWO_FACTOR_ERROR_CODES } from "../error-code"; import { TWO_FACTOR_ERROR_CODES } from "../error-code";
import { verifyTwoFactor } from "../verify-two-factor"; import { verifyTwoFactor } from "../verify-two-factor";
import type { GenericEndpointContext } from "@better-auth/core"; import { safeJSONParse } from "../../../utils/json";
export interface BackupCodeOptions { export interface BackupCodeOptions {
/** /**
@@ -53,17 +53,33 @@ export async function generateBackupCodes(
secret: string, secret: string,
options?: BackupCodeOptions, options?: BackupCodeOptions,
) { ) {
const key = secret;
const backupCodes = options?.customBackupCodesGenerate const backupCodes = options?.customBackupCodesGenerate
? options.customBackupCodesGenerate() ? options.customBackupCodesGenerate()
: generateBackupCodesFn(options); : generateBackupCodesFn(options);
const encCodes = await symmetricEncrypt({ if (options?.storeBackupCodes === "encrypted") {
data: JSON.stringify(backupCodes), const encCodes = await symmetricEncrypt({
key: key, data: JSON.stringify(backupCodes),
}); key: secret,
});
return {
backupCodes,
encryptedBackupCodes: encCodes,
};
}
if (
typeof options?.storeBackupCodes === "object" &&
"encrypt" in options?.storeBackupCodes
) {
return {
backupCodes,
encryptedBackupCodes: await options?.storeBackupCodes.encrypt(
JSON.stringify(backupCodes),
),
};
}
return { return {
backupCodes, backupCodes,
encryptedBackupCodes: encCodes, encryptedBackupCodes: JSON.stringify(backupCodes),
}; };
} }
@@ -73,8 +89,9 @@ export async function verifyBackupCode(
code: string; code: string;
}, },
key: string, key: string,
options?: BackupCodeOptions,
) { ) {
const codes = await getBackupCodes(data.backupCodes, key); const codes = await getBackupCodes(data.backupCodes, key, options);
if (!codes) { if (!codes) {
return { return {
status: false, status: false,
@@ -87,61 +104,29 @@ export async function verifyBackupCode(
}; };
} }
export async function getBackupCodes(backupCodes: string, key: string) { export async function getBackupCodes(
const secret = new TextDecoder("utf-8").decode( backupCodes: string,
new TextEncoder().encode( key: string,
await symmetricDecrypt({ key, data: backupCodes }), options?: BackupCodeOptions,
), ) {
); if (options?.storeBackupCodes === "encrypted") {
const data = JSON.parse(secret); const decrypted = await symmetricDecrypt({ key, data: backupCodes });
const result = z.array(z.string()).safeParse(data); return safeJSONParse<string[]>(decrypted);
if (result.success) {
return result.data;
} }
return null; if (
typeof options?.storeBackupCodes === "object" &&
"decrypt" in options?.storeBackupCodes
) {
const decrypted = await options?.storeBackupCodes.decrypt(backupCodes);
return safeJSONParse<string[]>(decrypted);
}
return safeJSONParse<string[]>(backupCodes);
} }
export const backupCode2fa = (options?: BackupCodeOptions) => { export const backupCode2fa = (opts: BackupCodeOptions) => {
const twoFactorTable = "twoFactor"; const twoFactorTable = "twoFactor";
async function storeBackupCodes(
ctx: GenericEndpointContext,
backupCodes: string,
) {
if (options?.storeBackupCodes === "encrypted") {
return await symmetricEncrypt({
key: ctx.context.secret,
data: backupCodes,
});
}
if (
typeof options?.storeBackupCodes === "object" &&
"encrypt" in options?.storeBackupCodes
) {
return await options?.storeBackupCodes.encrypt(backupCodes);
}
return backupCodes;
}
async function decryptBackupCodes(
ctx: GenericEndpointContext,
backupCodes: string,
) {
if (options?.storeBackupCodes === "encrypted") {
return await symmetricDecrypt({
key: ctx.context.secret,
data: backupCodes,
});
}
if (
typeof options?.storeBackupCodes === "object" &&
"decrypt" in options?.storeBackupCodes
) {
return await options?.storeBackupCodes.decrypt(backupCodes);
}
return backupCodes;
}
return { return {
id: "backup_code", id: "backup_code",
endpoints: { endpoints: {
@@ -319,16 +304,13 @@ export const backupCode2fa = (options?: BackupCodeOptions) => {
message: TWO_FACTOR_ERROR_CODES.BACKUP_CODES_NOT_ENABLED, message: TWO_FACTOR_ERROR_CODES.BACKUP_CODES_NOT_ENABLED,
}); });
} }
const decryptedBackupCodes = await decryptBackupCodes(
ctx,
twoFactor.backupCodes,
);
const validate = await verifyBackupCode( const validate = await verifyBackupCode(
{ {
backupCodes: decryptedBackupCodes, backupCodes: twoFactor.backupCodes,
code: ctx.body.code, code: ctx.body.code,
}, },
ctx.context.secret, ctx.context.secret,
opts,
); );
if (!validate.status) { if (!validate.status) {
throw new APIError("UNAUTHORIZED", { throw new APIError("UNAUTHORIZED", {
@@ -439,16 +421,13 @@ export const backupCode2fa = (options?: BackupCodeOptions) => {
await ctx.context.password.checkPassword(user.id, ctx); await ctx.context.password.checkPassword(user.id, ctx);
const backupCodes = await generateBackupCodes( const backupCodes = await generateBackupCodes(
ctx.context.secret, ctx.context.secret,
options, opts,
);
const storedBackupCodes = await storeBackupCodes(
ctx,
backupCodes.encryptedBackupCodes,
); );
await ctx.context.adapter.update({ await ctx.context.adapter.update({
model: twoFactorTable, model: twoFactorTable,
update: { update: {
backupCodes: storedBackupCodes, backupCodes: backupCodes.encryptedBackupCodes,
}, },
where: [ where: [
{ {
@@ -503,25 +482,23 @@ export const backupCode2fa = (options?: BackupCodeOptions) => {
}); });
if (!twoFactor) { if (!twoFactor) {
throw new APIError("BAD_REQUEST", { throw new APIError("BAD_REQUEST", {
message: "Backup codes aren't enabled", message: TWO_FACTOR_ERROR_CODES.BACKUP_CODES_NOT_ENABLED,
}); });
} }
const decryptedBackupCodes = await decryptBackupCodes( const decryptedBackupCodes = await getBackupCodes(
ctx,
twoFactor.backupCodes, twoFactor.backupCodes,
);
const backupCodes = await getBackupCodes(
decryptedBackupCodes,
ctx.context.secret, ctx.context.secret,
opts,
); );
if (!backupCodes) {
if (!decryptedBackupCodes) {
throw new APIError("BAD_REQUEST", { throw new APIError("BAD_REQUEST", {
message: TWO_FACTOR_ERROR_CODES.BACKUP_CODES_NOT_ENABLED, message: TWO_FACTOR_ERROR_CODES.INVALID_BACKUP_CODE,
}); });
} }
return ctx.json({ return ctx.json({
status: true, status: true,
backupCodes, backupCodes: decryptedBackupCodes,
}); });
}, },
), ),

View File

@@ -7,7 +7,11 @@ import {
import { sessionMiddleware } from "../../api"; import { sessionMiddleware } from "../../api";
import { symmetricEncrypt } from "../../crypto"; import { symmetricEncrypt } from "../../crypto";
import type { BetterAuthPlugin } from "@better-auth/core"; import type { BetterAuthPlugin } from "@better-auth/core";
import { backupCode2fa, generateBackupCodes } from "./backup-codes"; import {
backupCode2fa,
generateBackupCodes,
type BackupCodeOptions,
} from "./backup-codes";
import { otp2fa } from "./otp"; import { otp2fa } from "./otp";
import { totp2fa } from "./totp"; import { totp2fa } from "./totp";
import type { TwoFactorOptions, UserWithTwoFactor } from "./types"; import type { TwoFactorOptions, UserWithTwoFactor } from "./types";
@@ -27,8 +31,12 @@ export const twoFactor = (options?: TwoFactorOptions) => {
const opts = { const opts = {
twoFactorTable: "twoFactor", twoFactorTable: "twoFactor",
}; };
const backupCodeOptions = {
storeBackupCodes: "encrypted",
...options?.backupCodeOptions,
} satisfies BackupCodeOptions;
const totp = totp2fa(options?.totpOptions); const totp = totp2fa(options?.totpOptions);
const backupCode = backupCode2fa(options?.backupCodeOptions); const backupCode = backupCode2fa(backupCodeOptions);
const otp = otp2fa(options?.otpOptions); const otp = otp2fa(options?.otpOptions);
return { return {
@@ -120,7 +128,7 @@ export const twoFactor = (options?: TwoFactorOptions) => {
}); });
const backupCodes = await generateBackupCodes( const backupCodes = await generateBackupCodes(
ctx.context.secret, ctx.context.secret,
options?.backupCodeOptions, backupCodeOptions,
); );
if (options?.skipVerificationOnEnable) { if (options?.skipVerificationOnEnable) {
const updatedUser = await ctx.context.internalAdapter.updateUser( const updatedUser = await ctx.context.internalAdapter.updateUser(

View File

@@ -539,9 +539,6 @@ describe("two factor auth API", async () => {
}); });
}); });
// Regression tests for PR #5174 - viewBackupCodes endpoint
// Bug: viewBackupCodes was returning encrypted JSON string instead of parsed array
// This caused "SyntaxError: Unexpected number in JSON" errors
describe("view backup codes", async () => { describe("view backup codes", async () => {
const sendOTP = vi.fn(); const sendOTP = vi.fn();
const { auth, signInWithTestUser, testUser, db } = await getTestInstance({ const { auth, signInWithTestUser, testUser, db } = await getTestInstance({
@@ -580,7 +577,6 @@ describe("view backup codes", async () => {
body: { userId }, body: { userId },
}); });
// Critical: Verify it returns an array, NOT a JSON string (the bug that was fixed)
expect(typeof viewResult.backupCodes).not.toBe("string"); expect(typeof viewResult.backupCodes).not.toBe("string");
expect(Array.isArray(viewResult.backupCodes)).toBe(true); expect(Array.isArray(viewResult.backupCodes)).toBe(true);
expect(viewResult.backupCodes.length).toBe(10); expect(viewResult.backupCodes.length).toBe(10);