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 {