From 2e35cde729a3ca201477d375ee735614179cd3b0 Mon Sep 17 00:00:00 2001 From: khaira777 <777gurkirat@gmail.com> Date: Sun, 18 Oct 2026 14:54:11 -0400 Subject: [PATCH 1/2] fix(auth): prevent concurrent PIN limit bypass race and preserve cross-server failed login counter (#1221) - Centralize PIN rate limiting and concurrency serialization with withPinVerificationLock in security middleware - Re-check isPinRateLimited inside lock so concurrent bursts reject immediately with 429 without redundant bcrypt executions - Defer resetSuccessfulLogin in KDS and Server App until after role and permission validation so 403 Forbidden does not clear the failed-login counter - Add regression tests covering 403 failure counter preservation and concurrent PIN verification bursts --- main/kds-server.ts | 4 +- main/middleware/security.ts | 66 +++++ main/routes/bills.ts | 168 ++++++----- main/routes/orders.ts | 295 +++++++++++-------- main/server-app.ts | 4 +- tests/cross-server-login-rate-limits.test.ts | 47 +++ tests/manager-pin-verification.test.ts | 43 +++ 7 files changed, 430 insertions(+), 197 deletions(-) diff --git a/main/kds-server.ts b/main/kds-server.ts index 67e06fe70..d580d4141 100644 --- a/main/kds-server.ts +++ b/main/kds-server.ts @@ -269,8 +269,6 @@ export function startKdsServer(): Promise { }); } - 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 { .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(), diff --git a/main/middleware/security.ts b/main/middleware/security.ts index f39691d6f..8df997922 100644 --- a/main/middleware/security.ts +++ b/main/middleware/security.ts @@ -175,6 +175,72 @@ export function clearLoginRateLimits(): void { 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(); +const pinVerificationLocks = new Map>(); + +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(key: string, fn: () => Promise): Promise { + const previous = pinVerificationLocks.get(key) || Promise.resolve(); + let resolveCurrent!: () => void; + const current = new Promise((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({ diff --git a/main/routes/bills.ts b/main/routes/bills.ts index 87fa806c5..5f5037248 100644 --- a/main/routes/bills.ts +++ b/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'; @@ -127,7 +147,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, @@ -138,19 +159,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 }); + }); } function checkKitchenDeliveryStatus( @@ -208,7 +238,8 @@ 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, @@ -248,7 +279,7 @@ 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 }; } @@ -685,41 +716,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(); -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; @@ -3630,46 +3626,66 @@ router.post( .json({ error: 'Manager PIN required for discounts', requiresApproval: true }); } const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; - 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; - let user: any = null; - if (managerId) { - const candidate = db - .prepare('SELECT * FROM users WHERE id = ? AND pin_hash IS NOT NULL AND is_active = 1') - .get(managerId) as any; - if ( - candidate && - hasRole(candidate.role, ROLE_ACCESS.ownerManager) && - hasPermission(candidate.id, 'bills.discount.apply') && - (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 * FROM users WHERE pin_hash IS NOT NULL AND is_active = 1') - .all() as any[]; - for (const u of managers) { + let user: any = null; + if (managerId) { + const candidate = db + .prepare( + 'SELECT * FROM users WHERE id = ? AND pin_hash IS NOT NULL AND is_active = 1', + ) + .get(managerId) as any; if ( - hasRole(u.role, ROLE_ACCESS.ownerManager) && - hasPermission(u.id, 'bills.discount.apply') && - (await verifyPinAsync(u.pin_hash, override_pin)) + candidate && + hasRole(candidate.role, ROLE_ACCESS.ownerManager) && + hasPermission(candidate.id, 'bills.discount.apply') && + (await verifyPinAsync(candidate.pin_hash, override_pin)) ) { - user = u; - break; + user = candidate; + } + } else { + const managers = db + .prepare('SELECT * FROM users WHERE pin_hash IS NOT NULL AND is_active = 1') + .all() as any[]; + for (const u of managers) { + if ( + hasRole(u.role, ROLE_ACCESS.ownerManager) && + hasPermission(u.id, 'bills.discount.apply') && + (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; + approvedByUserId = verifiedResult.user.id; + preVerifiedPinHash = verifiedResult.user.pin_hash; } // Check discount mode diff --git a/main/routes/orders.ts b/main/routes/orders.ts index 098f1e30b..e97a0eac2 100644 --- a/main/routes/orders.ts +++ b/main/routes/orders.ts @@ -66,7 +66,27 @@ import { getCurrencyFractionDigits, getCurrencyMinorUnitFactor } from '../countr import { getTenantCurrency, syncUnpaidBillsForOrder } from './bills'; import expressRateLimit from 'express-rate-limit'; import { randomUUID } from 'crypto'; -import { isAllowedPrivateIp } from '../middleware/security'; +import { + isAllowedPrivateIp, + 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 { isOperationReference, serializeOperationFailure } from '../lib/operation-failure'; type PinApproverCandidate = { id: string; role: string; pin_hash: string }; @@ -269,46 +289,6 @@ function getStoredOrderReplay( } } -// Rate limiting for PIN validation (simple in-memory) -const pinAttempts = new Map(); -const PIN_MAX_ATTEMPTS = 5; -const PIN_WINDOW_MS = 15 * 60 * 1000; // 15 minutes - -export function checkPinRateLimit(key: string): boolean { - const now = Date.now(); - // Sweep expired rate limit entries when map grows beyond threshold. - if (pinAttempts.size > 500) { - for (const [k, v] of pinAttempts.entries()) { - if (now > v.resetAt) pinAttempts.delete(k); - } - } - const entry = pinAttempts.get(key); - if (!entry || now > entry.resetAt) { - pinAttempts.set(key, { count: 1, resetAt: now + 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(); -} - function syncCustomerTagCounts( db: any, customerId: string, @@ -2332,28 +2312,35 @@ router.patch( typeof override_pin === 'string' ) { const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; + const canonicalIp = normalizeIpForRateLimit(clientIp); const rateLimitKey = status === 'cancelled' - ? `pin:${clientIp}:order-cancel` - : `pin:${clientIp}:order-complete`; + ? `pin:${canonicalIp}:order-cancel` + : `pin:${canonicalIp}:order-complete`; if (isPinRateLimited(rateLimitKey)) { return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' }); } - if (/^\d{4,6}$/.test(override_pin)) { - const explicitApproverId = - req.body.approver_id || req.body.manager_id || req.body.user_id; - const candidateUsers = explicitApproverId - ? (db - .prepare( - 'SELECT id, role, pin_hash FROM users WHERE id = ? AND is_active = 1 AND pin_hash IS NOT NULL', - ) - .all(explicitApproverId) as PinApproverCandidate[]) - : (db - .prepare( - 'SELECT id, role, pin_hash FROM users WHERE is_active = 1 AND pin_hash IS NOT NULL', - ) - .all() as PinApproverCandidate[]); + if (!/^\d{4,6}$/.test(override_pin)) { + checkPinRateLimit(rateLimitKey); + return res.status(403).json({ error: 'Invalid manager PIN' }); + } + const explicitApproverId = req.body.approver_id || req.body.manager_id || req.body.user_id; + const candidateUsers = explicitApproverId + ? (db + .prepare( + 'SELECT id, role, pin_hash FROM users WHERE id = ? AND is_active = 1 AND pin_hash IS NOT NULL', + ) + .all(explicitApproverId) as PinApproverCandidate[]) + : (db + .prepare( + 'SELECT id, role, pin_hash FROM users WHERE is_active = 1 AND pin_hash IS NOT NULL', + ) + .all() as PinApproverCandidate[]); + const preVerifiedCandidate = await withPinVerificationLock(rateLimitKey, async () => { + if (isPinRateLimited(rateLimitKey)) { + return { rateLimited: true }; + } for (const candidate of candidateUsers) { if ( hasRole(candidate.role, ROLE_ACCESS.ownerManager) && @@ -2361,15 +2348,27 @@ router.patch( (status !== 'cancelled' || hasPermission(candidate.id, 'orders.item.cancel')) ) { if (await verifyPinAsync(candidate.pin_hash, override_pin)) { - preVerifiedStatusApprover = { - id: candidate.id, - verifiedPinHash: candidate.pin_hash, + clearPinRateLimit(rateLimitKey); + return { + approver: { + id: candidate.id, + verifiedPinHash: candidate.pin_hash, + }, }; - break; } } } + checkPinRateLimit(rateLimitKey); + return { approver: null }; + }); + + if (preVerifiedCandidate.rateLimited) { + return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' }); + } + if (!preVerifiedCandidate.approver) { + return res.status(403).json({ error: 'Invalid manager PIN' }); } + preVerifiedStatusApprover = preVerifiedCandidate.approver; } const nowStr = now(); @@ -2621,10 +2620,11 @@ router.patch( } const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; + const canonicalIp = normalizeIpForRateLimit(clientIp); const rateLimitKey = status === 'cancelled' - ? `pin:${clientIp}:order-cancel` - : `pin:${clientIp}:order-complete`; + ? `pin:${canonicalIp}:order-cancel` + : `pin:${canonicalIp}:order-complete`; if (isPinRateLimited(rateLimitKey)) { throw Object.assign(new Error('Too many PIN attempts. Try again in 15 minutes.'), { statusCode: 429, @@ -2659,7 +2659,7 @@ router.patch( checkPinRateLimit(rateLimitKey); throw Object.assign(new Error('Invalid manager PIN'), { statusCode: 403 }); } - pinAttempts.delete(rateLimitKey); + clearPinRateLimit(rateLimitKey); approvedByUserId = user.id; } @@ -3121,12 +3121,17 @@ router.patch( .json({ error: 'Manager PIN required for discounts', requiresApproval: true }); } const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; - const rateLimitKey = `pin:${clientIp}:discount`; + const canonicalIp = normalizeIpForRateLimit(clientIp); + const rateLimitKey = `pin:${canonicalIp}: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 explicitApproverId = req.body.approver_id || req.body.manager_id || req.body.user_id; const candidateUsers = explicitApproverId @@ -3140,24 +3145,40 @@ router.patch( 'SELECT id, role, pin_hash FROM users WHERE is_active = 1 AND pin_hash IS NOT NULL', ) .all() as PinApproverCandidate[]); - let user: PinApproverCandidate | undefined; - for (const candidate of candidateUsers) { - if ( - hasRole(candidate.role, ROLE_ACCESS.ownerManager) && - hasPermission(candidate.id, 'orders.discount.apply') && - (await verifyPinAsync(candidate.pin_hash, override_pin)) - ) { - user = candidate; - break; + + const verifiedResult = await withPinVerificationLock(rateLimitKey, async () => { + if (isPinRateLimited(rateLimitKey)) { + return { rateLimited: true }; } + let user: PinApproverCandidate | undefined; + for (const candidate of candidateUsers) { + if ( + hasRole(candidate.role, ROLE_ACCESS.ownerManager) && + hasPermission(candidate.id, 'orders.discount.apply') && + (await verifyPinAsync(candidate.pin_hash, override_pin)) + ) { + user = candidate; + 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; - preVerifiedOrderDiscountPinHash = user.pin_hash; + approvedByUserId = verifiedResult.user.id; + preVerifiedOrderDiscountPinHash = verifiedResult.user.pin_hash; } } @@ -3528,10 +3549,15 @@ router.patch( .json({ error: 'Manager PIN required for discounts', requiresApproval: true }); } const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; - const rateLimitKey = `pin:${clientIp}:item-discount`; + const canonicalIp = normalizeIpForRateLimit(clientIp); + const rateLimitKey = `pin:${canonicalIp}:item-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 explicitApproverId = req.body.approver_id || req.body.manager_id || req.body.user_id; const candidateUsers = explicitApproverId ? (db @@ -3544,24 +3570,38 @@ router.patch( 'SELECT id, role, pin_hash FROM users WHERE is_active = 1 AND pin_hash IS NOT NULL', ) .all() as PinApproverCandidate[]); - let user: PinApproverCandidate | undefined; - for (const candidate of candidateUsers) { - if ( - hasRole(candidate.role, ROLE_ACCESS.ownerManager) && - hasPermission(candidate.id, 'orders.discount.apply') && - (await verifyPinAsync(candidate.pin_hash, override_pin)) - ) { - user = candidate; - break; + + const verifiedResult = await withPinVerificationLock(rateLimitKey, async () => { + if (isPinRateLimited(rateLimitKey)) { + return { rateLimited: true }; + } + let user: PinApproverCandidate | undefined; + for (const candidate of candidateUsers) { + if ( + hasRole(candidate.role, ROLE_ACCESS.ownerManager) && + hasPermission(candidate.id, 'orders.discount.apply') && + (await verifyPinAsync(candidate.pin_hash, override_pin)) + ) { + user = candidate; + 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; - preVerifiedItemDiscountPinHash = user.pin_hash; + approvedByUserId = verifiedResult.user.id; + preVerifiedItemDiscountPinHash = verifiedResult.user.pin_hash; } // Check discount mode @@ -3985,41 +4025,61 @@ router.patch( let preVerifiedVoidApprover: { id: string; verifiedPinHash: string } | null = null; if (isItemVoidInProgress && override_pin && typeof override_pin === 'string') { const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; - const rateLimitKey = `pin:${clientIp}:item-void`; + const canonicalIp = normalizeIpForRateLimit(clientIp); + const rateLimitKey = `pin:${canonicalIp}:item-void`; if (isPinRateLimited(rateLimitKey)) { return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' }); } - if (/^\d{4,6}$/.test(override_pin)) { - const managerId = req.body.manager_id || req.body.user_id || req.body.approver_id; - const candidateUsers = managerId - ? (db - .prepare( - 'SELECT id, role, pin_hash, is_active FROM users WHERE id = ? AND pin_hash IS NOT NULL AND is_active = 1', - ) - .all(managerId) as { - id: string; - role: string; - pin_hash: string; - is_active: number; - }[]) - : (db - .prepare( - 'SELECT id, role, pin_hash, is_active FROM users WHERE pin_hash IS NOT NULL AND is_active = 1', - ) - .all() as { id: string; role: string; pin_hash: string; is_active: number }[]); + if (!/^\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; + const candidateUsers = managerId + ? (db + .prepare( + 'SELECT id, role, pin_hash, is_active FROM users WHERE id = ? AND pin_hash IS NOT NULL AND is_active = 1', + ) + .all(managerId) as { + id: string; + role: string; + pin_hash: string; + is_active: number; + }[]) + : (db + .prepare( + 'SELECT id, role, pin_hash, is_active FROM users WHERE pin_hash IS NOT NULL AND is_active = 1', + ) + .all() as { id: string; role: string; pin_hash: string; is_active: number }[]); + const preVerifiedCandidate = await withPinVerificationLock(rateLimitKey, async () => { + if (isPinRateLimited(rateLimitKey)) { + return { rateLimited: true }; + } for (const candidate of candidateUsers) { if ( hasRole(candidate.role, ROLE_ACCESS.ownerManager) && hasPermission(candidate.id, 'orders.item.void') ) { if (await verifyPinAsync(candidate.pin_hash, override_pin)) { - preVerifiedVoidApprover = { id: candidate.id, verifiedPinHash: candidate.pin_hash }; - break; + clearPinRateLimit(rateLimitKey); + return { + approver: { id: candidate.id, verifiedPinHash: candidate.pin_hash }, + }; } } } + checkPinRateLimit(rateLimitKey); + return { approver: null }; + }); + + if (preVerifiedCandidate.rateLimited) { + return res.status(429).json({ error: 'Too many PIN attempts. Try again in 15 minutes.' }); + } + if (!preVerifiedCandidate.approver) { + return res.status(403).json({ error: 'Invalid manager PIN' }); } + preVerifiedVoidApprover = preVerifiedCandidate.approver; } // Wrap item cancel and total recalculation in transaction. @@ -4107,8 +4167,9 @@ router.patch( } const clientIp = req.ip || req.socket.remoteAddress || 'unknown'; + const canonicalIp = normalizeIpForRateLimit(clientIp); // Rate-limit attempts per client rather than per item to prevent brute force attacks. - const rateLimitKey = `pin:${clientIp}:item-void`; + const rateLimitKey = `pin:${canonicalIp}:item-void`; if (isPinRateLimited(rateLimitKey)) { throw Object.assign(new Error('Too many PIN attempts. Try again in 15 minutes.'), { statusCode: 429, @@ -4136,7 +4197,7 @@ router.patch( checkPinRateLimit(rateLimitKey); throw Object.assign(new Error('Invalid manager PIN'), { statusCode: 403 }); } - pinAttempts.delete(rateLimitKey); + clearPinRateLimit(rateLimitKey); approvedByUserId = pinUser.id; } diff --git a/main/server-app.ts b/main/server-app.ts index fa66f8e6b..ac4fbf422 100644 --- a/main/server-app.ts +++ b/main/server-app.ts @@ -305,8 +305,6 @@ export function startServerApp(): Promise { }); } - resetSuccessfulLogin(ip); - if (!hasPermission(user.id, 'server-app.use')) { return res.status(403).json({ error: 'Access denied. Only server, manager, or owner accounts allowed.', @@ -314,6 +312,8 @@ export function startServerApp(): Promise { }); } + resetSuccessfulLogin(ip); + const token = jwt.sign( { userId: user.id, email: user.email, role: user.role, jti: randomUUID() }, getJWTSecret(), diff --git a/tests/cross-server-login-rate-limits.test.ts b/tests/cross-server-login-rate-limits.test.ts index 45f999815..7eef6351b 100644 --- a/tests/cross-server-login-rate-limits.test.ts +++ b/tests/cross-server-login-rate-limits.test.ts @@ -413,6 +413,53 @@ async function main() { '403 permission denial did not increment failed login attempts', ); + // Verify that 403 permission denial does NOT reset previously accumulated failed attempts (FloCafe Issue #1221) + // Record a 2nd failed attempt (leaving 3 attempts remaining): + const fail2 = await postJson(mainBaseUrl, '/api/auth/login', { + email: 'owner@test.com', + password: 'badPassword2', + }); + assert.equal(fail2.status, 401); + assert.equal(fail2.body.attempts_remaining, 3, 'Accumulated 2 failures, 3 remaining'); + + // Trigger 403 on KDS with valid credentials for 'server' + const serverOnKds2 = await postJson(kdsBaseUrl, '/api/auth/login', { + email: 'server@test.com', + password: 'CorrectPassword123!', + }); + assert.equal(serverOnKds2.status, 403, 'KDS returns 403'); + + // Next bad password must be the 3rd failure (2 remaining), NOT reset to 4 remaining! + const checkAfterKds403 = await postJson(mainBaseUrl, '/api/auth/login', { + email: 'owner@test.com', + password: 'badPassword3', + }); + assert.equal(checkAfterKds403.status, 401); + assert.equal( + checkAfterKds403.body.attempts_remaining, + 2, + '403 on KDS preserved accumulated failures and did not reset the failure counter', + ); + + // Trigger 403 on Server App with valid credentials for 'chef' + const chefOnServerApp2 = await postJson(serverAppBaseUrl, '/api/auth/login', { + email: 'chef@test.com', + password: 'CorrectPassword123!', + }); + assert.equal(chefOnServerApp2.status, 403, 'Server App returns 403'); + + // Next bad password must be the 4th failure (1 remaining) + const checkAfterServer403 = await postJson(serverAppBaseUrl, '/api/auth/login', { + email: 'server@test.com', + password: 'badPassword4', + }); + assert.equal(checkAfterServer403.status, 401); + assert.equal( + checkAfterServer403.body.attempts_remaining, + 1, + '403 on Server App preserved accumulated failures and did not reset the failure counter', + ); + clearLoginRateLimits(); // 2. Server App requires 'server-app.use' permission. diff --git a/tests/manager-pin-verification.test.ts b/tests/manager-pin-verification.test.ts index 86501898b..5c011febd 100644 --- a/tests/manager-pin-verification.test.ts +++ b/tests/manager-pin-verification.test.ts @@ -316,6 +316,49 @@ async function run() { 'PIN-only void records matching manager 1 in approved_by', ); + // Test 11a: Concurrent burst of 10 invalid PIN requests locks out after 5 attempts (Issue #1221) + resetBillPinRateLimit(); + const invalidBurst = await Promise.all( + Array.from({ length: 10 }, () => + request(app) + .post(`/api/bills/${billId}/applyDiscount`) + .set(ownerAuth) + .send({ type: 'percentage', value: 10, override_pin: '9999' }), + ), + ); + const statusCounts = invalidBurst.reduce((acc: Record, res: any) => { + acc[res.status] = (acc[res.status] || 0) + 1; + return acc; + }, {}); + assertEqual(statusCounts[403], 5, 'Exactly 5 invalid requests return 403 before lockout'); + assertEqual( + statusCounts[429], + 5, + 'Remaining 5 concurrent requests return 429 without running bcrypt', + ); + + // Any subsequent request returns 429 immediately while locked out + const lockedOutRes = await request(app) + .post(`/api/bills/${billId}/applyDiscount`) + .set(ownerAuth) + .send({ type: 'percentage', value: 10, override_pin: '1111' }); + assertEqual(lockedOutRes.status, 429, 'Subsequent attempt during lockout returns 429'); + + // Test 11b: Concurrent valid PIN requests succeed and do not trigger false 429 lockouts + resetBillPinRateLimit(); + const validBurst = await Promise.all( + Array.from({ length: 5 }, () => + request(app) + .post(`/api/bills/${billId}/applyDiscount`) + .set(ownerAuth) + .send({ type: 'percentage', value: 10, override_pin: '1111' }), + ), + ); + for (const res of validBurst) { + assertEqual(res.status, 200, 'Concurrent valid PIN request succeeds with 200'); + } + resetBillPinRateLimit(); + const results = getResults(); if (results.failed > 0) { throw new Error(`${results.failed} test assertions failed`); From c660712ad76d7366690f107dff7697662799c790 Mon Sep 17 00:00:00 2001 From: khaira777 <777gurkirat@gmail.com> Date: Sun, 18 Oct 2026 15:13:29 -0400 Subject: [PATCH 2/2] no-mistakes(document): docs: document manager PIN concurrency and login counter preservation --- docs/architecture/authentication-and-authorization.md | 10 ++++++---- docs/reference/api.md | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/architecture/authentication-and-authorization.md b/docs/architecture/authentication-and-authorization.md index 855e82eb7..9012fd21b 100644 --- a/docs/architecture/authentication-and-authorization.md +++ b/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). The pre-verified approver is then atomically re-validated +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). The pre-verified approver is then atomically re-validated inside `withTxn` against current SQLite state (active status, owner/manager role, required permissions, and matching `pin_hash`). The check cannot be hoisted into middleware without changing the read the decision is based on, and it is invisible to anything that inspects the route's middleware chain. @@ -172,8 +174,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 diff --git a/docs/reference/api.md b/docs/reference/api.md index 68bb57f59..95a346a66 100644 --- a/docs/reference/api.md +++ b/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. @@ -357,7 +357,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