Add comprehensive email validation for blocked users

[everettbu]

Test 3

[github-actions]

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.

[maloyan4good]

Code Review

Verdict: REQUEST CHANGES
Confidence: HIGH

Summary

This PR introduces a BlockedEmail model and an EmailValidator to replace the inline email_validator method in User, and adds client-side caching of rejected emails to avoid re-submitting known-bad addresses. The approach is sound, but there are several correctness and security issues that must be addressed before merging.

---

Findings

Priority	Issue	Location
P1	BlockedEmail.should_block? has a write-within-read race condition and silently swallows save errors	app/models/blocked_email.rb:12-19
P1	email_in_restriction_setting? builds a regex from unsanitized user-controlled site settings — ReDoS / injection risk	lib/validators/email_validator.rb:17-20
P1	values in the error response exposes all user attributes (including password digest, salt, etc.) via user.attributes	app/controllers/users_controller.rb:196-197
P1	BlockedEmail.should_block? increments match_count and updates last_match_at even for do_nothing records, conflating "matched" with "blocked"	app/models/blocked_email.rb:12-19
P2	and used instead of && in validator — low precedence can cause subtle bugs	lib/validators/email_validator.rb:13
P2	Missing newline at end of file	lib/validators/email_validator.rb:24
P2	rejectedEmails client cache is never cleared on email field change — stale rejections persist across edits	app/assets/javascripts/discourse/controllers/create_account_controller.js
P3	BlockedEmail.should_block? uses .where(...).first instead of .find_by	app/models/blocked_email.rb:13

---

Details

[P1] Race condition + silent failure in should_block?

File: app/models/blocked_email.rb:12-19

match_count is incremented in Ruby and then saved. Under concurrent requests for the same email, two threads can both read match_count = N, both write N+1, and one increment is lost. Additionally, record.save can fail (e.g. validation error) and the failure is silently ignored.

Suggested fix:

def self.should_block?(email)
  record = BlockedEmail.find_by(email: email)
  if record
    BlockedEmail.where(email: email)
                .update_all('match_count = match_count + 1, last_match_at = NOW()')
  end
  record && record.action_type == actions[:block]
end

---

[P1] Regex injection / ReDoS in email_in_restriction_setting?

File: lib/validators/email_validator.rb:17-20

The setting value is only partially sanitized (dots are escaped) but other regex metacharacters ((, ), *, +, ?, etc.) are passed through verbatim. A malicious or misconfigured site setting can cause catastrophic backtracking or inject arbitrary regex patterns.

Suggested fix:

def email_in_restriction_setting?(setting, value)
  domains = setting.split(/[\s,]+/).map { |d| Regexp.escape(d.strip) }.join('|')
  regexp = Regexp.new("@(#{domains})\z", true)
  value =~ regexp
end

---

[P1] user.attributes leaks sensitive fields

File: app/controllers/users_controller.rb:196-197

user.attributes returns every column in the users table, including password_hash, salt, auth_token, and any other sensitive columns. Only the three fields actually needed should be sliced.

Suggested fix:

values: { name: user.name, username: user.username, email: user.email }

(Replace user.attributes.slice("name", "username", "email") with explicit attribute access to avoid accidentally including future sensitive columns.)

---

[P1] Statistics updated for do_nothing records — semantic mismatch

File: app/models/blocked_email.rb:12-19

match_count and last_match_at are updated regardless of action_type. The field names imply "how many times was this email blocked", but they are incremented even when the action is do_nothing. This makes the audit data misleading. The stat update should be unconditional (tracking matches, not blocks) and the fields should be renamed, or the update should only happen when action_type == :block. Either way, the intent should be made explicit.

---

[P2] and vs && operator precedence

File: lib/validators/email_validator.rb:13

if record.errors[attribute].blank? and BlockedEmail.should_block?(value)

and has very low precedence in Ruby. While it works here because there is no assignment, it is a style hazard and inconsistent with the rest of the codebase. Use && instead.

---

[P2] Client-side rejectedEmails cache never invalidated

File: app/assets/javascripts/discourse/controllers/create_account_controller.js

Once an email is added to rejectedEmails, it stays there for the lifetime of the modal. If the user corrects a typo and re-enters a previously rejected email that is now valid (e.g. the admin removed the block), the UI will still show it as invalid. Consider clearing the cache when the email field changes, or scoping the check more narrowly.

---

Recommendation

