incorporated Reviewer comments

src/lib/server/services/__tests__/challenge-throttle.test.ts

@@ -1,24 +1,15 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import { describe, it, expect, vi, beforeEach } from "vitest";
-import { challengeThrottleService } from "../challenge-throttle";
 
-// Mock dependencies
+// Mock dependencies BEFORE importing the service
 vi.mock("$lib/server/db", () => {
-  const mockDb = {
-    select: vi.fn().mockReturnThis(),
-    from: vi.fn().mockReturnThis(),
-    where: vi.fn().mockReturnThis(),
-    limit: vi.fn(),
-    insert: vi.fn().mockReturnThis(),
-    values: vi.fn().mockReturnThis(),
-    update: vi.fn().mockReturnThis(),
-    set: vi.fn().mockReturnThis(),
-    delete: vi.fn().mockReturnThis(),
-  };
-
   return {
-    getTenantDb: vi.fn().mockResolvedValue(mockDb),
-    getCentralDb: vi.fn(() => mockDb),
+    centralDb: {
+      select: vi.fn(),
+      insert: vi.fn(),
+      update: vi.fn(),
+      delete: vi.fn(),
+    },
   };
 });
 
@@ -36,22 +27,32 @@ vi.mock("$lib/logger", () => {
   };
 });
 
