Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Limit chat message char length (#19207)
Only allow maximum of 6000 characters for chat messages when they
are created or edited. A hidden setting can control this limit,
6000 is the default.

There is also a migration here to truncate any existing messages to
6000 characters if the message is already over that and if the
chat_messages table exists. We also set cooked_version to NULL
for those messages so we can identify them for rebake.
  • Loading branch information
martin-brennan committed Nov 28, 2022
1 parent a71f6cf commit 3de765c
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 17 deletions.
@@ -1,7 +1,6 @@
# frozen_string_literal: true

class Chat::IncomingChatWebhooksController < ApplicationController
WEBHOOK_MAX_MESSAGE_LENGTH = 2000
WEBHOOK_MESSAGES_PER_MINUTE_LIMIT = 10

skip_before_action :verify_authenticity_token, :redirect_to_login_if_required
Expand Down Expand Up @@ -80,9 +79,9 @@ def find_and_rate_limit_webhook(key)
end

def validate_message_length(message)
return if message.length <= WEBHOOK_MAX_MESSAGE_LENGTH
return if message.length <= SiteSetting.chat_maximum_message_length
raise Discourse::InvalidParameters.new(
"Body cannot be over #{WEBHOOK_MAX_MESSAGE_LENGTH} characters",
"Body cannot be over #{SiteSetting.chat_maximum_message_length} characters",
)
end

Expand Down
14 changes: 14 additions & 0 deletions plugins/chat/app/models/chat_message.rb
Expand Up @@ -48,6 +48,16 @@ def validate_message(has_uploads:)
),
)
end

if message_too_long?
self.errors.add(
:base,
I18n.t(
"chat.errors.message_too_long",
maximum: SiteSetting.chat_maximum_message_length,
),
)
end
end

def attach_uploads(uploads)
Expand Down Expand Up @@ -205,6 +215,10 @@ def message_too_short?
message.length < SiteSetting.chat_minimum_message_length
end

def message_too_long?
message.length > SiteSetting.chat_maximum_message_length
end

def ensure_last_editor_id
self.last_editor_id ||= self.user_id
end
Expand Down
1 change: 1 addition & 0 deletions plugins/chat/config/locales/server.en.yml
Expand Up @@ -48,6 +48,7 @@ en:
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."
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
7 changes: 7 additions & 0 deletions plugins/chat/config/settings.yml
Expand Up @@ -75,6 +75,13 @@ chat:
min: 1
max: 50
client: true
chat_maximum_message_length:
type: integer
default: 6000
client: true
hidden: true
min: 100
max: 12000
chat_allow_uploads:
default: true
client: true
Expand Down
@@ -0,0 +1,20 @@
# frozen_string_literal: true

class TruncateChatMessagesOverMaxLength < ActiveRecord::Migration[7.0]
def up
if table_exists?(:chat_messages)
# 6000 is the default of the chat_maximum_message_length
# site setting, its safe to do this because this will be
# run the first time the setting is introduced.
execute <<~SQL
UPDATE chat_messages
SET message = LEFT(message, 6000), cooked_version = NULL
WHERE LENGTH(message) > 6000
SQL
end
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
17 changes: 17 additions & 0 deletions plugins/chat/spec/components/chat_message_creator_spec.rb
Expand Up @@ -72,6 +72,23 @@
)
end

it "errors when length is greater than `chat_maximum_message_length`" do
SiteSetting.chat_maximum_message_length = 100
creator =
Chat::ChatMessageCreator.create(
chat_channel: public_chat_channel,
user: user1,
content: "a really long and in depth message that is just too detailed" * 100,
)
expect(creator.failed?).to eq(true)
expect(creator.error.message).to match(
I18n.t(
"chat.errors.message_too_long",
{ maximum: SiteSetting.chat_maximum_message_length },
),
)
end

it "allows message creation when length is less than `chat_minimum_message_length` when upload is present" do
upload = Fabricate(:upload, user: user1)
SiteSetting.chat_minimum_message_length = 10
Expand Down
48 changes: 36 additions & 12 deletions plugins/chat/spec/components/chat_message_updater_spec.rb
Expand Up @@ -68,15 +68,38 @@ def create_chat_message(user, message, channel, upload_ids: nil)
expect(chat_message.reload.message).to eq(og_message)
end

it "errors when length is greater than `chat_maximum_message_length`" do
SiteSetting.chat_maximum_message_length = 100
og_message = "This won't be changed!"
chat_message = create_chat_message(user1, og_message, public_chat_channel)
new_message = "2 long" * 100

updater =
Chat::ChatMessageUpdater.update(
guardian: guardian,
chat_message: chat_message,
new_content: new_message,
)
expect(updater.failed?).to eq(true)
expect(updater.error.message).to match(
I18n.t(
"chat.errors.message_too_long",
{ maximum: SiteSetting.chat_maximum_message_length },
),
)
expect(chat_message.reload.message).to eq(og_message)
end

it "errors if a user other than the message user is trying to edit the message" do
og_message = "This won't be changed!"
chat_message = create_chat_message(user1, og_message, public_chat_channel)
new_message = "2 short"
updater = Chat::ChatMessageUpdater.update(
guardian: Guardian.new(Fabricate(:user)),
chat_message: chat_message,
new_content: new_message,
)
updater =
Chat::ChatMessageUpdater.update(
guardian: Guardian.new(Fabricate(:user)),
chat_message: chat_message,
new_content: new_message,
)
expect(updater.failed?).to eq(true)
expect(updater.error).to match(Discourse::InvalidAccess)
end
Expand All @@ -95,13 +118,14 @@ def create_chat_message(user, message, channel, upload_ids: nil)

it "publishes a DiscourseEvent for updated messages" do
chat_message = create_chat_message(user1, "This will be changed", public_chat_channel)
events = DiscourseEvent.track_events {
Chat::ChatMessageUpdater.update(
guardian: guardian,
chat_message: chat_message,
new_content: "Change to this!",
)
}
events =
DiscourseEvent.track_events do
Chat::ChatMessageUpdater.update(
guardian: guardian,
chat_message: chat_message,
new_content: "Change to this!",
)
end
expect(events.map { _1[:event_name] }).to include(:chat_message_edited)
end

Expand Down
Expand Up @@ -19,10 +19,10 @@
expect(response.status).to eq(400)
end

it "errors when the body is over WEBHOOK_MAX_MESSAGE_LENGTH characters" do
it "errors when the body is over chat_maximum_message_length characters" do
post "/chat/hooks/#{webhook.key}.json",
params: {
text: "$" * (Chat::IncomingChatWebhooksController::WEBHOOK_MAX_MESSAGE_LENGTH + 1),
text: "$" * (SiteSetting.chat_maximum_message_length + 1),
}
expect(response.status).to eq(400)
end
Expand Down

0 comments on commit 3de765c

Please sign in to comment.