Add comprehensive email validation for blocked users

[ShashankFC]

User description

Test 3

Summary by CodeRabbit

Release Notes

New Features

• Added email blocking mechanism for account creation that prevents specific email addresses from being used
• Enhanced email validation to track and reject previously failed emails
• Improved account creation error responses with detailed validation information

Database

• New database table added to persist blocked email configurations and tracking data

_{✏️ Tip: You can customize this high-level summary in your review settings.}

---

Replicated from ai-code-review-evaluation/discourse-coderabbit#3

---

CodeAnt-AI Description

Block specific email addresses from account creation and show persistent validation on the signup form

What Changed

• New blocked_emails table and model that records addresses to block and tracks how often and when they matched; matching addresses prevent account creation.
• Server-side email validation centralized into an EmailValidator that enforces whitelist/blacklist rules and rejects addresses listed as blocked; signup failure responses now include error details and the submitted email.
• Signup form remembers previously rejected email addresses and shows an immediate inline error instead of attempting to resubmit them.
• Automated tests added for the blocked email model and the email validator.

Impact

✅ Fewer spam signups
✅ Clearer signup errors for blocked/invalid emails
✅ Shorter signup retries when using an already-rejected email

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[ShashankFC]

@CodeAnt-AI: review

[codeant-ai]

CodeAnt AI is running the review.

[codeant-ai]

Suggestion: The should_block? method updates match_count with a read-modify-write sequence (record.match_count += 1; record.save), which is not atomic; under concurrent requests for the same email, increments can be lost, causing the match_count tracking to be incorrect. Using an atomic increment operation avoids this race condition and ensures every blocked-email check is counted accurately while still updating last_match_at. [race condition]

Severity Level: Major ⚠️

- ⚠️ BlockedEmail.match_count undercounts concurrent blocked signups.
- ⚠️ Monitoring of attack volume via match_count becomes unreliable.
- ⚠️ last_match_at reflects only last writer under contention.

Suggested change

	record.match_count += 1
	record.last_match_at = Time.zone.now
	record.save
	record.increment!(:match_count)
	record.update_column(:last_match_at, Time.zone.now)

Steps of Reproduction ✅

1. Create a `BlockedEmail` row for some address at `app/models/blocked_email.rb:1` with
`email: 'blocked@example.com'`, `match_count: 0`, and `action_type:
BlockedEmail.actions[:block]`.

2. Trigger two concurrent operations that save or create `User` records with the same
email `'blocked@example.com'`, so that the email validator runs on both. The validator is
attached to `User` via `validates :email, email: true, if: :email_changed?` in
`app/models/user.rb:46-47`.

3. For each concurrent validation, `EmailValidator#validate_each` in
`lib/validators/email_validator.rb:3-16` is invoked, which calls
`BlockedEmail.should_block?(value)` at line 13 when no previous email errors exist.

4. Inside `BlockedEmail.should_block?` at `app/models/blocked_email.rb:11-18`, both
requests load the same row (`where(email: email).first`), each computes
`record.match_count += 1` in Ruby, then `record.save`. Because these updates are not
atomic at the database level, both can persist `match_count` as `1` (or `N+1`) instead of
`2` (or `N+2`), demonstrating lost increments under concurrent access.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** app/models/blocked_email.rb
**Line:** 14:16
**Comment:**
	*Race Condition: The `should_block?` method updates `match_count` with a read-modify-write sequence (`record.match_count += 1; record.save`), which is not atomic; under concurrent requests for the same email, increments can be lost, causing the `match_count` tracking to be incorrect. Using an atomic increment operation avoids this race condition and ensures every blocked-email check is counted accurately while still updating `last_match_at`.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

👍 | 👎

[codeant-ai]

Suggestion: The validator assumes value is always a string, but if it is ever nil (for example when the attribute is unset or when the validator is reused without a presence check), email_in_restriction_setting? will call value =~ regexp and raise a NoMethodError, causing validation to crash instead of returning a proper error response. Adding a short-circuit for nil values in validate_each avoids this runtime failure and lets other validations (like presence) handle the case. [null pointer]

Severity Level: Major ⚠️

- ❌ User validations crash at app/models/user.rb:44-47 when email nil.
- ⚠️ Affects EmailValidator at lib/validators/email_validator.rb:3-21 with whitelists.

Suggested change

	if (setting = SiteSetting.email_domains_whitelist).present?
	return if value.nil?

Steps of Reproduction ✅

1. Ensure an email domain restriction is configured so the validator actually calls
`email_in_restriction_setting?`: set either `SiteSetting.email_domains_whitelist` or
`SiteSetting.email_domains_blacklist` to a non-empty value (these settings are read in
`EmailValidator#validate_each` at `lib/validators/email_validator.rb:4-12`).

2. Create a `User` record without setting an email so the `email` attribute remains `nil`,
for example in Rails console: `user = User.new(username: "test-user")` using the `User`
model defined in `app/models/user.rb:12` (note that the database schema enforces NOT NULL
only on persisted rows; unsaved instances can still have `email == nil`).

