Fix HelmChart valuesFile chart path restriction

config/testdata/helmchart-valuesfile/helmchart_gitrepository.yaml

@@ -8,4 +8,4 @@ spec:
     kind: GitRepository
     name: podinfo
   chart: charts/podinfo
-  valuesFile: values-prod.yaml
+  valuesFile: charts/podinfo/values-prod.yaml

controllers/helmchart_controller.go

@@ -22,6 +22,7 @@ import (
 	"io/ioutil"
 	"net/url"
 	"os"
+	"path/filepath"
 	"regexp"
 	"strings"
 	"time"
@@ -378,8 +379,18 @@ func (r *HelmChartReconciler) reconcileFromHelmRepository(ctx context.Context,
 			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 		}
 
+		// Find override file and retrieve contents
+		var valuesData []byte
+		cfn := filepath.Clean(chart.Spec.ValuesFile)
+		for _, f := range helmChart.Files {
+			if f.Name == cfn {
+				valuesData = f.Data
+				break
+			}
+		}
+
 		// Overwrite values file
-		if changed, err := helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile); err != nil {
+		if changed, err := helm.OverwriteChartDefaultValues(helmChart, valuesData); err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		} else if !changed {
 			// No changes, skip to write original package to storage
@@ -483,9 +494,27 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 	// or write the chart directly to storage.
 	pkgPath := chartPath
 	isValuesFileOverriden := false
-	if chart.Spec.ValuesFile != "" && chart.Spec.ValuesFile != chartutil.ValuesfileName {
-		// Overwrite default values if configured
-		isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, chart.Spec.ValuesFile)
+	if chart.Spec.ValuesFile != "" {
+		srcPath, err := securejoin.SecureJoin(tmpDir, chart.Spec.ValuesFile)
+		if err != nil {
+			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+		}
+		if f, err := os.Stat(srcPath); os.IsNotExist(err) || !f.Mode().IsRegular() {
+			err = fmt.Errorf("invalid values file path: %s", chart.Spec.ValuesFile)
+			return chart, err
+		}
+		src, err := os.Open(srcPath)
+		if err != nil {
+			err = fmt.Errorf("failed to open values file '%s': %w", chart.Spec.ValuesFile, err)
+			return chart, err
+		}
+		defer src.Close()
+
+		var valuesData []byte
+		if _, err := src.Read(valuesData); err == nil {
+			isValuesFileOverriden, err = helm.OverwriteChartDefaultValues(helmChart, valuesData)
+		}
+
 		if err != nil {
 			return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPackageFailedReason, err.Error()), err
 		}

controllers/helmchart_controller_test.go

@@ -693,6 +693,31 @@ var _ = Describe("HelmChartReconciler", func() {
 				return got.Status.Artifact != nil &&
 					storage.ArtifactExist(*got.Status.Artifact)
 			}, timeout, interval).Should(BeTrue())
+
+			When("Setting valid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "./charts/helmchart/override.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil &&
+						storage.ArtifactExist(*got.Status.Artifact)
+				}, timeout, interval).Should(BeTrue())
+			})
+
+			When("Setting invalid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "invalid.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil && got.Status.Artifact.Revision == updated.Status.Artifact.Revision
+				}, timeout, interval).Should(BeTrue())
+			})
 		})
 	})
 
@@ -936,6 +961,31 @@ var _ = Describe("HelmChartReconciler", func() {
 				return got.Status.Artifact != nil &&
 					storage.ArtifactExist(*got.Status.Artifact)
 			}, timeout, interval).Should(BeTrue())
+
+			When("Setting valid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "override.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil &&
+						storage.ArtifactExist(*got.Status.Artifact)
+				}, timeout, interval).Should(BeTrue())
+			})
+
+			When("Setting invalid valuesFile attribute", func() {
+				updated := &sourcev1.HelmChart{}
+				Expect(k8sClient.Get(context.Background(), key, updated)).To(Succeed())
+				chart.Spec.ValuesFile = "./charts/helmchart/override.yaml"
+				Expect(k8sClient.Update(context.Background(), updated)).To(Succeed())
+				got := &sourcev1.HelmChart{}
+				Eventually(func() bool {
+					_ = k8sClient.Get(context.Background(), key, got)
+					return got.Status.Artifact != nil && got.Status.Artifact.Revision == updated.Status.Artifact.Revision
+				}, timeout, interval).Should(BeTrue())
+			})
 		})
 	})
 })