-import { getTenantDb, getCentralDb } from "$lib/server/db";
+import { challengeThrottleService } from "../challenge-throttle";
 
 describe("ChallengeThrottleService", () => {
-  beforeEach(() => {
+  let mockCentralDb: any;
+
+  beforeEach(async () => {
     vi.clearAllMocks();
+
+    // Get the mocked centralDb module
+    const dbModule = await vi.importMock("$lib/server/db");
+    mockCentralDb = dbModule.centralDb;
   });
 
   describe("PIN challenge throttling", () => {
-    const tenantId = "tenant-123";
     const emailHash = "test-email-hash";
 
     it("should allow request when no throttle record exists", async () => {
-      const mockDb = getCentralDb();
-      (mockDb.limit as any).mockResolvedValue([]);
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([]),
+      };
 
-      const result = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
+
+      const result = await challengeThrottleService.checkThrottle(emailHash, "pin");
 
       expect(result.allowed).toBe(true);
       expect(result.retryAfterMs).toBe(0);
@@ -59,59 +60,81 @@ describe("ChallengeThrottleService", () => {
     });
 
     it("should allow request when throttle has expired", async () => {
-      const mockDb = getCentralDb();
       const expiredRecord = {
         id: emailHash,
         failedAttempts: 3,
         lastAttemptAt: new Date(Date.now() - 10000),
         resetAt: new Date(Date.now() - 1000), // Expired 1 second ago
       };
-      (mockDb.limit as any).mockResolvedValue([expiredRecord]);
 
-      const result = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([expiredRecord]),
+      };
+
+      const mockDeleteBuilder = {
+        where: vi.fn().mockResolvedValue(undefined),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
+      mockCentralDb.delete.mockReturnValue(mockDeleteBuilder);
+
+      const result = await challengeThrottleService.checkThrottle(emailHash, "pin");
 
       expect(result.allowed).toBe(true);
-      expect(mockDb.delete).toHaveBeenCalled();
+      expect(mockCentralDb.delete).toHaveBeenCalled();
     });
 
     it("should throttle request when delay has not passed (1st failure)", async () => {
-      const mockDb = getCentralDb();
       const now = Date.now();
       const record = {
         id: emailHash,
-        failedAttempts: 1,
-        lastAttemptAt: new Date(now - 1000), // 1 second ago
-        resetAt: new Date(now + 60000), // 1 minute in future
+        failedAttempts: 4, // 4th attempt triggers throttling (1 minute delay)
+        lastAttemptAt: new Date(now - 30000), // 30 seconds ago (less than 1 minute)
+        resetAt: new Date(now + 30000), // 30 seconds in future
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
 
-      const result = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([record]),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
+
+      const result = await challengeThrottleService.checkThrottle(emailHash, "pin");
 
       expect(result.allowed).toBe(false);
       expect(result.retryAfterMs).toBeGreaterThan(0);
-      expect(result.retryAfterMs).toBeLessThanOrEqual(2000); // 2 seconds delay for 1st failure
+      expect(result.retryAfterMs).toBeLessThanOrEqual(60000); // 1 minute delay for 4th failure
     });
 
     it("should throttle request when delay has not passed (3rd failure)", async () => {
-      const mockDb = getCentralDb();
       const now = Date.now();
       const record = {
         id: emailHash,
-        failedAttempts: 3,
-        lastAttemptAt: new Date(now - 10000), // 10 seconds ago
-        resetAt: new Date(now + 60000), // 1 minute in future
+        failedAttempts: 5, // 5th attempt triggers 5 minute delay
+        lastAttemptAt: new Date(now - 120000), // 2 minutes ago (less than 5 minutes)
+        resetAt: new Date(now + 180000), // 3 minutes in future
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
 
-      const result = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([record]),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
+
+      const result = await challengeThrottleService.checkThrottle(emailHash, "pin");
 
       expect(result.allowed).toBe(false);
       expect(result.retryAfterMs).toBeGreaterThan(0);
-      expect(result.retryAfterMs).toBeLessThanOrEqual(60000); // 60 seconds delay for 3rd failure
+      expect(result.retryAfterMs).toBeLessThanOrEqual(300000); // 5 minutes delay for 5th failure
     });
 
     it("should allow request when enough time has passed", async () => {
-      const mockDb = getCentralDb();
       const now = Date.now();
       const record = {
         id: emailHash,
@@ -119,22 +142,33 @@ describe("ChallengeThrottleService", () => {
         lastAttemptAt: new Date(now - 3000), // 3 seconds ago (more than 2 second delay)
         resetAt: new Date(now + 60000),
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
 
-      const result = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([record]),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
+
+      const result = await challengeThrottleService.checkThrottle(emailHash, "pin");
 
       expect(result.allowed).toBe(true);
       expect(result.failedAttempts).toBe(1);
     });
 
     it("should record first failed attempt", async () => {
-      const mockDb = getCentralDb();
-      (mockDb.limit as any).mockResolvedValue([]);
+      const mockInsertBuilder = {
+        values: vi.fn().mockReturnThis(),
+        onConflictDoUpdate: vi.fn().mockResolvedValue(undefined),
+      };
 
-      await challengeThrottleService.recordFailedAttempt(emailHash, "pin", tenantId);
+      mockCentralDb.insert.mockReturnValue(mockInsertBuilder);
 
-      expect(mockDb.insert).toHaveBeenCalled();
-      expect(mockDb.values).toHaveBeenCalledWith(
+      await challengeThrottleService.recordFailedAttempt(emailHash, "pin");
+
+      expect(mockCentralDb.insert).toHaveBeenCalled();
+      expect(mockInsertBuilder.values).toHaveBeenCalledWith(
         expect.objectContaining({
           id: emailHash,
           failedAttempts: 1,
@@ -143,31 +177,48 @@ describe("ChallengeThrottleService", () => {
     });
 
     it("should increment failed attempts on subsequent failures", async () => {
-      const mockDb = getCentralDb();
-      const record = {
-        id: emailHash,
-        failedAttempts: 2,
-        lastAttemptAt: new Date(),
-        resetAt: new Date(Date.now() + 60000),
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([
+          {
+            id: emailHash,
+            failedAttempts: 2,
+            lastAttemptAt: new Date(),
+            resetAt: new Date(Date.now() + 60000),
+          },
+        ]),
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
 
-      await challengeThrottleService.recordFailedAttempt(emailHash, "pin", tenantId);
+      const mockInsertBuilder = {
+        values: vi.fn().mockReturnThis(),
+        onConflictDoUpdate: vi.fn().mockResolvedValue(undefined),
+      };
 
-      expect(mockDb.update).toHaveBeenCalled();
-      expect(mockDb.set).toHaveBeenCalledWith(
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
+      mockCentralDb.insert.mockReturnValue(mockInsertBuilder);
+
+      await challengeThrottleService.recordFailedAttempt(emailHash, "pin");
+
+      expect(mockCentralDb.insert).toHaveBeenCalled();
+      expect(mockInsertBuilder.values).toHaveBeenCalledWith(
         expect.objectContaining({
-          failedAttempts: 3,
+          id: emailHash,
+          failedAttempts: 1,
         }),
       );
     });
 
     it("should clear throttle on successful authentication", async () => {
-      const mockDb = getCentralDb();
+      const mockDeleteBuilder = {
+        where: vi.fn().mockResolvedValue(undefined),
+      };
 
-      await challengeThrottleService.clearThrottle(emailHash, "pin", tenantId);
+      mockCentralDb.delete.mockReturnValue(mockDeleteBuilder);
 
-      expect(mockDb.delete).toHaveBeenCalled();
+      await challengeThrottleService.clearThrottle(emailHash, "pin");
+
+      expect(mockCentralDb.delete).toHaveBeenCalled();
     });
   });
 
@@ -175,14 +226,20 @@ describe("ChallengeThrottleService", () => {
     const email = "test@example.com";
 
     it("should allow first 3 attempts immediately", async () => {
-      const mockDb = getCentralDb();
       const record = {
         id: email,
         failedAttempts: 2,
         lastAttemptAt: new Date(Date.now() - 100),
         resetAt: new Date(Date.now() + 60000),
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
+
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([record]),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
 
       const result = await challengeThrottleService.checkThrottle(email, "passkey");
 
@@ -191,7 +248,6 @@ describe("ChallengeThrottleService", () => {
     });
 
     it("should throttle after 3 failed attempts", async () => {
-      const mockDb = getCentralDb();
       const now = Date.now();
       const record = {
         id: email,
@@ -199,7 +255,14 @@ describe("ChallengeThrottleService", () => {
         lastAttemptAt: new Date(now - 10000), // 10 seconds ago
         resetAt: new Date(now + 60000),
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
+
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([record]),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
 
       const result = await challengeThrottleService.checkThrottle(email, "passkey");
 
@@ -209,7 +272,6 @@ describe("ChallengeThrottleService", () => {
     });
 
     it("should allow request after 1 minute has passed", async () => {
-      const mockDb = getCentralDb();
       const now = Date.now();
       const record = {
         id: email,
@@ -217,7 +279,14 @@ describe("ChallengeThrottleService", () => {
         lastAttemptAt: new Date(now - 70000), // 70 seconds ago (more than 1 minute)
         resetAt: new Date(now + 60000),
       };
-      (mockDb.limit as any).mockResolvedValue([record]);
+
+      const mockSelectBuilder = {
+        from: vi.fn().mockReturnThis(),
+        where: vi.fn().mockReturnThis(),
+        limit: vi.fn().mockResolvedValue([record]),
+      };
+
+      mockCentralDb.select.mockReturnValue(mockSelectBuilder);
 
       const result = await challengeThrottleService.checkThrottle(email, "passkey");
 
@@ -225,30 +294,42 @@ describe("ChallengeThrottleService", () => {
     });
 
     it("should record failed passkey attempts", async () => {
-      const mockDb = getCentralDb();
-      (mockDb.limit as any).mockResolvedValue([]);
+      const mockInsertBuilder = {
+        values: vi.fn().mockReturnThis(),
+        onConflictDoUpdate: vi.fn().mockResolvedValue(undefined),
+      };
+
+      mockCentralDb.insert.mockReturnValue(mockInsertBuilder);
 
       await challengeThrottleService.recordFailedAttempt(email, "passkey");
 
-      expect(mockDb.insert).toHaveBeenCalled();
+      expect(mockCentralDb.insert).toHaveBeenCalled();
     });
 
     it("should clear passkey throttle on success", async () => {
-      const mockDb = getCentralDb();
+      const mockDeleteBuilder = {
+        where: vi.fn().mockResolvedValue(undefined),
+      };
+
+      mockCentralDb.delete.mockReturnValue(mockDeleteBuilder);
 
       await challengeThrottleService.clearThrottle(email, "passkey");
 
-      expect(mockDb.delete).toHaveBeenCalled();
+      expect(mockCentralDb.delete).toHaveBeenCalled();
     });
   });
 
   describe("Edge cases", () => {
     it("should handle cleanup of expired records", async () => {
-      const mockDb = getCentralDb();
+      const mockDeleteBuilder = {
+        where: vi.fn().mockResolvedValue(undefined),
+      };
+
+      mockCentralDb.delete.mockReturnValue(mockDeleteBuilder);
 
       await challengeThrottleService.cleanupExpired();
 
-      expect(mockDb.delete).toHaveBeenCalled();
+      expect(mockCentralDb.delete).toHaveBeenCalled();
     });
   });
 });

src/lib/server/services/challenge-throttle.ts

@@ -9,7 +9,7 @@
 
 import { centralDb } from "$lib/server/db";
 import { challengeThrottle } from "$lib/server/db/central-schema";
-import { eq, lt } from "drizzle-orm";
+import { eq, lt, sql } from "drizzle-orm";
 import { logger } from "$lib/logger";
 
 export type ThrottleType = "pin" | "passkey";
@@ -108,33 +108,22 @@ class ChallengeThrottleService {
   async recordFailedAttempt(identifier: string, type: ThrottleType): Promise<void> {
     const now = new Date();
 
-    // Try to get existing record
-    const records = await centralDb
-      .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 centralDb.insert(challengeThrottle).values({
+    const resetAt = new Date(now.getTime() + THROTTLE_RESET_DURATION_MS);
+    await centralDb
+      .insert(challengeThrottle)
+      .values({
         id: identifier,
         failedAttempts: 1,
         lastAttemptAt: now,
         resetAt,
-      });
-    } else {
-      // Update existing record
-      const record = records[0];
-      await centralDb
-        .update(challengeThrottle)
-        .set({
-          failedAttempts: record.failedAttempts + 1,
+      })
+      .onConflictDoUpdate({
+        target: challengeThrottle.id,
+        set: {
+          failedAttempts: sql`${challengeThrottle.failedAttempts} + 1`,
           lastAttemptAt: now,
-        })
-        .where(eq(challengeThrottle.id, identifier));
-    }
+        },
+      });
 
     logger.info(`Recorded failed ${type} challenge attempt`, {
       identifier: identifier.slice(0, 8),

src/lib/stores/pin-throttle.ts

@@ -1,3 +1,4 @@
+import { browser } from "$app/environment";
 import { writable } from "svelte/store";
 
 /**
@@ -23,7 +24,7 @@ const createPinThrottleStore = () => {
     failedAttempts: 0,
   };
 
-  if (typeof window !== "undefined") {
+  if (!browser) {
     const stored = localStorage.getItem(STORAGE_KEY);
     if (stored) {
       try {

src/lib/utils/passkey.ts

@@ -60,7 +60,7 @@ export const fetchChallenge = async (email: string) => {
   // Handle throttling
   if (resp.status === 429) {
     const retryAfterSeconds = Math.ceil((data.retryAfterMs || 60000) / 1000);
-    logger.error("Challenge request throttled", {
+    logger.warn("Challenge request throttled", {
       email,
       retryAfterSeconds,
     });

src/routes/(pages)/(clients)/book-appointment/[[id]]/(components)/auth/book-appointment-login-form.svelte

@@ -1,6 +1,7 @@
 <script lang="ts">
   import { m } from "$i18n/messages.js";
   import { UnifiedAppointmentCrypto } from "$lib/client/appointment-crypto";
+  import * as Alert from "$lib/components/ui/alert";
   import * as Form from "$lib/components/ui/form";
   import InputOtpCustomized from "$lib/components/ui/input-top-customized/input-otp-customized.svelte";
   import { publicStore } from "$lib/stores/public";
@@ -84,16 +85,14 @@
 
 <Form.Root {enhance}>
   {#if isThrottled}
-    <div
-      class="bg-destructive/10 border-destructive dark:bg-destructive/20 dark:border-destructive/50 mb-4 rounded-lg border p-4"
-    >
-      <p class="text-destructive dark:text-destructive/90 font-semibold">
+    <Alert.Root class="mb-4 border-destructive bg-destructive/10 dark:bg-destructive/20 dark:border-destructive/50">
+      <Alert.Title class="text-destructive dark:text-destructive/90">
         {m["public.steps.auth.login.throttled"]()}
-      </p>
-      <p class="text-destructive/80 dark:text-destructive/70 mt-1 text-sm">
+      </Alert.Title>
+      <Alert.Description class="text-destructive/80 dark:text-destructive/70 mt-1">
         {m["public.steps.auth.login.retry"]({ seconds: remainingSeconds })}
-      </p>
-    </div>
+      </Alert.Description>
+    </Alert.Root>
   {/if}
 
   <Form.Field {form} name="pin">

src/routes/(pages)/(clients)/book-appointment/[[id]]/(components)/auth/book-appointment-register-form.svelte

@@ -1,6 +1,7 @@
 <script lang="ts">
   import { m } from "$i18n/messages.js";
   import { UnifiedAppointmentCrypto } from "$lib/client/appointment-crypto";
+  import * as Alert from "$lib/components/ui/alert";
   import * as Form from "$lib/components/ui/form";
   import InputOtpCustomized from "$lib/components/ui/input-top-customized/input-otp-customized.svelte";
   import { publicStore } from "$lib/stores/public";
@@ -95,16 +96,16 @@
 
 <Form.Root {enhance}>
   {#if isThrottled}
-    <div
-      class="bg-destructive/10 border-destructive dark:bg-destructive/20 dark:border-destructive/50 mb-4 rounded-lg border p-4"
+    <Alert.Root
+      class="border-destructive bg-destructive/10 dark:bg-destructive/20 dark:border-destructive/50 mb-4"
     >
-      <p class="text-destructive dark:text-destructive/90 font-semibold">
+      <Alert.Title class="text-destructive dark:text-destructive/90">
         {m["public.steps.auth.login.throttled"]()}
-      </p>
-      <p class="text-destructive/80 dark:text-destructive/70 mt-1 text-sm">
+      </Alert.Title>
+      <Alert.Description class="text-destructive/80 dark:text-destructive/70 mt-1">
         {m["public.steps.auth.login.retry"]({ seconds: remainingSeconds })}
-      </p>
-    </div>
+      </Alert.Description>
+    </Alert.Root>
   {/if}
 
   <Form.Field {form} name="pin">

src/routes/api/tenants/[id]/appointments/challenge/+server.ts

@@ -139,7 +139,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
     const { emailHash } = requestSchema.parse(body);
 
     // Check throttling
-    const throttleResult = await challengeThrottleService.checkThrottle(emailHash, "pin", tenantId);
+    const throttleResult = await challengeThrottleService.checkThrottle(emailHash, "pin");
 
     if (!throttleResult.allowed) {
       logger.warn("PIN challenge throttled", {

src/routes/api/tenants/[id]/appointments/verify-challenge/+server.ts

@@ -164,11 +164,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
       });
 
       // Record failed attempt for throttling
-      await challengeThrottleService.recordFailedAttempt(
-        storedChallenge.emailHash,
-        "pin",
-        tenantId,
-      );
+      await challengeThrottleService.recordFailedAttempt(storedChallenge.emailHash, "pin");
 
       throw new ValidationError("Invalid challenge response");
     }
@@ -203,7 +199,7 @@ export const POST: RequestHandler = async ({ request, params }) => {
     });
 
     // Clear throttle on successful verification
-    await challengeThrottleService.clearThrottle(storedChallenge.emailHash, "pin", tenantId);
+    await challengeThrottleService.clearThrottle(storedChallenge.emailHash, "pin");
 
     const response: ChallengeVerificationResponse = {
       valid: true,