Improve address and shardID validation for the wallet (#1702)

* - Improve validation of sender and receiving addresses
- Improve validation of sender and receiving shardIDs
- Run tests using go test cmd/client/wallet_validation/validation_test.go

* - Fix linting issues
- Fix test issue
- Move from wallet_validation to validation

* Update test:
- The local profile might eventually get updated to include more than 2 shards, in that case the test will fail. Use obviously invalid values (-1 / 99) for now.

* - Fix order of imports (std should come first)
- Fix typo

* Fix goimports linting

* Fix go import linting (again...)

* Fix go import linting

* Fix goimport linting

* Fix go import linting (sigh...)

* Fix goimports formatting using go fmt

* Update validation checking, also validate addresses supplied (--address) to:
- getFreeToken
- format
- balances

* Merge remote master into branch

* - Move wallet validation to cmd/client/wallet/main.go
- Move validation test to cmd/client/wallet/validation_test.go

* Refactor validateAddress

cmd/client/wallet/main.go

@@ -10,14 +10,15 @@ import (
 	"math/rand"
 	"os"
 	"path"
+	"regexp"
 	"sync"
 	"time"
 
-	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/fatih/color"
 	ffi_bls "github.com/harmony-one/bls/ffi/go/bls"
 
+	"github.com/ethereum/go-ethereum/common"
 	"github.com/harmony-one/harmony/accounts"
 	"github.com/harmony-one/harmony/accounts/keystore"
 	"github.com/harmony-one/harmony/api/client"
@@ -520,7 +521,6 @@ func showAllBalances(sender, receiver string, fromS, toS int) {
 			}
 		}
 	}
