Skip to content

Commit 32dfaa7

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent 0d7cf7b commit 32dfaa7

File tree

13 files changed

+159
-61
lines changed

13 files changed

+159
-61
lines changed

‎models/git/lfs_lock.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ func init() {
3434

3535
// BeforeInsert is invoked from XORM before inserting an object of this type.
3636
func (l *LFSLock) BeforeInsert() {
37-
l.Path = util.CleanPath(l.Path)
37+
l.Path = util.SafePathRel(l.Path)
3838
}
3939

4040
// CreateLFSLock creates a new lock.
@@ -49,7 +49,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
4949
return nil, err
5050
}
5151

52-
lock.Path = util.CleanPath(lock.Path)
52+
lock.Path = util.SafePathRel(lock.Path)
5353
lock.RepoID = repo.ID
5454

5555
l, err := GetLFSLock(dbCtx, repo, lock.Path)
@@ -69,7 +69,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
6969

7070
// GetLFSLock returns release by given path.
7171
func GetLFSLock(ctx context.Context, repo *repo_model.Repository, path string) (*LFSLock, error) {
72-
path = util.CleanPath(path)
72+
path = util.SafePathRel(path)
7373
rel := &LFSLock{RepoID: repo.ID}
7474
has, err := db.GetEngine(ctx).Where("lower(path) = ?", strings.ToLower(path)).Get(rel)
7575
if err != nil {

‎modules/options/base.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,27 +16,27 @@ import (
1616

1717
// Locale reads the content of a specific locale from static/bindata or custom path.
1818
func Locale(name string) ([]byte, error) {
19-
return fileFromDir(path.Join("locale", util.CleanPath(name)))
19+
return fileFromDir(path.Join("locale", util.SafePathRel(name)))
2020
}
2121

2222
// Readme reads the content of a specific readme from static/bindata or custom path.
2323
func Readme(name string) ([]byte, error) {
24-
return fileFromDir(path.Join("readme", util.CleanPath(name)))
24+
return fileFromDir(path.Join("readme", util.SafePathRel(name)))
2525
}
2626

2727
// Gitignore reads the content of a gitignore locale from static/bindata or custom path.
2828
func Gitignore(name string) ([]byte, error) {
29-
return fileFromDir(path.Join("gitignore", util.CleanPath(name)))
29+
return fileFromDir(path.Join("gitignore", util.SafePathRel(name)))
3030
}
3131

3232
// License reads the content of a specific license from static/bindata or custom path.
3333
func License(name string) ([]byte, error) {
34-
return fileFromDir(path.Join("license", util.CleanPath(name)))
34+
return fileFromDir(path.Join("license", util.SafePathRel(name)))
3535
}
3636

3737
// Labels reads the content of a specific labels from static/bindata or custom path.
3838
func Labels(name string) ([]byte, error) {
39-
return fileFromDir(path.Join("label", util.CleanPath(name)))
39+
return fileFromDir(path.Join("label", util.SafePathRel(name)))
4040
}
4141

4242
// WalkLocales reads the content of a specific locale

‎modules/public/public.go

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -45,29 +45,19 @@ func AssetsHandlerFunc(opts *Options) http.HandlerFunc {
4545
return
4646
}
4747

48-
file := req.URL.Path
49-
file = file[len(opts.Prefix):]
50-
if len(file) == 0 {
51-
resp.WriteHeader(http.StatusNotFound)
52-
return
53-
}
54-
if strings.Contains(file, "\\") {
55-
resp.WriteHeader(http.StatusBadRequest)
56-
return
57-
}
58-
file = "/" + file
59-
60-
var written bool
48+
var corsSent bool
6149
if opts.CorsHandler != nil {
62-
written = true
6350
opts.CorsHandler(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
64-
written = false
51+
corsSent = true
6552
})).ServeHTTP(resp, req)
6653
}
67-
if written {
54+
// If CORS is not sent, the response must have been written by other handlers
55+
if !corsSent {
6856
return
6957
}
7058

59+
file := req.URL.Path[len(opts.Prefix):]
60+
7161
// custom files
7262
if opts.handle(resp, req, http.Dir(custPath), file) {
7363
return
@@ -102,8 +92,8 @@ func setWellKnownContentType(w http.ResponseWriter, file string) {
10292
}
10393

10494
func (opts *Options) handle(w http.ResponseWriter, req *http.Request, fs http.FileSystem, file string) bool {
105-
// use clean to keep the file is a valid path with no . or ..
106-
f, err := fs.Open(util.CleanPath(file))
95+
// 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
96+
f, err := fs.Open(util.SafePathRelX(file))
10797
if err != nil {
10898
if os.IsNotExist(err) {
10999
return false

‎modules/storage/local.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"net/url"
1010
"os"
1111
"path/filepath"
12-
"strings"
1312

1413
"code.gitea.io/gitea/modules/log"
1514
"code.gitea.io/gitea/modules/util"
@@ -58,7 +57,7 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
5857
}
5958

6059
func (l *LocalStorage) buildLocalPath(p string) string {
61-
return filepath.Join(l.dir, util.CleanPath(strings.ReplaceAll(p, "\\", "/")))
60+
return util.SafeFilePathAbs(l.dir, p)
6261
}
6362

6463
// Open a file
@@ -128,10 +127,7 @@ func (l *LocalStorage) URL(path, name string) (*url.URL, error) {
128127

129128
// IterateObjects iterates across the objects in the local storage
130129
func (l *LocalStorage) IterateObjects(prefix string, fn func(path string, obj Object) error) error {
131-
dir := l.dir
132-
if prefix != "" {
133-
dir = filepath.Join(l.dir, util.CleanPath(prefix))
134-
}
130+
dir := l.buildLocalPath(prefix)
135131
return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error {
136132
if err != nil {
137133
return err

‎modules/storage/minio.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
121121
}
122122

123123
func (m *MinioStorage) buildMinioPath(p string) string {
124-
return strings.TrimPrefix(path.Join(m.basePath, util.CleanPath(strings.ReplaceAll(p, "\\", "/"))), "/")
124+
return util.SafePathRelX(m.basePath, p)
125125
}
126126

127127
// Open open a file

‎modules/util/path.go

Lines changed: 75 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,21 +14,85 @@ import (
1414
"strings"
1515
)
1616

17-
// CleanPath ensure to clean the path
18-
func CleanPath(p string) string {
19-
if strings.HasPrefix(p, "/") {
20-
return path.Clean(p)
17+
// SafePathRel joins the path elements into a single path, each element is cleaned separately
18+
// It only returns the following values (like path.Join):
19+
//
20+
// empty => ``
21+
// `` => ``
22+
// `..` => `.`
23+
// `dir` => `dir`
24+
// `/dir/` => `dir`
25+
// `foo\..\bar` => `foo\..\bar`
26+
//
27+
// any redundant part(relative dots, slashes) is removed.
28+
func SafePathRel(elem ...string) string {
29+
elems := make([]string, len(elem))
30+
for i, e := range elem {
31+
if e == "" {
32+
continue
33+
}
34+
elems[i] = path.Clean("/" + e)
35+
}
36+
p := path.Join(elems...)
37+
if p == "" {
38+
return ""
39+
} else if p == "/" {
40+
return "."
41+
} else {
42+
return p[1:]
2143
}
22-
return path.Clean("/" + p)[1:]
2344
}
2445

25-
// EnsureAbsolutePath ensure that a path is absolute, making it
26-
// relative to absoluteBase if necessary
27-
func EnsureAbsolutePath(path, absoluteBase string) string {
28-
if filepath.IsAbs(path) {
29-
return path
46+
// SafePathRelX joins the path elements into a single path like SafePathRel,
47+
// and covert all backslashes to slashes. (X means "extended", also means the combination of `\` and `/`)
48+
// It returns similar results as SafePathRel except:
49+
//
50+
// `foo\..\bar` => `bar` (because it's processed as `foo/../bar`)
51+
//
52+
// any redundant part(relative dots, slashes) is removed.
53+
func SafePathRelX(elem ...string) string {
54+
elems := make([]string, len(elem))
55+
for i, e := range elem {
56+
if e == "" {
57+
continue
58+
}
59+
elems[i] = path.Clean("/" + strings.ReplaceAll(e, "\\", "/"))
60+
}
61+
return SafePathRel(elems...)
62+
}
63+
64+
const pathSeparator = string(os.PathSeparator)
65+
66+
// SafeFilePathAbs joins the path elements into a single file path, each element is cleaned separately
67+
// and convert all slashes/backslashes to path separators.
68+
// The first element must be an absolute path, caller should prepare the base path
69+
func SafeFilePathAbs(elem ...string) string {
70+
elems := make([]string, len(elem))
71+
72+
// POISX filesystem can have `\` in file names. Windows: `\` and `/` are both used for path separators
73+
// to keep the behavior consistent, we do not allow `\` in file names, replace all `\` with `/`
74+
if isOSWindows() {
75+
elems[0] = filepath.Clean(elem[0])
76+
} else {
77+
elems[0] = filepath.Clean(strings.ReplaceAll(elem[0], "\\", pathSeparator))
78+
}
79+
if !filepath.IsAbs(elems[0]) {
80+
// This shouldn't happen. If there is really necessary to pass in relative path, return the full path with filepath.Abs() instead
81+
panic("FilePathJoinAbs: result is not absolute, do not guess a relative path based on current working directory")
82+
}
83+
84+
for i := 1; i < len(elem); i++ {
85+
if elem[i] == "" {
86+
continue
87+
}
88+
if isOSWindows() {
89+
elems[i] = filepath.Clean(pathSeparator + elem[i])
90+
} else {
91+
elems[i] = filepath.Clean(pathSeparator + strings.ReplaceAll(elem[i], "\\", pathSeparator))
92+
}
3093
}
31-
return filepath.Join(absoluteBase, path)
94+
// the elems[0] must be an absolute path, just join them together
95+
return filepath.Join(elems...)
3296
}
3397

3498
// IsDir returns true if given path is a directory,

‎modules/util/path_test.go

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,63 @@ func TestMisc_IsReadmeFileName(t *testing.T) {
138138
}
139139

140140
func TestCleanPath(t *testing.T) {
141-
cases := map[string]string{
142-
"../../test": "test",
143-
"/test": "/test",
144-
"/../test": "/test",
141+
cases := []struct {
142+
elems []string
143+
expected string
144+
}{
145+
{[]string{}, ``},
146+
{[]string{``}, ``},
147+
{[]string{`..`}, `.`},
148+
{[]string{`a`}, `a`},
149+
{[]string{`/a/`}, `a`},
150+
{[]string{`../a/`, `../b`, `c/..`, `d`}, `a/b/d`},
151+
{[]string{`a\..\b`}, `a\..\b`},
152+
}
153+
for _, c := range cases {
154+
assert.Equal(t, c.expected, SafePathRel(c.elems...), "case: %v", c.elems)
155+
}
156+
157+
cases = []struct {
158+
elems []string
159+
expected string
160+
}{
161+
{[]string{}, ``},
162+
{[]string{``}, ``},
163+
{[]string{`..`}, `.`},
164+
{[]string{`a`}, `a`},
165+
{[]string{`/a/`}, `a`},
166+
{[]string{`../a/`, `../b`, `c/..`, `d`}, `a/b/d`},
167+
{[]string{`a\..\b`}, `b`},
168+
}
169+
for _, c := range cases {
170+
assert.Equal(t, c.expected, SafePathRelX(c.elems...), "case: %v", c.elems)
145171
}
146172

147-
for k, v := range cases {
148-
assert.Equal(t, v, CleanPath(k))
173+
// for POSIX only, but the result is similar on Windows, because the first element must be an absolute path
174+
if isOSWindows() {
175+
cases = []struct {
176+
elems []string
177+
expected string
178+
}{
179+
{[]string{`C:\..`}, `C:\`},
180+
{[]string{`C:\a`}, `C:\a`},
181+
{[]string{`C:\a/`}, `C:\a`},
182+
{[]string{`C:\..\a\`, `../b`, `c\..`, `d`}, `C:\a\b\d`},
183+
{[]string{`C:\a/..\b`}, `C:\b`},
184+
}
185+
} else {
186+
cases = []struct {
187+
elems []string
188+
expected string
189+
}{
190+
{[]string{`/..`}, `/`},
191+
{[]string{`/a`}, `/a`},
192+
{[]string{`/a/`}, `/a`},
193+
{[]string{`/../a/`, `../b`, `c/..`, `d`}, `/a/b/d`},
194+
{[]string{`/a\..\b`}, `/b`},
195+
}
196+
}
197+
for _, c := range cases {
198+
assert.Equal(t, c.expected, SafeFilePathAbs(c.elems...), "case: %v", c.elems)
149199
}
150200
}

‎routers/web/base.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
4545
routing.UpdateFuncInfo(req.Context(), funcInfo)
4646

4747
rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
48-
rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
48+
rPath = util.SafePathRelX(rPath)
4949

5050
u, err := objStore.URL(rPath, path.Base(rPath))
5151
if err != nil {
@@ -81,8 +81,8 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
8181
routing.UpdateFuncInfo(req.Context(), funcInfo)
8282

8383
rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
84-
rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
85-
if rPath == "" {
84+
rPath = util.SafePathRelX(rPath)
85+
if rPath == "" || rPath == "." {
8686
http.Error(w, "file not found", http.StatusNotFound)
8787
return
8888
}

‎routers/web/repo/editor.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,7 +726,7 @@ func UploadFilePost(ctx *context.Context) {
726726

727727
func cleanUploadFileName(name string) string {
728728
// Rebase the filename
729-
name = strings.Trim(util.CleanPath(name), "/")
729+
name = util.SafePathRel(name)
730730
// Git disallows any filenames to have a .git directory in them.
731731
for _, part := range strings.Split(name, "/") {
732732
if strings.ToLower(part) == ".git" {

‎routers/web/repo/lfs.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ func LFSLockFile(ctx *context.Context) {
207207
ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
208208
return
209209
}
210-
lockPath = util.CleanPath(lockPath)
210+
lockPath = util.SafePathRel(lockPath)
211211
if len(lockPath) == 0 {
212212
ctx.Flash.Error(ctx.Tr("repo.settings.lfs_invalid_locking_path", originalPath))
213213
ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")

‎services/migrations/gitea_uploader.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -865,8 +865,8 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
865865
_, _, line, _ = git.ParseDiffHunkString(comment.DiffHunk)
866866
}
867867

868-
// SECURITY: The TreePath must be cleaned!
869-
comment.TreePath = util.CleanPath(comment.TreePath)
868+
// SECURITY: The TreePath must be cleaned! use relative path
869+
comment.TreePath = util.SafePathRel(comment.TreePath)
870870

871871
var patch string
872872
reader, writer := io.Pipe()

‎services/packages/container/blob_uploader.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ import (
88
"errors"
99
"io"
1010
"os"
11-
"path/filepath"
12-
"strings"
1311

1412
packages_model "code.gitea.io/gitea/models/packages"
1513
packages_module "code.gitea.io/gitea/modules/packages"
@@ -33,7 +31,7 @@ type BlobUploader struct {
3331
}
3432

3533
func buildFilePath(id string) string {
36-
return filepath.Join(setting.Packages.ChunkedUploadPath, util.CleanPath(strings.ReplaceAll(id, "\\", "/")))
34+
return util.SafeFilePathAbs(setting.Packages.ChunkedUploadPath, id)
3735
}
3836

3937
// NewBlobUploader creates a new blob uploader for the given id

‎services/repository/files/file.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func GetAuthorAndCommitterUsers(author, committer *IdentityOptions, doer *user_m
129129
// CleanUploadFileName Trims a filename and returns empty string if it is a .git directory
130130
func CleanUploadFileName(name string) string {
131131
// Rebase the filename
132-
name = strings.Trim(util.CleanPath(name), "/")
132+
name = util.SafePathRel(name)
133133
// Git disallows any filenames to have a .git directory in them.
134134
for _, part := range strings.Split(name, "/") {
135135
if strings.ToLower(part) == ".git" {

0 commit comments

Comments
 (0)