Improve email whitelisting and extend to GitHub auth #2090
Conversation
|
WalkthroughThe changes introduce a new utility, Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webapp/app/utils/boolEnv.ts (1)
1-9: The BoolEnv utility looks well-designedThis utility provides a consistent way to handle boolean environment variables, ensuring only "true" and "1" strings (case-insensitive) are interpreted as boolean true values. This addresses the issue mentioned in the PR description where any non-empty string would coerce to true.
I'd suggest adding a JSDoc comment to document the expected behavior for future developers.
import { z } from "zod"; +/** + * Zod schema that converts string values to booleans in a consistent way: + * - Strings "true" and "1" (case-insensitive) are converted to boolean `true` + * - All other strings are converted to boolean `false` + * - Non-string values are passed through unchanged + */ export const BoolEnv = z.preprocess((val) => { if (typeof val !== "string") { return val; } return ["true", "1"].includes(val.toLowerCase().trim()); }, z.boolean());apps/webapp/app/utils/email.ts (1)
3-13: Good centralization of email validation logicThe
assertEmailAllowedfunction effectively centralizes the email whitelist validation, making it reusable across the application. This promotes consistency in how email restrictions are enforced.I have two suggestions for improvement:
- Consider adding error handling for invalid regex patterns in the environment variable
- Make the error message more user-friendly with guidance
export function assertEmailAllowed(email: string) { if (!env.WHITELISTED_EMAILS) { return; } - const regexp = new RegExp(env.WHITELISTED_EMAILS); + // Safely create regex with error handling + let regexp; + try { + regexp = new RegExp(env.WHITELISTED_EMAILS); + } catch (error) { + console.error("Invalid WHITELISTED_EMAILS regex pattern:", error); + throw new Error("Email validation configuration error"); + } if (!regexp.test(email)) { - throw new Error("This email is unauthorized"); + throw new Error("This email is not authorized. Please contact your administrator if you need access."); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
apps/webapp/app/env.server.ts(5 hunks)apps/webapp/app/models/user.server.ts(3 hunks)apps/webapp/app/routes/login._index/route.tsx(4 hunks)apps/webapp/app/services/email.server.ts(2 hunks)apps/webapp/app/utils/boolEnv.ts(1 hunks)apps/webapp/app/utils/email.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/webapp/app/services/email.server.ts (1)
apps/webapp/app/utils/email.ts (1)
assertEmailAllowed(3-13)
apps/webapp/app/env.server.ts (1)
apps/webapp/app/utils/boolEnv.ts (1)
BoolEnv(3-9)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
apps/webapp/app/env.server.ts (5)
5-5: Good addition of BoolEnv importCorrectly imports the new BoolEnv utility from the local path.
54-54: Fixed SMTP_SECURE boolean coercionReplaces
z.coerce.boolean()with the newBoolEnvschema to ensure proper boolean coercion that only treats "true" and "1" as true values, addressing the issue mentioned in the PR.
342-342: Fixed ALERT_SMTP_SECURE boolean coercionReplaces
z.coerce.boolean()with the newBoolEnvschema for proper boolean coercion.
382-382: Fixed MARQS_DISABLE_REBALANCING boolean coercion with default valueReplaces
z.coerce.boolean()with the newBoolEnvschema and sets a default value offalse, ensuring consistent behavior.
460-460: Fixed RUN_ENGINE_DEBUG_WORKER_NOTIFICATIONS boolean coercion with default valueReplaces
z.coerce.boolean()with the newBoolEnvschema and sets a default value offalse, ensuring consistent behavior.apps/webapp/app/services/email.server.ts (2)
10-10: LGTM: Import statement added correctlyThe import of
assertEmailAllowedfrom the utility file is correctly implemented.
68-70: Properly validates email before sending magic linkGood implementation of email validation before proceeding with sending the magic link email. This ensures that no unauthorized emails receive login links, improving security.
apps/webapp/app/models/user.server.ts (3)
10-10: LGTM: Import statement added correctlyThe import of
assertEmailAllowedfrom the utility file is correctly implemented.
41-44: Improved function signature and validationThe function signature has been updated to use destructuring, which makes the code cleaner and more consistent. The addition of the email validation ensures that users cannot be created with unauthorized email addresses.
80-81: Extended email validation to GitHub authExcellent addition of email validation for GitHub authentication, ensuring consistent enforcement of the whitelist across all authentication methods.
apps/webapp/app/routes/login._index/route.tsx (6)
9-9: LGTM: Import statements added correctlyThe imports for
FormErrorandgetUserSessionare correctly implemented.Also applies to: 16-16
53-57: Properly handles null auth error with redirectToGood implementation of explicitly setting
authErrorto null when there's a redirect. This ensures consistent data structure in the response.
65-75: Good error handling implementationThe implementation properly extracts error messages from session data, handling both error objects with message properties and other types of errors. This ensures all errors can be properly displayed to the user.
80-80: LGTM: Adding authError to the responseCorrectly includes the processed auth error in the TypedJSON response.
103-103: Good UI enhancement for horizontal centeringAdding the
items-centerclass to center the content horizontally is a good UI improvement for better alignment of the login elements.
125-125: Improves user experience with error feedbackGreat addition of error display in the UI. This provides important feedback to users when authentication fails, particularly when their email is not in the whitelist.
No magic link emails will be sent out to non-whitelisted emails. GitHub auth is now also covered and errors will be displayed.
Also includes a fix for boolean env vars. The following env vars always evaluated to true if any value was set:
Thanks @Kritik-J for the prior work on this! And sorry it took us so long to get this in 🫣
Fixes #833