[PORT] drop utils.IsExternalURL (and expand IsRiskyRedirectURL tests)…

… (#3167) Related to #2773 Related to Refactor URL detection [gitea#29960](go-gitea/gitea#29960) Related to Refactor external URL detection [gitea#29973](go-gitea/gitea#29973) I added a bunch of tests to `httplib.TestIsRiskyRedirectURL` and some cases should be better handled (however it is not an easy task). I also ported the removal of `utils.IsExternalURL`, since it prevents duplicated (subtle) code. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3167 Reviewed-by: Earl Warren <[email protected]> Reviewed-by: Gusted <[email protected]> Co-authored-by: oliverpool <[email protected]> Co-committed-by: oliverpool <[email protected]>
DennisRasey · Apr 15, 2024 · 16879b0 · 16879b0
1 parent 20c0292
commit 16879b0
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 74 deletions.
diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go
@@ -7,28 +7,113 @@ 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},
+		{"?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},
+		{"/\\", true},
+		{"\\/", true},
+		{"mail:[email protected]", true},
+		{"https://test.com", true},
+		{"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:[email protected]", true},
 		{"https://test.com", true},
-		{setting.AppURL + "/foo", false},
+		{"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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"