[v0.14] Validate resolved paths stay within the clone directory (#5278)

* Validate resolved paths stay within the clone directory

The image scan git commit job now checks that paths specified in
the GitRepo spec resolve within the repository clone directory.

* Fix flaky HelmURLRegex migration test

The "treats second run as no-op" spec manually resets HelmRepoURLRegex
after the first migration run using a plain Update on a cached object.
The gitjob reconciler can write to the same GitRepo concurrently,
leaving the cached resourceVersion stale and causing a 409 Conflict.

Wrap the reset in RetryOnConflict with a fresh Get on each attempt,
matching the pattern used by other helpers in this suite.

Author: thardeck

integrationtests/gitjob/controller/helm_url_regex_migration_test.go

@@ -12,6 +12,7 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
 )
 
 const (
@@ -254,9 +255,11 @@ var _ = Describe("HelmURLRegex migration", func() {
 		Expect(getGitRepo("run-twice").Spec.HelmRepoURLRegex).To(Equal(`^https://charts\.example\.com/`))
 
 		// Manually reset to simulate a hypothetical re-entry attempt.
-		gr := getGitRepo("run-twice")
-		gr.Spec.HelmRepoURLRegex = ""
-		Expect(k8sClient.Update(ctx, gr)).To(Succeed())
+		Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error {
+			gr := getGitRepo("run-twice")
+			gr.Spec.HelmRepoURLRegex = ""
+			return k8sClient.Update(ctx, gr)
+		})).To(Succeed())
 
 		// Second run: marker present, migration skipped.
 		Expect(runHelmURLRegexMigration()).To(Succeed())

internal/cmd/controller/imagescan/gitcommit_job.go

@@ -198,8 +198,13 @@ func (j *GitCommitJob) cloneAndReplace(ctx context.Context) {
 		paths = []string{"/"}
 	}
 
-	for _, path := range paths {
-		updatePath := filepath.Join(tmp, path)
+	for _, p := range paths {
+		updatePath := filepath.Clean(filepath.Join(tmp, p))
+		if pathEscapesBase(tmp, p) {
+			err = j.updateErrorStatus(ctx, gitrepo, fmt.Errorf("path %q resolves outside the repository root", p))
+			logger.V(1).Info("Skipping path outside repository root", "path", p, "error", err)
+			return
+		}
 		if err := update.WithSetters(updatePath, updatePath, scans); err != nil {
 			err = j.updateErrorStatus(ctx, gitrepo, err)
 			logger.V(1).Info("Cannot update image tags in repo", "error", err)
@@ -260,6 +265,13 @@ func (j *GitCommitJob) updateErrorStatus(ctx context.Context, gitrepo *fleet.Git
 	return errutil.NewAggregate(merr)
 }
 
+// pathEscapesBase reports whether relPath, when resolved relative to base, points outside base.
+func pathEscapesBase(base, relPath string) bool {
+	resolved := filepath.Clean(filepath.Join(base, relPath))
+	clean := filepath.Clean(base)
+	return !strings.HasPrefix(resolved, clean+string(os.PathSeparator)) && resolved != clean
+}
+
 func shouldSync(gitrepo *fleet.GitRepo) bool {
 	interval := gitrepo.Spec.ImageSyncInterval
 	if interval == nil || interval.Seconds() == 0.0 {

internal/cmd/controller/imagescan/gitcommit_job_test.go

@@ -0,0 +1,31 @@
+package imagescan
+
+import (
+	"testing"
+)
+
+func TestPathResolutionStaysInsideBase(t *testing.T) {
+	const base = "/tmp/repo"
+	tests := []struct {
+		name    string
+		relPath string
+		escapes bool
+	}{
+		{"plain subdirectory", "manifests", false},
+		{"nested subdirectory", "charts/my-chart", false},
+		{"current directory", ".", false},
+		{"root slash", "/", false},
+		{"parent traversal", "../etc", true},
+		{"deep traversal", "../../etc", true},
+		{"nested then traversal", "sub/../../etc", true},
+		{"leading-slash traversal", "/../etc", true},
+		{"absolute path stays inside", "/etc/passwd", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := pathEscapesBase(base, tt.relPath); got != tt.escapes {
+				t.Errorf("pathEscapesBase(%q, %q) = %v, want %v", base, tt.relPath, got, tt.escapes)
+			}
+		})
+	}
+}