feat(webapp): rate limit magic-link login attempts #2568
Conversation
Adds a simple rate limiter to the login with magic link flow. Similar implementation to the MFA rate limits.
|
|
Warning Rate limit exceeded@myftija has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a Redis-backed magic-link rate limiter and integrates it into the magic login route. New service exports MagicLinkRateLimitError and three rate-limit checks (per-email, per-email daily, per-IP) backed by Redis. The login.magic route now accepts a discriminated-union action payload, validates input, performs parallel IP/email/daily rate-limit checks (aggregating failures), maps rate-limit failures to session auth:error with logging and redirects, and proceeds with the existing magic-link email flow on success. The "reset" path still clears the magic-link trigger and redirects to the magic login page. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: apps/webapp/app/routes/login.magic/route.tsx
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/routes/login.magic/route.tsx(3 hunks)apps/webapp/app/services/magicLinkRateLimiter.server.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/services/magicLinkRateLimiter.server.tsapps/webapp/app/routes/login.magic/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/services/magicLinkRateLimiter.server.tsapps/webapp/app/routes/login.magic/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/services/magicLinkRateLimiter.server.tsapps/webapp/app/routes/login.magic/route.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/services/magicLinkRateLimiter.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/services/magicLinkRateLimiter.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/services/magicLinkRateLimiter.server.ts (1)
apps/webapp/app/services/rateLimiter.server.ts (2)
createRedisRateLimitClient(74-105)RateLimiter(21-72)
apps/webapp/app/routes/login.magic/route.tsx (1)
apps/webapp/app/services/magicLinkRateLimiter.server.ts (4)
checkMagicLinkIpRateLimit(89-96)checkMagicLinkEmailRateLimit(71-78)checkMagicLinkEmailDailyRateLimit(80-87)MagicLinkRateLimitError(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/webapp/app/routes/login.magic/route.tsx (3)
85-95: Tighten server-side email validation to avoid noisy limiter keys.Validate shape and cap length before rate‑limiting; normalize casing after validation.
- z.object({ - action: z.literal("send"), - email: z.string().trim().toLowerCase(), - }), + z.object({ + action: z.literal("send"), + // Trim → validate → normalize. 320 is the RFC cap for email length. + email: z.string().trim().email().max(320).toLowerCase(), + }),
110-140: Surface backoff hints: include Retry-After with rate-limit redirects.Adding a
Retry-Afterheader helps clients/backends throttle responsibly and is standard with 429s; it’s still useful on redirects.- return redirect("/login/magic", { - headers: { - "Set-Cookie": await commitSession(session), - }, - }); + const headers = new Headers({ + "Set-Cookie": await commitSession(session), + }); + if (error instanceof MagicLinkRateLimitError) { + headers.set( + "Retry-After", + String(Math.max(1, Math.ceil(error.retryAfter / 1000))) + ); + } + return redirect("/login/magic", { headers });
147-159: Avoid treating unknown actions as “reset”.Defaulting to reset on invalid input can unexpectedly clear session state. Prefer an explicit 400 for unknown actions.
- case "reset": - default: { - data.action satisfies "reset"; + case "reset": { const session = await getUserSession(request); session.unset("triggerdotdev:magiclink"); return redirect("/login/magic", { headers: { "Set-Cookie": await commitSession(session), }, }); } + default: { + return typedjson({ error: "Invalid action" }, { status: 400 }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/routes/login.magic/route.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/login.magic/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/login.magic/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/login.magic/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/routes/login.magic/route.tsx (1)
apps/webapp/app/services/magicLinkRateLimiter.server.ts (4)
checkMagicLinkIpRateLimit(89-96)checkMagicLinkEmailRateLimit(71-78)checkMagicLinkEmailDailyRateLimit(80-87)MagicLinkRateLimitError(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
🔇 Additional comments (3)
apps/webapp/app/routes/login.magic/route.tsx (3)
3-8: Import consolidation looks good.No issues; Remix node imports are correctly typed.
25-32: Good integration of rate limiter + logging utilities.
- Using subpath import
@trigger.dev/core/v3aligns with the webapp guideline on subpath imports.
125-128: Rate-limit vs. generic error copy is now correct.This addresses the earlier review that flagged the swapped messages. Nice fix.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/routes/login.magic/route.tsx (3)
86-95: Validate email format at the schema.Add
.email()to reject malformed inputs before hitting the auth/rate-limiter.- email: z.string().trim().toLowerCase(), + email: z.string().trim().toLowerCase().email(),
118-131: Avoid logging full email addresses; mask or hash to reduce PII in logs.Keep logs useful without storing raw PII.
- logger.warn("Login magic link rate limit exceeded", { - clientIp, - email, - error, - }); + const maskedEmail = email.replace(/^(.).+(@.*)$/, "$1***$2"); + logger.warn("Login magic link rate limit exceeded", { + clientIp, + email: maskedEmail, + error, + }); @@ - logger.error("Failed sending login magic link", { - clientIp, - email, - error, - }); + logger.error("Failed sending login magic link", { + clientIp, + email: maskedEmail, + error, + });
155-159: Drop the standalonesatisfiesassertion; it’s unnecessary.Control-flow already narrows to
"reset"here; this assertion adds noise.- data.action satisfies "reset";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/login.magic/route.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/env.server.tsapps/webapp/app/routes/login.magic/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/env.server.tsapps/webapp/app/routes/login.magic/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/env.server.tsapps/webapp/app/routes/login.magic/route.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/env.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/env.server.ts
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Applies to {apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts} : Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-09-29T14:56:52.730Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2568
File: apps/webapp/app/routes/login.magic/route.tsx:108-115
Timestamp: 2025-09-29T14:56:52.730Z
Learning: In the trigger.dev infrastructure, x-forwarded-for header is set by the ALB (Application Load Balancer) and can be trusted as the source of client IP addresses for rate limiting purposes.
Applied to files:
apps/webapp/app/routes/login.magic/route.tsx
🧬 Code graph analysis (2)
apps/webapp/app/env.server.ts (2)
apps/supervisor/src/envUtil.ts (1)
BoolEnv(15-17)apps/webapp/app/utils/boolEnv.ts (1)
BoolEnv(12-14)
apps/webapp/app/routes/login.magic/route.tsx (2)
apps/webapp/app/env.server.ts (1)
env(1185-1185)apps/webapp/app/services/magicLinkRateLimiter.server.ts (4)
checkMagicLinkIpRateLimit(89-96)checkMagicLinkEmailRateLimit(71-78)checkMagicLinkEmailDailyRateLimit(80-87)MagicLinkRateLimitError(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/routes/login.magic/route.tsx (2)
108-109: Normalize X-Forwarded-For to the first hop.Safe with your ALB, but parsing the first hop avoids surprises if multiple IPs appear.
- const clientIp = request.headers.get("x-forwarded-for"); + const clientIp = request.headers.get("x-forwarded-for")?.split(",")[0]?.trim();
133-137: Message mapping is correct now.Rate-limit errors show the rate-limit copy; other errors show the generic failure.
apps/webapp/app/env.server.ts (1)
62-62: ApprovedBoolEnv.default(true)enforces a boolean default and yields a boolean schema; the toggle behaves as intended.
* feat(webapp): rate limit magic-link login attempts Adds a simple rate limiter to the login with magic link flow. Similar implementation to the MFA rate limits. * Fix error message * Add an env var feature flags for login rate limiting * Use BoolEnv instead of `0`/`1` * Parse xff properly
* feat(webapp): rate limit magic-link login attempts Adds a simple rate limiter to the login with magic link flow. Similar implementation to the MFA rate limits. * Fix error message * Add an env var feature flags for login rate limiting * Use BoolEnv instead of `0`/`1` * Parse xff properly
Adds a simple rate limiter to the login with magic link flow. Similar implementation to the MFA rate limits.