Conversation
…tion locks This commit completely resolves the Time-Of-Check to Time-Of-Use (TOCTOU) race conditions in the OTP validation and generation flows. Validation: - Removed in-memory comparison logic for `attempts` and `hash`. - Replaced with a single, atomic `markUsed()` SQL update. - Updated `incrementAttempts()` to safely target only the latest active challenge. Generation: - Solved the empty-lock race condition where `FOR UPDATE` failed to lock when 0 codes existed. - Added `verification_generation_locks` table to act as a persistent mutex anchor. - Updated `lockActiveForUpdate()` usage to properly lock the identity scope before querying. Co-authored-by: Maatify <130119162+Maatify@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
- Fixed `markUsed()` to strictly update exactly one valid challenge row matching the code hash, to prevent consuming multiple codes if duplicates were somehow generated. Returned boolean check strictly uses `rowCount() === 1`. - Fixed `incrementAttempts()` to ensure it only increments attempts on active challenges that are still not expired by time (`expires_at >= :now`). Co-authored-by: Maatify <130119162+Maatify@users.noreply.github.com>
…e path - Added `AND attempts < max_attempts` to the target selection logic of `incrementAttempts()`. - This ensures that failed validation guesses never increment a row that has already been exhausted but hasn't had its status transitioned yet due to timing. Co-authored-by: Maatify <130119162+Maatify@users.noreply.github.com>
1 participant
Summary
Addresses critical concurrency vulnerabilities identified in the recent security audit. The
maatify/verificationmodule is now safe against brute-force amplification and generation-limit bypassing under high concurrency.Changes
verification_generation_lockstable indatabase/verification_generation_locks.sql.VerificationCodeValidatorto delegate state enforcement (OTP hash, expiration, attempt counts) entirely to an atomic database query (markUsed). This prevents TOCTOU race conditions where multiple requests could bypass the attempt limit.acquireGenerationLocktoPdoVerificationCodeRepositorywhich upserts and locks a row in the newverification_generation_lockstable. This prevents empty-set race conditions where multiple concurrent generation requests could insert OTPs simultaneously.VerificationCodeValidatorasClockInterfaceis no longer needed since expiration is handled in the query.PR created automatically by Jules for task 6459033912038507901 started by @Maatify