routes: fix open redirect vulnerability (#5355)

Reported by @cezar97.

gogs.go

@@ -16,7 +16,7 @@ import (
 	"github.com/gogs/gogs/pkg/setting"
 )
 
-const APP_VER = "0.11.66.0928"
+const APP_VER = "0.11.67.0928"
 
 func init() {
 	setting.AppVer = APP_VER

pkg/tool/path.go

@@ -0,0 +1,12 @@
+// Copyright 2018 The Gogs Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package tool
+
+// IsSameSiteURLPath returns true if the URL path belongs to the same site, false otherwise.
+// False: //url, http://url, /\url
+// True: /url
+func IsSameSiteURLPath(url string) bool {
+	return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\'
+}

pkg/tool/path_test.go

@@ -0,0 +1,32 @@
+// Copyright 2018 The Gogs Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package tool
+
+import (
+	"testing"
+
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func Test_IsSameSiteURLPath(t *testing.T) {
+	Convey("Check if a path belongs to the same site", t, func() {
+		testCases := []struct {
+			url    string
+			expect bool
+		}{
+			{"//github.com", false},
+			{"http://github.com", false},
+			{"https://github.com", false},
+			{"/\\github.com", false},
+
+			{"/admin", true},
+			{"/user/repo", true},
+		}
+
+		for _, tc := range testCases {
+			So(IsSameSiteURLPath(tc.url), ShouldEqual, tc.expect)
+		}
+	})
+}

routes/repo/branch.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/gogs/gogs/models"
 	"github.com/gogs/gogs/pkg/context"
+	"github.com/gogs/gogs/pkg/tool"
 )
 
 const (
@@ -112,7 +113,7 @@ func DeleteBranchPost(c *context.Context) {
 
 	defer func() {
 		redirectTo := c.Query("redirect_to")
-		if len(redirectTo) == 0 {
+		if !tool.IsSameSiteURLPath(redirectTo) {
 			redirectTo = c.Repo.RepoLink
 		}
 		c.Redirect(redirectTo)

routes/repo/repo.go

@@ -242,7 +242,7 @@ func Action(c *context.Context) {
 		err = models.StarRepo(c.User.ID, c.Repo.Repository.ID, false)
 	case "desc": // FIXME: this is not used
 		if !c.Repo.IsOwner() {
-			c.Error(404)
+			c.NotFound()
 			return
 		}
 
@@ -252,12 +252,12 @@ func Action(c *context.Context) {
 	}
 
 	if err != nil {
-		c.Handle(500, fmt.Sprintf("Action (%s)", c.Params(":action")), err)
+		c.ServerError(fmt.Sprintf("Action (%s)", c.Params(":action")), err)
 		return
 	}
 
 	redirectTo := c.Query("redirect_to")
-	if len(redirectTo) == 0 {
+	if !tool.IsSameSiteURLPath(redirectTo) {
 		redirectTo = c.Repo.RepoLink
 	}
 	c.Redirect(redirectTo)

routes/user/auth.go

@@ -17,6 +17,7 @@ import (
 	"github.com/gogs/gogs/pkg/form"
 	"github.com/gogs/gogs/pkg/mailer"
 	"github.com/gogs/gogs/pkg/setting"
+	"github.com/gogs/gogs/pkg/tool"
 )
 
 const (
@@ -72,13 +73,6 @@ func AutoLogin(c *context.Context) (bool, error) {
 	return true, nil
 }
 
-// isValidRedirect returns false if the URL does not redirect to same site.
-// False: //url, http://url, /\url
-// True: /url
-func isValidRedirect(url string) bool {
-	return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\'
-}
-
 func Login(c *context.Context) {
 	c.Title("sign_in")
 
@@ -97,7 +91,7 @@ func Login(c *context.Context) {
 	}
 
 	if isSucceed {
-		if isValidRedirect(redirectTo) {
+		if tool.IsSameSiteURLPath(redirectTo) {
 			c.Redirect(redirectTo)
 		} else {
 			c.SubURLRedirect("/")
@@ -143,7 +137,7 @@ func afterLogin(c *context.Context, u *models.User, remember bool) {
 
 	redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to"))
 	c.SetCookie("redirect_to", "", -1, setting.AppSubURL)
-	if isValidRedirect(redirectTo) {
+	if tool.IsSameSiteURLPath(redirectTo) {
 		c.Redirect(redirectTo)
 		return
 	}

routes/user/profile.go

@@ -15,6 +15,7 @@ import (
 	"github.com/gogs/gogs/models/errors"
 	"github.com/gogs/gogs/pkg/context"
 	"github.com/gogs/gogs/pkg/setting"
+	"github.com/gogs/gogs/pkg/tool"
 	"github.com/gogs/gogs/routes/repo"
 )
 
@@ -157,12 +158,12 @@ func Action(c *context.Context) {
 	}
 
 	if err != nil {
-		c.Handle(500, fmt.Sprintf("Action (%s)", c.Params(":action")), err)
+		c.ServerError(fmt.Sprintf("Action (%s)", c.Params(":action")), err)
 		return
 	}
 
 	redirectTo := c.Query("redirect_to")
-	if len(redirectTo) == 0 {
+	if !tool.IsSameSiteURLPath(redirectTo) {
 		redirectTo = u.HomeLink()
 	}
 	c.Redirect(redirectTo)

templates/.VERSION

@@ -1 +1 @@
-0.11.66.0928
+0.11.67.0928