Introduce path Clean/Join helper functions (#23495)

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

9 files changed, 249 insertions, 138 deletions

diff --git a/modules/options/base.go b/modules/options/base.go
index e83e8df5d0..7882ed0081 100644
--- a/modules/options/base.go
+++ b/modules/options/base.go
@@ -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
+}
diff --git a/modules/options/dynamic.go b/modules/options/dynamic.go
index 8c954492ae..3d6261983f 100644
--- a/modules/options/dynamic.go
+++ b/modules/options/dynamic.go
@@ -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)
diff --git a/modules/options/static.go b/modules/options/static.go
index 549f4e25b1..0482dea681 100644
--- a/modules/options/static.go
+++ b/modules/options/static.go
@@ -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)
 }
 
diff --git a/modules/public/public.go b/modules/public/public.go
index e1d60d89eb..30b03a2795 100644
--- a/modules/public/public.go
+++ b/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.PathJoinRelX(file))
 	if err != nil {
 		if os.IsNotExist(err) {
 			return false
diff --git a/modules/storage/local.go b/modules/storage/local.go
index 15f5761e8f..d22974a65a 100644
--- a/modules/storage/local.go
+++ b/modules/storage/local.go
@@ -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
diff --git a/modules/storage/local_test.go b/modules/storage/local_test.go
index 2b112df8f1..9649761a0f 100644
--- a/modules/storage/local_test.go
+++ b/modules/storage/local_test.go
@@ -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",
 		},
 	}
 
diff --git a/modules/storage/minio.go b/modules/storage/minio.go
index 8cc06bcdd3..5c67dbf26a 100644
--- a/modules/storage/minio.go
+++ b/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.PathJoinRelX(m.basePath, p)
 }
 
 // Open open a file
diff --git a/modules/util/path.go b/modules/util/path.go
index 5aa9e15f5c..37d06e9813 100644
--- a/modules/util/path.go
+++ b/modules/util/path.go
@@ -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,
diff --git a/modules/util/path_test.go b/modules/util/path_test.go
index 2f020f924d..1d27c9bf0c 100644
--- a/modules/util/path_test.go
+++ b/modules/util/path_test.go
@@ -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)
 	}
 }