diff --git a/internal/plugin/loader.go b/internal/plugin/loader.go index a58a84126..2f051b99e 100644 --- a/internal/plugin/loader.go +++ b/internal/plugin/loader.go @@ -47,6 +47,7 @@ func loadMetadataLegacy(metadataData []byte) (*Metadata, error) { var ml MetadataLegacy d := yaml.NewDecoder(bytes.NewReader(metadataData)) + // NOTE: No strict unmarshalling for legacy plugins - maintain backwards compatibility if err := d.Decode(&ml); err != nil { return nil, err } @@ -66,6 +67,7 @@ func loadMetadataV1(metadataData []byte) (*Metadata, error) { var mv1 MetadataV1 d := yaml.NewDecoder(bytes.NewReader(metadataData)) + d.KnownFields(true) if err := d.Decode(&mv1); err != nil { return nil, err } diff --git a/internal/plugin/loader_test.go b/internal/plugin/loader_test.go index 47c214910..e84905248 100644 --- a/internal/plugin/loader_test.go +++ b/internal/plugin/loader_test.go @@ -268,3 +268,96 @@ func TestFindPlugins(t *testing.T) { }) } } + +func TestLoadMetadataLegacy(t *testing.T) { + testCases := map[string]struct { + yaml string + expectError bool + errorContains string + expectedName string + logNote string + }{ + "capital name field": { + yaml: `Name: my-plugin +version: 1.0.0 +usage: test plugin +description: test description +command: echo test`, + expectError: true, + errorContains: `invalid plugin name "": must contain only a-z, A-Z, 0-9, _ and -`, + // Legacy plugins: No strict unmarshalling (backwards compatibility) + // YAML decoder silently ignores "Name:", then validation catches empty name + logNote: "NOTE: V1 plugins use strict unmarshalling and would get: yaml: field Name not found", + }, + "correct name field": { + yaml: `name: my-plugin +version: 1.0.0 +usage: test plugin +description: test description +command: echo test`, + expectError: false, + expectedName: "my-plugin", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + m, err := loadMetadataLegacy([]byte(tc.yaml)) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorContains) + t.Logf("Legacy error (validation catches empty name): %v", err) + if tc.logNote != "" { + t.Log(tc.logNote) + } + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedName, m.Name) + } + }) + } +} + +func TestLoadMetadataV1(t *testing.T) { + testCases := map[string]struct { + yaml string + expectError bool + errorContains string + expectedName string + }{ + "capital name field": { + yaml: `apiVersion: v1 +Name: my-plugin +type: cli/v1 +runtime: subprocess +`, + expectError: true, + errorContains: "field Name not found in type plugin.MetadataV1", + }, + "correct name field": { + yaml: `apiVersion: v1 +name: my-plugin +type: cli/v1 +runtime: subprocess +`, + expectError: false, + expectedName: "my-plugin", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + m, err := loadMetadataV1([]byte(tc.yaml)) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errorContains) + t.Logf("V1 error (strict unmarshalling): %v", err) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedName, m.Name) + } + }) + } +} diff --git a/internal/plugin/metadata.go b/internal/plugin/metadata.go index 111c0599f..b051c1223 100644 --- a/internal/plugin/metadata.go +++ b/internal/plugin/metadata.go @@ -54,7 +54,7 @@ func (m Metadata) Validate() error { var errs []error if !validPluginName.MatchString(m.Name) { - errs = append(errs, fmt.Errorf("invalid name")) + errs = append(errs, fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)) } if m.APIVersion == "" { diff --git a/internal/plugin/metadata_legacy.go b/internal/plugin/metadata_legacy.go index a7b245dc0..3cd1a50cd 100644 --- a/internal/plugin/metadata_legacy.go +++ b/internal/plugin/metadata_legacy.go @@ -69,7 +69,7 @@ type MetadataLegacy struct { func (m *MetadataLegacy) Validate() error { if !validPluginName.MatchString(m.Name) { - return fmt.Errorf("invalid plugin name") + return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name) } m.Usage = sanitizeString(m.Usage) diff --git a/internal/plugin/metadata_legacy_test.go b/internal/plugin/metadata_legacy_test.go new file mode 100644 index 000000000..9421e98b5 --- /dev/null +++ b/internal/plugin/metadata_legacy_test.go @@ -0,0 +1,126 @@ +/* +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 TestMetadataLegacyValidate(t *testing.T) { + testsValid := map[string]MetadataLegacy{ + "valid metadata": { + Name: "myplugin", + }, + "valid with command": { + Name: "myplugin", + Command: "echo hello", + }, + "valid with platformCommand": { + Name: "myplugin", + PlatformCommand: []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo hello"}, + }, + }, + "valid with hooks": { + Name: "myplugin", + Hooks: Hooks{ + "install": "echo install", + }, + }, + "valid with platformHooks": { + Name: "myplugin", + PlatformHooks: PlatformHooks{ + "install": []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo install"}, + }, + }, + }, + "valid with downloaders": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{"myproto"}, + Command: "echo download", + }, + }, + }, + } + + for testName, metadata := range testsValid { + t.Run(testName, func(t *testing.T) { + assert.NoError(t, metadata.Validate()) + }) + } + + testsInvalid := map[string]MetadataLegacy{ + "invalid name": { + Name: "my plugin", // further tested in TestValidPluginName + }, + "both command and platformCommand": { + Name: "myplugin", + Command: "echo hello", + PlatformCommand: []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo hello"}, + }, + }, + "both hooks and platformHooks": { + Name: "myplugin", + Hooks: Hooks{ + "install": "echo install", + }, + PlatformHooks: PlatformHooks{ + "install": []PlatformCommand{ + {OperatingSystem: "linux", Architecture: "amd64", Command: "echo install"}, + }, + }, + }, + "downloader with empty command": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{"myproto"}, + Command: "", + }, + }, + }, + "downloader with no protocols": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{}, + Command: "echo download", + }, + }, + }, + "downloader with empty protocol": { + Name: "myplugin", + Downloaders: []Downloaders{ + { + Protocols: []string{""}, + Command: "echo download", + }, + }, + }, + } + + for testName, metadata := range testsInvalid { + t.Run(testName, func(t *testing.T) { + assert.Error(t, metadata.Validate()) + }) + } +} diff --git a/internal/plugin/metadata_test.go b/internal/plugin/metadata_test.go index 28bc4cf51..145ef5101 100644 --- a/internal/plugin/metadata_test.go +++ b/internal/plugin/metadata_test.go @@ -53,10 +53,10 @@ func TestValidatePluginData(t *testing.T) { }{ {true, mockSubprocessCLIPlugin(t, "abcdefghijklmnopqrstuvwxyz0123456789_-ABC"), ""}, {true, mockSubprocessCLIPlugin(t, "foo-bar-FOO-BAR_1234"), ""}, - {false, mockSubprocessCLIPlugin(t, "foo -bar"), "invalid name"}, - {false, mockSubprocessCLIPlugin(t, "$foo -bar"), "invalid name"}, // Test leading chars - {false, mockSubprocessCLIPlugin(t, "foo -bar "), "invalid name"}, // Test trailing chars - {false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid name"}, // Test newline + {false, mockSubprocessCLIPlugin(t, "foo -bar"), "invalid plugin name"}, + {false, mockSubprocessCLIPlugin(t, "$foo -bar"), "invalid plugin name"}, // Test leading chars + {false, mockSubprocessCLIPlugin(t, "foo -bar "), "invalid plugin name"}, // Test trailing chars + {false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid plugin name"}, // Test newline {true, mockNoCommand, ""}, // Test no command metadata works {true, mockLegacyCommand, ""}, // Test legacy command metadata works } { @@ -66,8 +66,8 @@ func TestValidatePluginData(t *testing.T) { } else if !item.pass && err == nil { t.Errorf("expected case %d to fail", i) } - if !item.pass && err.Error() != item.errString { - t.Errorf("index [%d]: expected the following error: %s, but got: %s", i, item.errString, err.Error()) + if !item.pass && !strings.Contains(err.Error(), item.errString) { + t.Errorf("index [%d]: expected error to contain: %s, but got: %s", i, item.errString, err.Error()) } } } @@ -92,7 +92,7 @@ func TestMetadataValidateMultipleErrors(t *testing.T) { // Check that all expected errors are present in the joined error expectedErrors := []string{ - "invalid name", + "invalid plugin name", "empty APIVersion", "empty type field", "empty runtime field", diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index b6c2245ff..ae0b343f3 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -21,6 +21,44 @@ import ( "helm.sh/helm/v4/internal/plugin/schema" ) +func TestValidPluginName(t *testing.T) { + validNames := map[string]string{ + "lowercase": "myplugin", + "uppercase": "MYPLUGIN", + "mixed case": "MyPlugin", + "with digits": "plugin123", + "with hyphen": "my-plugin", + "with underscore": "my_plugin", + "mixed chars": "my-awesome_plugin_123", + } + + for name, pluginName := range validNames { + t.Run("valid/"+name, func(t *testing.T) { + if !validPluginName.MatchString(pluginName) { + t.Errorf("expected %q to match validPluginName regex", pluginName) + } + }) + } + + invalidNames := map[string]string{ + "empty": "", + "space": "my plugin", + "colon": "plugin:", + "period": "my.plugin", + "slash": "my/plugin", + "dollar": "$plugin", + "unicode": "plügîn", + } + + for name, pluginName := range invalidNames { + t.Run("invalid/"+name, func(t *testing.T) { + if validPluginName.MatchString(pluginName) { + t.Errorf("expected %q to not match validPluginName regex", pluginName) + } + }) + } +} + func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginRuntime { t.Helper()