Merge pull request from GHSA-q4w5-4gq2-98vm

Signed-off-by: Michael Crenshaw <[email protected]> fix tests/lint Signed-off-by: Michael Crenshaw <[email protected]>
argoproj · Jun 21, 2022 · 04c3053 · 04c3053
1 parent 17f7f4f
commit 04c3053
Show file tree

Hide file tree

Showing 10 changed files with 63 additions and 19 deletions.
diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go
@@ -1461,19 +1461,23 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
 		return err
 	}
 
-	if err := loadFileIntoIfExists(filepath.Join(appPath, "values.yaml"), &res.Helm.Values); err != nil {
-		return err
+	if resolvedValuesPath, _, err := pathutil.ResolveFilePath(appPath, repoRoot, "values.yaml", []string{}); err == nil {
+		if err := loadFileIntoIfExists(resolvedValuesPath, &res.Helm.Values); err != nil {
+			return err
+		}
+	} else {
+		log.Warnf("Values file %s is not allowed: %v", filepath.Join(appPath, "values.yaml"), err)
 	}
 	var resolvedSelectedValueFiles []pathutil.ResolvedFilePath
 	// drop not allowed values files
 	for _, file := range selectedValueFiles {
 		if resolvedFile, _, err := pathutil.ResolveFilePath(appPath, repoRoot, file, q.GetValuesFileSchemes()); err == nil {
 			resolvedSelectedValueFiles = append(resolvedSelectedValueFiles, resolvedFile)
 		} else {
-			log.Debugf("Values file %s is not allowed: %v", file, err)
+			log.Warnf("Values file %s is not allowed: %v", file, err)
 		}
 	}
-	params, err := h.GetParameters(resolvedSelectedValueFiles)
+	params, err := h.GetParameters(resolvedSelectedValueFiles, appPath, repoRoot)
 	if err != nil {
 		return err
 	}
@@ -1492,15 +1496,16 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
 	return nil
 }
 
