Clean paths when looking in Storage (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 <art27@cantab.net> * Apply suggestions from code review Co-authored-by: Lauris BH <lauris@nix.lv>
zjjhot · Mar 22, 2022 · 3f71ab9 · 3f71ab9
1 parent d2c1658
commit 3f71ab9
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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
@@ -11,7 +11,6 @@ import (
 	"net/http"
 	"os"
 	"path"
-	"path/filepath"
 	"strings"
 
 	"code.gitea.io/gitea/modules/context"
@@ -28,6 +27,7 @@ import (
 )
 
 func storageHandler(storageSetting setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler {
+	prefix = strings.Trim(prefix, "/")
 	funcInfo := routing.GetFuncInfo(storageHandler, prefix)
 	return func(next http.Handler) http.Handler {
 		if storageSetting.ServeDirect {
@@ -37,13 +37,15 @@ 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
 				}
 				routing.UpdateFuncInfo(req.Context(), funcInfo)
 
-				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) {
@@ -55,11 +57,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,
 				)
 			})
 		}
@@ -70,22 +73,18 @@ 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
 			}
 			routing.UpdateFuncInfo(req.Context(), funcInfo)
 
-			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) {