From d21b7fd3af2bd9081c704c9dcfe7a6f0831ddedd Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 23 Mar 2022 09:23:00 +0000 Subject: [PATCH] Clean paths when looking in Storage (#19124) (#19179) Backport #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 * Apply suggestions from code review Co-authored-by: Lauris BH --- modules/storage/local.go | 34 ++++++++-------------------------- modules/storage/local_test.go | 34 +++++++++++++++++++++------------- modules/storage/minio.go | 2 +- routers/web/base.go | 32 ++++++++++++++++---------------- 4 files changed, 46 insertions(+), 56 deletions(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index 8d9aa603d0..701b0b1a9f 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -6,7 +6,6 @@ package storage import ( "context" - "errors" "io" "net/url" "os" @@ -18,8 +17,6 @@ import ( "code.gitea.io/gitea/modules/util" ) -// ErrLocalPathNotSupported represents an error that path is not supported -var ErrLocalPathNotSupported = errors.New("local path is not supported") var _ ObjectStorage = &LocalStorage{} // LocalStorageType is the type descriptor for local storage @@ -62,21 +59,18 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error }, nil } +func (l *LocalStorage) buildLocalPath(p string) string { + return filepath.Join(l.dir, path.Clean("/" + strings.ReplaceAll(p, "\\", "/"))[1:]) +} + // Open a file func (l *LocalStorage) Open(path string) (Object, error) { - if !isLocalPathValid(path) { - return nil, ErrLocalPathNotSupported - } - return os.Open(filepath.Join(l.dir, path)) + return os.Open(l.buildLocalPath(path)) } // Save a file func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) { - if !isLocalPathValid(path) { - return 0, ErrLocalPathNotSupported - } - - p := filepath.Join(l.dir, path) + p := l.buildLocalPath(path) if err := os.MkdirAll(filepath.Dir(p), os.ModePerm); err != nil { return 0, err } @@ -116,24 +110,12 @@ func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) // Stat returns the info of the file func (l *LocalStorage) Stat(path string) (os.FileInfo, error) { - return os.Stat(filepath.Join(l.dir, path)) -} - -func isLocalPathValid(p string) bool { - a := path.Clean(p) - if strings.HasPrefix(a, "../") || strings.HasPrefix(a, "..\\") { - return false - } - return a == p + return os.Stat(l.buildLocalPath(path)) } // Delete delete a file func (l *LocalStorage) Delete(path string) error { - if !isLocalPathValid(path) { - return ErrLocalPathNotSupported - } - p := filepath.Join(l.dir, path) - return util.Remove(p) + return util.Remove(l.buildLocalPath(path)) } // URL gets the redirect URL to a file diff --git a/modules/storage/local_test.go b/modules/storage/local_test.go index 8714f37f0d..0749036cb7 100644 --- a/modules/storage/local_test.go +++ b/modules/storage/local_test.go @@ -10,36 +10,44 @@ import ( "github.com/stretchr/testify/assert" ) -func TestLocalPathIsValid(t *testing.T) { +func TestBuildLocalPath(t *testing.T) { kases := []struct { - path string - valid bool + localDir string + path string + expected string }{ { + "a", + "0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", "a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", - true, }, { - "../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", - false, + "a", + "../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", + "a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", }, { - "a\\0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", - true, + "a", + "0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", + "a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", }, { - "b/../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", - false, + "b", + "a/../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", + "b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", }, { - "..\\a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", - false, + "b", + "a\\..\\0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", + "b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14", }, } for _, k := range kases { t.Run(k.path, func(t *testing.T) { - assert.EqualValues(t, k.valid, isLocalPathValid(k.path)) + l := LocalStorage{dir: k.localDir} + + assert.EqualValues(t, k.expected, l.buildLocalPath(k.path)) }) } } diff --git a/modules/storage/minio.go b/modules/storage/minio.go index f78ba6aa27..638b67b761 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -117,7 +117,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error } func (m *MinioStorage) buildMinioPath(p string) string { - return strings.TrimPrefix(path.Join(m.basePath, p), "/") + return strings.TrimPrefix(path.Join(m.basePath, path.Clean("/" + strings.ReplaceAll(p, "\\", "/"))[1:]), "/") } // Open open a file diff --git a/routers/web/base.go b/routers/web/base.go index 98713bc881..dcd00f2c61 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -11,7 +11,6 @@ import ( "net/http" "os" "path" - "path/filepath" "strings" "code.gitea.io/gitea/modules/context" @@ -27,6 +26,8 @@ import ( ) func storageHandler(storageSetting setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler { + prefix = strings.Trim(prefix, "/") + return func(next http.Handler) http.Handler { if storageSetting.ServeDirect { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { @@ -35,12 +36,14 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor return } - if !strings.HasPrefix(req.URL.RequestURI(), "/"+prefix) { + if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { next.ServeHTTP(w, req) return } - rPath := strings.TrimPrefix(req.URL.RequestURI(), "/"+prefix) + rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") + rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:] + u, err := objStore.URL(rPath, path.Base(rPath)) if err != nil { if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { @@ -52,11 +55,12 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), 500) return } + http.Redirect( w, req, u.String(), - 301, + http.StatusMovedPermanently, ) }) } @@ -67,28 +71,24 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor return } - prefix := strings.Trim(prefix, "/") - - if !strings.HasPrefix(req.URL.EscapedPath(), "/"+prefix+"/") { + if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { next.ServeHTTP(w, req) return } - rPath := strings.TrimPrefix(req.URL.EscapedPath(), "/"+prefix+"/") - rPath = strings.TrimPrefix(rPath, "/") + rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") + rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:] if rPath == "" { http.Error(w, "file not found", 404) return } - rPath = path.Clean("/" + filepath.ToSlash(rPath)) - rPath = rPath[1:] fi, err := objStore.Stat(rPath) if err == nil && httpcache.HandleTimeCache(req, w, fi) { return } - //If we have matched and access to release or issue + // If we have matched and access to release or issue fr, err := objStore.Open(rPath) if err != nil { if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { @@ -121,7 +121,7 @@ func (d *dataStore) GetData() map[string]interface{} { // Recovery returns a middleware that recovers from any panics and writes a 500 and a log if so. // This error will be created with the gitea 500 page. func Recovery() func(next http.Handler) http.Handler { - var rnd = templates.HTMLRenderer() + rnd := templates.HTMLRenderer() return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { defer func() { @@ -131,14 +131,14 @@ func Recovery() func(next http.Handler) http.Handler { sessionStore := session.GetSession(req) - var lc = middleware.Locale(w, req) - var store = dataStore{ + lc := middleware.Locale(w, req) + store := dataStore{ "Language": lc.Language(), "CurrentURL": setting.AppSubURL + req.URL.RequestURI(), "i18n": lc, } - var user = context.GetContextUser(req) + user := context.GetContextUser(req) if user == nil { // Get user from session if logged in - do not attempt to sign-in user = auth.SessionUser(sessionStore)