From 4ae26bcaac135748ffeae8d1b51579ee121561b5 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 13 Jul 2023 09:44:56 +1000 Subject: [PATCH] FIX: Render excerpt HTML for chat replies and edit (#22559) It is now safe to render the message excerpt as HTML since it is no longer using text_entities: true in the server PrettyText.excerpt call when creating the message excerpt from the cooked HTML. This will fix the issue of things like mentions showing HTML code instead of the actual mention when replying, and cannot be used to inject improper HTML like style tags via XSS. --- .../chat-composer-message-details.hbs | 2 +- .../chat-message-in-reply-to-indicator.hbs | 2 +- .../spec/system/chat/composer/channel_spec.rb | 2 +- plugins/chat/spec/system/chat_channel_spec.rb | 12 +++++++++- .../spec/system/chat_composer_draft_spec.rb | 8 ++++++- .../system/reply_to_message/drawer_spec.rb | 7 +++++- .../system/reply_to_message/full_page_spec.rb | 24 ++++++++++++++++++- .../system/reply_to_message/mobile_spec.rb | 7 +++++- 8 files changed, 56 insertions(+), 8 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs index 43122bfb7f1..5b5a10eedc7 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-composer-message-details.hbs @@ -8,7 +8,7 @@ {{@message.user.username}} - {{replace-emoji @message.excerpt}} + {{replace-emoji (html-safe @message.excerpt)}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs index 0ad871a1398..716525f3fd1 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message-in-reply-to-indicator.hbs @@ -13,7 +13,7 @@ {{/if}} - {{replace-emoji @message.inReplyTo.excerpt}} + {{replace-emoji (html-safe @message.inReplyTo.excerpt)}} {{/if}} \ No newline at end of file diff --git a/plugins/chat/spec/system/chat/composer/channel_spec.rb b/plugins/chat/spec/system/chat/composer/channel_spec.rb index 0f6980ca6a1..92d2630bee5 100644 --- a/plugins/chat/spec/system/chat/composer/channel_spec.rb +++ b/plugins/chat/spec/system/chat/composer/channel_spec.rb @@ -24,7 +24,7 @@ RSpec.describe "Chat | composer | channel", type: :system, js: true do expect(channel_page.composer.message_details).to have_message( id: message_1.id, - exact_text: "<mark>not marked</mark>", + exact_text: "not marked", ) end diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 483cda9d265..6be6c217969 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -241,7 +241,17 @@ RSpec.describe "Chat channel", type: :system do chat.visit_channel(channel_1) expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq( - "&lt;mark&gt;not marked&lt;/mark&gt;", + "<mark>not marked</mark>", + ) + end + + it "renders safe HTML like mentions (which are just links) in the reply-to" do + message_2.update!(message: "@#{other_user.username} not marked") + message_2.rebake! + chat.visit_channel(channel_1) + + expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq( + "@#{other_user.username} <mark>not marked</mark>", ) end end diff --git a/plugins/chat/spec/system/chat_composer_draft_spec.rb b/plugins/chat/spec/system/chat_composer_draft_spec.rb index d3a3700dda3..a0c254f8d6c 100644 --- a/plugins/chat/spec/system/chat_composer_draft_spec.rb +++ b/plugins/chat/spec/system/chat_composer_draft_spec.rb @@ -3,7 +3,13 @@ RSpec.describe "Chat composer draft", type: :system do fab!(:current_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:chat_channel) } - fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_1) do + Fabricate( + :chat_message, + chat_channel: channel_1, + message: "This is a message for draft and replies", + ) + end let(:chat_page) { PageObjects::Pages::Chat.new } let(:channel_page) { PageObjects::Pages::ChatChannel.new } diff --git a/plugins/chat/spec/system/reply_to_message/drawer_spec.rb b/plugins/chat/spec/system/reply_to_message/drawer_spec.rb index 89f4bab3138..6e37b9700b7 100644 --- a/plugins/chat/spec/system/reply_to_message/drawer_spec.rb +++ b/plugins/chat/spec/system/reply_to_message/drawer_spec.rb @@ -9,7 +9,12 @@ RSpec.describe "Reply to message - channel - drawer", type: :system do fab!(:current_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:category_channel) } fab!(:original_message) do - Fabricate(:chat_message, chat_channel: channel_1, user: Fabricate(:user)) + Fabricate( + :chat_message, + chat_channel: channel_1, + user: Fabricate(:user), + message: "This is a message to reply to!", + ) end before do diff --git a/plugins/chat/spec/system/reply_to_message/full_page_spec.rb b/plugins/chat/spec/system/reply_to_message/full_page_spec.rb index f7ec6a392ab..fcc1a9772b8 100644 --- a/plugins/chat/spec/system/reply_to_message/full_page_spec.rb +++ b/plugins/chat/spec/system/reply_to_message/full_page_spec.rb @@ -9,7 +9,12 @@ RSpec.describe "Reply to message - channel - full page", type: :system do fab!(:current_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:category_channel) } fab!(:original_message) do - Fabricate(:chat_message, chat_channel: channel_1, user: Fabricate(:user)) + Fabricate( + :chat_message, + chat_channel: channel_1, + user: Fabricate(:user), + message: "This is a message to reply to!", + ) end before do @@ -100,5 +105,22 @@ RSpec.describe "Reply to message - channel - full page", type: :system do expect(channel_page).to have_message(text: "reply to message") end + + it "renders safe HTML from the original message excerpt" do + other_user = Fabricate(:user) + original_message.update!(message: "@#{other_user.username} not marked") + original_message.rebake! + chat_page.visit_channel(channel_1) + channel_page.reply_to(original_message) + + expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq( + "@#{other_user.username} <mark>not marked</mark>", + ) + + channel_page.fill_composer("reply to message") + channel_page.click_send_message + + expect(channel_page).to have_message(text: "reply to message") + end end end diff --git a/plugins/chat/spec/system/reply_to_message/mobile_spec.rb b/plugins/chat/spec/system/reply_to_message/mobile_spec.rb index 345dbdb28ab..089b08314bc 100644 --- a/plugins/chat/spec/system/reply_to_message/mobile_spec.rb +++ b/plugins/chat/spec/system/reply_to_message/mobile_spec.rb @@ -9,7 +9,12 @@ RSpec.describe "Reply to message - channel - mobile", type: :system, mobile: tru fab!(:current_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:category_channel) } fab!(:original_message) do - Fabricate(:chat_message, chat_channel: channel_1, user: Fabricate(:user)) + Fabricate( + :chat_message, + chat_channel: channel_1, + user: Fabricate(:user), + message: "This is a message to reply to!", + ) end before do