From 1cdeef2ce8a64ff4c68ff25a72136e9516bc2361 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Fri, 23 Jan 2026 21:13:27 -0500 Subject: [PATCH] Replace tool.IsMaliciousPath with pathutil.Clean and move IsSameSite to urlutil (#8106) --- internal/database/repo_editor.go | 7 +++-- internal/route/repo/branch.go | 4 +-- internal/route/repo/repo.go | 3 +- internal/route/user/auth.go | 5 ++-- internal/route/user/profile.go | 4 +-- internal/tool/path.go | 19 ------------- internal/tool/path_test.go | 49 -------------------------------- internal/urlutil/urlutil.go | 6 ++++ internal/urlutil/urlutil_test.go | 28 ++++++++++++++++++ 9 files changed, 47 insertions(+), 78 deletions(-) delete mode 100644 internal/tool/path.go delete mode 100644 internal/tool/path_test.go create mode 100644 internal/urlutil/urlutil.go create mode 100644 internal/urlutil/urlutil_test.go diff --git a/internal/database/repo_editor.go b/internal/database/repo_editor.go index f326eaa65..44bc64e52 100644 --- a/internal/database/repo_editor.go +++ b/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{ diff --git a/internal/route/repo/branch.go b/internal/route/repo/branch.go index a8ad7e744..ce4ce66d7 100644 --- a/internal/route/repo/branch.go +++ b/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) diff --git a/internal/route/repo/repo.go b/internal/route/repo/repo.go index caf9d1151..a30f5e68a 100644 --- a/internal/route/repo/repo.go +++ b/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) diff --git a/internal/route/user/auth.go b/internal/route/user/auth.go index b1f0b24e6..e562305fc 100644 --- a/internal/route/user/auth.go +++ b/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 } diff --git a/internal/route/user/profile.go b/internal/route/user/profile.go index b4f18a1ef..cb461d598 100644 --- a/internal/route/user/profile.go +++ b/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) diff --git a/internal/tool/path.go b/internal/tool/path.go deleted file mode 100644 index bb72c14c2..000000000 --- a/internal/tool/path.go +++ /dev/null @@ -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, "..") -} diff --git a/internal/tool/path_test.go b/internal/tool/path_test.go deleted file mode 100644 index be1c675c5..000000000 --- a/internal/tool/path_test.go +++ /dev/null @@ -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)) - }) - } -} diff --git a/internal/urlutil/urlutil.go b/internal/urlutil/urlutil.go new file mode 100644 index 000000000..afd378d82 --- /dev/null +++ b/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] != '\\' +} diff --git a/internal/urlutil/urlutil_test.go b/internal/urlutil/urlutil_test.go new file mode 100644 index 000000000..f93843981 --- /dev/null +++ b/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)) + }) + } +}