Skip to content
Jump to bottom

Automated Test: blocked-email-validation-post #326

Closed
wants to merge 1 commit into from
+155 −23
Closed

Automated Test: blocked-email-validation-post #326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion app/assets/javascripts/discourse/controllers/create_account_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
accountPasswordConfirm: 0,
accountChallenge: 0,
formSubmitted: false,
rejectedEmails: Em.A([]),

submitDisabled: function() {
if (this.get('formSubmitted')) return true;
Expand Down Expand Up @@ -64,6 +65,14 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
}

email = this.get("accountEmail");

if (this.get('rejectedEmails').contains(email)) {
return Discourse.InputValidation.create({
failed: true,
reason: I18n.t('user.email.invalid')
});
}

if ((this.get('authOptions.email') === email) && this.get('authOptions.email_valid')) {
return Discourse.InputValidation.create({
ok: true,
Expand All @@ -84,7 +93,7 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
failed: true,
reason: I18n.t('user.email.invalid')
});
}.property('accountEmail'),
}.property('accountEmail', 'rejectedEmails.@each'),

usernameMatch: function() {
if (this.usernameNeedsToBeValidatedWithEmail()) {
Expand Down Expand Up @@ -262,6 +271,9 @@ Discourse.CreateAccountController = Discourse.Controller.extend(Discourse.ModalF
createAccountController.set('complete', true);
} else {
createAccountController.flash(result.message || I18n.t('create_account.failed'), 'error');
if (result.errors && result.errors.email && result.values) {
createAccountController.get('rejectedEmails').pushObject(result.values.email);
}
createAccountController.set('formSubmitted', false);
}
if (result.active) {
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/users_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,9 @@ def create
else
render json: {
success: false,
message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n"))
message: I18n.t("login.errors", errors: user.errors.full_messages.join("\n")),
errors: user.errors.to_hash,
values: user.attributes.slice("name", "username", "email")
}
end
rescue ActiveRecord::StatementInvalid
Expand Down
25 changes: 25 additions & 0 deletions app/models/blocked_email.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
class BlockedEmail < ActiveRecord::Base

before_validation :set_defaults

validates :email, presence: true, uniqueness: true

def self.actions
@actions ||= Enum.new(:block, :do_nothing)
end
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the Enum class definition
rg -n 'class Enum' --type=ruby

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 119

🏁 Script executed:

#!/bin/bash
# Check if blocked_email.rb has any explicit requires
echo "=== Checking blocked_email.rb for requires ==="
cat -n app/models/blocked_email.rb | head -20

echo -e "\n=== Examining the Enum class implementation ==="
cat -n lib/enum.rb

echo -e "\n=== Checking if lib is in Rails autoload paths ==="
rg -n 'config\.autoload' --type=ruby config/

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2477

🏁 Script executed:

#!/bin/bash
# Search for any requires of 'enum' in the codebase
echo "=== Searching for require statements for Enum ==="
rg -n "require.*enum" --type=ruby -i

echo -e "\n=== Checking for initializers that might load lib/enum ==="
ls -la config/initializers/ 2>/dev/null | head -20
rg -n "require.*lib" --type=ruby config/initializers/

echo -e "\n=== Checking if lib directory is in autoload_paths ==="
cat config/application.rb | grep -A 10 -B 5 "autoload_paths"

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2629

Add require_dependency 'enum' at the top of blocked_email.rb.

The Enum class is defined in lib/enum.rb, but blocked_email.rb lacks the required require_dependency 'enum' statement. Other files using Enum in the codebase (e.g., post.rb, notification.rb, post_action_type.rb) include this require statement. Without it, calling Enum.new(:block, :do_nothing) will fail at runtime with a NameError.

🤖 Prompt for AI Agents 
In `@app/models/blocked_email.rb` around lines 7 - 9, Add a require_dependency
'enum' at the top of blocked_email.rb so the Enum constant is loaded before
self.actions calls Enum.new(:block, :do_nothing); specifically, open
blocked_email.rb and insert require_dependency 'enum' near other requires
(matching how post.rb, notification.rb, post_action_type.rb do it) to ensure the
Enum class is available for the actions method.


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
Comment on lines +11 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's explore the repository structure
git ls-files | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 348

🏁 Script executed:

# Check the BlockedEmail model and related models
fd -t f -e rb 'blocked_email|user' app/models/ | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 615

🏁 Script executed:

# Search for BlockedEmail references and email normalization patterns
rg -n 'BlockedEmail|should_block' --type=rb -g '!spec/**' | head -30

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 114

🏁 Script executed:

# Check the User model for email handling/normalization
fd -t f 'user.rb' app/models/ -x cat -n {}

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 39687

🏁 Script executed:

# Look for email normalization in the codebase
rg -n 'downcase|normalize.*email|email.*downcase' --type=rb -g '!spec/**' | head -30

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 114

🏁 Script executed:

cat -n app/models/blocked_email.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 805

🏁 Script executed:

# Search for how BlockedEmail is used in the codebase
rg "BlockedEmail" --type-list | head -5

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 477

🏁 Script executed:

# Try searching for BlockedEmail references without type specification
rg "BlockedEmail\|should_block" --max-count=20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 70

🏁 Script executed:

# Search for Email class and downcase method
rg "class Email|def downcase" --max-count=20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 685

🏁 Script executed:

# Find the Email class definition
find . -name 'email.rb' -type f | grep -v node_modules | head -10

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 102

🏁 Script executed:

# Search for where BlockedEmail.should_block? is called
rg "should_block" --max-count=20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 658

🏁 Script executed:

# Search for email normalization in validators
rg "downcase|strip|normalize" lib/validators/email_validator.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 70

🏁 Script executed:

# Check if there's any email normalization in database callbacks
rg "before_save|before_validation.*email" --max-count=20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 459

🏁 Script executed:

cat -n lib/email.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 945

🏁 Script executed:

cat -n lib/validators/email_validator.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1095

🏁 Script executed:

cat -n spec/models/blocked_email_spec.rb | head -80

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1987

🏁 Script executed:

# Check if email is normalized before being passed to BlockedEmail.should_block?
rg -B5 -A5 "BlockedEmail.should_block" lib/validators/email_validator.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 509

🏁 Script executed:

# Verify that Email.downcase is not called in the EmailValidator path
rg -B10 -A10 "Email.downcase" lib/validators/email_validator.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 70

🏁 Script executed:

# Check User model to confirm email is not normalized before validation
rg -B5 -A5 "validates.*email" app/models/user.rb | head -30

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 418

🏁 Script executed:

# Look at how User creation/validation handles email
rg -B5 -A5 "new_from_params|create_for_email" app/models/user.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 650

should_block? has no email normalization — case-sensitive lookup may miss blocked entries.

If an admin blocks spam@example.com but a user registers with Spam@Example.com, the where(email: email) lookup will miss the record (case-sensitive comparison in SQL). Although Email.downcase() is available in the codebase, it is not used in the validation flow. The email should be normalized before lookup to ensure consistent blocking.

Additionally, the read-modify-save pattern on match_count is susceptible to lost updates under concurrent requests. Using an atomic SQL update would be more robust for statistics tracking.

Proposed fix for normalization and atomic update 
 def self.should_block?(email)
-  record = BlockedEmail.where(email: email).first
+  record = BlockedEmail.where(email: email.strip.downcase).first
   if record
-    record.match_count += 1
-    record.last_match_at = Time.zone.now
-    record.save
+    BlockedEmail.where(id: record.id).update_all(
+      ['match_count = match_count + 1, last_match_at = ?', Time.zone.now]
+    )
   end
   record && record.action_type == actions[:block]
 end

You may also want to normalize the email in set_defaults or via a before_validation callback so stored emails are always lowercase:

before_validation { self.email = email.strip.downcase if email.present? }
🤖 Prompt for AI Agents 
In `@app/models/blocked_email.rb` around lines 11 - 19, should_block? performs a
case-sensitive lookup and non-atomic read-modify-save on match_count; normalize
the incoming email (strip and downcase) before querying so
BlockedEmail.where(email: normalized_email).first finds stored entries, and
replace the read-modify-save with an atomic DB update that increments
match_count and sets last_match_at in one statement (e.g. an update_all or
increment counter with a where clause on the record id) to avoid lost updates;
also add/ensure a before_validation (or set_defaults) callback to normalize
stored emails (self.email = email.strip.downcase if present) so saved records
are consistently lowercased.


def set_defaults
self.action_type ||= BlockedEmail.actions[:block]
end

end