Storage: improve path validation, add tests (#50441)

* improve path validation

* add test case

pkg/infra/filestorage/api.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"path/filepath"
 	"regexp"
 	"strings"
 	"time"
@@ -13,13 +14,57 @@ var (
 	ErrRelativePath          = errors.New("path cant be relative")
 	ErrNonCanonicalPath      = errors.New("path must be canonical")
 	ErrPathTooLong           = errors.New("path is too long")
-	ErrPathInvalid           = errors.New("path is invalid")
+	ErrInvalidCharacters     = errors.New("path contains unsupported characters")
 	ErrPathEndsWithDelimiter = errors.New("path can not end with delimiter")
+	ErrPathPartTooLong       = errors.New("path part is too long")
+	ErrEmptyPathPart         = errors.New("path can not have empty parts")
 	Delimiter                = "/"
 	DirectoryMimeType        = "directory"
 	multipleDelimiters       = regexp.MustCompile(`/+`)
+	pathRegex                = regexp.MustCompile(`(^/$)|(^(/[A-Za-z\d!\-_.*'() ]+)+$)`)
+	maxPathLength            = 1024
+	maxPathPartLength        = 256
 )
 
+func ValidatePath(path string) error {
+	if !filepath.IsAbs(path) {
+		return ErrRelativePath
+	}
+
+	if path == Delimiter {
+		return nil
+	}
+
+	if strings.HasSuffix(path, Delimiter) {
+		return ErrPathEndsWithDelimiter
+	}
+
+	if filepath.Clean(path) != path {
+		return ErrNonCanonicalPath
+	}
+
+	if len(path) > maxPathLength {
+		return ErrPathTooLong
+	}
+
+	for _, part := range strings.Split(strings.TrimPrefix(path, Delimiter), Delimiter) {
+		if strings.TrimSpace(part) == "" {
+			return ErrEmptyPathPart
+		}
+
+		if len(part) > maxPathPartLength {
+			return ErrPathPartTooLong
+		}
+	}
+
+	matches := pathRegex.MatchString(path)
+	if !matches {
+		return ErrInvalidCharacters
+	}
+
+	return nil
+}
+
 func Join(parts ...string) string {
 	joinedPath := Delimiter + strings.Join(parts, Delimiter)
 

pkg/infra/filestorage/api_test.go

@@ -1,6 +1,7 @@
 package filestorage
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -34,3 +35,81 @@ func TestFilestorageApi_Join(t *testing.T) {
 		})
 	}
 }
+
+func pathPart(length int) string {
+	sb := strings.Builder{}
+	for i := 0; i < length; i++ {
+		sb.WriteString("a")
+	}
+	return sb.String()
+}
+
+func TestFilestorageApi_ValidatePath(t *testing.T) {
+	var tests = []struct {
+		path          string
+		expectedError error
+	}{
+		{
+			path:          "abc/file.jpg",
+			expectedError: ErrRelativePath,
+		},
+		{
+			path:          "../abc/file.jpg",
+			expectedError: ErrRelativePath,
+		},
+		{
+			path:          "/abc/./file.jpg",
+			expectedError: ErrNonCanonicalPath,
+		},
+		{
+			path:          "/abc/../abc/file.jpg",
+			expectedError: ErrNonCanonicalPath,
+		},
+		{
+			path:          "/abc//folder/file.jpg",
+			expectedError: ErrNonCanonicalPath,
+		},
+		{
+			path:          "/abc/file.jpg/",
+			expectedError: ErrPathEndsWithDelimiter,
+		},
+		{
+			path:          "/abc/folder/ ",
+			expectedError: ErrEmptyPathPart,
+		},
+		{
+			path:          "/abc/ /file.jpg",
+			expectedError: ErrEmptyPathPart,
+		},
+		{
+			path:          "/abc/" + pathPart(260) + "/file.jpg",
+			expectedError: ErrPathPartTooLong,
+		},
+		{
+			path:          "/abc/" + pathPart(1050) + "/file.jpg",
+			expectedError: ErrPathTooLong,
+		},
+		{
+			path:          "/abc/folder🚀/file.jpg",
+			expectedError: ErrInvalidCharacters,
+		},
+		{
+			path:          "/path/with/utf/char/at/the/end.jpg<70>",
+			expectedError: ErrInvalidCharacters,
+		},
+		{
+			path: "/myFile/file.jpg",
+		},
+	}
+	for _, tt := range tests {
+		if tt.expectedError == nil {
+			t.Run("path "+tt.path+" should be valid", func(t *testing.T) {
+				require.Nil(t, ValidatePath(tt.path))
+			})
+		} else {
+			t.Run("path "+tt.path+" should be invalid: "+tt.expectedError.Error(), func(t *testing.T) {
+				require.Equal(t, tt.expectedError, ValidatePath(tt.path))
+			})
+		}
+	}
+}

pkg/infra/filestorage/wrapper.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"mime"
 	"path/filepath"
-	"regexp"
 	"strings"
 
 	"github.com/grafana/grafana/pkg/infra/log"
@@ -14,7 +13,6 @@ import (
 
 var (
 	directoryMarker = ".___gf_dir_marker___"
-	pathRegex       = regexp.MustCompile(`(^/$)|(^(/[A-Za-z0-9!\-_.*'() ]+)+$)`)
 )
 
 type wrapper struct {
@@ -100,37 +98,8 @@ func getName(path string) string {
 	return split[len(split)-1]
 }
 
-func validatePath(path string) error {
-	if !filepath.IsAbs(path) {
-		return ErrRelativePath
-	}
-
-	if path == Delimiter {
-		return nil
-	}
-
-	if filepath.Clean(path) != path {
-		return ErrNonCanonicalPath
-	}
-
-	if strings.HasSuffix(path, Delimiter) {
-		return ErrPathEndsWithDelimiter
-	}
-
-	if len(path) > 1000 {
-		return ErrPathTooLong
-	}
-
-	matches := pathRegex.MatchString(path)
-	if !matches {
-		return ErrPathInvalid
-	}
-
-	return nil
-}
-
 func (b wrapper) validatePath(path string) error {
-	if err := validatePath(path); err != nil {
+	if err := ValidatePath(path); err != nil {
 		b.log.Error("Path failed validation", "path", path, "error", err)
 		return err
 	}