fix(auth): improve error handling and rate limiting for authentication endpoints#167
Closed
eliteprox wants to merge 2 commits into
Closed
fix(auth): improve error handling and rate limiting for authentication endpoints#167eliteprox wants to merge 2 commits into
eliteprox wants to merge 2 commits into
Conversation
…n endpoints - Updated error messages for user feedback consistency across forgot password, registration, reset password, and email verification flows. - Implemented rate limiting for authentication-related API endpoints to prevent abuse and enhance security. - Refactored response handling to provide clearer messages for various error scenarios, improving user experience. This change enhances the robustness of the authentication system and aligns error handling across different user actions.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size/L
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoRun configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds rate limiting to authentication endpoints using per-IP request tracking with sliding windows, and refactors core auth library error handling. Five auth routes now enforce rate limits, the register function returns a simpler result type, and password comparison uses timing-safe methods. Token error messages are standardized across verification flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
8 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web-next/src/app/(auth)/verify-email/page.tsx (1)
14-14:⚠️ Potential issue | 🟡 MinorEmail address in URL query parameter violates PII handling guidelines.
The email is read from URL search params, but PII such as email addresses should not be included in URL query parameters. This can leak through browser history, server logs, and referer headers.
Consider using an opaque session token or storing the email server-side after registration, then retrieving it via an API call using the verification token as the key.
Based on learnings: "Do not include personal data (PII) such as email addresses in URL query parameters in Next.js projects. Use alternatives like server-side sessions, opaque tokens, or secure backend storage instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(auth)/verify-email/page.tsx at line 14, The page reads the user's email directly from searchParams.get('email'), which exposes PII in the URL; remove that usage and instead retrieve the email server-side via an opaque verification token. Change the flow so the page accepts a verification token (e.g., from searchParams.get('token') or the route), call a secure backend endpoint (e.g., GET /api/verify-email?token=...) that looks up the stored email by token (the email should have been saved server-side during registration), and use the API response to render/verify; in short, delete searchParams.get('email') usage and replace it with a server/API-backed lookup keyed by the opaque token (update page.tsx to call the API and handle errors accordingly).
🧹 Nitpick comments (2)
apps/web-next/src/app/(auth)/reset-password/page.tsx (1)
69-69: Prefer whitelisting user-safe error messages in the catch path.On Line 69, directly surfacing
err.messagecan leak low-level transport/internal strings. Safer pattern: only pass through known UI-safe messages and default everything else.Suggested refactor
} catch (err) { - setError(err instanceof Error ? err.message : 'Unable to reset password. Please try again later.'); + const safeMessage = + err instanceof Error && + [ + 'Invalid or expired reset token', + 'Too many attempts. Please wait and try again.', + 'Unable to reset password. Please try again later.', + ].includes(err.message) + ? err.message + : 'Unable to reset password. Please try again later.'; + + setError(safeMessage); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(auth)/reset-password/page.tsx at line 69, The catch path currently sets state with err.message (setError), which can leak internal details; instead implement a whitelist mapping inside the async handler (the function that calls setError in apps/web-next/src/app/(auth)/reset-password/page.tsx, e.g., the submit/reset handler) that only converts known error types/messages to safe UI strings (for example map "Token expired" -> "Your reset link has expired", "Invalid token" -> "Invalid reset link", network errors -> "Network error, try again"); for any unknown err or non-Error, set a generic message like "Unable to reset password. Please try again later." Ensure you check err instanceof Error and use the mapping before calling setError so only whitelisted messages are surfaced.apps/web-next/src/app/api/v1/auth/register/route.ts (1)
30-39: Password validation errors are now hidden from users.Looking at
apps/web-next/src/lib/api/auth.ts(lines 253-255), theregisterfunction throws"Password must be at least 8 characters"for short passwords. This error is now caught and converted to the generic"Registration failed"message, which removes useful client feedback.Consider distinguishing validation errors from security-sensitive errors:
Suggested approach
} catch (err) { // Surface database connection issues as 503 if (isDatabaseError(err)) { console.error('[AUTH] Database connection error:', (err as Error).message); return errors.serviceUnavailable( 'Database is not available. Please configure DATABASE_URL or try again later.' ); } + // Surface validation errors (safe to expose) + const message = err instanceof Error ? err.message : ''; + if (message.includes('Password must be at least')) { + return errors.badRequest(message); + } + return errors.badRequest('Registration failed'); }Alternatively, validate password length in the route handler before calling
register(), similar to the email/password presence check on lines 21-23.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/auth/register/route.ts` around lines 30 - 39, The catch block in the registration route is swallowing validation messages from lib/api/auth.register (e.g., "Password must be at least 8 characters"); update the handler so validation errors are surfaced to the client instead of returning the generic errors.badRequest("Registration failed"): either (preferred) validate password length in the route before calling register() (like the existing email/password presence checks) and return errors.badRequest with a clear message, or detect the specific validation error thrown by register() in the catch (inspect the error message/type) and return errors.badRequest(err.message) while still handling isDatabaseError(err) as a 503; reference: register (lib/api/auth.register), isDatabaseError, and errors.badRequest used in this route.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/`(auth)/register/page.tsx:
- Around line 62-63: In the catch block of the registration flow (where setError
is called in apps/web-next/src/app/(auth)/register/page.tsx), stop exposing
err.message and always pass a single generic message to setError (e.g., 'Unable
to create account. Please try again later.') regardless of the caught value;
update the catch to call setError with that fixed string so all failure paths
show the same user-facing text.
In `@apps/web-next/src/app/`(auth)/reset-password/page.tsx:
- Around line 62-64: The current check in the reset token verification branch
treats any non-ok response as "Invalid or expired reset token" (the response
variable in page.tsx), so update the error handling around the response.ok check
to branch on response.status: treat 400/401 as invalid/expired token, 429 as a
rate-limit error, 5xx (>=500) as a server error, and otherwise include the
numeric status and response.statusText in the thrown Error to preserve context;
reference the same response object used in the fetch/verify function so callers
can display accurate messages.
In `@apps/web-next/src/app/api/v1/auth/login/route.ts`:
- Around line 54-55: Replace returning the raw service error message from the
catch block — specifically where the local variable error (from err as Error & {
code?: string }) is passed into errors.unauthorized — with a constant generic
auth message (e.g., "Invalid email or password" or "Invalid credentials") and
ensure any detailed error information is retained only in server logs (log error
or err.stack) rather than sent to the client; update the errors.unauthorized
call to use the fixed message and add server-side logging where appropriate.
In `@apps/web-next/src/app/api/v1/auth/reset-password/route.ts`:
- Around line 43-45: The catch-all in the reset-password route is mapping every
exception to errors.badRequest('Invalid or expired reset token'); change it so
only known token validation failures map to that 400 response and unexpected
exceptions return a server error: update the try/catch in the route handler in
route.ts to distinguish by error type (e.g., instanceof
TokenValidationError/TokenExpiredError or error.code from
verifyResetToken/resetPassword functions) and return errors.badRequest(...) for
those known cases; for any other exception, log the error and return
errors.internal('Internal server error') (or rethrow) to preserve server-class
failures; if necessary, modify the token verification code to throw a specific
TokenValidationError so the route handler can reliably detect it.
In `@apps/web-next/src/contexts/auth-context.tsx`:
- Around line 193-197: The login response handling block (the if
(response.status === 503) branch in the auth-context login/sign-in flow) treats
every non-503 failure as invalid credentials; update that logic to explicitly
handle 429 (rate-limited) and other 5xx server errors: check if response.status
=== 429 and throw a rate-limit Error ('Too many requests — please try again
later.'), else if response.status >= 500 && response.status < 600 throw a server
Error ('Unable to sign in right now. Please try again later.'), otherwise fall
back to throwing the invalid credentials Error ('Invalid email or password').
In `@apps/web-next/src/lib/api/auth.ts`:
- Around line 266-271: The existing-email registration path unconditionally
calls resendVerificationEmail(email); add a per-user cooldown check before
invoking resendVerificationEmail by storing and checking a last-resend timestamp
for that email (e.g., Redis, DB column, or in-memory cache) and only calling
resendVerificationEmail(email) when now - lastResend >= COOLDOWN_MS (e.g., 15
minutes); after a successful send update the last-resend timestamp (ensure the
update is atomic or uses a set-if-not-stale pattern to avoid race conditions)
and keep the same external response { created: false } when throttled or sent;
modify the code around resendVerificationEmail in auth.ts to perform the
timestamp check, send only when allowed, and update the timestamp on success.
In `@apps/web-next/src/lib/api/rate-limit.ts`:
- Around line 18-19: The in-memory "store" Map< string, RateLimitEntry >
provides only process-local rate limiting and must be replaced with a shared
backing store (e.g., Redis/Upstash) so limits hold across instances and cold
starts; replace the const store = new Map<string, RateLimitEntry>() with an
async Redis-backed implementation and update all usages to perform atomic
counters and TTL-based expirations (INCR/EXPIRE or Redis scripts) instead of Map
reads/writes, implement connection pooling/retry and fall back safely if the
shared store is unavailable, and ensure the RateLimitEntry semantics (remaining,
reset timestamp) are derived from the shared store keys/values.
- Around line 39-40: The code currently collapses all requests with missing IP
into a single bucket by setting ip = 'unknown'; change this so missing IPs don't
share a global key: when getClientIP(request) returns null, either (a) fail open
by skipping/inhibiting rate-limit enforcement for that request, or (b) derive a
safer per-request fallback (e.g., a generated request-scoped UUID or a short
fingerprint built from non-sensitive headers) and use that in the key instead of
the literal 'unknown'. Update the logic that builds key (`const ip =
getClientIP(request) ?? 'unknown'; const key = `${options.keyPrefix}:${ip}``) to
implement one of these safer behaviors so unrelated clients with no IP don't get
throttled together.
---
Outside diff comments:
In `@apps/web-next/src/app/`(auth)/verify-email/page.tsx:
- Line 14: The page reads the user's email directly from
searchParams.get('email'), which exposes PII in the URL; remove that usage and
instead retrieve the email server-side via an opaque verification token. Change
the flow so the page accepts a verification token (e.g., from
searchParams.get('token') or the route), call a secure backend endpoint (e.g.,
GET /api/verify-email?token=...) that looks up the stored email by token (the
email should have been saved server-side during registration), and use the API
response to render/verify; in short, delete searchParams.get('email') usage and
replace it with a server/API-backed lookup keyed by the opaque token (update
page.tsx to call the API and handle errors accordingly).
---
Nitpick comments:
In `@apps/web-next/src/app/`(auth)/reset-password/page.tsx:
- Line 69: The catch path currently sets state with err.message (setError),
which can leak internal details; instead implement a whitelist mapping inside
the async handler (the function that calls setError in
apps/web-next/src/app/(auth)/reset-password/page.tsx, e.g., the submit/reset
handler) that only converts known error types/messages to safe UI strings (for
example map "Token expired" -> "Your reset link has expired", "Invalid token" ->
"Invalid reset link", network errors -> "Network error, try again"); for any
unknown err or non-Error, set a generic message like "Unable to reset password.
Please try again later." Ensure you check err instanceof Error and use the
mapping before calling setError so only whitelisted messages are surfaced.
In `@apps/web-next/src/app/api/v1/auth/register/route.ts`:
- Around line 30-39: The catch block in the registration route is swallowing
validation messages from lib/api/auth.register (e.g., "Password must be at least
8 characters"); update the handler so validation errors are surfaced to the
client instead of returning the generic errors.badRequest("Registration
failed"): either (preferred) validate password length in the route before
calling register() (like the existing email/password presence checks) and return
errors.badRequest with a clear message, or detect the specific validation error
thrown by register() in the catch (inspect the error message/type) and return
errors.badRequest(err.message) while still handling isDatabaseError(err) as a
503; reference: register (lib/api/auth.register), isDatabaseError, and
errors.badRequest used in this route.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (13)
apps/web-next/src/app/(auth)/forgot-password/page.tsxapps/web-next/src/app/(auth)/register/page.tsxapps/web-next/src/app/(auth)/reset-password/page.tsxapps/web-next/src/app/(auth)/verify-email/page.tsxapps/web-next/src/app/api/v1/auth/forgot-password/route.tsapps/web-next/src/app/api/v1/auth/login/route.tsapps/web-next/src/app/api/v1/auth/register/route.tsapps/web-next/src/app/api/v1/auth/resend-verification/route.tsapps/web-next/src/app/api/v1/auth/reset-password/route.tsapps/web-next/src/app/api/v1/auth/verify-email/route.tsapps/web-next/src/contexts/auth-context.tsxapps/web-next/src/lib/api/auth.tsapps/web-next/src/lib/api/rate-limit.ts
Comment on lines
62
to
64
| if (!response.ok) { | ||
| const data = await response.json(); | ||
| throw new Error(data.message || 'Failed to reset password'); | ||
| throw new Error('Invalid or expired reset token'); | ||
| } |
There was a problem hiding this comment.
Handle non-OK statuses explicitly instead of forcing a token error.
On Line 63, all non-OK responses become Invalid or expired reset token. This will mislabel rate-limited (429) and server (5xx) failures, which conflicts with the PR goal of clearer auth error handling.
Suggested fix
if (!response.ok) {
- throw new Error('Invalid or expired reset token');
+ if (response.status === 400 || response.status === 401) {
+ throw new Error('Invalid or expired reset token');
+ }
+ if (response.status === 429) {
+ throw new Error('Too many attempts. Please wait and try again.');
+ }
+ throw new Error('Unable to reset password. Please try again later.');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!response.ok) { | |
| const data = await response.json(); | |
| throw new Error(data.message || 'Failed to reset password'); | |
| throw new Error('Invalid or expired reset token'); | |
| } | |
| if (!response.ok) { | |
| if (response.status === 400 || response.status === 401) { | |
| throw new Error('Invalid or expired reset token'); | |
| } | |
| if (response.status === 429) { | |
| throw new Error('Too many attempts. Please wait and try again.'); | |
| } | |
| throw new Error('Unable to reset password. Please try again later.'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/`(auth)/reset-password/page.tsx around lines 62 - 64,
The current check in the reset token verification branch treats any non-ok
response as "Invalid or expired reset token" (the response variable in
page.tsx), so update the error handling around the response.ok check to branch
on response.status: treat 400/401 as invalid/expired token, 429 as a rate-limit
error, 5xx (>=500) as a server error, and otherwise include the numeric status
and response.statusText in the thrown Error to preserve context; reference the
same response object used in the fetch/verify function so callers can display
accurate messages.
Comment on lines
+54
to
55
| const error = err as Error & { code?: string }; | ||
| return errors.unauthorized(error.message || 'Invalid email or password'); |
There was a problem hiding this comment.
Avoid returning raw service error messages to login clients.
Line 55 sends arbitrary error.message to the caller. Unexpected internal messages can leak implementation details and still be surfaced as 401. Return a fixed generic auth message and keep details server-side.
🔒 Safer response behavior
const error = err as Error & { code?: string };
- return errors.unauthorized(error.message || 'Invalid email or password');
+ console.error('[AUTH] Login failure:', { code: error.code, message: error.message });
+ return errors.unauthorized('Invalid email or password');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const error = err as Error & { code?: string }; | |
| return errors.unauthorized(error.message || 'Invalid email or password'); | |
| const error = err as Error & { code?: string }; | |
| console.error('[AUTH] Login failure:', { code: error.code, message: error.message }); | |
| return errors.unauthorized('Invalid email or password'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/auth/login/route.ts` around lines 54 - 55,
Replace returning the raw service error message from the catch block —
specifically where the local variable error (from err as Error & { code?: string
}) is passed into errors.unauthorized — with a constant generic auth message
(e.g., "Invalid email or password" or "Invalid credentials") and ensure any
detailed error information is retained only in server logs (log error or
err.stack) rather than sent to the client; update the errors.unauthorized call
to use the fixed message and add server-side logging where appropriate.
Comment on lines
+43
to
45
| } catch { | ||
| return errors.badRequest('Invalid or expired reset token'); | ||
| } |
There was a problem hiding this comment.
Don’t map every failure to “invalid token” client error.
Line 44 now converts all exceptions (including unexpected server/runtime failures) into 400 Invalid or expired reset token. Keep this message for known token validation failures, but return a server-class error for unexpected exceptions.
🧭 Safer error partitioning
- } catch {
- return errors.badRequest('Invalid or expired reset token');
+ } catch (err) {
+ const message = err instanceof Error ? err.message : '';
+ if (message === 'Invalid or expired reset token') {
+ return errors.badRequest('Invalid or expired reset token');
+ }
+ console.error('Reset password error:', err);
+ return errors.serviceUnavailable('Unable to reset password. Please try again later.');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch { | |
| return errors.badRequest('Invalid or expired reset token'); | |
| } | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : ''; | |
| if (message === 'Invalid or expired reset token') { | |
| return errors.badRequest('Invalid or expired reset token'); | |
| } | |
| console.error('Reset password error:', err); | |
| return errors.serviceUnavailable('Unable to reset password. Please try again later.'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/auth/reset-password/route.ts` around lines 43 -
45, The catch-all in the reset-password route is mapping every exception to
errors.badRequest('Invalid or expired reset token'); change it so only known
token validation failures map to that 400 response and unexpected exceptions
return a server error: update the try/catch in the route handler in route.ts to
distinguish by error type (e.g., instanceof
TokenValidationError/TokenExpiredError or error.code from
verifyResetToken/resetPassword functions) and return errors.badRequest(...) for
those known cases; for any other exception, log the error and return
errors.internal('Internal server error') (or rethrow) to preserve server-class
failures; if necessary, modify the token verification code to throw a specific
TokenValidationError so the route handler can reliably detect it.
Comment on lines
+193
to
197
| if (response.status === 503) { | ||
| throw new Error('Unable to sign in right now. Please try again later.'); | ||
| } | ||
| throw new Error('Invalid email or password'); | ||
| } |
There was a problem hiding this comment.
Handle rate-limit and other server errors explicitly in login failures.
At Line 196, all non-503 failures are treated as invalid credentials. That misclassifies 429 (rate-limited) and other 5xx failures, producing incorrect user feedback in the hardened auth flow.
Proposed patch
if (!response.ok) {
- if (response.status === 503) {
+ if (response.status === 429) {
+ throw new Error('Too many sign-in attempts. Please try again later.');
+ }
+ if (response.status >= 500) {
throw new Error('Unable to sign in right now. Please try again later.');
}
throw new Error('Invalid email or password');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (response.status === 503) { | |
| throw new Error('Unable to sign in right now. Please try again later.'); | |
| } | |
| throw new Error('Invalid email or password'); | |
| } | |
| if (!response.ok) { | |
| if (response.status === 429) { | |
| throw new Error('Too many sign-in attempts. Please try again later.'); | |
| } | |
| if (response.status >= 500) { | |
| throw new Error('Unable to sign in right now. Please try again later.'); | |
| } | |
| throw new Error('Invalid email or password'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/contexts/auth-context.tsx` around lines 193 - 197, The
login response handling block (the if (response.status === 503) branch in the
auth-context login/sign-in flow) treats every non-503 failure as invalid
credentials; update that logic to explicitly handle 429 (rate-limited) and other
5xx server errors: check if response.status === 429 and throw a rate-limit Error
('Too many requests — please try again later.'), else if response.status >= 500
&& response.status < 600 throw a server Error ('Unable to sign in right now.
Please try again later.'), otherwise fall back to throwing the invalid
credentials Error ('Invalid email or password').
Comment on lines
+266
to
+271
| // Preserve a uniform external response while still helping legitimate users | ||
| // who may have an unverified account. | ||
| resendVerificationEmail(email).catch((err) => { | ||
| console.error('[AUTH] Failed to handle existing-email registration:', (err as Error).message); | ||
| }); | ||
| return { created: false }; |
There was a problem hiding this comment.
Add resend throttling for existing-email registration path.
Line 268 triggers resendVerificationEmail(email) for every existing-email registration attempt. This can be abused for inbox flooding and deliverability degradation, especially from distributed sources. Add a per-user resend cooldown (e.g., timestamp check) before sending another verification email.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/api/auth.ts` around lines 266 - 271, The existing-email
registration path unconditionally calls resendVerificationEmail(email); add a
per-user cooldown check before invoking resendVerificationEmail by storing and
checking a last-resend timestamp for that email (e.g., Redis, DB column, or
in-memory cache) and only calling resendVerificationEmail(email) when now -
lastResend >= COOLDOWN_MS (e.g., 15 minutes); after a successful send update the
last-resend timestamp (ensure the update is atomic or uses a set-if-not-stale
pattern to avoid race conditions) and keep the same external response { created:
false } when throttled or sent; modify the code around resendVerificationEmail
in auth.ts to perform the timestamp check, send only when allowed, and update
the timestamp on success.
Comment on lines
+18
to
+19
| const store = new Map<string, RateLimitEntry>(); | ||
|
|
There was a problem hiding this comment.
Process-local Map is insufficient for production-grade abuse control.
This limiter only protects a single runtime instance. In multi-instance/serverless deployments, attackers can bypass limits by spreading requests, and limits reset on cold starts. Use a shared backing store (Redis/Upstash/DB) for consistent cross-instance enforcement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/api/rate-limit.ts` around lines 18 - 19, The in-memory
"store" Map< string, RateLimitEntry > provides only process-local rate limiting
and must be replaced with a shared backing store (e.g., Redis/Upstash) so limits
hold across instances and cold starts; replace the const store = new Map<string,
RateLimitEntry>() with an async Redis-backed implementation and update all
usages to perform atomic counters and TTL-based expirations (INCR/EXPIRE or
Redis scripts) instead of Map reads/writes, implement connection pooling/retry
and fall back safely if the shared store is unavailable, and ensure the
RateLimitEntry semantics (remaining, reset timestamp) are derived from the
shared store keys/values.
Comment on lines
+39
to
+40
| const ip = getClientIP(request) ?? 'unknown'; | ||
| const key = `${options.keyPrefix}:${ip}`; |
There was a problem hiding this comment.
Avoid collapsing missing-IP traffic into one global bucket.
Using 'unknown' as a shared key means unrelated clients can throttle each other whenever IP extraction fails. Prefer failing open for missing IP (or use a safer fallback fingerprint) to avoid broad false-positive 429s.
🛠️ Minimal defensive adjustment
- const ip = getClientIP(request) ?? 'unknown';
+ const ip = getClientIP(request);
+ if (!ip) return null;
const key = `${options.keyPrefix}:${ip}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/api/rate-limit.ts` around lines 39 - 40, The code
currently collapses all requests with missing IP into a single bucket by setting
ip = 'unknown'; change this so missing IPs don't share a global key: when
getClientIP(request) returns null, either (a) fail open by skipping/inhibiting
rate-limit enforcement for that request, or (b) derive a safer per-request
fallback (e.g., a generated request-scoped UUID or a short fingerprint built
from non-sensitive headers) and use that in the key instead of the literal
'unknown'. Update the logic that builds key (`const ip = getClientIP(request) ??
'unknown'; const key = `${options.keyPrefix}:${ip}``) to implement one of these
safer behaviors so unrelated clients with no IP don't get throttled together.
|
This PR has conflicts with the base branch. Please rebase to resolve them: git fetch origin
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThe |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| // API returns { error: { code, message } } or { message } | ||
| const msg = body.error?.message || body.message || 'Login failed'; | ||
| throw new Error(msg); | ||
| if (response.status === 503) { |
Contributor
There was a problem hiding this comment.
Users who hit the rate limit during login see "Invalid email or password" instead of a rate limit message
7 tasks
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
This PR addresses some quality issues flagged in #162 which originate from the backend API. The changes enhance the robustness of the authentication system and align error handling across different user actions.
Changes
Type
Plugin(s) Affected
Checklist
npm run lint)npm run build)Breaking Changes
None
Screenshots / Recordings
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores