Skip to content

Commit 7994b2f

Browse files
zeripathlafriks
andcommitted
Clean paths when looking in Storage (go-gitea#19124)
Backport go-gitea#19124 * Clean paths when looking in Storage Ensure paths are clean for minio aswell as local storage. Use url.Path not RequestURI/EscapedPath in storageHandler. Signed-off-by: Andrew Thornton <[email protected]> * Apply suggestions from code review Co-authored-by: Lauris BH <[email protected]>
1 parent 23b8214 commit 7994b2f

File tree

4 files changed

+46
-56
lines changed

4 files changed

+46
-56
lines changed

modules/storage/local.go

+8-26
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ package storage
66

77
import (
88
"context"
9-
"errors"
109
"io"
1110
"net/url"
1211
"os"
@@ -18,8 +17,6 @@ import (
1817
"code.gitea.io/gitea/modules/util"
1918
)
2019

21-
// ErrLocalPathNotSupported represents an error that path is not supported
22-
var ErrLocalPathNotSupported = errors.New("local path is not supported")
2320
var _ ObjectStorage = &LocalStorage{}
2421

2522
// LocalStorageType is the type descriptor for local storage
@@ -62,21 +59,18 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
6259
}, nil
6360
}
6461

62+
func (l *LocalStorage) buildLocalPath(p string) string {
63+
return filepath.Join(l.dir, path.Clean("/" + strings.ReplaceAll(p, "\\", "/"))[1:])
64+
}
65+
6566
// Open a file
6667
func (l *LocalStorage) Open(path string) (Object, error) {
67-
if !isLocalPathValid(path) {
68-
return nil, ErrLocalPathNotSupported
69-
}
70-
return os.Open(filepath.Join(l.dir, path))
68+
return os.Open(l.buildLocalPath(path))
7169
}
7270

7371
// Save a file
7472
func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) {
75-
if !isLocalPathValid(path) {
76-
return 0, ErrLocalPathNotSupported
77-
}
78-
79-
p := filepath.Join(l.dir, path)
73+
p := l.buildLocalPath(path)
8074
if err := os.MkdirAll(filepath.Dir(p), os.ModePerm); err != nil {
8175
return 0, err
8276
}
@@ -116,24 +110,12 @@ func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error)
116110

117111
// Stat returns the info of the file
118112
func (l *LocalStorage) Stat(path string) (os.FileInfo, error) {
119-
return os.Stat(filepath.Join(l.dir, path))
120-
}
121-
122-
func isLocalPathValid(p string) bool {
123-
a := path.Clean(p)
124-
if strings.HasPrefix(a, "../") || strings.HasPrefix(a, "..\\") {
125-
return false
126-
}
127-
return a == p
113+
return os.Stat(l.buildLocalPath(path))
128114
}
129115

130116
// Delete delete a file
131117
func (l *LocalStorage) Delete(path string) error {
132-
if !isLocalPathValid(path) {
133-
return ErrLocalPathNotSupported
134-
}
135-
p := filepath.Join(l.dir, path)
136-
return util.Remove(p)
118+
return util.Remove(l.buildLocalPath(path))
137119
}
138120

139121
// URL gets the redirect URL to a file

modules/storage/local_test.go

+21-13
Original file line numberDiff line numberDiff line change
@@ -10,36 +10,44 @@ import (
1010
"github.com/stretchr/testify/assert"
1111
)
1212

13-
func TestLocalPathIsValid(t *testing.T) {
13+
func TestBuildLocalPath(t *testing.T) {
1414
kases := []struct {
15-
path string
16-
valid bool
15+
localDir string
16+
path string
17+
expected string
1718
}{
1819
{
20+
"a",
21+
"0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
1922
"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
20-
true,
2123
},
2224
{
23-
"../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
24-
false,
25+
"a",
26+
"../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
27+
"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
2528
},
2629
{
27-
"a\\0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
28-
true,
30+
"a",
31+
"0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
32+
"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
2933
},
3034
{
31-
"b/../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
32-
false,
35+
"b",
36+
"a/../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
37+
"b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
3338
},
3439
{
35-
"..\\a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
36-
false,
40+
"b",
41+
"a\\..\\0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
42+
"b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
3743
},
3844
}
3945

4046
for _, k := range kases {
4147
t.Run(k.path, func(t *testing.T) {
42-
assert.EqualValues(t, k.valid, isLocalPathValid(k.path))
48+
l := LocalStorage{dir: k.localDir}
49+
50+
assert.EqualValues(t, k.expected, l.buildLocalPath(k.path))
4351
})
4452
}
4553
}

modules/storage/minio.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
117117
}
118118

