From 1b9ac275785e67148305a731b0cee6e33c7bb007 Mon Sep 17 00:00:00 2001 From: Robert Wolff Date: Mon, 23 Jun 2025 23:00:32 +0200 Subject: [PATCH 1/2] feat(ui): add issue comment assignment doer links --- modules/templates/helper.go | 1 + modules/templates/util_render.go | 28 +++++++++++++------ modules/templates/util_render_test.go | 26 +++++++++++++++++ .../repo/issue/view_content/comments.tmpl | 22 ++++++--------- tests/integration/issue_comment_test.go | 6 ++-- 5 files changed, 58 insertions(+), 25 deletions(-) diff --git a/modules/templates/helper.go b/modules/templates/helper.go index 02b175e6f6..42b4bad83c 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -188,6 +188,7 @@ func NewFuncMap() template.FuncMap { "RenderMarkdownToHtml": RenderMarkdownToHtml, "RenderLabel": RenderLabel, "RenderLabels": RenderLabels, + "RenderUser": RenderUser, "RenderReviewRequest": RenderReviewRequest, // ----------------------------------------------------------------- diff --git a/modules/templates/util_render.go b/modules/templates/util_render.go index a4d7a82eea..48f4eb04a3 100644 --- a/modules/templates/util_render.go +++ b/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( + "%s", + user.HomeLink(), html.EscapeString(user.GetDisplayName()))) + } + return template.HTML(fmt.Sprintf("%s", + 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 := `` diff --git a/modules/templates/util_render_test.go b/modules/templates/util_render_test.go index b75b061218..00543a1b33 100644 --- a/modules/templates/util_render_test.go +++ b/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" @@ -221,3 +224,26 @@ func TestRenderLabels(t *testing.T) { assert.Contains(t, RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", true), "user2/repo1/pulls?labels=1") } + +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), + "user2") + assert.Contains(t, RenderUser(db.DefaultContext, *org), + "org3") + assert.Contains(t, RenderUser(db.DefaultContext, *ghost), + "Ghost") + + defer test.MockVariableValue(&setting.UI.DefaultShowFullName, true)() + assert.Contains(t, RenderUser(db.DefaultContext, *user), + "< U<se>r Tw<o > ><") + assert.Contains(t, RenderUser(db.DefaultContext, *org), + "<<<< >> >> > >> > >>> >>") + assert.Contains(t, RenderUser(db.DefaultContext, *ghost), + "Ghost") +} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index aca77a92e5..53a64b9b41 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -197,27 +197,23 @@ {{else if and (eq .Type 9) (gt .AssigneeID 0)}}
{{svg "octicon-person"}} - {{if .RemovedAssignee}} - {{template "shared/user/avatarlink" dict "user" .Assignee}} - - {{template "shared/user/authorlink" .Assignee}} + {{template "shared/user/avatarlink" dict "user" .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}} - - {{else}} - {{template "shared/user/avatarlink" dict "user" .Assignee}} - - {{template "shared/user/authorlink" .Assignee}} + {{else}} {{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}} - - {{end}} + {{end}} +
{{else if eq .Type 10}}
diff --git a/tests/integration/issue_comment_test.go b/tests/integration/issue_comment_test.go index ee62b418f4..70a60cd402 100644 --- a/tests/integration/issue_comment_test.go +++ b/tests/integration/issue_comment_test.go @@ -205,15 +205,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", "< Ur Tw ><", "/user2", []string{"user2 was assigned by user1"}, - []string{"/user2"}) - // []string{"/user2", "/user1"}) + []string{"/user2", "/user1"}) // Self-remove testIssueCommentChangeEvent(t, htmlDoc, "2043", From 76b3f4cd6ad1a692199eec90db7c8b7985708d9d Mon Sep 17 00:00:00 2001 From: Robert Wolff Date: Fri, 27 Jun 2025 13:27:06 +0200 Subject: [PATCH 2/2] test: prevent XSS for label rendering --- models/fixtures/label.yml | 24 ++++++++++++++++++++++++ modules/templates/util_render_test.go | 27 +++++++++++++++++++++++---- services/convert/issue_test.go | 9 +++++---- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/models/fixtures/label.yml b/models/fixtures/label.yml index acfac74968..84c2a7f418 100644 --- a/models/fixtures/label.yml +++ b/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: " /'?&" + description: "Malicious label ' " + 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 diff --git a/modules/templates/util_render_test.go b/modules/templates/util_render_test.go index 00543a1b33..5974c34073 100644 --- a/modules/templates/util_render_test.go +++ b/modules/templates/util_render_test.go @@ -218,11 +218,30 @@ 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), - "user2/repo1/issues?labels=1") - assert.Contains(t, RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", true), - "user2/repo1/pulls?labels=1") + rendered := RenderLabels(db.DefaultContext, tr, []*issues_model.Label{label}, "user2/repo1", false) + assert.Contains(t, rendered, "user2/repo1/issues?labels=1") + assert.Contains(t, rendered, ">label1<") + 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, "> <script>malicious</script> <") + assert.Contains(t, rendered, ">'?&<") + assert.Contains(t, rendered, "title='Malicious label ' <script>malicious</script>'") + 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<><") + assert.Contains(t, rendered, "title='repo.issues.archived_label_description'") } func TestRenderUser(t *testing.T) { diff --git a/services/convert/issue_test.go b/services/convert/issue_test.go index 97bacfb229..ea8ad9b7ef 100644 --- a/services/convert/issue_test.go +++ b/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, - Name: label.Name, - Color: "abcdef", - URL: fmt.Sprintf("%sapi/v1/repos/user2/repo1/labels/%d", setting.AppURL, label.ID), + ID: label.ID, + Name: label.Name, + Color: "abcdef", + Description: label.Description, + URL: fmt.Sprintf("%sapi/v1/repos/user2/repo1/labels/%d", setting.AppURL, label.ID), }, ToLabel(label, repo, nil)) }