Fix duplicate sub-path for avatars (#31365)

Fix #31361, and add tests

And this PR introduces an undocumented & debug-purpose-only config
option: `USE_SUB_URL_PATH`. It does nothing for end users, it only helps
the development of sub-path related problems.

And also fix #31366

Co-authored-by: @ExplodingDragon

custom/conf/app.example.ini

@@ -81,6 +81,10 @@ RUN_USER = ; git
 ;; Overwrite the automatically generated public URL. Necessary for proxies and docker.
 ;ROOT_URL = %(PROTOCOL)s://%(DOMAIN)s:%(HTTP_PORT)s/
 ;;
+;; For development purpose only. It makes Gitea handle sub-path ("/sub-path/owner/repo/...") directly when debugging without a reverse proxy.
+;; DO NOT USE IT IN PRODUCTION!!!
+;USE_SUB_URL_PATH = false
+;;
 ;; when STATIC_URL_PREFIX is empty it will follow ROOT_URL
 ;STATIC_URL_PREFIX =
 ;;

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 {
+// MakeAbsoluteURL tries to make a link to an absolute URL:
-	if IsRelativeURL(s) {
+// * If link is empty, it returns the current app URL.
-		return GuessCurrentAppURL(ctx) + strings.TrimPrefix(s, "/")
+// * 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,14 +46,14 @@ 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/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://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",
@@ -61,7 +61,7 @@ func TestMakeAbsoluteURL(t *testing.T) {
 			"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) {

modules/setting/global.go

@@ -0,0 +1,18 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package setting
+
+// Global settings
+var (
+	// RunUser is the OS user that Gitea is running as. ini:"RUN_USER"
+	RunUser string
+	// RunMode is the running mode of Gitea, it only accepts two values: "dev" and "prod".
+	// Non-dev values will be replaced by "prod". ini: "RUN_MODE"
+	RunMode string
+	// IsProd is true if RunMode is not "dev"
+	IsProd bool
+
+	// AppName is the Application name, used in the page title. ini: "APP_NAME"
+	AppName string
+)

modules/setting/server.go

@@ -40,16 +40,16 @@ const (
 	LandingPageLogin         LandingPage = "/user/login"
 )
 
+// Server settings
 var (
-	// AppName is the Application name, used in the page title.
-	// It maps to ini:"APP_NAME"
-	AppName string
 	// AppURL is the Application ROOT_URL. It always has a '/' suffix
 	// It maps to ini:"ROOT_URL"
 	AppURL string
 	// AppSubURL represents the sub-url mounting point for gitea. It is either "" or starts with '/' and ends without '/', such as '/{subpath}'.
 	// This value is empty if site does not have sub-url.
 	AppSubURL string
+	// UseSubURLPath makes Gitea handle requests with sub-path like "/sub-path/owner/repo/...", to make it easier to debug sub-path related problems without a reverse proxy.
+	UseSubURLPath bool
 	// AppDataPath is the default path for storing data.
 	// It maps to ini:"APP_DATA_PATH" in [server] and defaults to AppWorkPath + "/data"
 	AppDataPath string
@@ -59,8 +59,6 @@ var (
 	// AssetVersion holds a opaque value that is used for cache-busting assets
 	AssetVersion string
 
-	// Server settings
-
 	Protocol                   Scheme
 	UseProxyProtocol           bool // `ini:"USE_PROXY_PROTOCOL"`
 	ProxyProtocolTLSBridging   bool //`ini:"PROXY_PROTOCOL_TLS_BRIDGING"`
@@ -275,9 +273,10 @@ func loadServerFrom(rootCfg ConfigProvider) {
 	// This should be TrimRight to ensure that there is only a single '/' at the end of AppURL.
 	AppURL = strings.TrimRight(appURL.String(), "/") + "/"
 
-	// Suburl should start with '/' and end without '/', such as '/{subpath}'.
+	// AppSubURL should start with '/' and end without '/', such as '/{subpath}'.
 	// This value is empty if site does not have sub-url.
 	AppSubURL = strings.TrimSuffix(appURL.Path, "/")
+	UseSubURLPath = sec.Key("USE_SUB_URL_PATH").MustBool(false)
 	StaticURLPrefix = strings.TrimSuffix(sec.Key("STATIC_URL_PREFIX").MustString(AppSubURL), "/")
 
 	// Check if Domain differs from AppURL domain than update it to AppURL's domain

modules/setting/setting.go

@@ -25,12 +25,7 @@ var (
 	// AppStartTime store time gitea has started
 	AppStartTime time.Time
 
-	// Other global setting objects
-
 	CfgProvider ConfigProvider
-	RunMode     string
-	RunUser     string
-	IsProd      bool
 	IsWindows   bool
 
 	// IsInTesting indicates whether the testing is running. A lot of unreliable code causes a lot of nonsense error logs during testing

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)
 }

routers/common/middleware.go

@@ -25,7 +25,7 @@ import (
 // ProtocolMiddlewares returns HTTP protocol related middlewares, and it provides a global panic recovery
 func ProtocolMiddlewares() (handlers []any) {
 	// first, normalize the URL path
-	handlers = append(handlers, stripSlashesMiddleware)
+	handlers = append(handlers, normalizeRequestPathMiddleware)
 
 	// prepare the ContextData and panic recovery
 	handlers = append(handlers, func(next http.Handler) http.Handler {
@@ -75,9 +75,9 @@ func ProtocolMiddlewares() (handlers []any) {
 	return handlers
 }
 
-func stripSlashesMiddleware(next http.Handler) http.Handler {
+func normalizeRequestPathMiddleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
-		// First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL
+		// escape the URL RawPath to ensure that all routing is done using a correctly escaped URL
 		req.URL.RawPath = req.URL.EscapedPath()
 
 		urlPath := req.URL.RawPath
@@ -86,19 +86,42 @@ func stripSlashesMiddleware(next http.Handler) http.Handler {
 			urlPath = rctx.RoutePath
 		}
 
-		sanitizedPath := &strings.Builder{}
+		normalizedPath := strings.TrimRight(urlPath, "/")
-		prevWasSlash := false
+		// the following code block is a slow-path for replacing all repeated slashes "//" to one single "/"
-		for _, chr := range strings.TrimRight(urlPath, "/") {
+		// if the path doesn't have repeated slashes, then no need to execute it
-			if chr != '/' || !prevWasSlash {
+		if strings.Contains(normalizedPath, "//") {
-				sanitizedPath.WriteRune(chr)
+			buf := &strings.Builder{}
+			prevWasSlash := false
+			for _, chr := range normalizedPath {
+				if chr != '/' || !prevWasSlash {
+					buf.WriteRune(chr)
+				}
+				prevWasSlash = chr == '/'
+			}
+			normalizedPath = buf.String()
+		}
+
+		if setting.UseSubURLPath {
+			remainingPath, ok := strings.CutPrefix(normalizedPath, setting.AppSubURL+"/")
+			if ok {
+				normalizedPath = "/" + remainingPath
+			} else if normalizedPath == setting.AppSubURL {
+				normalizedPath = "/"
+			} else if !strings.HasPrefix(normalizedPath+"/", "/v2/") {
+				// do not respond to other requests, to simulate a real sub-path environment
+				http.Error(resp, "404 page not found, sub-path is: "+setting.AppSubURL, http.StatusNotFound)
+				return
 			}
-			prevWasSlash = chr == '/'
+			// TODO: it's not quite clear about how req.URL and rctx.RoutePath work together.
+			// Fortunately, it is only used for debug purpose, we have enough time to figure it out in the future.
+			req.URL.RawPath = normalizedPath
+			req.URL.Path = normalizedPath
 		}
 
 		if rctx == nil {
-			req.URL.Path = sanitizedPath.String()
+			req.URL.Path = normalizedPath
 		} else {
-			rctx.RoutePath = sanitizedPath.String()
+			rctx.RoutePath = normalizedPath
 		}
 		next.ServeHTTP(resp, req)
 	})

routers/common/middleware_test.go

@@ -61,7 +61,7 @@ func TestStripSlashesMiddleware(t *testing.T) {
 		})
 
 		// pass the test middleware to validate the changes
-		handlerToTest := stripSlashesMiddleware(testMiddleware)
+		handlerToTest := normalizeRequestPathMiddleware(testMiddleware)
 		// create a mock request to use
 		req := httptest.NewRequest("GET", tt.inputPath, nil)
 		// call the handler using a mock response recorder

services/pull/merge.go

@@ -23,6 +23,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/cache"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/httplib"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/references"
 	repo_module "code.gitea.io/gitea/modules/repository"
@@ -56,7 +57,7 @@ func getMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr *issue
 		issueReference = "!"
 	}
 
-	reviewedOn := fmt.Sprintf("Reviewed-on: %s/%s", setting.AppURL, pr.Issue.Link())
+	reviewedOn := fmt.Sprintf("Reviewed-on: %s", httplib.MakeAbsoluteURL(ctx, pr.Issue.Link()))
 	reviewedBy := pr.GetApprovers(ctx)
 
 	if mergeStyle != "" {