From 7e7376f3e905a91cd2ca1953235d50799a8b5807 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 26 Oct 2026 21:23:29 +0530 Subject: [PATCH] Reject helm template name and namespace starting with '-' spec.template[].helmTemplate.name is passed to "helm template" as the first positional argument without validation. A value starting with '-' is parsed by helm as a flag instead of a release name, so arbitrary helm template flags can be supplied through this field, e.g. "--set-file=key=" makes helm read a file from the controller (or sidecarexec) container filesystem into the rendered manifests. The same value can also be set on a PackageInstall through the ext.packaging.carvel.dev/helm-template-name annotation. Return an error before invoking helm when the resolved name or namespace starts with '-'. Both values come from the App spec (or the PackageInstall annotations), so they are validated at the point where the helm arguments are built, which covers both paths. Valid release names and namespaces can never start with '-', so this does not change behavior for existing valid configurations. Changes: 1. pkg/template/helm_template.go - Reject name and namespace values starting with '-' 2. pkg/template/helm_template_test.go (new) - Add Test_HelmTemplate_NameAndNamespace covering accepted and rejected values, and verifying helm is not invoked when rejected - Add Test_HelmTemplate_NameIsNotParsedAsHelmFlag, which runs helm with a name of "--set-file=value=" and verifies the file content is not rendered. Without this change the file content appears in the helm output. Signed-off-by: Himanshu Singh --- pkg/template/helm_template.go | 9 +++ pkg/template/helm_template_test.go | 106 +++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 pkg/template/helm_template_test.go diff --git a/pkg/template/helm_template.go b/pkg/template/helm_template.go index 5870cef852..f451c09089 100644 --- a/pkg/template/helm_template.go +++ b/pkg/template/helm_template.go @@ -73,6 +73,15 @@ func (t *HelmTemplate) template(dirPath string, input io.Reader) exec.CmdRunResu namespace = t.opts.Namespace } + // name is passed to helm as a positional argument, so a leading '-' + // would be parsed as a helm flag instead of a release name. + if strings.HasPrefix(name, "-") { + return exec.NewCmdRunResultWithErr(fmt.Errorf("invalid helm template name '%s': must not start with '-'", name)) + } + if strings.HasPrefix(namespace, "-") { + return exec.NewCmdRunResultWithErr(fmt.Errorf("invalid helm template namespace '%s': must not start with '-'", namespace)) + } + args := []string{"template", name, chartPath, "--namespace", namespace, "--include-crds"} vals := Values{t.opts.ValuesFrom, t.additionalValues, t.appContext, t.coreClient} diff --git a/pkg/template/helm_template_test.go b/pkg/template/helm_template_test.go new file mode 100644 index 0000000000..c1006b102e --- /dev/null +++ b/pkg/template/helm_template_test.go @@ -0,0 +1,106 @@ +// Copyright 2024 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package template_test + +import ( + "fmt" + "os" + goexec "os/exec" + "path/filepath" + "testing" + + "carvel.dev/kapp-controller/pkg/apis/kappctrl/v1alpha1" + "carvel.dev/kapp-controller/pkg/exec" + "carvel.dev/kapp-controller/pkg/template" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + k8sfake "k8s.io/client-go/kubernetes/fake" +) + +type recordingCmdRunner struct { + cmds []*goexec.Cmd +} + +var _ exec.CmdRunner = &recordingCmdRunner{} + +func (r *recordingCmdRunner) Run(cmd *goexec.Cmd) error { + r.cmds = append(r.cmds, cmd) + return nil +} + +func (r *recordingCmdRunner) RunWithCancel(cmd *goexec.Cmd, _ chan struct{}) error { + return r.Run(cmd) +} + +func Test_HelmTemplate_NameAndNamespace(t *testing.T) { + appContext := template.AppContext{Name: "app-name", Namespace: "app-ns"} + + testCases := []struct { + desc string + opts v1alpha1.AppTemplateHelmTemplate + expectedName string + expectedNs string + expectedErr string + }{ + {desc: "defaults to app name and namespace", expectedName: "app-name", expectedNs: "app-ns"}, + {desc: "custom name and namespace", opts: v1alpha1.AppTemplateHelmTemplate{Name: "my-release", Namespace: "my-ns"}, + expectedName: "my-release", expectedNs: "my-ns"}, + {desc: "name with dash in the middle", opts: v1alpha1.AppTemplateHelmTemplate{Name: "my-release-1"}, + expectedName: "my-release-1", expectedNs: "app-ns"}, + {desc: "name with leading double dash", opts: v1alpha1.AppTemplateHelmTemplate{Name: "--post-renderer=/bin/sh"}, + expectedErr: "invalid helm template name '--post-renderer=/bin/sh': must not start with '-'"}, + {desc: "name with leading single dash", opts: v1alpha1.AppTemplateHelmTemplate{Name: "-g"}, + expectedErr: "invalid helm template name '-g': must not start with '-'"}, + {desc: "namespace with leading dash", opts: v1alpha1.AppTemplateHelmTemplate{Namespace: "--debug"}, + expectedErr: "invalid helm template namespace '--debug': must not start with '-'"}, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + cmdRunner := &recordingCmdRunner{} + helmTemplate := template.NewHelmTemplate(tc.opts, appContext, k8sfake.NewSimpleClientset(), + cmdRunner, template.AdditionalDownwardAPIValues{}) + + result, _ := helmTemplate.TemplateDir(t.TempDir()) + + if tc.expectedErr != "" { + assert.ErrorContains(t, result.Error, tc.expectedErr) + assert.Empty(t, cmdRunner.cmds, "helm should not be invoked") + return + } + require.NoError(t, result.Error) + require.Len(t, cmdRunner.cmds, 1) + args := cmdRunner.cmds[0].Args + assert.Equal(t, []string{"helm", "template", tc.expectedName}, args[:3]) + assert.Equal(t, []string{"--namespace", tc.expectedNs}, args[4:6]) + }) + } +} + +func Test_HelmTemplate_NameIsNotParsedAsHelmFlag(t *testing.T) { + rootDir := t.TempDir() + chartDir := filepath.Join(rootDir, "chart") + outsideFile := filepath.Join(rootDir, "outside.txt") + require.NoError(t, os.MkdirAll(filepath.Join(chartDir, "templates"), 0700)) + require.NoError(t, os.WriteFile(filepath.Join(chartDir, "Chart.yaml"), + []byte("apiVersion: v2\nname: test-chart\nversion: 0.1.0\n"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(chartDir, "templates", "cm.yaml"), []byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm +data: + value: {{ .Values.value | default "" | quote }} +`), 0600)) + require.NoError(t, os.WriteFile(outsideFile, []byte("content-outside-chart-dir"), 0600)) + + opts := v1alpha1.AppTemplateHelmTemplate{Name: fmt.Sprintf("--set-file=value=%s", outsideFile)} + helmTemplate := template.NewHelmTemplate(opts, template.AppContext{Name: "app", Namespace: "default"}, + k8sfake.NewSimpleClientset(), exec.NewPlainCmdRunner(), template.AdditionalDownwardAPIValues{}) + + result, _ := helmTemplate.TemplateDir(chartDir) + + assert.ErrorContains(t, result.Error, "must not start with '-'") + assert.NotContains(t, result.Stdout, "content-outside-chart-dir") +}