fix: enforce default blocked email domains list on a stock install - #41767
Open
Osamaali313 wants to merge 2 commits into
Open
fix: enforce default blocked email domains list on a stock install#41767Osamaali313 wants to merge 2 commits into
Osamaali313 wants to merge 2 commits into
Conversation
`validateEmailDomain` gated the whole blocklist check behind `emailDomainBlackList.length`, where `emailDomainBlackList` comes from the admin setting `Accounts_BlockedDomainsList` (default empty). So on a default install the built-in disposable-domain list was never consulted, even though `Accounts_UseDefaultBlockedDomainsList` defaults to `true` — its dedicated toggle was a no-op unless an unrelated custom domain was also configured. Extract the decision into a pure `isEmailDomainBlocked` helper that evaluates the custom list and the default list independently, and use it in `validateEmailDomain`. A default-listed domain is now blocked at registration whenever the default-list toggle is on. Added a unit test.
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 |
🦋 Changeset detectedLatest commit: 867915c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
**/*.spec.ts📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
apps/meteor/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (16)📓 Common learnings📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2026-05-25T18:33:22.615ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-12-10T21:00:54.909ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2026-03-16T11:57:17.987ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2026-02-24T19:22:48.358ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-03-06T18:10:15.268ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
📚 Learning: 2026-08-05T22:02:59.828ZApplied to files:
🔇 Additional comments (1)
WalkthroughThe PR adds ChangesEmail domain blocking
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
A valid email address can carry an upper-case domain (e.g. `user@0-MAIL.COM`), which passed the case-sensitive lookup against the lower-cased default list and slipped through. Lowercase the domain before comparing (the default list is stored lowercase) and match the custom list case-insensitively too, since domains are case-insensitive.
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
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.
Proposed changes
The built-in list of blocked (disposable/throwaway) email domains is silently ignored during registration on a default install.
In
apps/meteor/server/lib/validateEmailDomain.js, the blocklist check was gated on the custom list being non-empty:emailDomainBlackListcomes fromAccounts_BlockedDomainsList, which defaults to empty.Accounts_UseDefaultBlockedDomainsListdefaults totrue. Because the whole condition is behindemailDomainBlackList.length, the&&short-circuits andemailDomainDefaultBlackList(the built-in ~943-entry list) is never consulted unless the admin also adds an unrelated custom domain. The dedicated default-list toggle is effectively a no-op on a stock install — a user can register with a disposable domain the admin believes is blocked.The two settings are meant to be independent:
Accounts_UseDefaultBlockedDomainsListshould enable the built-in list on its own.Fix
Extract the decision into a small pure helper,
isEmailDomainBlocked, that evaluates the custom list and the default list independently, and call it fromvalidateEmailDomain:A custom-listed domain is still blocked exactly as before; a default-listed domain is now blocked whenever the default-list toggle is on.
Types of changes
Checklist
apps/meteor/server/lib/isEmailDomainBlocked.spec.ts, registered injest.config.ts)@rocket.chat/meteorpatch)Further comments
The new helper is pure, so it's covered directly by the unit test. I verified the four cases against the real 943-entry default list: a disposable domain (
0-mail.com) is now blocked with stock defaults (empty custom list, toggle on) where it previously was not; a normal domain is unaffected; the toggle-off path stays unblocked; and custom-list blocking is unchanged.Summary by CodeRabbit
Bug Fixes
Tests