Repository navigation
fix(otp): reserve a verification attempt atomically before comparing the code - #9774
Conversation
…the code Chat and public file OTP verify read the attempt count, compared the guess, and only then incremented, so concurrent submissions all passed the limit check on the same stale count and every one was compared. Reserve the attempt first (Redis Lua script / one conditional Postgres UPDATE) and compare only against the reserved value, so at most MAX_OTP_ATTEMPTS guesses are ever checked against one code. Chat verify now uses safeCompare like the file path.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
 [Medium impact] The PR appears safe to merge; no new actionable issues remain. SummaryThe PR reserves an OTP attempt before comparing the submitted code on both Postgres and Redis.
Reviews (4) · Last reviewed commit: "test(otp): prove rejected verify request..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…n tests Move the constant-time compare and consume-on-match into verifyOTP so both OTP callers share it; drop getOTP and the decodeOTPValue export now that nothing reads a code without reserving an attempt; remove the unreachable Lua branch; keep the lockout log free of the email address. Tests: assert the Redis TTL is preserved rather than reset, drop the non-null assertion, and use createMockRequest.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…port TTL rounds a key with under a second left to 0, so the reservation script rewrote it without an expiry and the code never expired. Read PTTL and rewrite with PX, deleting a code with no remaining lifetime. deleteOTP is now only used inside the module.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…t real storage Replace the mock-asserting authorization unit tests with integration cases: with email auth disabled or the address removed from the allow list, five requests carrying the correct code are refused without a cookie, and the code still works once the chat is restored.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Summary
reserveOTPAttemptatomically spends one attempt and returns the stored code; callers compare only after reserving, so at mostMAX_OTP_ATTEMPTSguesses are ever checked against a code regardless of concurrencyUPDATE ... RETURNING(row lock + re-check replaces the optimistic retry loop); attempt parse uses a regexsubstringso otherverificationrows can never fail a castsafeComparelike the file pathincrementOTPAttemptsand the stale-read lockout branch; user-facing responses unchanged (4 wrong → invalid, 5th wrong → 429, correct on 5th still works)Type of Change
Testing
app/api/chat/[identifier]/otp/route.integration.tsdrives the real PUT handler against real Postgres and Rediscontent-sharing.integration.ts(file share OTP + policy-change race) passes with the spy moved to the new primitiveroute.test.tsupdated; removed two mock-restating brute-force tests and a DB-expiry test now owned by the integration suitebun run lint,check:test-patterns,check:comment-hygiene,check:utils,check:boundaries,check:source-conventions; type-check and full audits in CIChecklist
test-auditauthoring gate)🤖 Generated with Claude Code