Fix the three P1 issues before merging: use update_all for atomic stat updates, sanitize the restriction-setting regex with Regexp.escape, and replace user.attributes.slice with explicit field access in the controller. The do_nothing stat-tracking semantics should also be clarified. The P2 items are minor but easy to fix in the same pass.

[mfeuerstein]

PR Review — approved

Reviewed 10 files. 0 high-severity issues found. Verdict: approved.

app/models/blocked_email.rb (low)

• Reviewed app/models/blocked_email.rb — looks good

db/migrate/20130724201552_create_blocked_emails.rb (low)

• Reviewed db/migrate/20130724201552_create_blocked_emails.rb — looks good

app/controllers/users_controller.rb (low)

• Reviewed app/controllers/users_controller.rb — looks good

spec/components/validators/email_validator_spec.rb (low)

• Reviewed spec/components/validators/email_validator_spec.rb — looks good

config/locales/server.en.yml (low)

• Reviewed config/locales/server.en.yml — looks good

lib/validators/email_validator.rb (low)

• Reviewed lib/validators/email_validator.rb — looks good

app/models/user.rb (low)

• Reviewed app/models/user.rb — looks good

app/assets/javascripts/discourse/controllers/create_account_controller.js (low)

• Reviewed app/assets/javascripts/discourse/controllers/create_account_controller.js — looks good

spec/fabricators/blocked_email_fabricator.rb (low)

• Reviewed spec/fabricators/blocked_email_fabricator.rb — looks good

spec/models/blocked_email_spec.rb (low)

• Reviewed spec/models/blocked_email_spec.rb — looks good

[zach-source]

Race condition on concurrent signups and shared array reference across controller instances.

[ron-x5labs]

Code Review: Add BlockedEmail, extract EmailValidator, remember rejected emails on signup

Problem

This PR introduces admin-controlled email blocking for signups (a new BlockedEmail model tracking match stats), extracts email whitelist/blacklist validation out of the User model into a reusable ActiveModel::EachValidator (EmailValidator), and has the Ember signup form remember server-rejected emails to show inline validation without a round-trip.

Solution Reviewed

User validations are refactored from a custom email_validator method to validates :email, email: true, if: :email_changed?, delegating whitelist/blacklist regex + a new BlockedEmail.should_block? check to EmailValidator. UsersController#create's failure path now also returns errors: (to_hash) and values: (submitted name/username/email). The frontend pushes result.values.email into a rejectedEmails array on any email error, and emailValidation marks those emails invalid inline. A migration creates the blocked_emails table with a unique index on email.

Summary

The architecture is sound and the extraction preserves the original whitelist/blacklist behavior (existing user_spec.rb tests still cover it). However, there are two blocking issues: an unhandled RegexpError from a malformed admin setting that 500s every signup, and an empty-array truthiness bug that makes the frontend permanently reject valid emails on unrelated signup failures. Several non-blocking issues around the validator's DB side effects, case-sensitivity, and stats accuracy should also be addressed.

Files Reviewed

• lib/validators/email_validator.rb — deeply reviewed
• app/models/blocked_email.rb — deeply reviewed
• app/models/user.rb — deeply reviewed
• app/controllers/users_controller.rb — deeply reviewed
• app/assets/javascripts/discourse/controllers/create_account_controller.js — deeply reviewed
• db/migrate/20130724201552_create_blocked_emails.rb — deeply reviewed
• config/locales/server.en.yml — lightly reviewed (locale addition)
• spec/components/validators/email_validator_spec.rb — deeply reviewed
• spec/models/blocked_email_spec.rb — deeply reviewed
• spec/fabricators/blocked_email_fabricator.rb — lightly reviewed (consistent with existing fabricators)

Verification

• Verified ActiveModel::Errors#to_hash exists in Rails 3.2.12 (Gemfile.lock pins activemodel 3.2.12) via upstream source at rails/rails@v3.2.12 — def to_hash; messages.dup; end at errors.rb:223. (A reviewer concern that to_hash was added in Rails 4 was a false positive; dropped.)
• Verified ActiveModel::Errors#[] auto-creates an empty :email => [] entry on read (get(k) || set(k, []) at errors.rb:126), confirming the empty-array truthiness bug.
• Verified lib/validators is in config.autoload_paths (application.rb:36), so EmailValidator resolves at validation time.
• Verified the migration timestamp 20130724201552 does not collide with any existing migration.
• Bundler/Ruby not available in this environment, so the spec suite could not be executed locally.

