Fix URLJoin, markup render link reoslving, sign-in/up/linkaccount page common data (#36861)

The logic of "URLJoin" is unclear and it is often abused.

Also:
* Correct the `resolveLinkRelative` behavior
* Fix missing "PathEscape" in `ToTag`
* Fix more FIXMEs, and add new FIXMEs for newly found problems
* Refactor "auth page common template data"

modules/markup/camo.go

@@ -11,7 +11,6 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 )
 
 // CamoEncode encodes a lnk to fit with the go-camo and camo proxy links. The purposes of camo-proxy are:
@@ -27,7 +26,7 @@ func CamoEncode(link string) string {
 	macSum := b64encode(mac.Sum(nil))
 	encodedURL := b64encode([]byte(link))
 
-	return util.URLJoin(setting.Camo.ServerURL, macSum, encodedURL)
+	return strings.TrimSuffix(setting.Camo.ServerURL, "/") + "/" + macSum + "/" + encodedURL
 }
 
 func b64encode(data []byte) string {

modules/markup/html_commit.go

@@ -4,13 +4,13 @@
 package markup
 
 import (
+	"fmt"
 	"slices"
 	"strings"
 
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/httplib"
 	"code.gitea.io/gitea/modules/references"
-	"code.gitea.io/gitea/modules/util"
 
 	"golang.org/x/net/html"
 	"golang.org/x/net/html/atom"
@@ -219,7 +219,7 @@ func hashCurrentPatternProcessor(ctx *RenderContext, node *html.Node) {
 			continue
 		}
 
-		link := "/:root/" + util.URLJoin(ctx.RenderOptions.Metas["user"], ctx.RenderOptions.Metas["repo"], "commit", hash)
+		link := fmt.Sprintf("/:root/%s/%s/commit/%s", ctx.RenderOptions.Metas["user"], ctx.RenderOptions.Metas["repo"], hash)
 		replaceContent(node, m[2], m[3], createCodeLink(link, base.ShortSha(hash), "commit"))
 		start = 0
 		node = node.NextSibling.NextSibling
@@ -236,7 +236,7 @@ func commitCrossReferencePatternProcessor(ctx *RenderContext, node *html.Node) {
 		}
 
 		refText := ref.Owner + "/" + ref.Name + "@" + base.ShortSha(ref.CommitSha)
-		linkHref := "/:root/" + util.URLJoin(ref.Owner, ref.Name, "commit", ref.CommitSha)
+		linkHref := fmt.Sprintf("/:root/%s/%s/commit/%s", ref.Owner, ref.Name, ref.CommitSha)
 		link := createLink(ctx, linkHref, refText, "commit")
 
 		replaceContent(node, ref.RefLocation.Start, ref.RefLocation.End, link)

modules/markup/html_issue.go

@@ -4,6 +4,7 @@
 package markup
 
 import (
+	"fmt"
 	"strconv"
 	"strings"
 
@@ -162,7 +163,7 @@ func issueIndexPatternProcessor(ctx *RenderContext, node *html.Node) {
 			issueOwner := util.Iif(ref.Owner == "", ctx.RenderOptions.Metas["user"], ref.Owner)
 			issueRepo := util.Iif(ref.Owner == "", ctx.RenderOptions.Metas["repo"], ref.Name)
 			issuePath := util.Iif(ref.IsPull, "pulls", "issues")
-			linkHref := "/:root/" + util.URLJoin(issueOwner, issueRepo, issuePath, ref.Issue)
+			linkHref := fmt.Sprintf("/:root/%s/%s/%s/%s", issueOwner, issueRepo, issuePath, ref.Issue)
 
 			// at the moment, only render the issue index in a full line (or simple line) as icon+title
 			// otherwise it would be too noisy for "take #1 as an example" in a sentence

modules/markup/html_link.go

@@ -113,16 +113,17 @@ func shortLinkProcessor(ctx *RenderContext, node *html.Node) {
 		}
 		childNode.Parent = linkNode
 		absoluteLink := IsFullURLString(link)
-		if !absoluteLink {
+		// FIXME: it should be fully refactored in the future, it uses various hacky approaches to guess how to encode a path for wiki
+		// When a link contains "/", then we assume that the user has provided a well-encoded link.
+		if !absoluteLink && !strings.Contains(link, "/") {
+			// So only guess for links without "/".
 			if image {
 				link = strings.ReplaceAll(link, " ", "+")
 			} else {
 				// the hacky wiki name encoding: space to "-"
 				link = strings.ReplaceAll(link, " ", "-") // FIXME: it should support dashes in the link, eg: "the-dash-support.-"
 			}
-			if !strings.Contains(link, "/") {
-				link = url.PathEscape(link) // FIXME: it doesn't seem right and it might cause double-escaping
-			}
+			link = url.PathEscape(link)
 		}
 		if image {
 			title := props["title"]

modules/markup/html_mention.go

@@ -4,6 +4,7 @@
 package markup
 
 import (
+	"fmt"
 	"strings"
 
 	"code.gitea.io/gitea/modules/references"
@@ -26,14 +27,11 @@ func mentionProcessor(ctx *RenderContext, node *html.Node) {
 		loc.End += start
 		mention := node.Data[loc.Start:loc.End]
 		teams, ok := ctx.RenderOptions.Metas["teams"]
-		// FIXME: util.URLJoin may not be necessary here:
-		// - setting.AppURL is defined to have a terminal '/' so unless mention[1:]
-		// is an AppSubURL link we can probably fallback to concatenation.
-		// team mention should follow @orgName/teamName style
+
 		if ok && strings.Contains(mention, "/") {
 			mentionOrgAndTeam := strings.Split(mention, "/")
 			if mentionOrgAndTeam[0][1:] == ctx.RenderOptions.Metas["org"] && strings.Contains(teams, ","+strings.ToLower(mentionOrgAndTeam[1])+",") {
-				link := "/:root/" + util.URLJoin("org", ctx.RenderOptions.Metas["org"], "teams", mentionOrgAndTeam[1])
+				link := fmt.Sprintf("/:root/org/%s/teams/%s", ctx.RenderOptions.Metas["org"], mentionOrgAndTeam[1])
 				replaceContent(node, loc.Start, loc.End, createLink(ctx, link, mention, "" /*mention*/))
 				node = node.NextSibling.NextSibling
 				start = 0

modules/markup/html_test.go

@@ -389,7 +389,7 @@ func TestRender_ShortLinks(t *testing.T) {
 	imgurl := util.URLJoin(tree, "Link.jpg")
 	otherImgurl := util.URLJoin(tree, "Link+Other.jpg")
 	encodedImgurl := util.URLJoin(tree, "Link+%23.jpg")
-	notencodedImgurl := util.URLJoin(tree, "some", "path", "Link+#.jpg")
+	notencodedImgurl := util.URLJoin(tree, "some", "path", "Link%20#.jpg")
 	renderableFileURL := util.URLJoin(tree, "markdown_file.md")
 	unrenderableFileURL := util.URLJoin(tree, "file.zip")
 	favicon := "http://google.com/favicon.ico"
@@ -466,6 +466,8 @@ func TestRender_ShortLinks(t *testing.T) {
 		"[[Name|Link #.jpg|alt=\"AltName\"|title='Title']]",
 		`<p><a href="`+encodedImgurl+`" rel="nofollow"><img src="`+encodedImgurl+`" title="Title" alt="AltName"/></a></p>`,
 	)
+	// FIXME: it's unable to resolve: [[link?k=v]]
+	// FIXME: it is a wrong test case, it is not an image, but a link with anchor "#.jpg"
 	test(
 		"[[some/path/Link #.jpg]]",
 		`<p><a href="`+notencodedImgurl+`" rel="nofollow"><img src="`+notencodedImgurl+`" title="Link #.jpg" alt="some/path/Link #.jpg"/></a></p>`,

modules/markup/markdown/markdown_test.go

@@ -14,7 +14,6 @@ import (
 	"code.gitea.io/gitea/modules/markup/markdown"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/test"
-	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -23,7 +22,6 @@ const (
 	AppURL            = "http://localhost:3000/"
 	testRepoOwnerName = "user13"
 	testRepoName      = "repo11"
-	FullURL           = AppURL + testRepoOwnerName + "/" + testRepoName + "/"
 )
 
 // these values should match the const above
@@ -47,8 +45,9 @@ func TestRender_StandardLinks(t *testing.T) {
 func TestRender_Images(t *testing.T) {
 	setting.AppURL = AppURL
 
+	const baseLink = "http://localhost:3000/user13/repo11"
 	render := func(input, expected string) {
-		buffer, err := markdown.RenderString(markup.NewTestRenderContext(FullURL), input)
+		buffer, err := markdown.RenderString(markup.NewTestRenderContext(baseLink), input)
 		assert.NoError(t, err)
 		assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(buffer)))
 	}
@@ -56,7 +55,7 @@ func TestRender_Images(t *testing.T) {
 	url := "../../.images/src/02/train.jpg"
 	title := "Train"
 	href := "https://gitea.io"
-	result := util.URLJoin(FullURL, url)
+	result := baseLink + "/.images/src/02/train.jpg" // resolved link should not go out of the base link
 	// hint: With Markdown v2.5.2, there is a new syntax: [link](URL){:target="_blank"} , but we do not support it now
 
 	render(
@@ -88,6 +87,7 @@ func TestRender_Images(t *testing.T) {
 }
 
 func TestTotal_RenderString(t *testing.T) {
+	const FullURL = AppURL + testRepoOwnerName + "/" + testRepoName + "/"
 	setting.AppURL = AppURL
 	defer test.MockVariableValue(&markup.RenderBehaviorForTesting.DisableAdditionalAttributes, true)()
 

modules/markup/render_link.go

@@ -5,28 +5,47 @@ package markup
 
 import (
 	"context"
+	"net/url"
+	"path"
 	"strings"
 
 	"code.gitea.io/gitea/modules/httplib"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 )
 
+// resolveLinkRelative tries to resolve the link relative to the "{base}/{cur}", and returns the final link.
+// It only resolves the link, doesn't do any sanitization or validation, invalid links will be returned as is.
 func resolveLinkRelative(ctx context.Context, base, cur, link string, absolute bool) (finalLink string) {
-	if IsFullURLString(link) {
-		return link
+	linkURL, err := url.Parse(link)
+	if err != nil {
+		return link // invalid URL, return as is
 	}
+	if linkURL.Scheme != "" || linkURL.Host != "" {
+		return link // absolute URL, return as is
+	}
+
 	if strings.HasPrefix(link, "/") {
 		if strings.HasPrefix(link, base) && strings.Count(base, "/") >= 4 {
-			// a trick to tolerate that some users were using absolute paths (the old gitea's behavior)
+			// a trick to tolerate that some users were using absolute paths (the old Gitea's behavior)
+			// if the link is likely "{base}/src/main" while "{base}" is something like "/owner/repo"
 			finalLink = link
 		} else {
-			finalLink = util.URLJoin(base, "./", link)
+			// need to resolve the link relative to "{base}"
+			cur = ""
+		}
+	} // else: link is relative to "{base}/{cur}"
+
+	if finalLink == "" {
+		finalLink = strings.TrimSuffix(base, "/") + path.Join("/"+cur, "/"+linkURL.EscapedPath())
+		finalLink = strings.TrimSuffix(finalLink, "/")
+		if linkURL.RawQuery != "" {
+			finalLink += "?" + linkURL.RawQuery
+		}
+		if linkURL.Fragment != "" {
+			finalLink += "#" + linkURL.Fragment
 		}
-	} else {
-		finalLink = util.URLJoin(base, "./", cur, link)
 	}
-	finalLink = strings.TrimSuffix(finalLink, "/")
+
 	if absolute {
 		finalLink = httplib.MakeAbsoluteURL(ctx, finalLink)
 	}

modules/markup/render_link_test.go

@@ -18,8 +18,16 @@ func TestResolveLinkRelative(t *testing.T) {
 	assert.Equal(t, "/a/b", resolveLinkRelative(ctx, "/a", "b", "", false))
 	assert.Equal(t, "/a/b/c", resolveLinkRelative(ctx, "/a", "b", "c", false))
 	assert.Equal(t, "/a/c", resolveLinkRelative(ctx, "/a", "b", "/c", false))
+	assert.Equal(t, "/a/c#id", resolveLinkRelative(ctx, "/a", "b", "/c#id", false))
+	assert.Equal(t, "/a/%2f?k=/", resolveLinkRelative(ctx, "/a", "b", "/%2f/?k=/", false))
+	assert.Equal(t, "/a/b/c?k=v#id", resolveLinkRelative(ctx, "/a", "b", "c/?k=v#id", false))
+	assert.Equal(t, "%invalid", resolveLinkRelative(ctx, "/a", "b", "%invalid", false))
 	assert.Equal(t, "http://localhost:3000/a", resolveLinkRelative(ctx, "/a", "", "", true))
 
+	// absolute link is returned as is
+	assert.Equal(t, "mailto:user@domain.com", resolveLinkRelative(ctx, "/a", "", "mailto:user@domain.com", false))
+	assert.Equal(t, "http://other/path/", resolveLinkRelative(ctx, "/a", "", "http://other/path/", false))
+
 	// some users might have used absolute paths a lot, so if the prefix overlaps and has enough slashes, we should tolerate it
 	assert.Equal(t, "/owner/repo/foo/owner/repo/foo/bar/xxx", resolveLinkRelative(ctx, "/owner/repo/foo", "", "/owner/repo/foo/bar/xxx", false))
 	assert.Equal(t, "/owner/repo/foo/bar/xxx", resolveLinkRelative(ctx, "/owner/repo/foo/bar", "", "/owner/repo/foo/bar/xxx", false))

modules/recaptcha/recaptcha.go

@@ -13,7 +13,6 @@ import (
 
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/setting"
-	"code.gitea.io/gitea/modules/util"
 )
 
 // Response is the structure of JSON returned from API
@@ -24,17 +23,16 @@ type Response struct {
 	ErrorCodes  []ErrorCode `json:"error-codes"`
 }
 
-const apiURL = "api/siteverify"
-
 // Verify calls Google Recaptcha API to verify token
 func Verify(ctx context.Context, response string) (bool, error) {
 	post := url.Values{
 		"secret":   {setting.Service.RecaptchaSecret},
 		"response": {response},
 	}
+
+	reqURL := strings.TrimSuffix(setting.Service.RecaptchaURL, "/") + "/api/siteverify"
 	// Basically a copy of http.PostForm, but with a context
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost,
-		util.URLJoin(setting.Service.RecaptchaURL, apiURL), strings.NewReader(post.Encode()))
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, strings.NewReader(post.Encode()))
 	if err != nil {
 		return false, fmt.Errorf("Failed to create CAPTCHA request: %w", err)
 	}

modules/setting/server.go

@@ -15,7 +15,6 @@ import (
 
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/util"
 )
 
 // Scheme describes protocol types
@@ -163,7 +162,7 @@ func MakeManifestData(appName, appURL, absoluteAssetURL string) []byte {
 }
 
 // MakeAbsoluteAssetURL returns the absolute asset url prefix without a trailing slash
-func MakeAbsoluteAssetURL(appURL, staticURLPrefix string) string {
+func MakeAbsoluteAssetURL(appURL *url.URL, staticURLPrefix string) string {
 	parsedPrefix, err := url.Parse(strings.TrimSuffix(staticURLPrefix, "/"))
 	if err != nil {
 		log.Fatal("Unable to parse STATIC_URL_PREFIX: %v", err)
@@ -171,11 +170,12 @@ func MakeAbsoluteAssetURL(appURL, staticURLPrefix string) string {
 
 	if err == nil && parsedPrefix.Hostname() == "" {
 		if staticURLPrefix == "" {
-			return strings.TrimSuffix(appURL, "/")
+			return strings.TrimSuffix(appURL.String(), "/")
 		}
 
 		// StaticURLPrefix is just a path
-		return util.URLJoin(appURL, strings.TrimSuffix(staticURLPrefix, "/"))
+		appHostURL := &url.URL{Scheme: appURL.Scheme, Host: appURL.Host}
+		return appHostURL.String() + "/" + strings.Trim(staticURLPrefix, "/")
 	}
 
 	return strings.TrimSuffix(staticURLPrefix, "/")
@@ -316,7 +316,7 @@ func loadServerFrom(rootCfg ConfigProvider) {
 		Domain = urlHostname
 	}
 
-	AbsoluteAssetURL = MakeAbsoluteAssetURL(AppURL, StaticURLPrefix)
+	AbsoluteAssetURL = MakeAbsoluteAssetURL(appURL, StaticURLPrefix)
 	AssetVersion = strings.ReplaceAll(AppVer, "+", "~") // make sure the version string is clear (no real escaping is needed)
 
 	manifestBytes := MakeManifestData(AppName, AppURL, AbsoluteAssetURL)

modules/setting/setting_test.go

@@ -4,6 +4,7 @@
 package setting
 
 import (
+	"net/url"
 	"testing"
 
 	"code.gitea.io/gitea/modules/json"
@@ -12,18 +13,26 @@ import (
 )
 
 func TestMakeAbsoluteAssetURL(t *testing.T) {
-	assert.Equal(t, "https://localhost:2345", MakeAbsoluteAssetURL("https://localhost:1234", "https://localhost:2345"))
-	assert.Equal(t, "https://localhost:2345", MakeAbsoluteAssetURL("https://localhost:1234/", "https://localhost:2345"))
-	assert.Equal(t, "https://localhost:2345", MakeAbsoluteAssetURL("https://localhost:1234/", "https://localhost:2345/"))
-	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL("https://localhost:1234", "/foo"))
-	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL("https://localhost:1234/", "/foo"))
-	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL("https://localhost:1234/", "/foo/"))
-	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL("https://localhost:1234/foo", "/foo"))
-	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL("https://localhost:1234/foo/", "/foo"))
-	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL("https://localhost:1234/foo/", "/foo/"))
-	assert.Equal(t, "https://localhost:1234/bar", MakeAbsoluteAssetURL("https://localhost:1234/foo", "/bar"))
-	assert.Equal(t, "https://localhost:1234/bar", MakeAbsoluteAssetURL("https://localhost:1234/foo/", "/bar"))
-	assert.Equal(t, "https://localhost:1234/bar", MakeAbsoluteAssetURL("https://localhost:1234/foo/", "/bar/"))
+	appURL1, _ := url.Parse("https://localhost:1234")
+	appURL2, _ := url.Parse("https://localhost:1234/")
+	appURLSub1, _ := url.Parse("https://localhost:1234/foo")
+	appURLSub2, _ := url.Parse("https://localhost:1234/foo/")
+
+	// static URL is an absolute URL, so should be used
+	assert.Equal(t, "https://localhost:2345", MakeAbsoluteAssetURL(appURL1, "https://localhost:2345"))
+	assert.Equal(t, "https://localhost:2345", MakeAbsoluteAssetURL(appURL1, "https://localhost:2345/"))
+
+	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL(appURL1, "/foo"))
+	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL(appURL2, "/foo"))
+	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL(appURL1, "/foo/"))
+
+	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL(appURLSub1, "/foo"))
+	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL(appURLSub2, "/foo"))
+	assert.Equal(t, "https://localhost:1234/foo", MakeAbsoluteAssetURL(appURLSub1, "/foo/"))
+
+	assert.Equal(t, "https://localhost:1234/bar", MakeAbsoluteAssetURL(appURLSub1, "/bar"))
+	assert.Equal(t, "https://localhost:1234/bar", MakeAbsoluteAssetURL(appURLSub2, "/bar"))
+	assert.Equal(t, "https://localhost:1234/bar", MakeAbsoluteAssetURL(appURLSub1, "/bar/"))
 }
 
 func TestMakeManifestData(t *testing.T) {

modules/templates/helper.go

@@ -37,7 +37,6 @@ func NewFuncMap() template.FuncMap {
 		"QueryEscape":  queryEscape,
 		"QueryBuild":   QueryBuild,
 		"SanitizeHTML": SanitizeHTML,
-		"URLJoin":      util.URLJoin,
 		"DotEscape":    dotEscape,
 
 		"PathEscape":         url.PathEscape,

modules/util/url.go

@@ -20,6 +20,8 @@ func PathEscapeSegments(path string) string {
 }
 
 // URLJoin joins url components, like path.Join, but preserving contents
+// Deprecated: it has unclear behaviors, should not be used anymore. It is only used in some tests.
+// Need to be removed in the future.
 func URLJoin(base string, elems ...string) string {
 	if !strings.HasSuffix(base, "/") {
 		base += "/"