-
 }
 
 func processBalancesCommand() {
@@ -532,6 +532,13 @@ func processBalancesCommand() {
 		showAllBalances("", "", -1, -1)
 	} else {
 		address := common2.ParseAddr(*balanceAddressPtr)
+		valid, errorMessage := validateAddress(*balanceAddressPtr, address, "")
+
+		if !valid && len(errorMessage) > 0 {
+			fmt.Println(errorMessage)
+			return
+		}
+
 		fmt.Printf("Account: %s:\n", common2.MustAddressToBech32(address))
 		for shardID, balanceNonce := range FetchBalance(address) {
 			if balanceNonce != nil {
@@ -553,6 +560,12 @@ func formatAddressCommand() {
 		fmt.Println("Please specify the --address to show formats for.")
 	} else {
 		address := common2.ParseAddr(*formatAddressPtr)
+		valid, errorMessage := validateAddress(*formatAddressPtr, address, "")
+
+		if !valid && len(errorMessage) > 0 {
+			fmt.Println(errorMessage)
+			return
+		}
 
 		fmt.Printf("account address in Bech32: %s\n", common2.MustAddressToBech32(address))
 		fmt.Printf("account address in Base16 (deprecated): %s\n", address.Hex())
@@ -665,6 +678,13 @@ func processGetFreeToken() {
 		fmt.Println("Error: --address is required")
 	} else {
 		address := common2.ParseAddr(*freeTokenAddressPtr)
+		valid, errorMessage := validateAddress(*freeTokenAddressPtr, address, "")
+
+		if !valid && len(errorMessage) > 0 {
+			fmt.Println(errorMessage)
+			return
+		}
+
 		GetFreeToken(address)
 	}
 }
@@ -690,24 +710,34 @@ func processTransferCommand() {
 		return
 	}
 
-	if shardID == -1 || toShardID == -1 {
-		fmt.Println("Please specify the shard ID for the transfer (e.g. --shardID=0)")
+	if !validShard(shardID, walletProfile.Shards) {
+		fmt.Println("Please specify a valid sender shard ID for the transfer (e.g. --shardID=0)")
+		return
+	}
+
+	if !validShard(toShardID, walletProfile.Shards) {
+		fmt.Println("Please specify a valid receiver shard ID for the transfer (e.g. --toShardID=0)")
 		return
 	}
+
 	if amount <= 0 {
 		fmt.Println("Please specify positive amount to transfer")
 		return
 	}
 
-	receiverAddress := common2.ParseAddr(receiver)
-	if len(receiverAddress) != 20 {
-		fmt.Println("The receiver address is not valid.")
+	senderAddress := common2.ParseAddr(sender)
+	valid, errorMessage := validateAddress(sender, senderAddress, "sender")
+
+	if !valid && len(errorMessage) > 0 {
+		fmt.Println(errorMessage)
 		return
 	}
 
-	senderAddress := common2.ParseAddr(sender)
-	if len(senderAddress) != 20 {
-		fmt.Println("The sender address is not valid.")
+	receiverAddress := common2.ParseAddr(receiver)
+	valid, errorMessage = validateAddress(receiver, receiverAddress, "receiver")
+
+	if !valid && len(errorMessage) > 0 {
+		fmt.Println(errorMessage)
 		return
 	}
 
@@ -956,3 +986,32 @@ func submitTransaction(tx *types.Transaction, walletNode *node.Node, shardID uin
 
 	return nil
 }
+
+var (
+	addressValidationRegexp = regexp.MustCompile(`(?i)^(one[a-zA-Z0-9]{39})|(0x[a-fA-F0-9]{40})`)
+)
+
+func validateAddress(address string, commonAddress common.Address, addressType string) (bool, string) {
+	var valid = true
+	var errorMessage string
+
+	if len(addressType) > 0 {
+		addressType = fmt.Sprintf("%s ", addressType)
+	}
+
+	matches := addressValidationRegexp.FindAllStringSubmatch(address, -1)
+	if len(matches) == 0 || len(commonAddress) != 20 {
+		valid = false
+		errorMessage = fmt.Sprintf("The %saddress you supplied (%s) is in an invalid format. Please provide a valid address.", addressType, address)
+	}
+
+	return valid, errorMessage
+}
+
+func validShard(shardID int, shardCount int) bool {
+	if shardID < 0 || shardID > (shardCount-1) {
+		return false
+	}
+
+	return true
+}

cmd/client/wallet/validation_test.go

@@ -0,0 +1,51 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/harmony-one/harmony/internal/common"
+)
+
+func TestIsValidAddress(t *testing.T) {
+	tests := []struct {
+		str string
+		exp bool
+	}{
+		{"one1ay37rp2pc3kjarg7a322vu3sa8j9puahg679z3", true},
+		{"0x7c41E0668B551f4f902cFaec05B5Bdca68b124CE", true},
+		{"onefoofoo", false},
+		{"0xbarbar", false},
+		{"dsasdadsasaadsas", false},
+		{"32312123213213212321", false},
+	}
+
+	for _, test := range tests {
+		valid, _ := validateAddress(test.str, common.ParseAddr(test.str), "sender")
+
+		if valid != test.exp {
+			t.Errorf("validateAddress(\"%s\") returned %v, expected %v", test.str, valid, test.exp)
+		}
+	}
+}
+
+func TestIsValidShard(t *testing.T) {
+	readProfile("local")
+
+	tests := []struct {
+		shardID int
+		exp     bool
+	}{
+		{0, true},
+		{1, true},
+		{-1, false},
+		{99, false},
+	}
+
+	for _, test := range tests {
+		valid := validShard(test.shardID, walletProfile.Shards)
+
+		if valid != test.exp {
+			t.Errorf("validShard(%d) returned %v, expected %v", test.shardID, valid, test.exp)
+		}
+	}
+}

go.mod

@@ -62,6 +62,7 @@ require (
 	golang.org/x/lint v0.0.0-20190409202823-959b441ac422
 	golang.org/x/sys v0.0.0-20190801041406-cbf593c0f2f3 // indirect
 	golang.org/x/tools v0.0.0-20190924052046-3ac2a5bbd98a
+	google.golang.org/appengine v1.4.0 // indirect
 	google.golang.org/grpc v1.22.0
 	gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127
 	gopkg.in/ini.v1 v1.42.0