Refactor URL detection

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):https://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
  }
+ 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)
- if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(s), strings.ToLower(setting.AppURL))) {
- return true
- }
+ return err == nil && urlIsRelative(s, u)
+}
 
- return false
+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) {
- setting.AppURL = "http:https://localhost:3000/"
- tests := []struct {
- input string
- want bool
- }{
- {"", false},
- {"foo", false},
- {"/", false},
- {"/foo?k=%20#abc", false},
+func TestIsRelativeURL(t *testing.T) {
+ defer test.MockVariableValue(&setting.AppURL, "http:https://localhost:3000/sub/")()
+ defer test.MockVariableValue(&setting.AppSubURL, "/sub")()
+ rel := []string{
+ "",
+ "foo",
+ "/",
+ "/foo?k=%20#abc",
+ }
+ for _, s := range rel {
+ assert.True(t, IsRelativeURL(s), "rel = %q", s)
+ }
+ abs := []string{
+ "//",
+ "\\\\",
+ "/\\",
+ "\\/",
+ "mailto:[email protected]",
+ "https://test.com",
+ }
+ for _, s := range abs {
+ assert.False(t, IsRelativeURL(s), "abs = %q", s)
+ }
+}
 
- {"//", true},
- {"\\\\", true},
- {"/\\", true},
- {"\\/", true},
- {"mail:[email protected]", true},
- {"https://test.com", true},
- {setting.AppURL + "/foo", false},
- }
- for _, tt := range tests {
- t.Run(tt.input, func(t *testing.T) {
- assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input))
- })
+func TestIsCurrentGiteaSiteURL(t *testing.T) {
+ defer test.MockVariableValue(&setting.AppURL, "http:https://localhost:3000/sub/")()
+ defer test.MockVariableValue(&setting.AppSubURL, "/sub")()
+ good := []string{
+ "?key=val",
+ "/sub",
+ "/sub/",
+ "/sub/foo",
+ "/sub/foo/",
+ "http:https://localhost:3000/sub?key=val",
+ "http:https://localhost:3000/sub/",
  }
+ for _, s := range good {
+ assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s)
+ }
+ bad := []string{
+ "/",
+ "//",
+ "\\\\",
+ "/foo",
+ "http:https://localhost:3000/sub/..",
+ "http:https://localhost:3000/other",
+ "http:https://other/",
+ }
+ for _, s := range bad {
+ assert.False(t, IsCurrentGiteaSiteURL(s), "bad = %q", s)
+ }
+
+ setting.AppURL = "http:https://localhost:3000/"
+ setting.AppSubURL = ""
+ assert.True(t, IsCurrentGiteaSiteURL("http:https://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
-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
  }