Issues Found

See inline comments for the 2 blocking and 6 non-blocking issues. One suggestion is noted inline as well.

Verdict

Recommend changes before merge — the RegexpError 500 and the empty-array email-rejection bug both break core signup flows and should be fixed before this ships.

[ron-x5labs]

🔴 Blocking — RegexpError / ReDoS from unescaped admin SiteSetting.

email_in_restriction_setting? does domains = setting.gsub('.', '\\.') then Regexp.new("@(#{domains})", true). Only literal dots are escaped; every other regex metacharacter in SiteSetting.email_domains_whitelist/blacklist is interpolated raw. The setting is pipe-delimited (e.g. mailinator.com|trashmail.net) and the | is intentionally relied on as alternation, so it can't be blanket-escaped — but an unbalanced (, [, { or a stray */+/?/\\ entered by an admin raises RegexpError.

This runs inside validate_each on every signup and every email change whenever a whitelist/blacklist setting is present. RegexpError is a StandardError not caught by the controller's rescue ActiveRecord::StatementInvalid (nor the DiscourseHub/RestClient rescues), so a single malformed setting turns every signup into an unhandled 500 with no logging. The existing specs only exercise well-formed settings, so this slips through tests.

Fix: split the setting on |, Regexp.escape each domain, re-join with | (preserving the alternation), and/or rescue RegexpError to fail closed with a logged warning. As a bonus this also closes a ReDoS vector: a crafted backtracking pattern (e.g. (a+)+) matched against a user-controlled email could hang the request.

This logic was carried over verbatim from the deleted User#email_in_restriction_setting?, but it now lives in newly-added code, so it's in scope to fix here.

[ron-x5labs]

🔴 Blocking — empty [] truthiness permanently rejects valid emails on unrelated failures.

The guard if (result.errors && result.errors.email && result.values) treats result.errors.email as a presence check, but errors is user.errors.to_hash (users_controller.rb:198), and in Rails 3.2 ActiveModel::Errors#[] is defined as get(attribute.to_sym) || set(attribute.to_sym, []) — it auto-creates an empty :email => [] entry on first read.

EmailValidator line 13 reads record.errors[attribute].blank?, which triggers that auto-create. Since email_changed? is true for new records, the validator runs on every signup, so after any failed user.save (too-short password, taken username) with a perfectly valid email, the serialized JSON contains "email": []. In JavaScript [] is truthy, so this branch succeeds and result.values.email is pushed into rejectedEmails.

emailValidation (line 70) then marks that valid email as failed (user.email.invalid) for the rest of the session, forcing the user to abandon a good email to retry. The rejected-email feature misfires on every unrelated validation failure, not just on blocked emails.

Fix: check result.errors.email.length (or result.errors.email[0]) instead of truthiness, and in the validator use record.errors.get(attribute) / record.errors.added?(attribute, ...) instead of record.errors[attribute] to avoid the empty-entry side effect.

[ron-x5labs]

🟡 Non-blocking — validator has a DB-write side effect and double-counts.

should_block? does a read-modify-write (record.match_count += 1; record.last_match_at = Time.zone.now; record.save) and is called from EmailValidator#validate_each, so every user.valid?/user.save now writes to blocked_emails. UsersController#create calls user.valid? explicitly (line 168) and then user.save (line 172), and save re-runs validations, so a single blocked signup attempt increments match_count at least twice, corrupting the very stats the model exists to track. The save during valid? also commits in its own transaction (not rolled back if user.save later fails for an unrelated reason), so a never-completed signup still bumps the counter.

Additionally record.save ignores its return value: a failed stats save is silently swallowed, and a DB error during save (a StatementInvalid in 3.2) propagates out of the validator and is caught by the controller's rescue ActiveRecord::StatementInvalid, rendering the misleading login.something_already_taken — a blocked_emails-table problem mislabeled as an account conflict.

Finally the read-modify-write is non-atomic, so concurrent signups of the same blocked email lose counter increments.

Fix: separate the block decision from the stat mutation. Make should_block? a pure read, and bump stats atomically and only after a successful user save, e.g. BlockedEmail.where(email: email).update_all(['match_count = match_count + 1, last_match_at = ?', Time.zone.now]).

[ron-x5labs]

🟡 Non-blocking — case-sensitive lookup lets users evade a block.

