feat(ui): add links to assigners in issue comments (#8264)

When a user is assigning an issue or PR to another user, an entry is posted in the issue comment list. This PR adds a link to the one assigning the issue. Additionally, tests for preventing XSS vulnerabilities in label rendering were added.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8264
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

models/fixtures/label.yml

@@ -3,6 +3,7 @@
   repo_id: 1
   org_id: 0
   name: label1
+  description: 'First label'
   color: '#abcdef'
   exclusive: false
   num_issues: 2
@@ -107,3 +108,26 @@
   num_issues: 0
   num_closed_issues: 0
   archived_unix: 0
+
+-
+  id: 11
+  repo_id: 3
+  org_id: 0
+  name: "  <script>malicious</script> /'?&"
+  description: "Malicious label ' <script>malicious</script>"
+  color: '#000000'
+  exclusive: true
+  num_issues: 0
+  num_closed_issues: 0
+  archived_unix: 0
+
+-
+  id: 12
+  repo_id: 3
+  org_id: 0
+  name: 'archived label<>'
+  color: '#000000'
+  exclusive: false
+  num_issues: 0
+  num_closed_issues: 0
+  archived_unix: 2991092130

modules/templates/helper.go

@@ -188,6 +188,7 @@ func NewFuncMap() template.FuncMap {
 		"RenderMarkdownToHtml": RenderMarkdownToHtml,
 		"RenderLabel":          RenderLabel,
 		"RenderLabels":         RenderLabels,
+		"RenderUser":           RenderUser,
 		"RenderReviewRequest":  RenderReviewRequest,
 
 		// -----------------------------------------------------------------

modules/templates/util_render.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/hex"
 	"fmt"
+	"html"
 	"html/template"
 	"math"
 	"net/url"
@@ -15,6 +16,7 @@ import (
 	"unicode"
 
 	issues_model "forgejo.org/models/issues"
+	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/emoji"
 	"forgejo.org/modules/log"
 	"forgejo.org/modules/markup"
@@ -26,7 +28,7 @@ import (
 
 // RenderCommitMessage renders commit message with XSS-safe and special links.
 func RenderCommitMessage(ctx context.Context, msg string, metas map[string]string) template.HTML {
-	cleanMsg := template.HTMLEscapeString(msg)
+	cleanMsg := html.EscapeString(msg)
 	// we can safely assume that it will not return any error, since there
 	// shouldn't be any special HTML.
 	fullMessage, err := markup.RenderCommitMessage(&markup.RenderContext{
@@ -63,7 +65,7 @@ func RenderCommitMessageLinkSubject(ctx context.Context, msg, urlDefault string,
 		Ctx:         ctx,
 		DefaultLink: urlDefault,
 		Metas:       metas,
-	}, template.HTMLEscapeString(msgLine))
+	}, html.EscapeString(msgLine))
 	if err != nil {
 		log.Error("RenderCommitMessageSubject: %v", err)
 		return template.HTML("")
@@ -88,7 +90,7 @@ func RenderCommitBody(ctx context.Context, msg string, metas map[string]string)
 	renderedMessage, err := markup.RenderCommitMessage(&markup.RenderContext{
 		Ctx:   ctx,
 		Metas: metas,
-	}, template.HTMLEscapeString(msgLine))
+	}, html.EscapeString(msgLine))
 	if err != nil {
 		log.Error("RenderCommitMessage: %v", err)
 		return ""
@@ -122,7 +124,7 @@ func RenderIssueTitle(ctx context.Context, text string, metas map[string]string)
 	renderedText, err := markup.RenderIssueTitle(&markup.RenderContext{
 		Ctx:   ctx,
 		Metas: metas,
-	}, template.HTMLEscapeString(text))
+	}, html.EscapeString(text))
 	if err != nil {
 		log.Error("RenderIssueTitle: %v", err)
 		return template.HTML("")
@@ -132,7 +134,7 @@ func RenderIssueTitle(ctx context.Context, text string, metas map[string]string)
 
 // RenderRefIssueTitle renders referenced issue/pull title with defined post processors
 func RenderRefIssueTitle(ctx context.Context, text string) template.HTML {
-	renderedText, err := markup.RenderRefIssueTitle(&markup.RenderContext{Ctx: ctx}, template.HTMLEscapeString(text))
+	renderedText, err := markup.RenderRefIssueTitle(&markup.RenderContext{Ctx: ctx}, html.EscapeString(text))
 	if err != nil {
 		log.Error("RenderRefIssueTitle: %v", err)
 		return ""
@@ -150,7 +152,7 @@ func RenderLabel(ctx context.Context, locale translation.Locale, label *issues_m
 		labelScope       = label.ExclusiveScope()
 	)
 
-	description := emoji.ReplaceAliases(template.HTMLEscapeString(label.Description))
+	description := emoji.ReplaceAliases(html.EscapeString(label.Description))
 
 	if label.IsArchived() {
 		archivedCSSClass = "archived-label"
@@ -212,7 +214,7 @@ func RenderLabel(ctx context.Context, locale translation.Locale, label *issues_m
 // RenderEmoji renders html text with emoji post processors
 func RenderEmoji(ctx context.Context, text string) template.HTML {
 	renderedText, err := markup.RenderEmoji(&markup.RenderContext{Ctx: ctx},
-		template.HTMLEscapeString(text))
+		html.EscapeString(text))
 	if err != nil {
 		log.Error("RenderEmoji: %v", err)
 		return template.HTML("")
@@ -263,10 +265,20 @@ func RenderLabels(ctx context.Context, locale translation.Locale, labels []*issu
 	return template.HTML(htmlCode)
 }
 
+func RenderUser(ctx context.Context, user user_model.User) template.HTML {
+	if user.ID > 0 {
+		return template.HTML(fmt.Sprintf(
+			"<a href='%s' rel='nofollow'><strong>%s</strong></a>",
+			user.HomeLink(), html.EscapeString(user.GetDisplayName())))
+	}
+	return template.HTML(fmt.Sprintf("<strong>%s</strong>",
+		html.EscapeString(user.GetDisplayName())))
+}
+
 func RenderReviewRequest(users []issues_model.RequestReviewTarget) template.HTML {
 	usernames := make([]string, 0, len(users))
 	for _, user := range users {
-		usernames = append(usernames, template.HTMLEscapeString(user.Name()))
+		usernames = append(usernames, html.EscapeString(user.Name()))
 	}
 
 	htmlCode := `<span class="review-request-list">`

modules/templates/util_render_test.go

@@ -11,6 +11,9 @@ import (
 	"forgejo.org/models/db"
 	issues_model "forgejo.org/models/issues"
 	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
+	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
 	"forgejo.org/modules/translation"
 
 	"github.com/stretchr/testify/assert"
@@ -215,9 +218,51 @@ func TestRenderLabels(t *testing.T) {
 
 	tr := &translation.MockLocale{}
 	label := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1})
+	labelScoped := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 7})
+	labelMalicious := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 11})
+	labelArchived := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 12})
 
-	assert.Contains(t, RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", false),
+	rendered := RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", false)
-		"user2/repo1/issues?labels=1")
+	assert.Contains(t, rendered, "user2/repo1/issues?labels=1")
-	assert.Contains(t, RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", true),
+	assert.Contains(t, rendered, ">label1<")
-		"user2/repo1/pulls?labels=1")
+	assert.Contains(t, rendered, "title='First label'")
+	rendered = RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", true)
+	assert.Contains(t, rendered, "user2/repo1/pulls?labels=1")
+	assert.Contains(t, rendered, ">label1<")
+	rendered = RenderLabels(db.DefaultContext, tr, []*issues_model.Label{labelScoped}, "user2/repo1", false)
+	assert.Contains(t, rendered, "user2/repo1/issues?labels=7")
+	assert.Contains(t, rendered, ">scope<")
+	assert.Contains(t, rendered, ">label1<")
+	rendered = RenderLabels(db.DefaultContext, tr, []*issues_model.Label{labelMalicious}, "user2/repo1", false)
+	assert.Contains(t, rendered, "user2/repo1/issues?labels=11")
+	assert.Contains(t, rendered, ">  &lt;script&gt;malicious&lt;/script&gt; <")
+	assert.Contains(t, rendered, ">&#39;?&amp;<")
+	assert.Contains(t, rendered, "title='Malicious label &#39; &lt;script&gt;malicious&lt;/script&gt;'")
+	rendered = RenderLabels(db.DefaultContext, tr, []*issues_model.Label{labelArchived}, "user2/repo1", false)
+	assert.Contains(t, rendered, "user2/repo1/issues?labels=12")
+	assert.Contains(t, rendered, ">archived label&lt;&gt;<")
+	assert.Contains(t, rendered, "title='repo.issues.archived_label_description'")
+}
+
+func TestRenderUser(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3})
+	ghost := user_model.NewGhostUser()
+
+	assert.Contains(t, RenderUser(db.DefaultContext, *user),
+		"<a href='/user2' rel='nofollow'><strong>user2</strong></a>")
+	assert.Contains(t, RenderUser(db.DefaultContext, *org),
+		"<a href='/org3' rel='nofollow'><strong>org3</strong></a>")
+	assert.Contains(t, RenderUser(db.DefaultContext, *ghost),
+		"<strong>Ghost</strong>")
+
+	defer test.MockVariableValue(&setting.UI.DefaultShowFullName, true)()
+	assert.Contains(t, RenderUser(db.DefaultContext, *user),
+		"<a href='/user2' rel='nofollow'><strong>&lt; U&lt;se&gt;r Tw&lt;o &gt; &gt;&lt;</strong></a>")
+	assert.Contains(t, RenderUser(db.DefaultContext, *org),
+		"<a href='/org3' rel='nofollow'><strong>&lt;&lt;&lt;&lt; &gt;&gt; &gt;&gt; &gt; &gt;&gt; &gt; &gt;&gt;&gt; &gt;&gt;</strong></a>")
+	assert.Contains(t, RenderUser(db.DefaultContext, *ghost),
+		"<strong>Ghost</strong>")
 }

