Add comprehensive email validation for blocked users

[zaibkhan]

No description provided.

[codoki-pr-intelligence]

Codoki PR Review

Summary: Make blocked email lookup case-insensitive, prevent bypass
What’s good: Good extraction of email domain rules into a reusable validator, addition of BlockedEmail model with match stats, and client-side feedback loop via rejectedEmails to avoid re-submitting bad addresses.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Security — Blocked email check is case-sensitive, allowing bypass	…/models/blocked_email.rb Blocked-email matching is case-sensitive, so a blocked address can bypass the check by changing letter case (e.g., 'Block@spam.org' vs 'block@spam.org'). Query case-insensitively to enforce blocks regardless of case. Also note: the subsequent match_count increment is a non-atomic read-modify-write and can lose updates under concurrency; consider a single atomic update for correctness.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: Harden should_block? against case-based bypass and concurrent updates; use explicit Regexp options and defensive matching in EmailValidator; ensure the custom validator is reliably autoloaded in all environments.
• Testing: Add a spec for case-insensitive matching in BlockedEmail.should_block? (e.g., record stored as 'Block@spam.org' still blocks 'block@spam.org'), and a spec ensuring stats increment when action_type is :do_nothing (as already implied by shared examples) under multiple validation invocations.
• Documentation: Consider documenting intended matching semantics for blocked emails (case sensitivity, local vs domain handling), and the expected format for SiteSetting email domain lists (literal domains vs regex).
• Compatibility: Custom validators are typically placed in app/validators to guarantee autoloading; verify that lib/validators is included in autoload_paths in all environments to avoid NameError for EmailValidator.
• Performance: Incrementing match_count via read-modify-write is prone to lost updates under concurrency; prefer a single atomic update (e.g., update_all with match_count = match_count + 1) to avoid contention and extra validations.
• Security: Case-sensitive matching in should_block? allows trivial bypass by altering email case; normalize/query case-insensitively to enforce blocks consistently.
• Open questions: Are user and blocked email addresses normalized to lowercase anywhere globally? Is lib/validators on autoload_paths in all environments (esp. production)?

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant UserModel
    participant EmailValidator
    participant BlockedEmail
    UserModel->>EmailValidator: validate_each(record, :email, value)
    alt whitelist present
        EmailValidator-->>UserModel: add :not_allowed error if not matched
    else blacklist present
        EmailValidator-->>UserModel: add :not_allowed error if matched
    end
    alt no prior errors
        EmailValidator->>BlockedEmail: should_block?(value)
        alt blocked
            EmailValidator-->>UserModel: add :blocked error
        else not blocked
            EmailValidator-->>UserModel: no error
        end
    end

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: Blocked-email matching is case-sensitive, so a blocked address can bypass the check by changing letter case (e.g., 'Block@spam.org' vs 'block@spam.org'). Query case-insensitively to enforce blocks regardless of case. Also note: the subsequent match_count increment is a non-atomic read-modify-write and can lose updates under concurrency; consider a single atomic update for correctness.

Suggested change

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

[codoki-pr-intelligence]

🔷 Medium: Use explicit Regexp options for clarity and future compatibility; passing true is non-idiomatic and may be brittle across Ruby versions. Also consider guarding against nil by matching on value.to_s to avoid surprises.

Suggested change

	regexp = Regexp.new("@(#{domains})", true)
	regexp = Regexp.new("@(#{domains})", Regexp::IGNORECASE)