BlockedEmail.where(email: email).first is an exact, case-sensitive comparison, but the signup path stores the email as-submitted (User.new_from_params sets user.email = params[:email] without downcasing). On a case-sensitive collation (Postgres default), a blocked Spam@x.com is bypassed by signing up with spam@x.com (or any case variation), defeating the block. BlockedEmail.email has a uniqueness validator but no normalization.

Fix: normalize the email to lower case on both storage and lookup (consistent with Email.downcase used elsewhere in Discourse) so blocking cannot be evaded by case variation.

[ron-x5labs]

🟡 Non-blocking — rejectedEmails conflates blocked with already-taken/blank.

Even after fixing the empty-array bug, the push fires on any result.errors.email, which includes the uniqueness error ("has already been taken") and presence error, not only the blocked/not_allowed case the feature was built for. An already-registered (otherwise valid) email is permanently added to rejectedEmails and shown the generic user.email.invalid ("Please enter a valid email address") message on every subsequent keystroke, conflating "blocked by admin" with "already taken"/"blank".

Fix: distinguish the blocked error specifically (e.g. check the specific error message/key or use a dedicated error code) before remembering the email, and show the user.email.blocked ("is not allowed.") locale for blocked emails rather than the generic invalid message.

[ron-x5labs]

🟡 Non-blocking — action_type has no DB default; manual loads bypass validation.

action_type is null: false with no DB-level default; the default is set only via before_validation :set_defaults. The blocked_email_spec.rb comment notes the table may be manually/bulk-loaded ("If we manually load the table with some emails…"), which bypasses validations and either fails the NOT NULL constraint or requires every loader to specify action_type explicitly. match_count already has a DB default (default: 0); action_type should too.

Fix: t.integer :action_type, null: false, default: 1 (the :block enum value), so the model default and DB default agree and manual loads work.

[ron-x5labs]

🟡 Non-blocking — validator spec only covers the BlockedEmail branch.

This spec only exercises the BlockedEmail branch (and does so with BlockedEmail.stubs(:should_block?)), so the whitelist/blacklist regex branches now living in EmailValidator#validate_each (case-insensitivity, period-escaping, partial-match prevention, multi-domain pipe syntax) are untested in isolation. They remain covered indirectly via user_spec.rb (lines 488-564) which drives the validator through validates :email, email: true, so the behavior isn't untested — but a newly extracted reusable EachValidator should unit-test all of its own branches. If those integration tests are ever refactored away, the regex logic loses all direct coverage. Add unit tests for the whitelist/blacklist branches here.

[ron-x5labs]

🟡 Non-blocking — user.email.blocked added only to en.

The new user.email.blocked key was added only to server.en.yml; every other server.*.yml locale that carries the sibling user.email.not_allowed key (fr, de, es, it, nl, cs, da, ko, id, pt, ru, sv, zh_CN, zh_TW) lacks it, so non-English users fall back to the English string. This matches Discourse's en-first convention (translations land later via Transifex) and I18n fallback is graceful, so it's acceptable — noting it so the key is queued for translation alongside the feature.

[ron-x5labs]

💡 Suggestion — assert the model-level validations.

The spec is adequate for the model's core behavior (should_block? return values, statistics updates, default action_type, null last_match_at). Gap: validates :email, presence: true, uniqueness: true (blocked_email.rb:5) is not directly asserted — uniqueness is DB-enforced by the unique index, and the presence validation + before_validation :set_defaults defaulting path are only indirectly exercised. Minor; add a presence/uniqueness example if the team wants full model-level validation coverage.

[ron-x5labs]

Code Review: Add comprehensive email validation for blocked users

Problem

This PR introduces admin-controlled email blocking for signups via a new BlockedEmail model + migration, extracts the pre-existing whitelist/blacklist email-domain validation out of User into a reusable EmailValidator, and adds a client-side "remember rejected emails" mechanism in the signup controller so a once-rejected email is flagged invalid on subsequent keystrokes.

Solution Reviewed

User now declares validates :email, email: true, if: :email_changed? (replacing the inline email_validator method); the new EmailValidator (autoloaded from lib/validators) reproduces the whitelist/blacklist regex check and adds a BlockedEmail.should_block? lookup. UsersController#create now also returns errors: user.errors.to_hash and values: user.attributes.slice(...) on failure so the Ember controller can record the rejected email. The implementation is coherent, but the interaction between a validation-time DB write, a Rails 3.2 Errors#[] quirk, and the client-side caching condition produces two real correctness defects in the core signup retry flow.

