fix: Storage prefix validation (#4044) (#4047)

Our validation has been overly strict, since inception of the project.
Although we failed to call the validate alltogether. This was fixed in

This PR loosens the criteria accordingly

Fixes #3968

(cherry picked from commit ed11e4a)

Co-authored-by: Christian Simon <[email protected]>

Author: github-actions[bot]

pkg/objstore/client/config.go

@@ -4,7 +4,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"regexp"
 	"strings"
 
 	"github.com/go-kit/log"
@@ -40,18 +39,25 @@ const (
 
 	// Filesystem is the value for the filesystem storage backend.
 	Filesystem = "filesystem"
-
-	// validPrefixCharactersRegex allows only alphanumeric characters to prevent subtle bugs and simplify validation
-	validPrefixCharactersRegex = `^[\da-zA-Z]+$`
 )
 
 var (
 	SupportedBackends = []string{S3, GCS, Azure, Swift, Filesystem, COS}
 
-	ErrUnsupportedStorageBackend        = errors.New("unsupported storage backend")
-	ErrInvalidCharactersInStoragePrefix = errors.New("storage prefix contains invalid characters, it may only contain digits and English alphabet letters")
+	ErrUnsupportedStorageBackend      = errors.New("unsupported storage backend")
+	ErrStoragePrefixStartsWithSlash   = errors.New("storage prefix starts with a slash")
+	ErrStoragePrefixEmptyPathSegment  = errors.New("storage prefix contains an empty path segment")
+	ErrStoragePrefixInvalidCharacters = errors.New("storage prefix contains invalid characters: only alphanumeric, hyphen, underscore, dot, and no segement should be . or ..")
 )
 
+type ErrInvalidCharactersInStoragePrefix struct {
+	Prefix string
+}
+
+func (e *ErrInvalidCharactersInStoragePrefix) Error() string {
+	return fmt.Sprintf("storage prefix '%s' contains invalid characters", e.Prefix)
+}
+
 type StorageBackendConfig struct {
 	Backend string `yaml:"backend"`
 
@@ -132,13 +138,49 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet, logge
 	cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "./data-shared", f, logger)
 }
 
-func (cfg *Config) Validate() error {
-	if cfg.StoragePrefix != "" {
-		acceptablePrefixCharacters := regexp.MustCompile(validPrefixCharactersRegex)
-		if !acceptablePrefixCharacters.MatchString(cfg.StoragePrefix) {
-			return ErrInvalidCharactersInStoragePrefix
+// alphanumeric, hyphen, underscore, dot, and must not be . or ..
+func validStoragePrefixPart(part string) bool {
+	if part == "." || part == ".." {
+		return false
+	}
+	for i, b := range part {
+		if !((b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '_' || b == '-' || b == '.' || (b >= '0' && b <= '9' && i > 0)) {
+			return false
+		}
+	}
+	return true
+}
+
+func validStoragePrefix(prefix string) error {
+	// without a prefix exit quickly
+	if prefix == "" {
+		return nil
+	}
+
+	parts := strings.Split(prefix, "/")
+
+	for idx, part := range parts {
+		if part == "" {
+			if idx == 0 {
+				return ErrStoragePrefixStartsWithSlash
+			}
+			if idx != len(parts)-1 {
+				return ErrStoragePrefixEmptyPathSegment
+			}
+			// slash at the end is fine
+		}
+		if !validStoragePrefixPart(part) {
+			return ErrStoragePrefixInvalidCharacters
 		}
 	}
 
+	return nil
+}
+
+func (cfg *Config) Validate() error {
+	if err := validStoragePrefix(cfg.StoragePrefix); err != nil {
+		return err
+	}
+
 	return cfg.StorageBackendConfig.Validate()
 }

pkg/objstore/client/factory_test.go

@@ -138,28 +138,51 @@ func TestClient_ConfigValidation(t *testing.T) {
 		expectedError error
 	}{
 		{
-			name: "valid storage_prefix",
-			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "helloworld"},
+			name: "storage_prefix/valid",
+			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "helloWORLD123"},
 		},
 		{
-			name:          "storage_prefix non-alphanumeric characters",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello-world!"},
-			expectedError: ErrInvalidCharactersInStoragePrefix,
+			name: "storage_prefix/valid-subdir",
+			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello/world/env"},
 		},
 		{
-			name:          "storage_prefix suffixed with a slash (non-alphanumeric)",
-			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "helloworld/"},
-			expectedError: ErrInvalidCharactersInStoragePrefix,
+			name: "storage_prefix/valid-subdir-trailing-slash",
+			cfg:  Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello/world/env/"},
 		},
 		{
-			name:          "storage_prefix that has some character strings that have a meaning in unix paths (..)",
+			name:          "storage_prefix/invalid-directory-up",
 			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: ".."},
-			expectedError: ErrInvalidCharactersInStoragePrefix,
+			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
-			name:          "storage_prefix that has some character strings that have a meaning in unix paths (.)",
+			name:          "storage_prefix/invalid-directory",
 			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "."},
-			expectedError: ErrInvalidCharactersInStoragePrefix,
+			expectedError: ErrStoragePrefixInvalidCharacters,
+		},
+		{
+			name:          "storage_prefix/invalid-absolute-path",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "/hello/world"},
+			expectedError: ErrStoragePrefixStartsWithSlash,
+		},
+		{
+			name:          "storage_prefix/invalid-..-in-a-path-segement",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello/../test"},
+			expectedError: ErrStoragePrefixInvalidCharacters,
+		},
+		{
+			name:          "storage_prefix/invalid-empty-path-segement",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello//test"},
+			expectedError: ErrStoragePrefixEmptyPathSegment,
+		},
+		{
+			name:          "storage_prefix/invalid-emoji",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "👋"},
+			expectedError: ErrStoragePrefixInvalidCharacters,
+		},
+		{
+			name:          "storage_prefix/invalid-emoji",
+			cfg:           Config{StorageBackendConfig: StorageBackendConfig{Backend: Filesystem}, StoragePrefix: "hello!world"},
+			expectedError: ErrStoragePrefixInvalidCharacters,
 		},
 		{
 			name:          "unsupported backend",
@@ -172,7 +195,11 @@ func TestClient_ConfigValidation(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			actualErr := tc.cfg.Validate()
-			assert.ErrorIs(t, actualErr, tc.expectedError)
+			if tc.expectedError != nil {
+				assert.Equal(t, actualErr, tc.expectedError)
+			} else {
+				assert.NoError(t, actualErr)
+			}
 		})
 	}
 }