From 52925e9c7c22163ea66729ebfc091292e8a22eee Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Jun 2024 11:44:44 +0800 Subject: [PATCH] Fix duplicate sub-path for avatars (#31365) (#31368) Backport #31365, only backport necessary changes. --- 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 ++++---- routers/api/packages/container/container.go | 2 +- 6 files changed, 86 insertions(+), 16 deletions(-) create mode 100644 models/repo/avatar_test.go create mode 100644 models/user/avatar_test.go diff --git a/models/repo/avatar_test.go b/models/repo/avatar_test.go new file mode 100644 index 0000000000..fc1f8baeca --- /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 921bc1b1a1..5453c78fc6 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 0000000000..1078875ee1 --- /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 8dc5b71181..219dfe695c 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 9980cb74e8..28aaee6e12 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/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index b0c4458d51..5007037bee 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) }