Summary

Needs changes before merge. The headline blocker is a Rails 3.2 ActiveModel::Errors#[] auto-vivification quirk: the validator's record.errors[attribute] probe inserts a truthy {"email": []} into the error hash, which the JS treats as a real error and permanently caches the user's valid email as rejected after any unrelated signup failure (bad password, taken username). Secondary: a validation-time DB write that double-counts and can be bypassed by case variation, plus a shared Ember prototype array.

Files Reviewed

• lib/validators/email_validator.rb — deeply reviewed
• app/models/blocked_email.rb — deeply reviewed
• app/models/user.rb — deeply reviewed (validation refactor)
• app/controllers/users_controller.rb — deeply reviewed
• app/assets/javascripts/discourse/controllers/create_account_controller.js — deeply reviewed
• db/migrate/20130724201552_create_blocked_emails.rb — deeply reviewed
• spec/components/validators/email_validator_spec.rb — deeply reviewed
• spec/models/blocked_email_spec.rb — deeply reviewed
• spec/fabricators/blocked_email_fabricator.rb — lightly reviewed (consistent with existing fabricators)
• config/locales/server.en.yml — lightly reviewed (locale addition)

Verification

• Confirmed lib/validators is in config.autoload_paths (application.rb:36) so EmailValidator resolves.
• Confirmed Enum.new(:block, :do_nothing) maps :block => 1 (lib/enum.rb default start: 1).
• Confirmed User.new_from_params stores email as-submitted (no downcase) — user.rb:87.
• Verified the Rails 3.2.12 ActiveModel::Errors#[] auto-vivification behavior (get(k) || set(k, [])) and to_hash returning messages.dup against the v3.2.12 source, which underpins the blocking finding.
• Bundler/Ruby not available in this environment, so the RSpec suite could not be executed locally.

Issues Found

See inline comments: 1 blocking, 8 non-blocking.

Verdict

Recommend changes before merge — the empty-array auto-vivification breaks the signup retry flow for any user whose email is valid but fails signup for another reason; the case-sensitivity and validation-time write issues undermine the block feature's correctness and should be addressed too.

[ron-x5labs]

🔴 Blocking — record.errors[attribute] auto-vivifies an empty array that the JS reads as a real error, permanently rejecting valid emails.

In Rails 3.2 ActiveModel::Errors#[] is get(k) || set(k, []) — reading a missing attribute inserts :email => [] into the errors hash. This line probes record.errors[attribute], so after the validator runs the hash contains a spurious :email => []. UsersController#create renders errors: user.errors.to_hash (which is messages.dup), so the client gets {"email": []}. JavaScript treats [] as truthy, so create_account_controller.js:274 (if (result.errors && result.errors.email && result.values)) passes and pushes the user's valid email into rejectedEmails. Because email_changed? is true on create, the validator runs on every signup, so any failed signup (too-short password, taken username) with a valid email permanently marks that email invalid client-side with the generic user.email.invalid message — the user can never retry with the same address.

# get() returns nil without inserting, unlike []
if record.errors.get(attribute).blank? and BlockedEmail.should_block?(value)

[ron-x5labs]

🟡 Non-blocking — case-sensitive lookup lets users evade a block by varying letter case.

BlockedEmail.where(email: email).first is an exact, case-sensitive match, but User.new_from_params stores the email as-submitted (user.email = params[:email], user.rb:87 — no downcase) and validates :email, uniqueness: true defaults to case_sensitive: true. A block on spam@evil.com is bypassed by registering Spam@evil.com, which creates a distinct account. The sibling whitelist/blacklist regex is case-insensitive (Regexp.new(..., true)), so the two mechanisms disagree. Normalize on both lookup and storage:

def self.should_block?(email)
  email = Email.downcase(email.to_s).strip
  record = BlockedEmail.where(email: email).first
  # ...
  record && record.action_type == actions[:block]
end

[ron-x5labs]

🟡 Non-blocking — should_block? writes to the DB inside a validator; valid? + save double-invoke it and the write commits during a read-only check.

should_block? does record.match_count += 1; record.last_match_at = Time.zone.now; record.save and is called from EmailValidator#validate_each. UsersController#create calls user.valid? (line 168) and then user.save (line 172); save re-runs validations, so a single blocked-email signup increments match_count by 2. The save during valid? commits in its own transaction, so a signup that later fails for an unrelated reason still permanently bumps the stats. The read-modify-write is also non-atomic (lost updates under concurrent signups). Separate decision from bookkeeping:

