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.
This commit is contained in:
Joffrey JAFFEUX 2023-01-17 12:55:14 +01:00 committed by Bianca Nenciu
parent 971f97d03a
commit a02af9e6db
11 changed files with 121 additions and 4 deletions

View File

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

View File

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

View File

@ -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();
}

View File

@ -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."

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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