SECURITY: Limit chat drafts length and preloaded count (#19987)

Only allow maximum of `50_000` characters for chat drafts. A hidden `max_chat_draft_length` setting can control this limit. A migration is also provided to delete any abusive draft in the database.

The number of drafts loaded on current user has also been limited and ordered by most recent update.

Note that spec files moved are not directly related to the fix.

Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
Co-authored-by: Régis Hanol <regis@hanol.fr>

plugins/chat/app/controllers/chat_controller.rb

@@ -432,9 +432,10 @@ class Chat::ChatController < Chat::ChatBaseController
 
   def set_draft
     if params[:data].present?
-      ChatDraft.find_or_initialize_by(user: current_user, chat_channel_id: @chat_channel.id).update(
-        data: params[:data],
-      )
+      ChatDraft.find_or_initialize_by(
+        user: current_user,
+        chat_channel_id: @chat_channel.id,
+      ).update!(data: params[:data])
     else
       ChatDraft.where(user: current_user, chat_channel_id: @chat_channel.id).destroy_all
     end

plugins/chat/app/models/chat_draft.rb

@@ -3,6 +3,13 @@
 class ChatDraft < ActiveRecord::Base
   belongs_to :user
   belongs_to :chat_channel
+
+  validate :data_length
+  def data_length
+    if self.data && self.data.length > SiteSetting.max_chat_draft_length
+      self.errors.add(:base, I18n.t("chat.errors.draft_too_long"))
+    end
+  end
 end
 
 # == Schema Information

plugins/chat/assets/javascripts/discourse/services/chat.js

@@ -373,11 +373,13 @@ export default class Chat extends Service {
       data.data = JSON.stringify(draft);
     }
 
-    ajax("/chat/drafts", { type: "POST", data, ignoreUnsent: false })
+    ajax("/chat/drafts.json", { type: "POST", data, ignoreUnsent: false })
       .then(() => {
         this.markNetworkAsReliable();
       })
       .catch((error) => {
+        // we ignore a draft which can't be saved because it's too big
+        // and only deal with network error for now
         if (!error.jqXHR?.responseJSON?.errors?.length) {
           this.markNetworkAsUnreliable();
         }

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

@@ -59,6 +59,7 @@ en:
       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."
+      draft_too_long: "Draft is too long."
       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

@@ -110,3 +110,6 @@ chat:
     default: 5
     max: 10
     min: 0
+  max_chat_draft_length:
+    default: 50_000
+    hidden: true

plugins/chat/db/post_migrate/20230116090324_drop_chat_drafts_over_max_length.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class DropChatDraftsOverMaxLength < ActiveRecord::Migration[7.0]
+  def up
+    if table_exists?(:chat_drafts)
+      # Delete abusive drafts
+      execute <<~SQL
+        DELETE FROM chat_drafts
+        WHERE LENGTH(data) > 50000
+      SQL
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/plugin.rb

@@ -448,6 +448,8 @@ after_initialize do
   add_to_serializer(:current_user, :chat_drafts) do
     ChatDraft
       .where(user_id: object.id)
+      .order(updated_at: :desc)
+      .limit(20)
       .pluck(:chat_channel_id, :data)
       .map { |row| { channel_id: row[0], data: row[1] } }
   end

plugins/chat/spec/fabricators/chat_fabricator.rb

@@ -126,3 +126,16 @@ Fabricator(:user_chat_channel_membership_for_dm, from: :user_chat_channel_member
   desktop_notification_level 2
   mobile_notification_level 2
 end
+
+Fabricator(:chat_draft) do
+  user
+  chat_channel
+
+  transient :value, "chat draft message"
+  transient :uploads, []
+  transient :reply_to_msg
+
+  data do |attrs|
+    { value: attrs[:value], replyToMsg: attrs[:reply_to_msg], uploads: attrs[:uploads] }.to_json
+  end
+end

plugins/chat/spec/models/chat_draft_spec.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+RSpec.describe ChatDraft do
+  before { SiteSetting.max_chat_draft_length = 100 }
+
+  it "errors when data.value is greater than `max_chat_draft_length`" do
+    draft =
+      described_class.create(
+        user_id: Fabricate(:user).id,
+        chat_channel_id: Fabricate(:chat_channel).id,
+        data: { value: "A" * (SiteSetting.max_chat_draft_length + 1) }.to_json,
+      )
+
+    expect(draft.errors.full_messages).to eq(
+      [I18n.t("chat.errors.draft_too_long", { maximum: SiteSetting.max_chat_draft_length })],
+    )
+  end
+end

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -1286,6 +1286,19 @@ RSpec.describe Chat::ChatController do
         post "/chat/drafts.json", params: { chat_channel_id: dm_channel.id, data: "{}" }
       }.to change { ChatDraft.count }.by(1)
     end
+
+    it "cannot create a too long chat draft" do
+      SiteSetting.max_chat_draft_length = 100
+
+      post "/chat/drafts.json",
+           params: {
+             chat_channel_id: chat_channel.id,
+             data: { value: "a" * (SiteSetting.max_chat_draft_length + 1) }.to_json,
+           }
+
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to eq([I18n.t("chat.errors.draft_too_long")])
+    end
   end
 
   describe "#message_link" do

plugins/chat/spec/serializer/core_ext/current_user_serializer_spec.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+RSpec.describe CurrentUserSerializer do
+  fab!(:current_user) { Fabricate(:user) }
+
+  let(:serializer) do
+    described_class.new(current_user, scope: Guardian.new(current_user), root: false)
+  end
+
+  before do
+    SiteSetting.chat_enabled = true
+    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
+    current_user.user_option.update(chat_enabled: true)
+  end
+
+  describe "#chat_drafts" do
+    context "when user can't chat" do
+      before { SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:staff] }
+
+      it "is not present" do
+        expect(serializer.as_json[:chat_drafts]).to be_blank
+      end
+    end
+
+    it "is ordered by most recent drafts" do
+      Fabricate(:chat_draft, user: current_user, value: "second draft")
+      Fabricate(:chat_draft, user: current_user, value: "first draft")
+
+      values =
+        serializer.as_json[:chat_drafts].map { |draft| MultiJson.load(draft[:data])["value"] }
+      expect(values).to eq(["first draft", "second draft"])
+    end
+
+    it "limits the numbers of drafts" do
+      21.times { Fabricate(:chat_draft, user: current_user) }
+
+      expect(serializer.as_json[:chat_drafts].length).to eq(20)
+    end
+  end
+end