-func loadFileIntoIfExists(path string, destination *string) error {
-	info, err := os.Stat(path)
+func loadFileIntoIfExists(path pathutil.ResolvedFilePath, destination *string) error {
+	stringPath := string(path)
+	info, err := os.Stat(stringPath)
 
 	if err == nil && !info.IsDir() {
-		if bytes, err := ioutil.ReadFile(path); err != nil {
-			*destination = string(bytes)
-		} else {
+		bytes, err := ioutil.ReadFile(stringPath);
+		if err != nil {
 			return err
 		}
+		*destination = string(bytes)
 	}
 
 	return nil

diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go
@@ -1125,11 +1125,13 @@ func TestListApps(t *testing.T) {
 		"app-parameters/single-app-only": "Kustomize",
 		"app-parameters/single-global":   "Kustomize",
 		"invalid-helm":                   "Helm",
+		"in-bounds-values-file-link":     "Helm",
 		"invalid-kustomize":              "Kustomize",
 		"kustomization_yaml":             "Kustomize",
 		"kustomization_yml":              "Kustomize",
 		"my-chart":                       "Helm",
 		"my-chart-2":                     "Helm",
+		"out-of-bounds-values-file-link": "Helm",
 		"values-files":                   "Helm",
 	}
 	assert.Equal(t, expectedApps, res.Apps)
@@ -2027,3 +2029,23 @@ func Test_populateHelmAppDetails(t *testing.T) {
 	assert.Len(t, res.Helm.Parameters, 3)
 	assert.Len(t, res.Helm.ValueFiles, 4)
 }
+
+func Test_populateHelmAppDetails_values_symlinks(t *testing.T) {
+	t.Run("inbound", func(t *testing.T) {
+		res := apiclient.RepoAppDetailsResponse{}
+		q := apiclient.RepoServerAppDetailsQuery{Repo: &argoappv1.Repository{}, Source: &argoappv1.ApplicationSource{}}
+		err := populateHelmAppDetails(&res, "./testdata/in-bounds-values-file-link/", "./testdata/in-bounds-values-file-link/", &q)
+		require.NoError(t, err)
+		assert.NotEmpty(t, res.Helm.Values)
+		assert.NotEmpty(t, res.Helm.Parameters)
+	})
+
+	t.Run("out of bounds", func(t *testing.T) {
+		res := apiclient.RepoAppDetailsResponse{}
+		q := apiclient.RepoServerAppDetailsQuery{Repo: &argoappv1.Repository{}, Source: &argoappv1.ApplicationSource{}}
+		err := populateHelmAppDetails(&res, "./testdata/out-of-bounds-values-file-link/", "./testdata/out-of-bounds-values-file-link/", &q)
+		require.NoError(t, err)
+		assert.Empty(t, res.Helm.Values)
+		assert.Empty(t, res.Helm.Parameters)
+	})
+}
diff --git a/reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml b/reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml
@@ -0,0 +1,2 @@
+name: my-chart
+version: 1.1.0
diff --git a/reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml b/reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml
@@ -0,0 +1 @@
+some: yaml
diff --git a/reposerver/repository/testdata/in-bounds-values-file-link/values.yaml b/reposerver/repository/testdata/in-bounds-values-file-link/values.yaml
@@ -0,0 +1 @@
+values-2.yaml
diff --git a/reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml b/reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml
@@ -0,0 +1,2 @@
+name: my-chart
+version: 1.1.0
diff --git a/reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml b/reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml
@@ -0,0 +1 @@
+../out-of-bounds.yaml
diff --git a/reposerver/repository/testdata/out-of-bounds.yaml b/reposerver/repository/testdata/out-of-bounds.yaml
@@ -0,0 +1 @@
+some: yaml
diff --git a/util/helm/helm.go b/util/helm/helm.go
@@ -6,9 +6,11 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 
 	"github.com/ghodss/yaml"
+	log "github.com/sirupsen/logrus"
 
 	"github.com/argoproj/argo-cd/v2/util/config"
 	executil "github.com/argoproj/argo-cd/v2/util/exec"
@@ -27,7 +29,7 @@ type Helm interface {
 	// Template returns a list of unstructured objects from a `helm template` command
 	Template(opts *TemplateOpts) (string, error)
 	// GetParameters returns a list of chart parameters taking into account values in provided YAML files.
-	GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[string]string, error)
+	GetParameters(valuesFiles []pathutil.ResolvedFilePath, appPath, repoRoot string) (map[string]string, error)
 	// DependencyBuild runs `helm dependency build` to download a chart's dependencies
 	DependencyBuild() error
 	// Init runs `helm init --client-only`
@@ -129,12 +131,19 @@ func Version(shortForm bool) (string, error) {
 	return strings.TrimSpace(version), nil
 }
 
-func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[string]string, error) {
-	out, err := h.cmd.inspectValues(".")
-	if err != nil {
-		return nil, err
+func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath, appPath, repoRoot string) (map[string]string, error) {
+	var values []string
+	// Don't load values.yaml if it's an out-of-bounds link.
+	if resolved, _, err := pathutil.ResolveFilePath(appPath, repoRoot, "values.yaml", []string{}); err == nil {
+		fmt.Println(resolved)
+		out, err := h.cmd.inspectValues(".")
+		if err != nil {
+			return nil, err
+		}
+		values = append(values, out)
+	} else {
+		log.Warnf("Values file %s is not allowed: %v", filepath.Join(appPath, "values.yaml"), err)
 	}
-	values := []string{out}
 	for i := range valuesFiles {
 		file := string(valuesFiles[i])
 		var fileValues []byte
@@ -156,7 +165,7 @@ func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[strin
 	output := map[string]string{}
 	for _, file := range values {
 		values := map[string]interface{}{}
-		if err = yaml.Unmarshal([]byte(file), &values); err != nil {
+		if err := yaml.Unmarshal([]byte(file), &values); err != nil {
 			return nil, fmt.Errorf("failed to parse values: %s", err)
 		}
 		flatVals(values, output)

diff --git a/util/helm/helm_test.go b/util/helm/helm_test.go
@@ -85,7 +85,7 @@ func TestHelmGetParams(t *testing.T) {
 	require.NoError(t, err)
 	h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false)
 	assert.NoError(t, err)
-	params, err := h.GetParameters(nil)
+	params, err := h.GetParameters(nil, repoRootAbs, repoRootAbs)
 	assert.Nil(t, err)
 
 	slaveCountParam := params["cluster.slaveCount"]
@@ -100,7 +100,7 @@ func TestHelmGetParamsValueFiles(t *testing.T) {
 	assert.NoError(t, err)
 	valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
 	require.NoError(t, err)
-	params, err := h.GetParameters([]path.ResolvedFilePath{valuesPath})
+	params, err := h.GetParameters([]path.ResolvedFilePath{valuesPath}, repoRootAbs, repoRootAbs)
 	assert.Nil(t, err)
 
 	slaveCountParam := params["cluster.slaveCount"]
@@ -117,7 +117,7 @@ func TestHelmGetParamsValueFilesThatExist(t *testing.T) {
 	require.NoError(t, err)
 	valuesProductionPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
 	require.NoError(t, err)
-	params, err := h.GetParameters([]path.ResolvedFilePath{valuesMissingPath, valuesProductionPath})
+	params, err := h.GetParameters([]path.ResolvedFilePath{valuesMissingPath, valuesProductionPath}, repoRootAbs, repoRootAbs)
 	assert.Nil(t, err)
 
 	slaveCountParam := params["cluster.slaveCount"]