Remote Provisioning: Fix empty folder synchronization (#102789)

* Add behavior for hidden

* Rename to IsFilePathSupported

* Modify sync

* Revert renaming

* Fix the tests

* Add more tests

* Maintain empty folders until next full pulling

* Fix wording

* Consider the file as ignored

* Handle folder creation in sync

* Record folder creation / update

* Fix in manual test

* Ensure / slash for folders

* Refactor the tests

* Keep safe path

* Fix some cases

* Remove log lines

pkg/registry/apis/provisioning/jobs/sync/changes.go

@@ -3,6 +3,7 @@ package sync
 import (
 	"fmt"
 	"sort"
+	"strings"
 
 	folders "github.com/grafana/grafana/pkg/apis/folder/v0alpha1"
 	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
@@ -29,19 +30,21 @@ func Changes(source []repository.FileTreeEntry, target *provisioning.ResourceLis
 			}
 			continue
 		}
+
+		// TODO: why do we have to do this here?
+		if item.Group == folders.GROUP && !strings.HasSuffix(item.Path, "/") {
+			item.Path = item.Path + "/"
+		}
+
 		lookup[item.Path] = &item
 	}
 
 	keep := safepath.NewTrie()
 	changes := make([]ResourceFileChange, 0, len(source))
 	for _, file := range source {
-		if !file.Blob {
-			continue // skip folder references?
-		}
-
 		check, ok := lookup[file.Path]
 		if ok {
-			if check.Hash != file.Hash {
+			if check.Hash != file.Hash && check.Resource != folders.RESOURCE {
 				changes = append(changes, ResourceFileChange{
 					Action:   repository.FileActionUpdated,
 					Path:     check.Path,
@@ -53,16 +56,46 @@ func Changes(source []repository.FileTreeEntry, target *provisioning.ResourceLis
 				return nil, fmt.Errorf("failed to add path to keep trie: %w", err)
 			}
 
-			delete(lookup, file.Path)
+			if check.Resource != folders.RESOURCE {
+				delete(lookup, file.Path)
+			}
+
 			continue
 		}
 
-		// TODO: does this work with empty folders?
 		if resources.IsPathSupported(file.Path) == nil {
 			changes = append(changes, ResourceFileChange{
 				Action: repository.FileActionCreated, // or previously ignored/failed
 				Path:   file.Path,
 			})
+
+			if err := keep.Add(file.Path); err != nil {
+				return nil, fmt.Errorf("failed to add path to keep trie: %w", err)
+			}
+
+			continue
+		}
+
+		// Maintain the safe segment for empty folders
+		safeSegment := safepath.SafeSegment(file.Path)
+		if !safepath.IsDir(safeSegment) {
+			safeSegment = safepath.Dir(safeSegment)
+		}
+
+		if safeSegment != "" && resources.IsPathSupported(safeSegment) == nil {
+			if err := keep.Add(safeSegment); err != nil {
+				return nil, fmt.Errorf("failed to add path to keep trie: %w", err)
+			}
+
+			_, ok := lookup[safeSegment]
+			if ok {
+				continue
+			}
+
+			changes = append(changes, ResourceFileChange{
+				Action: repository.FileActionCreated, // or previously ignored/failed
+				Path:   safeSegment,
+			})
 		}
 	}
 

pkg/registry/apis/provisioning/jobs/sync/changes_test.go

@@ -1,7 +1,6 @@
 package sync
 
 import (
-	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -12,17 +11,25 @@ import (
 
 func TestChanges(t *testing.T) {
 	t.Run("start the same", func(t *testing.T) {
-		source, target := getBase(t)
+		source := []repository.FileTreeEntry{
+			{Path: "simplelocal/dashboard.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "simplelocal/dashboard.json", Hash: "xyz"},
+			},
+		}
+
 		changes, err := Changes(source, target)
 		require.NoError(t, err)
 		require.Empty(t, changes)
 	})
 
 	t.Run("create a source file", func(t *testing.T) {
-		source, target := getBase(t)
-		source = append(source, repository.FileTreeEntry{
-			Path: "muta.json", Hash: "xyz", Blob: true,
-		})
+		source := []repository.FileTreeEntry{
+			{Path: "muta.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{}
 
 		changes, err := Changes(source, target)
 		require.NoError(t, err)
@@ -33,9 +40,78 @@ func TestChanges(t *testing.T) {
 		}, changes[0])
 	})
 
+	t.Run("create empty folder structure for folders with unsupported file types", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "one/two/first.md", Hash: "xyz", Blob: true},
+			{Path: "other/second.md", Hash: "xyz", Blob: true},
+		}
+
+		target := &provisioning.ResourceList{}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Len(t, changes, 2)
+
+		require.Equal(t, ResourceFileChange{
+			Action: repository.FileActionCreated,
+			Path:   "one/two/",
+		}, changes[0])
+		require.Equal(t, ResourceFileChange{
+			Action: repository.FileActionCreated,
+			Path:   "other/",
+		}, changes[1])
+	})
+
+	t.Run("keep empty folders when unsupported file types are present", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "one/two/first.md", Hash: "xyz", Blob: true},
+			{Path: "other/second.md", Hash: "xyz", Blob: true},
+		}
+
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "one/two/", Resource: "folders"},
+				{Path: "other/", Resource: "folders"},
+			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Empty(t, changes)
+	})
+	t.Run("keep common path to unsupported file types", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "common/first.md", Hash: "xyz", Blob: true},
+			{Path: "alsocommon/second.md", Hash: "xyz", Blob: true},
+		}
+
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "common/", Resource: "folders"},
+				{Path: "common/not-common/", Resource: "folders", Name: "uncommon-name", Hash: "xyz"},
+				{Path: "alsocommon/", Resource: "folders"},
+			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Len(t, changes, 1)
+		require.Equal(t, ResourceFileChange{
+			Action: repository.FileActionDeleted,
+			Path:   "common/not-common/",
+			Existing: &provisioning.ResourceListItem{
+				Path:     "common/not-common/",
+				Resource: "folders",
+				Name:     "uncommon-name",
+				Hash:     "xyz",
+			},
+		}, changes[0], "the uncommon path should be deleted")
+	})
+
 	t.Run("delete a source file", func(t *testing.T) {
-		source, target := getBase(t)
-		source = []repository.FileTreeEntry{source[0]}
+		source := []repository.FileTreeEntry{}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "adsl62h.yaml", Hash: "xyz", Group: "dashboard.grafana.app", Resource: "dashboards", Name: "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c"},
+			},
+		}
 
 		changes, err := Changes(source, target)
 		require.NoError(t, err)
