Add per-IP rate limit to OTP#3999
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesOTP Rate-limiting Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
| const { success } = await ratelimit(2, "1 m").limit( | ||
| `send-otp:${email}:${await getIP()}`, | ||
| ); | ||
| const ip = await getIP(); |
There was a problem hiding this comment.
getIP() can silently return 0.0.0.0 when request IP headers are missing. Since this PR adds an IP-only limiter, fallback usage becomes much more important to observe. Could we expose/log the IP source here, so we can monitor how often OTP requests are falling into the shared fallback bucket?
| ]); | ||
|
|
||
| if (!emailSuccess || !ipSuccess) { | ||
| throw new Error("Too many requests. Please try again later."); |
There was a problem hiding this comment.
would recommend logging more detailed info when we are rejecting OTP requests
There was a problem hiding this comment.
Overall I think this is a great system protection to add (esp. against bots / AI), however would recommend adding a little better logging/observability so we can understand how often we're using FALLBACK_IP_ADDRESS and denying OTP requests.
Also, it would be good to setup some sort of alarming on OTP throttling so we can understand if we are getting attacked vs. many new signups for a new corp client etc. Maybe >10 blocks in 10m to start
Summary by CodeRabbit
Bug Fixes