From 3dcb00ca730dc207251f5b3cc9a8d56659e955da Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 19 May 2026 12:19:16 +0200 Subject: [PATCH 1/2] Resolve bucket object paths with SecureJoin Bucket object keys are external input and may contain arbitrary characters. Joining them with the reconciler's working directory through `filepath.Join` applies `filepath.Clean`, which collapses parent-directory segments and can yield a destination outside the working directory. `securejoin.SecureJoin` resolves the key while keeping the result within the working directory, matching the pattern already used elsewhere in the controllers for similar joins (e.g. GitRepository include paths). Assisted-by: claude-code/opus-4.7 Signed-off-by: Hidde Beydals (cherry picked from commit 6d2d86d5010a6e39cf32e495eee6df26cd10065e) --- internal/controller/bucket_controller.go | 6 ++- .../bucket_controller_fetch_test.go | 40 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index 7c371ae2a..c5bb806e0 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -27,6 +27,7 @@ import ( "strings" "time" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/opencontainers/go-digest" "golang.org/x/sync/errgroup" "golang.org/x/sync/semaphore" @@ -752,7 +753,10 @@ func fetchIndexFiles(ctx context.Context, provider BucketProvider, obj *sourcev1 } group.Go(func() error { defer sem.Release(1) - localPath := filepath.Join(tempDir, k) + localPath, err := securejoin.SecureJoin(tempDir, k) + if err != nil { + return fmt.Errorf("failed to resolve path for '%s' object: %w", k, err) + } etag, err := provider.FGetObject(ctxTimeout, obj.Spec.BucketName, k, localPath) if err != nil { if provider.ObjectIsNotFound(err) { diff --git a/internal/controller/bucket_controller_fetch_test.go b/internal/controller/bucket_controller_fetch_test.go index 707d645f3..a0f26a1c9 100644 --- a/internal/controller/bucket_controller_fetch_test.go +++ b/internal/controller/bucket_controller_fetch_test.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "time" @@ -289,4 +290,43 @@ func Test_fetchFiles(t *testing.T) { t.Fatal(err) } }) + + t.Run("resolves object keys relative to the working directory", func(t *testing.T) { + g := NewWithT(t) + + // Place the working directory inside a parent so we can observe + // where files end up on disk. + parent := t.TempDir() + tmp := filepath.Join(parent, "work") + g.Expect(os.Mkdir(tmp, 0o700)).To(Succeed()) + + client := mockBucketClient{bucketName: bucketName} + client.addObject("../sibling.yaml", mockBucketObject{etag: "etag1", data: "sibling"}) + + index := client.objectsToDigestIndex() + + err := fetchIndexFiles(context.TODO(), client, bucket.DeepCopy(), index, tmp) + g.Expect(err).ToNot(HaveOccurred()) + + // All fetched files must live under the working directory. + var outside []string + walkErr := filepath.Walk(parent, func(p string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + return nil + } + rel, relErr := filepath.Rel(tmp, p) + if relErr != nil { + return relErr + } + if strings.HasPrefix(rel, "..") { + outside = append(outside, p) + } + return nil + }) + g.Expect(walkErr).ToNot(HaveOccurred()) + g.Expect(outside).To(BeEmpty(), "files placed outside the working directory: %v", outside) + }) } From 153b7ab0ad9eb6e8b1d3aabe5514f4927d5e45d8 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 19 May 2026 12:29:20 +0200 Subject: [PATCH 2/2] Resolve sparse checkout paths with SecureJoin When validating that the paths listed in `spec.sparseCheckout` exist in the cloned working tree, resolve each entry with `securejoin.SecureJoin` instead of `filepath.Join`. `filepath.Join` collapses parent-directory segments via `filepath.Clean`, so a configured path like `../foo` would have been checked against a location outside the working tree, masking a missing entry behind an unrelated filesystem stat. SecureJoin keeps the resolved path inside the working tree, matching the pattern already used for include paths elsewhere in the controller. Assisted-by: claude-code/opus-4.7 Signed-off-by: Hidde Beydals (cherry picked from commit f5fe0341758b5207a551ac38ad6dcb9a98532aee) --- .../controller/gitrepository_controller.go | 5 +- .../gitrepository_controller_test.go | 66 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index e553fa120..5098a5288 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -1317,7 +1317,10 @@ func gitContentConfigChanged(obj *sourcev1.GitRepository, includes *artifactSet) func (r *GitRepositoryReconciler) validateSparseCheckoutPaths(obj *sourcev1.GitRepository, dir string) error { if obj.Spec.SparseCheckout != nil { for _, path := range obj.Spec.SparseCheckout { - fullPath := filepath.Join(dir, path) + fullPath, err := securejoin.SecureJoin(dir, path) + if err != nil { + return fmt.Errorf("sparse checkout dir '%s' cannot be resolved: %w", path, err) + } if _, err := os.Lstat(fullPath); err != nil { return fmt.Errorf("sparse checkout dir '%s' does not exist in repository: %w", path, err) } diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 46835e5d7..095f17afa 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -3084,6 +3084,72 @@ func resetChmod(path string, dirMode os.FileMode, fileMode os.FileMode) error { return nil } +func TestGitRepositoryReconciler_validateSparseCheckoutPaths(t *testing.T) { + tests := []struct { + name string + paths []string + beforeFunc func(t *WithT, dir string) + wantErr bool + errContains string + }{ + { + name: "no paths configured", + }, + { + name: "configured path exists in the working directory", + paths: []string{"a/b"}, + beforeFunc: func(t *WithT, dir string) { + t.Expect(os.MkdirAll(filepath.Join(dir, "a", "b"), 0o700)).To(Succeed()) + }, + }, + { + name: "configured path is missing from the working directory", + paths: []string{"missing"}, + wantErr: true, + errContains: "missing", + }, + { + name: "configured path is resolved relative to the working directory", + paths: []string{"../sibling"}, + beforeFunc: func(t *WithT, dir string) { + // Create a sibling of the working directory; the path must + // not resolve to it. + t.Expect(os.MkdirAll(filepath.Join(filepath.Dir(dir), "sibling"), 0o700)).To(Succeed()) + }, + wantErr: true, + errContains: "../sibling", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + parent := t.TempDir() + dir := filepath.Join(parent, "work") + g.Expect(os.Mkdir(dir, 0o700)).To(Succeed()) + if tt.beforeFunc != nil { + tt.beforeFunc(g, dir) + } + + obj := &sourcev1.GitRepository{ + Spec: sourcev1.GitRepositorySpec{SparseCheckout: tt.paths}, + } + + r := &GitRepositoryReconciler{} + err := r.validateSparseCheckoutPaths(obj, dir) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + if tt.errContains != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.errContains)) + } + return + } + g.Expect(err).NotTo(HaveOccurred()) + }) + } +} + func TestGitRepositoryIncludeEqual(t *testing.T) { tests := []struct { name string