Skip to content
Permalink
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
3 people committed Jan 25, 2023
1 parent ec2ed5b commit 5eaf080
Show file tree
Hide file tree
Showing 11 changed files with 121 additions and 4 deletions.
7 changes: 4 additions & 3 deletions plugins/chat/app/controllers/chat_controller.rb
Expand Up @@ -432,9 +432,10 @@ def flag

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
Expand Down
7 changes: 7 additions & 0 deletions plugins/chat/app/models/chat_draft.rb
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion plugins/chat/assets/javascripts/discourse/services/chat.js
Expand Up @@ -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();
}
Expand Down
1 change: 1 addition & 0 deletions plugins/chat/config/locales/server.en.yml
Expand Up @@ -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."
Expand Down
3 changes: 3 additions & 0 deletions plugins/chat/config/settings.yml
Expand Up @@ -110,3 +110,6 @@ chat:
default: 5
max: 10
min: 0
max_chat_draft_length:
default: 50_000
hidden: true
@@ -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
2 changes: 2 additions & 0 deletions plugins/chat/plugin.rb
Expand Up @@ -448,6 +448,8 @@ def self.onebox_template
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
Expand Down
13 changes: 13 additions & 0 deletions plugins/chat/spec/fabricators/chat_fabricator.rb
Expand Up @@ -126,3 +126,16 @@
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
18 changes: 18 additions & 0 deletions 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
13 changes: 13 additions & 0 deletions plugins/chat/spec/requests/chat_controller_spec.rb
Expand Up @@ -1286,6 +1286,19 @@ def create_notification_and_mention_for(user, sender, msg)
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
Expand Down
@@ -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

0 comments on commit 5eaf080

Please sign in to comment.