Add comprehensive email validation for blocked users

[ShashankFC]

Test 3nn---nReplicated from ai-code-review-evaluation/discourse-greptile#3

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 47s —— View job

---

Code Review Completed

Tasks

• Read repository guidelines (CLAUDE.md)
• Review all changed files
• Analyze code for bugs and security issues
• Check test coverage and quality
• Post comprehensive review feedback

---

Review Summary

This PR introduces a new BlockedEmail model to track and block email addresses during signup, along with moving email validation logic from the User model into a dedicated EmailValidator. The implementation is generally solid, but I've identified several important issues that should be addressed.

Critical Issues

1. Race Condition in BlockedEmail.should_block? ⚠️

Location: app/models/blocked_email.rb:11-18

The method has a race condition vulnerability:

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

Problem: Multiple concurrent requests could read the same match_count, increment it, and save - resulting in incorrect statistics. This is especially problematic during spam attacks.

Solution: Use atomic updates:

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

Or better yet, use increment!:

record.increment!(:match_count)
record.update_column(:last_match_at, Time.zone.now)

2. Improper Use of and Operator ⚠️

Location: lib/validators/email_validator.rb:13

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

Problem: Using and instead of && can cause precedence issues. Ruby's and has very low precedence, which can lead to unexpected behavior.

Solution: Use && for boolean logic:

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

3. Security: ReDoS Vulnerability in Regex 🔴

Location: lib/validators/email_validator.rb:18-21

def email_in_restriction_setting?(setting, value)
  domains = setting.gsub('.', '\.')
  regexp = Regexp.new("@(#{domains})", true)
  value =~ regexp
end

Problem: If setting contains pipe-separated domains (e.g., "evil.com|.*|good.com"), a malicious user could craft input that causes catastrophic backtracking, leading to a Regular Expression Denial of Service (ReDoS) attack.

Solution: Properly escape and validate the domain list:

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

Major Issues

4. Missing Email Validation

Location: app/models/blocked_email.rb:5

The model only validates presence and uniqueness of email, but doesn't validate that it's a valid email format. This could lead to garbage data in the database.

Recommendation: Add email format validation:

validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP }

5. Missing Database Index for Performance

Location: db/migrate/20130724201552_create_blocked_emails.rb:10

While there's a unique index on email, there's no index on action_type, which is used in the lookup query.

Recommendation: Consider adding an index:

add_index :blocked_emails, [:email, :action_type]

This composite index would optimize the common query pattern in should_block?.

6. Inefficient Validation Flow

Location: lib/validators/email_validator.rb:13-15

The validator calls BlockedEmail.should_block? even when there are existing errors on the email field. While there's a blank check, this still triggers a database query unnecessarily.

Better approach: The early return is good, but consider the comment explaining why.

7. JavaScript: Poor Variable Naming

Location: app/assets/javascripts/discourse/controllers/create_account_controller.js:69

if (this.get('rejectedEmails').contains(email)) {

Issue: Using the Ember.js deprecated contains method. This should be includes in modern Ember/JavaScript.

8. Missing Translation Key

Location: config/locales/server.en.yml:734

Only added blocked: "is not allowed." but it's quite generic. Consider a more specific message like:

blocked: "is not allowed. This email address has been blocked from creating accounts."

Minor Issues / Suggestions

9. Test Coverage Could Be Better

Location: spec/components/validators/email_validator_spec.rb

The email validator spec only tests the blocked email functionality but doesn't test:

• Whitelist validation
• Blacklist validation
• The interaction between whitelist/blacklist and blocked emails

Recommendation: Add comprehensive test cases for all validation paths.

10. Missing Edge Case Tests

Location: spec/models/blocked_email_spec.rb

Tests don't cover:

• Email case sensitivity (is Test@Example.com the same as test@example.com?)
• Email with whitespace
• Concurrent updates to match_count

11. No Migration Rollback Test

Consider what happens if this migration needs to be rolled back. The change method is good, but ensure it's reversible.

Code Quality Observations

Positive Points ✅

1. Good separation of concerns - moving validation logic out of User model
2. Comprehensive test coverage for the BlockedEmail model
3. Proper use of Fabricator for test data
4. Good use of Rails conventions (Enum, validations)
5. Client-side UX improvement by remembering rejected emails
6. Proper use of I18n for error messages

Documentation

The code lacks comments explaining:

• Why action_type has a :do_nothing option (what's the use case?)
• The purpose of tracking match_count and last_match_at statistics

Performance Considerations

The BlockedEmail.should_block? method is called on every signup attempt. With the unique index on email, this should be fast, but consider:

1. Adding query result caching for frequently checked emails
2. Monitoring the performance impact on high-traffic signup pages

Recommendations Summary

Must Fix:

1. Fix the race condition in should_block? (use atomic updates)
2. Fix the ReDoS vulnerability in regex construction
3. Replace and with && operator

Should Fix:
4. Add email format validation to BlockedEmail model
5. Improve error messages for better user experience
6. Add comprehensive test coverage for all validator paths

Nice to Have:
7. Add composite index for better query performance
8. Document the action_type enum usage
9. Consider caching strategy for high-traffic scenarios
10. Add case-insensitive email handling

Testing Recommendations

Before merging, ensure:

• Run the full test suite to verify no regressions
• Test concurrent signup attempts with blocked emails
• Verify the frontend properly displays the blocked email error
• Test with various email formats (uppercase, whitespace, etc.)
• Load test the signup flow with blocked emails enabled

---