Path Traversal / Path normalization weakness and error-suppression issue

Proof-of-concept (actionable steps): - Background: In Grafana 12.x with provisioning, folder titles are used to derive repository export paths during migrate/export. Prior to this fix, a folder with a title containing unsafe characters (e.g., a title like "» Reports" or a title that collides with another when sanitized) could yield an unsafe or ambiguous path. The exporter could fail to write the folder metadata, and the progress recorder would treat the error as FileActionIgnored, causing the overall export job to report success with no changes. - Reproduction scenarios: 1) Path sanitization collision (two titles map to the same path): - Create two folders at root with titles: • Title: "» Reports" (maps to path segment after sanitization, e.g. "Reports") • Title: "Reports" (maps to the same path segment "Reports") - Attempt a full export/migrate. - Expected before fix: export may silently fail to write one folder's metadata while dashboards land under the shared path, potentially causing metadata/UID mismatches and a silent success. - Expected after fix: writeFolderTree now detects the collision and returns an error like: "folders \"<uid-a>\" (...) and \"<uid-b>\" (...) both export to path \"Reports\"; rename one so their normalized paths differ". The export fails loudly. 2) Path normalization of a dangerous title (space/emoji/punctuation): - Title: "» R&D" and/or "My Group/Folder" (contains slash or emoji). - Before fix: raw title could produce an unsafe path, potentially leading to traversal or hidden paths and an ignored error. - After fix: SanitizeSegment("» R&D", fallback) yields "RD"; SanitizeSegment("My Group/Folder", fallback) yields a single safe segment (e.g., "My GroupFolder" by dropping the '/'); the path is sanitized to a single, safe segment. - Minimal code illustration (Go): package main import ( "fmt" sp "github.com/grafana/grafana/apps/provisioning/pkg/safepath" ) func main() { fmt.Println(sp.SanitizeSegment("» R&D", "uid-123")) // -> RD fmt.Println(sp.IsSafe("RD")) // -> nil (safe) } - Expected outcome after upgrade: - Folders are mapped to safe, single-segment paths and do not collide unexpectedly with siblings. - Genuine failures are surfaced rather than discarded as ignored, aiding detection of misconfigurations or invalid titles.

