fix(security): add rate limiting to auth and email endpoints - #2875
Draft
VirginiaWu11 wants to merge 3 commits into
Draft
fix(security): add rate limiting to auth and email endpoints#2875VirginiaWu11 wants to merge 3 commits into
VirginiaWu11 wants to merge 3 commits into
Conversation
Adds express-rate-limit to slow down brute force / credential stuffing and abuse against auth-adjacent endpoints, which previously had no request limits: - strictAuthLimiter (5 req / 15 min per IP): login, forgotPassword, resetPassword - moderateAuthLimiter (10 req / hour per IP): register, resendConfirmationEmail, confirmRegister, email/contact Also sets `trust proxy` in server.ts so the limiter keys off the real client IP behind Heroku's proxy rather than the proxy's own address. CAPTCHA on register/contact and per-account (DB-tracked) lockout are left as separate follow-ups — out of scope for this pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes two Major findings from independent review of 96e711e: - rate-limit.ts exported single shared limiter instances reused across multiple unrelated routes (login/forgotPassword/resetPassword sharing one instance, register/resendConfirmationEmail/confirmRegister/contact sharing another). Since express-rate-limit keys by IP only, this meant all routes on one shared instance drained the same counter/store -- e.g. 5 failed logins would also 429 a subsequent forgotPassword request from the same IP. Fixed by converting to factory functions (createStrictAuthLimiter/createModerateAuthLimiter) called separately at each route registration, giving each route its own limiter/store. - The client's login() silently swallowed a 429 response and returned undefined, causing the UI to fall through to "the password is incorrect... use Forgot Password" -- actively misleading once rate limited, especially compounded by the bug above. Fixed by having login() pass through the server's RATE_LIMITED response instead of discarding it, and Login.tsx now shows the actual rate-limit message. Also aligned the 429 response body shape ({isSuccess, code, message}) with the rest of the account API's convention instead of an ad hoc {error} shape (review Nit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
VirginiaWu11
marked this pull request as draft
September 1, 2026 02:07
Extends the login-page 429 fix (49697e4) to the other rate-limited account endpoints, which had the same class of bug: their service functions let axios throw on 429, so a rate-limited request fell into each component's generic/hardcoded error handling instead of showing the actual "too many attempts" message. Reported by the user after manually testing forgotPassword and seeing a "Server error" toast (axios's default message for a thrown 429). - account-service.ts: extracted the login() 429-handling into a shared postAuthRequest() helper and applied it to register, resendConfirmationEmail, forgotPassword, resetPassword, and confirmRegister -- a 429 now returns the server's {isSuccess:false, code:"RATE_LIMITED", message} body instead of throwing. Any other error still throws, preserving each caller's existing behavior. - ForgotPassword.tsx and ResetPassword.tsx already show response.message in their fallback branch, so no component change was needed there once the service layer stopped throwing. - Register.tsx's fallback branch showed a hardcoded, unrelated message for any non-REG_DUPLICATE_EMAIL failure -- added an explicit RATE_LIMITED branch. - ConfirmEmail.tsx's confirmRegister effect showed no toast at all on failure -- added a RATE_LIMITED branch with the real message. - Login.tsx's AUTH_NOT_CONFIRMED branch called resendConfirmationEmail() without checking its result, always showing a "confirmation email sent" toast regardless of outcome. Since that call no longer throws on 429, it would have silently claimed success while actually rate-limited -- fixed to check the result and show the real message when rate-limited. Verified: typecheck (client) clean, build (client) clean, manual smoke test confirms the server returns the RATE_LIMITED body at the configured threshold and the client no longer discards it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Problem
No
express-rate-limitor equivalent existed anywhere inserver/./api/accounts/login,/register,/forgotPassword,/resetPassword,/resendConfirmationEmail,/confirmRegister, and/api/email/contactaccepted unlimited requests, enabling credential-stuffing/brute-force against login, mass account creation, and mail-relay abuse.Fix
express-rate-limitwith per-IP limiting:login,forgotPassword,resetPasswordregister,resendConfirmationEmail,confirmRegister,email/contactcreateStrictAuthLimiter()/createModerateAuthLimiter()) so unrelated actions don't share a rate-limit budget.app.set("trust proxy", 1)so the limiter resolves the real client IP behind Heroku's proxy rather than the proxy's own address.login()now surfaces the server's actual rate-limit message on HTTP 429 instead of falling through to a misleading "incorrect password" message.{isSuccess, message}convention.Explicitly out of scope (flagged, not silently decided)
Checks
npm run typecheck(client + server): cleannpm run lint(server): cleannpx jest --ci(server): 10/27 failing — pre-existing stale-snapshot baseline inaccount.test.ts(unrelated to this change), no new failuresnpm run build(client + server): clean/login's budget doesn't affect/forgotPassword), confirmedtrust proxyresolves real client IP and resists spoofedX-Forwarded-ForprefixesReview
Independently reviewed via this repo's two-agent workflow (see
docs/agent-workflow.md). Initial review found two Major issues (shared limiter buckets across unrelated routes; misleading 429 handling on login) — both fixed and re-verified. Final verdict: APPROVED.🤖 Generated with Claude Code