From a02af9e6db81ab1cb18ace38b8ecf17c3527bdd6 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 17 Jan 2023 12:55:14 +0100 Subject: [PATCH] SECURITY: Limit chat drafts length and preloaded count 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. --- .../chat/app/controllers/chat_controller.rb | 7 ++-- plugins/chat/app/models/chat_draft.rb | 7 ++++ .../javascripts/discourse/services/chat.js | 4 +- plugins/chat/config/locales/server.en.yml | 1 + plugins/chat/config/settings.yml | 3 ++ ...090324_drop_chat_drafts_over_max_length.rb | 17 ++++++++ plugins/chat/plugin.rb | 2 + .../chat/spec/fabricators/chat_fabricator.rb | 13 ++++++ plugins/chat/spec/models/chat_draft_spec.rb | 18 +++++++++ .../spec/requests/chat_controller_spec.rb | 13 ++++++ .../core_ext/current_user_serializer_spec.rb | 40 +++++++++++++++++++ 11 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 plugins/chat/db/post_migrate/20230116090324_drop_chat_drafts_over_max_length.rb create mode 100644 plugins/chat/spec/models/chat_draft_spec.rb create mode 100644 plugins/chat/spec/serializer/core_ext/current_user_serializer_spec.rb diff --git a/plugins/chat/app/controllers/chat_controller.rb b/plugins/chat/app/controllers/chat_controller.rb index 9849ffc2909..31ad38e02a6 100644 --- a/plugins/chat/app/controllers/chat_controller.rb +++ b/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 diff --git a/plugins/chat/app/models/chat_draft.rb b/plugins/chat/app/models/chat_draft.rb index 1d3781fa826..7dc1b7feeb0 100644 --- a/plugins/chat/app/models/chat_draft.rb +++ b/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 diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index 0cb1c56ace3..931f480e438 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/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(); } diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index f0c8abefb9d..372eda5e4a3 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/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." diff --git a/plugins/chat/config/settings.yml b/plugins/chat/config/settings.yml index 072e9d97921..015477dc038 100644 --- a/plugins/chat/config/settings.yml +++ b/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 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 00000000000..1d6913086f5 --- /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 b3b5070a198..8a9127e51e6 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -455,6 +455,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 diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index e123c06bc54..a8d7f3e544e 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/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 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 00000000000..27794bafaec --- /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 8ff5912df5b..c2eac575378 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -1285,6 +1285,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 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 00000000000..2e2fd20a0ef --- /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