[release/v1.8.x] Improve path handling in source reconcilers

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) {

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)
+	})
 }

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)
 			}

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