services/convert/issue_test.go

@@ -24,10 +24,11 @@ func TestLabel_ToLabel(t *testing.T) {
 	label := unittest.AssertExistsAndLoadBean(t, &issues_model.Label{ID: 1})
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: label.RepoID})
 	assert.Equal(t, &api.Label{
-		ID:    label.ID,
+		ID:          label.ID,
-		Name:  label.Name,
+		Name:        label.Name,
-		Color: "abcdef",
+		Color:       "abcdef",
-		URL:   fmt.Sprintf("%sapi/v1/repos/user2/repo1/labels/%d", setting.AppURL, label.ID),
+		Description: label.Description,
+		URL:         fmt.Sprintf("%sapi/v1/repos/user2/repo1/labels/%d", setting.AppURL, label.ID),
 	}, ToLabel(label, repo, nil))
 }
 

templates/repo/issue/view_content/comments.tmpl

@@ -221,27 +221,23 @@
 		{{else if and (eq .Type 9) (gt .AssigneeID 0)}}
 			<div class="timeline-item event" id="{{.HashTag}}">
 				<span class="badge">{{svg "octicon-person"}}</span>
-				{{if .RemovedAssignee}}
+				{{template "shared/user/avatarlink" dict "user" .Assignee}}
-					{{template "shared/user/avatarlink" dict "user" .Assignee}}
+				<span class="text grey muted-links">
-					<span class="text grey muted-links">
+					{{template "shared/user/authorlink" .Assignee}}
-						{{template "shared/user/authorlink" .Assignee}}
+					{{if .RemovedAssignee}}
 						{{if eq .Poster.ID .Assignee.ID}}
 							{{ctx.Locale.Tr "repo.issues.remove_self_assignment" $createdStr}}
 						{{else}}
