From bdcbf7429d45f383cc58cda6e49d62c950a783cc Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 10 Oct 2023 19:25:29 +0200 Subject: [PATCH] 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. --- backend/app/rest/api/rest_public_test.go | 16 +++------------- backend/app/store/comment.go | 6 +++--- backend/app/store/comment_test.go | 12 ++++++++++-- backend/app/store/service/service_test.go | 2 +- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 8a0c0da45a..65234398d3 100644 --- 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: "test", - 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, "<a href=\"https://example.com\" rel=\"nofollow\">test</a>", 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 index ad5051634d..ef026a4d6f 100644 --- 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 index 2716d056a5..fe44817f7b 100644 --- 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: `<a href="http://blah.com">username</a>`, Name: "name <b/>"}, + User: User{ID: `<a href="http://blah.com">username</a>`, Name: "name"}, }, }, { @@ -88,6 +88,14 @@ func TestComment_Sanitize(t *testing.T) { inp: Comment{Text: "blah blah", PostTitle: "something"}, out: Comment{Text: "blah blah", PostTitle: "something"}, }, + { + inp: Comment{Text: "blah blah", PostTitle: "test"}, + 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 + out: Comment{Text: "blah blah", PostTitle: "https://example.com/blah"}, + }, { inp: Comment{Text: `

Silicon iMac Concepthttps://t.co/7ga95QxVXn by @marcsheep pic.twitter.com/ULnVpG8w55

— Andreas Storm (@avstorm) November 9, 2020
`, PostTitle: "Twitter quote"}, out: Comment{Text: `

Silicon iMac Concepthttps://t.co/7ga95QxVXn by @marcsheep pic.twitter.com/ULnVpG8w55

— Andreas Storm (@avstorm) November 9, 2020
`, PostTitle: "Twitter quote"}, @@ -263,7 +271,7 @@ func TestComment_sanitizeText(t *testing.T) { }, { "xxx", - "xxx</a>", + "xxx", }, } diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go index 43574da565..74245d9398 100644 --- 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, <a href=\"http://radio-t.com\" rel=\"nofollow\">link</a>", res[0].PostTitle) + assert.Equal(t, "some title, link", res[0].PostTitle) } func TestService_FindSince(t *testing.T) {