From 67c1a07285008cc00036a87cef966c3bd519a50c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 7 May 2024 16:26:13 +0800 Subject: [PATCH] Refactor AppURL usage (#30885) Fix #30883 Fix #29591 --------- Co-authored-by: KN4CK3R --- models/repo/avatar.go | 12 ++--- models/user/avatar.go | 10 ++-- modules/httplib/url.go | 60 ++++++++++++++++++++- modules/httplib/url_test.go | 59 +++++++++++++++++--- modules/markup/html_codepreview.go | 2 +- routers/api/actions/artifacts.go | 11 ++-- routers/api/actions/artifactsv4.go | 9 ++-- routers/api/packages/container/container.go | 3 +- routers/common/middleware.go | 3 ++ routers/common/redirect.go | 2 +- routers/web/auth/auth.go | 2 +- services/context/base.go | 2 +- services/context/context_response.go | 2 +- 13 files changed, 138 insertions(+), 39 deletions(-) diff --git a/models/repo/avatar.go b/models/repo/avatar.go index 72ee938ada..8395b8c2b7 100644 --- a/models/repo/avatar.go +++ b/models/repo/avatar.go @@ -9,10 +9,10 @@ import ( "image/png" "io" "net/url" - "strings" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/avatar" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" @@ -84,13 +84,7 @@ func (repo *Repository) relAvatarLink(ctx context.Context) string { return setting.AppSubURL + "/repo-avatars/" + url.PathEscape(repo.Avatar) } -// AvatarLink returns a link to the repository's avatar. +// 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 (repo *Repository) AvatarLink(ctx context.Context) string { - link := repo.relAvatarLink(ctx) - // we only prepend our AppURL to our known (relative, internal) avatar link to get an absolute URL - if strings.HasPrefix(link, "/") && !strings.HasPrefix(link, "//") { - return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:] - } - // otherwise, return the link as it is - return link + return httplib.MakeAbsoluteURL(ctx, repo.relAvatarLink(ctx)) } diff --git a/models/user/avatar.go b/models/user/avatar.go index c6937d7b51..921bc1b1a1 100644 --- a/models/user/avatar.go +++ b/models/user/avatar.go @@ -9,11 +9,11 @@ import ( "fmt" "image/png" "io" - "strings" "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/avatar" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" @@ -89,13 +89,9 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string { return avatars.GenerateEmailAvatarFastLink(ctx, u.AvatarEmail, size) } -// AvatarLink returns the full avatar link with http host +// 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 { - link := u.AvatarLinkWithSize(ctx, 0) - if !strings.HasPrefix(link, "//") && !strings.Contains(link, "://") { - return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL+"/") - } - return link + return httplib.MakeAbsoluteURL(ctx, u.AvatarLinkWithSize(ctx, 0)) } // IsUploadAvatarChanged returns true if the current user's avatar would be changed with the provided data diff --git a/modules/httplib/url.go b/modules/httplib/url.go index 903799cb68..541c4f325b 100644 --- a/modules/httplib/url.go +++ b/modules/httplib/url.go @@ -4,6 +4,8 @@ package httplib import ( + "context" + "net/http" "net/url" "strings" @@ -11,6 +13,10 @@ import ( "code.gitea.io/gitea/modules/util" ) +type RequestContextKeyStruct struct{} + +var RequestContextKey = RequestContextKeyStruct{} + func urlIsRelative(s string, u *url.URL) bool { // Unfortunately browsers consider a redirect Location with preceding "//", "\\", "/\" and "\/" as meaning redirect to "http(s)://REST_OF_PATH" // Therefore we should ignore these redirect locations to prevent open redirects @@ -26,7 +32,56 @@ func IsRelativeURL(s string) bool { return err == nil && urlIsRelative(s, u) } -func IsCurrentGiteaSiteURL(s string) bool { +func guessRequestScheme(req *http.Request, def string) string { + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Proto + if s := req.Header.Get("X-Forwarded-Proto"); s != "" { + return s + } + if s := req.Header.Get("X-Forwarded-Protocol"); s != "" { + return s + } + if s := req.Header.Get("X-Url-Scheme"); s != "" { + return s + } + if s := req.Header.Get("Front-End-Https"); s != "" { + return util.Iif(s == "on", "https", "http") + } + if s := req.Header.Get("X-Forwarded-Ssl"); s != "" { + return util.Iif(s == "on", "https", "http") + } + return def +} + +func guessForwardedHost(req *http.Request) string { + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host + 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 +func GuessCurrentAppURL(ctx context.Context) string { + req, ok := ctx.Value(RequestContextKey).(*http.Request) + if !ok { + return setting.AppURL + } + if host := guessForwardedHost(req); host != "" { + // if it is behind a reverse proxy, use "https" as default scheme in case the site admin forgets to set the correct forwarded-protocol headers + return guessRequestScheme(req, "https") + "://" + host + setting.AppSubURL + "/" + } else if req.Host != "" { + // if it is not behind a reverse proxy, use the scheme from config options, meanwhile use "https" as much as possible + defaultScheme := util.Iif(setting.Protocol == "http", "http", "https") + return guessRequestScheme(req, defaultScheme) + "://" + req.Host + setting.AppSubURL + "/" + } + return setting.AppURL +} + +func MakeAbsoluteURL(ctx context.Context, s string) string { + if IsRelativeURL(s) { + return GuessCurrentAppURL(ctx) + strings.TrimPrefix(s, "/") + } + return s +} + +func IsCurrentGiteaSiteURL(ctx context.Context, s string) bool { u, err := url.Parse(s) if err != nil { return false @@ -45,5 +100,6 @@ func IsCurrentGiteaSiteURL(s string) bool { if u.Path == "" { u.Path = "/" } - return strings.HasPrefix(strings.ToLower(u.String()), strings.ToLower(setting.AppURL)) + urlLower := strings.ToLower(u.String()) + return strings.HasPrefix(urlLower, strings.ToLower(setting.AppURL)) || strings.HasPrefix(urlLower, strings.ToLower(GuessCurrentAppURL(ctx))) } diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 9bf09bcf2f..e021cd610d 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -4,6 +4,8 @@ package httplib import ( + "context" + "net/http" "testing" "code.gitea.io/gitea/modules/setting" @@ -37,9 +39,44 @@ func TestIsRelativeURL(t *testing.T) { } } +func TestMakeAbsoluteURL(t *testing.T) { + defer test.MockVariableValue(&setting.Protocol, "http")() + defer test.MockVariableValue(&setting.AppURL, "http://the-host/sub/")() + defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + + ctx := context.Background() + assert.Equal(t, "http://the-host/sub/", MakeAbsoluteURL(ctx, "")) + assert.Equal(t, "http://the-host/sub/foo", MakeAbsoluteURL(ctx, "foo")) + assert.Equal(t, "http://the-host/sub/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://user-host/sub/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, "https://forwarded-host/sub/foo", MakeAbsoluteURL(ctx, "/foo")) + + ctx = context.WithValue(ctx, RequestContextKey, &http.Request{ + Host: "user-host", + Header: map[string][]string{ + "X-Forwarded-Host": {"forwarded-host"}, + "X-Forwarded-Proto": {"https"}, + }, + }) + assert.Equal(t, "https://forwarded-host/sub/foo", MakeAbsoluteURL(ctx, "/foo")) +} + func TestIsCurrentGiteaSiteURL(t *testing.T) { defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")() defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + ctx := context.Background() good := []string{ "?key=val", "/sub", @@ -50,7 +87,7 @@ func TestIsCurrentGiteaSiteURL(t *testing.T) { "http://localhost:3000/sub/", } for _, s := range good { - assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s) + assert.True(t, IsCurrentGiteaSiteURL(ctx, s), "good = %q", s) } bad := []string{ ".", @@ -64,13 +101,23 @@ func TestIsCurrentGiteaSiteURL(t *testing.T) { "http://other/", } for _, s := range bad { - assert.False(t, IsCurrentGiteaSiteURL(s), "bad = %q", s) + assert.False(t, IsCurrentGiteaSiteURL(ctx, s), "bad = %q", s) } setting.AppURL = "http://localhost:3000/" setting.AppSubURL = "" - assert.False(t, IsCurrentGiteaSiteURL("//")) - assert.False(t, IsCurrentGiteaSiteURL("\\\\")) - assert.False(t, IsCurrentGiteaSiteURL("http://localhost")) - assert.True(t, IsCurrentGiteaSiteURL("http://localhost:3000?key=val")) + assert.False(t, IsCurrentGiteaSiteURL(ctx, "//")) + assert.False(t, IsCurrentGiteaSiteURL(ctx, "\\\\")) + assert.False(t, IsCurrentGiteaSiteURL(ctx, "http://localhost")) + assert.True(t, IsCurrentGiteaSiteURL(ctx, "http://localhost:3000?key=val")) + + ctx = context.WithValue(ctx, RequestContextKey, &http.Request{ + Host: "user-host", + Header: map[string][]string{ + "X-Forwarded-Host": {"forwarded-host"}, + "X-Forwarded-Proto": {"https"}, + }, + }) + assert.True(t, IsCurrentGiteaSiteURL(ctx, "http://localhost:3000")) + assert.True(t, IsCurrentGiteaSiteURL(ctx, "https://forwarded-host")) } diff --git a/modules/markup/html_codepreview.go b/modules/markup/html_codepreview.go index 5ef2217e3d..5ab9290b3e 100644 --- a/modules/markup/html_codepreview.go +++ b/modules/markup/html_codepreview.go @@ -42,7 +42,7 @@ func renderCodeBlock(ctx *RenderContext, node *html.Node) (urlPosStart, urlPosSt CommitID: node.Data[m[6]:m[7]], FilePath: node.Data[m[8]:m[9]], } - if !httplib.IsCurrentGiteaSiteURL(opts.FullURL) { + if !httplib.IsCurrentGiteaSiteURL(ctx.Ctx, opts.FullURL) { return 0, 0, "", nil } u, err := url.Parse(opts.FilePath) diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index 5bd004bd37..35e3ee6906 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -71,6 +71,7 @@ import ( "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -184,8 +185,8 @@ type artifactRoutes struct { fs storage.ObjectStorage } -func (ar artifactRoutes) buildArtifactURL(runID int64, artifactHash, suffix string) string { - uploadURL := strings.TrimSuffix(setting.AppURL, "/") + strings.TrimSuffix(ar.prefix, "/") + +func (ar artifactRoutes) buildArtifactURL(ctx *ArtifactContext, runID int64, artifactHash, suffix string) string { + uploadURL := strings.TrimSuffix(httplib.GuessCurrentAppURL(ctx), "/") + strings.TrimSuffix(ar.prefix, "/") + strings.ReplaceAll(artifactRouteBase, "{run_id}", strconv.FormatInt(runID, 10)) + "/" + artifactHash + "/" + suffix return uploadURL @@ -224,7 +225,7 @@ func (ar artifactRoutes) getUploadArtifactURL(ctx *ArtifactContext) { // use md5(artifact_name) to create upload url artifactHash := fmt.Sprintf("%x", md5.Sum([]byte(req.Name))) resp := getUploadArtifactResponse{ - FileContainerResourceURL: ar.buildArtifactURL(runID, artifactHash, "upload"+retentionQuery), + FileContainerResourceURL: ar.buildArtifactURL(ctx, runID, artifactHash, "upload"+retentionQuery), } log.Debug("[artifact] get upload url: %s", resp.FileContainerResourceURL) ctx.JSON(http.StatusOK, resp) @@ -365,7 +366,7 @@ func (ar artifactRoutes) listArtifacts(ctx *ArtifactContext) { artifactHash := fmt.Sprintf("%x", md5.Sum([]byte(art.ArtifactName))) item := listArtifactsResponseItem{ Name: art.ArtifactName, - FileContainerResourceURL: ar.buildArtifactURL(runID, artifactHash, "download_url"), + FileContainerResourceURL: ar.buildArtifactURL(ctx, runID, artifactHash, "download_url"), } items = append(items, item) values[art.ArtifactName] = true @@ -437,7 +438,7 @@ func (ar artifactRoutes) getDownloadArtifactURL(ctx *ArtifactContext) { } } if downloadURL == "" { - downloadURL = ar.buildArtifactURL(runID, strconv.FormatInt(artifact.ID, 10), "download") + downloadURL = ar.buildArtifactURL(ctx, runID, strconv.FormatInt(artifact.ID, 10), "download") } item := downloadArtifactResponseItem{ Path: util.PathJoinRel(itemPath, artifact.ArtifactPath), diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 8300989c75..dde9caf4f2 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -92,6 +92,7 @@ import ( "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" @@ -160,9 +161,9 @@ func (r artifactV4Routes) buildSignature(endp, expires, artifactName string, tas return mac.Sum(nil) } -func (r artifactV4Routes) buildArtifactURL(endp, artifactName string, taskID int64) string { +func (r artifactV4Routes) buildArtifactURL(ctx *ArtifactContext, endp, artifactName string, taskID int64) string { expires := time.Now().Add(60 * time.Minute).Format("2006-01-02 15:04:05.999999999 -0700 MST") - uploadURL := strings.TrimSuffix(setting.AppURL, "/") + strings.TrimSuffix(r.prefix, "/") + + uploadURL := strings.TrimSuffix(httplib.GuessCurrentAppURL(ctx), "/") + strings.TrimSuffix(r.prefix, "/") + "/" + endp + "?sig=" + base64.URLEncoding.EncodeToString(r.buildSignature(endp, expires, artifactName, taskID)) + "&expires=" + url.QueryEscape(expires) + "&artifactName=" + url.QueryEscape(artifactName) + "&taskID=" + fmt.Sprint(taskID) return uploadURL } @@ -278,7 +279,7 @@ func (r *artifactV4Routes) createArtifact(ctx *ArtifactContext) { respData := CreateArtifactResponse{ Ok: true, - SignedUploadUrl: r.buildArtifactURL("UploadArtifact", artifactName, ctx.ActionTask.ID), + SignedUploadUrl: r.buildArtifactURL(ctx, "UploadArtifact", artifactName, ctx.ActionTask.ID), } r.sendProtbufBody(ctx, &respData) } @@ -454,7 +455,7 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { } } if respData.SignedUrl == "" { - respData.SignedUrl = r.buildArtifactURL("DownloadArtifact", artifactName, ctx.ActionTask.ID) + respData.SignedUrl = r.buildArtifactURL(ctx, "DownloadArtifact", artifactName, ctx.ActionTask.ID) } r.sendProtbufBody(ctx, &respData) } diff --git a/routers/api/packages/container/container.go b/routers/api/packages/container/container.go index 2cb16daebc..1efd166eb3 100644 --- a/routers/api/packages/container/container.go +++ b/routers/api/packages/container/container.go @@ -17,6 +17,7 @@ import ( packages_model "code.gitea.io/gitea/models/packages" container_model "code.gitea.io/gitea/models/packages/container" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" packages_module "code.gitea.io/gitea/modules/packages" @@ -115,7 +116,7 @@ func apiErrorDefined(ctx *context.Context, err *namedError) { } func apiUnauthorizedError(ctx *context.Context) { - ctx.Resp.Header().Add("WWW-Authenticate", `Bearer realm="`+setting.AppURL+`v2/token",service="container_registry",scope="*"`) + ctx.Resp.Header().Add("WWW-Authenticate", `Bearer realm="`+httplib.GuessCurrentAppURL(ctx)+`v2/token",service="container_registry",scope="*"`) apiErrorDefined(ctx, errUnauthorized) } diff --git a/routers/common/middleware.go b/routers/common/middleware.go index c7c75fb099..8b661993bb 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -4,11 +4,13 @@ package common import ( + go_context "context" "fmt" "net/http" "strings" "code.gitea.io/gitea/modules/cache" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web/middleware" @@ -34,6 +36,7 @@ func ProtocolMiddlewares() (handlers []any) { } }() req = req.WithContext(middleware.WithContextData(req.Context())) + req = req.WithContext(go_context.WithValue(req.Context(), httplib.RequestContextKey, req)) next.ServeHTTP(resp, req) }) }) diff --git a/routers/common/redirect.go b/routers/common/redirect.go index 34044e814b..d64f74ec82 100644 --- a/routers/common/redirect.go +++ b/routers/common/redirect.go @@ -17,7 +17,7 @@ func FetchRedirectDelegate(resp http.ResponseWriter, req *http.Request) { // The typical page is "issue comment" page. The backend responds "/owner/repo/issues/1#comment-2", // then frontend needs this delegate to redirect to the new location with hash correctly. redirect := req.PostFormValue("redirect") - if !httplib.IsCurrentGiteaSiteURL(redirect) { + if !httplib.IsCurrentGiteaSiteURL(req.Context(), redirect) { resp.WriteHeader(http.StatusBadRequest) return } diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 7c873796fe..4083d64226 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -368,7 +368,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe return setting.AppSubURL + "/" } - if redirectTo := ctx.GetSiteCookie("redirect_to"); redirectTo != "" && httplib.IsCurrentGiteaSiteURL(redirectTo) { + if redirectTo := ctx.GetSiteCookie("redirect_to"); redirectTo != "" && httplib.IsCurrentGiteaSiteURL(ctx, redirectTo) { middleware.DeleteRedirectToCookie(ctx.Resp) if obeyRedirect { ctx.RedirectToCurrentSite(redirectTo) diff --git a/services/context/base.go b/services/context/base.go index 05b8ab1b9b..29e62ae389 100644 --- a/services/context/base.go +++ b/services/context/base.go @@ -254,7 +254,7 @@ func (b *Base) Redirect(location string, status ...int) { code = status[0] } - if strings.HasPrefix(location, "http://") || strings.HasPrefix(location, "https://") || strings.HasPrefix(location, "//") { + if !httplib.IsRelativeURL(location) { // Some browsers (Safari) have buggy behavior for Cookie + Cache + External Redirection, eg: /my-path => https://other/path // 1. the first request to "/my-path" contains cookie // 2. some time later, the request to "/my-path" doesn't contain cookie (caused by Prevent web tracking) diff --git a/services/context/context_response.go b/services/context/context_response.go index 87c34c35ed..c43a649b49 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -52,7 +52,7 @@ func (ctx *Context) RedirectToCurrentSite(location ...string) { continue } - if !httplib.IsCurrentGiteaSiteURL(loc) { + if !httplib.IsCurrentGiteaSiteURL(ctx, loc) { continue }