diff --git a/pkg/chart/metadata.go b/pkg/chart/metadata.go index 97bfc2c0c..a08a97cd1 100644 --- a/pkg/chart/metadata.go +++ b/pkg/chart/metadata.go @@ -16,6 +16,7 @@ limitations under the License. package chart import ( + "path/filepath" "strings" "unicode" @@ -110,6 +111,11 @@ func (md *Metadata) Validate() error { if md.Name == "" { return ValidationError("chart.metadata.name is required") } + + if md.Name != filepath.Base(md.Name) { + return ValidationErrorf("chart.metadata.name %q is invalid", md.Name) + } + if md.Version == "" { return ValidationError("chart.metadata.version is required") } diff --git a/pkg/chart/metadata_test.go b/pkg/chart/metadata_test.go index c8b89ae55..62aea7261 100644 --- a/pkg/chart/metadata_test.go +++ b/pkg/chart/metadata_test.go @@ -40,6 +40,11 @@ func TestValidate(t *testing.T) { &Metadata{APIVersion: "v2", Version: "1.0"}, ValidationError("chart.metadata.name is required"), }, + { + "chart without name", + &Metadata{Name: "../../test", APIVersion: "v2", Version: "1.0"}, + ValidationError("chart.metadata.name \"../../test\" is invalid"), + }, { "chart without version", &Metadata{Name: "test", APIVersion: "v2"}, diff --git a/pkg/chartutil/errors.go b/pkg/chartutil/errors.go index fcdcc27ea..0a4046d2e 100644 --- a/pkg/chartutil/errors.go +++ b/pkg/chartutil/errors.go @@ -33,3 +33,11 @@ type ErrNoValue struct { } func (e ErrNoValue) Error() string { return fmt.Sprintf("%q is not a value", e.Key) } + +type ErrInvalidChartName struct { + Name string +} + +func (e ErrInvalidChartName) Error() string { + return fmt.Sprintf("%q is not a valid chart name", e.Name) +} diff --git a/pkg/chartutil/save.go b/pkg/chartutil/save.go index 2ce4eddaf..4ee90709c 100644 --- a/pkg/chartutil/save.go +++ b/pkg/chartutil/save.go @@ -39,6 +39,10 @@ var headerBytes = []byte("+aHR0cHM6Ly95b3V0dS5iZS96OVV6MWljandyTQo=") // directory, writing the chart's contents to that subdirectory. func SaveDir(c *chart.Chart, dest string) error { // Create the chart directory + err := validateName(c.Name()) + if err != nil { + return err + } outdir := filepath.Join(dest, c.Name()) if fi, err := os.Stat(outdir); err == nil && !fi.IsDir() { return errors.Errorf("file %s already exists and is not a directory", outdir) @@ -149,6 +153,10 @@ func Save(c *chart.Chart, outDir string) (string, error) { } func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error { + err := validateName(c.Name()) + if err != nil { + return err + } base := filepath.Join(prefix, c.Name()) // Pull out the dependencies of a v1 Chart, since there's no way @@ -242,3 +250,15 @@ func writeToTar(out *tar.Writer, name string, body []byte) error { _, err := out.Write(body) return err } + +// If the name has directory name has characters which would change the location +// they need to be removed. +func validateName(name string) error { + nname := filepath.Base(name) + + if nname != name { + return ErrInvalidChartName{name} + } + + return nil +} diff --git a/pkg/chartutil/save_test.go b/pkg/chartutil/save_test.go index db485c7cb..98b4e641b 100644 --- a/pkg/chartutil/save_test.go +++ b/pkg/chartutil/save_test.go @@ -106,6 +106,24 @@ func TestSave(t *testing.T) { } }) } + + c := &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: chart.APIVersionV1, + Name: "../ahab", + Version: "1.2.3", + }, + Lock: &chart.Lock{ + Digest: "testdigest", + }, + Files: []*chart.File{ + {Name: "scheherazade/shahryar.txt", Data: []byte("1,001 Nights")}, + }, + } + _, err := Save(c, tmp) + if err == nil { + t.Fatal("Expected error saving chart with invalid name") + } } // Creates a copy with a different schema; does not modify anything. @@ -232,4 +250,15 @@ func TestSaveDir(t *testing.T) { if len(c2.Files) != 1 || c2.Files[0].Name != c.Files[0].Name { t.Fatal("Files data did not match") } + + tmp2 := t.TempDir() + c.Metadata.Name = "../ahab" + pth := filepath.Join(tmp2, "tmpcharts") + if err := os.MkdirAll(filepath.Join(pth), 0755); err != nil { + t.Fatal(err) + } + + if err := SaveDir(c, pth); err.Error() != "\"../ahab\" is not a valid chart name" { + t.Fatalf("Did not get expected error for chart named %q", c.Name()) + } } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index f7ab1a568..db2487d16 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -262,6 +262,32 @@ func TestDownloadAll(t *testing.T) { if _, err := os.Stat(filepath.Join(chartPath, "charts", "signtest-0.1.0.tgz")); os.IsNotExist(err) { t.Error(err) } + + // A chart with a bad name like this cannot be loaded and saved. Handling in + // the loading and saving will return an error about the invalid name. In + // this case, the chart needs to be created directly. + badchartyaml := `apiVersion: v2 +description: A Helm chart for Kubernetes +name: ../bad-local-subchart +version: 0.1.0` + if err := os.MkdirAll(filepath.Join(chartPath, "testdata", "bad-local-subchart"), 0755); err != nil { + t.Fatal(err) + } + err = os.WriteFile(filepath.Join(chartPath, "testdata", "bad-local-subchart", "Chart.yaml"), []byte(badchartyaml), 0644) + if err != nil { + t.Fatal(err) + } + + badLocalDep := &chart.Dependency{ + Name: "../bad-local-subchart", + Repository: "file://./testdata/bad-local-subchart", + Version: "0.1.0", + } + + err = m.downloadAll([]*chart.Dependency{badLocalDep}) + if err == nil { + t.Fatal("Expected error for bad dependency name") + } } func TestUpdateBeforeBuild(t *testing.T) { diff --git a/pkg/lint/rules/chartfile.go b/pkg/lint/rules/chartfile.go index 70532ad4f..910602b7d 100644 --- a/pkg/lint/rules/chartfile.go +++ b/pkg/lint/rules/chartfile.go @@ -106,6 +106,10 @@ func validateChartName(cf *chart.Metadata) error { if cf.Name == "" { return errors.New("name is required") } + name := filepath.Base(cf.Name) + if name != cf.Name { + return fmt.Errorf("chart name %q is invalid", cf.Name) + } return nil } diff --git a/pkg/lint/rules/chartfile_test.go b/pkg/lint/rules/chartfile_test.go index 087cda047..a06d7dc31 100644 --- a/pkg/lint/rules/chartfile_test.go +++ b/pkg/lint/rules/chartfile_test.go @@ -30,16 +30,19 @@ import ( ) const ( + badCharNametDir = "testdata/badchartname" badChartDir = "testdata/badchartfile" anotherBadChartDir = "testdata/anotherbadchartfile" ) var ( + badChartNamePath = filepath.Join(badCharNametDir, "Chart.yaml") badChartFilePath = filepath.Join(badChartDir, "Chart.yaml") nonExistingChartFilePath = filepath.Join(os.TempDir(), "Chart.yaml") ) var badChart, _ = chartutil.LoadChartfile(badChartFilePath) +var badChartName, _ = chartutil.LoadChartfile(badChartNamePath) // Validation functions Test func TestValidateChartYamlNotDirectory(t *testing.T) { @@ -69,6 +72,11 @@ func TestValidateChartName(t *testing.T) { if err == nil { t.Errorf("validateChartName to return a linter error, got no error") } + + err = validateChartName(badChartName) + if err == nil { + t.Error("expected validateChartName to return a linter error for an invalid name, got no error") + } } func TestValidateChartVersion(t *testing.T) { diff --git a/pkg/lint/rules/testdata/badchartname/Chart.yaml b/pkg/lint/rules/testdata/badchartname/Chart.yaml new file mode 100644 index 000000000..64f8fb8bf --- /dev/null +++ b/pkg/lint/rules/testdata/badchartname/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +description: A Helm chart for Kubernetes +version: 0.1.0 +name: "../badchartname" +type: application diff --git a/pkg/lint/rules/testdata/badchartname/values.yaml b/pkg/lint/rules/testdata/badchartname/values.yaml new file mode 100644 index 000000000..9f367033b --- /dev/null +++ b/pkg/lint/rules/testdata/badchartname/values.yaml @@ -0,0 +1 @@ +# Default values for badchartfile. diff --git a/pkg/repo/index.go b/pkg/repo/index.go index 8a23ba060..2f062b028 100644 --- a/pkg/repo/index.go +++ b/pkg/repo/index.go @@ -362,7 +362,7 @@ func loadIndex(data []byte, source string) (*IndexFile, error) { if cvs[idx].APIVersion == "" { cvs[idx].APIVersion = chart.APIVersionV1 } - if err := cvs[idx].Validate(); err != nil { + if err := cvs[idx].Validate(); ignoreSkippableChartValidationError(err) != nil { log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", name, cvs[idx].Version, source, err) cvs = append(cvs[:idx], cvs[idx+1:]...) } @@ -388,3 +388,23 @@ func jsonOrYamlUnmarshal(b []byte, i interface{}) error { } return yaml.UnmarshalStrict(b, i) } + +// ignoreSkippableChartValidationError inspect the given error and returns nil if +// the error isn't important for index loading +// +// In particular, charts may introduce validations that don't impact repository indexes +// And repository indexes may be generated by older/non-complient software, which doesn't +// conform to all validations. +func ignoreSkippableChartValidationError(err error) error { + verr, ok := err.(chart.ValidationError) + if !ok { + return err + } + + // https://github.com/helm/helm/issues/12748 (JFrog repository strips alias field) + if strings.HasPrefix(verr.Error(), "validation: more than one dependency with name or alias") { + return nil + } + + return err +} diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index cb9317f7e..3852dc15d 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -20,6 +20,7 @@ import ( "bufio" "bytes" "encoding/json" + "fmt" "net/http" "os" "path/filepath" @@ -592,3 +593,50 @@ func TestAddFileIndexEntriesNil(t *testing.T) { } } } + +func TestIgnoreSkippableChartValidationError(t *testing.T) { + type TestCase struct { + Input error + ErrorSkipped bool + } + testCases := map[string]TestCase{ + "nil": { + Input: nil, + }, + "generic_error": { + Input: fmt.Errorf("foo"), + }, + "non_skipped_validation_error": { + Input: chart.ValidationError("chart.metadata.type must be application or library"), + }, + "skipped_validation_error": { + Input: chart.ValidationErrorf("more than one dependency with name or alias %q", "foo"), + ErrorSkipped: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + result := ignoreSkippableChartValidationError(tc.Input) + + if tc.Input == nil { + if result != nil { + t.Error("expected nil result for nil input") + } + return + } + + if tc.ErrorSkipped { + if result != nil { + t.Error("expected nil result for skipped error") + } + return + } + + if tc.Input != result { + t.Error("expected the result equal to input") + } + + }) + } +}