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.

app/models/watched_word.rb

@@ -3,7 +3,10 @@
 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?://}
@@ -13,6 +16,7 @@ class WatchedWord < ActiveRecord::Base
   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] }

app/models/watched_word_group.rb

@@ -2,25 +2,25 @@
 
 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
 

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

spec/models/watched_word_group_spec.rb

@@ -2,7 +2,7 @@
 
 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
@@ -26,7 +26,7 @@ RSpec.describe WatchedWordGroup do
       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
@@ -38,19 +38,12 @@ RSpec.describe WatchedWordGroup do
       )
       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

spec/models/watched_word_spec.rb

@@ -26,6 +26,18 @@ RSpec.describe WatchedWord 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") }
 

spec/requests/admin/watched_words_controller_spec.rb

@@ -89,7 +89,7 @@ RSpec.describe Admin::WatchedWordsController do
 
       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"
 

spec/system/admin_watched_words_spec.rb

@@ -17,4 +17,11 @@ describe "Admin Watched Words", type: :system, js: true do
 
     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

spec/system/page_objects/pages/admin_watched_words.rb

@@ -20,6 +20,10 @@ module PageObjects
       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