@@ -48,7 +124,7 @@ func TestChanges(t *testing.T) {
 				Group:    "dashboard.grafana.app",
 				Resource: "dashboards",
 				Name:     "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c",
-				Hash:     "ce5d497c4deadde6831162ce8509e2b2b1776237",
+				Hash:     "xyz",
 			},
 		}, changes[0])
 	})
@@ -77,6 +153,7 @@ func TestChanges(t *testing.T) {
 		}
 		require.Equal(t, []string{
 			"zzz/longest/path/here.json", // not sorted yet
+			"x/y/z/",
 			"x/y/file.json",
 			"short/file.yml",
 			"a.json",
@@ -84,8 +161,20 @@ func TestChanges(t *testing.T) {
 	})
 
 	t.Run("modify a file", func(t *testing.T) {
-		source, target := getBase(t)
-		source[1].Hash = "different"
+		source := []repository.FileTreeEntry{
+			{Path: "adsl62h.yaml", Hash: "modified", Blob: true},
+		}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{
+					Path:     "adsl62h.yaml",
+					Group:    "dashboard.grafana.app",
+					Resource: "dashboards",
+					Name:     "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c",
+					Hash:     "original",
+				},
+			},
+		}
 
 		changes, err := Changes(source, target)
 		require.NoError(t, err)
@@ -98,48 +187,104 @@ func TestChanges(t *testing.T) {
 				Group:    "dashboard.grafana.app",
 				Resource: "dashboards",
 				Name:     "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c",
-				Hash:     "ce5d497c4deadde6831162ce8509e2b2b1776237",
+				Hash:     "original",
 			},
 		}, changes[0])
 	})
-}
 
