fix

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.CleanPath(l.Path)
+	l.Path = util.SafePathRel(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.CleanPath(lock.Path)
+	lock.Path = util.SafePathRel(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.CleanPath(path)
+	path = util.SafePathRel(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

@@ -16,27 +16,27 @@ import (
 
 // Locale reads the content of a specific locale from static/bindata or custom path.
 func Locale(name string) ([]byte, error) {
-	return fileFromDir(path.Join("locale", util.CleanPath(name)))
+	return fileFromDir(path.Join("locale", util.SafePathRel(name)))
 }
 
 // Readme reads the content of a specific readme from static/bindata or custom path.
 func Readme(name string) ([]byte, error) {
-	return fileFromDir(path.Join("readme", util.CleanPath(name)))
+	return fileFromDir(path.Join("readme", util.SafePathRel(name)))
 }
 
 // Gitignore reads the content of a gitignore locale from static/bindata or custom path.
 func Gitignore(name string) ([]byte, error) {
-	return fileFromDir(path.Join("gitignore", util.CleanPath(name)))
+	return fileFromDir(path.Join("gitignore", util.SafePathRel(name)))
 }
 
 // License reads the content of a specific license from static/bindata or custom path.
 func License(name string) ([]byte, error) {
-	return fileFromDir(path.Join("license", util.CleanPath(name)))
+	return fileFromDir(path.Join("license", util.SafePathRel(name)))
 }
 
 // Labels reads the content of a specific labels from static/bindata or custom path.
 func Labels(name string) ([]byte, error) {
-	return fileFromDir(path.Join("label", util.CleanPath(name)))
+	return fileFromDir(path.Join("label", util.SafePathRel(name)))
 }
 
 // WalkLocales reads the content of a specific locale

modules/public/public.go

@@ -45,29 +45,19 @@ func AssetsHandlerFunc(opts *Options) http.HandlerFunc {
 			return
 		}
 
-		file := req.URL.Path
-		file = file[len(opts.Prefix):]
-		if len(file) == 0 {
-			resp.WriteHeader(http.StatusNotFound)
-			return
-		}
-		if strings.Contains(file, "\\") {
-			resp.WriteHeader(http.StatusBadRequest)
-			return
-		}
-		file = "/" + file
-
-		var written bool
+		var corsSent bool
 		if opts.CorsHandler != nil {
-			written = true
 			opts.CorsHandler(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
-				written = false
+				corsSent = true
 			})).ServeHTTP(resp, req)
 		}
-		if written {
+		// If CORS is not sent, the response must have been written by other handlers
+		if !corsSent {
 			return
 		}
 
+		file := req.URL.Path[len(opts.Prefix):]
+
 		// custom files
 		if opts.handle(resp, req, http.Dir(custPath), file) {
 			return
@@ -102,8 +92,8 @@ func setWellKnownContentType(w http.ResponseWriter, file string) {
 }
 
 func (opts *Options) handle(w http.ResponseWriter, req *http.Request, fs http.FileSystem, file string) bool {
-	// use clean to keep the file is a valid path with no . or ..
-	f, err := fs.Open(util.CleanPath(file))
+	// 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))
 	if err != nil {
 		if os.IsNotExist(err) {
 			return false

modules/storage/local.go

@@ -9,7 +9,6 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/util"
@@ -58,7 +57,7 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 }
 
 func (l *LocalStorage) buildLocalPath(p string) string {
-	return filepath.Join(l.dir, util.CleanPath(strings.ReplaceAll(p, "\\", "/")))
+	return util.SafeFilePathAbs(l.dir, p)
 }
 
 // Open a file
@@ -128,10 +127,7 @@ func (l *LocalStorage) URL(path, name string) (*url.URL, error) {
 
 // IterateObjects iterates across the objects in the local storage
 func (l *LocalStorage) IterateObjects(prefix string, fn func(path string, obj Object) error) error {
-	dir := l.dir
-	if prefix != "" {
-		dir = filepath.Join(l.dir, util.CleanPath(prefix))
-	}
+	dir := l.buildLocalPath(prefix)
 	return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error {
 		if err != nil {
 			return err

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 strings.TrimPrefix(path.Join(m.basePath, util.CleanPath(strings.ReplaceAll(p, "\\", "/"))), "/")
+	return util.SafePathRelX(m.basePath, p)
 }
 
 // Open open a file

modules/util/path.go

@@ -14,21 +14,85 @@ import (
 	"strings"
 )
 
-// CleanPath ensure to clean the path
-func CleanPath(p string) string {
-	if strings.HasPrefix(p, "/") {
-		return path.Clean(p)
+// SafePathRel joins the path elements into a single path, each element is cleaned separately
+// It only returns the following values (like path.Join):
+//
+//	empty => ``
+//	`` => ``
+//	`..` => `.`
+//	`dir` => `dir`
+//	`/dir/` => `dir`
+//	`foo\..\bar` => `foo\..\bar`
+//
+// any redundant part(relative dots, slashes) is removed.
+func SafePathRel(elem ...string) string {
+	elems := make([]string, len(elem))
+	for i, e := range elem {
+		if e == "" {
+			continue
+		}
+		elems[i] = path.Clean("/" + e)
+	}
+	p := path.Join(elems...)
+	if p == "" {
+		return ""
+	} else if p == "/" {
+		return "."
+	} else {
+		return p[1:]
 	}
-	return path.Clean("/" + p)[1:]
 }
 
-// EnsureAbsolutePath ensure that a path is absolute, making it
-// relative to absoluteBase if necessary
-func EnsureAbsolutePath(path, absoluteBase string) string {
-	if filepath.IsAbs(path) {
-		return path
+// SafePathRelX joins the path elements into a single path like SafePathRel,
+// and covert all backslashes to slashes. (X means "extended", also means the combination of `\` and `/`)
+// It returns similar results as SafePathRel except:
+//
+//	`foo\..\bar` => `bar`  (because it's processed as `foo/../bar`)
+//
+// any redundant part(relative dots, slashes) is removed.
+func SafePathRelX(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...)
+}
+
+const pathSeparator = string(os.PathSeparator)
+
+// SafeFilePathAbs joins the path elements into a single file path, each element is cleaned separately
+// and convert all slashes/backslashes to path separators.
+// The first element must be an absolute path, caller should prepare the base path
+func SafeFilePathAbs(elem ...string) string {
+	elems := make([]string, len(elem))
+
+	// POISX filesystem can have `\` in file names. Windows: `\` and `/` are both used for path separators
+	// to keep the behavior consistent, we do not allow `\` in file names, replace all `\` with `/`
+	if isOSWindows() {
+		elems[0] = filepath.Clean(elem[0])
+	} else {
+		elems[0] = filepath.Clean(strings.ReplaceAll(elem[0], "\\", pathSeparator))
+	}
+	if !filepath.IsAbs(elems[0]) {
+		// This shouldn't happen. If there is really necessary to pass in relative path, return the full path with filepath.Abs() instead
+		panic("FilePathJoinAbs: result is not absolute, do not guess a relative path based on current working directory")
+	}
+
+	for i := 1; i < len(elem); i++ {
+		if elem[i] == "" {
+			continue
+		}
+		if isOSWindows() {
+			elems[i] = filepath.Clean(pathSeparator + elem[i])
+		} else {
+			elems[i] = filepath.Clean(pathSeparator + strings.ReplaceAll(elem[i], "\\", pathSeparator))
+		}
 	}
-	return filepath.Join(absoluteBase, path)
+	// the elems[0] must be an absolute path, just join them together
+	return filepath.Join(elems...)
 }
 
 // IsDir returns true if given path is a directory,

modules/util/path_test.go

@@ -138,13 +138,63 @@ func TestMisc_IsReadmeFileName(t *testing.T) {
 }
 
 func TestCleanPath(t *testing.T) {
-	cases := map[string]string{
-		"../../test": "test",
-		"/test":      "/test",
-		"/../test":   "/test",
+	cases := []struct {
+		elems    []string
+		expected string
+	}{
+		{[]string{}, ``},
+		{[]string{``}, ``},
+		{[]string{`..`}, `.`},
+		{[]string{`a`}, `a`},
+		{[]string{`/a/`}, `a`},
+		{[]string{`../a/`, `../b`, `c/..`, `d`}, `a/b/d`},
+		{[]string{`a\..\b`}, `a\..\b`},
+	}
+	for _, c := range cases {
+		assert.Equal(t, c.expected, SafePathRel(c.elems...), "case: %v", c.elems)
+	}
+
+	cases = []struct {
+		elems    []string
+		expected string
+	}{
+		{[]string{}, ``},
+		{[]string{``}, ``},
+		{[]string{`..`}, `.`},
+		{[]string{`a`}, `a`},
+		{[]string{`/a/`}, `a`},
+		{[]string{`../a/`, `../b`, `c/..`, `d`}, `a/b/d`},
+		{[]string{`a\..\b`}, `b`},
+	}
+	for _, c := range cases {
+		assert.Equal(t, c.expected, SafePathRelX(c.elems...), "case: %v", c.elems)
 	}
 
-	for k, v := range cases {
-		assert.Equal(t, v, CleanPath(k))
+	// for POSIX only, but the result is similar on Windows, because the first element must be an absolute path
+	if isOSWindows() {
+		cases = []struct {
+			elems    []string
+			expected string
+		}{
+			{[]string{`C:\..`}, `C:\`},
+			{[]string{`C:\a`}, `C:\a`},
+			{[]string{`C:\a/`}, `C:\a`},
+			{[]string{`C:\..\a\`, `../b`, `c\..`, `d`}, `C:\a\b\d`},
+			{[]string{`C:\a/..\b`}, `C:\b`},
+		}
+	} else {
+		cases = []struct {
+			elems    []string
+			expected string
+		}{
+			{[]string{`/..`}, `/`},
+			{[]string{`/a`}, `/a`},
+			{[]string{`/a/`}, `/a`},
+			{[]string{`/../a/`, `../b`, `c/..`, `d`}, `/a/b/d`},
+			{[]string{`/a\..\b`}, `/b`},
+		}
+	}
+	for _, c := range cases {
+		assert.Equal(t, c.expected, SafeFilePathAbs(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.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
+				rPath = util.SafePathRelX(rPath)
 
 				u, err := objStore.URL(rPath, path.Base(rPath))
 				if err != nil {
@@ -81,8 +81,8 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 			routing.UpdateFuncInfo(req.Context(), funcInfo)
 
 			rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-			rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
-			if rPath == "" {
+			rPath = util.SafePathRelX(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 = strings.Trim(util.CleanPath(name), "/")
+	name = util.SafePathRel(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.CleanPath(lockPath)
+	lockPath = util.SafePathRel(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

@@ -865,8 +865,8 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
 				_, _, line, _ = git.ParseDiffHunkString(comment.DiffHunk)
 			}
 
-			// SECURITY: The TreePath must be cleaned!
-			comment.TreePath = util.CleanPath(comment.TreePath)
+			// SECURITY: The TreePath must be cleaned! use relative path
+			comment.TreePath = util.SafePathRel(comment.TreePath)
 
 			var patch string
 			reader, writer := io.Pipe()

services/packages/container/blob_uploader.go

@@ -8,8 +8,6 @@ import (
 	"errors"
 	"io"
 	"os"
-	"path/filepath"
-	"strings"
 
 	packages_model "code.gitea.io/gitea/models/packages"
 	packages_module "code.gitea.io/gitea/modules/packages"
@@ -33,7 +31,7 @@ type BlobUploader struct {
 }
 
 func buildFilePath(id string) string {
-	return filepath.Join(setting.Packages.ChunkedUploadPath, util.CleanPath(strings.ReplaceAll(id, "\\", "/")))
+	return util.SafeFilePathAbs(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 = strings.Trim(util.CleanPath(name), "/")
+	name = util.SafePathRel(name)
 	// Git disallows any filenames to have a .git directory in them.
 	for _, part := range strings.Split(name, "/") {
 		if strings.ToLower(part) == ".git" {