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

2 files changed, 151 insertions, 17 deletions

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)
 	}
 }