From ca4107dc96e5110dec6a8732e7caa3b071222dcf Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 22 Mar 2024 04:32:40 +0800 Subject: [PATCH] Refactor external URL detection (#29973) Follow #29960, `IsExternalURL` is not needed anymore. Add some tests for `RedirectToCurrentSite` --- modules/httplib/url.go | 8 ++++--- modules/httplib/url_test.go | 5 ++++ routers/utils/utils.go | 16 ------------- routers/utils/utils_test.go | 39 -------------------------------- routers/web/auth/auth.go | 4 ++-- routers/web/auth/password.go | 3 +-- services/context/context_test.go | 27 ++++++++++++++++++++++ 7 files changed, 40 insertions(+), 62 deletions(-) diff --git a/modules/httplib/url.go b/modules/httplib/url.go index b679b4450..903799cb6 100644 --- a/modules/httplib/url.go +++ b/modules/httplib/url.go @@ -32,9 +32,11 @@ func IsCurrentGiteaSiteURL(s string) bool { return false } if u.Path != "" { - u.Path = "/" + util.PathJoinRelX(u.Path) - if !strings.HasSuffix(u.Path, "/") { - u.Path += "/" + cleanedPath := util.PathJoinRelX(u.Path) + if cleanedPath == "" || cleanedPath == "." { + u.Path = "/" + } else { + u.Path += "/" + cleanedPath + "/" } } if urlIsRelative(s, u) { diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 9b7b24229..9bf09bcf2 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -53,6 +53,8 @@ func TestIsCurrentGiteaSiteURL(t *testing.T) { assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s) } bad := []string{ + ".", + "foo", "/", "//", "\\\\", @@ -67,5 +69,8 @@ func TestIsCurrentGiteaSiteURL(t *testing.T) { 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")) } 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 ab81740e3..8b5cd986b 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -17,6 +17,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" @@ -25,7 +26,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" @@ -368,7 +368,7 @@ 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) { + if redirectTo := ctx.GetSiteCookie("redirect_to"); redirectTo != "" && httplib.IsCurrentGiteaSiteURL(redirectTo) { middleware.DeleteRedirectToCookie(ctx.Resp) if obeyRedirect { ctx.RedirectToCurrentSite(redirectTo) diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 3af8b7edf..f6b76c1ff 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,7 +311,7 @@ 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) { + if redirectTo := ctx.GetSiteCookie("redirect_to"); redirectTo != "" { middleware.DeleteRedirectToCookie(ctx.Resp) ctx.RedirectToCurrentSite(redirectTo) return diff --git a/services/context/context_test.go b/services/context/context_test.go index 033ce2ef0..984593398 100644 --- a/services/context/context_test.go +++ b/services/context/context_test.go @@ -6,9 +6,11 @@ package context import ( "net/http" "net/http/httptest" + "net/url" "testing" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" ) @@ -22,3 +24,28 @@ func TestRemoveSessionCookieHeader(t *testing.T) { assert.Len(t, w.Header().Values("Set-Cookie"), 1) assert.Contains(t, "other=bar", w.Header().Get("Set-Cookie")) } + +func TestRedirectToCurrentSite(t *testing.T) { + defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")() + defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + cases := []struct { + location string + want string + }{ + {"/", "/sub/"}, + {"http://localhost:3000/sub?k=v", "http://localhost:3000/sub?k=v"}, + {"http://other", "/sub/"}, + } + for _, c := range cases { + t.Run(c.location, func(t *testing.T) { + req := &http.Request{URL: &url.URL{Path: "/"}} + resp := httptest.NewRecorder() + base, baseCleanUp := NewBaseContext(resp, req) + defer baseCleanUp() + ctx := NewWebContext(base, nil, nil) + ctx.RedirectToCurrentSite(c.location) + redirect := test.RedirectURL(resp) + assert.Equal(t, c.want, redirect) + }) + } +}