def self.should_block?(email)
  record = BlockedEmail.where(email: email).first
  record && record.action_type == actions[:block]
end
# increment once, atomically, outside validation:
# BlockedEmail.where(email: email).update_all(['match_count = match_count + 1, last_match_at = ?', Time.zone.now])

[ron-x5labs]

🟡 Non-blocking — rejectedEmails: Em.A([]) is a prototype default shared across all controller instances.

Ember does not copy array/object defaults per instance, so every CreateAccountController shares the same array reference. A rejected email pushed in one flow (line 275) is visible to all other instances, and the array is never cleared when the modal reopens. Initialize it per-instance and clear it on modal open / email change:

init: function() {
  this._super();
  this.set('rejectedEmails', Em.A([]));
}

[ron-x5labs]

🟡 Non-blocking — the push fires for any email error (uniqueness/blank/format), not only admin-blocked emails.

Even after fixing the empty-array bug, the server's errors.email aggregates every email failure (user.errors.to_hash), so an already-registered email ("has already been taken") is also permanently cached and shown the generic user.email.invalid message, conflating "blocked by admin" with "already taken". Narrow the contract so only blocked emails are remembered — e.g. have the server emit a dedicated blocked_email: true flag (or a distinct error key) and gate the push on that:

if (result.blocked_email && result.values) {
  createAccountController.get('rejectedEmails').pushObject(result.values.email);
}

[ron-x5labs]

🟡 Non-blocking — action_type is null: false with no DB default; manual/bulk loads bypass validation.

The default (:block = 1) is set only by before_validation :set_defaults, but blocked_email_spec.rb notes the table may be "manually loaded" — raw SQL / bulk inserts skip callbacks and hit a NotNullViolation instead of getting the intended default. match_count already has a DB default; action_type should too, so the model and DB defaults agree:

t.integer :action_type, null: false, default: 1  # 1 == BlockedEmail.actions[:block]

[ron-x5labs]

🟡 Non-blocking — admin setting interpolated into a Regexp with only . escaped; a malformed whitelist/blacklist raises unhandled RegexpError and 500s every signup.

