Add comprehensive email validation for blocked users

[joelachance]

Benchmark PR recreated from ai-code-review-evaluation/discourse-graphite for Code Review Bench. Upstream: ai-code-review-evaluation#3

[joelachance]

Regex injection via user-controlled SiteSetting values in email_in_restriction_setting?

The email_in_restriction_setting? method builds a Regexp directly from the email_domains_whitelist or email_domains_blacklist site setting value. It only escapes dots (. → \.) but does not escape other regex metacharacters such as |, (, ), +, *, ?, {, }, [, ], ^, $, or \. If an admin enters a domain like example.com|.* or any string containing regex metacharacters, the resulting regexp may match unintended emails or raise a RegexpError. More critically, the pattern @(#{domains}) treats | in the setting value as regex alternation, meaning the setting foo.com|bar.com is split by | at the regex level, not by any explicit parsing — this happens to work for the pipe-delimited format but means the regex anchoring is wrong: @(foo\.com|bar\.com) will match evil@notfoo.com because there is no end-of-string anchor ($) and no start anchor after @ for the second alternative. The evolved version in R2 fixes this with @(.+\.)?(#{domains}) which intentionally supports subdomains, but this diff's version uses @(#{domains}) which matches anywhere after @ without anchoring, allowing partial domain matches.

Prevents both false negatives (blocked emails sneaking through) and false positives (legitimate emails being rejected) by correctly anchoring the domain match.

In lib/validators/email_validator.rb line 19, change the regex construction to anchor at end-of-string and properly split/escape each domain:

def email_in_restriction_setting?(setting, value)
  domains = setting.split('|').map { |d| Regexp.escape(d) }
  regexp = Regexp.new("@(.*\\.)?(#{domains.join('|')})$", true)
  value =~ regexp
end

This ensures each domain is properly escaped and the match is anchored to the end of the email address. Verify with a test like email_in_restriction_setting?('mail.com', 'user@gmail.com') returning false.

[joelachance]

BlockedEmail.should_block? has a write side-effect during validation — causes save failures on new records

BlockedEmail.should_block? is called from EmailValidator#validate_each during model validation. Inside should_block?, it calls record.save to update match_count and last_match_at. If this is invoked during a transaction (e.g., within User creation), the unconditional save could raise on concurrent access or fail silently (returning false from save) without any error handling — and importantly, if the BlockedEmail record's own validation fails for any reason, save returns false, match_count is not persisted, and the method still returns the correct boolean. However, more critically, this performs a write in a read path (validation), which is architecturally problematic and can cause unexpected behavior in test transactions or nested transactions.

Separating the read concern (should we block?) from the write concern (update statistics) prevents unexpected transaction side-effects during validation and makes the code testable without database writes.

Use update_columns instead of save to avoid running callbacks/validations during the statistics update, and wrap it to avoid raising:

def self.should_block?(email)
  record = BlockedEmail.where(email: email).first
  if record
    record.update_columns(match_count: record.match_count + 1, last_match_at: Time.zone.now)
  end
  record && record.action_type == actions[:block]
end

This avoids validation cycles and is consistent with the later record_match! pattern in R3.

[joelachance]

BlockedEmail.should_block? performs exact email match — no case normalization

BlockedEmail.should_block? queries BlockedEmail.where(email: email).first using the raw email value. If the blocked_emails table stores emails in one case (e.g., lowercase from admin input) but the user registers with a different case (e.g., User@Spam.com), the lookup will miss and fail to block the email. The later evolution of this code (R3, R5) introduces downcase_email before save and uses downcased comparison. This version has no such normalization.

Ensures blocked-email checks cannot be trivially bypassed by changing letter casing in the email address.

In app/models/blocked_email.rb, downcase the email before lookup in should_block?:

def self.should_block?(email)
  record = BlockedEmail.where(email: email.downcase).first
  ...
end

Also add a before_save callback to downcase the stored email:

before_save :downcase_email

def downcase_email
  self.email = email.downcase
end

Add a spec that verifies BlockedEmail.should_block?('BLOCK@SPAMFROMHOME.ORG') returns true when the stored record has 'block@spamfromhome.org'.

[joelachance]

Prevent unbounded memory growth in client-side rejectedEmails list

CreateAccountController stores rejected emails in an array that only grows (pushObject on each failed attempt). A bot or a user repeatedly trying different emails can cause the controller instance to retain an unbounded list during the session, and the validation dependency rejectedEmails.@each will cause additional recomputation work.

Reduces a reachable client-side DoS/perf degradation vector during signup attempts and avoids unnecessary recomputations as the list grows.

In app/assets/javascripts/discourse/controllers/create_account_controller.js, cap rejectedEmails size (e.g., keep last N=10), and/or store a Set-like structure keyed by normalized email (lowercased/trimmed) to avoid duplicates. After pushing, if length > N, shiftObject() until capped. Verify by adding a small unit/integration test (or at minimum manual steps) that repeated failed signups do not grow without bound and that re-entering a previously rejected email still blocks immediately.

[joelachance]

Avoid duplicating screened-email Module: use existing ScreenedEmail instead of new BlockedEmail

The patch introduces BlockedEmail + blocked_emails table, but the repo already has ScreenedEmail with similar responsibilities (blocking emails, action_type Enum, statistics updates, default_action, and broader matching). This creates parallel Modules and conflicting data sources for "email is blocked" checks.

Reduces architectural drift and avoids bugs where one codepath blocks based on screened_emails while another blocks based on blocked_emails, leading to inconsistent enforcement and harder operations.

Remove app/models/blocked_email.rb and db/migrate/20130724201552_create_blocked_emails.rb (and its specs/fabricator) and instead wire EmailValidator to use ScreenedEmail.should_block? (or ScreenedEmail.find_match/record_match! patterns) consistently. Concretely: in lib/validators/email_validator.rb replace BlockedEmail.should_block?(value) with ScreenedEmail.should_block?(value) and update locale key usage accordingly. Verify by running the existing screened email specs (spec/models/screened_email_spec.rb) and the email validator specs.