Skip to content

fix(auth): prevent concurrent PIN limit bypass race and preserve cross-server failed login counter #1221

Closed

Description

@khaira777

Summary

Investigation into authentication and rate limiting controls identified two regressions/defects in Workstream C:

  1. Cross-Server Failed-Login Counter Reset (PR fix(auth): enforce install-wide login attempt rate limiting across servers #1199 follow-up)

    • In main/kds-server.ts and main/server-app.ts, resetSuccessfulLogin(ip) was invoked immediately after bcrypt password verification, before evaluating role and permission constraints (kitchen.use, assigned stations, server-app.use).
    • When an account with valid credentials but without required permissions attempts login, the response is HTTP 403 Forbidden. However, because resetSuccessfulLogin(ip) had already executed, the 5-attempt failed-login lockout budget on that source IP was prematurely wiped, resetting remaining attempts back to 5.
    • Correction: Move resetSuccessfulLogin(ip) to execute only after all authorization checks pass and immediately before issuing the JWT token and returning HTTP 200.
  2. Concurrent Manager PIN Limit Race Condition (PR fix(auth): asynchronously verify manager PINs without blocking event loop #1197 follow-up)

    • Asynchronous manager PIN verification (verifyPinAsync) runs outside the database transaction to prevent event-loop blocking.
    • However, isPinRateLimited(key) was checked before bcrypt, while checkPinRateLimit(key) was only called after bcrypt failed.
    • When multiple requests with invalid PINs arrive simultaneously from the same client IP, all requests pass isPinRateLimited(key) concurrently and execute expensive bcrypt calculations simultaneously across candidate managers before any request updates the failure counter.
    • Furthermore, a naive pre-increment would introduce an easy denial-of-service condition or count valid concurrent approvals as failures.
    • Correction: Implement serialized verification locks per rate-limit key with concurrency accounting. Requests queued for the same key await prior completion; when their turn arrives, isPinRateLimited(key) is re-evaluated. If prior failures have exhausted the 5-attempt limit, subsequent requests immediately return HTTP 429 without executing bcrypt. Valid approvals clear the failure count without consuming budget.

Acceptance Criteria

  • A valid password attempt resulting in HTTP 403 (due to lacking kitchen.use or server-app.use) preserves existing failed-login counts and does not reset the lockout counter.
  • Simultaneous invalid PIN requests from the same IP serialize verification such that after 5 failed attempts, all remaining concurrent/queued requests immediately return HTTP 429 without performing redundant bcrypt calculations.
  • Successful approvals are never counted as failures and clear the failure count.
  • Concurrent valid approvals do not trigger false-positive 429 rate limit errors.
  • Unrelated IPs and operation keys continue to execute concurrently without mutual blocking.
  • Regression tests cover both cross-server 403 counter preservation and concurrent PIN rate limiting.

Activity

  1. khaira777 commented on Oct 20, 2026

    @khaira777
    ContributorAuthor

    Verified and delivered on main via PR #1222 (80769e0bfc31e65dc22d651439a449566d443a70).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions