Refactor flash message and remove SanitizeHTML template func

models/issues/comment.go

@@ -24,6 +24,7 @@ import (
 	"code.gitea.io/gitea/modules/htmlutil"
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/markup"
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/references"
 	"code.gitea.io/gitea/modules/structs"
@@ -543,6 +544,12 @@ func (c *Comment) EventTag() string {
 	return fmt.Sprintf("event-%d", c.ID)
 }
 
+func (c *Comment) GetSanitizedContentHTML() template.HTML {
+	// mainly for type=4 CommentTypeCommitRef
+	// the content is a link like <a href="{RepoLink}/commit/{CommitID}">message title</a> (from CreateRefComment)
+	return markup.Sanitize(c.Content)
+}
+
 // LoadLabel if comment.Type is CommentTypeLabel, then load Label
 func (c *Comment) LoadLabel(ctx context.Context) error {
 	var label Label

modules/markup/html.go

@@ -6,6 +6,7 @@ package markup
 import (
 	"bytes"
 	"fmt"
+	"html/template"
 	"io"
 	"regexp"
 	"slices"
@@ -149,9 +150,9 @@ func PostProcessDefault(ctx *RenderContext, input io.Reader, output io.Writer) e
 	return postProcess(ctx, procs, input, output)
 }
 
-// PostProcessCommitMessage will use the same logic as PostProcess, but will disable
-// the shortLinkProcessor.
-func PostProcessCommitMessage(ctx *RenderContext, content string) (string, error) {
+// PostProcessCommitMessage will use the same logic as PostProcess, but will disable the shortLinkProcessor.
+// FIXME: this function and its family have a very strange design: it takes HTML as input and output, processes the "escaped" content.
+func PostProcessCommitMessage(ctx *RenderContext, content template.HTML) (template.HTML, error) {
 	procs := []processor{
 		fullIssuePatternProcessor,
 		comparePatternProcessor,
@@ -165,7 +166,8 @@ func PostProcessCommitMessage(ctx *RenderContext, content string) (string, error
 		emojiProcessor,
 		emojiShortCodeProcessor,
 	}
-	return postProcessString(ctx, procs, content)
+	s, err := postProcessString(ctx, procs, string(content))
+	return template.HTML(s), err
 }
 
 var emojiProcessors = []processor{

modules/templates/helper.go

@@ -32,13 +32,12 @@ func newFuncMapWebPage() template.FuncMap {
 
 		// -----------------------------------------------------------------
 		// html/template related functions
-		"dict":         dict, // it's lowercase because this name has been widely used. Our other functions should have uppercase names.
-		"Iif":          iif,
-		"Eval":         evalTokens,
-		"HTMLFormat":   htmlFormat,
-		"QueryEscape":  queryEscape,
-		"QueryBuild":   QueryBuild,
-		"SanitizeHTML": SanitizeHTML,
+		"dict":        dict, // it's lowercase because this name has been widely used. Our other functions should have uppercase names.
+		"Iif":         iif,
+		"Eval":        evalTokens,
+		"HTMLFormat":  htmlFormat,
+		"QueryEscape": queryEscape,
+		"QueryBuild":  QueryBuild,
 
 		"PathEscape":         url.PathEscape,
 		"PathEscapeSegments": util.PathEscapeSegments,
@@ -146,9 +145,8 @@ func newFuncMapWebPage() template.FuncMap {
 	}
 }
 
-// SanitizeHTML sanitizes the input by default sanitization rules.
-func SanitizeHTML(s string) template.HTML {
-	return markup.Sanitize(s)
+func sanitizeHTML(msg string) template.HTML {
+	return markup.Sanitize(msg)
 }
 
 func htmlFormat(s any, args ...any) template.HTML {

modules/templates/helper_test.go

@@ -58,7 +58,7 @@ func TestSubjectBodySeparator(t *testing.T) {
 }
 
 func TestSanitizeHTML(t *testing.T) {
-	assert.Equal(t, template.HTML(`<a href="/" rel="nofollow">link</a> xss <div>inline</div>`), SanitizeHTML(`<a href="/">link</a> <a href="javascript:">xss</a> <div style="dangerous">inline</div>`))
+	assert.Equal(t, template.HTML(`<a href="/" rel="nofollow">link</a> xss <div>inline</div>`), sanitizeHTML(`<a href="/">link</a> <a href="javascript:">xss</a> <div style="dangerous">inline</div>`))
 }
 
 func TestTemplateIif(t *testing.T) {

modules/templates/mail.go

@@ -65,13 +65,16 @@ func mailBodyFuncMap() template.FuncMap {
 		"NIL":     func() any { return nil },
 
 		// html/template related functions
-		"dict":         dict,
-		"Iif":          iif,
-		"Eval":         evalTokens,
-		"HTMLFormat":   htmlFormat,
-		"QueryEscape":  queryEscape,
-		"QueryBuild":   QueryBuild,
-		"SanitizeHTML": SanitizeHTML,
+		"dict":        dict,
+		"Iif":         iif,
+		"Eval":        evalTokens,
+		"HTMLFormat":  htmlFormat,
+		"QueryEscape": queryEscape,
+		"QueryBuild":  QueryBuild,
+
+		// deprecated, use "HTMLFormat" instead, but some user custom mail templates still use it
+		// see: https://github.com/go-gitea/gitea/issues/36049
+		"SanitizeHTML": sanitizeHTML,
 
 		"PathEscape":         url.PathEscape,
 		"PathEscapeSegments": util.PathEscapeSegments,

modules/templates/util_render.go

@@ -40,15 +40,15 @@ func NewRenderUtils(ctx reqctx.RequestContext) *RenderUtils {
 
 // RenderCommitMessage renders commit message with XSS-safe and special links.
 func (ut *RenderUtils) RenderCommitMessage(msg string, repo *repo.Repository) template.HTML {
-	cleanMsg := template.HTMLEscapeString(msg)
+	cleanMsg := template.HTML(template.HTMLEscapeString(msg))
 	// we can safely assume that it will not return any error, since there shouldn't be any special HTML.
 	// "repo" can be nil when rendering commit messages for deleted repositories in a user's dashboard feed.
 	fullMessage, err := markup.PostProcessCommitMessage(renderhelper.NewRenderContextRepoComment(ut.ctx, repo), cleanMsg)
 	if err != nil {
 		log.Error("PostProcessCommitMessage: %v", err)
 		return ""
 	}
-	msgLines := strings.Split(strings.TrimSpace(fullMessage), "\n")
+	msgLines := strings.Split(strings.TrimSpace(string(fullMessage)), "\n")
 	if len(msgLines) == 0 {
 		return ""
 	}
@@ -91,12 +91,14 @@ func (ut *RenderUtils) RenderCommitBody(msg string, repo *repo.Repository) templ
 		return ""
 	}
 
-	renderedMessage, err := markup.PostProcessCommitMessage(renderhelper.NewRenderContextRepoComment(ut.ctx, repo), template.HTMLEscapeString(msgLine))
+	rctx := renderhelper.NewRenderContextRepoComment(ut.ctx, repo)
+	htmlContent := template.HTML(template.HTMLEscapeString(msgLine))
+	renderedMessage, err := markup.PostProcessCommitMessage(rctx, htmlContent)
 	if err != nil {
 		log.Error("PostProcessCommitMessage: %v", err)
 		return ""
 	}
-	return template.HTML(renderedMessage)
+	return renderedMessage
 }
 
 // Match text that is between back ticks.
@@ -279,6 +281,35 @@ func (ut *RenderUtils) RenderThemeItem(info *webtheme.ThemeMetaInfo, iconSize in
 	return htmlutil.HTMLFormat(`<div class="theme-menu-item" data-tooltip-content="%s">%s %s %s</div>`, info.GetDescription(), icon, info.DisplayName, extraIcon)
 }
 
+func (ut *RenderUtils) RenderFlashMessage(typ, msg string) template.HTML {
+	msg = strings.TrimSpace(msg)
+	if msg == "" {
+		return ""
+	}
+
+	cls := typ
+	// legacy logic: "negative" for error, "positive" for success
+	switch cls {
+	case "error":
+		cls = "negative"
+	case "success":
+		cls = "positive"
+	}
+
+	var msgContent template.HTML
+	if strings.Contains(msg, "</pre>") || strings.Contains(msg, "</details>") || strings.Contains(msg, "</ul>") || strings.Contains(msg, "</div>") {
+		// If the message contains some known "block" elements, no need to do more alignment or line-break processing, just sanitize it directly.
+		msgContent = sanitizeHTML(msg)
+	} else if !strings.Contains(msg, "\n") {
+		// If the message is a single line, center-align it by wrapping it
+		msgContent = htmlutil.HTMLFormat(`<div class="tw-text-center">%s</div>`, sanitizeHTML(msg))
+	} else {
+		// For a multi-line message, preserve line breaks, and left-align it.
+		msgContent = htmlutil.HTMLFormat(`%s`, sanitizeHTML(strings.ReplaceAll(msg, "\n", "<br>")))
+	}
+	return htmlutil.HTMLFormat(`<div class="ui %s message flash-message flash-%s">%s</div>`, cls, typ, msgContent)
+}
+
 func (ut *RenderUtils) RenderUnicodeEscapeToggleButton(escapeStatus *charset.EscapeStatus) template.HTML {
 	if escapeStatus == nil || !escapeStatus.Escaped {
 		return ""

routers/utils/utils.go

@@ -5,10 +5,11 @@ package utils
 
 import (
 	"html"
-	"strings"
+	"html/template"
 )
 
-// SanitizeFlashErrorString will sanitize a flash error string
-func SanitizeFlashErrorString(x string) string {
-	return strings.ReplaceAll(html.EscapeString(x), "\n", "<br>")
+// EscapeFlashErrorString will escape the flash error string
+// Maybe do more sanitization in the future, e.g.: hide sensitive information, etc.
+func EscapeFlashErrorString(x string) template.HTML {
+	return template.HTML(html.EscapeString(x))
 }

routers/utils/utils_test.go

@@ -4,16 +4,17 @@
 package utils
 
 import (
+	"html/template"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestSanitizeFlashErrorString(t *testing.T) {
+func TestEscapeFlashErrorString(t *testing.T) {
 	tests := []struct {
 		name string
 		arg  string
-		want string
+		want template.HTML
 	}{
 		{
 			name: "no error",
@@ -28,13 +29,13 @@ func TestSanitizeFlashErrorString(t *testing.T) {
 		{
 			name: "line break error",
 			arg:  "some error:\n\nawesome!",
-			want: "some error:<br><br>awesome!",
+			want: "some error:\n\nawesome!",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := SanitizeFlashErrorString(tt.arg)
+			got := EscapeFlashErrorString(tt.arg)
 			assert.Equal(t, tt.want, got)
 		})
 	}

routers/web/auth/oauth.go

@@ -79,7 +79,7 @@ func SignInOAuthCallback(ctx *context.Context) {
 			}
 		}
 		sort.Strings(errorKeyValues)
-		ctx.Flash.Error(strings.Join(errorKeyValues, "<br>"), true)
+		ctx.Flash.Error(strings.Join(errorKeyValues, "\n"), true)
 	}
 
 	// first look if the provider is still active

routers/web/devtest/devtest.go

@@ -45,8 +45,8 @@ func List(ctx *context.Context) {
 
 func FetchActionTest(ctx *context.Context) {
 	_ = ctx.Req.ParseForm()
-	ctx.Flash.Info("fetch-action: " + ctx.Req.Method + " " + ctx.Req.RequestURI + "<br>" +
-		"Form: " + ctx.Req.Form.Encode() + "<br>" +
+	ctx.Flash.Info("fetch-action: " + ctx.Req.Method + " " + ctx.Req.RequestURI + "\n" +
+		"Form: " + ctx.Req.Form.Encode() + "\n" +
 		"PostForm: " + ctx.Req.PostForm.Encode(),
 	)
 	time.Sleep(2 * time.Second)
@@ -192,11 +192,31 @@ func prepareMockData(ctx *context.Context) {
 		prepareMockDataBadgeActionsSvg(ctx)
 	case "/devtest/relative-time":
 		prepareMockDataRelativeTime(ctx)
+	case "/devtest/toast-and-message":
+		prepareMockDataToastAndMessage(ctx)
 	case "/devtest/unicode-escape":
 		prepareMockDataUnicodeEscape(ctx)
 	}
 }
 
+func prepareMockDataToastAndMessage(ctx *context.Context) {
+	msgWithDetails, _ := ctx.RenderToHTML("base/alert_details", map[string]any{
+		"Message": "message with details <script>escape xss</script>",
+		"Summary": "summary with details",
+		"Details": "details line 1\n details line 2\n details line 3",
+	})
+	msgWithSummary, _ := ctx.RenderToHTML("base/alert_details", map[string]any{
+		"Message": "message with summary <script>escape xss</script>",
+		"Summary": "summary only",
+	})
+
+	ctx.Flash.ErrorMsg = string(msgWithDetails)
+	ctx.Flash.WarningMsg = string(msgWithSummary)
+	ctx.Flash.InfoMsg = "a long message with line break\nthe second line <script>removed xss</script>"
+	ctx.Flash.SuccessMsg = "single line message <script>removed xss</script>"
+	ctx.Data["Flash"] = ctx.Flash
+}
+
 func prepareMockDataUnicodeEscape(ctx *context.Context) {
 	content := "// demo code\n"
 	content += "if accessLevel != \"user\u202E \u2066// Check if admin (invisible char)\u2069 \u2066\" { }\n"
@@ -223,8 +243,8 @@ func TmplCommon(ctx *context.Context) {
 	prepareMockData(ctx)
 	if ctx.Req.Method == http.MethodPost {
 		_ = ctx.Req.ParseForm()
-		ctx.Flash.Info("form: "+ctx.Req.Method+" "+ctx.Req.RequestURI+"<br>"+
-			"Form: "+ctx.Req.Form.Encode()+"<br>"+
+		ctx.Flash.Info("form: "+ctx.Req.Method+" "+ctx.Req.RequestURI+"\n"+
+			"Form: "+ctx.Req.Form.Encode()+"\n"+
 			"PostForm: "+ctx.Req.PostForm.Encode(),
 			true,
 		)

routers/web/feed/convert.go

@@ -15,6 +15,7 @@ import (
 	activities_model "code.gitea.io/gitea/models/activities"
 	"code.gitea.io/gitea/models/renderhelper"
 	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/modules/markup"
 	"code.gitea.io/gitea/modules/markup/markdown"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/templates"
@@ -237,7 +238,7 @@ func feedActionsToFeedItems(ctx *context.Context, actions activities_model.Actio
 			}
 		}
 		if len(content) == 0 {
-			content = templates.SanitizeHTML(desc)
+			content = markup.Sanitize(desc)
 		}
 
 		items = append(items, &feeds.Item{

routers/web/repo/branch.go

@@ -231,7 +231,7 @@ func CreateBranch(ctx *context.Context) {
 				flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 					"Message": ctx.Tr("repo.editor.push_rejected"),
 					"Summary": ctx.Tr("repo.editor.push_rejected_summary"),
-					"Details": utils.SanitizeFlashErrorString(e.Message),
+					"Details": utils.EscapeFlashErrorString(e.Message),
 				})
 				if err != nil {
 					ctx.ServerError("UpdatePullRequest.HTMLString", err)

routers/web/repo/commit.go

@@ -410,7 +410,8 @@ func Diff(ctx *context.Context) {
 		ctx.Data["NoteCommit"] = note.Commit
 		ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit)
 		rctx := renderhelper.NewRenderContextRepoComment(ctx, ctx.Repo.Repository, renderhelper.RepoCommentOptions{CurrentRefPath: path.Join("commit", util.PathEscapeSegments(commitID))})
-		ctx.Data["NoteRendered"], err = markup.PostProcessCommitMessage(rctx, template.HTMLEscapeString(string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{}))))
+		htmlMessage := template.HTML(template.HTMLEscapeString(string(charset.ToUTF8WithFallback(note.Message, charset.ConvertOpts{}))))
+		ctx.Data["NoteRendered"], err = markup.PostProcessCommitMessage(rctx, htmlMessage)
 		if err != nil {
 			ctx.ServerError("PostProcessCommitMessage", err)
 			return

routers/web/repo/editor_error.go

@@ -27,13 +27,13 @@ func editorHandleFileOperationErrorRender(ctx *context_service.Context, message,
 	flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 		"Message": message,
 		"Summary": summary,
-		"Details": utils.SanitizeFlashErrorString(details),
+		"Details": utils.EscapeFlashErrorString(details),
 	})
 	if err == nil {
 		ctx.JSONError(flashError)
 	} else {
-		log.Error("RenderToHTML: %v", err)
-		ctx.JSONError(message + "\n" + summary + "\n" + utils.SanitizeFlashErrorString(details))
+		log.Error("RenderToHTML(%q, %q, %q), error: %v", message, summary, details, err)
+		ctx.JSONError("Unable to render error details, see server logs") // it should never happen
 	}
 }
 

routers/web/repo/issue_new.go

@@ -170,7 +170,7 @@ func renderErrorOfTemplates(ctx *context.Context, errs map[string]error) templat
 	flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 		"Message": ctx.Tr("repo.issues.choose.ignore_invalid_templates"),
 		"Summary": ctx.Tr("repo.issues.choose.invalid_templates", len(errs)),
-		"Details": utils.SanitizeFlashErrorString(strings.Join(lines, "\n")),
+		"Details": utils.EscapeFlashErrorString(strings.Join(lines, "\n")),
 	})
 	if err != nil {
 		log.Debug("render flash error: %v", err)

routers/web/repo/issue_view.go

@@ -29,7 +29,6 @@ import (
 	"code.gitea.io/gitea/modules/markup"
 	"code.gitea.io/gitea/modules/markup/markdown"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/templates"
 	"code.gitea.io/gitea/modules/templates/vars"
 	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web/middleware"
@@ -781,14 +780,14 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue
 		} else if comment.Type == issues_model.CommentTypeAddTimeManual ||
 			comment.Type == issues_model.CommentTypeStopTracking ||
 			comment.Type == issues_model.CommentTypeDeleteTimeManual {
-			// drop error since times could be pruned from DB..
+			// drop error since times could be pruned from DB
 			_ = comment.LoadTime(ctx)
 			if comment.Content != "" {
 				// Content before v1.21 did store the formatted string instead of seconds,
 				// so "|" is used as delimiter to mark the new format
 				if comment.Content[0] != '|' {
 					// handle old time comments that have formatted text stored
-					comment.RenderedContent = templates.SanitizeHTML(comment.Content)
+					comment.RenderedContent = markup.Sanitize(comment.Content)
 					comment.Content = ""
 				} else {
 					// else it's just a duration in seconds to pass on to the frontend