-func getBase(t *testing.T) (source []repository.FileTreeEntry, target *provisioning.ResourceList) {
-	target = &provisioning.ResourceList{}
-	err := json.Unmarshal([]byte(`{
-		"kind": "ResourceList",
-		"apiVersion": "provisioning.grafana.app/v0alpha1",
-		"metadata": {},
-		"items": [
-			{
-				"path": "",
-				"group": "folder.grafana.app",
-				"resource": "folders",
-				"name": "simplelocal-3794ab9",
-				"hash": ""
+	t.Run("keep folder with hidden files", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "folder/.hidden.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "folder/", Resource: "folders"},
 			},
-			{
-				"path": "ad4lwp2.yaml",
-				"group": "dashboard.grafana.app",
-				"resource": "dashboards",
-				"name": "ad4lwp2-xofjsuo-mr5blr1zwimlfi0ds0pyrrpd",
-				"hash": "ca83d64b9c4a23fed975aacdf47e7de8878b4ae0"
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Empty(t, changes, "folder should be kept when it contains hidden files")
+	})
+
+	t.Run("keep folder with invalid hidden paths", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "folder/.invalid/path.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "folder/", Resource: "folders"},
 			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Empty(t, changes, "folder should be kept when it contains invalid hidden paths")
+	})
+
+	t.Run("keep folder with hidden folders", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "folder/.hidden/valid.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "folder/", Resource: "folders"},
+			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Empty(t, changes, "folder should be kept when it contains hidden folders")
+	})
+
+	t.Run("unhidden path from hidden file", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "folder/.hidden/dashboard.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{
+			Items: []provisioning.ResourceListItem{
+				{Path: "folder/", Resource: "folders"},
+			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Empty(t, changes, "hidden file should not be unhidden")
+	})
+	t.Run("hidden path to file", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "one/two/.hidden/dashboard.json", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{}
+		expected := []ResourceFileChange{
 			{
-				"path": "adsl62h.yaml",
-				"group": "dashboard.grafana.app",
-				"resource": "dashboards",
-				"name": "adsl62h-hrw-f-fvlt2dghp-gufrc4lisksgmq-c",
-				"hash": "ce5d497c4deadde6831162ce8509e2b2b1776237"
-			}
-		]
-	}`), target)
-	require.NoError(t, err)
+				Action: repository.FileActionCreated,
+				Path:   "one/two/",
+			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Equal(t, expected, changes)
+	})
+	t.Run("hidden path to folder", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: "one/two/.hidden/folder/", Hash: "xyz", Blob: true},
+		}
+		target := &provisioning.ResourceList{}
+		expected := []ResourceFileChange{
+			{
+				Action: repository.FileActionCreated,
+				Path:   "one/two/",
+			},
+		}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Equal(t, expected, changes)
+	})
+	t.Run("hidden at the root", func(t *testing.T) {
+		source := []repository.FileTreeEntry{
+			{Path: ".hidden/dashboard.json", Hash: "xyz", Blob: true},
+		}
 
-	source = []repository.FileTreeEntry{
-		{Path: "ad4lwp2.yaml", Hash: "ca83d64b9c4a23fed975aacdf47e7de8878b4ae0", Blob: true},
-		{Path: "adsl62h.yaml", Hash: "ce5d497c4deadde6831162ce8509e2b2b1776237", Blob: true},
-	}
-
-	return // named values!
+		target := &provisioning.ResourceList{}
+		changes, err := Changes(source, target)
+		require.NoError(t, err)
+		require.Empty(t, changes)
+	})
 }

pkg/registry/apis/provisioning/jobs/sync/worker.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"sort"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -263,15 +262,9 @@ func (r *syncJob) applyChanges(ctx context.Context, changes []ResourceFileChange
 		return fmt.Errorf("this should be empty")
 	}
 
-	// Do the deepest paths first (important for delete)
-	sort.Slice(changes, func(i, j int) bool {
-		return safepath.Depth(changes[i].Path) > safepath.Depth(changes[j].Path)
-	})
-
 	r.progress.SetTotal(ctx, len(changes))
 	r.progress.SetMessage(ctx, "replicating changes")
 
