From 5b2f0b83cc61ff2af9ddf118ece7eda5d402b39e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 14 Dec 2025 14:54:20 +0000 Subject: [PATCH] Consolidate throttle storage to central DB per review feedback Co-authored-by: hbel <7416029+hbel@users.noreply.github.com> --- migrations/0008_tense_gamora.sql | 2 +- src/lib/server/db/central-schema.ts | 11 +- src/lib/server/db/tenant-schema.ts | 15 - .../__tests__/challenge-throttle.test.ts | 24 +- src/lib/server/services/challenge-throttle.ts | 276 +++++------------- .../0008_melted_captain_marvel.sql | 6 - 6 files changed, 96 insertions(+), 238 deletions(-) delete mode 100644 tenant-migrations/0008_melted_captain_marvel.sql diff --git a/migrations/0008_tense_gamora.sql b/migrations/0008_tense_gamora.sql index 871ad38..0ca17d2 100644 --- a/migrations/0008_tense_gamora.sql +++ b/migrations/0008_tense_gamora.sql @@ -1,4 +1,4 @@ -CREATE TABLE "passkey_challenge_throttle" ( +CREATE TABLE "challenge_throttle" ( "id" text PRIMARY KEY NOT NULL, "failed_attempts" integer DEFAULT 0 NOT NULL, "last_attempt_at" timestamp DEFAULT now() NOT NULL, diff --git a/src/lib/server/db/central-schema.ts b/src/lib/server/db/central-schema.ts index a6501a0..c4073ab 100644 --- a/src/lib/server/db/central-schema.ts +++ b/src/lib/server/db/central-schema.ts @@ -230,12 +230,13 @@ export const userInvite = pgTable( ); /** - * Passkey Challenge Throttle table - tracks failed passkey authentication attempts - * Used to implement exponential backoff for failed authentication attempts - * @table passkey_challenge_throttle + * Challenge Throttle table - tracks failed authentication attempts for all challenge types + * Used to implement rate limiting for both PIN and passkey authentication + * Stored centrally to prevent brute force attacks across all tenants + * @table challenge_throttle */ -export const passkeyChallengeThrottle = pgTable("passkey_challenge_throttle", { - /** Primary key - email address of the user attempting authentication */ +export const challengeThrottle = pgTable("challenge_throttle", { + /** Primary key - identifier (email hash for PIN challenges, email for passkey challenges) */ id: text("id").primaryKey(), /** Number of failed attempts */ failedAttempts: integer("failed_attempts").default(0).notNull(), diff --git a/src/lib/server/db/tenant-schema.ts b/src/lib/server/db/tenant-schema.ts index b46b741..cd44d24 100644 --- a/src/lib/server/db/tenant-schema.ts +++ b/src/lib/server/db/tenant-schema.ts @@ -323,21 +323,6 @@ export const authChallenge = pgTable("auth_challenge", { consumed: boolean("consumed").default(false).notNull(), }); -/** - * @table challengeThrottle - * Tracks failed authentication attempts for challenge-based authentication - */ -export const challengeThrottle = pgTable("challenge_throttle", { - /** Primary key - unique identifier (emailHash or email depending on challenge type) */ - id: text("id").primaryKey(), - /** Number of failed attempts */ - failedAttempts: integer("failed_attempts").default(0).notNull(), - /** When the throttle was last updated */ - lastAttemptAt: timestamp("last_attempt_at").defaultNow().notNull(), - /** When the throttle should reset/expire */ - resetAt: timestamp("reset_at").notNull(), -}); - /** StaffCrypto record type for database queries */ export type SelectStaffCrypto = InferSelectModel; diff --git a/src/lib/server/services/__tests__/challenge-throttle.test.ts b/src/lib/server/services/__tests__/challenge-throttle.test.ts index 1b5df83..734e431 100644 --- a/src/lib/server/services/__tests__/challenge-throttle.test.ts +++ b/src/lib/server/services/__tests__/challenge-throttle.test.ts @@ -48,7 +48,7 @@ describe("ChallengeThrottleService", () => { const emailHash = "test-email-hash"; it("should allow request when no throttle record exists", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); (mockDb.limit as any).mockResolvedValue([]); const result = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId); @@ -59,7 +59,7 @@ describe("ChallengeThrottleService", () => { }); it("should allow request when throttle has expired", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); const expiredRecord = { id: emailHash, failedAttempts: 3, @@ -75,7 +75,7 @@ describe("ChallengeThrottleService", () => { }); it("should throttle request when delay has not passed (1st failure)", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); const now = Date.now(); const record = { id: emailHash, @@ -93,7 +93,7 @@ describe("ChallengeThrottleService", () => { }); it("should throttle request when delay has not passed (3rd failure)", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); const now = Date.now(); const record = { id: emailHash, @@ -111,7 +111,7 @@ describe("ChallengeThrottleService", () => { }); it("should allow request when enough time has passed", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); const now = Date.now(); const record = { id: emailHash, @@ -128,7 +128,7 @@ describe("ChallengeThrottleService", () => { }); it("should record first failed attempt", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); (mockDb.limit as any).mockResolvedValue([]); await challengeThrottleService.recordFailedAttempt(emailHash, "pin", tenantId); @@ -143,7 +143,7 @@ describe("ChallengeThrottleService", () => { }); it("should increment failed attempts on subsequent failures", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); const record = { id: emailHash, failedAttempts: 2, @@ -163,7 +163,7 @@ describe("ChallengeThrottleService", () => { }); it("should clear throttle on successful authentication", async () => { - const mockDb = await getTenantDb(tenantId); + const mockDb = getCentralDb(); await challengeThrottleService.clearThrottle(emailHash, "pin", tenantId); @@ -243,16 +243,10 @@ describe("ChallengeThrottleService", () => { }); describe("Edge cases", () => { - it("should throw error when tenantId missing for PIN throttle", async () => { - await expect(challengeThrottleService.checkThrottle("hash", "pin")).rejects.toThrow( - "Tenant ID required", - ); - }); - it("should handle cleanup of expired records", async () => { const mockDb = getCentralDb(); - await challengeThrottleService.cleanupExpired("passkey"); + await challengeThrottleService.cleanupExpired(); expect(mockDb.delete).toHaveBeenCalled(); }); diff --git a/src/lib/server/services/challenge-throttle.ts b/src/lib/server/services/challenge-throttle.ts index c1fdd88..fdda7c6 100644 --- a/src/lib/server/services/challenge-throttle.ts +++ b/src/lib/server/services/challenge-throttle.ts @@ -2,13 +2,13 @@ * Challenge Throttling Service * * Implements throttling for challenge-based authentication to prevent brute force attacks. - * - PIN challenges (appointments): Exponential backoff (2 seconds, 10 seconds, 60 seconds, up to 30 minutes) + * All throttle data is stored centrally in the central database. + * - PIN challenges (appointments): Escalating delays (2s, 10s, 1m, 5m, 30m) * - Passkey challenges (auth): Fixed delay after 3 attempts (1 minute) */ -import { getTenantDb, getCentralDb } from "$lib/server/db"; -import { challengeThrottle } from "$lib/server/db/tenant-schema"; -import { passkeyChallengeThrottle } from "$lib/server/db/central-schema"; +import { getCentralDb } from "$lib/server/db"; +import { challengeThrottle } from "$lib/server/db/central-schema"; import { eq, lt } from "drizzle-orm"; import { logger } from "$lib/logger"; @@ -57,7 +57,7 @@ class ChallengeThrottleService { * Check if a challenge request should be throttled * @param identifier - Email hash for PIN challenges, email for passkey challenges * @param type - Type of challenge (pin or passkey) - * @param tenantId - Tenant ID (required for PIN challenges, optional for passkey) + * @param tenantId - Tenant ID (optional, no longer used but kept for API compatibility) */ async checkThrottle( identifier: string, @@ -65,107 +65,58 @@ class ChallengeThrottleService { tenantId?: string, ): Promise { const now = new Date(); + const db = getCentralDb(); - if (type === "pin") { - if (!tenantId) { - throw new Error("Tenant ID required for PIN challenge throttling"); - } + // Get throttle record from central DB + const records = await db + .select() + .from(challengeThrottle) + .where(eq(challengeThrottle.id, identifier)) + .limit(1); - const db = await getTenantDb(tenantId); + if (records.length === 0) { + // No throttle record, allow request + return { allowed: true, retryAfterMs: 0, failedAttempts: 0 }; + } - // Get throttle record - const records = await db - .select() - .from(challengeThrottle) - .where(eq(challengeThrottle.id, identifier)) - .limit(1); + const record = records[0]; - if (records.length === 0) { - // No throttle record, allow request - return { allowed: true, retryAfterMs: 0, failedAttempts: 0 }; - } + // Check if throttle has expired + if (now > record.resetAt) { + // Throttle expired, clean up and allow + await db.delete(challengeThrottle).where(eq(challengeThrottle.id, identifier)); + return { allowed: true, retryAfterMs: 0, failedAttempts: 0 }; + } - const record = records[0]; + // Calculate how long to wait based on challenge type + const delay = + type === "pin" + ? calculatePinThrottleDelay(record.failedAttempts) + : calculatePasskeyThrottleDelay(record.failedAttempts); + const timeSinceLastAttempt = now.getTime() - record.lastAttemptAt.getTime(); - // Check if throttle has expired - if (now > record.resetAt) { - // Throttle expired, clean up and allow - await db.delete(challengeThrottle).where(eq(challengeThrottle.id, identifier)); - return { allowed: true, retryAfterMs: 0, failedAttempts: 0 }; - } - - // Calculate how long to wait - const delay = calculatePinThrottleDelay(record.failedAttempts); - const timeSinceLastAttempt = now.getTime() - record.lastAttemptAt.getTime(); - - if (timeSinceLastAttempt < delay) { - // Still throttled - return { - allowed: false, - retryAfterMs: delay - timeSinceLastAttempt, - failedAttempts: record.failedAttempts, - }; - } - - // Enough time has passed, allow the request + if (timeSinceLastAttempt < delay) { + // Still throttled return { - allowed: true, - retryAfterMs: 0, - failedAttempts: record.failedAttempts, - }; - } else { - // Passkey throttling (central DB) - const db = getCentralDb(); - - const records = await db - .select() - .from(passkeyChallengeThrottle) - .where(eq(passkeyChallengeThrottle.id, identifier)) - .limit(1); - - if (records.length === 0) { - // No throttle record, allow request - return { allowed: true, retryAfterMs: 0, failedAttempts: 0 }; - } - - const record = records[0]; - - // Check if throttle has expired - if (now > record.resetAt) { - // Throttle expired, clean up and allow - await db - .delete(passkeyChallengeThrottle) - .where(eq(passkeyChallengeThrottle.id, identifier)); - return { allowed: true, retryAfterMs: 0, failedAttempts: 0 }; - } - - // Calculate how long to wait - const delay = calculatePasskeyThrottleDelay(record.failedAttempts); - const timeSinceLastAttempt = now.getTime() - record.lastAttemptAt.getTime(); - - if (timeSinceLastAttempt < delay) { - // Still throttled - return { - allowed: false, - retryAfterMs: delay - timeSinceLastAttempt, - failedAttempts: record.failedAttempts, - }; - } - - // Enough time has passed, allow the request - return { - allowed: true, - retryAfterMs: 0, + allowed: false, + retryAfterMs: delay - timeSinceLastAttempt, failedAttempts: record.failedAttempts, }; } + + // Enough time has passed, allow the request + return { + allowed: true, + retryAfterMs: 0, + failedAttempts: record.failedAttempts, + }; } /** * Record a failed challenge attempt * @param identifier - Email hash for PIN challenges, email for passkey challenges * @param type - Type of challenge (pin or passkey) - * @param tenantId - Tenant ID (required for PIN challenges) + * @param tenantId - Tenant ID (optional, no longer used but kept for API compatibility) */ async recordFailedAttempt( identifier: string, @@ -173,139 +124,72 @@ class ChallengeThrottleService { tenantId?: string, ): Promise { const now = new Date(); + const db = getCentralDb(); - if (type === "pin") { - if (!tenantId) { - throw new Error("Tenant ID required for PIN challenge throttling"); - } + // Try to get existing record + const records = await db + .select() + .from(challengeThrottle) + .where(eq(challengeThrottle.id, identifier)) + .limit(1); - const db = await getTenantDb(tenantId); - - // Try to get existing record - const records = await db - .select() - .from(challengeThrottle) - .where(eq(challengeThrottle.id, identifier)) - .limit(1); - - if (records.length === 0) { - // Create new throttle record - const resetAt = new Date(now.getTime() + THROTTLE_RESET_DURATION_MS); - await db.insert(challengeThrottle).values({ - id: identifier, - failedAttempts: 1, - lastAttemptAt: now, - resetAt, - }); - } else { - // Update existing record - const record = records[0]; - await db - .update(challengeThrottle) - .set({ - failedAttempts: record.failedAttempts + 1, - lastAttemptAt: now, - }) - .where(eq(challengeThrottle.id, identifier)); - } - - logger.info("Recorded failed PIN challenge attempt", { - identifier: identifier.slice(0, 8), - tenantId, + if (records.length === 0) { + // Create new throttle record + const resetAt = new Date(now.getTime() + THROTTLE_RESET_DURATION_MS); + await db.insert(challengeThrottle).values({ + id: identifier, + failedAttempts: 1, + lastAttemptAt: now, + resetAt, }); } else { - // Passkey throttling (central DB) - const db = getCentralDb(); - - const records = await db - .select() - .from(passkeyChallengeThrottle) - .where(eq(passkeyChallengeThrottle.id, identifier)) - .limit(1); - - if (records.length === 0) { - // Create new throttle record - const resetAt = new Date(now.getTime() + THROTTLE_RESET_DURATION_MS); - await db.insert(passkeyChallengeThrottle).values({ - id: identifier, - failedAttempts: 1, + // Update existing record + const record = records[0]; + await db + .update(challengeThrottle) + .set({ + failedAttempts: record.failedAttempts + 1, lastAttemptAt: now, - resetAt, - }); - } else { - // Update existing record - const record = records[0]; - await db - .update(passkeyChallengeThrottle) - .set({ - failedAttempts: record.failedAttempts + 1, - lastAttemptAt: now, - }) - .where(eq(passkeyChallengeThrottle.id, identifier)); - } - - logger.info("Recorded failed passkey challenge attempt", { - identifier: identifier.slice(0, 8), - }); + }) + .where(eq(challengeThrottle.id, identifier)); } + + logger.info(`Recorded failed ${type} challenge attempt`, { + identifier: identifier.slice(0, 8), + type, + }); } /** * Clear throttle for successful authentication * @param identifier - Email hash for PIN challenges, email for passkey challenges * @param type - Type of challenge (pin or passkey) - * @param tenantId - Tenant ID (required for PIN challenges) + * @param tenantId - Tenant ID (optional, no longer used but kept for API compatibility) */ async clearThrottle(identifier: string, type: ThrottleType, tenantId?: string): Promise { - if (type === "pin") { - if (!tenantId) { - throw new Error("Tenant ID required for PIN challenge throttling"); - } + const db = getCentralDb(); + await db.delete(challengeThrottle).where(eq(challengeThrottle.id, identifier)); - const db = await getTenantDb(tenantId); - await db.delete(challengeThrottle).where(eq(challengeThrottle.id, identifier)); - - logger.debug("Cleared PIN challenge throttle", { - identifier: identifier.slice(0, 8), - tenantId, - }); - } else { - const db = getCentralDb(); - await db.delete(passkeyChallengeThrottle).where(eq(passkeyChallengeThrottle.id, identifier)); - - logger.debug("Cleared passkey challenge throttle", { - identifier: identifier.slice(0, 8), - }); - } + logger.debug(`Cleared ${type} challenge throttle`, { + identifier: identifier.slice(0, 8), + type, + }); } /** * Clean up expired throttle records * Should be called periodically */ - async cleanupExpired(type: ThrottleType, tenantId?: string): Promise { + async cleanupExpired(): Promise { const now = new Date(); try { - if (type === "pin") { - if (!tenantId) { - throw new Error("Tenant ID required for PIN challenge throttle cleanup"); - } + const db = getCentralDb(); + await db.delete(challengeThrottle).where(lt(challengeThrottle.resetAt, now)); - const db = await getTenantDb(tenantId); - await db.delete(challengeThrottle).where(lt(challengeThrottle.resetAt, now)); - - logger.debug("Cleaned up expired PIN challenge throttles", { tenantId }); - } else { - const db = getCentralDb(); - await db.delete(passkeyChallengeThrottle).where(lt(passkeyChallengeThrottle.resetAt, now)); - - logger.debug("Cleaned up expired passkey challenge throttles"); - } + logger.debug("Cleaned up expired challenge throttles"); } catch (error) { logger.warn("Failed to cleanup expired throttles", { - type, - tenantId, error: String(error), }); } diff --git a/tenant-migrations/0008_melted_captain_marvel.sql b/tenant-migrations/0008_melted_captain_marvel.sql deleted file mode 100644 index 0ca17d2..0000000 --- a/tenant-migrations/0008_melted_captain_marvel.sql +++ /dev/null @@ -1,6 +0,0 @@ -CREATE TABLE "challenge_throttle" ( - "id" text PRIMARY KEY NOT NULL, - "failed_attempts" integer DEFAULT 0 NOT NULL, - "last_attempt_at" timestamp DEFAULT now() NOT NULL, - "reset_at" timestamp NOT NULL -);