From 109d2669ab1e87cd2e56e568391d3b2d7dfd0a97 Mon Sep 17 00:00:00 2001
From: Meisam <39205857+MFTabriz@users.noreply.github.com>
Date: Wed, 28 Jul 2021 00:26:27 +0200
Subject: [PATCH] better chat message sanitization (#1266)

* strip <p> in chat sanitization, keep the content

* update sanitization tests

* update tests

* rm <p></p> comparison for empty messages
---
 core/chat/events/events.go            |  6 ++++--
 core/chat/messageRendering_test.go    | 12 ++++++------
 test/automated/chat.test.js           |  2 +-
 test/automated/chatmoderation.test.js |  2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/core/chat/events/events.go b/core/chat/events/events.go
index 44d84d575..bda8b21d2 100644
--- a/core/chat/events/events.go
+++ b/core/chat/events/events.go
@@ -73,7 +73,7 @@ func (m *MessageEvent) RenderAndSanitizeMessageBody() {
 
 // Empty will return if this message's contents is empty.
 func (m *MessageEvent) Empty() bool {
-	return m.Body == "" || m.Body == "<p></p>"
+	return m.Body == ""
 }
 
 // RenderBody will render markdown to html without any sanitization.
@@ -136,7 +136,9 @@ func sanitize(raw string) string {
 	p.AddTargetBlankToFullyQualifiedLinks(true)
 
 	// Allow breaks
-	p.AllowElements("br", "p")
+	p.AllowElements("br")
+
+	p.AllowElementsContent("p")
 
 	// Allow img tags from the the local emoji directory only
 	p.AllowAttrs("src").Matching(regexp.MustCompile(`(?i)^/img/emoji`)).OnElements("img")
diff --git a/core/chat/messageRendering_test.go b/core/chat/messageRendering_test.go
index b0e0c2e4a..effa9777a 100644
--- a/core/chat/messageRendering_test.go
+++ b/core/chat/messageRendering_test.go
@@ -19,11 +19,11 @@ func TestRenderAndSanitize(t *testing.T) {
   <script src="http://hackers.org/hack.js"></script>
   `
 
-	expected := `<p>Test one two three!  I go to <a href="http://yahoo.com" rel="nofollow noreferrer noopener" target="_blank">http://yahoo.com</a> and search for <em>sports</em> and <strong>answers</strong>.
-Here is an iframe </p>
+	expected := `Test one two three!  I go to <a href="http://yahoo.com" rel="nofollow noreferrer noopener" target="_blank">http://yahoo.com</a> and search for <em>sports</em> and <strong>answers</strong>.
+Here is an iframe 
 blah blah blah
-<p><a href="http://owncast.online" rel="nofollow noreferrer noopener" target="_blank">test link</a>
-<img class="emoji" src="/img/emoji/bananadance.gif"></p>`
+<a href="http://owncast.online" rel="nofollow noreferrer noopener" target="_blank">test link</a>
+<img class="emoji" src="/img/emoji/bananadance.gif">`
 
 	result := events.RenderAndSanitize(messageContent)
 	if result != expected {
@@ -34,7 +34,7 @@ blah blah blah
 // Test to make sure we block remote images in chat messages.
 func TestBlockRemoteImages(t *testing.T) {
 	messageContent := `<img src="https://via.placeholder.com/img/emoji/350x150"> test ![](https://via.placeholder.com/img/emoji/350x150)`
-	expected := `<p> test </p>`
+	expected := `test`
 	result := events.RenderAndSanitize(messageContent)
 
 	if result != expected {
@@ -45,7 +45,7 @@ func TestBlockRemoteImages(t *testing.T) {
 // Test to make sure emoji images are allowed in chat messages.
 func TestAllowEmojiImages(t *testing.T) {
 	messageContent := `<img alt=":beerparrot:" title=":beerparrot:" src="/img/emoji/beerparrot.gif"> test ![](/img/emoji/beerparrot.gif)`
-	expected := `<p><img alt=":beerparrot:" title=":beerparrot:" src="/img/emoji/beerparrot.gif"> test <img src="/img/emoji/beerparrot.gif"></p>`
+	expected := `<img alt=":beerparrot:" title=":beerparrot:" src="/img/emoji/beerparrot.gif"> test <img src="/img/emoji/beerparrot.gif">`
 	result := events.RenderAndSanitize(messageContent)
 
 	if result != expected {
diff --git a/test/automated/chat.test.js b/test/automated/chat.test.js
index caf99774f..818e1366e 100644
--- a/test/automated/chat.test.js
+++ b/test/automated/chat.test.js
@@ -27,7 +27,7 @@ test('can fetch chat messages', async (done) => {
     .auth('admin', 'abc123')
     .expect(200);
 
-    const expectedBody = `<p>${testMessage.body}</p>`
+    const expectedBody = `${testMessage.body}`
   const message = res.body.filter(function (msg) {
     return msg.body ===  expectedBody
   })[0];
diff --git a/test/automated/chatmoderation.test.js b/test/automated/chatmoderation.test.js
index feae1fbd0..f50fc9727 100644
--- a/test/automated/chatmoderation.test.js
+++ b/test/automated/chatmoderation.test.js
@@ -34,7 +34,7 @@ test('verify message has become hidden', async (done) => {
         .auth('admin', 'abc123')
 
     const message = res.body.filter(obj => {
-        return obj.body === `<p>${testVisibilityMessage.body}</p>`;
+        return obj.body === `${testVisibilityMessage.body}`;
     });
     expect(message.length).toBe(1);
     expect(message[0].hiddenAt).toBeTruthy();