Repository navigation
fix(main): serialize manager PIN verification and preserve failed login counter on 403 - #1222
Merged
Merged
Conversation
…s-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
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…in counter preservation
…1222-1229-r1 # Conflicts: # main/routes/bills.ts
…1222-1229-r1 # Conflicts: # main/routes/bills.ts # main/routes/orders.ts
…1222-1229-r1 # Conflicts: # docs/architecture/authentication-and-authorization.md # main/routes/orders.ts # tests/manager-pin-verification.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Harden manager PIN verification concurrency and preserve cross-server failed login counter on 403
What Changed
main/middleware/security.tswithwithPinVerificationLockand canonical IP normalization, serializing verification across bill and order routes to prevent concurrent brute-force races before bcrypt evaluations.resetSuccessfulLoginin KDS and Server App login handlers to execute only after permission checks pass, preserving accumulated failed login attempt counters on 403 responses.Risk Assessment
✅ Low: The changes are well-bounded, security-hardening bug fixes that correctly serialize manager PIN verification to eliminate race conditions and ensure cross-server failed login counters are preserved on 403 permission denials.
Testing
Exercised the real running FloCafe backend servers (Main Express API :3101, Standalone KDS :3102, Server App :3103) against a disposable SQLite database. Verified that cross-server 403 responses preserve accumulated failed login attempts up to the 5-attempt lockout, and verified that concurrent manager PIN bursts serialize correctly via withPinVerificationLock yielding exactly 5 403s before immediate 429 lockouts without false positives or rate-limit bypasses. All checks and live scenarios passed; no UI/renderer changes were altered in this backend auth/PIN hardening commit so no screenshot artifact applies.
Evidence: Cross-Server 403 Counter Preservation JSON Log
{ "scenario": "Cross-Server 403 Counter Preservation", "passed": true, "steps": 9, "verified": "403 on KDS and Server App does not reset failure counter; 5th failure triggers 429 lockout" }Evidence: Concurrent Invalid PIN Burst Lockout JSON Log
{ "scenario": "Concurrent Invalid PIN Verification Burst", "totalRequests": 10, "durationMs": 998, "statusDistribution": { "403": 5, "429": 5 } }Evidence: PIN Lockout Persistence JSON Log
{ "scenario": "Manager PIN Lockout Persistence", "status": 429, "error": "Too many PIN attempts. Try again in 15 minutes.", "discount_amount": 0 }Evidence: Concurrent Valid PIN Burst Success JSON Log
{ "scenario": "Concurrent Valid PIN Requests", "count": 5, "statuses": [200, 200, 200, 200, 200] }Evidence: Order Item Void PIN Concurrency JSON Log
{ "scenario": "Order Item Void PIN Concurrency & Rate Limiting", "burstCounts": { "403": 5, "429": 5 }, "itemStatusBefore": "preparing", "validVoidStatus": 200, "itemStatusAfter": "voided" }Evidence: Live Product Validation Summary Report
# Live Product Validation: Manager PIN Concurrency & 403 Counter Preservation All 5 live scenarios passed against real running backend servers with full evidence captured.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
node tests/run-electron-node-test.cjs tests/manager-pin-verification.test.tsnpm run test:cross-server-login-rate-limitsnode scratch/live-product-hardening-test.js (Main Express API :3101, KDS Server :3102, Server App :3103)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.