controllers/testdata/charts/helmchart/override.yaml

@@ -0,0 +1,66 @@
+# Override values for helmchart.
+# This is a YAML-formatted file.
+# Declare variables to be passed into your templates.
+
+replicaCount: 3
+
+image:
+  repository: nginx
+  pullPolicy: IfNotPresent
+
+imagePullSecrets: []
+nameOverride: ""
+fullnameOverride: ""
+
+serviceAccount:
+  # Specifies whether a service account should be created
+  create: true
+  # The name of the service account to use.
+  # If not set and create is true, a name is generated using the fullname template
+  name:
+
+podSecurityContext: {}
+  # fsGroup: 2000
+
+securityContext: {}
+  # capabilities:
+  #   drop:
+  #   - ALL
+  # readOnlyRootFilesystem: true
+  # runAsNonRoot: true
+  # runAsUser: 1000
+
+service:
+  type: ClusterIP
+  port: 80
+
+ingress:
+  enabled: false
+  annotations: {}
+    # kubernetes.io/ingress.class: nginx
+    # kubernetes.io/tls-acme: "true"
+  hosts:
+    - host: chart-example.local
+      paths: []
+  tls: []
+  #  - secretName: chart-example-tls
+  #    hosts:
+  #      - chart-example.local
+
+resources: {}
+  # We usually recommend not to specify default resources and to leave this as a conscious
+  # choice for the user. This also increases chances charts run on environments with little
+  # resources, such as Minikube. If you do want to specify resources, uncomment the following
+  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.
+  # limits:
+  #   cpu: 100m
+  #   memory: 128Mi
+  # requests:
+  #   cpu: 100m
+  #   memory: 128Mi
+
+nodeSelector: {}
+
+tolerations: []
+
+affinity: {}

internal/helm/chart.go

@@ -25,46 +25,30 @@ import (
 )
 
 // OverwriteChartDefaultValues overwrites the chart default values file with the
-// contents of the given valuesFile.
-func OverwriteChartDefaultValues(chart *helmchart.Chart, valuesFile string) (bool, error) {
-	if valuesFile == "" || valuesFile == chartutil.ValuesfileName {
-		return false, nil
-	}
-
-	// Find override file and retrieve contents
-	var valuesData []byte
-	for _, f := range chart.Files {
-		if f.Name == valuesFile {
-			valuesData = f.Data
-			break
-		}
-	}
-	if valuesData == nil {
-		return false, fmt.Errorf("failed to locate override values file: %s", valuesFile)
-	}
-
+// given data.
+func OverwriteChartDefaultValues(chart *helmchart.Chart, data []byte) (bool, error) {
 	// Read override values file data
-	values, err := chartutil.ReadValues(valuesData)
+	values, err := chartutil.ReadValues(data)
 	if err != nil {
-		return false, fmt.Errorf("failed to parse override values file: %s", valuesFile)
+		return false, fmt.Errorf("failed to parse provided override values file data")
 	}
 
 	// Replace current values file in Raw field
 	for _, f := range chart.Raw {
 		if f.Name == chartutil.ValuesfileName {
 			// Do nothing if contents are equal
-			if reflect.DeepEqual(f.Data, valuesData) {
+			if reflect.DeepEqual(f.Data, data) {
 				return false, nil
 			}
 
 			// Replace in Files field
 			for _, f := range chart.Files {
 				if f.Name == chartutil.ValuesfileName {
-					f.Data = valuesData
+					f.Data = data
 				}
 			}
 
-			f.Data = valuesData
+			f.Data = data
 			chart.Values = values
 			return true, nil
 		}