diff --git a/apps/provisioning/pkg/safepath/safe.go b/apps/provisioning/pkg/safepath/safe.go index b1ca34cc17b55..57af570945b8e 100644 --- a/apps/provisioning/pkg/safepath/safe.go +++ b/apps/provisioning/pkg/safepath/safe.go @@ -77,6 +77,34 @@ func IsSafe(path string) error { return nil } +// SanitizeSegment converts an arbitrary folder title into a single path segment +// that satisfies IsSafe. Characters outside the allowed set ([a-zA-Z0-9 _.-]) +// are dropped, and leading/trailing spaces and dots are trimmed so the result +// is neither a hidden path (leading dot) nor a traversal token ("." / ".."). +// The result never contains a path separator, so a title always maps to exactly +// one directory level. When no usable characters remain (e.g. a title made up +// entirely of unsupported symbols), fallback is returned so the segment is +// always non-empty. +func SanitizeSegment(title, fallback string) string { + var b strings.Builder + b.Grow(len(title)) + for _, r := range title { + switch { + case r >= 'a' && r <= 'z', + r >= 'A' && r <= 'Z', + r >= '0' && r <= '9', + r == ' ', r == '_', r == '.', r == '-': + b.WriteRune(r) + } + } + + cleaned := strings.Trim(b.String(), " .") + if cleaned == "" { + return fallback + } + return cleaned +} + // SafeSegment returns a safe part of the path // It ensures the path is free from traversal attempts, hidden files, // and other potentially dangerous patterns. diff --git a/apps/provisioning/pkg/safepath/safe_test.go b/apps/provisioning/pkg/safepath/safe_test.go index 07831d70dbc11..af91af24c2e2f 100644 --- a/apps/provisioning/pkg/safepath/safe_test.go +++ b/apps/provisioning/pkg/safepath/safe_test.go @@ -23,6 +23,21 @@ func TestIsSafe(t *testing.T) { path: "path/to/my file.json", wantErr: nil, }, + { + name: "directory with a space and trailing slash", + path: "My Group/", + wantErr: nil, + }, + { + name: "nested directories with spaces", + path: "My Group/Sub Group/dashboard.json", + wantErr: nil, + }, + { + name: "nested directories with spaces and trailing slash", + path: "My Group/Sub Group/", + wantErr: nil, + }, { name: "valid path with extension", path: "path/to/file.json", @@ -301,3 +316,71 @@ func TestSafeSegment(t *testing.T) { }) } } + +func TestSanitizeSegment(t *testing.T) { + const fallback = "abc123" + tests := []struct { + name string + title string + want string + }{ + { + name: "plain title unchanged", + title: "Reports", + want: "Reports", + }, + { + name: "title with spaces preserved", + title: "My Group Folder", + want: "My Group Folder", + }, + { + name: "title with allowed punctuation preserved", + title: "my-folder_v1.2", + want: "my-folder_v1.2", + }, + { + name: "invalid character dropped and leading space trimmed", + title: "» Reports", + want: "Reports", + }, + { + name: "slash dropped so title stays a single segment", + title: "Team A/Team B", + want: "Team ATeam B", + }, + { + name: "leading dot trimmed to avoid hidden path", + title: ".hidden", + want: "hidden", + }, + { + name: "traversal token collapses to fallback", + title: "..", + want: fallback, + }, + { + name: "unsupported-only title falls back to uid", + title: "🎉🚀", + want: fallback, + }, + { + name: "empty title falls back to uid", + title: "", + want: fallback, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeSegment(tt.title, fallback) + if got != tt.want { + t.Errorf("SanitizeSegment(%q) = %q, want %q", tt.title, got, tt.want) + } + // A sanitized segment must always be a safe path on its own. + if err := IsSafe(got); err != nil { + t.Errorf("SanitizeSegment(%q) produced unsafe path %q: %v", tt.title, got, err) + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/files_test.go b/pkg/registry/apis/provisioning/files_test.go index 4c3f99bd6a819..0c5b1e7f173f5 100644 --- a/pkg/registry/apis/provisioning/files_test.go +++ b/pkg/registry/apis/provisioning/files_test.go @@ -459,6 +459,27 @@ func TestParseRequestOptionsPathValidation(t *testing.T) { } } +// TestParseRequestOptionsPathWithSpace verifies the /files endpoint accepts a +// path whose folder segment contains a space (e.g. a folder titled "My Group"). +// The apiserver decodes %20 into r.URL.Path before we extract the file path, and +// IsSafe explicitly allows spaces, so the path is served as-is. +func TestParseRequestOptionsPathWithSpace(t *testing.T) { + mockRepo := repository.NewMockRepository(t) + mockRepo.On("Config").Return(&provisioningapi.Repository{ + Spec: provisioningapi.RepositorySpec{ + Title: "test-repo", + }, + }).Maybe() + + connector := &filesConnector{} + // The space is percent-encoded on the wire; the server decodes it into URL.Path. + r := httptest.NewRequest(http.MethodGet, "/test-repo/files/My%20Group/dashboard.json", nil) + + opts, err := connector.parseRequestOptions(r, "test-repo", mockRepo) + require.NoError(t, err) + require.Equal(t, "My Group/dashboard.json", opts.Path) +} + func TestParseRequestOptionsRefValidation(t *testing.T) { tests := []struct { name string diff --git a/pkg/registry/apis/provisioning/jobs/export/folders.go b/pkg/registry/apis/provisioning/jobs/export/folders.go index dc4b8142229e1..06dc224e13822 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders.go @@ -57,6 +57,10 @@ func ExportFolders(ctx context.Context, repoName string, options provisioning.Ex // selective export, which only assembles the folders that the requested // resources actually need. func writeFolderTree(ctx context.Context, options provisioning.ExportJobOptions, repositoryResources resources.RepositoryResources, tree resources.FolderTree, progress jobs.JobProgressRecorder) error { + if err := checkFolderPathCollisions(ctx, tree); err != nil { + return err + } + return repositoryResources.EnsureFolderTreeExists(ctx, tree, resources.EnsureFolderTreeExistsOptions{ Ref: options.Branch, Path: options.Path, @@ -64,11 +68,14 @@ func writeFolderTree(ctx context.Context, options provisioning.ExportJobOptions, OnFolder: func(folder resources.Folder, created bool, err error) error { resultBuilder := jobs.NewFolderResult(folder.Path).WithName(folder.ID).WithAction(repository.FileActionCreated) - if err != nil { + // A folder that failed to write must keep a non-ignored action: the + // recorder discards errors on FileActionIgnored results, so labelling + // a failure as ignored would let a broken export (e.g. an invalid + // folder path) drop the error and report the job as a success. + switch { + case err != nil: resultBuilder.WithError(fmt.Errorf("creating folder %s at path %s: %w", folder.ID, folder.Path, err)) - } - - if !created { + case !created: resultBuilder.WithAction(repository.FileActionIgnored) } progress.Record(ctx, resultBuilder.Build()) @@ -78,6 +85,27 @@ func writeFolderTree(ctx context.Context, options provisioning.ExportJobOptions, }) } +// checkFolderPathCollisions fails the export when two distinct folders normalize +// to the same repository path. SanitizeSegment can map different titles under the +// same parent onto one segment (for example "» Reports" and "Reports", or "A&B" +// and "AB"). Writing both would let the second folder be treated as already +// present — its _folder.json skipped or overwriting the first — while dashboards +// still land under the shared path, so one folder's on-disk metadata would +// silently represent the wrong UID/title. Refuse loudly instead, naming both +// folders so the user can rename one. +func checkFolderPathCollisions(ctx context.Context, tree resources.FolderTree) error { + seen := make(map[string]resources.Folder) + return tree.Walk(ctx, func(_ context.Context, folder resources.Folder, _ string) error { + if prev, dup := seen[folder.Path]; dup && prev.ID != folder.ID { + return fmt.Errorf( + "folders %q (%s) and %q (%s) both export to path %q; rename one so their normalized paths differ", + prev.Title, prev.ID, folder.Title, folder.ID, folder.Path) + } + seen[folder.Path] = folder + return nil + }) +} + // exportFolderAncestry writes only the folders needed to place the selectively // exported resources, instead of dragging in the entire instance folder tree. // For every folder UID in folderUIDs it walks up to the root via the folder diff --git a/pkg/registry/apis/provisioning/jobs/export/folders_test.go b/pkg/registry/apis/provisioning/jobs/export/folders_test.go index 14d5532b64403..858bb54aa3fdf 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders_test.go @@ -193,7 +193,7 @@ func TestExportFolders(t *testing.T) { progress.On("SetMessage", mock.Anything, "read folder tree from API server").Return() progress.On("SetMessage", mock.Anything, "write folders to repository").Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name() == "folder-1-uid" && result.Action() == repository.FileActionIgnored && result.Error() != nil && result.Error().Error() == "creating folder folder-1-uid at path grafana/folder-1: didn't work" + return result.Name() == "folder-1-uid" && result.Action() == repository.FileActionCreated && result.Error() != nil && result.Error().Error() == "creating folder folder-1-uid at path grafana/folder-1: didn't work" })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { return result.Name() == "folder-2-uid" && result.Action() == repository.FileActionCreated @@ -586,3 +586,37 @@ func (m *mockDynamicInterface) Get(ctx context.Context, name string, opts metav1 } return &m.items[0], nil } + +// TestWriteFolderTree_PathCollisionFails verifies that two distinct folders whose +// titles normalize to the same repository path fail the export loudly, instead of +// silently letting one folder's _folder.json represent the wrong UID/title. +func TestWriteFolderTree_PathCollisionFails(t *testing.T) { + tree := resources.NewEmptyFolderTree() + // Two distinct root folders whose titles both sanitize to "Reports". + tree.Add(resources.Folder{ID: "uid-a", Title: "» Reports"}, "") + tree.Add(resources.Folder{ID: "uid-b", Title: "Reports"}, "") + + // Leaving both mocks without expectations asserts EnsureFolderTreeExists (and + // therefore any per-folder Record) is never reached once a collision is found. + repoResources := resources.NewMockRepositoryResources(t) + progress := jobs.NewMockJobProgressRecorder(t) + + err := writeFolderTree(context.Background(), v0alpha1.ExportJobOptions{}, repoResources, tree, progress) + require.Error(t, err) + require.Contains(t, err.Error(), "both export to path") +} + +// TestWriteFolderTree_NoCollisionProceeds verifies that folders with distinct +// normalized paths pass the collision check and reach EnsureFolderTreeExists. +func TestWriteFolderTree_NoCollisionProceeds(t *testing.T) { + tree := resources.NewEmptyFolderTree() + tree.Add(resources.Folder{ID: "uid-a", Title: "Reports"}, "") + tree.Add(resources.Folder{ID: "uid-b", Title: "Metrics"}, "") + + repoResources := resources.NewMockRepositoryResources(t) + repoResources.On("EnsureFolderTreeExists", mock.Anything, mock.Anything, mock.Anything).Return(nil) + progress := jobs.NewMockJobProgressRecorder(t) + + err := writeFolderTree(context.Background(), v0alpha1.ExportJobOptions{}, repoResources, tree, progress) + require.NoError(t, err) +} diff --git a/pkg/registry/apis/provisioning/jobs/export/resources.go b/pkg/registry/apis/provisioning/jobs/export/resources.go index bc61860141ddd..db0243067ef27 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources.go @@ -224,9 +224,11 @@ func exportItem(ctx context.Context, meta, err := utils.MetaAccessor(item) if err != nil { metaError := fmt.Errorf("extracting meta accessor for resource %s: %w", name, err) - resultBuilder.WithAction(repository.FileActionIgnored).WithError(metaError) + // Keep the default (created) action so the failure is counted rather + // than silently discarded as an ignored result. + resultBuilder.WithError(metaError) progress.Record(ctx, resultBuilder.Build()) - return nil + return progress.TooManyErrors() } manager, managed := meta.GetManagerProperties() @@ -264,8 +266,10 @@ func exportItem(ctx context.Context, if errors.Is(err, resources.ErrAlreadyInRepository) { resultBuilder.WithAction(repository.FileActionIgnored) } else if err != nil { - resultBuilder.WithAction(repository.FileActionIgnored). - WithError(fmt.Errorf("writing resource file for %s: %w", name, err)) + // Keep the default (created) action so the error is counted: recording a + // genuine write failure as FileActionIgnored would let the recorder + // discard the error and pass the export off as successful. + resultBuilder.WithError(fmt.Errorf("writing resource file for %s: %w", name, err)) } progress.Record(ctx, resultBuilder.Build()) diff --git a/pkg/registry/apis/provisioning/jobs/export/resources_test.go b/pkg/registry/apis/provisioning/jobs/export/resources_test.go index 93d42145b0321..ec1fa6d505126 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources_test.go @@ -142,7 +142,7 @@ func TestExportResources_Dashboards_WithErrors(t *testing.T) { progress.On("SetMessage", mock.Anything, "start resource export").Return() progress.On("SetMessage", mock.Anything, "export dashboards").Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name() == "dashboard-1" && result.Action() == repository.FileActionIgnored && result.Error() != nil && result.Error().Error() == "writing resource file for dashboard-1: failed to export dashboard" + return result.Name() == "dashboard-1" && result.Action() == repository.FileActionCreated && result.Error() != nil && result.Error().Error() == "writing resource file for dashboard-1: failed to export dashboard" })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { return result.Name() == "dashboard-2" && result.Action() == repository.FileActionCreated @@ -180,7 +180,7 @@ func TestExportResources_Dashboards_TooManyErrors(t *testing.T) { progress.On("SetMessage", mock.Anything, "start resource export").Return() progress.On("SetMessage", mock.Anything, "export dashboards").Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool ... [truncated]