CVE-2024-38360: Insufficient Information (NVD-CWE-noinfo)
value: Discourse is an open source platform for community discussion. In affected versions by creating replacement words with an almost unlimited number of characters, a moderator can reduce the availability of a Discourse instance. This issue has been addressed in stable version 3.2.3 and in current betas. Users are advised to upgrade. Users unable to upgrade may manually remove the long watched words either via SQL or Rails console.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-07-15T20:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/models/watched_word.rb, app/models/watched_word_group.rb, spec/fabricators/watched_word_group_fabricator.rb, spec/models/watched_word_group_spec.rb, spec/models/watched_word_spec.rb, spec/requests/admin/watched_words_controller_spec.rb, spec/system/admin_watched_words_spec.rb, spec/system/page_objects/pages/admin_watched_words.rb
Commit date: 2024-07-15 19:25:17 +0800
Commit message: SECURITY: limit the number of characters in watched word replacements.

The watch words controller creation function, create_or_update_word(), doesn’t validate the size of the replacement parameter, unlike the word parameter, when creating a replace watched word. So anyone with moderator privileges can create watched words with almost unlimited characters.

Diff: diff --git a/app/models/watched_word.rb b/app/models/watched_word.rb
index aebc1a701d86b..f91a4e0ac3f92 100644
--- a/app/models/watched_word.rb
+++ b/app/models/watched_word.rb
@@ -1,20 +1,24 @@
 # frozen_string_literal: true
 
 class WatchedWord < ActiveRecord::Base
   MAX_WORDS_PER_ACTION = 2000
 
-  before_validation { self.word = WatchedWord.normalize_word(self.word) }
+  before_validation do
+    self.word = WatchedWord.normalize_word(self.word)
+    self.replacement = WatchedWord.normalize_word(self.replacement) if self.replacement.present?
+  end
 
   before_validation do
     if self.action == WatchedWord.actions[:link] && self.replacement !~ %r{\Ahttps?://}
       self.replacement =
         "#{Discourse.base_url}#{self.replacement&.starts_with?("/") ? "" : "/"}#{self.replacement}"
     end
   end
 
   validates :word, presence: true, uniqueness: true, length: { maximum: 100 }
+  validates :replacement, length: { maximum: 100 }
   validates :action, presence: true
   validate :replacement_is_url, if: -> { action == WatchedWord.actions[:link] }
   validate :replacement_is_tag_list, if: -> { action == WatchedWord.actions[:tag] }
   validate :replacement_is_html, if: -> { replacement.present? && html? }
 
diff --git a/app/models/watched_word_group.rb b/app/models/watched_word_group.rb
index b63197ac5b4a3..d65ff39efb39e 100644
--- a/app/models/watched_word_group.rb
+++ b/app/models/watched_word_group.rb
@@ -1,28 +1,28 @@
 # frozen_string_literal: true
 
 class WatchedWordGroup < ActiveRecord::Base
   validates :action, presence: true
+  validate :watched_words_validation
 
   has_many :watched_words, dependent: :destroy
 
+  def watched_words_validation
+    watched_words.each { |word| errors.merge!(word.errors) }
+    errors.add(:watched_words, :empty) if watched_words.empty?
+  end
+
   def create_or_update_members(words, params)
     WatchedWordGroup.transaction do
       self.action = WatchedWord.actions[params[:action_key].to_sym]
-      self.save! if self.changed?
 
       words.each do |word|
-        watched_word =
-          WatchedWord.create_or_update_word(
-            params.merge(word: word, watched_word_group_id: self.id),
-          )
-
-        if !watched_word.valid?
-          self.errors.merge!(watched_word.errors)
-          raise ActiveRecord::Rollback
-        end
+        watched_word = WatchedWord.create_or_update_word(params.merge(word: word))
+        self.watched_words << watched_word
       end
+
+      self.save!
     end
   end
 
   def action_log_details
     "#{WatchedWord.actions.key(self.action)} → #{watched_words.pluck(:word).join(", ")}"
diff --git a/spec/fabricators/watched_word_group_fabricator.rb b/spec/fabricators/watched_word_group_fabricator.rb
index 94a3c2cff8b5d..de4684ebb4ca8 100644
--- a/spec/fabricators/watched_word_group_fabricator.rb
+++ b/spec/fabricators/watched_word_group_fabricator.rb
@@ -1,3 +1,6 @@
 # frozen_string_literal: true
 
-Fabricator(:watched_word_group) { action WatchedWord.actions[:block] }
+Fabricator(:watched_word_group) do
+  action WatchedWord.actions[:block]
+  watched_words { [Fabricate.build(:watched_word)] }
+end
diff --git a/spec/models/watched_word_group_spec.rb b/spec/models/watched_word_group_spec.rb
index 98d684617c9a7..4b106a347fe02 100644
--- a/spec/models/watched_word_group_spec.rb
+++ b/spec/models/watched_word_group_spec.rb
@@ -1,10 +1,10 @@
 # frozen_string_literal: true
 
 RSpec.describe WatchedWordGroup do
   fab!(:watched_word_group)
-  fab!(:watched_word_1) { Fabricate(:watched_word, watched_word_group_id: watched_word_group.id) }
+  let!(:watched_word_1) { watched_word_group.watched_words.first }
   fab!(:watched_word_2) { Fabricate(:watched_word, watched_word_group_id: watched_word_group.id) }
 
   describe "#create_or_update_members" do
     it "updates watched word action" do
       words = [watched_word_1.word, watched_word_2.word, "damn", "4sale"]
@@ -24,11 +24,11 @@ RSpec.describe WatchedWordGroup do
       expect(watched_words.map(&:word)).to contain_exactly(*words)
       expect(watched_words.map(&:action).uniq).to contain_exactly(WatchedWord.actions[:censor])
       expect(watched_word_group.action).to eq(WatchedWord.actions[:censor])
     end
 
-    it "leaves membership intact if update fails" do
+    it "raises an error if validation fails" do
       words = [watched_word_1.word, watched_word_2.word, "a" * 120]
       old_action = watched_word_group.action
       watched_words_before_update = watched_word_group.watched_words
 
       expect(watched_words_before_update.size).to eq(2)
@@ -36,21 +36,14 @@ RSpec.describe WatchedWordGroup do
         watched_word_1.word,
         watched_word_2.word,
       )
       expect(watched_words_before_update.map(&:action).uniq).to contain_exactly(old_action)
 
