SECURITY: Limit chat message char length (#19207)

Only allow maximum of 6000 characters for chat messages when they
are created or edited. A hidden setting can control this limit,
6000 is the default.

There is also a migration here to truncate any existing messages to
6000 characters if the message is already over that and if the
chat_messages table exists. We also set cooked_version to NULL
for those messages so we can identify them for rebake.

plugins/chat/app/controllers/incoming_chat_webhooks_controller.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 class Chat::IncomingChatWebhooksController < ApplicationController
-  WEBHOOK_MAX_MESSAGE_LENGTH = 2000
   WEBHOOK_MESSAGES_PER_MINUTE_LIMIT = 10
 
   skip_before_action :verify_authenticity_token, :redirect_to_login_if_required
@@ -80,9 +79,9 @@ class Chat::IncomingChatWebhooksController < ApplicationController
   end
 
   def validate_message_length(message)
-    return if message.length <= WEBHOOK_MAX_MESSAGE_LENGTH
+    return if message.length <= SiteSetting.chat_maximum_message_length
     raise Discourse::InvalidParameters.new(
-            "Body cannot be over #{WEBHOOK_MAX_MESSAGE_LENGTH} characters",
+            "Body cannot be over #{SiteSetting.chat_maximum_message_length} characters",
           )
   end
 

plugins/chat/app/models/chat_message.rb

@@ -48,6 +48,16 @@ class ChatMessage < ActiveRecord::Base
         ),
       )
     end
+
+    if message_too_long?
+      self.errors.add(
+        :base,
+        I18n.t(
+          "chat.errors.message_too_long",
+          maximum: SiteSetting.chat_maximum_message_length,
+        ),
+      )
+    end
   end
 
   def attach_uploads(uploads)
@@ -205,6 +215,10 @@ class ChatMessage < ActiveRecord::Base
     message.length < SiteSetting.chat_minimum_message_length
   end
 
+  def message_too_long?
+    message.length > SiteSetting.chat_maximum_message_length
+  end
+
   def ensure_last_editor_id
     self.last_editor_id ||= self.user_id
   end

plugins/chat/config/locales/server.en.yml

@@ -48,6 +48,7 @@ en:
       duplicate_message: "You posted an identical message too recently."
       delete_channel_failed: "Delete channel failed, please try again."
       minimum_length_not_met: "Message is too short, must have a minimum of %{minimum} characters."
+      message_too_long: "Message is too long, messages must be a maximum of %{maximum} characters."
       max_reactions_limit_reached: "New reactions are not allowed on this message."
       message_move_invalid_channel: "The source and destination channel must be public channels."
       message_move_no_messages_found: "No messages were found with the provided message IDs."

plugins/chat/config/settings.yml

@@ -75,6 +75,13 @@ chat:
     min: 1
     max: 50
     client: true
+  chat_maximum_message_length:
+    type: integer
+    default: 6000
+    client: true
+    hidden: true
+    min: 100
+    max: 12000
   chat_allow_uploads:
     default: true
     client: true

plugins/chat/db/post_migrate/20221117052348_truncate_chat_messages_over_max_length.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class TruncateChatMessagesOverMaxLength < ActiveRecord::Migration[7.0]
+  def up
+    if table_exists?(:chat_messages)
+      # 6000 is the default of the chat_maximum_message_length
+      # site setting, its safe to do this because this will be
+      # run the first time the setting is introduced.
+      execute <<~SQL
+        UPDATE chat_messages
+        SET message = LEFT(message, 6000), cooked_version = NULL
+        WHERE LENGTH(message) > 6000
+      SQL
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/spec/components/chat_message_creator_spec.rb

@@ -72,6 +72,23 @@ describe Chat::ChatMessageCreator do
       )
     end
 
+    it "errors when length is greater than `chat_maximum_message_length`" do
+      SiteSetting.chat_maximum_message_length = 100
+      creator =
+        Chat::ChatMessageCreator.create(
+          chat_channel: public_chat_channel,
+          user: user1,
+          content: "a really long and in depth message that is just too detailed" * 100,
+        )
+      expect(creator.failed?).to eq(true)
+      expect(creator.error.message).to match(
+        I18n.t(
+          "chat.errors.message_too_long",
+          { maximum: SiteSetting.chat_maximum_message_length },
+        ),
+      )
+    end
+
     it "allows message creation when length is less than `chat_minimum_message_length` when upload is present" do
       upload = Fabricate(:upload, user: user1)
       SiteSetting.chat_minimum_message_length = 10

plugins/chat/spec/components/chat_message_updater_spec.rb

@@ -68,15 +68,38 @@ describe Chat::ChatMessageUpdater do
     expect(chat_message.reload.message).to eq(og_message)
   end
 
+  it "errors when length is greater than `chat_maximum_message_length`" do
+    SiteSetting.chat_maximum_message_length = 100
+    og_message = "This won't be changed!"
+    chat_message = create_chat_message(user1, og_message, public_chat_channel)
+    new_message = "2 long" * 100
+
+    updater =
+      Chat::ChatMessageUpdater.update(
+        guardian: guardian,
+        chat_message: chat_message,
+        new_content: new_message,
+      )
+    expect(updater.failed?).to eq(true)
+    expect(updater.error.message).to match(
+      I18n.t(
+        "chat.errors.message_too_long",
+        { maximum: SiteSetting.chat_maximum_message_length },
+      ),
+    )
+    expect(chat_message.reload.message).to eq(og_message)
+  end
+
   it "errors if a user other than the message user is trying to edit the message" do
     og_message = "This won't be changed!"
     chat_message = create_chat_message(user1, og_message, public_chat_channel)
     new_message = "2 short"
-    updater = Chat::ChatMessageUpdater.update(
+    updater =
-      guardian: Guardian.new(Fabricate(:user)),
+      Chat::ChatMessageUpdater.update(
-      chat_message: chat_message,
+        guardian: Guardian.new(Fabricate(:user)),
-      new_content: new_message,
+        chat_message: chat_message,
-    )
+        new_content: new_message,
+      )
     expect(updater.failed?).to eq(true)
     expect(updater.error).to match(Discourse::InvalidAccess)
   end
@@ -95,13 +118,14 @@ describe Chat::ChatMessageUpdater do
 
   it "publishes a DiscourseEvent for updated messages" do
     chat_message = create_chat_message(user1, "This will be changed", public_chat_channel)
-    events = DiscourseEvent.track_events {
+    events =
-      Chat::ChatMessageUpdater.update(
+      DiscourseEvent.track_events do
-        guardian: guardian,
+        Chat::ChatMessageUpdater.update(
-        chat_message: chat_message,
+          guardian: guardian,
-        new_content: "Change to this!",
+          chat_message: chat_message,
-      )
+          new_content: "Change to this!",
-    }
+        )
+      end
     expect(events.map { _1[:event_name] }).to include(:chat_message_edited)
   end
 

plugins/chat/spec/requests/incoming_chat_webhooks_controller_spec.rb

@@ -19,10 +19,10 @@ RSpec.describe Chat::IncomingChatWebhooksController do
       expect(response.status).to eq(400)
     end
 
-    it "errors when the body is over WEBHOOK_MAX_MESSAGE_LENGTH characters" do
+    it "errors when the body is over chat_maximum_message_length characters" do
       post "/chat/hooks/#{webhook.key}.json",
            params: {
-             text: "$" * (Chat::IncomingChatWebhooksController::WEBHOOK_MAX_MESSAGE_LENGTH + 1),
+             text: "$" * (SiteSetting.chat_maximum_message_length + 1),
            }
       expect(response.status).to eq(400)
     end