119119
func (m *MinioStorage) buildMinioPath(p string) string {
120-
return strings.TrimPrefix(path.Join(m.basePath, p), "/")
120+
return strings.TrimPrefix(path.Join(m.basePath, path.Clean("/" + strings.ReplaceAll(p, "\\", "/"))[1:]), "/")
121121
}
122122

123123
// Open open a file

routers/web/base.go

+16-16
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"net/http"
1212
"os"
1313
"path"
14-
"path/filepath"
1514
"strings"
1615

1716
"code.gitea.io/gitea/modules/context"
@@ -27,6 +26,8 @@ import (
2726
)
2827

2928
func storageHandler(storageSetting setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler {
29+
prefix = strings.Trim(prefix, "/")
30+
3031
return func(next http.Handler) http.Handler {
3132
if storageSetting.ServeDirect {
3233
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
@@ -35,12 +36,14 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
3536
return
3637
}
3738

38-
if !strings.HasPrefix(req.URL.RequestURI(), "/"+prefix) {
39+
if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") {
3940
next.ServeHTTP(w, req)
4041
return
4142
}
4243

43-
rPath := strings.TrimPrefix(req.URL.RequestURI(), "/"+prefix)
44+
rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
45+
rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
46+
4447
u, err := objStore.URL(rPath, path.Base(rPath))
4548
if err != nil {
4649
if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) {
@@ -52,11 +55,12 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
5255
http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), 500)
5356
return
5457
}
58+
5559
http.Redirect(
5660
w,
5761
req,
5862
u.String(),
59-
301,
63+
http.StatusMovedPermanently,
6064
)
6165
})
6266
}
@@ -67,28 +71,24 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
6771
return
6872
}
6973

70-
prefix := strings.Trim(prefix, "/")
71-
72-
if !strings.HasPrefix(req.URL.EscapedPath(), "/"+prefix+"/") {
74+
if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") {
7375
next.ServeHTTP(w, req)
7476
return
7577
}
7678

77-
rPath := strings.TrimPrefix(req.URL.EscapedPath(), "/"+prefix+"/")
78-
rPath = strings.TrimPrefix(rPath, "/")
79+
rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
80+
rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
7981
if rPath == "" {
8082
http.Error(w, "file not found", 404)
8183
return
8284
}
83-
rPath = path.Clean("/" + filepath.ToSlash(rPath))
84-
rPath = rPath[1:]
8585

8686
fi, err := objStore.Stat(rPath)
8787
if err == nil && httpcache.HandleTimeCache(req, w, fi) {
8888
return
8989
}
9090

91-
//If we have matched and access to release or issue
91+
// If we have matched and access to release or issue
9292
fr, err := objStore.Open(rPath)
9393
if err != nil {
9494
if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) {
@@ -121,7 +121,7 @@ func (d *dataStore) GetData() map[string]interface{} {
121121
// Recovery returns a middleware that recovers from any panics and writes a 500 and a log if so.
122122
// This error will be created with the gitea 500 page.
123123
func Recovery() func(next http.Handler) http.Handler {
124-
var rnd = templates.HTMLRenderer()
124+
rnd := templates.HTMLRenderer()
125125
return func(next http.Handler) http.Handler {
126126
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
127127
defer func() {
@@ -131,14 +131,14 @@ func Recovery() func(next http.Handler) http.Handler {
131131

132132
sessionStore := session.GetSession(req)
133133

134-
var lc = middleware.Locale(w, req)
135-
var store = dataStore{
134+
lc := middleware.Locale(w, req)
135+
store := dataStore{
136136
"Language": lc.Language(),
137137
"CurrentURL": setting.AppSubURL + req.URL.RequestURI(),
138138
"i18n": lc,
139139
}
140140

141-
var user = context.GetContextUser(req)
141+
user := context.GetContextUser(req)
142142
if user == nil {
143143
// Get user from session if logged in - do not attempt to sign-in
144144
user = auth.SessionUser(sessionStore)

0 commit comments

Comments
 (0)