Refactor URL detection (#29960)

"Redirect" functions should only redirect if the target is for current Gitea site.

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 urlIsRelative(s string, u *url.URL) bool {
-func IsRiskyRedirectURL(s string) 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
-	}
-
-	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))
+}

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) {
+func TestIsRelativeURL(t *testing.T) {
-	setting.AppURL = "http://localhost:3000/"
+	defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")()
-	tests := []struct {
+	defer test.MockVariableValue(&setting.AppSubURL, "/sub")()
-		input string
+	rel := []string{
-		want  bool
+		"",
-	}{
+		"foo",
-		{"", false},
+		"/",
-		{"foo", false},
+		"/foo?k=%20#abc",
-		{"/", false},
+	}
-		{"/foo?k=%20#abc", false},
+	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)
+	}
+}
 
-		{"//", true},
+func TestIsCurrentGiteaSiteURL(t *testing.T) {
-		{"\\\\", true},
+	defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")()
-		{"/\\", true},
+	defer test.MockVariableValue(&setting.AppSubURL, "/sub")()
-		{"\\/", true},
+	good := []string{
-		{"mail:a@b.com", true},
+		"?key=val",
-		{"https://test.com", true},
+		"/sub",
-		{setting.AppURL + "/foo", false},
+		"/sub/",
+		"/sub/foo",
+		"/sub/foo/",
+		"http://localhost:3000/sub?key=val",
+		"http://localhost:3000/sub/",
 	}
-	for _, tt := range tests {
+	for _, s := range good {
-		t.Run(tt.input, func(t *testing.T) {
+		assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s)
-			assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input))
-		})
 	}
+	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"))
 }

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
 	}

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
 	}
 

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
 		}
 

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
 	}
 

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 {

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
 		}
 

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
+// RedirectToCurrentSite redirects to first not empty URL which belongs to current site
-func (ctx *Context) RedirectToFirst(location ...string) {
+func (ctx *Context) RedirectToCurrentSite(location ...string) {
 	for _, loc := range location {
 		if len(loc) == 0 {
 			continue
 		}
 
-		if httplib.IsRiskyRedirectURL(loc) {
+		if !httplib.IsCurrentGiteaSiteURL(loc) {
 			continue
 		}