Skip to content

fix: enforce default blocked domains list independently and migrate validateEmailDomain to TypeScript#40030

Open
manasvi30 wants to merge 3 commits intoRocketChat:developfrom
manasvi30:fix/default-blocked-domains-not-enforced
Open

fix: enforce default blocked domains list independently and migrate validateEmailDomain to TypeScript#40030
manasvi30 wants to merge 3 commits intoRocketChat:developfrom
manasvi30:fix/default-blocked-domains-not-enforced

Conversation

@manasvi30
Copy link
Copy Markdown

@manasvi30 manasvi30 commented Apr 1, 2026

Proposed changes

Fixes default blocked domains list not being enforced when custom blocked

domains list is empty. Also migrates validateEmailDomain.js to TypeScript.

Closes #40029

Changes made

  • Migrated validateEmailDomain.jsvalidateEmailDomain.ts

  • Added proper TypeScript types:

  • emailDomainBlackList: string[]

  • emailDomainWhiteList: string[]

  • email: string parameter type

  • Promise<void> return type on validateEmailDomain

  • Fixed bug: split the email domain blacklist check into two independent

conditions so the default blocked domains list is enforced regardless

of whether the custom list is empty or not

Root cause:

The default blocklist check was nested inside emailDomainBlackList.length &&

in validateEmailDomain.js. When the custom list was empty, the entire

condition short-circuited and the default list was never checked.

How to test:

  1. Try registering with [hello@yopmail.com](mailto:hello@yopmail.com) or any other email that uses domain from apps/meteor/app/lib/server/lib/defaultBlockedDomainsList.ts

  2. should now be blocked

Type of change

  • Bug fix

  • Refactor (migration from JS to TypeScript)

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where the default blocked email domains list was not being enforced when a custom blocked domains list was provided but left empty, ensuring email domain blocking protection works as expected.

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot bot commented Apr 1, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 1, 2026

🦋 Changeset detected

Latest commit: c8af27e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/server-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3a3470b4-a5d9-4f27-85e6-20660f88a6cd

📥 Commits

Reviewing files that changed from the base of the PR and between 92a44ee and c8af27e.

📒 Files selected for processing (2)
  • .changeset/two-lizards-smash.md
  • apps/meteor/app/lib/server/lib/validateEmailDomain.ts
📜 Recent 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.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
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.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
📚 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:

  • .changeset/two-lizards-smash.md
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.

Applied to files:

  • .changeset/two-lizards-smash.md
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.

Applied to files:

  • apps/meteor/app/lib/server/lib/validateEmailDomain.ts
📚 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.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.

Applied to files:

  • apps/meteor/app/lib/server/lib/validateEmailDomain.ts
📚 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.ts
📚 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.ts
📚 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.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.

Applied to files:

  • apps/meteor/app/lib/server/lib/validateEmailDomain.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/app/lib/server/lib/validateEmailDomain.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/app/lib/server/lib/validateEmailDomain.ts
🔇 Additional comments (5)
apps/meteor/app/lib/server/lib/validateEmailDomain.ts (4)

12-13: LGTM!

Type annotations for the module-level arrays are correctly specified as string[].


15-25: LGTM!

Settings watchers correctly typed with value: string parameter. The parsing logic properly handles empty/null values and trims domains.

Also applies to: 27-37


39-39: LGTM!

Function signature correctly typed with email: string parameter and Promise<void> return type.


54-65: Bug fix correctly separates the blacklist checks.

The two sequential if-blocks ensure the default blocked domains list is evaluated independently of whether the custom blocked domains list is empty. This properly fixes the short-circuit issue described in the linked issue.

Consider using .includes() for consistency.

Lines 55 and 61 use .indexOf() !== -1 while line 49 uses .includes(). Using .includes() throughout would be more idiomatic and consistent.
[approve_code_changes, suggest_optional_refactor]

♻️ Optional: Use `.includes()` for consistency
-	if (emailDomainBlackList.length && emailDomainBlackList.indexOf(emailDomain) !== -1) {
+	if (emailDomainBlackList.length && emailDomainBlackList.includes(emailDomain)) {
 		throw new Meteor.Error('error-email-domain-blacklisted', 'The email domain is blacklisted', {
 			function: 'RocketChat.validateEmailDomain',
 		});
 	}

-	if (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.indexOf(emailDomain) !== -1) {
+	if (settings.get('Accounts_UseDefaultBlockedDomainsList') && emailDomainDefaultBlackList.includes(emailDomain)) {
 		throw new Meteor.Error('error-email-domain-blacklisted', 'The email domain is blacklisted', {
 			function: 'RocketChat.validateEmailDomain',
 		});
 	}
.changeset/two-lizards-smash.md (1)

1-5: LGTM!

Changeset correctly uses patch for a bug fix and accurately describes the issue being resolved.


Walkthrough

A bug fix addresses the issue where the default blocked domains list was not being enforced when a custom blocked domains list was provided but left empty. The fix separates sequential validation checks and adds TypeScript typings to the email domain validation function.

Changes

Cohort / File(s) Summary
Changeset
.changeset/two-lizards-smash.md
Documentation of patch release recording the bug fix for default blocked domains list enforcement.
Email Domain Validation Logic
apps/meteor/app/lib/server/lib/validateEmailDomain.ts
Restructured conditional logic to perform two sequential checks instead of one nested condition (preventing short-circuit when custom list is empty). Added TypeScript type annotations for email parameter and return type (async function (email: string): Promise<void>).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

type: bug, area: authentication

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: fixing the default blocked domains enforcement issue and migrating to TypeScript with proper typing.
Linked Issues check ✅ Passed The pull request directly addresses issue #40029 by fixing the conditional logic to check default blocked domains independently, regardless of custom list state, and adds TypeScript types as mentioned in the objectives.
Out of Scope Changes check ✅ Passed All changes are in scope: the changeset documents the fix, validateEmailDomain.ts implements the conditional logic fix and TypeScript migration as required by issue #40029.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default blocked domains list not enforced when custom blocked domains list is empty

1 participant