fix(main): serialize manager PIN verification and preserve failed login counter on 403

docs/architecture/authentication-and-authorization.md

@@ -134,8 +134,10 @@ additionally require the presenting user to satisfy `hasRole(role, ROLE_ACCESS.o
 deliberate context-policy role check, not a permission, because the override PIN belongs to a
 person acting in an owner/manager capacity regardless of what their individual permission grants
 say. To prevent event loop starvation and transaction lock contention during CPU-intensive bcrypt
-evaluations, manager PINs are verified asynchronously via `verifyPinAsync` outside the transaction
-(with client-IP rate limits checked first). Candidate filtering during pre-verification enforces the
+evaluations, manager PINs are verified asynchronously via `verifyPinAsync` outside the transaction,
+serialized per client IP and action key with `withPinVerificationLock` from
+[`main/middleware/security.ts`](../../main/middleware/security.ts) (re-checking client-IP rate limits
+inside the lock before executing bcrypt). Candidate filtering during pre-verification enforces the
 role and permissions required by the transition (including `payments.take` when completion is
 blocked by unsettled payment), ensuring fallback PIN scanning only matches eligible approvers. The
 pre-verified approver is then atomically re-validated inside `withTxn` against current SQLite state
@@ -175,8 +177,8 @@ All three HTTP listeners (`:3001` Main API, `:3002` Standalone KDS, and `:3003`
 
 1. **Volume rate limiter (`loginAuthRateLimit`):** Used on all three default login endpoints, `loginAuthRateLimit()` reuses a shared singleton instance so total authentication request volume across listeners shares a single budget (default 10 requests per 15 minutes, configurable via `FLO_AUTH_RATE_LIMIT_MAX`). Non-login authentication routes (e.g. staff management, password changes) use independent rate-limiter instances.
 2. **Shared failed-login lockout (`checkLoginRateLimit` / `incrementFailedLogin`):** Keyed to the canonical client IP (`normalizeIpForRateLimit`, which strips `::ffff:` IPv4-mapped IPv6 prefixes so IPv4 and IPv6 representations share the exact same bucket). Five failed credential attempts across any combination of the three listeners triggers a 15-minute lockout (HTTP `429`). During the lockout, any further login attempt on any listener is rejected before password evaluation.
-3. **Reset on success (`resetSuccessfulLogin`):** A successful login from any listener immediately clears the failed-attempt count for that client IP.
-4. **Role and permission decoupling:** Non-credential rejections (such as HTTP `403` when valid credentials lack `kitchen.use` on `:3002` or `server-app.use` on `:3003`) do not count as credential failures.
+3. **Reset on success (`resetSuccessfulLogin`):** A successful login from any listener immediately clears the failed-attempt count for that client IP once role and permission checks pass.
+4. **Role and permission decoupling:** Non-credential rejections (such as HTTP `403` when valid credentials lack `kitchen.use` on `:3002` or `server-app.use` on `:3003`) do not count as credential failures and do not clear previously accumulated failed-login attempts.
 
 ## Master PIN as a second factor
 

docs/reference/api.md

@@ -111,7 +111,7 @@ LAN clients are counted against authentication rate limits.
 In addition, these three login endpoints share an install-wide failed-login attempt lockout keyed to
 canonical client IP (`normalizeIpForRateLimit`): 5 failed credential attempts across any
 combination of the three listeners triggers a 15-minute lockout (`429`). Successful login on any listener
-resets the counter for that IP. See
+resets the counter for that IP (non-credential `403` responses preserve accumulated failures). See
 [`docs/architecture/authentication-and-authorization.md`](../architecture/authentication-and-authorization.md#shared-login-attempt-rate-limiting-across-servers)
 for architecture and role/permission handling details.
 
@@ -359,7 +359,7 @@ does not wrap the handler again.
   line stays visible. Inventory is not restored on the void path. A plain cancel sets the item to
   `cancelled` and restores the recorded inventory deduction and recipe components.
 - Cancelling the last active item cancels the order and frees its table.
-- `override_pin` attempts are rate limited per client IP, not per item, to slow brute force (counting only failed attempts; 429 after 5 failed attempts).
+- `override_pin` attempts are rate limited per canonical client IP, not per item, to slow brute force (counting only failed attempts; 429 after 5 failed attempts within 15 minutes; serialized to prevent concurrent brute-force races).
 
 ### Order item status
 

main/kds-server.ts

@@ -269,8 +269,6 @@ export function startKdsServer(): Promise<void> {
           });
         }
 
-        resetSuccessfulLogin(ip);
-
         // Only allow chef, manager, owner roles
         if (!hasPermission(user.id, 'kitchen.use')) {
           return res.status(403).json({
@@ -289,6 +287,8 @@ export function startKdsServer(): Promise<void> {
             .json({ error: 'No active kitchen station is assigned to this user' });
         }
 
+        resetSuccessfulLogin(ip);
+
         const token = jwt.sign(
           { userId: user.id, email: user.email, role: user.role, jti: randomUUID() },
           getJWTSecret(),

main/middleware/security.ts

@@ -70,7 +70,7 @@
 
     if (options.skipSuccessfulRequests) {
       const originalSend = res.send.bind(res);
       res.send = (body: any) => {
         if (res.statusCode >= 200 && res.statusCode < 300) {
           record!.count = Math.max(0, record!.count - 1);
         }
@@ -175,6 +175,72 @@
   loginAttempts.clear();
 }
 
+// ── Shared Manager PIN Rate Limiting & Concurrency Accounting ─────────────────
+export const PIN_MAX_ATTEMPTS = 5;
+export const PIN_WINDOW_MS = 15 * 60 * 1000; // 15 minutes
+
+const pinAttempts = new Map<string, { count: number; resetAt: number }>();
+const pinVerificationLocks = new Map<string, Promise<void>>();
+
+export function checkPinRateLimit(key: string): boolean {
+  const nowMs = Date.now();
+  if (pinAttempts.size > 500) {
+    for (const [k, v] of pinAttempts.entries()) {
+      if (nowMs > v.resetAt) pinAttempts.delete(k);
+    }
+  }
+  const entry = pinAttempts.get(key);
+  if (!entry || nowMs > entry.resetAt) {
+    pinAttempts.set(key, { count: 1, resetAt: nowMs + PIN_WINDOW_MS });
+    return true;
+  }
+  if (entry.count >= PIN_MAX_ATTEMPTS) return false;
+  entry.count++;
+  return true;
+}
+
+export function isPinRateLimited(key: string): boolean {
+  const entry = pinAttempts.get(key);
+  if (!entry || Date.now() > entry.resetAt) {
+    pinAttempts.delete(key);
+    return false;
+  }
+  return entry.count >= PIN_MAX_ATTEMPTS;
+}
+
+export function clearPinRateLimit(key: string): void {
+  pinAttempts.delete(key);
+}
+
+export function resetPinRateLimitForTests(): void {
+  pinAttempts.clear();
+  pinVerificationLocks.clear();
+}
+
+/**
+ * Serializes PIN verification for the given rateLimitKey to ensure that
+ * simultaneous invalid requests do not execute redundant bcrypt computations
+ * in parallel before the failure counter is updated.
+ */
+export async function withPinVerificationLock<T>(key: string, fn: () => Promise<T>): Promise<T> {
+  const previous = pinVerificationLocks.get(key) || Promise.resolve();
+  let resolveCurrent!: () => void;
+  const current = new Promise<void>((resolve) => {
+    resolveCurrent = resolve;
+  });
+  pinVerificationLocks.set(key, current);
+
+  try {
+    await previous.catch(() => {});
+    return await fn();
+  } finally {
+    resolveCurrent();
+    if (pinVerificationLocks.get(key) === current) {
+      pinVerificationLocks.delete(key);
+    }
+  }
+}
+
 /** Shared rate limiter for static/SPA file serving with private IP bypass. */
 export function staticRouteRateLimit(options: { windowMs?: number; limit?: number } = {}) {
   return expressRateLimit({
@@ -355,7 +421,7 @@
 /** Role-based authorization middleware (must follow requireAuth). */
 export function requireRole(...roles: readonly string[]) {
   return (req: Request, res: Response, next: () => void) => {
     const user = (req as any).user;
     if (!user) {
       return res.status(401).json({ error: 'Authentication required' });
     }
@@ -502,7 +568,7 @@
       }
 
       callback(new Error('Not allowed by CORS'));
     } catch (err) {
       callback(new Error('Invalid origin format'));
     }
   },

main/routes/bills.ts

@@ -27,6 +27,26 @@ import {
   NO_CASH_SESSION_ID,
   requireOpenSessionForCashTender,
 } from '../services/shift-session-gate';
+import {
+  checkPinRateLimit,
+  isPinRateLimited,
+  clearPinRateLimit,
+  resetPinRateLimitForTests,
+  withPinVerificationLock,
+  normalizeIpForRateLimit,
+  PIN_MAX_ATTEMPTS,
+  PIN_WINDOW_MS,
+} from '../middleware/security';
+
+export {
+  checkPinRateLimit,
+  isPinRateLimited,
+  clearPinRateLimit,
+  resetPinRateLimitForTests,
+  withPinVerificationLock,
+  PIN_MAX_ATTEMPTS,
+  PIN_WINDOW_MS,
+};
 import { getActiveCountryPack, scaleTaxSnapshots, type Customer } from '../services/tax';
 import { applyPayableRounding } from '../services/tax-engine';
 import { calculateOrderTotals, recomputeOrderTotals } from '../services/orders';
@@ -134,7 +154,8 @@ export async function preVerifyKitchenDeliveryPin(
   if (overridePin === undefined || overridePin === null || overridePin === '') {
     return null;
   }
-  const rateLimitKey = `pin:${clientIp}:kitchen-delivery`;
+  const canonicalIp = normalizeIpForRateLimit(clientIp);
+  const rateLimitKey = `pin:${canonicalIp}:kitchen-delivery`;
   if (isPinRateLimited(rateLimitKey)) {
     throw Object.assign(new Error('Too many PIN attempts. Try again in 15 minutes.'), {
       statusCode: 429,
@@ -145,19 +166,28 @@ export async function preVerifyKitchenDeliveryPin(
     throw Object.assign(new Error('Invalid manager PIN'), { statusCode: 403 });
   }
 
-  const managers = db
-    .prepare('SELECT id, role, pin_hash FROM users WHERE pin_hash IS NOT NULL AND is_active = 1')
-    .all() as { id: string; role: string; pin_hash: string }[];
-  for (const user of managers) {
-    if (hasRole(user.role, ROLE_ACCESS.ownerManager)) {
-      if (await verifyPinAsync(user.pin_hash, overridePin)) {
-        return { id: user.id, verifiedPinHash: user.pin_hash };
+  return withPinVerificationLock(rateLimitKey, async () => {
+    if (isPinRateLimited(rateLimitKey)) {
+      throw Object.assign(new Error('Too many PIN attempts. Try again in 15 minutes.'), {
+        statusCode: 429,
+      });
+    }
+
+    const managers = db
+      .prepare('SELECT id, role, pin_hash FROM users WHERE pin_hash IS NOT NULL AND is_active = 1')
+      .all() as { id: string; role: string; pin_hash: string }[];
+    for (const user of managers) {
+      if (hasRole(user.role, ROLE_ACCESS.ownerManager)) {
+        if (await verifyPinAsync(user.pin_hash, overridePin)) {
+          clearPinRateLimit(rateLimitKey);
+          return { id: user.id, verifiedPinHash: user.pin_hash };
+        }
       }
     }
-  }
 
-  checkPinRateLimit(rateLimitKey);
-  throw Object.assign(new Error('Invalid manager PIN'), { statusCode: 403 });
+    checkPinRateLimit(rateLimitKey);
+    throw Object.assign(new Error('Invalid manager PIN'), { statusCode: 403 });
+  });
 }
 
 export function checkKitchenDeliveryStatus(
@@ -215,7 +245,8 @@ export function checkKitchenDeliveryStatus(
   if (overridePin === undefined || overridePin === null || overridePin === '') {
     return { allowed: false, count: items.length, items };
   }
-  const rateLimitKey = `pin:${clientIp}:kitchen-delivery`;
+  const canonicalIp = normalizeIpForRateLimit(clientIp);
+  const rateLimitKey = `pin:${canonicalIp}:kitchen-delivery`;
   if (isPinRateLimited(rateLimitKey)) {
     throw Object.assign(new Error('Too many PIN attempts. Try again in 15 minutes.'), {
       statusCode: 429,
@@ -255,7 +286,7 @@ export function checkKitchenDeliveryStatus(
     checkPinRateLimit(rateLimitKey);
     throw Object.assign(new Error('Invalid manager PIN'), { statusCode: 403 });
   }
-  pinAttempts.delete(rateLimitKey);
+  clearPinRateLimit(rateLimitKey);
   return { allowed: true, count: items.length, items, overridden: true, managerUserId: manager.id };
 }
 
@@ -692,41 +723,6 @@ export function getOrdersWithItemsForBills(
 // use this rate so a customer's point balance always equals its currency value.
 const LOYALTY_REDEMPTION_RATE = 1;
 
-// Rate limiting for PIN validation (simple in-memory)
-const pinAttempts = new Map<string, { count: number; resetAt: number }>();
-const PIN_MAX_ATTEMPTS = 5;
-const PIN_WINDOW_MS = 15 * 60 * 1000; // 15 minutes
-
-function checkPinRateLimit(key: string): boolean {
-  const nowMs = Date.now();
-  if (pinAttempts.size > 500) {
-    for (const [k, v] of pinAttempts.entries()) {
-      if (nowMs > v.resetAt) pinAttempts.delete(k);
-    }
-  }
-  const entry = pinAttempts.get(key);
-  if (!entry || nowMs > entry.resetAt) {
-    pinAttempts.set(key, { count: 1, resetAt: nowMs + PIN_WINDOW_MS });
-    return true;
-  }
-  if (entry.count >= PIN_MAX_ATTEMPTS) return false;
-  entry.count++;
-  return true;
-}
-
-function isPinRateLimited(key: string): boolean {
-  const entry = pinAttempts.get(key);
-  if (!entry || Date.now() > entry.resetAt) {
-    pinAttempts.delete(key);
-    return false;
-  }
-  return entry.count >= PIN_MAX_ATTEMPTS;
-}
-
-export function resetPinRateLimitForTests(): void {
-  pinAttempts.clear();
-}
-
 function parsePaginationInteger(value: unknown, defaultValue: number): number | null {
   if (value === undefined || value === null || value === '') return defaultValue;
   if (Array.isArray(value)) return null;
@@ -4109,53 +4105,71 @@ router.post(
             .status(403)
             .json({ error: 'Manager PIN required for discounts', requiresApproval: true });
         }
-        const rateLimitKey = `pin:${clientIp}:bill-discount`;
+        const canonicalIp = normalizeIpForRateLimit(clientIp);
+        const rateLimitKey = `pin:${canonicalIp}:bill-discount`;
         if (isPinRateLimited(rateLimitKey)) {
           return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' });
         }
+        if (typeof override_pin !== 'string' || !/^\d{4,6}$/.test(override_pin)) {
+          checkPinRateLimit(rateLimitKey);
+          return res.status(403).json({ error: 'Invalid manager PIN' });
+        }
         const managerId = req.body.manager_id || req.body.user_id || req.body.approver_id;
         type ManagerCandidate = { id: string; role: string; pin_hash: string | null };
-        let user: ManagerCandidate | null = null;
-        if (managerId) {
-          const candidate = db
-            .prepare(
-              'SELECT id, role, pin_hash FROM users WHERE id = ? AND pin_hash IS NOT NULL AND is_active = 1',
-            )
-            .get(managerId) as ManagerCandidate | undefined;
-          if (
-            candidate &&
-            hasRole(candidate.role, ROLE_ACCESS.ownerManager) &&
-            hasPermission(candidate.id, 'bills.discount.apply') &&
-            candidate.pin_hash &&
-            (await verifyPinAsync(candidate.pin_hash, override_pin))
-          ) {
-            user = candidate;
+        const verifiedResult = await withPinVerificationLock(rateLimitKey, async () => {
+          if (isPinRateLimited(rateLimitKey)) {
+            return { rateLimited: true };
           }
-        } else {
-          const managers = db
-            .prepare(
-              'SELECT id, role, pin_hash FROM users WHERE pin_hash IS NOT NULL AND is_active = 1',
-            )
-            .all() as ManagerCandidate[];
-          for (const u of managers) {
+          let user: ManagerCandidate | null = null;
+          if (managerId) {
+            const candidate = db
+              .prepare(
+                'SELECT id, role, pin_hash FROM users WHERE id = ? AND pin_hash IS NOT NULL AND is_active = 1',
+              )
+              .get(managerId) as ManagerCandidate | undefined;
             if (
-              hasRole(u.role, ROLE_ACCESS.ownerManager) &&
-              hasPermission(u.id, 'bills.discount.apply') &&
-              u.pin_hash &&
-              (await verifyPinAsync(u.pin_hash, override_pin))
+              candidate &&
+              hasRole(candidate.role, ROLE_ACCESS.ownerManager) &&
+              hasPermission(candidate.id, 'bills.discount.apply') &&
+              candidate.pin_hash &&
+              (await verifyPinAsync(candidate.pin_hash, override_pin))
             ) {
-              user = u;
-              break;
+              user = candidate;
+            }
+          } else {
+            const managers = db
+              .prepare(
+                'SELECT id, role, pin_hash FROM users WHERE pin_hash IS NOT NULL AND is_active = 1',
+              )
+              .all() as ManagerCandidate[];
+            for (const u of managers) {
+              if (
+                hasRole(u.role, ROLE_ACCESS.ownerManager) &&
+                hasPermission(u.id, 'bills.discount.apply') &&
+                u.pin_hash &&
+                (await verifyPinAsync(u.pin_hash, override_pin))
+              ) {
+                user = u;
+                break;
+              }
             }
           }
+          if (!user) {
+            checkPinRateLimit(rateLimitKey);
+            return { user: null };
+          }
+          clearPinRateLimit(rateLimitKey);
+          return { user };
+        });
+
+        if (verifiedResult.rateLimited) {
+          return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' });
         }
-        if (!user) {
-          checkPinRateLimit(rateLimitKey);
+        if (!verifiedResult.user) {
           return res.status(403).json({ error: 'Invalid manager PIN' });
         }
-        pinAttempts.delete(rateLimitKey);
-        approvedByUserId = user.id;
-        preVerifiedPinHash = user.pin_hash ?? undefined;
+        approvedByUserId = verifiedResult.user.id;
+        preVerifiedPinHash = verifiedResult.user.pin_hash ?? undefined;
       }
 
       // Check discount mode