Merge #1376

1376: Fix encoding user input in failed transaction message r=m4ksio a=m4ksio



Co-authored-by: Maks Pawlak <120831+m4ksio@users.noreply.github.com>

fvm/handler/accountKey.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"encoding/hex"
 	"fmt"
 
 	"github.com/onflow/cadence/runtime"
@@ -244,8 +245,9 @@ func (h *AccountKeyHandler) GetAccountKey(address runtime.Address, keyIndex int)
 
 // AddEncodedAccountKey adds an encoded public key to an existing account.
 //
-// This function returns an error if the specified account does not exist or
-// if the key insertion fails.
+// This function returns following error
+// * NewAccountNotFoundError - if the specified account does not exist
+// * ValueError - if the provided encodedPublicKey is not valid public key
 func (e *AccountKeyHandler) AddEncodedAccountKey(address runtime.Address, encodedPublicKey []byte) (err error) {
 	accountAddress := flow.Address(address)
 
@@ -262,7 +264,8 @@ func (e *AccountKeyHandler) AddEncodedAccountKey(address runtime.Address, encode
 
 	publicKey, err = flow.DecodeRuntimeAccountPublicKey(encodedPublicKey, 0)
 	if err != nil {
-		err = errors.NewValueErrorf(string(encodedPublicKey), "invalid encoded public key value: %w", err)
+		hexEncodedPublicKey := hex.EncodeToString(encodedPublicKey)
+		err = errors.NewValueErrorf(hexEncodedPublicKey, "invalid encoded public key value: %w", err)
 		return fmt.Errorf("adding encoded account key failed: %w", err)
 	}
 

fvm/handler/accountKey_test.go

@@ -0,0 +1,108 @@
+package handler
+
+import (
+	errors2 "errors"
+	"testing"
+	"unicode/utf8"
+
+	"github.com/fxamacker/cbor/v2"
+	"github.com/onflow/cadence"
+	"github.com/onflow/cadence/runtime"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/onflow/flow-go/fvm/errors"
+
+	"github.com/onflow/flow-go/crypto"
+	"github.com/onflow/flow-go/crypto/hash"
+	"github.com/onflow/flow-go/model/flow"
+)
+
+func TestAddEncodedAccountKey_error_handling_produces_valid_utf8(t *testing.T) {
+
+	akh := &AccountKeyHandler{accounts: FakeAccounts{}}
+
+	address := cadence.BytesToAddress([]byte{1, 2, 3, 4})
+
+	// emulate encoded public key (which comes as a user input)
+	// containing bytes which are invalid UTF8
+
+	invalidEncodedKey := make([]byte, 64)
+	invalidUTF8 := []byte{0xc3, 0x28}
+	copy(invalidUTF8, invalidEncodedKey)
+	publicKey := FakePublicKey{data: invalidEncodedKey}
+
+	accountPublicKey := flow.AccountPublicKey{
+		Index:     1,
+		PublicKey: publicKey,
+		SignAlgo:  crypto.ECDSASecp256k1,
+		HashAlgo:  hash.SHA3_256,
+		SeqNumber: 0,
+		Weight:    1000,
+		Revoked:   false,
+	}
+
+	encodedPublicKey, err := flow.EncodeRuntimeAccountPublicKey(accountPublicKey)
+	require.NoError(t, err)
+
+	err = akh.AddEncodedAccountKey(runtime.Address(address), encodedPublicKey)
+	require.Error(t, err)
+
+	err = errors2.Unwrap(err)
+	require.IsType(t, &errors.ValueError{}, err)
+
+	errorString := err.Error()
+	assert.True(t, utf8.ValidString(errorString))
+
+	// check if they can encoded and decoded using CBOR
+	marshalledBytes, err := cbor.Marshal(errorString)
+	require.NoError(t, err)
+
+	var unmarshalledString string
+
+	err = cbor.Unmarshal(marshalledBytes, &unmarshalledString)
+	require.NoError(t, err)
+
+	require.Equal(t, errorString, unmarshalledString)
+}
+
+type FakePublicKey struct {
+	data []byte
+}
+
+func (f FakePublicKey) Encode() []byte {
+	return f.data
+}
+
+func (f FakePublicKey) Algorithm() crypto.SigningAlgorithm { return crypto.ECDSASecp256k1 }
+func (f FakePublicKey) Size() int                          { return 0 }
+func (f FakePublicKey) String() string                     { return "" }
+func (f FakePublicKey) Verify(_ crypto.Signature, _ []byte, _ hash.Hasher) (bool, error) {
+	return false, nil
+}
+func (f FakePublicKey) EncodeCompressed() []byte         { return nil }
+func (f FakePublicKey) Equals(key crypto.PublicKey) bool { return false }
+
+type FakeAccounts struct{}
+
+func (f FakeAccounts) Exists(address flow.Address) (bool, error) { return true, nil }
+
+func (f FakeAccounts) GetPublicKeyCount(_ flow.Address) (uint64, error)              { return 0, nil }
+func (f FakeAccounts) AppendPublicKey(_ flow.Address, _ flow.AccountPublicKey) error { return nil }
+func (f FakeAccounts) GetPublicKey(_ flow.Address, _ uint64) (flow.AccountPublicKey, error) {
+	return flow.AccountPublicKey{}, nil
+}
+func (f FakeAccounts) SetPublicKey(_ flow.Address, _ uint64, _ flow.AccountPublicKey) ([]byte, error) {
+	return nil, nil
+}
+func (f FakeAccounts) GetContractNames(_ flow.Address) ([]string, error)             { return nil, nil }
+func (f FakeAccounts) GetContract(_ string, _ flow.Address) ([]byte, error)          { return nil, nil }
+func (f FakeAccounts) SetContract(_ string, _ flow.Address, _ []byte) error          { return nil }
+func (f FakeAccounts) DeleteContract(_ string, _ flow.Address) error                 { return nil }
+func (f FakeAccounts) Create(_ []flow.AccountPublicKey, _ flow.Address) error        { return nil }
+func (f FakeAccounts) GetValue(_ flow.Address, _ string) (flow.RegisterValue, error) { return nil, nil }
+func (f FakeAccounts) CheckAccountNotFrozen(_ flow.Address) error                    { return nil }
+func (f FakeAccounts) GetStorageUsed(_ flow.Address) (uint64, error)                 { return 0, nil }
+func (f FakeAccounts) SetValue(_ flow.Address, _ string, _ []byte) error             { return nil }
+func (f FakeAccounts) AllocateStorageIndex(_ flow.Address) (uint64, error)           { return 0, nil }
+func (f FakeAccounts) SetAccountFrozen(_ flow.Address, _ bool) error                 { return nil }