Merge branch 'smh-clean-storage-paths' into 'master'

Clean configured storage paths

See merge request gitlab-org/gitaly!2223

3 files changed, 32 insertions, 9 deletions

diff --git a/changelogs/unreleased/smh-clean-storage-paths.yml b/changelogs/unreleased/smh-clean-storage-paths.yml
new file mode 100644
index 000000000..051a18326
--- /dev/null
+++ b/changelogs/unreleased/smh-clean-storage-paths.yml
@@ -0,0 +1,5 @@
+---
+title: Clean configured storage paths
+merge_request: 2223
+author:
+type: fixed
diff --git a/internal/config/config.go b/internal/config/config.go
index 306ec3cbf..2b9a55b75 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -129,6 +129,10 @@ func Load(file io.Reader) error {
 
 	Config.setDefaults()
 
+	for i := range Config.Storages {
+		Config.Storages[i].Path = filepath.Clean(Config.Storages[i].Path)
+	}
+
 	return nil
 }
 
@@ -287,22 +291,19 @@ func validateStorages() error {
 			return fmt.Errorf("storage %+v path must be a dir", storage)
 		}
 
-		stPath := filepath.Clean(storage.Path)
-		for j := 0; j < i; j++ {
-			other := Config.Storages[j]
+		for _, other := range Config.Storages[:i] {
 			if other.Name == storage.Name {
 				return fmt.Errorf("storage %q is defined more than once", storage.Name)
 			}
 
-			otherPath := filepath.Clean(other.Path)
-			if stPath == otherPath {
+			if storage.Path == other.Path {
 				// This is weird but we allow it for legacy gitlab.com reasons.
 				continue
 			}
 
-			if strings.HasPrefix(stPath, otherPath) || strings.HasPrefix(otherPath, stPath) {
+			if strings.HasPrefix(storage.Path, other.Path) || strings.HasPrefix(other.Path, storage.Path) {
 				// If storages have the same sub directory, that is allowed
-				if filepath.Dir(stPath) == filepath.Dir(otherPath) {
+				if filepath.Dir(storage.Path) == filepath.Dir(other.Path) {
 					continue
 				}
 				return fmt.Errorf("storage paths may not nest: %q and %q", storage.Name, other.Name)
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index f57ef4972..8e31520bc 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -9,6 +9,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -52,7 +53,7 @@ func TestLoadEmptyConfig(t *testing.T) {
 func TestLoadStorage(t *testing.T) {
 	tmpFile := configFileReader(`[[storage]]
 name = "default"
-path = "/tmp"`)
+path = "/tmp/"`)
 
 	err := Load(tmpFile)
 	assert.NoError(t, err)
@@ -69,6 +70,22 @@ path = "/tmp"`)
 	}
 }
 
+func TestUncleanStoragePaths(t *testing.T) {
+	require.NoError(t, Load(strings.NewReader(`[[storage]]
+name="unclean-path-1"
+path="/tmp/repos1//"
+
+[[storage]]
+name="unclean-path-2"
+path="/tmp/repos2/subfolder/.."
+`)))
+
+	require.Equal(t, []Storage{
+		{Name: "unclean-path-1", Path: "/tmp/repos1"},
+		{Name: "unclean-path-2", Path: "/tmp/repos2"},
+	}, Config.Storages)
+}
+
 func TestLoadMultiStorage(t *testing.T) {
 	tmpFile := configFileReader(`[[storage]]
 name="default"
@@ -76,7 +93,7 @@ path="/tmp/repos1"
 
 [[storage]]
 name="other"
-path="/tmp/repos2"`)
+path="/tmp/repos2/"`)
 
 	err := Load(tmpFile)
 	assert.NoError(t, err)