remove all HTML tags from comment title and username

Previously, we stripped unsafe HTML tags but left some, but it's not expected to have a link in a title or username, so the new behaviour is stripping everything.
umputun · Oct 10, 2023 · bdcbf74 · bdcbf74
1 parent 41d27e2
commit bdcbf74
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 19 deletions.
diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go
@@ -493,16 +493,10 @@ func TestRest_FindUserComments_CWE_918(t *testing.T) {
 
 	arbitraryURLComment := store.Comment{Text: "arbitrary URL request test",
 		Locator: store.Locator{SiteID: "remark42", URL: arbitraryServer.URL}}
-	aHrefTitleComment := store.Comment{Text: "a href title test", PostTitle: "<a href=\"https://example.com\">test</a>",
-		Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}}
-	urlTitleComment := store.Comment{Text: "url title test", PostTitle: "https://j5pxshabxb5037lms6z182pkjbp4d01p.oastify.com",
-		Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah2"}}
 
 	assert.False(t, backendRequestedArbitraryServer)
 	addComment(t, arbitraryURLComment, ts)
 	assert.True(t, backendRequestedArbitraryServer)
-	addComment(t, aHrefTitleComment, ts)
-	addComment(t, urlTitleComment, ts)
 
 	res, code := get(t, ts.URL+"/api/v1/comments?site=remark42&user=provider1_dev")
 	assert.Equal(t, http.StatusOK, code)
@@ -514,14 +508,10 @@ func TestRest_FindUserComments_CWE_918(t *testing.T) {
 
 	err := json.Unmarshal([]byte(res), &resp)
 	assert.NoError(t, err)
-	require.Equal(t, 3, len(resp.Comments), "should have 2 comments")
+	require.Equal(t, 1, len(resp.Comments), "should have 2 comments")
 
-	assert.Equal(t, "https://j5pxshabxb5037lms6z182pkjbp4d01p.oastify.com", resp.Comments[0].PostTitle, "unsanitised post title")
-	assert.Equal(t, "https://radio-t.com/blah2", resp.Comments[0].Locator.URL)
-	assert.Equal(t, "&lt;a href=\"https://example.com\" rel=\"nofollow\"&gt;test&lt;/a&gt;", resp.Comments[1].PostTitle, "unsanitised post title")
-	assert.Equal(t, "https://radio-t.com/blah1", resp.Comments[1].Locator.URL)
-	assert.Equal(t, "", resp.Comments[2].PostTitle, "empty from the first post")
-	assert.Equal(t, arbitraryServer.URL, resp.Comments[2].Locator.URL, "arbitrary URL provided by the request")
+	assert.Equal(t, "", resp.Comments[0].PostTitle, "empty from the first post")
+	assert.Equal(t, arbitraryServer.URL, resp.Comments[0].Locator.URL, "arbitrary URL provided by the request")
 }
 
 func TestRest_UserInfo(t *testing.T) {

diff --git a/backend/app/store/comment.go b/backend/app/store/comment.go
@@ -182,8 +182,8 @@ func (c *Comment) escapeHTMLWithSome(inp string) string {
 	return res
 }
 
-// SanitizeText used to sanitize any input string
+// SanitizeText used to sanitize any input string, and removes any HTML tags
 func (c *Comment) SanitizeText(inp string) string {
-	clean := bluemonday.UGCPolicy().Sanitize(inp)
-	return c.escapeHTMLWithSome(clean)
+	clean := bluemonday.StrictPolicy().Sanitize(inp)
+	return strings.TrimSpace(c.escapeHTMLWithSome(clean))
 }
diff --git a/backend/app/store/comment_test.go b/backend/app/store/comment_test.go
@@ -22,7 +22,7 @@ func TestComment_Sanitize(t *testing.T) {
 			},
 			out: Comment{
 				Text: "blah XSS\n\t",
-				User: User{ID: `&lt;a href=&#34;http://blah.com&#34;&gt;username&lt;/a&gt;`, Name: "name &lt;b/&gt;"},
+				User: User{ID: `&lt;a href=&#34;http://blah.com&#34;&gt;username&lt;/a&gt;`, Name: "name"},
 			},
 		},
 		{
@@ -88,6 +88,14 @@ func TestComment_Sanitize(t *testing.T) {
 			inp: Comment{Text: "blah blah", PostTitle: "<script>alert()</script>something"},
 			out: Comment{Text: "blah blah", PostTitle: "something"},
 		},
+		{
+			inp: Comment{Text: "blah blah", PostTitle: "<a href=\"https://example.com\">test</a>"},
+			out: Comment{Text: "blah blah", PostTitle: "test"},
+		},
+		{
+			inp: Comment{Text: "blah blah", PostTitle: "https://example.com/blah"}, // link is left as-is, but not rendered as <a>
+			out: Comment{Text: "blah blah", PostTitle: "https://example.com/blah"},
+		},
 		{
 			inp: Comment{Text: `<blockquote class="twitter-tweet"><p lang="es" dir="ltr">Silicon iMac Concept<a href="https://t.co/7ga95QxVXn">https://t.co/7ga95QxVXn</a> by <a href="https://twitter.com/marcsheep?ref_src=twsrc%5Etfw">@marcsheep</a> <a href="https://t.co/ULnVpG8w55">pic.twitter.com/ULnVpG8w55</a></p>&mdash; Andreas Storm (@avstorm) <a href="https://twitter.com/avstorm/status/1325693387798933504?ref_src=twsrc%5Etfw">November 9, 2020</a></blockquote> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>`, PostTitle: "Twitter quote"},
 			out: Comment{Text: `<blockquote class="twitter-tweet"><p lang="es" dir="ltr">Silicon iMac Concept<a href="https://t.co/7ga95QxVXn" rel="nofollow">https://t.co/7ga95QxVXn</a> by <a href="https://twitter.com/marcsheep?ref_src=twsrc%5Etfw" rel="nofollow">@marcsheep</a> <a href="https://t.co/ULnVpG8w55" rel="nofollow">pic.twitter.com/ULnVpG8w55</a></p>— Andreas Storm (@avstorm) <a href="https://twitter.com/avstorm/status/1325693387798933504?ref_src=twsrc%5Etfw" rel="nofollow">November 9, 2020</a></blockquote> `, PostTitle: "Twitter quote"},
@@ -263,7 +271,7 @@ func TestComment_sanitizeText(t *testing.T) {
 		},
 		{
 			"<a href=javascript:alert(document.domain)//>xxx</a>",
-			"xxx&lt;/a&gt;",
+			"xxx",
 		},
 	}
 

diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go
@@ -1156,7 +1156,7 @@ func TestService_Find(t *testing.T) {
 	assert.InDelta(t, 0, res[1].Controversy, 0.01)
 
 	// make sure title sanitized
-	assert.Equal(t, "some title, &lt;a href=\"http://radio-t.com\" rel=\"nofollow\"&gt;link&lt;/a&gt;", res[0].PostTitle)
+	assert.Equal(t, "some title, link", res[0].PostTitle)
 }
 
 func TestService_FindSince(t *testing.T) {