Fix: Make email domain validation case-insensitive (#40045)#40060
Fix: Make email domain validation case-insensitive (#40045)#40060Rohit544 wants to merge 4 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughTwo files were modified to improve email domain validation and optimize URL processing. Email domain validation now performs case-insensitive matching for both whitelist and blacklist checks. URL processing in message embeds was refactored from sequential async iteration to concurrent processing using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Hi maintainers 👋 This PR fixes case-sensitive email domain validation by normalizing domains to lowercase. Happy to make any changes if needed. Thanks! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/lib/server/lib/validateEmailDomain.js">
<violation number="1" location="apps/meteor/app/lib/server/lib/validateEmailDomain.js:55">
P1: Blacklist validation regressed: custom blocked domains are ignored and default-list toggle is effectively bypassed due to duplicate default-list checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| emailDomainBlackList.length && | ||
| (emailDomainBlackList.indexOf(emailDomain) !== -1 || | ||
| (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.indexOf(emailDomain) !== -1)) | ||
| (emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain) !== -1 || (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain)!== -1)) |
There was a problem hiding this comment.
P1: Blacklist validation regressed: custom blocked domains are ignored and default-list toggle is effectively bypassed due to duplicate default-list checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/lib/server/lib/validateEmailDomain.js, line 55:
<comment>Blacklist validation regressed: custom blocked domains are ignored and default-list toggle is effectively bypassed due to duplicate default-list checks.</comment>
<file context>
@@ -52,8 +52,7 @@ export const validateEmailDomain = async function (email) {
emailDomainBlackList.length &&
- (emailDomainBlackList.indexOf(emailDomain) !== -1 ||
- (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.indexOf(emailDomain) !== -1))
+ (emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain) !== -1 || (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain)!== -1))
) {
throw new Meteor.Error('error-email-domain-blacklisted', 'The email domain is blacklisted', {
</file context>
| (emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain) !== -1 || (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain)!== -1)) | |
| (emailDomainBlackList.includes(emailDomain) || | |
| (settings.get('Accounts_UseDefaultBlockedDomainsList') && | |
| emailDomainDefaultBlackList.map((d) => d.toLowerCase()).includes(emailDomain))) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (1)
53-60:⚠️ Potential issue | 🔴 CriticalCritical logic error: custom blacklist is never checked.
The condition at line 54 checks if
emailDomainBlackList.lengthexists but then completely ignores it—the logic only validates againstemailDomainDefaultBlackList. This means any custom blocked domains configured by admins are never enforced.Additionally:
- The condition contains a redundant
A || (B && A)pattern (equivalent to just checking the default list unconditionally).- The
.map((d) => d.toLowerCase())call is unnecessary overhead; all entries indefaultBlockedDomainsList.tsare already lowercase.- Formatting inconsistency:
indexOf(emailDomain)!== -1should have a space before!==.Compare with the whitelist logic at line 48, which correctly uses
.includes(emailDomain).Proposed fix
if ( - emailDomainBlackList.length && - (emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain) !== -1 || (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.map((d) => d.toLowerCase()).indexOf(emailDomain)!== -1)) + emailDomainBlackList.includes(emailDomain) || + (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.includes(emailDomain)) ) { throw new Meteor.Error('error-email-domain-blacklisted', 'The email domain is blacklisted', { function: 'RocketChat.validateEmailDomain', }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js` around lines 53 - 60, The blacklist branch in RocketChat.validateEmailDomain incorrectly ignores emailDomainBlackList and redundantly rechecks emailDomainDefaultBlackList; update the condition to check both lists properly by ensuring emailDomain is normalized to lowercase (same as the whitelist logic), then test membership with .includes against emailDomainBlackList and, only if settings.get('Accounts_UseDefaultBlockedDomainsList') is true, against emailDomainDefaultBlackList (or always if intended), removing the unnecessary .map calls and fix spacing around !== for consistency; throw the same Meteor.Error when either list includes the domain.
🤖 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/meteor/app/lib/server/lib/validateEmailDomain.js`:
- Line 46: Replace the deprecated use of String.prototype.substr when extracting
the domain in validateEmailDomain by using a non-deprecated method: compute the
start index with email.lastIndexOf('@') + 1 and call either email.slice(start)
or email.substring(start) to produce emailDomain (the current line that sets
const emailDomain = ...). Ensure you call .toLowerCase() on the result as
before.
In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts`:
- Around line 362-370: The code is fanning out parseUrl calls with
Promise.allSettled over urlsToProcess causing unbounded concurrent fetches and
silent failures; change the logic around Promise.allSettled to process
urlsToProcess with a bounded concurrency (or sequentially) and dedupe identical
urls before fetching (use the url string as key), await each parseUrl via a
small worker pool (e.g., concurrency 3-5) or simple for-await loop, merge
results back into urlsToProcess by index, set changed when foundMeta is true,
and ensure any rejected parseUrl results are logged (include the error and the
url) rather than being dropped.
---
Outside diff comments:
In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js`:
- Around line 53-60: The blacklist branch in RocketChat.validateEmailDomain
incorrectly ignores emailDomainBlackList and redundantly rechecks
emailDomainDefaultBlackList; update the condition to check both lists properly
by ensuring emailDomain is normalized to lowercase (same as the whitelist
logic), then test membership with .includes against emailDomainBlackList and,
only if settings.get('Accounts_UseDefaultBlockedDomainsList') is true, against
emailDomainDefaultBlackList (or always if intended), removing the unnecessary
.map calls and fix spacing around !== for consistency; throw the same
Meteor.Error when either list includes the domain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01860ed0-a985-4e4e-ab84-a70efa017875
📒 Files selected for processing (2)
apps/meteor/app/lib/server/lib/validateEmailDomain.jsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/validateEmailDomain.jsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧠 Learnings (4)
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (1)
24-24: LGTM!The lowercase normalization for both blocked and allowed domain lists during settings watch is correct and aligns with the PR objective.
Also applies to: 35-35
| } | ||
|
|
||
| const emailDomain = email.substr(email.lastIndexOf('@') + 1); | ||
| const emailDomain = email.substr(email.lastIndexOf('@') + 1).toLowerCase(); |
There was a problem hiding this comment.
substr is deprecated; prefer substring or slice.
String.prototype.substr is deprecated. Use substring or slice instead.
Proposed fix
- const emailDomain = email.substr(email.lastIndexOf('@') + 1).toLowerCase();
+ const emailDomain = email.slice(email.lastIndexOf('@') + 1).toLowerCase();📝 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.
| const emailDomain = email.substr(email.lastIndexOf('@') + 1).toLowerCase(); | |
| const emailDomain = email.slice(email.lastIndexOf('@') + 1).toLowerCase(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js` at line 46, Replace
the deprecated use of String.prototype.substr when extracting the domain in
validateEmailDomain by using a non-deprecated method: compute the start index
with email.lastIndexOf('@') + 1 and call either email.slice(start) or
email.substring(start) to produce emailDomain (the current line that sets const
emailDomain = ...). Ensure you call .toLowerCase() on the result as before.
| const results = await Promise.allSettled(urlsToProcess.map((item) => parseUrl(item.url))); | ||
|
|
||
| results.forEach((result, index) => { | ||
| if (result.status === 'fulfilled') { | ||
| const { urlPreview, foundMeta } = result.value; | ||
| Object.assign(urlsToProcess[index], foundMeta ? urlPreview : {}); | ||
| changed = changed || foundMeta; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Don't fan out OEmbed fetches with unbounded Promise.allSettled.
This starts one parseUrl() per remaining URL with no total cap. The guard above only limits URLs outside Site_Url, so a message with many same-site links can still trigger a large burst of HTTP fetches from this after-save hook, duplicate URLs will race past the cache together, and rejected parses are now dropped silently. Please keep this sequential or add a small concurrency limit and log failures.
Suggested fix
-const results = await Promise.allSettled(urlsToProcess.map((item) => parseUrl(item.url)));
-
-results.forEach((result, index) => {
- if (result.status === 'fulfilled') {
- const { urlPreview, foundMeta } = result.value;
- Object.assign(urlsToProcess[index], foundMeta ? urlPreview : {});
- changed = changed || foundMeta;
- }
-});
+for (const item of urlsToProcess) {
+ try {
+ const { urlPreview, foundMeta } = await parseUrl(item.url);
+ Object.assign(item, foundMeta ? urlPreview : {});
+ changed = changed || foundMeta;
+ } catch (err) {
+ log.error({ msg: 'Error parsing URL for OEmbed', url: item.url, err });
+ }
+}📝 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.
| const results = await Promise.allSettled(urlsToProcess.map((item) => parseUrl(item.url))); | |
| results.forEach((result, index) => { | |
| if (result.status === 'fulfilled') { | |
| const { urlPreview, foundMeta } = result.value; | |
| Object.assign(urlsToProcess[index], foundMeta ? urlPreview : {}); | |
| changed = changed || foundMeta; | |
| } | |
| }); | |
| for (const item of urlsToProcess) { | |
| try { | |
| const { urlPreview, foundMeta } = await parseUrl(item.url); | |
| Object.assign(item, foundMeta ? urlPreview : {}); | |
| changed = changed || foundMeta; | |
| } catch (err) { | |
| log.error({ msg: 'Error parsing URL for OEmbed', url: item.url, err }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts` around lines
362 - 370, The code is fanning out parseUrl calls with Promise.allSettled over
urlsToProcess causing unbounded concurrent fetches and silent failures; change
the logic around Promise.allSettled to process urlsToProcess with a bounded
concurrency (or sequentially) and dedupe identical urls before fetching (use the
url string as key), await each parseUrl via a small worker pool (e.g.,
concurrency 3-5) or simple for-await loop, merge results back into urlsToProcess
by index, set changed when foundMeta is true, and ensure any rejected parseUrl
results are logged (include the error and the url) rather than being dropped.
1 participant
Summary
Fixes case-sensitive email domain validation.
Problem
Email domains like GMAIL.com were rejected when gmail.com was configured.
Solution
Testing
Summary by CodeRabbit
Bug Fixes
Performance