Merge commit from fork

fix: Plugin version path traversal

cmd/helm/helm_test.go

@@ -69,7 +69,7 @@ func TestCliPluginExitCode(t *testing.T) {
 
 		assert.Empty(t, stdout.String())
 
-		expectedStderr := "Error: plugin \"exitwith\" exited with error\n"
+		expectedStderr := "level=WARN msg=\"failed to load plugin (ignoring)\" plugin_yaml=../../pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml error=\"failed to load plugin \\\"../../pkg/cmd/testdata/helmhome/helm/plugins/noversion\\\": plugin `version` is required\"\nError: plugin \"exitwith\" exited with error\n"
 		if stderr.String() != expectedStderr {
 			t.Errorf("Expected %q written to stderr: Got %q", expectedStderr, stderr.String())
 		}

internal/plugin/loader.go

@@ -19,6 +19,7 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"log/slog"
 	"os"
 	"path/filepath"
 
@@ -158,18 +159,27 @@ func LoadDir(dirname string) (Plugin, error) {
 	return pm.CreatePlugin(dirname, m)
 }
 
-// LoadAll loads all plugins found beneath the base directory.
+func LogIgnorePluginLoadErrorFilterFunc(pluginYAML string, err error) error {
+	slog.Warn("failed to load plugin (ignoring)", slog.String("plugin_yaml", pluginYAML), slog.Any("error", err))
+	return nil
+}
+
+// errorFilterFunc is a function that can filter errors during plugin loading
+type ErrorFilterFunc func(string, error) error
+
+// LoadAllDir load all plugins found beneath the base directory, using the provided error filter to determine whether to fail on individual plugin load errors.
 //
 // This scans only one directory level.
-func LoadAll(basedir string) ([]Plugin, error) {
-	var plugins []Plugin
-	// We want basedir/*/plugin.yaml
+func LoadAllDir(basedir string, errorFilter ErrorFilterFunc) ([]Plugin, error) {
+	// We want <basedir>/*/plugin.yaml
 	scanpath := filepath.Join(basedir, "*", PluginFileName)
 	matches, err := filepath.Glob(scanpath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to search for plugins in %q: %w", scanpath, err)
 	}
 
+	plugins := make([]Plugin, 0, len(matches))
+
 	// empty dir should load
 	if len(matches) == 0 {
 		return plugins, nil
@@ -179,9 +189,12 @@ func LoadAll(basedir string) ([]Plugin, error) {
 		dir := filepath.Dir(yamlFile)
 		p, err := LoadDir(dir)
 		if err != nil {
-			return plugins, err
+			if errNew := errorFilter(yamlFile, err); errNew != nil {
+				return plugins, errNew
+			}
+		} else {
+			plugins = append(plugins, p)
 		}
-		plugins = append(plugins, p)
 	}
 	return plugins, detectDuplicates(plugins)
 }
@@ -193,8 +206,12 @@ type findFunc func(pluginsDir string) ([]Plugin, error)
 type filterFunc func(Plugin) bool
 
 // FindPlugins returns a list of plugins that match the descriptor
+// Errors loading a plugin are ignored with a warning
 func FindPlugins(pluginsDirs []string, descriptor Descriptor) ([]Plugin, error) {
-	return findPlugins(pluginsDirs, LoadAll, makeDescriptorFilter(descriptor))
+	loadAllIgnoreErrors := func(pluginsDir string) ([]Plugin, error) {
+		return LoadAllDir(pluginsDir, LogIgnorePluginLoadErrorFilterFunc)
+	}
+	return findPlugins(pluginsDirs, loadAllIgnoreErrors, makeDescriptorFilter(descriptor))
 }
 
 // findPlugins is the internal implementation that uses the find and filter functions
@@ -237,7 +254,11 @@ func makeDescriptorFilter(descriptor Descriptor) filterFunc {
 
 // FindPlugin returns a single plugin that matches the descriptor
 func FindPlugin(dirs []string, descriptor Descriptor) (Plugin, error) {
-	plugins, err := FindPlugins(dirs, descriptor)
+	loadAllIgnoreErrors := func(pluginsDir string) ([]Plugin, error) {
+		return LoadAllDir(pluginsDir, LogIgnorePluginLoadErrorFilterFunc)
+	}
+
+	plugins, err := findPlugins(dirs, loadAllIgnoreErrors, makeDescriptorFilter(descriptor))
 	if err != nil {
 		return nil, err
 	}

internal/plugin/loader_test.go

@@ -204,16 +204,16 @@ func TestDetectDuplicates(t *testing.T) {
 	}
 }
 
-func TestLoadAll(t *testing.T) {
-	// Verify that empty dir loads:
-	{
-		plugs, err := LoadAll("testdata")
-		require.NoError(t, err)
-		assert.Len(t, plugs, 0)
-	}
+func TestLoadAllDir_Empty(t *testing.T) {
+	emptyDir := t.TempDir()
+	plugs, err := LoadAllDir(emptyDir, func(_ string, err error) error { return err })
+	require.NoError(t, err)
+	assert.Len(t, plugs, 0)
+}
 
+func TestLoadAllPluginsDir(t *testing.T) {
 	basedir := "testdata/plugdir/good"
-	plugs, err := LoadAll(basedir)
+	plugs, err := LoadAllDir(basedir, func(_ string, err error) error { return err })
 	require.NoError(t, err)
 	require.NotEmpty(t, plugs, "expected plugins to be loaded from %s", basedir)
 
@@ -232,7 +232,7 @@ func TestLoadAll(t *testing.T) {
 	assert.Contains(t, plugsMap, "postrenderer-v1")
 }
 
-func TestFindPlugins(t *testing.T) {
+func TestLoadAllPluginsDir_Zero(t *testing.T) {
 	cases := []struct {
 		name     string
 		plugdirs string
@@ -240,28 +240,20 @@ func TestFindPlugins(t *testing.T) {
 	}{
 		{
 			name:     "plugdirs is empty",
-			plugdirs: "",
-			expected: 0,
+			plugdirs: t.TempDir(),
 		},
 		{
 			name:     "plugdirs isn't dir",
 			plugdirs: "./plugin_test.go",
-			expected: 0,
 		},
 		{
 			name:     "plugdirs doesn't have plugin",
 			plugdirs: ".",
-			expected: 0,
-		},
-		{
-			name:     "normal",
-			plugdirs: "./testdata/plugdir/good",
-			expected: 7,
 		},
 	}
 	for _, c := range cases {
 		t.Run(t.Name(), func(t *testing.T) {
-			plugin, err := LoadAll(c.plugdirs)
+			plugin, err := LoadAllDir(c.plugdirs, func(_ string, err error) error { return err })
 			require.NoError(t, err)
 			assert.Len(t, plugin, c.expected, "expected %d plugins, got %d", c.expected, len(plugin))
 		})
@@ -337,6 +329,7 @@ runtime: subprocess
 		"correct name field": {
 			yaml: `apiVersion: v1
 name: my-plugin
+version: 1.0.0
 type: cli/v1
 runtime: subprocess
 `,

internal/plugin/metadata.go

@@ -19,9 +19,17 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/Masterminds/semver/v3"
+
 	"helm.sh/helm/v4/internal/plugin/schema"
 )
 
+// isValidSemver checks if the given string is a valid semantic version
+func isValidSemver(v string) bool {
+	_, err := semver.StrictNewVersion(v)
+	return err == nil
+}
+
 // Metadata of a plugin, converted from the "on-disk" legacy or v1 plugin.yaml
 // Specifically, Config and RuntimeConfig are converted to their respective types based on the plugin type and runtime
 type Metadata struct {
@@ -57,6 +65,11 @@ func (m Metadata) Validate() error {
 		errs = append(errs, fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name))
 	}
 
+	// Require version to be valid semver if specified
+	if m.Version != "" && !isValidSemver(m.Version) {
+		errs = append(errs, fmt.Errorf("invalid plugin version %q: must be valid semver", m.Version))
+	}
+
 	if m.APIVersion == "" {
 		errs = append(errs, errors.New("empty APIVersion"))
 	}

internal/plugin/metadata_legacy.go

@@ -72,6 +72,11 @@ func (m *MetadataLegacy) Validate() error {
 	if !validPluginName.MatchString(m.Name) {
 		return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)
 	}
+
+	if m.Version != "" && !isValidSemver(m.Version) {
+		return fmt.Errorf("invalid plugin version %q: must be valid semver", m.Version)
+	}
+
 	m.Usage = sanitizeString(m.Usage)
 
 	if len(m.PlatformCommand) > 0 && len(m.Command) > 0 {

internal/plugin/metadata_legacy_test.go

@@ -26,6 +26,10 @@ func TestMetadataLegacyValidate(t *testing.T) {
 		"valid metadata": {
 			Name: "myplugin",
 		},
+		"valid metadata (empty version)": {
+			Name:    "myplugin",
+			Version: "",
+		},
 		"valid with command": {
 			Name:    "myplugin",
 			Command: "echo hello",
@@ -59,6 +63,13 @@ func TestMetadataLegacyValidate(t *testing.T) {
 				},
 			},
 		},
+		"valid with version": {
+			Name:    "myplugin",
+			Version: "1.0.0",
+		},
+		"valid with empty version": {
+			Name: "myplugin",
+		},
 	}
 
 	for testName, metadata := range testsValid {
@@ -116,6 +127,14 @@ func TestMetadataLegacyValidate(t *testing.T) {
 				},
 			},
 		},
+		"path traversal version": {
+			Name:    "myplugin",
+			Version: "../../../../tmp/evil",
+		},
+		"invalid version": {
+			Name:    "myplugin",
+			Version: "not-a-version",
+		},
 	}
 
 	for testName, metadata := range testsInvalid {

internal/plugin/metadata_test.go

@@ -18,6 +18,8 @@ package plugin
 import (
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestValidatePluginData(t *testing.T) {
@@ -72,6 +74,43 @@ func TestValidatePluginData(t *testing.T) {
 	}
 }
 
+func TestMetadataValidateVersion(t *testing.T) {
+	testValid := map[string]struct {
+		version string
+	}{
+		"valid semver":                 {version: "1.0.0"},
+		"valid semver with prerelease": {version: "1.2.3-alpha.1+build.123"},
+		"empty version":                {version: ""},
+	}
+
+	testInvalid := map[string]struct {
+		version string
+	}{
+		"valid semver with v prefix": {version: "v1.0.0"},
+		"path traversal":             {version: "../../../../tmp/evil"},
+		"path traversal in version":  {version: "1.0.0/../../etc"},
+		"not a version":              {version: "not-a-version"},
+	}
+
+	for name, tc := range testValid {
+		t.Run(name, func(t *testing.T) {
+			m := mockSubprocessCLIPlugin(t, "testplugin")
+			m.metadata.Version = tc.version
+			err := m.Metadata().Validate()
+			assert.NoError(t, err)
+		})
+	}
+
+	for name, tc := range testInvalid {
+		t.Run(name, func(t *testing.T) {
+			m := mockSubprocessCLIPlugin(t, "testplugin")
+			m.metadata.Version = tc.version
+			err := m.Metadata().Validate()
+			assert.ErrorContains(t, err, "invalid plugin version")
+		})
+	}
+}
+
 func TestMetadataValidateMultipleErrors(t *testing.T) {
 	// Create metadata with multiple validation issues
 	metadata := Metadata{

internal/plugin/metadata_v1.go

@@ -52,6 +52,13 @@ func (m *MetadataV1) Validate() error {
 		return errors.New("invalid plugin `name`")
 	}
 
+	if m.Version == "" {
+		return errors.New("plugin `version` is required")
+	}
+	if !isValidSemver(m.Version) {
+		return fmt.Errorf("invalid plugin `version` %q: must be valid semver", m.Version)
+	}
+
 	if m.APIVersion != "v1" {
 		return fmt.Errorf("invalid `apiVersion`: %q", m.APIVersion)
 	}

internal/plugin/metadata_v1_test.go

@@ -0,0 +1,85 @@
+/*
+Copyright The Helm Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package plugin
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMetadataV1ValidateVersion(t *testing.T) {
+	base := func() MetadataV1 {
+		return MetadataV1{
+			APIVersion: "v1",
+			Name:       "myplugin",
+			Type:       "cli/v1",
+			Runtime:    "subprocess",
+			Version:    "1.0.0",
+		}
+	}
+
+	testsValid := map[string]string{
+		"simple version":  "1.0.0",
+		"with prerelease": "1.2.3-alpha.1",
+		"with build meta": "1.2.3+build.123",
+		"full prerelease": "1.2.3-alpha.1+build.123",
+	}
+
+	for name, version := range testsValid {
+		t.Run("valid/"+name, func(t *testing.T) {
+			m := base()
+			m.Version = version
+			assert.NoError(t, m.Validate())
+		})
+	}
+
+	testsInvalid := map[string]struct {
+		version string
+		errMsg  string
+	}{
+		"empty version": {
+			version: "",
+			errMsg:  "plugin `version` is required",
+		},
+		"v prefix": {
+			version: "v1.0.0",
+			errMsg:  "invalid plugin `version` \"v1.0.0\": must be valid semver",
+		},
+		"path traversal": {
+			version: "../../../../tmp/evil",
+			errMsg:  "invalid plugin `version`",
+		},
+		"path traversal etc": {
+			version: "../../../etc/passwd",
+			errMsg:  "invalid plugin `version`",
+		},
+		"not a version": {
+			version: "not-a-version",
+			errMsg:  "invalid plugin `version`",
+		},
+	}
+
+	for name, tc := range testsInvalid {
+		t.Run("invalid/"+name, func(t *testing.T) {
+			m := base()
+			m.Version = tc.version
+			err := m.Validate()
+			assert.Error(t, err)
+			assert.Contains(t, err.Error(), tc.errMsg)
+		})
+	}
+}

internal/plugin/plugin_test.go

@@ -82,7 +82,7 @@ func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginR
 	return &SubprocessPluginRuntime{
 		metadata: Metadata{
 			Name:       pluginName,
-			Version:    "v0.1.2",
+			Version:    "0.1.2",
 			Type:       "cli/v1",
 			APIVersion: "v1",
 			Runtime:    "subprocess",

internal/plugin/runtime_subprocess_test.go

@@ -42,7 +42,7 @@ func mockSubprocessCLIPluginErrorExit(t *testing.T, pluginName string, exitCode
 
 	md := Metadata{
 		Name:       pluginName,
-		Version:    "v0.1.2",
+		Version:    "0.1.2",
 		Type:       "cli/v1",
 		APIVersion: "v1",
 		Runtime:    "subprocess",

pkg/cmd/plugin_test.go

@@ -117,6 +117,7 @@ func TestLoadCLIPlugins(t *testing.T) {
 		{"exitwith", "exitwith code", "This exits with the specified exit code", "", []string{"2"}, 2},
 		{"fullenv", "show env vars", "show all env vars", fullEnvOutput, []string{}, 0},
 		{"shortenv", "env stuff", "show the env", "HELM_PLUGIN_NAME=shortenv\n", []string{}, 0},
+		// "noversion": plugin is invalid, and should not be loaded
 	}
 
 	pluginCmds := cmd.Commands()

pkg/cmd/plugin_uninstall.go

@@ -62,7 +62,7 @@ func (o *pluginUninstallOptions) complete(args []string) error {
 
 func (o *pluginUninstallOptions) run(out io.Writer) error {
 	slog.Debug("loading installer plugins", "dir", settings.PluginsDirectory)
-	plugins, err := plugin.LoadAll(settings.PluginsDirectory)
+	plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc)
 	if err != nil {
 		return err
 	}

pkg/cmd/plugin_update.go

@@ -62,7 +62,7 @@ func (o *pluginUpdateOptions) complete(args []string) error {
 
 func (o *pluginUpdateOptions) run(out io.Writer) error {
 	slog.Debug("loading installed plugins", "path", settings.PluginsDirectory)
-	plugins, err := plugin.LoadAll(settings.PluginsDirectory)
+	plugins, err := plugin.LoadAllDir(settings.PluginsDirectory, plugin.LogIgnorePluginLoadErrorFilterFunc)
 	if err != nil {
 		return err
 	}

pkg/cmd/testdata/helm home with space/helm/plugins/fullenv/plugin.yaml

@@ -1,6 +1,7 @@
 ---
 apiVersion: v1
 name: fullenv
+version: 0.1.0
 type: cli/v1
 runtime: subprocess
 config:

pkg/cmd/testdata/helmhome/helm/plugins/args/plugin.yaml

@@ -1,4 +1,5 @@
 name: args
+version: 0.1.0
 type: cli/v1
 apiVersion: v1
 runtime: subprocess

pkg/cmd/testdata/helmhome/helm/plugins/echo/plugin.yaml

@@ -1,4 +1,5 @@
 name: echo
+version: 0.1.0
 type: cli/v1
 apiVersion: v1
 runtime: subprocess

pkg/cmd/testdata/helmhome/helm/plugins/exitwith/plugin.yaml

@@ -1,6 +1,7 @@
 ---
 apiVersion: v1
 name: exitwith
+version: 0.1.0
 type: cli/v1
 runtime: subprocess
 config:

pkg/cmd/testdata/helmhome/helm/plugins/fullenv/plugin.yaml

@@ -1,6 +1,7 @@
 ---
 apiVersion: v1
 name: fullenv
+version: 0.1.0
 type: cli/v1
 runtime: subprocess
 config:

pkg/cmd/testdata/helmhome/helm/plugins/noversion/plugin.yaml

@@ -0,0 +1,7 @@
+apiVersion: v1
+name: noversion
+type: cli/v1
+runtime: subprocess
+runtimeConfig:
+  platformCommand:
+  - command: "echo hello"

pkg/cmd/testdata/helmhome/helm/plugins/shortenv/plugin.yaml

@@ -1,6 +1,7 @@
 ---
 apiVersion: v1
 name: shortenv
+version: 0.1.0
 type: cli/v1
 runtime: subprocess
 config:

pkg/cmd/testdata/testplugin/plugin.yaml

@@ -1,6 +1,7 @@
 ---
 apiVersion: v1
 name: testplugin
+version: 0.1.0
 type: cli/v1
 runtime: subprocess
 config: