]> source.dussan.org Git - gitea.git/commitdiff
Introduce path Clean/Join helper functions (#23495)
authorwxiaoguang <wxiaoguang@gmail.com>
Tue, 21 Mar 2023 20:02:49 +0000 (04:02 +0800)
committerGitHub <noreply@github.com>
Tue, 21 Mar 2023 20:02:49 +0000 (16:02 -0400)
Since #23493 has conflicts with latest commits, this PR is my proposal
for fixing #23371

Details are in the comments

And refactor the `modules/options` module, to make it always use
"filepath" to access local files.

Benefits:

* No need to do `util.CleanPath(strings.ReplaceAll(p, "\\", "/"))),
"/")` any more (not only one before)
* The function behaviors are clearly defined

16 files changed:
models/git/lfs_lock.go
modules/options/base.go
modules/options/dynamic.go
modules/options/static.go
modules/public/public.go
modules/storage/local.go
modules/storage/local_test.go
modules/storage/minio.go
modules/util/path.go
modules/util/path_test.go
routers/web/base.go
routers/web/repo/editor.go
routers/web/repo/lfs.go
services/migrations/gitea_uploader.go
services/packages/container/blob_uploader.go
services/repository/files/file.go

index 178fa72f09bf62b3b7c84987493405810d143afa..261c73032aa21115e0ec733b56e97f41e9417d3a 100644 (file)
@@ -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.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.CleanPath(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.CleanPath(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 {
index e83e8df5d0945e6d29ef31807dd6e0ed907e4c07..7882ed008159a6cdfab2af0d3aaf23e5c42767c7 100644 (file)
@@ -7,36 +7,38 @@ import (
        "fmt"
        "io/fs"
        "os"
-       "path"
        "path/filepath"
 
+       "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/setting"
        "code.gitea.io/gitea/modules/util"
 )
 
+var directories = make(directorySet)
+
 // 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 fileFromOptionsDir("locale", 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 fileFromOptionsDir("readme", 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 fileFromOptionsDir("gitignore", 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 fileFromOptionsDir("license", 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 fileFromOptionsDir("label", name)
 }
 
 // WalkLocales reads the content of a specific locale
@@ -79,17 +81,54 @@ func walkAssetDir(root string, callback func(path, name string, d fs.DirEntry, e
        return nil
 }
 
-func statDirIfExist(dir string) ([]string, error) {
-       isDir, err := util.IsDir(dir)
+// mustLocalPathAbs coverts a path to absolute path
+// FIXME: the old behavior (StaticRootPath might not be absolute), not ideal, just keep the same as before
+func mustLocalPathAbs(s string) string {
+       abs, err := filepath.Abs(s)
        if err != nil {
-               return nil, fmt.Errorf("unable to check if static directory %s is a directory. %w", dir, err)
+               // This should never happen in a real system. If it happens, the user must have already been in trouble: the system is not able to resolve its own paths.
+               log.Fatal("Unable to get absolute path for %q: %v", s, err)
        }
-       if !isDir {
-               return nil, nil
+       return abs
+}
+
+func joinLocalPaths(baseDirs []string, subDir string, elems ...string) (paths []string) {
+       abs := make([]string, len(elems)+2)
+       abs[1] = subDir
+       copy(abs[2:], elems)
+       for _, baseDir := range baseDirs {
+               abs[0] = mustLocalPathAbs(baseDir)
+               paths = append(paths, util.FilePathJoinAbs(abs...))
        }
-       files, err := util.StatDir(dir, true)
-       if err != nil {
-               return nil, fmt.Errorf("unable to read directory %q. %w", dir, err)
+       return paths
+}
+
+func listLocalDirIfExist(baseDirs []string, subDir string, elems ...string) (files []string, err error) {
+       for _, localPath := range joinLocalPaths(baseDirs, subDir, elems...) {
+               isDir, err := util.IsDir(localPath)
+               if err != nil {
+                       return nil, fmt.Errorf("unable to check if path %q is a directory. %w", localPath, err)
+               } else if !isDir {
+                       continue
+               }
+
+               dirFiles, err := util.StatDir(localPath, true)
+               if err != nil {
+                       return nil, fmt.Errorf("unable to read directory %q. %w", localPath, err)
+               }
+               files = append(files, dirFiles...)
        }
        return files, nil
 }
+
+func readLocalFile(baseDirs []string, subDir string, elems ...string) ([]byte, error) {
+       for _, localPath := range joinLocalPaths(baseDirs, subDir, elems...) {
+               data, err := os.ReadFile(localPath)
+               if err == nil {
+                       return data, nil
+               } else if !os.IsNotExist(err) {
+                       log.Error("Unable to read file %q. Error: %v", localPath, err)
+               }
+       }
+       return nil, os.ErrNotExist
+}
index 8c954492ae51fefe76ed7bbb8bfc3747fed7075c..3d6261983f2de9ea568fd2d5fe3e300bc8cf5dc4 100644 (file)
@@ -6,62 +6,26 @@
 package options
 
 import (
-       "fmt"
-       "os"
-       "path"
-
-       "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/setting"
-       "code.gitea.io/gitea/modules/util"
 )
 
-var directories = make(directorySet)
-
 // Dir returns all files from static or custom directory.
 func Dir(name string) ([]string, error) {
        if directories.Filled(name) {
                return directories.Get(name), nil
        }
 
-       var result []string
-
-       for _, dir := range []string{
-               path.Join(setting.CustomPath, "options", name),     // custom dir
-               path.Join(setting.StaticRootPath, "options", name), // static dir
-       } {
-               files, err := statDirIfExist(dir)
-               if err != nil {
-                       return nil, err
-               }
-               result = append(result, files...)
+       result, err := listLocalDirIfExist([]string{setting.CustomPath, setting.StaticRootPath}, "options", name)
+       if err != nil {
+               return nil, err
        }
 
        return directories.AddAndGet(name, result), nil
 }
 
-// fileFromDir is a helper to read files from static or custom path.
-func fileFromDir(name string) ([]byte, error) {
-       customPath := path.Join(setting.CustomPath, "options", name)
-
-       isFile, err := util.IsFile(customPath)
-       if err != nil {
-               log.Error("Unable to check if %s is a file. Error: %v", customPath, err)
-       }
-       if isFile {
-               return os.ReadFile(customPath)
-       }
-
-       staticPath := path.Join(setting.StaticRootPath, "options", name)
-
-       isFile, err = util.IsFile(staticPath)
-       if err != nil {
-               log.Error("Unable to check if %s is a file. Error: %v", staticPath, err)
-       }
-       if isFile {
-               return os.ReadFile(staticPath)
-       }
-
-       return []byte{}, fmt.Errorf("Asset file does not exist: %s", name)
+// fileFromOptionsDir is a helper to read files from custom or static path.
+func fileFromOptionsDir(elems ...string) ([]byte, error) {
+       return readLocalFile([]string{setting.CustomPath, setting.StaticRootPath}, "options", elems...)
 }
 
 // IsDynamic will return false when using embedded data (-tags bindata)
index 549f4e25b11a388cb361a714d6097128616d0d79..0482dea6817ce6d7816fba56ca06a9f44edc0e48 100644 (file)
@@ -8,33 +8,20 @@ package options
 import (
        "fmt"
        "io"
-       "os"
-       "path"
 
-       "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/setting"
        "code.gitea.io/gitea/modules/util"
 )
 
-var directories = make(directorySet)
-
-// Dir returns all files from bindata or custom directory.
+// Dir returns all files from custom directory or bindata.
 func Dir(name string) ([]string, error) {
        if directories.Filled(name) {
                return directories.Get(name), nil
        }
 
-       var result []string
-
-       for _, dir := range []string{
-               path.Join(setting.CustomPath, "options", name), // custom dir
-               // no static dir
-       } {
-               files, err := statDirIfExist(dir)
-               if err != nil {
-                       return nil, err
-               }
-               result = append(result, files...)
+       result, err := listLocalDirIfExist([]string{setting.CustomPath}, "options", name)
+       if err != nil {
+               return nil, err
        }
 
        files, err := AssetDir(name)
@@ -64,24 +51,18 @@ func AssetDir(dirName string) ([]string, error) {
        return results, nil
 }
 
-// fileFromDir is a helper to read files from bindata or custom path.
-func fileFromDir(name string) ([]byte, error) {
-       customPath := path.Join(setting.CustomPath, "options", name)
-
-       isFile, err := util.IsFile(customPath)
-       if err != nil {
-               log.Error("Unable to check if %s is a file. Error: %v", customPath, err)
-       }
-       if isFile {
-               return os.ReadFile(customPath)
+// fileFromOptionsDir is a helper to read files from custom path or bindata.
+func fileFromOptionsDir(elems ...string) ([]byte, error) {
+       // only try custom dir, no static dir
+       if data, err := readLocalFile([]string{setting.CustomPath}, "options", elems...); err == nil {
+               return data, nil
        }
 
-       f, err := Assets.Open(name)
+       f, err := Assets.Open(util.PathJoinRelX(elems...))
        if err != nil {
                return nil, err
        }
        defer f.Close()
-
        return io.ReadAll(f)
 }
 
index e1d60d89eb9f9df5b0251ea4077a8d54bb440d77..30b03a27954a2fa2e4db9c597f0558db7e585f0b 100644 (file)
@@ -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.PathJoinRelX(file))
        if err != nil {
                if os.IsNotExist(err) {
                        return false
index 15f5761e8f055af31e37b0bfe2d296772b75fbd4..d22974a65add05420b7c25712f95327c29c36534 100644 (file)
@@ -5,11 +5,11 @@ package storage
 
 import (
        "context"
+       "fmt"
        "io"
        "net/url"
        "os"
        "path/filepath"
-       "strings"
 
        "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/util"
@@ -41,13 +41,19 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
        }
        config := configInterface.(LocalStorageConfig)
 
+       if !filepath.IsAbs(config.Path) {
+               return nil, fmt.Errorf("LocalStorageConfig.Path should have been prepared by setting/storage.go and should be an absolute path, but not: %q", config.Path)
+       }
        log.Info("Creating new Local Storage at %s", config.Path)
        if err := os.MkdirAll(config.Path, os.ModePerm); err != nil {
                return nil, err
        }
 
        if config.TemporaryPath == "" {
-               config.TemporaryPath = config.Path + "/tmp"
+               config.TemporaryPath = filepath.Join(config.Path, "tmp")
+       }
+       if !filepath.IsAbs(config.TemporaryPath) {
+               return nil, fmt.Errorf("LocalStorageConfig.TemporaryPath should be an absolute path, but not: %q", config.TemporaryPath)
        }
 
        return &LocalStorage{
@@ -58,7 +64,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.FilePathJoinAbs(l.dir, p)
 }
 
 // Open a file
@@ -128,10 +134,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
index 2b112df8f12b373538ece601c9e0af61416a3e73..9649761a0f96fc75889c6327c0104dbb2f1e8133 100644 (file)
@@ -20,29 +20,29 @@ func TestBuildLocalPath(t *testing.T) {
                expected string
        }{
                {
-                       "a",
+                       "/a",
                        "0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
-                       "a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+                       "/a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
                },
                {
-                       "a",
+                       "/a",
                        "../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
-                       "a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+                       "/a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
                },
                {
-                       "a",
+                       "/a",
                        "0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
-                       "a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+                       "/a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
                },
                {
-                       "b",
+                       "/b",
                        "a/../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
-                       "b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+                       "/b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
                },
                {
-                       "b",
+                       "/b",
                        "a\\..\\0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
-                       "b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+                       "/b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
                },
        }
 
index 8cc06bcdd3df23ac8d32a83ca7de86d6a7a717fd..5c67dbf26a298488597e6d90c65d60f59e41684e 100644 (file)
@@ -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.PathJoinRelX(m.basePath, p)
 }
 
 // Open open a file
index 5aa9e15f5c3e95826d87370ad28429289c627425..37d06e9813e07fe72294ba3b9aad74c402f8a85b 100644 (file)
@@ -5,6 +5,7 @@ package util
 
 import (
        "errors"
+       "fmt"
        "net/url"
        "os"
        "path"
@@ -14,21 +15,92 @@ import (
        "strings"
 )
 
-// CleanPath ensure to clean the path
-func CleanPath(p string) string {
-       if strings.HasPrefix(p, "/") {
-               return path.Clean(p)
+// 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 => ``
+//     `` => ``
+//     `..` => `.`
+//     `dir` => `dir`
+//     `/dir/` => `dir`
+//     `foo\..\bar` => `foo\..\bar`
+//     {`foo`, ``, `bar`} => `foo/bar`
+//     {`foo`, `..`, `bar`} => `foo/bar`
+func PathJoinRel(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:]
+       }
+}
+
+// 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'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 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 path.Clean("/" + p)[1:]
+       return PathJoinRel(elems...)
 }
 
-// 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
+const pathSeparator = string(os.PathSeparator)
+
+// 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.
+// 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 FilePathJoinAbs(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(fmt.Sprintf("FilePathJoinAbs: %q (for path %v) is not absolute, do not guess a relative path based on current working directory", elems[0], elems))
+       }
+
+       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,
index 2f020f924dd2a94654b3515ba4d86b981a8c5512..1d27c9bf0c0f89afa1bb87663271bdd72aa630d8 100644 (file)
@@ -138,13 +138,75 @@ 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`},
+               {[]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, PathJoinRel(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`},
+               {[]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, PathJoinRelX(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`},
+                       {[]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 {
+                       elems    []string
+                       expected string
+               }{
+                       {[]string{`/..`}, `/`},
+                       {[]string{`/a`}, `/a`},
+                       {[]string{`/a/`}, `/a`},
+                       {[]string{`/../a/`, `../b`, `c/..`, `d`}, `/a/b/d`},
+                       {[]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, FilePathJoinAbs(c.elems...), "case: %v", c.elems)
        }
 }
index 2eb0b6f39118ae0964a19cc53441177b81b61e71..da18a75643e68727e6fe5db415cf49b2d3ce749e 100644 (file)
@@ -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.PathJoinRelX(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.PathJoinRelX(rPath)
+                       if rPath == "" || rPath == "." {
                                http.Error(w, "file not found", http.StatusNotFound)
                                return
                        }
index 4f208098e4766a4eaacfbc02cab72297c99cce3b..07241b88700fbdff0e90a065226e9c45241fef62 100644 (file)
@@ -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.PathJoinRel(name)
        // Git disallows any filenames to have a .git directory in them.
        for _, part := range strings.Split(name, "/") {
                if strings.ToLower(part) == ".git" {
index 43f5527986b9ba97f84a67bd9b6809a8605140e4..9957869c999a355432831bd7c48bad6ab2ed43ad 100644 (file)
@@ -207,7 +207,7 @@ func LFSLockFile(ctx *context.Context) {
                ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
                return
        }
-       lockPath = util.CleanPath(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")
index ca961524d12c2aa31434add9820c176872900055..0eb34b5fe562f55abb5b0c514260266110db41de 100644 (file)
@@ -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.PathJoinRel(comment.TreePath)
 
                        var patch string
                        reader, writer := io.Pipe()
index 860672587d2b4bc975f32f533c8dbaa7ba181aff..bae2e2d6af6679066e602a1c1bcee6a76bf185f6 100644 (file)
@@ -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.FilePathJoinAbs(setting.Packages.ChunkedUploadPath, id)
 }
 
 // NewBlobUploader creates a new blob uploader for the given id
index 7939491aec62423a72d95ee9eba1cdc7d8b78246..dc1e547dcdae9014739d28ff5170efde1aff06c3 100644 (file)
@@ -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.PathJoinRel(name)
        // Git disallows any filenames to have a .git directory in them.
        for _, part := range strings.Split(name, "/") {
                if strings.ToLower(part) == ".git" {