Question mark in URL renders as broken link in Markdown

[fnetX]

• Gitea version (or commit ref): 1.13 - 1.15-dev
• Database (use [x]):
  • MySQL
• Can you reproduce the bug at https://try.gitea.io:
  • Yes https://try.gitea.io/fnetx/everything/issues/1

Description

As a user on Codeberg pointed out a while ago, adding a question mark in a URL makes Gitea add a = sign at the end whcih breaks the url, see https://codeberg.org/Codeberg/Community/issues/292 (and originally https://codeberg.org/Codeberg/Community/issues/249#issuecomment-79991)
I didn't find this reported anywhere (although I found it's somehow hard to search for this if no one spelled out question mark and or equals sign), and it's still reproducible.

[silverwind]

Should rename the issue that this relates to URLs rendered in Markdown. It was not immediately clear to me. Sample of incorrect rendering:

<a href="https://htmlpreview.codeberg.eu/?https%3A%2F%2Fcodeberg.org%2Fivan-paleo%2FinitiationR%2Fsrc%2Fbranch%2Fmaster%2FinitiationR.html=" rel="nofollow">https://htmlpreview.codeberg.eu/?https://codeberg.org/ivan-paleo/initiationR/src/branch/master/initiationR.html</a>

[zeripath]

OK it appears that the = is being added by the bluemonday Sanitizer.

[zeripath]

Likely at:

gitea/vendor/github.com/microcosm-cc/bluemonday/sanitize.go

Lines 168 to 179 in 477c453

	func encodeQueries(queries []Query) string {
	var b strings.Builder
	for i, query := range queries {
	b.WriteString(url.QueryEscape(query.Key))
	b.WriteString("=")
	b.WriteString(url.QueryEscape(query.Value))
	if i < len(queries)-1 {
	b.WriteString("&")
	}
	}
	return b.String()
	}

Bluemonday has made the common but actually non-compliant assumption that the query component must contain key=value pairs.

[silverwind]

Bluemonday has made the common but actually non-compliant assumption that the query component must contain key=value pairs.

Ugh, yeah that is a very bad assumption.

[zeripath]

Now the following patch will handle that assumption:

diff --git a/vendor/github.com/microcosm-cc/bluemonday/sanitize.go b/vendor/github.com/microcosm-cc/bluemonday/sanitize.go
index 99559bbab..f4ec4126e 100644
--- a/vendor/github.com/microcosm-cc/bluemonday/sanitize.go
+++ b/vendor/github.com/microcosm-cc/bluemonday/sanitize.go
@@ -124,8 +124,9 @@ func escapeUrlComponent(val string) string {
 
 // Query represents a query
 type Query struct {
-	Key   string
-	Value string
+	Key      string
+	Value    string
+	HasValue bool
 }
 
 func parseQuery(query string) (values []Query, err error) {
@@ -139,9 +140,11 @@ func parseQuery(query string) (values []Query, err error) {
 		if key == "" {
 			continue
 		}
+		hasValue := false
 		value := ""
 		if i := strings.Index(key, "="); i >= 0 {
 			key, value = key[:i], key[i+1:]
+			hasValue = true
 		}
 		key, err1 := url.QueryUnescape(key)
 		if err1 != nil {
@@ -158,8 +161,9 @@ func parseQuery(query string) (values []Query, err error) {
 			continue
 		}
 		values = append(values, Query{
-			Key:   key,
-			Value: value,
+			Key:      key,
+			Value:    value,
+			HasValue: hasValue,
 		})
 	}
 	return values, err
@@ -169,8 +173,10 @@ func encodeQueries(queries []Query) string {
 	var b strings.Builder
 	for i, query := range queries {
 		b.WriteString(url.QueryEscape(query.Key))
-		b.WriteString("=")
-		b.WriteString(url.QueryEscape(query.Value))
+		if query.HasValue {
+			b.WriteString("=")
+			b.WriteString(url.QueryEscape(query.Value))
+		}
 		if i < len(queries)-1 {
 			b.WriteString("&")
 		}

This will make the url: https://htmlpreview.codeberg.eu/?https%3A%2F%2Fcodeberg.org%2Fivan-paleo%2FinitiationR%2Fsrc%2Fbranch%2Fmaster%2FinitiationR.html

Presumably this is correct enough? Technically AFAIU the / in the query component do not need to be escaped - nor potentially the :

[silverwind]

Presumably this is correct enough?

Yes, decodes fine

Technically AFAIU the / in the query component do not need to be escaped

Yes, I think url.QueryEscape is overzealous with the escaping. In that sample, I see no characters that require escaping. I'm not sure if query string actually requires special escaping at all, but I guess they won't hurt either.