diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 72033b120..2842edd51 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -7,20 +7,39 @@ 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/" + defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")() + defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + tests := []struct { input string want bool }{ {"", false}, {"foo", false}, - {"/", false}, + {"./", false}, + {"?key=val", false}, + {"/sub/", false}, + {"http://localhost:3000/sub/", false}, + {"/sub/foo", false}, + {"http://localhost:3000/sub/foo", false}, + {"http://localhost:3000/sub/test?param=false", false}, + // FIXME: should probably be true (would requires resolving references using setting.appURL.ResolveReference(u)) + {"/sub/../", false}, + {"http://localhost:3000/sub/../", false}, + {"/sUb/", false}, + {"http://localhost:3000/sUb/foo", false}, + {"/sub", false}, {"/foo?k=%20#abc", false}, + {"/", false}, + {"a/", false}, + {"test?param=false", false}, + {"/hey/hey/hey#3244", false}, {"//", true}, {"\\\\", true}, @@ -28,7 +47,73 @@ func TestIsRiskyRedirectURL(t *testing.T) { {"\\/", true}, {"mail:a@b.com", true}, {"https://test.com", true}, - {setting.AppURL + "/foo", false}, + {"http://localhost:3000/foo", true}, + {"http://localhost:3000/sub", true}, + {"http://localhost:3000/sub?key=val", true}, + {"https://example.com/", true}, + {"//example.com", true}, + {"http://example.com", true}, + {"http://localhost:3000/test?param=false", true}, + {"//localhost:3000/test?param=false", true}, + {"://missing protocol scheme", true}, + // FIXME: should probably be false + {"//localhost:3000/sub/test?param=false", true}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input)) + }) + } +} + +func TestIsRiskyRedirectURLWithoutSubURL(t *testing.T) { + defer test.MockVariableValue(&setting.AppURL, "https://next.forgejo.org/")() + defer test.MockVariableValue(&setting.AppSubURL, "")() + + tests := []struct { + input string + want bool + }{ + {"", false}, + {"foo", false}, + {"./", false}, + {"?key=val", false}, + {"/sub/", false}, + {"https://next.forgejo.org/sub/", false}, + {"/sub/foo", false}, + {"https://next.forgejo.org/sub/foo", false}, + {"https://next.forgejo.org/sub/test?param=false", false}, + {"https://next.forgejo.org/sub/../", false}, + {"/sub/../", false}, + {"/sUb/", false}, + {"https://next.forgejo.org/sUb/foo", false}, + {"/sub", false}, + {"/foo?k=%20#abc", false}, + {"/", false}, + {"a/", false}, + {"test?param=false", false}, + {"/hey/hey/hey#3244", false}, + {"https://next.forgejo.org/test?param=false", false}, + {"https://next.forgejo.org/foo", false}, + {"https://next.forgejo.org/sub", false}, + {"https://next.forgejo.org/sub?key=val", false}, + + {"//", true}, + {"\\\\", true}, + {"/\\", true}, + {"\\/", true}, + {"mail:a@b.com", true}, + {"https://test.com", true}, + {"https://example.com/", true}, + {"//example.com", true}, + {"http://example.com", true}, + {"://missing protocol scheme", true}, + {"https://forgejo.org", true}, + {"https://example.org?url=https://next.forgejo.org", true}, + // FIXME: should probably be false + {"https://next.forgejo.org", true}, + {"//next.forgejo.org/test?param=false", true}, + {"//next.forgejo.org/sub/test?param=false", true}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { diff --git a/routers/utils/utils.go b/routers/utils/utils.go index 1f4d11fd3..3035073d5 100644 --- a/routers/utils/utils.go +++ b/routers/utils/utils.go @@ -5,26 +5,10 @@ package utils import ( "html" - "net/url" "strings" - - "code.gitea.io/gitea/modules/setting" ) // SanitizeFlashErrorString will sanitize a flash error string func SanitizeFlashErrorString(x string) string { return strings.ReplaceAll(html.EscapeString(x), "\n", "
") } - -// IsExternalURL checks if rawURL points to an external URL like http://example.com -func IsExternalURL(rawURL string) bool { - parsed, err := url.Parse(rawURL) - if err != nil { - return true - } - appURL, _ := url.Parse(setting.AppURL) - if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(appURL.Host, "www.", "", 1) { - return true - } - return false -} diff --git a/routers/utils/utils_test.go b/routers/utils/utils_test.go index 440aad87c..6e7f3c33c 100644 --- a/routers/utils/utils_test.go +++ b/routers/utils/utils_test.go @@ -5,47 +5,8 @@ package utils import ( "testing" - - "code.gitea.io/gitea/modules/setting" - - "github.com/stretchr/testify/assert" ) -func TestIsExternalURL(t *testing.T) { - setting.AppURL = "https://try.gitea.io/" - type test struct { - Expected bool - RawURL string - } - newTest := func(expected bool, rawURL string) test { - return test{Expected: expected, RawURL: rawURL} - } - for _, test := range []test{ - newTest(false, - "https://try.gitea.io"), - newTest(true, - "https://example.com/"), - newTest(true, - "//example.com"), - newTest(true, - "http://example.com"), - newTest(false, - "a/"), - newTest(false, - "https://try.gitea.io/test?param=false"), - newTest(false, - "test?param=false"), - newTest(false, - "//try.gitea.io/test?param=false"), - newTest(false, - "/hey/hey/hey#3244"), - newTest(true, - "://missing protocol scheme"), - } { - assert.Equal(t, test.Expected, IsExternalURL(test.RawURL)) - } -} - func TestSanitizeFlashErrorString(t *testing.T) { tests := []struct { name string diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 1c55256db..50fd760d8 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -18,6 +18,7 @@ import ( "code.gitea.io/gitea/modules/auth/password" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/eventsource" + "code.gitea.io/gitea/modules/httplib" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/session" @@ -26,7 +27,6 @@ import ( "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/modules/web/middleware" - "code.gitea.io/gitea/routers/utils" auth_service "code.gitea.io/gitea/services/auth" "code.gitea.io/gitea/services/auth/source/oauth2" "code.gitea.io/gitea/services/context" @@ -372,16 +372,15 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe return setting.AppSubURL + "/" } - if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) { + redirectTo := ctx.GetSiteCookie("redirect_to") + if redirectTo != "" { middleware.DeleteRedirectToCookie(ctx.Resp) - if obeyRedirect { - ctx.RedirectToFirst(redirectTo) - } - return redirectTo } - if obeyRedirect { - ctx.Redirect(setting.AppSubURL + "/") + return ctx.RedirectToFirst(redirectTo) + } + if !httplib.IsRiskyRedirectURL(redirectTo) { + return redirectTo } return setting.AppSubURL + "/" } diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index c9e038604..d47aeb5e9 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/modules/web/middleware" - "code.gitea.io/gitea/routers/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/forms" "code.gitea.io/gitea/services/mailer" @@ -312,11 +311,9 @@ func MustChangePasswordPost(ctx *context.Context) { log.Trace("User updated password: %s", ctx.Doer.Name) - if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) { + redirectTo := ctx.GetSiteCookie("redirect_to") + if redirectTo != "" { middleware.DeleteRedirectToCookie(ctx.Resp) - ctx.RedirectToFirst(redirectTo) - return } - - ctx.Redirect(setting.AppSubURL + "/") + ctx.RedirectToFirst(redirectTo) } diff --git a/services/context/context_response.go b/services/context/context_response.go index 372b4cb38..2f2d7b0e1 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -44,8 +44,10 @@ 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) { +// RedirectToFirst redirects to first not empty URL which likely belongs to current site. +// If no suitable redirection is found, it redirects to the home. +// It returns the location it redirected to. +func (ctx *Context) RedirectToFirst(location ...string) string { for _, loc := range location { if len(loc) == 0 { continue @@ -56,10 +58,11 @@ func (ctx *Context) RedirectToFirst(location ...string) { } ctx.Redirect(loc) - return + return loc } ctx.Redirect(setting.AppSubURL + "/") + return setting.AppSubURL + "/" } const tplStatus500 base.TplName = "status/500"