-      watched_word_group.create_or_update_members(
-        words,
-        action_key: WatchedWord.actions[watched_word_group.action],
-      )
-
-      expect(watched_word_group.reload.errors).not_to be_empty
-
-      watched_words = watched_word_group.watched_words
-
-      expect(watched_word_group.action).to eq(old_action)
-      expect(watched_words.size).to eq(2)
-      expect(watched_words.map(&:word)).to contain_exactly(watched_word_1.word, watched_word_2.word)
-      expect(watched_words.map(&:action).uniq).to contain_exactly(old_action)
+      expect {
+        watched_word_group.create_or_update_members(
+          words,
+          action_key: WatchedWord.actions[watched_word_group.action],
+        )
+      }.to raise_error(ActiveRecord::RecordInvalid)
     end
   end
 end
diff --git a/spec/models/watched_word_spec.rb b/spec/models/watched_word_spec.rb
index 16b992a157e18..7e0415b8f8494 100644
--- a/spec/models/watched_word_spec.rb
+++ b/spec/models/watched_word_spec.rb
@@ -24,10 +24,22 @@ RSpec.describe WatchedWord do
 
   it "is case-insensitive by default" do
     expect(described_class.create(word: "Jest").case_sensitive?).to eq(false)
   end
 
+  it "limits the number of characters in a word" do
+    w = Fabricate.build(:watched_word, word: "a" * 101)
+    expect(w).to_not be_valid
+    expect(w.errors[:word]).to be_present
+  end
+
+  it "limits the number of characters in a replacement" do
+    w = Fabricate.build(:watched_word, replacement: "a" * 101)
+    expect(w).to_not be_valid
+    expect(w.errors[:replacement]).to be_present
+  end
+
   describe "action_key=" do
     let(:w) { WatchedWord.new(word: "troll") }
 
     it "sets action attr from symbol" do
       described_class.actions.keys.each do |k|
diff --git a/spec/requests/admin/watched_words_controller_spec.rb b/spec/requests/admin/watched_words_controller_spec.rb
index a5b7e791d7ff1..dd214f6829660 100644
--- a/spec/requests/admin/watched_words_controller_spec.rb
+++ b/spec/requests/admin/watched_words_controller_spec.rb
@@ -87,11 +87,11 @@ RSpec.describe Admin::WatchedWordsController do
         expect(UserHistory.where(action: UserHistory.actions[:watched_word_destroy]).count).to eq(1)
       end
 
       it "should delete watched word group if it's the last word" do
         watched_word_group = Fabricate(:watched_word_group)
