From 04c305396458508a31d03d44afea07b1c620d7cd Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Tue, 21 Jun 2022 06:39:56 -0700 Subject: [PATCH] Merge pull request from GHSA-q4w5-4gq2-98vm Signed-off-by: Michael Crenshaw fix tests/lint Signed-off-by: Michael Crenshaw --- reposerver/repository/repository.go | 23 +++++++++++-------- reposerver/repository/repository_test.go | 22 ++++++++++++++++++ .../in-bounds-values-file-link/Chart.yaml | 2 ++ .../in-bounds-values-file-link/values-2.yaml | 1 + .../in-bounds-values-file-link/values.yaml | 1 + .../out-of-bounds-values-file-link/Chart.yaml | 2 ++ .../values.yaml | 1 + .../repository/testdata/out-of-bounds.yaml | 1 + util/helm/helm.go | 23 +++++++++++++------ util/helm/helm_test.go | 6 ++--- 10 files changed, 63 insertions(+), 19 deletions(-) create mode 100644 reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml create mode 100644 reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml create mode 120000 reposerver/repository/testdata/in-bounds-values-file-link/values.yaml create mode 100644 reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml create mode 120000 reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml create mode 100644 reposerver/repository/testdata/out-of-bounds.yaml diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 1545be65183c8..8a53ba6c5047c 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -1461,8 +1461,12 @@ 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 @@ -1470,10 +1474,10 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin 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 index f2402044b5a50..723bfe1aae1f9 100644 --- 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 new file mode 100644 index 0000000000000..f01dd5407e5a6 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..aad07b2bfc83b --- /dev/null +++ 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 new file mode 120000 index 0000000000000..d860891957ff4 --- /dev/null +++ b/reposerver/repository/testdata/in-bounds-values-file-link/values.yaml @@ -0,0 +1 @@ +values-2.yaml \ No newline at end of file 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 new file mode 100644 index 0000000000000..f01dd5407e5a6 --- /dev/null +++ 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 new file mode 120000 index 0000000000000..4e034a19440a6 --- /dev/null +++ b/reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml @@ -0,0 +1 @@ +../out-of-bounds.yaml \ No newline at end of file diff --git a/reposerver/repository/testdata/out-of-bounds.yaml b/reposerver/repository/testdata/out-of-bounds.yaml new file mode 100644 index 0000000000000..aad07b2bfc83b --- /dev/null +++ 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 index 6b6e109d1a8f8..8b83881fe0961 100644 --- 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 index 400b1132cc4ff..bee3cdbdd2d88 100644 --- 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"]