-							{{ctx.Locale.Tr "repo.issues.remove_assignee_at" .Poster.GetDisplayName $createdStr}}
+							{{ctx.Locale.Tr "repo.issues.remove_assignee_at" (RenderUser $.Context .Poster) $createdStr}}
 						{{end}}
-					</span>
+					{{else}}
-				{{else}}
-					{{template "shared/user/avatarlink" dict "user" .Assignee}}
-					<span class="text grey muted-links">
-						{{template "shared/user/authorlink" .Assignee}}
 						{{if eq .Poster.ID .AssigneeID}}
 							{{ctx.Locale.Tr "repo.issues.self_assign_at" $createdStr}}
 						{{else}}
-							{{ctx.Locale.Tr "repo.issues.add_assignee_at" .Poster.GetDisplayName $createdStr}}
+							{{ctx.Locale.Tr "repo.issues.add_assignee_at" (RenderUser $.Context .Poster) $createdStr}}
 						{{end}}
-					</span>
+					{{end}}
-				{{end}}
+				</span>
 			</div>
 		{{else if eq .Type 10}}
 			<div class="timeline-item event" id="{{.HashTag}}">

tests/integration/issue_comment_test.go

@@ -223,15 +223,13 @@ func TestIssueCommentChangeAssignee(t *testing.T) {
 	testIssueCommentChangeEvent(t, htmlDoc, "2041",
 		"octicon-person", "User One", "/user1",
 		[]string{"user1 was unassigned by user2"},
-		[]string{"/user1"})
+		[]string{"/user1", "/user2"})
-	// []string{"/user1", "/user2"})
 
 	// Add other
 	testIssueCommentChangeEvent(t, htmlDoc, "2042",
 		"octicon-person", "< U<se>r Tw<o > ><", "/user2",
 		[]string{"user2 was assigned by user1"},
-		[]string{"/user2"})
+		[]string{"/user2", "/user1"})
-	// []string{"/user2", "/user1"})
 
 	// Self-remove
 	testIssueCommentChangeEvent(t, htmlDoc, "2043",