domains = setting.gsub('.', '\\.'); Regexp.new("@(#{domains})", true) leaves (, [, {, +, *, ?, \\ unescaped. The | separator is intentionally used as alternation, so it can't be blanket-escaped, but an unbalanced bracket from an admin raises RegexpError inside validate_each; UsersController#create rescues only ActiveRecord::StatementInvalid, so it surfaces as a 500 on every account creation until the setting is fixed. This logic was moved verbatim from User, but the PR centralizes it in a validator that runs on every email validation. Split-and-escape each token and/or rescue:

domains = setting.split('|').map { |d| Regexp.escape(d.strip) }.join('|')
regexp = Regexp.new("@(#{domains})$", true) rescue nil

[ron-x5labs]

🟡 Non-blocking — spec only exercises the BlockedEmail branch; the whitelist/blacklist regex paths are untested in isolation.

Both tests stub BlockedEmail.should_block?, so the email_in_restriction_setting? branches (case-insensitivity, period-escaping, multi-domain pipe syntax) living in this extracted EmailValidator#validate_each have no direct unit coverage. They are covered transitively via user_spec.rb (driven through validates :email, email: true), but a newly extracted reusable EachValidator should unit-test all of its own branches; if the integration tests are ever refactored away, the regex logic loses all coverage. Add unit tests for the whitelist/blacklist paths here.

[ron-x5labs]

🟡 Non-blocking — user.email.blocked added only to en.

Every other server.*.yml locale that carries the sibling user.email.not_allowed key (fr, de, es, it, nl, cs, da, ko, id, pt, ru, sv, zh_CN, zh_TW) lacks user.email.blocked, so non-English users fall back to the English string. This matches Discourse's en-first convention (translations land later via Transifex) and I18n fallback is graceful, so it's acceptable — queuing the key for translation alongside the feature.

[ron-x5labs]

Code Review: Add comprehensive email validation for blocked users

Problem

This PR introduces admin-controlled email blocking for signups via a new BlockedEmail model + migration, extracts the pre-existing whitelist/blacklist email-domain validation out of User into a reusable EmailValidator, and adds a client-side "remember rejected emails" mechanism in the signup controller so a once-rejected email is flagged invalid on subsequent keystrokes.

Solution Reviewed

User now declares validates :email, email: true, if: :email_changed? (replacing the inline email_validator method); the new EmailValidator (autoloaded from lib/validators) reproduces the whitelist/blacklist regex check and adds a BlockedEmail.should_block? lookup. UsersController#create now also returns errors: user.errors.to_hash and values: user.attributes.slice(...) on failure so the Ember controller can record the rejected email. The implementation is coherent, but the interaction between a validation-time DB write, a Rails 3.2 Errors#[] quirk, and the client-side caching condition produces two real correctness defects in the core signup retry flow.

Summary

Needs changes before merge. The headline blocker is a Rails 3.2 ActiveModel::Errors#[] auto-vivification quirk: the validator's record.errors[attribute] probe inserts a truthy {"email": []} into the error hash, which the JS treats as a real error and permanently caches the user's valid email as rejected after any unrelated signup failure (bad password, taken username). Secondary: a validation-time DB write that double-counts and can be bypassed by case variation, plus a shared Ember prototype array.

Files Reviewed

• lib/validators/email_validator.rb — deeply reviewed
• app/models/blocked_email.rb — deeply reviewed
• app/models/user.rb — deeply reviewed (validation refactor)
• app/controllers/users_controller.rb — deeply reviewed
• app/assets/javascripts/discourse/controllers/create_account_controller.js — deeply reviewed
• db/migrate/20130724201552_create_blocked_emails.rb — deeply reviewed
• spec/components/validators/email_validator_spec.rb — deeply reviewed
• spec/models/blocked_email_spec.rb — deeply reviewed
• spec/fabricators/blocked_email_fabricator.rb — lightly reviewed (consistent with existing fabricators)
• config/locales/server.en.yml — lightly reviewed (locale addition)

Verification

• Confirmed lib/validators is in config.autoload_paths (application.rb:36) so EmailValidator resolves.
• Confirmed Enum.new(:block, :do_nothing) maps :block => 1 (lib/enum.rb default start: 1).
• Confirmed User.new_from_params stores email as-submitted (no downcase) — user.rb:87.
• Verified the Rails 3.2.12 ActiveModel::Errors#[] auto-vivification behavior (get(k) || set(k, [])) and to_hash returning messages.dup against the v3.2.12 source, which underpins the blocking finding.
• Bundler/Ruby not available in this environment, so the RSpec suite could not be executed locally.

Issues Found

See inline comments: 1 blocking, 8 non-blocking.

Verdict

Recommend changes before merge — the empty-array auto-vivification breaks the signup retry flow for any user whose email is valid but fails signup for another reason; the case-sensitivity and validation-time write issues undermine the block feature's correctness and should be addressed too.

[ron-x5labs]

Code Review: Add BlockedEmail model, extract EmailValidator, remember rejected emails on signup

Problem

This PR introduces admin-controlled email blocking for signups (new BlockedEmail model with match statistics), extracts the pre-existing whitelist/blacklist email-domain validation out of User into a reusable EmailValidator (ActiveModel::EachValidator), and has the Ember signup form remember server-rejected emails so they show inline validation without a round-trip.

Solution Reviewed

User validations are refactored from a custom email_validator method to validates :email, email: true, if: :email_changed?, delegating the whitelist/blacklist regex check plus a new BlockedEmail.should_block? check to EmailValidator. UsersController#create's failure branch now returns errors: user.errors.to_hash and values: user.attributes.slice(...) so the client can identify which field failed. The Ember controller maintains a rejectedEmails array; on a failed signup with an email error, it pushes the submitted email into that array and re-validates against it on subsequent keystrokes.

Summary

The architecture is sound, but there is a blocking bug in the validator: record.errors[attribute] on Rails 3.2.12 auto-vivifies an empty array on read, which gets serialized to the client and causes valid emails to be permanently rejected client-side on any unrelated validation failure. Several non-blocking issues around case-sensitivity, write-side-effects in the validator, regex safety, and test coverage should also be addressed.

Files Reviewed

• lib/validators/email_validator.rb — deeply reviewed (high risk: validation logic, regex construction)
• app/models/blocked_email.rb — deeply reviewed (medium risk: new model with DB writes in validation path)
• app/models/user.rb — deeply reviewed (medium risk: validation refactor)
• app/controllers/users_controller.rb — deeply reviewed (medium risk: API contract change)
• app/assets/javascripts/discourse/controllers/create_account_controller.js — deeply reviewed (medium risk: client-side validation logic)
• db/migrate/20130724201552_create_blocked_emails.rb — lightly reviewed (low risk: straightforward migration)
• config/locales/server.en.yml — lightly reviewed (low risk: one locale key)
• spec/components/validators/email_validator_spec.rb — deeply reviewed (test coverage)
• spec/models/blocked_email_spec.rb — deeply reviewed (test coverage)
• spec/fabricators/blocked_email_fabricator.rb — lightly reviewed (low risk: test helper)

Verification

• Rails version confirmed as 3.2.12 via Gemfile.lock — ActiveModel::Errors#[] source fetched from rails/rails v3.2.12 tag and inspected: def [](attribute); get(attribute.to_sym) || set(attribute.to_sym, []); end confirms auto-vivification on read.
• ActiveModel::Errors#to_hash confirmed as messages.dup — auto-vivified empty arrays ARE serialized.
• Email flow confirmed: UsersController#create sets user.email = params[:email] raw (no downcasing), calls user.valid? then user.save (validator runs twice).

Verdict

Recommend changes before merge — the auto-vivification bug will cause valid emails to be permanently rejected on unrelated failures and should be fixed before this ships.

[ron-x5labs]

🔴 Blocking — record.errors[attribute] auto-vivifies an empty array that permanently rejects valid emails.

On Rails 3.2.12, ActiveModel::Errors#[] is implemented as get(attribute.to_sym) || set(attribute.to_sym, []) — it writes an empty array into the internal messages hash on every read. to_hash returns messages.dup, so that empty array is serialized to the client. In the Ember controller (line 274), result.errors.email is truthy in JS even when it's [], so the email gets pushed into rejectedEmails. The chain: a user fails on an unrelated field (e.g. username taken) → the validator vivifies errors[:email] = [] → controller returns {email: [], username: [...]} → JS sees result.errors.email as truthy → pushes the valid email into rejectedEmails → that email now shows as invalid on every subsequent keystroke.

Fix — use the read-only accessor:

if record.errors.get(attribute).blank? && BlockedEmail.should_block?(value)

get returns nil without mutating state; include? is another safe alternative.

[ron-x5labs]

🟡 Non-blocking — case-sensitive lookup lets users evade a block by varying letter case.

BlockedEmail.where(email: email).first is an exact-match query. UsersController#create sets user.email = params[:email] raw with no normalization (confirmed at user.rb line 87), so User@Example.com and user@example.com are treated as different emails. A blocked address can be evaded simply by capitalizing a letter. Consider normalizing on lookup:

record = BlockedEmail.where("lower(email) = ?", email.to_s.downcase).first

or adding a before_save to downcase stored emails and looking up with the same normalization.

[ron-x5labs]

🟡 Non-blocking — DB write inside a validator; double-counted on every signup.

should_block? performs a read-modify-write (match_count += 1, record.save) as a side effect of validation. UsersController#create calls user.valid? (line 185) and then user.save (line 191), so the validator — and thus the stat write — runs twice per signup, inflating match_count by 2. A validator with write side effects also violates the validation/persistence separation and makes the model harder to reason about (calling valid? for a pre-flight check mutates the DB). Consider moving stat updates to an explicit method called from the controller after a confirmed block, or at minimum guarding against double-invocation.

[ron-x5labs]

🟡 Non-blocking — admin setting interpolated into a Regexp with only . escaped; malformed input raises RegexpError.

domains = setting.gsub('.', '\\.') escapes dots but passes all other regex metacharacters ((, [, *, +, |, etc.) raw to Regexp.new. A whitelist/blacklist containing any of these raises RegexpError at signup time, producing a 500 error. Consider splitting on the intended separator and escaping each domain individually:

domains = setting.split('|').map { |d| Regexp.escape(d.strip) }.join('|')
regexp = Regexp.new("@(#{domains})", true)

This also mitigates ReDoS risk from crafted values.

[ron-x5labs]

🟡 Non-blocking — spec only covers the BlockedEmail branch; whitelist/blacklist regex paths are untested.

This spec stubs BlockedEmail.should_block? and only verifies that branch. The whitelist/blacklist regex logic in email_in_restriction_setting? — including the Regexp.new construction and domain matching — has no test coverage in isolation. Adding tests for both the whitelist (email not in list → error, email in list → no error) and blacklist paths would guard against regressions in the regex construction, which is the most error-prone part of this validator.