Check index or plugin name safety in cmd (#583)

* Check index or plugin name safety in cmd Moving plugin name or index name safety checks to cmd/ where they are given by the user to the program. This way we shift the need for validation from being unit tests of low-level machinery to the integration_test which tests user-facing concerns. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com> * code review feedback Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com> * address code review feedback Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com> * fix bug in install Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
kubernetes-sigs · Apr 4, 2020 · da05925 · da05925
1 parent f958833
commit da05925
Show file tree

Hide file tree

Showing 11 changed files with 147 additions and 14 deletions.
diff --git a/cmd/krew/cmd/index.go b/cmd/krew/cmd/index.go
@@ -29,7 +29,8 @@ import (
 )
 
 var (
-	forceIndexDelete *bool
+	forceIndexDelete    *bool
+	errInvalidIndexName = errors.New("invalid index name")
 )
 
 // indexCmd represents the index command
@@ -70,7 +71,11 @@ var indexAddCmd = &cobra.Command{
 	Example: "kubectl krew index add default " + constants.IndexURI,
 	Args:    cobra.ExactArgs(2),
 	RunE: func(_ *cobra.Command, args []string) error {
-		return indexoperations.AddIndex(paths, args[0], args[1])
+		name := args[0]
+		if !indexoperations.IsValidIndexName(name) {
+			return errInvalidIndexName
+		}
+		return indexoperations.AddIndex(paths, name, args[1])
 	},
 }
 
@@ -89,6 +94,9 @@ option is used ( not recommended).`,
 
 func indexDelete(_ *cobra.Command, args []string) error {
 	name := args[0]
+	if !indexoperations.IsValidIndexName(name) {
+		return errInvalidIndexName
+	}
 
 	ps, err := installation.InstalledPluginsFromIndex(paths.InstallReceiptsPath(), name)
 	if err != nil {

diff --git a/cmd/krew/cmd/install.go b/cmd/krew/cmd/install.go
@@ -26,6 +26,7 @@ import (
 
 	"sigs.k8s.io/krew/cmd/krew/cmd/internal"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
+	"sigs.k8s.io/krew/internal/index/validation"
 	"sigs.k8s.io/krew/internal/installation"
 	"sigs.k8s.io/krew/internal/pathutil"
 	"sigs.k8s.io/krew/pkg/index"
@@ -98,6 +99,10 @@ Remarks:
 			var install []pluginEntry
 			for _, name := range pluginNames {
 				indexName, pluginName := pathutil.CanonicalPluginName(name)
+				if !validation.IsSafePluginName(pluginName) {
+					return unsafePluginNameErr(pluginName)
+				}
+
 				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(indexName), pluginName)
 				if err != nil {
 					if os.IsNotExist(err) {

diff --git a/cmd/krew/cmd/uninstall.go b/cmd/krew/cmd/uninstall.go
@@ -22,6 +22,7 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/klog"
 
+	"sigs.k8s.io/krew/internal/index/validation"
 	"sigs.k8s.io/krew/internal/installation"
 )
 
@@ -38,6 +39,9 @@ Remarks:
   Failure to uninstall a plugin will result in an error and exit immediately.`,
 	RunE: func(cmd *cobra.Command, args []string) error {
 		for _, name := range args {
+			if !validation.IsSafePluginName(name) {
+				return unsafePluginNameErr(name)
+			}
 			klog.V(4).Infof("Going to uninstall plugin %s\n", name)
 			if err := installation.Uninstall(paths, name); err != nil {
 				return errors.Wrapf(err, "failed to uninstall plugin %s", name)
@@ -51,6 +55,8 @@ Remarks:
 	Aliases: []string{"remove"},
 }
 
+func unsafePluginNameErr(n string) error { return errors.Errorf("plugin name %q not allowed", n) }
+
 func init() {
 	rootCmd.AddCommand(uninstallCmd)
 }
diff --git a/cmd/krew/cmd/upgrade.go b/cmd/krew/cmd/upgrade.go
@@ -24,6 +24,7 @@ import (
 
 	"sigs.k8s.io/krew/cmd/krew/cmd/internal"
 	"sigs.k8s.io/krew/internal/index/indexscanner"
+	"sigs.k8s.io/krew/internal/index/validation"
 	"sigs.k8s.io/krew/internal/installation"
 	"sigs.k8s.io/krew/internal/pathutil"
 )
@@ -64,6 +65,9 @@ kubectl krew upgrade foo bar"`,
 			var nErrors int
 			for _, name := range pluginNames {
 				indexName, pluginName := pathutil.CanonicalPluginName(name)
+				if !validation.IsSafePluginName(pluginName) {
+					return unsafePluginNameErr(pluginName)
+				}
 				plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(indexName), pluginName)
 				if err != nil {
 					if !os.IsNotExist(err) {

diff --git a/integration_test/index_test.go b/integration_test/index_test.go
@@ -16,6 +16,7 @@ package integrationtest
 
 import (
 	"os"
+	"strings"
 	"testing"
 
 	"sigs.k8s.io/krew/pkg/constants"
@@ -45,6 +46,25 @@ func TestKrewIndexAdd(t *testing.T) {
 	}
 }
 
+func TestKrewIndexAddUnsafe(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	defer cleanup()
+	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+
+	cases := []string{"a/b", `a\b`, "../a", `..\a`}
+	expected := "invalid index name"
+
+	for _, c := range cases {
+		b, err := test.Krew("index", "add", c, constants.IndexURI).Run()
+		if err == nil {
+			t.Fatalf("%q: expected error", c)
+		} else if !strings.Contains(string(b), expected) {
+			t.Fatalf("%q: output doesn't contain %q: %q", c, expected, string(b))
+		}
+	}
+}
+
 func TestKrewIndexList(t *testing.T) {
 	skipShort(t)
 
@@ -109,6 +129,24 @@ func TestKrewIndexRemove_ok(t *testing.T) {
 	test.Krew("index", "remove", "foo").RunOrFail()
 }
 
+func TestKrewIndexRemove_unsafe(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	test = test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	defer cleanup()
+
+	expected := "invalid index name"
+	cases := []string{"a/b", `a\b`, "../a", `..\a`}
+	for _, c := range cases {
+		b, err := test.Krew("index", "remove", c).Run()
+		if err == nil {
+			t.Fatalf("%q: expected error", c)
+		} else if !strings.Contains(string(b), expected) {
+			t.Fatalf("%q: output doesn't contain %q: %q", c, expected, string(b))
+		}
+	}
+}
+
 func TestKrewIndexRemoveFailsWhenPluginsInstalled(t *testing.T) {
 	skipShort(t)
 	test, cleanup := NewTest(t)

diff --git a/integration_test/install_test.go b/integration_test/install_test.go
@@ -58,6 +58,31 @@ func TestKrewInstallReRun(t *testing.T) {
 	test.AssertExecutableInPATH("kubectl-" + validPlugin)
 }
 
+func TestKrewInstallUnsafe(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	defer cleanup()
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+
+	cases := []string{
+		`../index/` + validPlugin,
+		`..\index\` + validPlugin,
+		`../default/` + validPlugin,
+		`..\default\` + validPlugin,
+		`index-name/sub-directory/plugin-name`,
+	}
+
+	expectedErr := `not allowed`
+	for _, c := range cases {
+		b, err := test.Krew("install", c).Run()
+		if err == nil {
+			t.Fatalf("%q expected failure", c)
+		} else if !strings.Contains(string(b), expectedErr) {
+			t.Fatalf("%q does not contain err %q: %q", c, expectedErr, string(b))
+		}
+	}
+}
+
 func TestKrewInstall_MultiplePositionalArgs(t *testing.T) {
 	skipShort(t)
 

diff --git a/integration_test/uninstall_test.go b/integration_test/uninstall_test.go
@@ -17,6 +17,7 @@ package integrationtest
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"sigs.k8s.io/krew/internal/environment"
@@ -75,3 +76,30 @@ func TestKrewRemove_ManifestRemovedFromIndex(t *testing.T) {
 	}
 	test.Krew("remove", validPlugin).RunOrFail()
 }
+
+func TestKrewRemove_Unsafe(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	defer cleanup()
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	test.Krew("install", validPlugin).RunOrFailOutput()
+
+	cases := []string{
+		`../index/` + validPlugin,
+		`..\index\` + validPlugin,
+		`../default/` + validPlugin,
+		`..\default\` + validPlugin,
+		`../receipts/` + validPlugin,
+		`..\receipts\` + validPlugin,
+		`default/subdir/plugin-name`,
+	}
+	expectedErr := `not allowed`
+	for _, c := range cases {
+		b, err := test.Krew("uninstall", c).Run()
+		if err == nil {
+			t.Fatalf("%q expected failure", c)
+		} else if !strings.Contains(string(b), expectedErr) {
+			t.Fatalf("%q does not contain err %q: %q", c, expectedErr, string(b))
+		}
+	}
+}
diff --git a/integration_test/upgrade_test.go b/integration_test/upgrade_test.go
@@ -50,6 +50,31 @@ func TestKrewUpgrade(t *testing.T) {
 	}
 }
 
+func TestKrewUpgradeUnsafe(t *testing.T) {
+	skipShort(t)
+	test, cleanup := NewTest(t)
+	defer cleanup()
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+
+	cases := []string{
+		`../index/` + validPlugin,
+		`..\index\` + validPlugin,
+		`../default/` + validPlugin,
+		`..\default\` + validPlugin,
+		`index-name/sub-directory/plugin-name`,
+	}
+
+	expectedErr := `not allowed`
+	for _, c := range cases {
+		b, err := test.Krew("upgrade", c).Run()
+		if err == nil {
+			t.Fatalf("%q expected failure", c)
+		} else if !strings.Contains(string(b), expectedErr) {
+			t.Fatalf("%q does not contain err %q: %q", c, expectedErr, string(b))
+		}
+	}
+}
+
 func TestKrewUpgradeWhenPlatformNoLongerMatches(t *testing.T) {
 	skipShort(t)
 

diff --git a/internal/index/indexoperations/index.go b/internal/index/indexoperations/index.go
@@ -59,10 +59,6 @@ func ListIndexes(paths environment.Paths) ([]Index, error) {
 
 // AddIndex initializes a new index to install plugins from.
 func AddIndex(paths environment.Paths, name, url string) error {
-	if !IsValidIndexName(name) {
-		return errors.New("invalid index name")
-	}
-
 	dir := paths.IndexPath(name)
 	if _, err := os.Stat(dir); os.IsNotExist(err) {
 		return gitutil.EnsureCloned(url, dir)
@@ -74,14 +70,11 @@ func AddIndex(paths environment.Paths, name, url string) error {
 
 // DeleteIndex removes specified index name. If index does not exist, returns an error that can be tested by os.IsNotExist.
 func DeleteIndex(paths environment.Paths, name string) error {
-	if !IsValidIndexName(name) {
-		return errors.New("invalid index name")
-	}
-
 	dir := paths.IndexPath(name)
 	if _, err := os.Stat(dir); err != nil {
 		return err
 	}
+
 	return os.RemoveAll(dir)
 }
 

diff --git a/internal/index/indexoperations/index_test.go b/internal/index/indexoperations/index_test.go
@@ -181,6 +181,11 @@ func TestIsValidIndexName(t *testing.T) {
 			index: "foo/bar",
 			want:  false,
 		},
+		{
+			name:  "relative path",
+			index: "../foo",
+			want:  false,
+		},
 		{
 			name:  "with back slash",
 			index: "foo\\bar",

diff --git a/internal/index/indexscanner/scanner.go b/internal/index/indexscanner/scanner.go
@@ -75,10 +75,6 @@ func LoadPluginListFromFS(indexDir string) ([]index.Plugin, error) {
 // LoadPluginByName loads a plugins index file by its name. When plugin
 // file not found, it returns an error that can be checked with os.IsNotExist.
 func LoadPluginByName(pluginsDir, pluginName string) (index.Plugin, error) {
-	if !validation.IsSafePluginName(pluginName) {
-		return index.Plugin{}, errors.Errorf("plugin name %q not allowed", pluginName)
-	}
-
 	klog.V(4).Infof("Reading plugin %q from %s", pluginName, pluginsDir)
 	return ReadPluginFromFile(filepath.Join(pluginsDir, pluginName+constants.ManifestExtension))
 }