3. Trigger validations on this user by calling `user.valid?` or `user.save`; this runs the
validations declared in `app/models/user.rb:44-47`, including `validates :email, email:
true, if: :email_changed?`, which invokes `EmailValidator#validate_each(record, :email,
record.email)` with `value == nil`.

4. Inside `EmailValidator#validate_each` (`lib/validators/email_validator.rb:3-16`),
because a whitelist/blacklist setting is present, the code calls
`email_in_restriction_setting?(setting, value)`
(`lib/validators/email_validator.rb:18-21`); there `value =~ regexp` executes with `value
== nil`, raising `NoMethodError: undefined method '=~' for nil:NilClass` and causing the
validation (and any controller action or job relying on it) to raise instead of returning
normal validation errors.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** lib/validators/email_validator.rb
**Line:** 4:4
**Comment:**
	*Null Pointer: The validator assumes `value` is always a string, but if it is ever `nil` (for example when the attribute is unset or when the validator is reused without a presence check), `email_in_restriction_setting?` will call `value =~ regexp` and raise a NoMethodError, causing validation to crash instead of returning a proper error response. Adding a short-circuit for `nil` values in `validate_each` avoids this runtime failure and lets other validations (like presence) handle the case.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

👍 | 👎

[codeant-ai]

Suggestion: The example asserting the result when no record exists expects a strict false, but BlockedEmail.should_block? returns nil in that case, causing this spec to fail even though the method correctly indicates a non-blocking result; the expectation should allow any falsey value. [logic error]

Severity Level: Critical 🚨

- CRIT: RSpec model spec fails for BlockedEmail.should_block?.
- WARN: CI pipeline or pre-commit hooks blocked by failure.
- WARN: Production email validation unaffected; only test expectation misaligned.

Suggested change

	subject.should be_false
	subject.should be_falsey

Steps of Reproduction ✅

1. Open `app/models/blocked_email.rb` and inspect `BlockedEmail.should_block?(email)` at
lines 11–18, where it loads a record with `record = BlockedEmail.where(email:
email).first` and returns `record && record.action_type == actions[:block]`.

2. Note that when no `BlockedEmail` row exists for the given email, `record` is `nil`, so
the expression `record && record.action_type == actions[:block]` evaluates to `nil`, not
`false`.

3. Open `spec/models/blocked_email_spec.rb` and inspect the example at lines 19–24:
`describe "#should_block?" do` with `subject { BlockedEmail.should_block?(email) }` and
the example `"returns false if a record with the email doesn't exist"` expecting
`subject.should be_false`.

4. Run the test file, e.g. `bundle exec rspec spec/models/blocked_email_spec.rb`; the
example for the no-record case will fail because RSpec's `be_false` matcher expects the
value `false`, but the method returns `nil`, causing a mismatch even though both values
are treated as non-blocking in actual usage (e.g. `lib/validators/email_validator.rb:13`
only relies on a truthy/falsey check).

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** spec/models/blocked_email_spec.rb
**Line:** 23:23
**Comment:**
	*Logic Error: The example asserting the result when no record exists expects a strict `false`, but `BlockedEmail.should_block?` returns `nil` in that case, causing this spec to fail even though the method correctly indicates a non-blocking result; the expectation should allow any falsey value.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

👍 | 👎

[codeant-ai]

Nitpicks 🔍

🔒 No security issues identified

⚡ Recommended areas for review Sensitive Responses The create action now returns user.errors and user.attributes.slice("name","username","email") in JSON on validation failure. This increases the level of detail exposed to clients (validation messages and the attempted email) and can be used to enumerate existing/blocked emails or other account state. Consider whether this level of detail is acceptable for public APIs and add mitigations (generic errors, rate-limiting, or limiting returned fields). Email normalization BlockedEmail.should_block? looks up records by the raw email argument and validates :email, uniqueness: true is used without normalization or case-insensitive handling. That can cause missed matches or duplicate records differing only by case/whitespace. Normalize emails (downcase/strip) on write and use normalized lookups. Nil/blank email handling The methods call regex match and BlockedEmail.should_block?(value) without guarding against nil/blank value. If value is nil or an unexpected type, this can raise at runtime (e.g., calling =~ on nil). Add guards so validation safely skips when email is blank. Domain matching fragility The domain-matching logic builds a regex from the raw SiteSetting string using simple gsub on '.' and then constructs Regexp.new("@(#{domains})", true). This won't correctly handle multiple domains (comma/newline separated), will not escape arbitrary input safely, and does not reliably match subdomains or anchors at the end of the email. It should split and escape domains, and anchor/match subdomains according to intended policy. Ember dependent key / array observation The email validation depends on rejectedEmails.@each which is not the correct dependent key for observing additions/removals of plain array items in Ember. Also comparisons between client and server emails are done as raw strings (no normalization), so case/whitespace differences will cause mismatches and the client may not block intended emails.

[codeant-ai]

CodeAnt AI finished running the review.