From 01500957c29f6bfa2396b8457dbb0645edaafa99 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 21 Mar 2024 20:02:34 +0800 Subject: [PATCH] Refactor URL detection (#29960) "Redirect" functions should only redirect if the target is for current Gitea site. --- modules/httplib/url.go | 40 +++++++++++---- modules/httplib/url_test.go | 77 ++++++++++++++++++++-------- routers/common/redirect.go | 2 +- routers/web/auth/auth.go | 6 +-- routers/web/auth/oauth.go | 2 +- routers/web/auth/password.go | 2 +- routers/web/repo/repo.go | 2 +- routers/web/web.go | 2 +- services/context/context_response.go | 6 +-- 9 files changed, 96 insertions(+), 43 deletions(-) diff --git a/modules/httplib/url.go b/modules/httplib/url.go index 14b95898f..b679b4450 100644 --- a/modules/httplib/url.go +++ b/modules/httplib/url.go @@ -8,20 +8,40 @@ import ( "strings" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) -// IsRiskyRedirectURL returns true if the URL is considered risky for redirects -func IsRiskyRedirectURL(s string) bool { +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 if len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') { - return true + return false } - - u, err := url.Parse(s) - if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(s), strings.ToLower(setting.AppURL))) { - return true - } - - return false + return u != nil && u.Scheme == "" && u.Host == "" +} + +// IsRelativeURL detects if a URL is relative (no scheme or host) +func IsRelativeURL(s string) bool { + u, err := url.Parse(s) + return err == nil && urlIsRelative(s, u) +} + +func IsCurrentGiteaSiteURL(s string) bool { + u, err := url.Parse(s) + if err != nil { + return false + } + if u.Path != "" { + u.Path = "/" + util.PathJoinRelX(u.Path) + if !strings.HasSuffix(u.Path, "/") { + u.Path += "/" + } + } + if urlIsRelative(s, u) { + return u.Path == "" || strings.HasPrefix(strings.ToLower(u.Path), strings.ToLower(setting.AppSubURL+"/")) + } + if u.Path == "" { + u.Path = "/" + } + return strings.HasPrefix(strings.ToLower(u.String()), strings.ToLower(setting.AppURL)) } diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 72033b120..9b7b24229 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -7,32 +7,65 @@ import ( "testing" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" ) -func TestIsRiskyRedirectURL(t *testing.T) { - setting.AppURL = "http://localhost:3000/" - tests := []struct { - input string - want bool - }{ - {"", false}, - {"foo", false}, - {"/", false}, - {"/foo?k=%20#abc", false}, - - {"//", true}, - {"\\\\", true}, - {"/\\", true}, - {"\\/", true}, - {"mail:a@b.com", true}, - {"https://test.com", true}, - {setting.AppURL + "/foo", false}, +func TestIsRelativeURL(t *testing.T) { + defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")() + defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + rel := []string{ + "", + "foo", + "/", + "/foo?k=%20#abc", } - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input)) - }) + for _, s := range rel { + assert.True(t, IsRelativeURL(s), "rel = %q", s) + } + abs := []string{ + "//", + "\\\\", + "/\\", + "\\/", + "mailto:a@b.com", + "https://test.com", + } + for _, s := range abs { + assert.False(t, IsRelativeURL(s), "abs = %q", s) } } + +func TestIsCurrentGiteaSiteURL(t *testing.T) { + defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")() + defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + good := []string{ + "?key=val", + "/sub", + "/sub/", + "/sub/foo", + "/sub/foo/", + "http://localhost:3000/sub?key=val", + "http://localhost:3000/sub/", + } + for _, s := range good { + assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s) + } + bad := []string{ + "/", + "//", + "\\\\", + "/foo", + "http://localhost:3000/sub/..", + "http://localhost:3000/other", + "http://other/", + } + for _, s := range bad { + assert.False(t, IsCurrentGiteaSiteURL(s), "bad = %q", s) + } + + setting.AppURL = "http://localhost:3000/" + setting.AppSubURL = "" + assert.True(t, IsCurrentGiteaSiteURL("http://localhost:3000?key=val")) +} diff --git a/routers/common/redirect.go b/routers/common/redirect.go index 9bf2025e1..34044e814 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.IsRiskyRedirectURL(redirect) { + if !httplib.IsCurrentGiteaSiteURL(redirect) { resp.WriteHeader(http.StatusBadRequest) return } diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index da6bef207..ab81740e3 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -133,7 +133,7 @@ func RedirectAfterLogin(ctx *context.Context) { if setting.LandingPageURL == setting.LandingPageLogin { nextRedirectTo = setting.AppSubURL + "/" // do not cycle-redirect to the login page } - ctx.RedirectToFirst(redirectTo, nextRedirectTo) + ctx.RedirectToCurrentSite(redirectTo, nextRedirectTo) } func CheckAutoLogin(ctx *context.Context) bool { @@ -371,7 +371,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) { middleware.DeleteRedirectToCookie(ctx.Resp) if obeyRedirect { - ctx.RedirectToFirst(redirectTo) + ctx.RedirectToCurrentSite(redirectTo) } return redirectTo } @@ -808,7 +808,7 @@ func handleAccountActivation(ctx *context.Context, user *user_model.User) { ctx.Flash.Success(ctx.Tr("auth.account_activated")) if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 { middleware.DeleteRedirectToCookie(ctx.Resp) - ctx.RedirectToFirst(redirectTo) + ctx.RedirectToCurrentSite(redirectTo) return } diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index d5ca7397f..3189d1372 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -1157,7 +1157,7 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 { middleware.DeleteRedirectToCookie(ctx.Resp) - ctx.RedirectToFirst(redirectTo) + ctx.RedirectToCurrentSite(redirectTo) return } diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index c9e038604..3af8b7edf 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -314,7 +314,7 @@ func MustChangePasswordPost(ctx *context.Context) { if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) { middleware.DeleteRedirectToCookie(ctx.Resp) - ctx.RedirectToFirst(redirectTo) + ctx.RedirectToCurrentSite(redirectTo) return } diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 7f9bf3210..0490feb62 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -371,7 +371,7 @@ func Action(ctx *context.Context) { return } - ctx.RedirectToFirst(ctx.FormString("redirect_to"), ctx.Repo.RepoLink) + ctx.RedirectToCurrentSite(ctx.FormString("redirect_to"), ctx.Repo.RepoLink) } func acceptOrRejectRepoTransfer(ctx *context.Context, accept bool) error { diff --git a/routers/web/web.go b/routers/web/web.go index fc1432873..3d790d162 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -174,7 +174,7 @@ func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Cont // Redirect to dashboard (or alternate location) if user tries to visit any non-login page. if options.SignOutRequired && ctx.IsSigned && ctx.Req.URL.RequestURI() != "/" { - ctx.RedirectToFirst(ctx.FormString("redirect_to")) + ctx.RedirectToCurrentSite(ctx.FormString("redirect_to")) return } diff --git a/services/context/context_response.go b/services/context/context_response.go index 372b4cb38..d7fd18aca 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -44,14 +44,14 @@ func RedirectToUser(ctx *Base, userName string, redirectUserID int64) { ctx.Redirect(path.Join(setting.AppSubURL, redirectPath), http.StatusTemporaryRedirect) } -// RedirectToFirst redirects to first not empty URL -func (ctx *Context) RedirectToFirst(location ...string) { +// RedirectToCurrentSite redirects to first not empty URL which belongs to current site +func (ctx *Context) RedirectToCurrentSite(location ...string) { for _, loc := range location { if len(loc) == 0 { continue } - if httplib.IsRiskyRedirectURL(loc) { + if !httplib.IsCurrentGiteaSiteURL(loc) { continue }