fix: prevent rate limiter quota leak on limit exceeded (#5895)#5932
Conversation
When using KV, the INCR command incremented the counter before checking the limit, causing rejected requests to still consume quota. This meant the rate limiter effectively had a lower limit than configured. Now checks the current count first and only increments if under the limit. Fixes JhaSourav07#5895
|
@vipul674 is attempting to deploy a commit to the jhasourav07's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@JhaSourav07 Ready for review. |
📦 Next.js Bundle Size Report (Gzipped Sizes)✨ No significant bundle size changes detected. 📊 Summary of Totals
|
|
@JhaSourav07 Review Reminder |
Aamod-Dev
added
level:intermediate
Aamod-Dev
left a comment
Aamod-Dev
left a comment
There was a problem hiding this comment.
Difficulty: intermediate – Changes rate-limit.ts to GET+check before INCR, preventing quota leak when limit exceeded (39 additions, 12 deletions).
Quality: clean – Fixes INCR-on-rejection bug.
Type: bug + security – Prevents rate limiter bypass issue #5895.
Important security fix!
JhaSourav07
added
the
gssoc:approved
|
🎉 Congratulations @vipul674! Your PR has been successfully merged. 🚀 Thank you for contributing to CommitPulse. Your work helps us build a better tool for the community.
Keep building! 💻✨ |
3 participants
Fixes #5895
Problem
When using Upstash Redis/KV, the rate limiter used
INCRto increment the counter before checking if the limit was exceeded. This meant rejected requests still consumed quota, effectively lowering the rate limit below the configured value.For example, with
limit=10, if a user makes 10 requests (counter=10), the 11th request would:But now the counter is 11, meaning the user is permanently one request over the limit until the window resets.
Fix
Check the current count first using
GET+TTL, and onlyINCRif the count is below the limit. Rejected requests no longer consume quota.