feat: implement reusable rate limiting middleware#99
Conversation
|
@pari-28 is attempting to deploy a commit to the participationcorner2025-8967's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 SummaryThe pull request introduces a rate limiting feature to prevent excessive requests from a single IP address. It adds a new utility function 📂 Files Changed
🎭 Code PoemRate limiting is now in place, 🚨 Bugs & Architectural Violations* The `rateLimit` function uses an in-memory store, which may not be suitable for production deployments with multiple server instances. Consider using a shared backend like Upstash Redis or Vercel KV. * The `rateLimit` function does not handle errors well. If an error occurs while checking the rate limit, it will be swallowed and not propagated to the caller. * The `getClientIp` function assumes that the `x-forwarded-for` header is always present and correctly formatted. Consider adding more robust handling for this header. * The `rateLimit` function does not provide any logging or monitoring capabilities. Consider adding logging statements to track rate limit events. * No accessibility features are added or modified in this pull request, but it's essential to ensure that any new features or changes do not introduce accessibility issues.💡 Suggestions & Best Practices* Consider adding more robust error handling to the `rateLimit` function, including logging and propagation of errors to the caller. * Add logging statements to track rate limit events, such as when a request is blocked due to exceeding the rate limit. * Consider using a more robust IP address parsing library instead of relying on the `x-forwarded-for` header. * Add documentation for the `rateLimit` function, including its parameters, return values, and any exceptions it may throw. * Consider adding a `defer` statement to the `rateLimit` function to ensure that it does not block the main thread. * Review the code for any potential performance issues, such as excessive memory allocation or computationally expensive operations. |
|
@pari-28 Great initiative taking on security! The implementation logic itself is very clean, but there is a critical architectural blocker we need to fix before merging.
Could you update the implementation to use Prisma for the storage? Let me know if you need any pointers on designing the schema for it! |
|
Hi @rushikesh-bobade, thank you for the detailed review! I'm updating the implementation to use a Prisma-backed Should the Also, if you have any recommendations for the Thanks! |
|
Hi @pari-28! Great questions. 1. Prisma Migration WorkflowWe actually just merged a massive infrastructure update (PR #110) specifically to make this easier for contributors! You can now manage the database locally:
2. Suggested Schema DesignHere is a lightweight, performant schema you can add to the bottom of model RateLimit {
ip String @id
count Int @default(1)
resetAt DateTime
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
}Note: We use the 3. Handling IPs Securely on VercelBecause we host on Vercel, the safest way to extract the true client IP (and prevent export function getClientIp(request: Request) {
return request.headers.get("x-vercel-forwarded-for") ||
request.headers.get("x-real-ip") ||
request.headers.get("x-forwarded-for")?.split(",")[0].trim() ||
"unknown_ip";
}Let me know if you run into any issues spinning up the local Docker database! |
beea7f1 to
deaa5be
Compare
📝 SummaryThe provided pull request introduces a rate limiting feature to prevent excessive requests from a single IP address. It adds a new 📂 Files Changed
🎭 Code PoemRate limiting's in place now, 🚨 Bugs & Architectural ViolationsThe provided code seems to follow the architectural rules, but there are a few potential issues: * The `rateLimit` function uses an in-memory store, which may not be suitable for a distributed environment. As mentioned in the documentation, a shared backend like Upstash Redis or Vercel KV should be considered for production deployments. * The `getClientIp` function relies on specific headers to determine the client's IP address. This might not work in all scenarios, especially if the request is proxied or load-balanced. * There is no error handling for the `rateLimit` function. If an error occurs while checking or updating the rate limit, it will not be caught or propagated. * The `RateLimit` model has a default value of 1 for the `count` field, but it's not clear why this is the case. It might be better to use a more explicit value or to calculate the initial count based on the request's timestamp. Looks solid in terms of architectural rules, but these potential issues should be addressed.💡 Suggestions & Best PracticesSome suggestions for improving the code: * Consider using a more robust IP address detection mechanism, such as using a library like `ip-address` or `geoip-lite`. * Add error handling for the `rateLimit` function to ensure that any errors are caught and propagated correctly. * Use a more explicit value for the `count` field in the `RateLimit` model, or calculate the initial count based on the request's timestamp. * Consider adding a `max` field to the `RateLimit` model to store the maximum allowed requests within the time window. * Use a more secure way to store the `accessToken` and `refreshToken` fields in the `Integration` model, such as using a secure encryption mechanism. * Consider adding a `ttl` (time to live) field to the `RateLimit` model to automatically remove expired rate limits. |
|
I've addressed the requested changes:
Please let me know if you'd like me to make any further refinements. Thanks for the guidance! |
|
Hi @pari-28, Amazing job adapting to the Prisma schema! The migration file is perfect, the However, during my line-by-line review, I spotted a critical security vulnerability in the rate limit logic: A TOCTOU (Time-Of-Check to Time-Of-Use) Race Condition. Currently, you are calling The Fix: const now = new Date();
const resetAt = new Date(now.getTime() + windowMs);
// 1. Atomically increment the count (or create if it doesn't exist)
const result = await prisma.rateLimit.upsert({
where: { ip },
create: { ip, count: 1, resetAt },
update: { count: { increment: 1 } },
});
// 2. If the reset period has expired, reset it back to 1
if (result.resetAt <= now) {
await prisma.rateLimit.update({
where: { ip },
data: { count: 1, resetAt },
});
return;
}
// 3. Now safely check the atomic result
if (result.count > limit) {
throw new Response(
JSON.stringify({ error: "Too many attempts. Please try again later." }),
{ status: 429, headers: { "Content-Type": "application/json" } }
);
}
Could you update rate-limit.server.ts with this atomic upsert pattern? Once you push that fix, this is good to merge! Great work on this so far. |
Description
This PR introduces a reusable backend rate-limiting utility to improve security against brute-force attacks and excessive API requests.
Changes made
rateLimit(request, limit, windowMs)utility inapp/utils/rate-limit.server.tslogin-page.tsx)signup-page.tsx)api.insights.ts)429 Too Many Requestswith a clear error message when the request limit is exceededCONTRIBUTING.mdwith usage instructions and documented the limitations of the in-memory implementation for serverless environments.Notes
reset-passwordaction, so no changes were made there.Related Issues
Fixes #74
Type of Change
Checklist:
Screenshots (if applicable)
N/A (Backend changes only)