-	// Create folder structure first
 	for _, change := range changes {
 		if err := r.progress.TooManyErrors(); err != nil {
 			return err
@@ -304,6 +297,28 @@ func (r *syncJob) applyChanges(ctx context.Context, changes []ResourceFileChange
 			continue
 		}
 
+		// If folder ensure it exists
+		if safepath.IsDir(change.Path) {
+			result := jobs.JobResourceResult{
+				Path:   change.Path,
+				Action: change.Action,
+			}
+
+			folder, err := r.folders.EnsureFolderPathExist(ctx, change.Path)
+			if err != nil {
+				result.Error = fmt.Errorf("create folder: %w", err)
+				r.progress.Record(ctx, result)
+				continue
+			}
+
+			result.Name = folder
+			result.Resource = folders.RESOURCE
+			result.Group = folders.GROUP
+			r.progress.Record(ctx, result)
+
+			continue
+		}
+
 		// Write the resource file
 		r.progress.Record(ctx, r.writeResourceFromFile(ctx, change.Path, "", change.Action))
 	}
@@ -334,6 +349,29 @@ func (r *syncJob) applyVersionedChanges(ctx context.Context, repo repository.Ver
 		}
 
 		if err := resources.IsPathSupported(change.Path); err != nil {
+			// Maintain the safe segment for empty folders
+			safeSegment := safepath.SafeSegment(change.Path)
+			if !safepath.IsDir(safeSegment) {
+				safeSegment = safepath.Dir(safeSegment)
+			}
+
+			if safeSegment != "" && resources.IsPathSupported(safeSegment) == nil {
+				folder, err := r.folders.EnsureFolderPathExist(ctx, safeSegment)
+				if err != nil {
+					return fmt.Errorf("unable to create empty file folder: %w", err)
+				}
+
+				r.progress.Record(ctx, jobs.JobResourceResult{
+					Path:     safeSegment,
+					Action:   repository.FileActionCreated,
+					Resource: folders.RESOURCE,
+					Group:    folders.GROUP,
+					Name:     folder,
+				})
+
+				continue
+			}
+
 			r.progress.Record(ctx, jobs.JobResourceResult{
 				Path:   change.Path,
 				Action: repository.FileActionIgnored,

pkg/registry/apis/provisioning/safepath/safe.go

@@ -54,17 +54,14 @@ func IsSafe(path string) error {
 	}
 
 	parts := Split(path)
-
-	// Check for path traversal attempts first
 	for _, part := range parts {
+		// Check for path traversal attempts first
 		if part == ".." || part == "." {
 			return ErrPathTraversalAttempt
 		}
-	}
 
-	// Check for hidden files/directories in any part of the path
-	for _, part := range parts {
-		if part != "" && part[0] == '.' && part != ".." && part != "." {
+		// Check for hidden files/directories in any part of the path
+		if part == "" || strings.HasPrefix(part, ".") {
 			return ErrHiddenPath
 		}
 	}
@@ -79,3 +76,41 @@ func IsSafe(path string) error {
 
 	return nil
 }
+
+// SafeSegment returns a safe part of the path
+// It ensures the path is free from traversal attempts, hidden files,
+// and other potentially dangerous patterns.
+func SafeSegment(path string) string {
+	if path == "" {
+		return ""
+	}
+
+	parts := Split(path)
+	if len(parts) == 0 {
+		return ""
+	}
+
+	// Build up the path segment by segment, checking safety
+	var safePath string
+	for _, part := range parts {
+		// Check if this segment is safe
+		testPath := Join(safePath, part)
+		if IsSafe(testPath) != nil || part == "" {
+			// If this segment is unsafe, return the path up to but not including this segment
+			// Add trailing slash for directories
+			if safePath != "" {
+				return safePath + "/"
+			}
+			return ""
+		}
+		safePath = testPath
+	}
+
+	// If we made it through all segments, the path is safe
+	// Preserve trailing slash if original path had one
+	if IsDir(path) && safePath != "" {
+		return safePath + "/"
+	}
+
+	return safePath
+}

pkg/registry/apis/provisioning/safepath/safe_test.go

@@ -233,3 +233,71 @@ func TestIsSafe(t *testing.T) {
 		})
 	}
 }
+
+func TestSafeSegment(t *testing.T) {
+	tests := []struct {
+		name     string
+		path     string
+		wantPath string
+	}{
+		{
+			name:     "empty path",
+			path:     "",
+			wantPath: "",
+		},
+		{
+			name:     "simple valid path",
+			path:     "path/to/file.txt",
+			wantPath: "path/to/file.txt",
+		},
+		{
+			name:     "path with valid special characters",
+			path:     "my-path/some_file/test.json",
+			wantPath: "my-path/some_file/test.json",
+		},
+		{
+			name:     "path with trailing slash",
+			path:     "path/to/folder/",
+			wantPath: "path/to/folder/",
+		},
+		{
+			name:     "path with multiple extensions",
+			path:     "path/to/file.min.js",
+			wantPath: "path/to/file.min.js",
+		},
+		{
+			name:     "path with invalid characters",
+			path:     "path/to/file#.txt",
+			wantPath: "path/to/",
+		},
+		{
+			name:     "path with traversal attempt",
+			path:     "path/../file.txt",
+			wantPath: "path/",
+		},
+		{
+			name:     "path with hidden file",
+			path:     "path/to/.hidden",
+			wantPath: "path/to/",
+		},
+		{
+			name:     "path with percent character",
+			path:     "path/to/%20file.txt",
+			wantPath: "path/to/",
+		},
+		{
+			name:     "path with double slashes",
+			path:     "path//to/file.txt",
+			wantPath: "path/",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotPath := SafeSegment(tt.path)
+			if gotPath != tt.wantPath {
+				t.Errorf("SafeSegment() = %v, want %v", gotPath, tt.wantPath)
+			}
+		})
+	}
+}