From 809e2d999e2c33e20e77f6bff30652d79c287542 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Thu, 17 Sep 2020 12:35:10 -0600 Subject: [PATCH] Merge pull request from GHSA-m54r-vrmv-hw33 Signed-off-by: Matt Butcher --- cmd/helm/load_plugins.go | 2 +- cmd/helm/plugin_install.go | 3 ++- pkg/plugin/plugin.go | 47 +++++++++++++++++++++++++++++++----- pkg/plugin/plugin_test.go | 49 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 8 deletions(-) diff --git a/cmd/helm/load_plugins.go b/cmd/helm/load_plugins.go index e4aac6c0f..83590210a 100644 --- a/cmd/helm/load_plugins.go +++ b/cmd/helm/load_plugins.go @@ -59,7 +59,7 @@ func loadPlugins(baseCmd *cobra.Command, out io.Writer) { found, err := plugin.FindPlugins(settings.PluginsDirectory) if err != nil { - fmt.Fprintf(os.Stderr, "failed to load plugins: %s", err) + fmt.Fprintf(os.Stderr, "failed to load plugins: %s\n", err) return } diff --git a/cmd/helm/plugin_install.go b/cmd/helm/plugin_install.go index 183d3dc57..4e8ee327b 100644 --- a/cmd/helm/plugin_install.go +++ b/cmd/helm/plugin_install.go @@ -19,6 +19,7 @@ import ( "fmt" "io" + "github.com/pkg/errors" "github.com/spf13/cobra" "helm.sh/helm/v3/cmd/helm/require" @@ -81,7 +82,7 @@ func (o *pluginInstallOptions) run(out io.Writer) error { debug("loading plugin from %s", i.Path()) p, err := plugin.LoadDir(i.Path()) if err != nil { - return err + return errors.Wrap(err, "plugin is installed but unusable") } if err := runHook(p, plugin.Install); err != nil { diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go index caa34fbd3..9bac2244c 100644 --- a/pkg/plugin/plugin.go +++ b/pkg/plugin/plugin.go @@ -20,9 +20,11 @@ import ( "io/ioutil" "os" "path/filepath" + "regexp" "runtime" "strings" + "github.com/pkg/errors" "sigs.k8s.io/yaml" "helm.sh/helm/v3/pkg/cli" @@ -157,18 +159,51 @@ func (p *Plugin) PrepareCommand(extraArgs []string) (string, []string, error) { return main, baseArgs, nil } +// validPluginName is a regular expression that validates plugin names. +// +// Plugin names can only contain the ASCII characters a-z, A-Z, 0-9, ​_​ and ​-. +var validPluginName = regexp.MustCompile("^[A-Za-z0-9_-]+$") + +// validatePluginData validates a plugin's YAML data. +func validatePluginData(plug *Plugin, filepath string) error { + if !validPluginName.MatchString(plug.Metadata.Name) { + return fmt.Errorf("invalid plugin name at %q", filepath) + } + // We could also validate SemVer, executable, and other fields should we so choose. + return nil +} + +func detectDuplicates(plugs []*Plugin) error { + names := map[string]string{} + + for _, plug := range plugs { + if oldpath, ok := names[plug.Metadata.Name]; ok { + return fmt.Errorf( + "two plugins claim the name %q at %q and %q", + plug.Metadata.Name, + oldpath, + plug.Dir, + ) + } + names[plug.Metadata.Name] = plug.Dir + } + + return nil +} + // LoadDir loads a plugin from the given directory. func LoadDir(dirname string) (*Plugin, error) { - data, err := ioutil.ReadFile(filepath.Join(dirname, PluginFileName)) + pluginfile := filepath.Join(dirname, PluginFileName) + data, err := ioutil.ReadFile(pluginfile) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to read plugin at %q", pluginfile) } plug := &Plugin{Dir: dirname} if err := yaml.Unmarshal(data, &plug.Metadata); err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to load plugin at %q", pluginfile) } - return plug, nil + return plug, validatePluginData(plug, pluginfile) } // LoadAll loads all plugins found beneath the base directory. @@ -180,7 +215,7 @@ func LoadAll(basedir string) ([]*Plugin, error) { scanpath := filepath.Join(basedir, "*", PluginFileName) matches, err := filepath.Glob(scanpath) if err != nil { - return plugins, err + return plugins, errors.Wrapf(err, "failed to find plugins in %q", scanpath) } if matches == nil { @@ -195,7 +230,7 @@ func LoadAll(basedir string) ([]*Plugin, error) { } plugins = append(plugins, p) } - return plugins, nil + return plugins, detectDuplicates(plugins) } // FindPlugins returns a list of YAML files that describe plugins. diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go index af0b61846..88add037d 100644 --- a/pkg/plugin/plugin_test.go +++ b/pkg/plugin/plugin_test.go @@ -16,6 +16,7 @@ limitations under the License. package plugin // import "helm.sh/helm/v3/pkg/plugin" import ( + "fmt" "os" "path/filepath" "reflect" @@ -320,3 +321,51 @@ func TestSetupEnv(t *testing.T) { } } } + +func TestValidatePluginData(t *testing.T) { + for i, item := range []struct { + pass bool + plug *Plugin + }{ + {true, mockPlugin("abcdefghijklmnopqrstuvwxyz0123456789_-ABC")}, + {true, mockPlugin("foo-bar-FOO-BAR_1234")}, + {false, mockPlugin("foo -bar")}, + {false, mockPlugin("$foo -bar")}, // Test leading chars + {false, mockPlugin("foo -bar ")}, // Test trailing chars + {false, mockPlugin("foo\nbar")}, // Test newline + } { + err := validatePluginData(item.plug, fmt.Sprintf("test-%d", i)) + if item.pass && err != nil { + t.Errorf("failed to validate case %d: %s", i, err) + } else if !item.pass && err == nil { + t.Errorf("expected case %d to fail", i) + } + } +} + +func TestDetectDuplicates(t *testing.T) { + plugs := []*Plugin{ + mockPlugin("foo"), + mockPlugin("bar"), + } + if err := detectDuplicates(plugs); err != nil { + t.Error("no duplicates in the first set") + } + plugs = append(plugs, mockPlugin("foo")) + if err := detectDuplicates(plugs); err == nil { + t.Error("duplicates in the second set") + } +} + +func mockPlugin(name string) *Plugin { + return &Plugin{ + Metadata: &Metadata{ + Name: name, + Version: "v0.1.2", + Usage: "Mock plugin", + Description: "Mock plugin for testing", + Command: "echo mock plugin", + }, + Dir: "no-such-dir", + } +}