feat(verification): back email/reset codes with Redis when available#4537
Open
jr-ni wants to merge 2 commits intoQuantumNous:mainfrom
Open
feat(verification): back email/reset codes with Redis when available#4537jr-ni wants to merge 2 commits intoQuantumNous:mainfrom
jr-ni wants to merge 2 commits intoQuantumNous:mainfrom
Conversation
The verification helpers stored codes in an in-process map keyed by purpose+key, which breaks horizontal scaling: a code registered on one pod is invisible to another, so signup/password-reset round-trips that hit different replicas always fail. When RedisEnabled, store codes at `verify_code:<purpose>:<key>` with the existing `VerificationValidMinutes` TTL so Redis ages them out automatically. Verification fails closed on Redis errors. The in-memory path is preserved for single-pod deployments and is unchanged. Adds a unit test covering the in-memory contract (registration, wrong code, purpose isolation, deletion). The Redis branch was validated live in a multi-pod deployment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds an optional Redis-backed storage path for verification codes controlled by Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Verif as Verification Logic
participant Redis as Redis Cache
participant Memory as In-Memory Store
rect rgba(76, 175, 80, 0.5)
Note over App,Redis: Redis Enabled Path
App->>Verif: RegisterVerificationCodeWithKey(key, code, purpose)
Verif->>Redis: SET verification:purpose:key = code (EX TTL)
Redis-->>Verif: OK
App->>Verif: VerifyCodeWithKey(key, inputCode, purpose)
Verif->>Redis: GET verification:purpose:key
Redis-->>Verif: stored code / nil
Verif-->>App: constant-time compare result
end
rect rgba(33, 150, 243, 0.5)
Note over App,Memory: Redis Disabled Path
App->>Verif: RegisterVerificationCodeWithKey(key, code, purpose)
Verif->>Memory: store map[purpose:key] = code with timestamp
Memory-->>Verif: stored
App->>Verif: VerifyCodeWithKey(key, inputCode, purpose)
Verif->>Memory: lookup map[purpose:key]
Memory-->>Verif: stored code / missing
Verif-->>App: constant-time compare result
end
rect rgba(244, 67, 54, 0.5)
Note over App,Verif: Deletion
App->>Verif: DeleteKey(key, purpose)
alt Redis Enabled
Verif->>Redis: DEL verification:purpose:key
Redis-->>Verif: deleted
else Redis Disabled
Verif->>Memory: delete map[purpose:key]
Memory-->>Verif: removed
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
common/verification.go (1)
71-71: Consider constant-time comparison for verification codes.The direct string comparison
stored == codeis theoretically vulnerable to timing attacks. While the risk is low due to code entropy and TTL, usingsubtle.ConstantTimeComparewould follow security best practices.🔒 Optional: use constant-time comparison
+import "crypto/subtle" + func VerifyCodeWithKey(key string, code string, purpose string) bool { if RedisEnabled { stored, err := RedisGet(verificationRedisKey(key, purpose)) if err != nil { if err != redis.Nil { SysError("VerifyCodeWithKey: redis get failed: " + err.Error()) } return false } - return stored == code + return subtle.ConstantTimeCompare([]byte(stored), []byte(code)) == 1 } // ... also update line 80 for consistency ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/verification.go` at line 71, Replace the direct equality check (the line returning `stored == code`) with a constant-time comparison using `subtle.ConstantTimeCompare`: first ensure the lengths match (return false if len differs) then compare the byte slices and return whether the result equals 1; also add the `crypto/subtle` import. Update the function that contains `return stored == code` accordingly (e.g., VerifyCode/Compare function) so it uses the length check plus `subtle.ConstantTimeCompare([]byte(stored), []byte(code)) == 1`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/verification.go`:
- Around line 44-50: The Redis branch in RegisterVerificationCodeWithKey
currently logs but swallows RedisSet errors; change
RegisterVerificationCodeWithKey to return an error (update its signature), and
when RedisSet(verificationRedisKey(key, purpose), code, ttl) fails return that
error instead of just SysError; update all callers (e.g., the handlers in
controller/misc.go) to handle the returned error (respond to the user or retry)
and preserve the existing behavior when RedisEnabled is false by returning nil
on success.
- Around line 83-89: DeleteKey currently calls RDB.Del directly; replace that
direct Redis call with the existing wrapper RedisDelKey for consistency with
RedisSet/RedisGet. In the DeleteKey function, remove the
RDB.Del(context.Background(), verificationRedisKey(key, purpose)).Err() usage
and call RedisDelKey(verificationRedisKey(key, purpose)) instead, keeping the
RedisEnabled check and SysError handling consistent with other callers like
RegisterVerificationCodeWithKey and VerifyCodeWithKey.
---
Nitpick comments:
In `@common/verification.go`:
- Line 71: Replace the direct equality check (the line returning `stored ==
code`) with a constant-time comparison using `subtle.ConstantTimeCompare`: first
ensure the lengths match (return false if len differs) then compare the byte
slices and return whether the result equals 1; also add the `crypto/subtle`
import. Update the function that contains `return stored == code` accordingly
(e.g., VerifyCode/Compare function) so it uses the length check plus
`subtle.ConstantTimeCompare([]byte(stored), []byte(code)) == 1`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0125f7f5-e129-4cfe-809d-e0b9645c023d
📒 Files selected for processing (2)
common/verification.gocommon/verification_test.go
- Use crypto/subtle.ConstantTimeCompare for code equality check to avoid timing-attack surface, even though codes are random UUIDs. - Use the existing common.RedisDelKey wrapper in DeleteKey instead of calling RDB.Del directly, matching the rest of the file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
Thanks for the review @coderabbitai. Pushed b1b28bd addressing two of the three points:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The deferral on error propagation is entirely reasonable. The fail-closed path ( Would you like me to open a follow-up GitHub issue to track the error-propagation improvement for 🧠 Learnings used |
jr-ni
commented
Apr 29, 2026
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
1 participant
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.
Summary
`common/verification.go` stores email-verification and password-reset codes in an in-process `map` (`verificationMap`) protected by a `sync.Mutex`, with no Redis branch. This breaks horizontal scaling: when new-api runs with >1 replica behind a load balancer, the first request (e.g. `POST /api/email_verification`) registers the code in pod A's memory, the next request (`POST /api/user/register`) is routed to pod B and finds nothing — registration always fails.
Since `common/redis.go` is already integrated and most other in-memory fallbacks (caches, rate limiters, notify limit) already check `RedisEnabled`, it was natural to extend the same pattern here.
Changes
Test plan
Backwards compatibility
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests