Refactor flash message and remove SanitizeHTML template func (#37179)

1. Fix the "flash message" layout problem for different cases
* I am sure most of the users should have ever seen the ugly
center-aligned error message with multiple lines.
2. Fix inconsistent "Details" flash message EOL handling, sometimes
`\n`, sometimes `<br>`
   * Now, always use "\n" and use `<pre>` to render
3. Remove SanitizeHTML template func because it is not useful and can be
easily abused.
* But it is still kept for mail templates, for example:
#36049
4. Clarify PostProcessCommitMessage's behavior and add FIXME comment

By the way: cleaned up some devtest pages, move embedded style block to
CSS file

Author: wxiaoguang

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,
 		)