From a373bf2a01488c206e7feb28a9d2361b22ce6e70 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Thu, 16 Mar 2023 13:40:43 -0600 Subject: [PATCH] SECURITY: XSS on chat excerpts Non-markdown tags weren't being escaped in chat excerpts. This could be triggered by editing a chat message containing a tag (self XSS), or by replying to a chat message with a tag (XSS). Co-authored-by: Jan Cernik --- .../discourse/app/components/choose-topic.hbs | 2 +- .../discourse/app/helpers/replace-emoji.js | 4 ++- .../app/templates/modal/insert-hyperlink.hbs | 2 +- .../discourse/app/templates/review-topics.hbs | 2 +- .../integration/helpers/replace-emoji-test.js | 29 +++++++++++++++++++ .../addon/templates/components/topic-row.hbs | 2 +- .../components/chat-channel-about-view.hbs | 2 +- .../components/chat-channel-card.hbs | 4 +-- .../components/chat-channel-title.hbs | 2 +- plugins/chat/spec/system/chat_channel_spec.rb | 28 ++++++++++++++++++ 10 files changed, 68 insertions(+), 9 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js diff --git a/app/assets/javascripts/discourse/app/components/choose-topic.hbs b/app/assets/javascripts/discourse/app/components/choose-topic.hbs index f9bf06d223a..15e42850396 100644 --- a/app/assets/javascripts/discourse/app/components/choose-topic.hbs +++ b/app/assets/javascripts/discourse/app/components/choose-topic.hbs @@ -30,7 +30,7 @@ /> - {{replace-emoji t.fancy_title}} + {{replace-emoji t.title}} {{bound-category-link diff --git a/app/assets/javascripts/discourse/app/helpers/replace-emoji.js b/app/assets/javascripts/discourse/app/helpers/replace-emoji.js index 3109cf29003..81d78a3a1f4 100644 --- a/app/assets/javascripts/discourse/app/helpers/replace-emoji.js +++ b/app/assets/javascripts/discourse/app/helpers/replace-emoji.js @@ -1,7 +1,9 @@ import { emojiUnescape } from "discourse/lib/text"; -import { htmlSafe } from "@ember/template"; +import { htmlSafe, isHTMLSafe } from "@ember/template"; import { registerUnbound } from "discourse-common/lib/helpers"; +import { escapeExpression } from "discourse/lib/utilities"; registerUnbound("replace-emoji", (text, options) => { + text = isHTMLSafe(text) ? text.toString() : escapeExpression(text); return htmlSafe(emojiUnescape(text, options)); }); diff --git a/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs b/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs index affbf5d1b6e..a69187ed8fb 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs +++ b/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs @@ -21,7 +21,7 @@ data-title={{result.fancy_title}} > - {{replace-emoji result.fancy_title}} + {{replace-emoji result.title}}
{{#if result.category.parentCategory}} {{category-link result.category.parentCategory}} diff --git a/app/assets/javascripts/discourse/app/templates/review-topics.hbs b/app/assets/javascripts/discourse/app/templates/review-topics.hbs index 286e391e0a7..5bed189751f 100644 --- a/app/assets/javascripts/discourse/app/templates/review-topics.hbs +++ b/app/assets/javascripts/discourse/app/templates/review-topics.hbs @@ -16,7 +16,7 @@ href={{rt.relative_url}} rel="noopener noreferrer" target="_blank" - >{{replace-emoji rt.fancy_title}} + >{{replace-emoji rt.title}}
diff --git a/app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js b/app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js new file mode 100644 index 00000000000..2ce8e73be3c --- /dev/null +++ b/app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js @@ -0,0 +1,29 @@ +import { module, test } from "qunit"; +import { setupRenderingTest } from "discourse/tests/helpers/component-test"; +import { render } from "@ember/test-helpers"; +import { hbs } from "ember-cli-htmlbars"; + +module("Integration | Helper | replace-emoji", function (hooks) { + setupRenderingTest(hooks); + + test("it replaces the emoji", async function (assert) { + await render(hbs`{{replace-emoji "some text :heart:"}}`); + + assert.dom(`span`).includesText("some text"); + assert.dom(`.emoji[title="heart"]`).exists(); + }); + + test("it escapes the text", async function (assert) { + await render( + hbs`{{replace-emoji ""}}` + ); + + assert.dom(`span`).hasText(""); + }); + + test("it renders html-safe text", async function (assert) { + await render(hbs`{{replace-emoji (html-safe "safe text")}}`); + + assert.dom(`span`).hasText("safe text"); + }); +}); diff --git a/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs b/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs index d9b2074909b..fc832e4c640 100644 --- a/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs +++ b/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs @@ -1,5 +1,5 @@ -
{{replace-emoji this.item.fancy_title}}
+
{{replace-emoji this.item.title}}
{{bound-category-link this.item.category diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs index 77f999c9b69..29b3fa73c0a 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs @@ -31,7 +31,7 @@
- {{replace-emoji this.channel.escapedTitle}} + {{replace-emoji this.channel.title}}
{{this.channel.slug}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs index a7c4b39bb4f..9b4d077b090 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs @@ -15,7 +15,7 @@ class="chat-channel-card__name-container" > - {{replace-emoji @channel.escapedTitle}} + {{replace-emoji @channel.title}} {{#if @channel.chatable.read_restricted}} {{d-icon "lock" class="chat-channel-card__read-restricted"}} @@ -47,7 +47,7 @@ {{#if @channel.description}}
- {{replace-emoji @channel.escapedDescription}} + {{replace-emoji @channel.description}}
{{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs index 5fb7fde9883..b25ceb099fd 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs @@ -59,7 +59,7 @@ {{/if}} - {{replace-emoji this.channel.escapedTitle}} + {{replace-emoji this.channel.title}} {{#if (has-block)}} diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 455a5c17ace..b934db3ad81 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -189,6 +189,34 @@ RSpec.describe "Chat channel", type: :system, js: true do end end + context "when replying to message that has tags" do + fab!(:other_user) { Fabricate(:user) } + fab!(:message_2) do + Fabricate( + :chat_message, + user: other_user, + chat_channel: channel_1, + message: "not marked", + ) + end + + before do + Fabricate(:chat_message, user: other_user, chat_channel: channel_1) + Fabricate(:chat_message, in_reply_to: message_2, user: current_user, chat_channel: channel_1) + channel_1.add(other_user) + channel_1.add(current_user) + sign_in(current_user) + end + + it "escapes the reply-to line" do + chat.visit_channel(channel_1) + + expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq( + "<mark>not marked</mark>", + ) + end + end + context "when messages are separated by a day" do before do Fabricate(:chat_message, chat_channel: channel_1, created_at: 2.days.ago)