From cc0c86b286df19122e66eb42543fc362dac7d3fb Mon Sep 17 00:00:00 2001 From: Mariell Hoversholm Date: Tue, 25 Mar 2025 11:09:24 +0100 Subject: [PATCH] Provisioning: Fix migration: Go-git should pretend the Path is the repo root (#102755) * fix: go git migration should pretend the path is the repo root The migration was broken due to its having the path already in the repository, then adding it once more. This now pretends the path is the repository root as intended. * chore: remove test log * fix: reduce permissions of directory * chore: make update-workspace --- .../provisioning/repository/go-git/wrapper.go | 16 +++--- .../repository/go-git/wrapper_test.go | 54 +++++++++++++++++++ .../apis/provisioning/safepath/path.go | 13 +++-- .../apis/provisioning/safepath/path_test.go | 3 ++ 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/pkg/registry/apis/provisioning/repository/go-git/wrapper.go b/pkg/registry/apis/provisioning/repository/go-git/wrapper.go index 4447aa2174a..580bf61f622 100644 --- a/pkg/registry/apis/provisioning/repository/go-git/wrapper.go +++ b/pkg/registry/apis/provisioning/repository/go-git/wrapper.go @@ -249,17 +249,21 @@ func (g *GoGitRepo) ReadTree(ctx context.Context, ref string) ([]repository.File if g.config.Spec.GitHub.Path != "" { treePath = g.config.Spec.GitHub.Path } - - // TODO: do we really need this? - if !strings.HasPrefix(treePath, "/") { - treePath = "/" + treePath - } + treePath = safepath.Clean(treePath) entries := make([]repository.FileTreeEntry, 0, 100) err := util.Walk(g.tree.Filesystem, treePath, func(path string, info fs.FileInfo, err error) error { - if err != nil || path == "/" { + // We already have an error, just pass it onwards. + if err != nil || + // This is the root of the repository (or should pretend to be) + safepath.Clean(path) == "" || path == treePath || + // This is the Git data + (treePath == "" && strings.HasPrefix(path, ".git/")) { return err } + if treePath != "" { + path = strings.TrimPrefix(path, treePath) + } entry := repository.FileTreeEntry{ Path: strings.TrimLeft(path, "/"), Size: info.Size(), diff --git a/pkg/registry/apis/provisioning/repository/go-git/wrapper_test.go b/pkg/registry/apis/provisioning/repository/go-git/wrapper_test.go index 4ce86d18859..beaac3d4da6 100644 --- a/pkg/registry/apis/provisioning/repository/go-git/wrapper_test.go +++ b/pkg/registry/apis/provisioning/repository/go-git/wrapper_test.go @@ -5,9 +5,11 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "testing" "time" + "github.com/go-git/go-git/v5" "github.com/stretchr/testify/require" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -92,3 +94,55 @@ func TestGoGitWrapper(t *testing.T) { }, os.Stdout) require.NoError(t, err) } + +func TestReadTree(t *testing.T) { + dir := t.TempDir() + gitRepo, err := git.PlainInit(dir, false) + require.NoError(t, err, "failed to init a new git repository") + worktree, err := gitRepo.Worktree() + require.NoError(t, err, "failed to get worktree") + + repo := &GoGitRepo{ + config: &v0alpha1.Repository{ + ObjectMeta: v1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Spec: v0alpha1.RepositorySpec{ + Title: "test", + Workflows: []v0alpha1.Workflow{v0alpha1.WriteWorkflow}, + Type: v0alpha1.GitHubRepositoryType, + GitHub: &v0alpha1.GitHubRepositoryConfig{ + URL: "https://github.com/grafana/__unit-test", + Path: "grafana/", + Branch: "main", + }, + }, + Status: v0alpha1.RepositoryStatus{}, + }, + decryptedPassword: "password", + + repo: gitRepo, + tree: worktree, + dir: dir, + } + + err = os.WriteFile(filepath.Join(dir, "test.txt"), []byte("test"), 0644) + require.NoError(t, err, "failed to write test file") + + err = os.Mkdir(filepath.Join(dir, "grafana"), 0750) + require.NoError(t, err, "failed to mkdir grafana") + + err = os.WriteFile(filepath.Join(dir, "grafana", "test2.txt"), []byte("test"), 0644) + require.NoError(t, err, "failed to write grafana/test2 file") + + ctx := context.Background() + entries, err := repo.ReadTree(ctx, "HEAD") + require.NoError(t, err, "failed to read tree") + + // Here is the meat of why this test exists: the ReadTree call should only read the config.Spec.GitHub.Path files. + // All prefixes are removed (i.e. a file is just its name, not ${Path}/${Name}). + // And it does not include the directory in the listing, as it pretends to be the root. + require.Len(t, entries, 1, "entries from ReadTree") + require.Equal(t, entries[0].Path, "test2.txt", "entry path") +} diff --git a/pkg/registry/apis/provisioning/safepath/path.go b/pkg/registry/apis/provisioning/safepath/path.go index d314fa80cdf..e1fa2fa464c 100644 --- a/pkg/registry/apis/provisioning/safepath/path.go +++ b/pkg/registry/apis/provisioning/safepath/path.go @@ -13,15 +13,22 @@ import ( var osSeparator = os.PathSeparator // Performs a [path.Clean] on the path, as well as replacing its OS separators. +// // This replaces the OS separator with a slash. // All OSes we target (Linux, macOS, and Windows) support forward-slashes in path traversals, as such it's simpler to use the same character everywhere. // BSDs do as well (even though they're not a target as of writing). +// +// The output of a root path (i.e. absolute root or relative current dir) is always "" (empty string). func Clean(p string) string { - if osSeparator == '/' { // perf: nothing to do! - return path.Clean(p) + if osSeparator != '/' { + p = strings.ReplaceAll(p, string(osSeparator), "/") } - return path.Clean(strings.ReplaceAll(p, string(osSeparator), "/")) + cleaned := path.Clean(p) + if cleaned == "." || cleaned == "/" { + return "" + } + return cleaned } // Join is like path.Join but preserves trailing slashes from the last element diff --git a/pkg/registry/apis/provisioning/safepath/path_test.go b/pkg/registry/apis/provisioning/safepath/path_test.go index d42bde6296e..23375e4226f 100644 --- a/pkg/registry/apis/provisioning/safepath/path_test.go +++ b/pkg/registry/apis/provisioning/safepath/path_test.go @@ -55,6 +55,9 @@ func TestPathClean(t *testing.T) { {"Path traversal with multiple slashes", "/test////abc/..//def", "/test/def"}, {"Path traversal with current directory", "/test/./abc/../def/./ghi", "/test/def/ghi"}, {"Empty path segments with traversal", "//test//abc//..//def", "/test/def"}, + {"Root path returns empty string", "/", ""}, + {"Current directory returns empty string", ".", ""}, + {"Path traversal to root returns empty string", "/test/..", ""}, } for _, tc := range testCases {