refactor(client): check name validation for keys add|import|rename (backport #18950) (#24152)

Co-authored-by: levisyin <150114626+levisyin@users.noreply.github.com>
Co-authored-by: aljo242 <alex@interchainlabs.io>

CHANGELOG.md

@@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
+* (client/keys) [#18950](https://github.com/cosmos/cosmos-sdk/pull/18950) Improve `<appd> keys add`, `<appd> keys import` and `<appd> keys rename` by checking name validation.
 * (client/keys) [#18703](https://github.com/cosmos/cosmos-sdk/pull/18703) Improve `<appd> keys add` and `<appd> keys show` by checking whether there are duplicate keys in the multisig case.
 * (client/keys) [#18745](https://github.com/cosmos/cosmos-sdk/pull/18745) Improve `<appd> keys export` and `<appd> keys mnemonic` by adding --yes option to skip interactive confirmation.
 * (x/bank) [#24106](https://github.com/cosmos/cosmos-sdk/pull/24106) `SendCoins` now checks for `SendRestrictions` before instead of after deducting coins using `subUnlockedCoins`.

client/keys/add.go

@@ -10,6 +10,7 @@ import (
 	"io"
 	"os"
 	"sort"
+	"strings"
 
 	"github.com/cosmos/go-bip39"
 	"github.com/spf13/cobra"
@@ -115,6 +116,14 @@ func runAddCmdPrepare(cmd *cobra.Command, args []string) error {
 	return runAddCmd(clientCtx, cmd, args, buf)
 }
 
+func checkName(name string) error {
+	if strings.TrimSpace(name) == "" {
+		return errors.New("the provided name is invalid or empty after trimming whitespace")
+	}
+
+	return nil
+}
+
 /*
 input
   - bip39 mnemonic
@@ -129,6 +138,9 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf
 	var err error
 
 	name := args[0]
+	if err = checkName(name); err != nil {
+		return err
+	}
 	interactive, _ := cmd.Flags().GetBool(flagInteractive)
 	noBackup, _ := cmd.Flags().GetBool(flagNoBackup)
 	showMnemonic := !noBackup

client/keys/add_test.go

@@ -39,6 +39,17 @@ func Test_runAddCmdBasic(t *testing.T) {
 		_ = kb.Delete("keyname2")
 	})
 
+	// test empty name
+	cmd.SetArgs([]string{
+		"",
+		fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome),
+		fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText),
+		fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type),
+		fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+	})
+	mockIn.Reset("y\n")
+	require.ErrorContains(t, cmd.ExecuteContext(ctx), "the provided name is invalid or empty after trimming whitespace")
+
 	cmd.SetArgs([]string{
 		"keyname1",
 		fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome),

client/keys/import.go

@@ -2,10 +2,8 @@ package keys
 
 import (
 	"bufio"
-	"errors"
 	"fmt"
 	"os"
-	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -29,8 +27,8 @@ func ImportKeyCommand() *cobra.Command {
 				return err
 			}
 			name := args[0]
-			if strings.TrimSpace(name) == "" {
-				return errors.New("the provided name is invalid or empty after trimming whitespace")
+			if err := checkName(name); err != nil {
+				return err
 			}
 			buf := bufio.NewReader(clientCtx.Input)
 
@@ -61,8 +59,8 @@ func ImportKeyHexCommand() *cobra.Command {
 				return err
 			}
 			name := args[0]
-			if strings.TrimSpace(name) == "" {
-				return errors.New("the provided name is invalid or empty after trimming whitespace")
+			if err := checkName(name); err != nil {
+				return err
 			}
 
 			keyType, _ := cmd.Flags().GetString(flags.FlagKeyType)

client/keys/import_test.go

@@ -189,3 +189,37 @@ func Test_runImportHexCmd(t *testing.T) {
 		})
 	}
 }
+
+func Test_runImportCmdWithEmptyName(t *testing.T) {
+	cmd := ImportKeyCommand()
+	cmd.Flags().AddFlagSet(Commands().PersistentFlags())
+	mockIn := testutil.ApplyMockIODiscardOutErr(cmd)
+	// Now add a temporary keybase
+	kbHome := t.TempDir()
+	cdc := moduletestutil.MakeTestEncodingConfig().Codec
+	kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn, cdc)
+	require.NoError(t, err)
+
+	clientCtx := client.Context{}.
+		WithKeyringDir(kbHome).
+		WithKeyring(kb).
+		WithInput(mockIn).
+		WithCodec(cdc)
+	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
+	cmd.SetArgs([]string{
+		"", "fake-file",
+		fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+	})
+
+	require.ErrorContains(t, cmd.ExecuteContext(ctx), "the provided name is invalid or empty after trimming whitespace")
+
+	cmd = ImportKeyHexCommand()
+	cmd.Flags().AddFlagSet(Commands().PersistentFlags())
+	testutil.ApplyMockIODiscardOutErr(cmd)
+	cmd.SetArgs([]string{
+		"", "fake-hex",
+		fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest),
+	})
+
+	require.ErrorContains(t, cmd.ExecuteContext(ctx), "the provided name is invalid or empty after trimming whitespace")
+}

client/keys/rename.go

@@ -2,7 +2,9 @@ package keys
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -31,6 +33,9 @@ private keys stored in a ledger device cannot be renamed with the CLI.
 			}
 
 			oldName, newName := args[0], args[1]
+			if strings.TrimSpace(newName) == "" {
+				return errors.New("the new name cannot be empty or consist solely of whitespace")
+			}
 
 			k, err := clientCtx.Keyring.Key(oldName)
 			if err != nil {

client/keys/rename_test.go

@@ -27,6 +27,7 @@ func Test_runRenameCmd(t *testing.T) {
 	yesF, _ := cmd.Flags().GetBool(flagYes)
 	require.False(t, yesF)
 
+	invalidName := ""
 	fakeKeyName1 := "runRenameCmd_Key1"
 	fakeKeyName2 := "runRenameCmd_Key2"
 
@@ -46,6 +47,9 @@ func Test_runRenameCmd(t *testing.T) {
 
 	ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)
 
+	cmd.SetArgs([]string{fakeKeyName1, invalidName, fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome)})
+	require.ErrorContains(t, cmd.ExecuteContext(ctx), "the new name cannot be empty or consist solely of whitespace")
+
 	// rename a key 'blah' which doesnt exist
 	cmd.SetArgs([]string{"blah", "blaah", fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome)})
 	err = cmd.ExecuteContext(ctx)