Refactor HTMLFormat, update chroma render, fix js error (#33136)

A small refactor to improve HTMLFormat, to help to prevent low-level
mistakes.

And fix #33141, fix #33139

Author: wxiaoguang

go.mod

@@ -27,7 +27,7 @@ require (
 	github.com/ProtonMail/go-crypto v1.1.4
 	github.com/PuerkitoBio/goquery v1.10.0
 	github.com/SaveTheRbtz/zstd-seekable-format-go/pkg v0.7.3
-	github.com/alecthomas/chroma/v2 v2.14.0
+	github.com/alecthomas/chroma/v2 v2.15.0
 	github.com/aws/aws-sdk-go-v2/credentials v1.17.42
 	github.com/aws/aws-sdk-go-v2/service/codecommit v1.27.3
 	github.com/blakesmith/ar v0.0.0-20190502131153-809d4375e1fb

go.sum

@@ -81,11 +81,11 @@ github.com/RoaringBitmap/roaring v1.9.4 h1:yhEIoH4YezLYT04s1nHehNO64EKFTop/wBhxv
 github.com/RoaringBitmap/roaring v1.9.4/go.mod h1:6AXUsoIEzDTFFQCe1RbGA6uFONMhvejWj5rqITANK90=
 github.com/SaveTheRbtz/zstd-seekable-format-go/pkg v0.7.3 h1:BP0HiyNT3AQEYi+if3wkRcIdQFHtsw6xX3Kx0glckgA=
 github.com/SaveTheRbtz/zstd-seekable-format-go/pkg v0.7.3/go.mod h1:hMNtySovKkn2gdDuLqnqveP+mfhUSaBdoBcr2I7Zt0E=
-github.com/alecthomas/assert/v2 v2.7.0 h1:QtqSACNS3tF7oasA8CU6A6sXZSBDqnm7RfpLl9bZqbE=
-github.com/alecthomas/assert/v2 v2.7.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k=
+github.com/alecthomas/assert/v2 v2.11.0 h1:2Q9r3ki8+JYXvGsDyBXwH3LcJ+WK5D0gc5E8vS6K3D0=
+github.com/alecthomas/assert/v2 v2.11.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k=
 github.com/alecthomas/chroma/v2 v2.2.0/go.mod h1:vf4zrexSH54oEjJ7EdB65tGNHmH3pGZmVkgTP5RHvAs=
-github.com/alecthomas/chroma/v2 v2.14.0 h1:R3+wzpnUArGcQz7fCETQBzO5n9IMNi13iIs46aU4V9E=
-github.com/alecthomas/chroma/v2 v2.14.0/go.mod h1:QolEbTfmUHIMVpBqxeDnNBj2uoeI4EbYP4i6n68SG4I=
+github.com/alecthomas/chroma/v2 v2.15.0 h1:LxXTQHFoYrstG2nnV9y2X5O94sOBzf0CIUpSTbpxvMc=
+github.com/alecthomas/chroma/v2 v2.15.0/go.mod h1:gUhVLrPDXPtp/f+L1jo9xepo9gL4eLwRuGAunSZMkio=
 github.com/alecthomas/repr v0.0.0-20220113201626-b1b626ac65ae/go.mod h1:2kn6fqh/zIyPLmm3ugklbEi5hg5wS435eygvNfaDQL8=
 github.com/alecthomas/repr v0.4.0 h1:GhI2A8MACjfegCPVq9f1FLvIBS+DrQ2KQBFZP1iFzXc=
 github.com/alecthomas/repr v0.4.0/go.mod h1:Fr0507jx4eOXV7AlPV6AVZLYrLIuIeSOWtW57eE/O/4=

modules/htmlutil/html.go

@@ -30,7 +30,7 @@ func ParseSizeAndClass(defaultSize int, defaultClass string, others ...any) (int
 	return size, class
 }
 
-func HTMLFormat(s string, rawArgs ...any) template.HTML {
+func HTMLFormat(s template.HTML, rawArgs ...any) template.HTML {
 	args := slices.Clone(rawArgs)
 	for i, v := range args {
 		switch v := v.(type) {
@@ -44,5 +44,5 @@ func HTMLFormat(s string, rawArgs ...any) template.HTML {
 			args[i] = template.HTMLEscapeString(fmt.Sprint(v))
 		}
 	}
-	return template.HTML(fmt.Sprintf(s, args...))
+	return template.HTML(fmt.Sprintf(string(s), args...))
 }

modules/markup/internal/renderinternal.go

@@ -76,7 +76,7 @@ func (r *RenderInternal) ProtectSafeAttrs(content template.HTML) template.HTML {
 	return template.HTML(reAttrClass().ReplaceAllString(string(content), `$1 data-attr-class="`+r.secureIDPrefix+`$2"$3`))
 }
 
-func (r *RenderInternal) FormatWithSafeAttrs(w io.Writer, fmt string, a ...any) error {
+func (r *RenderInternal) FormatWithSafeAttrs(w io.Writer, fmt template.HTML, a ...any) error {
 	_, err := w.Write([]byte(r.ProtectSafeAttrs(htmlutil.HTMLFormat(fmt, a...))))
 	return err
 }

modules/markup/markdown/math/block_renderer.go

@@ -4,6 +4,8 @@
 package math
 
 import (
+	"html/template"
+
 	"code.gitea.io/gitea/modules/markup/internal"
 	giteaUtil "code.gitea.io/gitea/modules/util"
 
@@ -50,7 +52,7 @@ func (r *BlockRenderer) renderBlock(w util.BufWriter, source []byte, node gast.N
 	n := node.(*Block)
 	if entering {
 		code := giteaUtil.Iif(n.Inline, "", `<pre class="code-block is-loading">`) + `<code class="language-math display">`
-		_ = r.renderInternal.FormatWithSafeAttrs(w, code)
+		_ = r.renderInternal.FormatWithSafeAttrs(w, template.HTML(code))
 		r.writeLines(w, source, n)
 	} else {
 		_, _ = w.WriteString(`</code>` + giteaUtil.Iif(n.Inline, "", `</pre>`) + "\n")

modules/markup/orgmode/orgmode.go

@@ -147,7 +147,7 @@ func (r *orgWriter) resolveLink(kind, link string) string {
 func (r *orgWriter) WriteRegularLink(l org.RegularLink) {
 	link := r.resolveLink(l.Kind(), l.URL)
 
-	printHTML := func(html string, a ...any) {
+	printHTML := func(html template.HTML, a ...any) {
 		_, _ = fmt.Fprint(r, htmlutil.HTMLFormat(html, a...))
 	}
 	// Inspired by https://github.com/niklasfasching/go-org/blob/6eb20dbda93cb88c3503f7508dc78cbbc639378f/org/html_writer.go#L406-L427

modules/markup/orgmode/orgmode_test.go

@@ -103,8 +103,8 @@ func HelloWorld() {
 }
 #+end_src
 `, `<div class="src src-go">
-<pre><code class="chroma language-go"><span class="c1">// HelloWorld prints &#34;Hello World&#34;
-</span><span class="c1"></span><span class="kd">func</span> <span class="nf">HelloWorld</span><span class="p">()</span> <span class="p">{</span>
+<pre><code class="chroma language-go"><span class="c1">// HelloWorld prints &#34;Hello World&#34;</span>
+<span class="kd">func</span> <span class="nf">HelloWorld</span><span class="p">()</span> <span class="p">{</span>
 	<span class="nx">fmt</span><span class="p">.</span><span class="nf">Println</span><span class="p">(</span><span class="s">&#34;Hello World&#34;</span><span class="p">)</span>
 <span class="p">}</span></code></pre>
 </div>`)

modules/templates/helper.go

@@ -38,7 +38,7 @@ func NewFuncMap() template.FuncMap {
 		"Iif":          iif,
 		"Eval":         evalTokens,
 		"SafeHTML":     safeHTML,
-		"HTMLFormat":   htmlutil.HTMLFormat,
+		"HTMLFormat":   htmlFormat,
 		"HTMLEscape":   htmlEscape,
 		"QueryEscape":  queryEscape,
 		"QueryBuild":   QueryBuild,
@@ -207,6 +207,20 @@ func htmlEscape(s any) template.HTML {
 	panic(fmt.Sprintf("unexpected type %T", s))
 }
 
+func htmlFormat(s any, args ...any) template.HTML {
+	if len(args) == 0 {
+		// to prevent developers from calling "HTMLFormat $userInput" by mistake which will lead to XSS
+		panic("missing arguments for HTMLFormat")
+	}
+	switch v := s.(type) {
+	case string:
+		return htmlutil.HTMLFormat(template.HTML(v), args...)
+	case template.HTML:
+		return htmlutil.HTMLFormat(v, args...)
+	}
+	panic(fmt.Sprintf("unexpected type %T", s))
+}
+
 func jsEscapeSafe(s string) template.HTML {
 	return template.HTML(template.JSEscapeString(s))
 }

modules/templates/helper_test.go

@@ -8,7 +8,6 @@ import (
 	"strings"
 	"testing"
 
-	"code.gitea.io/gitea/modules/htmlutil"
 	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
@@ -88,7 +87,7 @@ func TestTemplateIif(t *testing.T) {
 func TestTemplateEscape(t *testing.T) {
 	execTmpl := func(code string) string {
 		tmpl := template.New("test")
-		tmpl.Funcs(template.FuncMap{"QueryBuild": QueryBuild, "HTMLFormat": htmlutil.HTMLFormat})
+		tmpl.Funcs(template.FuncMap{"QueryBuild": QueryBuild, "HTMLFormat": htmlFormat})
 		template.Must(tmpl.Parse(code))
 		w := &strings.Builder{}
 		assert.NoError(t, tmpl.Execute(w, nil))

web_src/js/features/common-button.ts

@@ -17,7 +17,8 @@ export function initGlobalDeleteButton(): void {
   // Some model/form elements will be filled by `data-id` / `data-name` / `data-data-xxx` attributes.
   // If there is a form defined by `data-form`, then the form will be submitted as-is (without any modification).
   // If there is no form, then the data will be posted to `data-url`.
-  // TODO: it's not encouraged to use this method. `show-modal` does far better than this.
+  // TODO: do not use this method in new code. `show-modal` / `link-action(data-modal-confirm)` does far better than this.
+  // FIXME: all legacy `delete-button` should be refactored to use `show-modal` or `link-action`
   for (const btn of document.querySelectorAll<HTMLElement>('.delete-button')) {
     btn.addEventListener('click', (e) => {
       e.preventDefault();

web_src/js/utils.test.ts

@@ -50,6 +50,7 @@ test('parseIssueNewHref', () => {
   expect(parseIssueNewHref('/owner/repo/issues/new?query')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'issues'});
   expect(parseIssueNewHref('/sub/owner/repo/issues/new#hash')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'issues'});
   expect(parseIssueNewHref('/sub/owner/repo/compare/feature/branch-1...fix/branch-2')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'pulls'});
+  expect(parseIssueNewHref('/other')).toEqual({});
 });
 
 test('parseUrl', () => {

web_src/js/utils.ts

@@ -40,7 +40,7 @@ export function parseIssueHref(href: string): IssuePathInfo {
 export function parseIssueNewHref(href: string): IssuePathInfo {
   const path = (href || '').replace(/[#?].*$/, '');
   const [_, ownerName, repoName, pathTypeField] = /([^/]+)\/([^/]+)\/(issues\/new|compare\/.+\.\.\.)/.exec(path) || [];
-  const pathType = pathTypeField.startsWith('issues/new') ? 'issues' : 'pulls';
+  const pathType = pathTypeField ? (pathTypeField.startsWith('issues/new') ? 'issues' : 'pulls') : undefined;
   return {ownerName, repoName, pathType};
 }