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 <art27@cantab.net>

* Apply suggestions from code review

Co-authored-by: Lauris BH <lauris@nix.lv>

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 os.Open(l.buildLocalPath(path))
-		return nil, ErrLocalPathNotSupported
-	}
-	return os.Open(filepath.Join(l.dir, path))
 }
 
 // Save a file
 func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) {
-	if !isLocalPathValid(path) {
+	p := l.buildLocalPath(path)
-		return 0, ErrLocalPathNotSupported
-	}
-
-	p := filepath.Join(l.dir, 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))
+	return os.Stat(l.buildLocalPath(path))
-}
-
-func isLocalPathValid(p string) bool {
-	a := path.Clean(p)
-	if strings.HasPrefix(a, "../") || strings.HasPrefix(a, "..\\") {
-		return false
-	}
-	return a == p
 }
 
 // Delete delete a file
 func (l *LocalStorage) Delete(path string) error {
-	if !isLocalPathValid(path) {
+	return util.Remove(l.buildLocalPath(path))
-		return ErrLocalPathNotSupported
-	}
-	p := filepath.Join(l.dir, path)
-	return util.Remove(p)
 }
 
 // URL gets the redirect URL to a file

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
+		localDir string
-		valid bool
+		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",
+			"a",
-			false,
+			"../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+			"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
 		},
 		{
-			"a\\0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+			"a",
-			true,
+			"0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+			"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
 		},
 		{
-			"b/../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+			"b",
-			false,
+			"a/../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+			"b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
 		},
 		{
-			"..\\a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
+			"b",
-			false,
+			"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))
 		})
 	}
 }

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

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.Path, "/"+prefix+"/") {
-
-			if !strings.HasPrefix(req.URL.EscapedPath(), "/"+prefix+"/") {
 				next.ServeHTTP(w, req)
 				return
 			}
 
-			rPath := strings.TrimPrefix(req.URL.EscapedPath(), "/"+prefix+"/")
+			rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-			rPath = strings.TrimPrefix(rPath, "/")
+			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)
+					lc := middleware.Locale(w, req)
-					var store = dataStore{
+					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)