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:
https://github.com/go-gitea/gitea/issues/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

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

routers/web/repo/pull.go

@@ -1042,7 +1042,7 @@ func UpdatePullRequest(ctx *context.Context) {
 			flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 				"Message": ctx.Tr("repo.pulls.merge_conflict"),
 				"Summary": ctx.Tr("repo.pulls.merge_conflict_summary"),
-				"Details": utils.SanitizeFlashErrorString(conflictError.StdErr) + "<br>" + utils.SanitizeFlashErrorString(conflictError.StdOut),
+				"Details": utils.EscapeFlashErrorString(conflictError.StdErr) + "\n" + utils.EscapeFlashErrorString(conflictError.StdOut),
 			})
 			if err != nil {
 				ctx.ServerError("UpdatePullRequest.HTMLString", err)
@@ -1054,9 +1054,9 @@ func UpdatePullRequest(ctx *context.Context) {
 		} else if pull_service.IsErrRebaseConflicts(err) {
 			conflictError := err.(pull_service.ErrRebaseConflicts)
 			flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
-				"Message": ctx.Tr("repo.pulls.rebase_conflict", utils.SanitizeFlashErrorString(conflictError.CommitSHA)),
+				"Message": ctx.Tr("repo.pulls.rebase_conflict", utils.EscapeFlashErrorString(conflictError.CommitSHA)),
 				"Summary": ctx.Tr("repo.pulls.rebase_conflict_summary"),
-				"Details": utils.SanitizeFlashErrorString(conflictError.StdErr) + "<br>" + utils.SanitizeFlashErrorString(conflictError.StdOut),
+				"Details": utils.EscapeFlashErrorString(conflictError.StdErr) + "\n" + utils.EscapeFlashErrorString(conflictError.StdOut),
 			})
 			if err != nil {
 				ctx.ServerError("UpdatePullRequest.HTMLString", err)
@@ -1191,7 +1191,7 @@ func MergePullRequest(ctx *context.Context) {
 			flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 				"Message": ctx.Tr("repo.editor.merge_conflict"),
 				"Summary": ctx.Tr("repo.editor.merge_conflict_summary"),
-				"Details": utils.SanitizeFlashErrorString(conflictError.StdErr) + "<br>" + utils.SanitizeFlashErrorString(conflictError.StdOut),
+				"Details": utils.EscapeFlashErrorString(conflictError.StdErr) + "\n" + utils.EscapeFlashErrorString(conflictError.StdOut),
 			})
 			if err != nil {
 				ctx.ServerError("MergePullRequest.HTMLString", err)
@@ -1202,9 +1202,9 @@ func MergePullRequest(ctx *context.Context) {
 		} else if pull_service.IsErrRebaseConflicts(err) {
 			conflictError := err.(pull_service.ErrRebaseConflicts)
 			flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
-				"Message": ctx.Tr("repo.pulls.rebase_conflict", utils.SanitizeFlashErrorString(conflictError.CommitSHA)),
+				"Message": ctx.Tr("repo.pulls.rebase_conflict", utils.EscapeFlashErrorString(conflictError.CommitSHA)),
 				"Summary": ctx.Tr("repo.pulls.rebase_conflict_summary"),
-				"Details": utils.SanitizeFlashErrorString(conflictError.StdErr) + "<br>" + utils.SanitizeFlashErrorString(conflictError.StdOut),
+				"Details": utils.EscapeFlashErrorString(conflictError.StdErr) + "\n" + utils.EscapeFlashErrorString(conflictError.StdOut),
 			})
 			if err != nil {
 				ctx.ServerError("MergePullRequest.HTMLString", err)
@@ -1234,7 +1234,7 @@ func MergePullRequest(ctx *context.Context) {
 				flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 					"Message": ctx.Tr("repo.pulls.push_rejected"),
 					"Summary": ctx.Tr("repo.pulls.push_rejected_summary"),
-					"Details": utils.SanitizeFlashErrorString(pushrejErr.Message),
+					"Details": utils.EscapeFlashErrorString(pushrejErr.Message),
 				})
 				if err != nil {
 					ctx.ServerError("MergePullRequest.HTMLString", err)
@@ -1454,7 +1454,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
 			flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
 				"Message": ctx.Tr("repo.pulls.push_rejected"),
 				"Summary": ctx.Tr("repo.pulls.push_rejected_summary"),
-				"Details": utils.SanitizeFlashErrorString(pushrejErr.Message),
+				"Details": utils.EscapeFlashErrorString(pushrejErr.Message),
 			})
 			if err != nil {
 				ctx.ServerError("CompareAndPullRequest.HTMLString", err)