Repository: helm/helm
Pull request #8750 (2020-09-17 17:21:16)

validate the name passed in during helm create
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/master/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:

Improve validation of chart during helm create

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility



Patch:
diff --git a/pkg/chartutil/create.go b/pkg/chartutil/create.go
index 6e382b961..d4b65e9b8 100644
--- a/pkg/chartutil/create.go
+++ b/pkg/chartutil/create.go
@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -30,6 +31,12 @@ import (
 	"helm.sh/helm/v3/pkg/chart/loader"
 )
 
+// chartName is a regular expression for testing the supplied name of a chart.
+// This regular expression is probably stricter than it needs to be. We can relax it
+// somewhat. Newline characters, as well as $, quotes, +, parens, and % are known to be
+// problematic.
+var chartName = regexp.MustCompile("^[a-zA-Z0-9._-]+$")
+
 const (
 	// ChartfileName is the default Chart file name.
 	ChartfileName = "Chart.yaml"
@@ -63,6 +70,10 @@ const (
 	TestConnectionName = TemplatesTestsDir + sep + "test-connection.yaml"
 )
 
+// maxChartNameLength is lower than the limits we know of with certain file systems,
+// and with certain Kubernetes fields.
+const maxChartNameLength = 250
+
 const sep = string(filepath.Separator)
 
 const defaultChartfile = `apiVersion: v2
@@ -522,6 +533,12 @@ func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
 // error. In such a case, this will attempt to clean up by removing the
 // new chart directory.
 func Create(name, dir string) (string, error) {
+
+	// Sanity-check the name of a chart so user doesn't create one that causes problems.
+	if err := validateChartName(name); err != nil {
+		return "", err
+	}
+
 	path, err := filepath.Abs(dir)
 	if err != nil {
 		return path, err
@@ -627,3 +644,13 @@ func writeFile(name string, content []byte) error {
 	}
 	return ioutil.WriteFile(name, content, 0644)
 }
+
+func validateChartName(name string) error {
+	if name == "" || len(name) > maxChartNameLength {
+		return fmt.Errorf("chart name must be between 1 and %d characters", maxChartNameLength)
+	}
+	if !chartName.MatchString(name) {
+		return fmt.Errorf("chart name must match the regular expression %q", chartName.String())
+	}
+	return nil
+}


Test patch:
diff --git a/pkg/chartutil/create_test.go b/pkg/chartutil/create_test.go
index a11c45140..f68ebbd63 100644
--- a/pkg/chartutil/create_test.go
+++ b/pkg/chartutil/create_test.go
@@ -117,3 +117,30 @@ func TestCreateFrom(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateChartName(t *testing.T) {
+	for name, shouldPass := range map[string]bool{
+		"":                              false,
+		"abcdefghijklmnopqrstuvwxyz-_.": true,
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": true,
+		"$hello":                        false,
+		"Hellô":                         false,
+		"he%%o":                         false,
+		"he\nllo":                       false,
+
+		"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"abcdefghijklmnopqrstuvwxyz-_." +
+			"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": false,
+	} {
+		if err := validateChartName(name); (err != nil) == shouldPass {
+			t.Errorf("test for %q failed", name)
+		}
+	}
+}