Add comprehensive email validation for blocked users #3
Conversation
… many times each email address is blocked, and last time it was blocked. Move email validation out of User model and into EmailValidator. Signup form remembers which email addresses have failed and shows validation error on email field.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive email validation system for blocked users by adding a new BlockedEmail model and associated validation infrastructure. The changes introduce database-backed email blocking capabilities that extend beyond the existing domain-based whitelist/blacklist system.
The core functionality centers around a new blocked_emails table that stores specific email addresses with action types (block/do_nothing), match counts, and timestamps for tracking usage attempts. A new EmailValidator class replaces inline validation methods in the User model, centralizing email validation logic and integrating the blocked email checking via BlockedEmail.should_block?.
On the client side, the account creation controller now tracks rejected emails to provide immediate feedback without requiring server round-trips for previously blocked addresses. The system maintains backward compatibility with existing domain-based restrictions while adding granular email-level blocking capabilities.
The implementation includes proper test coverage with new fabricators and comprehensive specs for the BlockedEmail model and EmailValidator. Error messaging is enhanced in both the API responses (providing structured error data) and localization (adding a 'blocked' message variant).
Confidence score: 1/5
- This PR contains several critical security vulnerabilities and logic flaws that make it unsafe to merge
- The EmailValidator has ReDoS vulnerabilities, race conditions in BlockedEmail.should_block?, and flawed whitelist/blacklist logic that prevents both from working simultaneously
- Files requiring immediate attention:
lib/validators/email_validator.rb,app/models/blocked_email.rb,spec/models/blocked_email_spec.rb
10 files reviewed, 13 comments
|
|
||
| let(:record) { Fabricate.build(:user, email: "[email protected]") } | ||
| let(:validator) { described_class.new({attributes: :email}) } | ||
| subject(:validate) { validator.validate_each(record,:email,record.email) } |
There was a problem hiding this comment.
style: Missing space after comma in method call parameters
|
|
||
| describe "new record" do | ||
| it "sets a default action_type" do | ||
| BlockedEmail.create(email: email).action_type.should == BlockedEmail.actions[:block] |
There was a problem hiding this comment.
style: Use modern RSpec syntax: expect(BlockedEmail.create(email: email).action_type).to eq(BlockedEmail.actions[:block])
| BlockedEmail.create(email: email).action_type.should == BlockedEmail.actions[:block] | |
| expect(BlockedEmail.create(email: email).action_type).to eq(BlockedEmail.actions[:block]) |
| it "updates statistics" do | ||
| Timecop.freeze(Time.zone.now) do | ||
| expect { subject }.to change { blocked_email.reload.match_count }.by(1) | ||
| blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now) |
There was a problem hiding this comment.
logic: Custom matcher be_within_one_second_of is not defined. Need to implement this matcher or use built-in alternatives like be_within(1.second).of(Time.zone.now)
| blocked_email.last_match_at.should be_within_one_second_of(Time.zone.now) | |
| expect(blocked_email.last_match_at).to be_within(1.second).of(Time.zone.now) |
| subject { BlockedEmail.should_block?(email) } | ||
|
|
||
| it "returns false if a record with the email doesn't exist" do | ||
| subject.should be_false |
There was a problem hiding this comment.
style: Use modern RSpec syntax: expect(subject).to be_falsey
| subject.should be_false | |
| expect(subject).to be_falsey |
|
|
||
| context "action_type is :block" do | ||
| let!(:blocked_email) { Fabricate(:blocked_email, email: email, action_type: BlockedEmail.actions[:block]) } | ||
| it { should be_true } |
There was a problem hiding this comment.
style: Use modern RSpec syntax: expect(subject).to be_truthy
| it { should be_true } | |
| it { expect(subject).to be_truthy } |
| record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) | ||
| end | ||
| end | ||
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) |
There was a problem hiding this comment.
style: Use && instead of and for boolean operations to avoid precedence issues in Ruby.
| if record.errors[attribute].blank? and BlockedEmail.should_block?(value) | |
| if record.errors[attribute].blank? && BlockedEmail.should_block?(value) |
| 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 |
There was a problem hiding this comment.
logic: This method has side effects during a read operation - it updates statistics even when just checking if an email should be blocked. This could cause race conditions in concurrent environments and makes the method name misleading.
| end | ||
|
|
||
| def self.should_block?(email) | ||
| record = BlockedEmail.where(email: email).first |
There was a problem hiding this comment.
style: Using .first instead of .find_by(email: email) is less idiomatic and potentially less performant.
| record = BlockedEmail.where(email: email).first | |
| record = BlockedEmail.find_by(email: email) |
| record.match_count += 1 | ||
| record.last_match_at = Time.zone.now | ||
| record.save |
There was a problem hiding this comment.
logic: The increment and save operations should use increment! or be wrapped in a transaction to prevent race conditions when multiple requests check the same email simultaneously.
| if (this.get('rejectedEmails').contains(email)) { | ||
| return Discourse.InputValidation.create({ | ||
| failed: true, | ||
| reason: I18n.t('user.email.invalid') | ||
| }); | ||
| } |
There was a problem hiding this comment.
style: Using generic 'invalid' message instead of specific 'blocked' message may confuse users who don't understand why their email was rejected
2 participants
Test 3