Fix duplicate sub-path for avatars (#31365)

models/repo/avatar_test.go

@@ -0,0 +1,28 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package repo
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRepoAvatarLink(t *testing.T) {
+	defer test.MockVariableValue(&setting.AppURL, "https://localhost/")()
+	defer test.MockVariableValue(&setting.AppSubURL, "")()
+
+	repo := &Repository{ID: 1, Avatar: "avatar.png"}
+	link := repo.AvatarLink(db.DefaultContext)
+	assert.Equal(t, "https://localhost/repo-avatars/avatar.png", link)
+
+	setting.AppURL = "https://localhost/sub-path/"
+	setting.AppSubURL = "/sub-path"
+	link = repo.AvatarLink(db.DefaultContext)
+	assert.Equal(t, "https://localhost/sub-path/repo-avatars/avatar.png", link)
+}

models/user/avatar.go

@@ -89,9 +89,11 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string {
 	return avatars.GenerateEmailAvatarFastLink(ctx, u.AvatarEmail, size)
 }
 
-// AvatarLink returns the full avatar url with http host. TODO: refactor it to a relative URL, but it is still used in API response at the moment
+// AvatarLink returns the full avatar url with http host.
+// TODO: refactor it to a relative URL, but it is still used in API response at the moment
 func (u *User) AvatarLink(ctx context.Context) string {
-	return httplib.MakeAbsoluteURL(ctx, u.AvatarLinkWithSize(ctx, 0))
+	relLink := u.AvatarLinkWithSize(ctx, 0) // it can't be empty
+	return httplib.MakeAbsoluteURL(ctx, relLink)
 }
 
 // IsUploadAvatarChanged returns true if the current user's avatar would be changed with the provided data

models/user/avatar_test.go

@@ -0,0 +1,28 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package user
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUserAvatarLink(t *testing.T) {
+	defer test.MockVariableValue(&setting.AppURL, "https://localhost/")()
+	defer test.MockVariableValue(&setting.AppSubURL, "")()
+
+	u := &User{ID: 1, Avatar: "avatar.png"}
+	link := u.AvatarLink(db.DefaultContext)
+	assert.Equal(t, "https://localhost/avatars/avatar.png", link)
+
+	setting.AppURL = "https://localhost/sub-path/"
+	setting.AppSubURL = "/sub-path"
+	link = u.AvatarLink(db.DefaultContext)
+	assert.Equal(t, "https://localhost/sub-path/avatars/avatar.png", link)
+}

modules/httplib/url.go

@@ -57,11 +57,16 @@ func getForwardedHost(req *http.Request) string {
 	return req.Header.Get("X-Forwarded-Host")
 }
 
-// GuessCurrentAppURL tries to guess the current full URL by http headers. It always has a '/' suffix, exactly the same as setting.AppURL
+// GuessCurrentAppURL tries to guess the current full app URL (with sub-path) by http headers. It always has a '/' suffix, exactly the same as setting.AppURL
 func GuessCurrentAppURL(ctx context.Context) string {
+	return GuessCurrentHostURL(ctx) + setting.AppSubURL + "/"
+}
+
+// GuessCurrentHostURL tries to guess the current full host URL (no sub-path) by http headers, there is no trailing slash.
+func GuessCurrentHostURL(ctx context.Context) string {
 	req, ok := ctx.Value(RequestContextKey).(*http.Request)
 	if !ok {
-		return setting.AppURL
+		return strings.TrimSuffix(setting.AppURL, setting.AppSubURL+"/")
 	}
 	// If no scheme provided by reverse proxy, then do not guess the AppURL, use the configured one.
 	// At the moment, if site admin doesn't configure the proxy headers correctly, then Gitea would guess wrong.
@@ -74,20 +79,27 @@ func GuessCurrentAppURL(ctx context.Context) string {
 	// So in the future maybe it should introduce a new config option, to let site admin decide how to guess the AppURL.
 	reqScheme := getRequestScheme(req)
 	if reqScheme == "" {
-		return setting.AppURL
+		return strings.TrimSuffix(setting.AppURL, setting.AppSubURL+"/")
 	}
 	reqHost := getForwardedHost(req)
 	if reqHost == "" {
 		reqHost = req.Host
 	}
-	return reqScheme + "://" + reqHost + setting.AppSubURL + "/"
+	return reqScheme + "://" + reqHost
 }
 
-func MakeAbsoluteURL(ctx context.Context, s string) string {
-	if IsRelativeURL(s) {
-		return GuessCurrentAppURL(ctx) + strings.TrimPrefix(s, "/")
+// MakeAbsoluteURL tries to make a link to an absolute URL:
+// * If link is empty, it returns the current app URL.
+// * If link is absolute, it returns the link.
+// * Otherwise, it returns the current host URL + link, the link itself should have correct sub-path (AppSubURL) if needed.
+func MakeAbsoluteURL(ctx context.Context, link string) string {
+	if link == "" {
+		return GuessCurrentAppURL(ctx)
+	}
+	if !IsRelativeURL(link) {
+		return link
 	}
-	return s
+	return GuessCurrentHostURL(ctx) + "/" + strings.TrimPrefix(link, "/")
 }
 
 func IsCurrentGiteaSiteURL(ctx context.Context, s string) bool {

modules/httplib/url_test.go

@@ -46,22 +46,22 @@ func TestMakeAbsoluteURL(t *testing.T) {
 
 	ctx := context.Background()
 	assert.Equal(t, "http://cfg-host/sub/", MakeAbsoluteURL(ctx, ""))
-	assert.Equal(t, "http://cfg-host/sub/foo", MakeAbsoluteURL(ctx, "foo"))
-	assert.Equal(t, "http://cfg-host/sub/foo", MakeAbsoluteURL(ctx, "/foo"))
+	assert.Equal(t, "http://cfg-host/foo", MakeAbsoluteURL(ctx, "foo"))
+	assert.Equal(t, "http://cfg-host/foo", MakeAbsoluteURL(ctx, "/foo"))
 	assert.Equal(t, "http://other/foo", MakeAbsoluteURL(ctx, "http://other/foo"))
 
 	ctx = context.WithValue(ctx, RequestContextKey, &http.Request{
 		Host: "user-host",
 	})
-	assert.Equal(t, "http://cfg-host/sub/foo", MakeAbsoluteURL(ctx, "/foo"))
+	assert.Equal(t, "http://cfg-host/foo", MakeAbsoluteURL(ctx, "/foo"))
 
 	ctx = context.WithValue(ctx, RequestContextKey, &http.Request{
 		Host: "user-host",
 		Header: map[string][]string{
 			"X-Forwarded-Host": {"forwarded-host"},
 		},
 	})
-	assert.Equal(t, "http://cfg-host/sub/foo", MakeAbsoluteURL(ctx, "/foo"))
+	assert.Equal(t, "http://cfg-host/foo", MakeAbsoluteURL(ctx, "/foo"))
 
 	ctx = context.WithValue(ctx, RequestContextKey, &http.Request{
 		Host: "user-host",
@@ -70,7 +70,7 @@ func TestMakeAbsoluteURL(t *testing.T) {
 			"X-Forwarded-Proto": {"https"},
 		},
 	})
-	assert.Equal(t, "https://forwarded-host/sub/foo", MakeAbsoluteURL(ctx, "/foo"))
+	assert.Equal(t, "https://forwarded-host/foo", MakeAbsoluteURL(ctx, "/foo"))
 }
 
 func TestIsCurrentGiteaSiteURL(t *testing.T) {

routers/api/packages/container/container.go

@@ -117,7 +117,7 @@ func apiErrorDefined(ctx *context.Context, err *namedError) {
 
 func apiUnauthorizedError(ctx *context.Context) {
 	// container registry requires that the "/v2" must be in the root, so the sub-path in AppURL should be removed
-	realmURL := strings.TrimSuffix(httplib.GuessCurrentAppURL(ctx), setting.AppSubURL+"/") + "/v2/token"
+	realmURL := httplib.GuessCurrentHostURL(ctx) + "/v2/token"
 	ctx.Resp.Header().Add("WWW-Authenticate", `Bearer realm="`+realmURL+`",service="container_registry",scope="*"`)
 	apiErrorDefined(ctx, errUnauthorized)
 }