From 84cbb6c4d2ad57dc8816c0320eac061f753b50c1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Jun 2024 11:43:57 +0800 Subject: [PATCH] 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 | 4 ++ models/repo/avatar_test.go | 28 +++++++++++++ models/user/avatar.go | 6 ++- models/user/avatar_test.go | 28 +++++++++++++ modules/httplib/url.go | 28 +++++++++---- modules/httplib/url_test.go | 10 ++--- modules/setting/global.go | 18 +++++++++ modules/setting/server.go | 11 +++-- modules/setting/setting.go | 5 --- routers/api/packages/container/container.go | 2 +- routers/common/middleware.go | 45 ++++++++++++++++----- routers/common/middleware_test.go | 2 +- services/pull/merge.go | 3 +- 13 files changed, 150 insertions(+), 40 deletions(-) create mode 100644 models/repo/avatar_test.go create mode 100644 models/user/avatar_test.go create mode 100644 modules/setting/global.go diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index e619aae72..9196180d8 100644 --- a/custom/conf/app.example.ini +++ b/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 = ;; diff --git a/models/repo/avatar_test.go b/models/repo/avatar_test.go new file mode 100644 index 000000000..fc1f8baec --- /dev/null +++ b/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) +} diff --git a/models/user/avatar.go b/models/user/avatar.go index 921bc1b1a..5453c78fc 100644 --- a/models/user/avatar.go +++ b/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 diff --git a/models/user/avatar_test.go b/models/user/avatar_test.go new file mode 100644 index 000000000..1078875ee --- /dev/null +++ b/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) +} diff --git a/modules/httplib/url.go b/modules/httplib/url.go index 8dc5b7118..219dfe695 100644 --- a/modules/httplib/url.go +++ b/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) } - return s + if !IsRelativeURL(link) { + return link + } + return GuessCurrentHostURL(ctx) + "/" + strings.TrimPrefix(link, "/") } func IsCurrentGiteaSiteURL(ctx context.Context, s string) bool { diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 9980cb74e..28aaee6e1 100644 --- a/modules/httplib/url_test.go +++ b/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/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", @@ -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) { diff --git a/modules/setting/global.go b/modules/setting/global.go new file mode 100644 index 000000000..55dfe485b --- /dev/null +++ b/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 +) diff --git a/modules/setting/server.go b/modules/setting/server.go index 7d6ece272..d7a71578d 100644 --- a/modules/setting/server.go +++ b/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 diff --git a/modules/setting/setting.go b/modules/setting/setting.go index f056fbfc6..b4f913cda 100644 --- a/modules/setting/setting.go +++ b/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 diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index b0c4458d5..5007037be 100644 --- a/routers/api/packages/container/container.go +++ b/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) } diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 8b661993b..de4964839 100644 --- a/routers/common/middleware.go +++ b/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{} - prevWasSlash := false - for _, chr := range strings.TrimRight(urlPath, "/") { - if chr != '/' || !prevWasSlash { - sanitizedPath.WriteRune(chr) + normalizedPath := strings.TrimRight(urlPath, "/") + // the following code block is a slow-path for replacing all repeated slashes "//" to one single "/" + // if the path doesn't have repeated slashes, then no need to execute it + if strings.Contains(normalizedPath, "//") { + buf := &strings.Builder{} + prevWasSlash := false + for _, chr := range normalizedPath { + if chr != '/' || !prevWasSlash { + buf.WriteRune(chr) + } + prevWasSlash = 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 + } + // 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) }) diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index f16b9374e..c96071c3a 100644 --- a/routers/common/middleware_test.go +++ b/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 diff --git a/services/pull/merge.go b/services/pull/merge.go index 9ef3fb2e0..e19292c31 100644 --- a/services/pull/merge.go +++ b/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 != "" {