fix(email): make domain validation case-insensitive#40049
Open
rashidnarikkodan wants to merge 5 commits intoRocketChat:developfrom
Open
fix(email): make domain validation case-insensitive#40049rashidnarikkodan wants to merge 5 commits intoRocketChat:developfrom
rashidnarikkodan wants to merge 5 commits intoRocketChat:developfrom
Conversation
Normalize email domain to lowercase before validation. Normalize allowed and blocked domain lists at ingestion time. Normalize default blocked domains to ensure consistent comparison. Replace array lookups with Set for O(1) performance. Fixes incorrect rejection of valid emails like test@GMAIL.com when gmail.com is allowed. Fixes RocketChat#40045
|
Contributor
|
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 |
|
|
Contributor
WalkthroughReplaces array-based domain lists with normalized Sets and adds domain normalization. Validation extracts and normalizes the email domain, throws Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
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:12">
P0: A malformed declaration (`yarn devlet ...`) introduces a JavaScript syntax error that can prevent the server module from loading.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (1)
16-16: Remove the step-by-step implementation comments.These comments mostly restate the code and add churn in a small function. Prefer self-explanatory names and block structure here.
As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 19-19, 22-22, 52-52, 60-60, 70-70, 77-77, 93-93
🤖 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 16, Remove the step-by-step implementation comments that simply restate code inside the small helpers—strip inline explanatory lines inside the normalization helper and the other small functions (e.g., normalizeDomain, validateEmailDomain and any isAllowed/isBlocked helpers) so the code reads self-explanatory; keep only a concise top-level description if needed, rely on clear function/variable names, and ensure no behavioral changes are introduced while deleting those redundant comment lines.
🤖 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`:
- Around line 29-34: The current creation of emailDomainBlackSet (and similarly
emailDomainWhiteSet) calls filter(Boolean) before mapping to normalize, so
whitespace-only entries survive; change the pipeline to map(normalize) first and
then filter(Boolean) (i.e., split(...).map(normalize).filter(Boolean)) so
trim/normalization runs before filtering out empty strings; update the
corresponding block that constructs emailDomainWhiteSet the same way and keep
using the existing normalize function and Set construction.
- Around line 78-87: The current conditional wrongly requires
emailDomainBlackSet.size > 0 so default-blocked domains are ignored when the
custom list is empty; in the validateEmailDomain logic remove the outer size
check and instead directly check if emailDomainBlackSet.has(emailDomain) OR
(settings.get('Accounts_UseDefaultBlockedDomainsList') &&
defaultBlackSet.has(emailDomain)) so the default blacklist is honored even when
Accounts_BlockedDomainsList is empty; update the condition around
emailDomainBlackSet, defaultBlackSet, and
settings.get('Accounts_UseDefaultBlockedDomainsList') accordingly.
- Line 12: The declaration for the variable emailDomainBlackSet is invalid (it
currently reads "yarn devlet"); replace that token sequence with a proper
JavaScript variable declaration such as using let/const (e.g., declare
emailDomainBlackSet with let or const) so the Set is created correctly
(emailDomainBlackSet = new Set()) and the file can parse; locate the declaration
of emailDomainBlackSet in this module to update it.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js`:
- Line 16: Remove the step-by-step implementation comments that simply restate
code inside the small helpers—strip inline explanatory lines inside the
normalization helper and the other small functions (e.g., normalizeDomain,
validateEmailDomain and any isAllowed/isBlocked helpers) so the code reads
self-explanatory; keep only a concise top-level description if needed, rely on
clear function/variable names, and ensure no behavioral changes are introduced
while deleting those redundant comment lines.
🪄 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: 1fb9e6d3-9200-4dd2-a0f7-21939429a998
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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.js
🪛 Biome (2.4.10)
apps/meteor/app/lib/server/lib/validateEmailDomain.js
[error] 12-12: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 12-12: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
[error] 51-103: Illegal use of an export declaration outside of a module
(parse)
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (2)
78-87:⚠️ Potential issue | 🟠 MajorDefault blocked domains are bypassed when custom blacklist is empty.
Line 79 requires
emailDomainBlackSet.size > 0, which disables default blacklist enforcement whenAccounts_BlockedDomainsListis empty.Proposed fix
- if ( - emailDomainBlackSet.size > 0 && - ( - emailDomainBlackSet.has(emailDomain) || - ( - settings.get('Accounts_UseDefaultBlockedDomainsList') && - defaultBlackSet.has(emailDomain) - ) - ) - ) { + const isBlacklisted = + emailDomainBlackSet.has(emailDomain) || + ( + settings.get('Accounts_UseDefaultBlockedDomainsList') && + defaultBlackSet.has(emailDomain) + ); + + if (isBlacklisted) { 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 78 - 87, The current conditional requiring emailDomainBlackSet.size > 0 prevents defaultBlackSet checks when the custom Accounts_BlockedDomainsList is empty; update the if in validateEmailDomain.js to separately evaluate the two cases: check custom blacklist presence and match via emailDomainBlackSet.has(emailDomain) only when emailDomainBlackSet.size > 0, and independently check default blacklist using settings.get('Accounts_UseDefaultBlockedDomainsList') && defaultBlackSet.has(emailDomain). In practice replace the combined condition with something like "(emailDomainBlackSet.size > 0 && emailDomainBlackSet.has(emailDomain)) || (settings.get('Accounts_UseDefaultBlockedDomainsList') && defaultBlackSet.has(emailDomain))" so defaultBlocked domains are enforced even if the custom set is empty; refer to symbols emailDomainBlackSet, defaultBlackSet, and settings.get('Accounts_UseDefaultBlockedDomainsList').
29-34:⚠️ Potential issue | 🟡 MinorNormalize before filtering domain entries.
Line 32 and Line 46 filter before
normalize, so whitespace-only tokens can survive and become''after normalization.Proposed fix
emailDomainBlackSet = new Set( value .split(',') - .filter(Boolean) .map(normalize) + .filter(Boolean) ); @@ emailDomainWhiteSet = new Set( value .split(',') - .filter(Boolean) .map(normalize) + .filter(Boolean) );Also applies to: 43-48
🤖 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 29 - 34, The current creation of emailDomainBlackSet (and the analogous emailDomainWhiteSet) filters tokens before calling normalize, so whitespace-only tokens can pass filter(Boolean) then become empty after normalization; change the pipeline to map(normalize) first and then filter(Boolean) (i.e., call normalize on each split token and then remove falsy/empty results) when building emailDomainBlackSet and the corresponding emailDomainWhiteSet so trimmed/normalized empty strings are dropped.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (1)
16-23: Remove implementation comments from this JS module.Please drop the step/process comments and keep intent in identifiers/structure instead.
As per coding guidelines: "Avoid code comments in the implementation".
Also applies to: 52-53, 60-61, 70-71, 77-78, 93-94
🤖 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 16 - 23, Remove the implementation/process comments in this module and rely on clear identifiers and structure instead: delete the comment lines around the normalize helper, the defaultBlackSet initialization, and the settings.watch registration, leaving the function normalize, the Set assignment defaultBlackSet = new Set(emailDomainDefaultBlackList.map(normalize)), and the settings.watch('Accounts_BlockedDomainsList', (value) => ...) intact; similarly remove the other inline step comments near the code blocks referenced (around the lines for Accounts_BlockedDomainsList handling and any other comment pairs at 52-53, 60-61, 70-71, 77-78, 93-94) so the module contains only code and self-explanatory identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js`:
- Around line 78-87: The current conditional requiring emailDomainBlackSet.size
> 0 prevents defaultBlackSet checks when the custom Accounts_BlockedDomainsList
is empty; update the if in validateEmailDomain.js to separately evaluate the two
cases: check custom blacklist presence and match via
emailDomainBlackSet.has(emailDomain) only when emailDomainBlackSet.size > 0, and
independently check default blacklist using
settings.get('Accounts_UseDefaultBlockedDomainsList') &&
defaultBlackSet.has(emailDomain). In practice replace the combined condition
with something like "(emailDomainBlackSet.size > 0 &&
emailDomainBlackSet.has(emailDomain)) ||
(settings.get('Accounts_UseDefaultBlockedDomainsList') &&
defaultBlackSet.has(emailDomain))" so defaultBlocked domains are enforced even
if the custom set is empty; refer to symbols emailDomainBlackSet,
defaultBlackSet, and settings.get('Accounts_UseDefaultBlockedDomainsList').
- Around line 29-34: The current creation of emailDomainBlackSet (and the
analogous emailDomainWhiteSet) filters tokens before calling normalize, so
whitespace-only tokens can pass filter(Boolean) then become empty after
normalization; change the pipeline to map(normalize) first and then
filter(Boolean) (i.e., call normalize on each split token and then remove
falsy/empty results) when building emailDomainBlackSet and the corresponding
emailDomainWhiteSet so trimmed/normalized empty strings are dropped.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js`:
- Around line 16-23: Remove the implementation/process comments in this module
and rely on clear identifiers and structure instead: delete the comment lines
around the normalize helper, the defaultBlackSet initialization, and the
settings.watch registration, leaving the function normalize, the Set assignment
defaultBlackSet = new Set(emailDomainDefaultBlackList.map(normalize)), and the
settings.watch('Accounts_BlockedDomainsList', (value) => ...) intact; similarly
remove the other inline step comments near the code blocks referenced (around
the lines for Accounts_BlockedDomainsList handling and any other comment pairs
at 52-53, 60-61, 70-71, 77-78, 93-94) so the module contains only code and
self-explanatory identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e560b2c9-739a-4a1e-853e-f590617edfa0
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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.js
🧠 Learnings (10)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
🪛 Biome (2.4.10)
apps/meteor/app/lib/server/lib/validateEmailDomain.js
[error] 51-103: Illegal use of an export declaration outside of a module
(parse)
…fault blacklist when the custom list is empty.)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (1)
42-74: Consider removing inline step comments.The step comments (
// Step 1,// Step 2, etc.) add clarity but violate the coding guideline to avoid code comments in the implementation. The function is relatively self-documenting with descriptive variable names and clear logic flow.As per coding guidelines: "Avoid code comments in the implementation".
🤖 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 42 - 74, Remove the inline step comments within RocketChat.validateEmailDomain and rely on the existing descriptive code and variable names (validateEmail, lastIndexOf('@'), normalize, emailDomain, emailDomainWhiteSet, emailDomainBlackSet, settings.get('Accounts_UseDefaultBlockedDomainsList'), defaultBlackSet) to convey the flow; delete the "// Step 1:", "// Step 2:", etc. comment lines but keep all logic, error throws, and existing error metadata intact so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/validateEmailDomain.js`:
- Around line 42-74: Remove the inline step comments within
RocketChat.validateEmailDomain and rely on the existing descriptive code and
variable names (validateEmail, lastIndexOf('@'), normalize, emailDomain,
emailDomainWhiteSet, emailDomainBlackSet,
settings.get('Accounts_UseDefaultBlockedDomainsList'), defaultBlackSet) to
convey the flow; delete the "// Step 1:", "// Step 2:", etc. comment lines but
keep all logic, error throws, and existing error metadata intact so behavior is
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d5c012d-0984-4df0-875f-b0086bfa50dc
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📜 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.js
🧠 Learnings (10)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/lib/server/lib/validateEmailDomain.js
🔇 Additional comments (3)
apps/meteor/app/lib/server/lib/validateEmailDomain.js (3)
12-20: LGTM - Clean initialization and normalization pattern.The Set-based storage and
normalizehelper provide a single source of truth for case-insensitive comparison. NormalizingemailDomainDefaultBlackListat startup is efficient since that list is static.
23-39: LGTM - Watchers correctly normalize at ingestion time.The
.map(normalize).filter(Boolean)order ensures whitespace-only entries are filtered out after normalization, addressing the previous review concern. Resetting to empty Sets on falsy values aligns with the registered defaults ('').
67-72: LGTM - Blacklist logic correctly handles all configurations.The previous issue where the default blacklist was ignored when the custom list was empty has been fixed. Now
defaultBlackSetis consulted wheneverAccounts_UseDefaultBlockedDomainsListis enabled, regardless of whetheremailDomainBlackSethas entries.
rashidnarikkodan
commented
Apr 6, 2026
Author
rashidnarikkodan
left a comment
rashidnarikkodan
left a comment
There was a problem hiding this comment.
I have updated the email domain validation logic to be case-insensitive, so domains like Gmail.com and gmail.COM are treated equivalently.
Key improvements:
- Normalized comparisons by converting domains to lowercase before validation.
- Optimized the logic using a Set for faster lookups, improving performance for large blacklists.
- Preserved the default blacklist behavior, ensuring it is not disabled when a custom blacklist is empty.
These changes make the validation more robust, consistent, and efficient.
Please review and let me know if there are any edge cases I might have missed
Author
|
The PR is ready for QA review. Could a maintainer please add the stat: QA assured label? |
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
2 participants
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.
Issue
Fixes #40045
Problem
Email domain validation was case-sensitive, causing valid emails like
test@GMAIL.comto be rejected whengmail.comwas configured in the allowed domains list.Solution
Testing
test@GMAIL.compasses whengmail.comis allowedImpact
Ensures domain validation follows standard case-insensitive behavior.
Summary by CodeRabbit
Refactor
Bug Fixes