Skip to content

Commit

Permalink
validation fix
Browse files Browse the repository at this point in the history
Signed-off-by: Matt Farina <matt.farina@suse.com>
  • Loading branch information
mattfarina committed Feb 7, 2024
1 parent e81f614 commit 8e6a514
Show file tree
Hide file tree
Showing 10 changed files with 112 additions and 0 deletions.
6 changes: 6 additions & 0 deletions pkg/chart/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ limitations under the License.
package chart

import (
"path/filepath"
"strings"
"unicode"

Expand Down Expand Up @@ -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")
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/chart/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down
8 changes: 8 additions & 0 deletions pkg/chartutil/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
20 changes: 20 additions & 0 deletions pkg/chartutil/save.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
29 changes: 29 additions & 0 deletions pkg/chartutil/save_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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())
}
}
26 changes: 26 additions & 0 deletions pkg/downloader/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/lint/rules/chartfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/lint/rules/chartfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions pkg/lint/rules/testdata/badchartname/Chart.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
apiVersion: v2
description: A Helm chart for Kubernetes
version: 0.1.0
name: "../badchartname"
type: application
1 change: 1 addition & 0 deletions pkg/lint/rules/testdata/badchartname/values.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# Default values for badchartfile.

1 comment on commit 8e6a514

@davidcorrigan714
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattfarina This commit seems to have broke a few things with chart name validation generating a few issues in the repo and for my organization internally. Unfortunately it seems there's no details in your commit or any associated pull request to understand the context or intention of these validation change. Can you explain what this was supposed to fix or what issues prompted this commit that it "fixed"?

Please sign in to comment.