-        watched_word.update!(watched_word_group: watched_word_group)
+        watched_word = watched_word_group.watched_words.first
 
         delete "/admin/customize/watched_words/#{watched_word.id}.json"
 
         expect(response.status).to eq(200)
         expect(WatchedWordGroup.exists?(id: watched_word_group.id)).to be_falsey
diff --git a/spec/system/admin_watched_words_spec.rb b/spec/system/admin_watched_words_spec.rb
index 627f9cce92847..500ff9d193230 100644
--- a/spec/system/admin_watched_words_spec.rb
+++ b/spec/system/admin_watched_words_spec.rb
@@ -15,6 +15,13 @@ describe "Admin Watched Words", type: :system, js: true do
 
     ww_page.visit
 
     expect(ww_page).to have_word
   end
+
+  it "shows error when character limit is exceeded" do
+    ww_page.visit
+    ww_page.add_word "a" * 101
+
+    expect(ww_page).to have_error("Word is too long (maximum is 100 characters)")
+  end
 end
diff --git a/spec/system/page_objects/pages/admin_watched_words.rb b/spec/system/page_objects/pages/admin_watched_words.rb
index 43ca568f498d5..2c61f992182bc 100644
--- a/spec/system/page_objects/pages/admin_watched_words.rb
+++ b/spec/system/page_objects/pages/admin_watched_words.rb
@@ -18,8 +18,12 @@ module PageObjects
       end
 
       def has_word?
         has_css?(".watched-words-detail .show-words-checkbox")
       end
+
+      def has_error?(error)
+        has_css?(".dialog-container .dialog-body", text: error)
+      end
     end
   end
 end

Vulnerable code:   before_validation { self.word = WatchedWord.normalize_word(self.word) }
      self.save! if self.changed?
        watched_word =
          WatchedWord.create_or_update_word(
            params.merge(word: word, watched_word_group_id: self.id),
          )

        if !watched_word.valid?
          self.errors.merge!(watched_word.errors)
          raise ActiveRecord::Rollback
        end
Fabricator(:watched_word_group) { action WatchedWord.actions[:block] }
  fab!(:watched_word_1) { Fabricate(:watched_word, watched_word_group_id: watched_word_group.id) }
    it "leaves membership intact if update fails" do
      watched_word_group.create_or_update_members(
        words,
        action_key: WatchedWord.actions[watched_word_group.action],
      )

      expect(watched_word_group.reload.errors).not_to be_empty

      watched_words = watched_word_group.watched_words

      expect(watched_word_group.action).to eq(old_action)
      expect(watched_words.size).to eq(2)
      expect(watched_words.map(&:word)).to contain_exactly(watched_word_1.word, watched_word_2.word)
      expect(watched_words.map(&:action).uniq).to contain_exactly(old_action)
        watched_word.update!(watched_word_group: watched_word_group)
Fixed code:   before_validation do
    self.word = WatchedWord.normalize_word(self.word)
    self.replacement = WatchedWord.normalize_word(self.replacement) if self.replacement.present?
  end
  validates :replacement, length: { maximum: 100 }
  validate :watched_words_validation
  def watched_words_validation
    watched_words.each { |word| errors.merge!(word.errors) }
    errors.add(:watched_words, :empty) if watched_words.empty?
  end

        watched_word = WatchedWord.create_or_update_word(params.merge(word: word))
        self.watched_words << watched_word

      self.save!
Fabricator(:watched_word_group) do
  action WatchedWord.actions[:block]
  watched_words { [Fabricate.build(:watched_word)] }
end
  let!(:watched_word_1) { watched_word_group.watched_words.first }
    it "raises an error if validation fails" do
      expect {
        watched_word_group.create_or_update_members(
          words,
          action_key: WatchedWord.actions[watched_word_group.action],
        )
      }.to raise_error(ActiveRecord::RecordInvalid)
  it "limits the number of characters in a word" do
    w = Fabricate.build(:watched_word, word: "a" * 101)
    expect(w).to_not be_valid
    expect(w.errors[:word]).to be_present
  end

  it "limits the number of characters in a replacement" do
    w = Fabricate.build(:watched_word, replacement: "a" * 101)
    expect(w).to_not be_valid
    expect(w.errors[:replacement]).to be_present
  end

        watched_word = watched_word_group.watched_words.first

  it "shows error when character limit is exceeded" do
    ww_page.visit
    ww_page.add_word "a" * 101

    expect(ww_page).to have_error("Word is too long (maximum is 100 characters)")
  end

      def has_error?(error)
        has_css?(".dialog-container .dialog-body", text: error)
      end