From 32dfaa7b63a518d98a28da84231de57b1588ff09 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 15 Mar 2023 17:43:19 +0800 Subject: [PATCH 01/11] fix --- models/git/lfs_lock.go | 6 +- modules/options/base.go | 10 +-- modules/public/public.go | 26 ++---- modules/storage/local.go | 8 +- modules/storage/minio.go | 2 +- modules/util/path.go | 86 +++++++++++++++++--- modules/util/path_test.go | 62 ++++++++++++-- routers/web/base.go | 6 +- routers/web/repo/editor.go | 2 +- routers/web/repo/lfs.go | 2 +- services/migrations/gitea_uploader.go | 4 +- services/packages/container/blob_uploader.go | 4 +- services/repository/files/file.go | 2 +- 13 files changed, 159 insertions(+), 61 deletions(-) diff --git a/models/git/lfs_lock.go b/models/git/lfs_lock.go index 178fa72f09bf6..2e73d606ab0bd 100644 --- a/models/git/lfs_lock.go +++ b/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 { diff --git a/modules/options/base.go b/modules/options/base.go index e83e8df5d0945..4feab8fea1a69 100644 --- a/modules/options/base.go +++ b/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 diff --git a/modules/public/public.go b/modules/public/public.go index e1d60d89eb9f9..f6b90403d3586 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.SafePathRelX(file)) if err != nil { if os.IsNotExist(err) { return false diff --git a/modules/storage/local.go b/modules/storage/local.go index 15f5761e8f055..059167ab2ece6 100644 --- a/modules/storage/local.go +++ b/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 diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 8cc06bcdd3df2..946268f52462e 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.SafePathRelX(m.basePath, p) } // Open open a file diff --git a/modules/util/path.go b/modules/util/path.go index 5aa9e15f5c3e9..45beea48035a5 100644 --- a/modules/util/path.go +++ b/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, diff --git a/modules/util/path_test.go b/modules/util/path_test.go index 2f020f924dd2a..461344162ca25 100644 --- a/modules/util/path_test.go +++ b/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) } } diff --git a/routers/web/base.go b/routers/web/base.go index 2eb0b6f39118a..3a81ce5d53123 100644 --- a/routers/web/base.go +++ b/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 } diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 4f208098e4766..a94b09d8f8f05 100644 --- a/routers/web/repo/editor.go +++ b/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" { diff --git a/routers/web/repo/lfs.go b/routers/web/repo/lfs.go index 43f5527986b9b..eb5bf49c37c51 100644 --- a/routers/web/repo/lfs.go +++ b/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") diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index ca961524d12c2..e9cc89c00cac0 100644 --- a/services/migrations/gitea_uploader.go +++ b/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() diff --git a/services/packages/container/blob_uploader.go b/services/packages/container/blob_uploader.go index 860672587d2b4..68aceff4bc425 100644 --- a/services/packages/container/blob_uploader.go +++ b/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 diff --git a/services/repository/files/file.go b/services/repository/files/file.go index 7939491aec624..1276fc1133d1a 100644 --- a/services/repository/files/file.go +++ b/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" { From cb9c518379a9dd4b255a4c12305002fb00a804cd Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 16 Mar 2023 09:47:02 +0800 Subject: [PATCH 02/11] always use absolute path in LocalStorage --- modules/storage/local.go | 9 ++++++++- modules/storage/local_test.go | 20 ++++++++++---------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index 059167ab2ece6..25821b503c8aa 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -5,6 +5,7 @@ package storage import ( "context" + "fmt" "io" "net/url" "os" @@ -40,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{ diff --git a/modules/storage/local_test.go b/modules/storage/local_test.go index 2b112df8f12b3..9649761a0f96f 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", }, } From dd48b195c5e84b7827aa1dfcb575e1b9c7d11836 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 16 Mar 2023 14:31:09 +0800 Subject: [PATCH 03/11] improve comments and tests --- modules/util/path.go | 22 +++++++++++++--------- modules/util/path_test.go | 8 ++++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/modules/util/path.go b/modules/util/path.go index 45beea48035a5..d973f5bc9cc43 100644 --- a/modules/util/path.go +++ b/modules/util/path.go @@ -14,8 +14,8 @@ import ( "strings" ) -// SafePathRel joins the path elements into a single path, each element is cleaned separately -// It only returns the following values (like path.Join): +// SafePathRel 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. // // empty => `` // `` => `` @@ -23,8 +23,8 @@ import ( // `dir` => `dir` // `/dir/` => `dir` // `foo\..\bar` => `foo\..\bar` -// -// any redundant part(relative dots, slashes) is removed. +// {`foo`, ``, `bar`} => `foo/bar` +// {`foo`, `..`, `bar`} => `foo/bar` func SafePathRel(elem ...string) string { elems := make([]string, len(elem)) for i, e := range elem { @@ -44,12 +44,12 @@ func SafePathRel(elem ...string) string { } // 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 `/`) +// 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. +// All backslashes are handled as slashes, the result only contains slashes. func SafePathRelX(elem ...string) string { elems := make([]string, len(elem)) for i, e := range elem { @@ -63,9 +63,13 @@ func SafePathRelX(elem ...string) string { 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 +// SafeFilePathAbs 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. +// +// {`/foo`, ``, `bar`} => `/foo/bar` +// {`/foo`, `..`, `bar`} => `/foo/bar` func SafeFilePathAbs(elem ...string) string { elems := make([]string, len(elem)) diff --git a/modules/util/path_test.go b/modules/util/path_test.go index 461344162ca25..4a5f02e735e1c 100644 --- a/modules/util/path_test.go +++ b/modules/util/path_test.go @@ -149,6 +149,8 @@ func TestCleanPath(t *testing.T) { {[]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`}, } for _, c := range cases { assert.Equal(t, c.expected, SafePathRel(c.elems...), "case: %v", c.elems) @@ -165,6 +167,8 @@ func TestCleanPath(t *testing.T) { {[]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`}, } for _, c := range cases { assert.Equal(t, c.expected, SafePathRelX(c.elems...), "case: %v", c.elems) @@ -181,6 +185,8 @@ func TestCleanPath(t *testing.T) { {[]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`}, } } else { cases = []struct { @@ -192,6 +198,8 @@ func TestCleanPath(t *testing.T) { {[]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`}, } } for _, c := range cases { From 9dd4db0c986ffe6eb2b9d1ca13bfa05af1fa5fbf Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 18 Mar 2023 21:23:13 +0800 Subject: [PATCH 04/11] fix function name in message --- modules/util/path.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/util/path.go b/modules/util/path.go index d973f5bc9cc43..2f12a3bf17260 100644 --- a/modules/util/path.go +++ b/modules/util/path.go @@ -82,7 +82,7 @@ func SafeFilePathAbs(elem ...string) string { } 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") + panic("SafeFilePathAbs: result is not absolute, do not guess a relative path based on current working directory") } for i := 1; i < len(elem); i++ { From 62114150949d735f3125776c9fddb38e2592e3b0 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 11:51:49 +0800 Subject: [PATCH 05/11] use SafeFilePathAbs instead of path.Join for local file accessing --- modules/options/base.go | 30 ++++++++++++++++++++++++------ modules/options/dynamic.go | 35 +++++------------------------------ modules/options/static.go | 23 ++++++----------------- 3 files changed, 35 insertions(+), 53 deletions(-) diff --git a/modules/options/base.go b/modules/options/base.go index 4feab8fea1a69..227beaec6279d 100644 --- a/modules/options/base.go +++ b/modules/options/base.go @@ -7,36 +7,36 @@ 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" ) // 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.SafePathRel(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.SafePathRel(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.SafePathRel(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.SafePathRel(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.SafePathRel(name))) + return fileFromOptionsDir("label", name) } // WalkLocales reads the content of a specific locale @@ -93,3 +93,21 @@ func statDirIfExist(dir string) ([]string, error) { } return files, nil } + +func readFileFromLocal(base []string, sub string, elems ...string) ([]byte, error) { + localPathElems := make([]string, len(elems)+2) // path[0] will be used for the custom path prefix + localPathElems[1] = sub + copy(localPathElems[2:], elems) + + for _, dir := range base { + localPathElems[0] = dir + localPath := util.SafeFilePathAbs(localPathElems...) + isFile, err := util.IsFile(localPath) + if err != nil { + log.Error("Unable to check if %s is a file. Error: %v", localPath, err) + } else if isFile { + return os.ReadFile(localPath) + } + } + return nil, fmt.Errorf("asset file does not exist: %v", elems) +} diff --git a/modules/options/dynamic.go b/modules/options/dynamic.go index 8c954492ae51f..b10239bac8127 100644 --- a/modules/options/dynamic.go +++ b/modules/options/dynamic.go @@ -6,11 +6,6 @@ package options import ( - "fmt" - "os" - "path" - - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" ) @@ -26,8 +21,8 @@ func Dir(name string) ([]string, error) { var result []string for _, dir := range []string{ - path.Join(setting.CustomPath, "options", name), // custom dir - path.Join(setting.StaticRootPath, "options", name), // static dir + util.SafeFilePathAbs(setting.CustomPath, "options", name), // custom dir + util.SafeFilePathAbs(setting.StaticRootPath, "options", name), // static dir } { files, err := statDirIfExist(dir) if err != nil { @@ -39,29 +34,9 @@ func Dir(name string) ([]string, error) { 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 static or custom path. +func fileFromOptionsDir(elems ...string) ([]byte, error) { + return readFileFromLocal([]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 549f4e25b11a3..37c952d7853c5 100644 --- a/modules/options/static.go +++ b/modules/options/static.go @@ -8,17 +8,13 @@ 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 @@ -27,7 +23,7 @@ func Dir(name string) ([]string, error) { var result []string for _, dir := range []string{ - path.Join(setting.CustomPath, "options", name), // custom dir + filepath.Join(setting.CustomPath, "options", name), // custom dir // no static dir } { files, err := statDirIfExist(dir) @@ -64,16 +60,10 @@ 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) { + if data, err := readFileFromLocal([]string{setting.CustomPath}, "options", elems...); err == nil { + return data, nil } f, err := Assets.Open(name) @@ -81,7 +71,6 @@ func fileFromDir(name string) ([]byte, error) { return nil, err } defer f.Close() - return io.ReadAll(f) } From 8525289fb630451c0eea3223c3e0600ecb2edd8f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 12:48:01 +0800 Subject: [PATCH 06/11] simplify os.ReadFile, fine tune messages and comments, fix lint --- modules/options/base.go | 12 ++++++------ modules/options/dynamic.go | 2 +- modules/options/static.go | 5 ++++- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/modules/options/base.go b/modules/options/base.go index 227beaec6279d..bbd6da0abd817 100644 --- a/modules/options/base.go +++ b/modules/options/base.go @@ -102,12 +102,12 @@ func readFileFromLocal(base []string, sub string, elems ...string) ([]byte, erro for _, dir := range base { localPathElems[0] = dir localPath := util.SafeFilePathAbs(localPathElems...) - isFile, err := util.IsFile(localPath) - if err != nil { - log.Error("Unable to check if %s is a file. Error: %v", localPath, err) - } else if isFile { - return os.ReadFile(localPath) + 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, fmt.Errorf("asset file does not exist: %v", elems) + return nil, fmt.Errorf("local file does not exist: %v", elems) } diff --git a/modules/options/dynamic.go b/modules/options/dynamic.go index b10239bac8127..e643e4a92f332 100644 --- a/modules/options/dynamic.go +++ b/modules/options/dynamic.go @@ -34,7 +34,7 @@ func Dir(name string) ([]string, error) { return directories.AddAndGet(name, result), nil } -// fileFromOptionsDir is a helper to read files from static or custom path. +// fileFromOptionsDir is a helper to read files from custom or static path. func fileFromOptionsDir(elems ...string) ([]byte, error) { return readFileFromLocal([]string{setting.CustomPath, setting.StaticRootPath}, "options", elems...) } diff --git a/modules/options/static.go b/modules/options/static.go index 37c952d7853c5..72a2569843c70 100644 --- a/modules/options/static.go +++ b/modules/options/static.go @@ -8,8 +8,10 @@ package options import ( "fmt" "io" + "path/filepath" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) var directories = make(directorySet) @@ -62,11 +64,12 @@ func AssetDir(dirName string) ([]string, error) { // 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 := readFileFromLocal([]string{setting.CustomPath}, "options", elems...); err == nil { return data, nil } - f, err := Assets.Open(name) + f, err := Assets.Open(util.SafePathRelX(elems...)) if err != nil { return nil, err } From 5bc51f93ce5d14133e26ef6e40731fa10da7d4d1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 14:09:29 +0800 Subject: [PATCH 07/11] fix legacy absolute problem --- modules/options/base.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/modules/options/base.go b/modules/options/base.go index bbd6da0abd817..47c4e406e5f57 100644 --- a/modules/options/base.go +++ b/modules/options/base.go @@ -100,6 +100,14 @@ func readFileFromLocal(base []string, sub string, elems ...string) ([]byte, erro copy(localPathElems[2:], elems) for _, dir := range base { + if !filepath.IsAbs(dir) { + // FIXME: the old behavior (CustomPath or StaticRootPath might not be absolute), not ideal, just keep the same as before + var err error + dir, err = filepath.Abs(dir) + if err != nil { + return nil, fmt.Errorf("unable to get absolute path for %q. %w", dir, err) + } + } localPathElems[0] = dir localPath := util.SafeFilePathAbs(localPathElems...) data, err := os.ReadFile(localPath) @@ -109,5 +117,5 @@ func readFileFromLocal(base []string, sub string, elems ...string) ([]byte, erro log.Error("Unable to read file %q. Error: %v", localPath, err) } } - return nil, fmt.Errorf("local file does not exist: %v", elems) + return nil, os.ErrNotExist } From 86974ffd34605da4e1f352a2bc9d5743302aa441 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 14:45:25 +0800 Subject: [PATCH 08/11] tolerate the legacy relative path behavior --- modules/options/base.go | 60 +++++++++++++++++++++++--------------- modules/options/dynamic.go | 19 +++--------- modules/options/static.go | 19 +++--------- 3 files changed, 44 insertions(+), 54 deletions(-) diff --git a/modules/options/base.go b/modules/options/base.go index 47c4e406e5f57..3f808ceb093c8 100644 --- a/modules/options/base.go +++ b/modules/options/base.go @@ -14,6 +14,8 @@ import ( "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 fileFromOptionsDir("locale", name) @@ -79,37 +81,47 @@ 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) + 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.SafeFilePathAbs(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 static directory %s 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 readFileFromLocal(base []string, sub string, elems ...string) ([]byte, error) { - localPathElems := make([]string, len(elems)+2) // path[0] will be used for the custom path prefix - localPathElems[1] = sub - copy(localPathElems[2:], elems) - - for _, dir := range base { - if !filepath.IsAbs(dir) { - // FIXME: the old behavior (CustomPath or StaticRootPath might not be absolute), not ideal, just keep the same as before - var err error - dir, err = filepath.Abs(dir) - if err != nil { - return nil, fmt.Errorf("unable to get absolute path for %q. %w", dir, err) - } - } - localPathElems[0] = dir - localPath := util.SafeFilePathAbs(localPathElems...) +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 diff --git a/modules/options/dynamic.go b/modules/options/dynamic.go index e643e4a92f332..3d6261983f2de 100644 --- a/modules/options/dynamic.go +++ b/modules/options/dynamic.go @@ -7,28 +7,17 @@ package options import ( "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{ - util.SafeFilePathAbs(setting.CustomPath, "options", name), // custom dir - util.SafeFilePathAbs(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 @@ -36,7 +25,7 @@ func Dir(name string) ([]string, error) { // fileFromOptionsDir is a helper to read files from custom or static path. func fileFromOptionsDir(elems ...string) ([]byte, error) { - return readFileFromLocal([]string{setting.CustomPath, setting.StaticRootPath}, "options", elems...) + 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 72a2569843c70..359e2dd2605af 100644 --- a/modules/options/static.go +++ b/modules/options/static.go @@ -8,31 +8,20 @@ package options import ( "fmt" "io" - "path/filepath" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" ) -var directories = make(directorySet) - // 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{ - filepath.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) @@ -65,7 +54,7 @@ func AssetDir(dirName string) ([]string, error) { // 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 := readFileFromLocal([]string{setting.CustomPath}, "options", elems...); err == nil { + if data, err := readLocalFile([]string{setting.CustomPath}, "options", elems...); err == nil { return data, nil } From 1fb8a535c2c81ef51a6e2c78b0be806bb9ba7c2c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 19:40:58 +0800 Subject: [PATCH 09/11] fine tune comments and messages --- modules/options/base.go | 3 ++- modules/util/path.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/options/base.go b/modules/options/base.go index 3f808ceb093c8..c5e7ac732299c 100644 --- a/modules/options/base.go +++ b/modules/options/base.go @@ -86,6 +86,7 @@ func walkAssetDir(root string, callback func(path, name string, d fs.DirEntry, e func mustLocalPathAbs(s string) string { abs, err := filepath.Abs(s) if err != nil { + // 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) } return abs @@ -106,7 +107,7 @@ func listLocalDirIfExist(baseDirs []string, subDir string, elems ...string) (fil for _, localPath := range joinLocalPaths(baseDirs, subDir, elems...) { isDir, err := util.IsDir(localPath) if err != nil { - return nil, fmt.Errorf("unable to check if static directory %s is a directory. %w", localPath, err) + return nil, fmt.Errorf("unable to check if path %q is a directory. %w", localPath, err) } else if !isDir { continue } diff --git a/modules/util/path.go b/modules/util/path.go index 2f12a3bf17260..126735f25cb8a 100644 --- a/modules/util/path.go +++ b/modules/util/path.go @@ -5,6 +5,7 @@ package util import ( "errors" + "fmt" "net/url" "os" "path" @@ -82,7 +83,7 @@ func SafeFilePathAbs(elem ...string) string { } 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("SafeFilePathAbs: result is not absolute, do not guess a relative path based on current working directory") + panic(fmt.Sprintf("SafeFilePathAbs: %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++ { From e94155b8d1253a1a914417c5e07bcea958a3a7f5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 21:29:39 +0800 Subject: [PATCH 10/11] rename to PathJoin (instead of SafeXxx), add tests, add comments --- models/git/lfs_lock.go | 6 +++--- modules/options/base.go | 2 +- modules/options/static.go | 2 +- modules/public/public.go | 2 +- modules/storage/local.go | 2 +- modules/storage/minio.go | 2 +- modules/util/path.go | 21 +++++++++++--------- modules/util/path_test.go | 10 +++++++--- routers/web/base.go | 4 ++-- routers/web/repo/editor.go | 2 +- routers/web/repo/lfs.go | 2 +- services/migrations/gitea_uploader.go | 2 +- services/packages/container/blob_uploader.go | 2 +- services/repository/files/file.go | 2 +- 14 files changed, 34 insertions(+), 27 deletions(-) diff --git a/models/git/lfs_lock.go b/models/git/lfs_lock.go index 2e73d606ab0bd..261c73032aa21 100644 --- a/models/git/lfs_lock.go +++ b/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 { diff --git a/modules/options/base.go b/modules/options/base.go index c5e7ac732299c..7882ed008159a 100644 --- a/modules/options/base.go +++ b/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 } diff --git a/modules/options/static.go b/modules/options/static.go index 359e2dd2605af..0482dea6817ce 100644 --- a/modules/options/static.go +++ b/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 } diff --git a/modules/public/public.go b/modules/public/public.go index f6b90403d3586..30b03a27954a2 100644 --- a/modules/public/public.go +++ b/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 diff --git a/modules/storage/local.go b/modules/storage/local.go index 25821b503c8aa..d22974a65add0 100644 --- a/modules/storage/local.go +++ b/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 diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 946268f52462e..5c67dbf26a298 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 util.SafePathRelX(m.basePath, p) + return util.PathJoinRelX(m.basePath, p) } // Open open a file diff --git a/modules/util/path.go b/modules/util/path.go index 126735f25cb8a..b89e6d576a43c 100644 --- a/modules/util/path.go +++ b/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,14 +45,15 @@ 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 == "" { @@ -59,19 +61,20 @@ func SafePathRelX(elem ...string) string { } 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 diff --git a/modules/util/path_test.go b/modules/util/path_test.go index 4a5f02e735e1c..1d27c9bf0c0f8 100644 --- a/modules/util/path_test.go +++ b/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) } } diff --git a/routers/web/base.go b/routers/web/base.go index 3a81ce5d53123..da18a75643e68 100644 --- a/routers/web/base.go +++ b/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 diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index a94b09d8f8f05..07241b88700fb 100644 --- a/routers/web/repo/editor.go +++ b/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" { diff --git a/routers/web/repo/lfs.go b/routers/web/repo/lfs.go index eb5bf49c37c51..9957869c999a3 100644 --- a/routers/web/repo/lfs.go +++ b/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") diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index e9cc89c00cac0..0eb34b5fe562f 100644 --- a/services/migrations/gitea_uploader.go +++ b/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() diff --git a/services/packages/container/blob_uploader.go b/services/packages/container/blob_uploader.go index 68aceff4bc425..bae2e2d6af667 100644 --- a/services/packages/container/blob_uploader.go +++ b/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 diff --git a/services/repository/files/file.go b/services/repository/files/file.go index 1276fc1133d1a..dc1e547dcdae9 100644 --- a/services/repository/files/file.go +++ b/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" { From 41918027752eaad4dcc67db3a20d7d34b10803d3 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 21 Mar 2023 21:55:04 +0800 Subject: [PATCH 11/11] fix message --- modules/util/path.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/util/path.go b/modules/util/path.go index b89e6d576a43c..37d06e9813e07 100644 --- a/modules/util/path.go +++ b/modules/util/path.go @@ -86,7 +86,7 @@ func FilePathJoinAbs(elem ...string) string { } 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("SafeFilePathAbs: %q (for path %v) is not absolute, do not guess a relative path based on current working directory", elems[0], elems)) + 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++ {