Refactor external URL detection (#29973)

Follow #29960, `IsExternalURL` is not needed anymore. 
Add some tests for `RedirectToCurrentSite`

7 files changed, 40 insertions, 62 deletions

diff --git a/modules/httplib/url.go b/modules/httplib/url.go
index b679b44500..903799cb68 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 9b7b242298..9bf09bcf2f 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 1f4d11fd3c..3035073d5c 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", "<br>")
 }
-
-// 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 440aad87c6..6e7f3c33cd 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 ab81740e3f..8b5cd986b8 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 3af8b7edf2..f6b76c1ffd 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 033ce2ef0a..984593398d 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)
+		})
+	}
+}