Replace tool.IsMaliciousPath with pathutil.Clean and move IsSameSite to urlutil (#8106)

internal/database/repo_editor.go

@@ -23,7 +23,6 @@ import (
 	"gogs.io/gogs/internal/osutil"
 	"gogs.io/gogs/internal/pathutil"
 	"gogs.io/gogs/internal/process"
-	"gogs.io/gogs/internal/tool"
 )
 
 // BranchAlreadyExists represents an error when branch already exists.
@@ -415,8 +414,10 @@ func (upload *Upload) LocalPath() string {
 
 // NewUpload creates a new upload object.
 func NewUpload(name string, buf []byte, file multipart.File) (_ *Upload, err error) {
-	if tool.IsMaliciousPath(name) {
-		return nil, errors.Newf("malicious path detected: %s", name)
+	// 🚨 SECURITY: Prevent path traversal.
+	name = pathutil.Clean(name)
+	if name == "" {
+		return nil, errors.New("empty file name")
 	}
 
 	upload := &Upload{

internal/route/repo/branch.go

@@ -10,7 +10,7 @@ import (
 
 	"gogs.io/gogs/internal/context"
 	"gogs.io/gogs/internal/database"
-	"gogs.io/gogs/internal/tool"
+	"gogs.io/gogs/internal/urlutil"
 )
 
 const (
@@ -109,7 +109,7 @@ func DeleteBranchPost(c *context.Context) {
 
 	defer func() {
 		redirectTo := c.Query("redirect_to")
-		if !tool.IsSameSiteURLPath(redirectTo) {
+		if !urlutil.IsSameSite(redirectTo) {
 			redirectTo = c.Repo.RepoLink
 		}
 		c.Redirect(redirectTo)

internal/route/repo/repo.go

@@ -17,6 +17,7 @@ import (
 	"gogs.io/gogs/internal/database"
 	"gogs.io/gogs/internal/form"
 	"gogs.io/gogs/internal/tool"
+	"gogs.io/gogs/internal/urlutil"
 )
 
 const (
@@ -260,7 +261,7 @@ func Action(c *context.Context) {
 	}
 
 	redirectTo := c.Query("redirect_to")
-	if !tool.IsSameSiteURLPath(redirectTo) {
+	if !urlutil.IsSameSite(redirectTo) {
 		redirectTo = c.Repo.RepoLink
 	}
 	c.Redirect(redirectTo)

internal/route/user/auth.go

@@ -18,6 +18,7 @@ import (
 	"gogs.io/gogs/internal/email"
 	"gogs.io/gogs/internal/form"
 	"gogs.io/gogs/internal/tool"
+	"gogs.io/gogs/internal/urlutil"
 	"gogs.io/gogs/internal/userutil"
 )
 
@@ -92,7 +93,7 @@ func Login(c *context.Context) {
 	}
 
 	if isSucceed {
-		if tool.IsSameSiteURLPath(redirectTo) {
+		if urlutil.IsSameSite(redirectTo) {
 			c.Redirect(redirectTo)
 		} else {
 			c.RedirectSubpath("/")
@@ -138,7 +139,7 @@ func afterLogin(c *context.Context, u *database.User, remember bool) {
 
 	redirectTo, _ := url.QueryUnescape(c.GetCookie("redirect_to"))
 	c.SetCookie("redirect_to", "", -1, conf.Server.Subpath)
-	if tool.IsSameSiteURLPath(redirectTo) {
+	if urlutil.IsSameSite(redirectTo) {
 		c.Redirect(redirectTo)
 		return
 	}

internal/route/user/profile.go

@@ -9,7 +9,7 @@ import (
 	"gogs.io/gogs/internal/context"
 	"gogs.io/gogs/internal/database"
 	"gogs.io/gogs/internal/route/repo"
-	"gogs.io/gogs/internal/tool"
+	"gogs.io/gogs/internal/urlutil"
 )
 
 const (
@@ -122,7 +122,7 @@ func Action(c *context.Context, puser *context.ParamsUser) {
 	}
 
 	redirectTo := c.Query("redirect_to")
-	if !tool.IsSameSiteURLPath(redirectTo) {
+	if !urlutil.IsSameSite(redirectTo) {
 		redirectTo = puser.HomeURLPath()
 	}
 	c.Redirect(redirectTo)

internal/tool/path.go

@@ -1,19 +0,0 @@
-package tool
-
-import (
-	"path/filepath"
-	"strings"
-)
-
-// 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] != '\\'
-}
-
-// IsMaliciousPath returns true if given path is an absolute path or contains malicious content
-// which has potential to traverse upper level directories.
-func IsMaliciousPath(path string) bool {
-	return filepath.IsAbs(path) || strings.Contains(path, "..")
-}

internal/tool/path_test.go

@@ -1,49 +0,0 @@
-package tool
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func Test_IsSameSiteURLPath(t *testing.T) {
-	tests := []struct {
-		url    string
-		expVal bool
-	}{
-		{url: "//github.com", expVal: false},
-		{url: "http://github.com", expVal: false},
-		{url: "https://github.com", expVal: false},
-		{url: "/\\github.com", expVal: false},
-
-		{url: "/admin", expVal: true},
-		{url: "/user/repo", expVal: true},
-	}
-
-	for _, test := range tests {
-		t.Run(test.url, func(t *testing.T) {
-			assert.Equal(t, test.expVal, IsSameSiteURLPath(test.url))
-		})
-	}
-}
-
-func Test_IsMaliciousPath(t *testing.T) {
-	tests := []struct {
-		path   string
-		expVal bool
-	}{
-		{path: "../../../../../../../../../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", expVal: true},
-		{path: "..\\/..\\/../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", expVal: true},
-		{path: "data/gogs/../../../../../../../../../data/sessions/a/9/a9f0ab6c3ef63dd8", expVal: true},
-		{path: "..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", expVal: true},
-		{path: "data\\gogs\\..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", expVal: true},
-
-		{path: "data/sessions/a/9/a9f0ab6c3ef63dd8", expVal: false},
-		{path: "data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", expVal: false},
-	}
-	for _, test := range tests {
-		t.Run(test.path, func(t *testing.T) {
-			assert.Equal(t, test.expVal, IsMaliciousPath(test.path))
-		})
-	}
-}

internal/urlutil/urlutil.go

@@ -0,0 +1,6 @@
+package urlutil
+
+// IsSameSite returns true if the URL path belongs to the same site.
+func IsSameSite(url string) bool {
+	return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\'
+}

internal/urlutil/urlutil_test.go

@@ -0,0 +1,28 @@
+package urlutil
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIsSameSite(t *testing.T) {
+	tests := []struct {
+		url  string
+		want bool
+	}{
+		{url: "//github.com", want: false},
+		{url: "http://github.com", want: false},
+		{url: "https://github.com", want: false},
+		{url: "/\\github.com", want: false},
+
+		{url: "/admin", want: true},
+		{url: "/user/repo", want: true},
+	}
+
+	for _, test := range tests {
+		t.Run(test.url, func(t *testing.T) {
+			assert.Equal(t, test.want, IsSameSite(test.url))
+		})
+	}
+}