CVE-2023-22740: Allocation of Resources Without Limits or Throttling (CWE-770)
value: Discourse is an open source platform for community discussion. Versions prior to 3.1.0.beta1 (beta) (tests-passed) are vulnerable to  Allocation of Resources Without Limits. Users can create chat drafts of an unlimited length, which can cause a denial of service by generating an excessive load on the server. Additionally, an unlimited number of drafts were loaded when loading the user. This issue has been patched in version 2.1.0.beta1 (beta) and (tests-passed). Users should upgrade to the latest version where a limit has been introduced. There are no workarounds available.
CWE description: The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
CVSS v3 base score: 6.5
Published: 2023-01-27T01:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: plugins/chat/app/controllers/chat_controller.rb, plugins/chat/app/models/chat_draft.rb, plugins/chat/assets/javascripts/discourse/services/chat.js, plugins/chat/config/locales/server.en.yml, plugins/chat/config/settings.yml, plugins/chat/db/post_migrate/20230116090324_drop_chat_drafts_over_max_length.rb, plugins/chat/plugin.rb, plugins/chat/spec/fabricators/chat_fabricator.rb, plugins/chat/spec/models/chat_draft_spec.rb, plugins/chat/spec/requests/chat_controller_spec.rb, plugins/chat/spec/serializer/core_ext/current_user_serializer_spec.rb
Commit date: 2023-01-25 13:50:10 +0200
Commit message: 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>
Diff: diff --git a/plugins/chat/app/controllers/chat_controller.rb b/plugins/chat/app/controllers/chat_controller.rb
index 9849ffc29094..31ad38e02a65 100644
--- a/plugins/chat/app/controllers/chat_controller.rb
+++ b/plugins/chat/app/controllers/chat_controller.rb
@@ -430,13 +430,14 @@ class Chat::ChatController < Chat::ChatBaseController
     end
   end
 
   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
 
     render json: success_json
diff --git a/plugins/chat/app/models/chat_draft.rb b/plugins/chat/app/models/chat_draft.rb
index 1d3781fa8267..7dc1b7feeb0e 100644
--- a/plugins/chat/app/models/chat_draft.rb
+++ b/plugins/chat/app/models/chat_draft.rb
@@ -1,10 +1,17 @@
 # frozen_string_literal: true
 
 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
 #
 # Table name: chat_drafts
diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js
index 0cb1c56ace32..931f480e4385 100644
--- a/plugins/chat/assets/javascripts/discourse/services/chat.js
+++ b/plugins/chat/assets/javascripts/discourse/services/chat.js
@@ -371,15 +371,17 @@ export default class Chat extends Service {
     const data = { chat_channel_id: channelId };
     if (draft) {
       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();
         }
       });
   }
diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml
index f0c8abefb9d8..372eda5e4a38 100644
--- a/plugins/chat/config/locales/server.en.yml
+++ b/plugins/chat/config/locales/server.en.yml
@@ -57,10 +57,11 @@ en:
       channel_cannot_be_archived: "The channel cannot be archived at this time, it must be either closed or open to archive."
       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."
+      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."
       cant_update_direct_message_channel: "Direct message channel properties like name and description can’t be updated."
       not_accepting_dms: "Sorry, %{username} is not accepting messages at the moment."
diff --git a/plugins/chat/config/settings.yml b/plugins/chat/config/settings.yml
index 072e9d979211..015477dc0387 100644
--- a/plugins/chat/config/settings.yml
+++ b/plugins/chat/config/settings.yml
@@ -108,5 +108,8 @@ chat:
     client: true
     type: integer
     default: 5
     max: 10
     min: 0
+  max_chat_draft_length:
+    default: 50_000
+    hidden: true
diff --git a/plugins/chat/db/post_migrate/20230116090324_drop_chat_drafts_over_max_length.rb b/plugins/chat/db/post_migrate/20230116090324_drop_chat_drafts_over_max_length.rb
new file mode 100644
index 000000000000..1d6913086f54
--- /dev/null
+++ b/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
diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb
index 04a5f83a1376..10e0cb4d3f76 100644
--- a/plugins/chat/plugin.rb
+++ b/plugins/chat/plugin.rb
@@ -446,10 +446,12 @@ after_initialize do
   end
 
   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
 
   add_to_serializer(:current_user, :include_chat_drafts?) { include_has_chat_enabled? }
diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb
index e123c06bc543..a8d7f3e544eb 100644
--- a/plugins/chat/spec/fabricators/chat_fabricator.rb
+++ b/plugins/chat/spec/fabricators/chat_fabricator.rb
@@ -124,5 +124,18 @@ Fabricator(:user_chat_channel_membership_for_dm, from: :user_chat_channel_member
   chat_channel
   following true
   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
diff --git a/plugins/chat/spec/models/chat_draft_spec.rb b/plugins/chat/spec/models/chat_draft_spec.rb
new file mode 100644
index 000000000000..27794bafaec0
--- /dev/null
+++ b/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
diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb
index 0b993612354a..2b9c3e5e3b6b 100644
--- a/plugins/chat/spec/requests/chat_controller_spec.rb
+++ b/plugins/chat/spec/requests/chat_controller_spec.rb
@@ -1284,10 +1284,23 @@ RSpec.describe Chat::ChatController do
       DirectMessageUser.create(user: user, direct_message: dm_channel.chatable)
       expect {
         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
     it "ensures message's channel can be seen" do
       channel = Fabricate(:category_channel, chatable: Fabricate(:category))
diff --git a/plugins/chat/spec/serializer/core_ext/current_user_serializer_spec.rb b/plugins/chat/spec/serializer/core_ext/current_user_serializer_spec.rb
new file mode 100644
index 000000000000..2e2fd20a0eff
--- /dev/null
+++ b/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

Vulnerable code:       ChatDraft.find_or_initialize_by(user: current_user, chat_channel_id: @chat_channel.id).update(
        data: params[:data],
      )
    ajax("/chat/drafts", { type: "POST", data, ignoreUnsent: false })
Fixed code:       ChatDraft.find_or_initialize_by(
        user: current_user,
        chat_channel_id: @chat_channel.id,
      ).update!(data: params[:data])

  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
    ajax("/chat/drafts.json", { type: "POST", data, ignoreUnsent: false })
        // we ignore a draft which can't be saved because it's too big
        // and only deal with network error for now
      draft_too_long: "Draft is too long."
  max_chat_draft_length:
    default: 50_000
    hidden: true
# 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
      .order(updated_at: :desc)
      .limit(20)

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
# 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

    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
# 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