Skip to content
Jump to bottom

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

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

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

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);
}
Comment on lines +274 to +276
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Guard against result.values.email being undefined.

If result.values is present but result.values.email is falsy/undefined, pushObject(undefined) would pollute the rejectedEmails array.

Proposed fix 
-        if (result.errors && result.errors.email && result.values) {
-          createAccountController.get('rejectedEmails').pushObject(result.values.email);
+        if (result.errors && result.errors.email && result.values && result.values.email) {
+          createAccountController.get('rejectedEmails').pushObject(result.values.email);
         }
🤖 Prompt for AI Agents 
In `@app/assets/javascripts/discourse/controllers/create_account_controller.js`
around lines 274 - 276, The code pushes result.values.email into rejectedEmails
without ensuring it exists; modify the block in create_account_controller (the
if handling result.errors && result.errors.email) to first check that
result.values.email is defined/non-falsy before calling
createAccountController.get('rejectedEmails').pushObject(...). Use a guard like
checking result.values && result.values.email (or optional chaining) and only
push when a valid email string is present to avoid inserting undefined into
rejectedEmails.

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

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

Case-sensitive email lookup can be bypassed with mixed-case input.

where(email: email) performs a case-sensitive match on most PostgreSQL configurations. A user could bypass blocking by registering with a differently-cased variant (e.g., Block@SpamFromHome.org vs. the stored block@spamfromhome.org). Downcase the email before lookup.

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

Also ensure emails are downcased on creation (in set_defaults or a separate callback):

before_validation :normalize_email

def normalize_email
  self.email = email.downcase if email.present?
end
🤖 Prompt for AI Agents 
In `@app/models/blocked_email.rb` around lines 11 - 19, The lookup in
BlockedEmail.should_block? uses BlockedEmail.where(email: email) which is
case-sensitive; change the lookup to use a downcased email (e.g., normalize the
incoming email with email.to_s.downcase before querying) and update the record
logic in should_block? to use the normalized value; also add a before_validation
callback (e.g., before_validation :normalize_email with a normalize_email
method) or ensure set_defaults downcases self.email on create/update so stored
emails are always lowercased.


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

end