rename to PathJoin (instead of SafeXxx), add tests, add comments

Author: wxiaoguang

models/git/lfs_lock.go

@@ -34,7 +34,7 @@ func init() {
 
 // BeforeInsert is invoked from XORM before inserting an object of this type.
 func (l *LFSLock) BeforeInsert() {
-	l.Path = util.SafePathRel(l.Path)
+	l.Path = util.PathJoinRel(l.Path)
 }
 
 // CreateLFSLock creates a new lock.
@@ -49,7 +49,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
 		return nil, err
 	}
 
-	lock.Path = util.SafePathRel(lock.Path)
+	lock.Path = util.PathJoinRel(lock.Path)
 	lock.RepoID = repo.ID
 
 	l, err := GetLFSLock(dbCtx, repo, lock.Path)
@@ -69,7 +69,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
 
 // GetLFSLock returns release by given path.
 func GetLFSLock(ctx context.Context, repo *repo_model.Repository, path string) (*LFSLock, error) {
-	path = util.SafePathRel(path)
+	path = util.PathJoinRel(path)
 	rel := &LFSLock{RepoID: repo.ID}
 	has, err := db.GetEngine(ctx).Where("lower(path) = ?", strings.ToLower(path)).Get(rel)
 	if err != nil {

modules/options/base.go

@@ -98,7 +98,7 @@ func joinLocalPaths(baseDirs []string, subDir string, elems ...string) (paths []
 	copy(abs[2:], elems)
 	for _, baseDir := range baseDirs {
 		abs[0] = mustLocalPathAbs(baseDir)
-		paths = append(paths, util.SafeFilePathAbs(abs...))
+		paths = append(paths, util.FilePathJoinAbs(abs...))
 	}
 	return paths
 }

modules/options/static.go

@@ -58,7 +58,7 @@ func fileFromOptionsDir(elems ...string) ([]byte, error) {
 		return data, nil
 	}
 
-	f, err := Assets.Open(util.SafePathRelX(elems...))
+	f, err := Assets.Open(util.PathJoinRelX(elems...))
 	if err != nil {
 		return nil, err
 	}

modules/public/public.go

@@ -93,7 +93,7 @@ func setWellKnownContentType(w http.ResponseWriter, file string) {
 
 func (opts *Options) handle(w http.ResponseWriter, req *http.Request, fs http.FileSystem, file string) bool {
 	// actually, fs (http.FileSystem) is designed to be a safe interface, relative paths won't bypass its parent directory, it's also fine to do a clean here
-	f, err := fs.Open(util.SafePathRelX(file))
+	f, err := fs.Open(util.PathJoinRelX(file))
 	if err != nil {
 		if os.IsNotExist(err) {
 			return false

modules/storage/local.go

@@ -64,7 +64,7 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 }
 
 func (l *LocalStorage) buildLocalPath(p string) string {
-	return util.SafeFilePathAbs(l.dir, p)
+	return util.FilePathJoinAbs(l.dir, p)
 }
 
 // Open a file

modules/storage/minio.go

@@ -121,7 +121,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 }
 
 func (m *MinioStorage) buildMinioPath(p string) string {
-	return util.SafePathRelX(m.basePath, p)
+	return util.PathJoinRelX(m.basePath, p)
 }
 
 // Open open a file

modules/util/path.go

@@ -15,8 +15,9 @@ import (
 	"strings"
 )
 
-// SafePathRel joins the path elements into a single path, each element is cleaned by path.Clean separately.
+// PathJoinRel joins the path elements into a single path, each element is cleaned by path.Clean separately.
 // It only returns the following values (like path.Join), any redundant part (empty, relative dots, slashes) is removed.
+// It's caller's duty to make every element not bypass its own directly level, to avoid security issues.
 //
 //	empty => ``
 //	`` => ``
@@ -26,7 +27,7 @@ import (
 //	`foo\..\bar` => `foo\..\bar`
 //	{`foo`, ``, `bar`} => `foo/bar`
 //	{`foo`, `..`, `bar`} => `foo/bar`
-func SafePathRel(elem ...string) string {
+func PathJoinRel(elem ...string) string {
 	elems := make([]string, len(elem))
 	for i, e := range elem {
 		if e == "" {
@@ -44,34 +45,36 @@ func SafePathRel(elem ...string) string {
 	}
 }
 
-// SafePathRelX joins the path elements into a single path like SafePathRel,
+// PathJoinRelX joins the path elements into a single path like PathJoinRel,
 // and covert all backslashes to slashes. (X means "extended", also means the combination of `\` and `/`).
-// It returns similar results as SafePathRel except:
+// It's caller's duty to make every element not bypass its own directly level, to avoid security issues.
+// It returns similar results as PathJoinRel except:
 //
 //	`foo\..\bar` => `bar`  (because it's processed as `foo/../bar`)
 //
 // All backslashes are handled as slashes, the result only contains slashes.
-func SafePathRelX(elem ...string) string {
+func PathJoinRelX(elem ...string) string {
 	elems := make([]string, len(elem))
 	for i, e := range elem {
 		if e == "" {
 			continue
 		}
 		elems[i] = path.Clean("/" + strings.ReplaceAll(e, "\\", "/"))
 	}
-	return SafePathRel(elems...)
+	return PathJoinRel(elems...)
 }
 
 const pathSeparator = string(os.PathSeparator)
 
-// SafeFilePathAbs joins the path elements into a single file path, each element is cleaned by filepath.Clean separately.
+// FilePathJoinAbs joins the path elements into a single file path, each element is cleaned by filepath.Clean separately.
 // All slashes/backslashes are converted to path separators before cleaning, the result only contains path separators.
 // The first element must be an absolute path, caller should prepare the base path.
-// Like SafePathRel, any redundant part (empty, relative dots, slashes) is removed.
+// It's caller's duty to make every element not bypass its own directly level, to avoid security issues.
+// Like PathJoinRel, any redundant part (empty, relative dots, slashes) is removed.
 //
 //	{`/foo`, ``, `bar`} => `/foo/bar`
 //	{`/foo`, `..`, `bar`} => `/foo/bar`
-func SafeFilePathAbs(elem ...string) string {
+func FilePathJoinAbs(elem ...string) string {
 	elems := make([]string, len(elem))
 
 	// POISX filesystem can have `\` in file names. Windows: `\` and `/` are both used for path separators

modules/util/path_test.go

@@ -151,9 +151,10 @@ func TestCleanPath(t *testing.T) {
 		{[]string{`a\..\b`}, `a\..\b`},
 		{[]string{`a`, ``, `b`}, `a/b`},
 		{[]string{`a`, `..`, `b`}, `a/b`},
+		{[]string{`lfs`, `repo/..`, `user/../path`}, `lfs/path`},
 	}
 	for _, c := range cases {
-		assert.Equal(t, c.expected, SafePathRel(c.elems...), "case: %v", c.elems)
+		assert.Equal(t, c.expected, PathJoinRel(c.elems...), "case: %v", c.elems)
 	}
 
 	cases = []struct {
@@ -169,9 +170,10 @@ func TestCleanPath(t *testing.T) {
 		{[]string{`a\..\b`}, `b`},
 		{[]string{`a`, ``, `b`}, `a/b`},
 		{[]string{`a`, `..`, `b`}, `a/b`},
+		{[]string{`lfs`, `repo/..`, `user/../path`}, `lfs/path`},
 	}
 	for _, c := range cases {
-		assert.Equal(t, c.expected, SafePathRelX(c.elems...), "case: %v", c.elems)
+		assert.Equal(t, c.expected, PathJoinRelX(c.elems...), "case: %v", c.elems)
 	}
 
 	// for POSIX only, but the result is similar on Windows, because the first element must be an absolute path
@@ -187,6 +189,7 @@ func TestCleanPath(t *testing.T) {
 			{[]string{`C:\a/..\b`}, `C:\b`},
 			{[]string{`C:\a`, ``, `b`}, `C:\a\b`},
 			{[]string{`C:\a`, `..`, `b`}, `C:\a\b`},
+			{[]string{`C:\lfs`, `repo/..`, `user/../path`}, `C:\lfs\path`},
 		}
 	} else {
 		cases = []struct {
@@ -200,9 +203,10 @@ func TestCleanPath(t *testing.T) {
 			{[]string{`/a\..\b`}, `/b`},
 			{[]string{`/a`, ``, `b`}, `/a/b`},
 			{[]string{`/a`, `..`, `b`}, `/a/b`},
+			{[]string{`/lfs`, `repo/..`, `user/../path`}, `/lfs/path`},
 		}
 	}
 	for _, c := range cases {
-		assert.Equal(t, c.expected, SafeFilePathAbs(c.elems...), "case: %v", c.elems)
+		assert.Equal(t, c.expected, FilePathJoinAbs(c.elems...), "case: %v", c.elems)
 	}
 }

routers/web/base.go

@@ -45,7 +45,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 				routing.UpdateFuncInfo(req.Context(), funcInfo)
 
 				rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-				rPath = util.SafePathRelX(rPath)
+				rPath = util.PathJoinRelX(rPath)
 
 				u, err := objStore.URL(rPath, path.Base(rPath))
 				if err != nil {
@@ -81,7 +81,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 			routing.UpdateFuncInfo(req.Context(), funcInfo)
 
 			rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-			rPath = util.SafePathRelX(rPath)
+			rPath = util.PathJoinRelX(rPath)
 			if rPath == "" || rPath == "." {
 				http.Error(w, "file not found", http.StatusNotFound)
 				return

routers/web/repo/editor.go

@@ -726,7 +726,7 @@ func UploadFilePost(ctx *context.Context) {
 
 func cleanUploadFileName(name string) string {
 	// Rebase the filename
-	name = util.SafePathRel(name)
+	name = util.PathJoinRel(name)
 	// Git disallows any filenames to have a .git directory in them.
 	for _, part := range strings.Split(name, "/") {
 		if strings.ToLower(part) == ".git" {

routers/web/repo/lfs.go

@@ -207,7 +207,7 @@ func LFSLockFile(ctx *context.Context) {
 		ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
 		return
 	}
-	lockPath = util.SafePathRel(lockPath)
+	lockPath = util.PathJoinRel(lockPath)
 	if len(lockPath) == 0 {
 		ctx.Flash.Error(ctx.Tr("repo.settings.lfs_invalid_locking_path", originalPath))
 		ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")

services/migrations/gitea_uploader.go

@@ -866,7 +866,7 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
 			}
 
 			// SECURITY: The TreePath must be cleaned! use relative path
-			comment.TreePath = util.SafePathRel(comment.TreePath)
+			comment.TreePath = util.PathJoinRel(comment.TreePath)
 
 			var patch string
 			reader, writer := io.Pipe()

services/packages/container/blob_uploader.go

@@ -31,7 +31,7 @@ type BlobUploader struct {
 }
 
 func buildFilePath(id string) string {
-	return util.SafeFilePathAbs(setting.Packages.ChunkedUploadPath, id)
+	return util.FilePathJoinAbs(setting.Packages.ChunkedUploadPath, id)
 }
 
 // NewBlobUploader creates a new blob uploader for the given id

services/repository/files/file.go

@@ -129,7 +129,7 @@ func GetAuthorAndCommitterUsers(author, committer *IdentityOptions, doer *user_m
 // CleanUploadFileName Trims a filename and returns empty string if it is a .git directory
 func CleanUploadFileName(name string) string {
 	// Rebase the filename
-	name = util.SafePathRel(name)
+	name = util.PathJoinRel(name)
 	// Git disallows any filenames to have a .git directory in them.
 	for _, part := range strings.Split(name, "/") {
 		if strings.ToLower(part) == ".git" {