From a24e19fea31f389fd59a5b26c77a9ddfa359783f Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 18 Jun 2024 10:42:14 +0200 Subject: [PATCH 1/2] chore: backport #4992 to release/v7.6.x --- .../controller/types/msgs.go | 15 ++++++++++++++- .../controller/types/msgs_test.go | 14 ++++++++++++++ .../27-interchain-accounts/types/account_test.go | 2 +- modules/apps/29-fee/types/msgs.go | 6 ++++++ modules/apps/29-fee/types/msgs_test.go | 7 +++++++ modules/apps/transfer/types/errors.go | 1 + modules/apps/transfer/types/msgs.go | 11 +++++++++++ modules/apps/transfer/types/msgs_test.go | 3 +++ testing/utils.go | 10 ++++++++++ testing/values.go | 3 ++- 10 files changed, 69 insertions(+), 3 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs.go b/modules/apps/27-interchain-accounts/controller/types/msgs.go index 3516ac15dc2..5ecb10146f6 100644 --- a/modules/apps/27-interchain-accounts/controller/types/msgs.go +++ b/modules/apps/27-interchain-accounts/controller/types/msgs.go @@ -11,7 +11,12 @@ import ( host "github.com/cosmos/ibc-go/v7/modules/core/24-host" ) -var _ sdk.Msg = &MsgRegisterInterchainAccount{} +const MaximumOwnerLength = 2048 // maximum length of the owner in bytes (value chosen arbitrarily) + +var ( + _ sdk.Msg = (*MsgRegisterInterchainAccount)(nil) + _ sdk.Msg = (*MsgSendTx)(nil) +) // NewMsgRegisterInterchainAccountWithOrdering creates a new instance of MsgRegisterInterchainAccount. func NewMsgRegisterInterchainAccountWithOrdering(connectionID, owner, version string, ordering channeltypes.Order) *MsgRegisterInterchainAccount { @@ -45,6 +50,10 @@ func (msg MsgRegisterInterchainAccount) ValidateBasic() error { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "owner address cannot be empty") } + if len(msg.Owner) > MaximumOwnerLength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "owner address must not exceed %d bytes", MaximumOwnerLength) + } + return nil } @@ -78,6 +87,10 @@ func (msg MsgSendTx) ValidateBasic() error { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "owner address cannot be empty") } + if len(msg.Owner) > MaximumOwnerLength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "owner address must not exceed %d bytes", MaximumOwnerLength) + } + if err := msg.PacketData.ValidateBasic(); err != nil { return sdkerrors.Wrap(err, "invalid interchain account packet data") } diff --git a/modules/apps/27-interchain-accounts/controller/types/msgs_test.go b/modules/apps/27-interchain-accounts/controller/types/msgs_test.go index e5afa00c93e..e25519eeeb2 100644 --- a/modules/apps/27-interchain-accounts/controller/types/msgs_test.go +++ b/modules/apps/27-interchain-accounts/controller/types/msgs_test.go @@ -62,6 +62,13 @@ func TestMsgRegisterInterchainAccountValidateBasic(t *testing.T) { }, false, }, + { + "owner address is too long", + func() { + msg.Owner = ibctesting.GenerateString(types.MaximumOwnerLength + 1) + }, + false, + }, } for i, tc := range testCases { @@ -118,6 +125,13 @@ func TestMsgSendTxValidateBasic(t *testing.T) { }, false, }, + { + "owner address is too long", + func() { + msg.Owner = ibctesting.GenerateString(types.MaximumOwnerLength + 1) + }, + false, + }, { "relative timeout is not set", func() { diff --git a/modules/apps/27-interchain-accounts/types/account_test.go b/modules/apps/27-interchain-accounts/types/account_test.go index 8acef7e33a9..96ba6930ce5 100644 --- a/modules/apps/27-interchain-accounts/types/account_test.go +++ b/modules/apps/27-interchain-accounts/types/account_test.go @@ -78,7 +78,7 @@ func (suite *TypesTestSuite) TestValidateAccountAddress() { }, { "address is too long", - ibctesting.LongString, + ibctesting.GenerateString(uint(types.DefaultMaxAddrLength) + 1), false, }, } diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go index c44f14925c4..6f06c9afc54 100644 --- a/modules/apps/29-fee/types/msgs.go +++ b/modules/apps/29-fee/types/msgs.go @@ -17,6 +17,8 @@ const ( TypeMsgPayPacketFeeAsync = "payPacketFeeAsync" ) +const MaximumCounterpartyPayeeLength = 2048 // maximum length of the counterparty payee in bytes (value chosen arbitrarily) + var ( _ sdk.Msg = (*MsgRegisterPayee)(nil) _ sdk.Msg = (*MsgRegisterCounterpartyPayee)(nil) @@ -102,6 +104,10 @@ func (msg MsgRegisterCounterpartyPayee) ValidateBasic() error { return ErrCounterpartyPayeeEmpty } + if len(msg.CounterpartyPayee) > MaximumCounterpartyPayeeLength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "counterparty payee address must not exceed %d bytes", MaximumCounterpartyPayeeLength) + } + return nil } diff --git a/modules/apps/29-fee/types/msgs_test.go b/modules/apps/29-fee/types/msgs_test.go index e20984e228b..bbeb9b377f5 100644 --- a/modules/apps/29-fee/types/msgs_test.go +++ b/modules/apps/29-fee/types/msgs_test.go @@ -135,6 +135,13 @@ func TestMsgRegisterCountepartyPayeeValidation(t *testing.T) { }, false, }, + { + "invalid counterparty payee address: too long", + func() { + msg.CounterpartyPayee = ibctesting.GenerateString(types.MaximumCounterpartyPayeeLength + 1) + }, + false, + }, } for i, tc := range testCases { diff --git a/modules/apps/transfer/types/errors.go b/modules/apps/transfer/types/errors.go index d4f85cf4fa7..c2e1d2141b1 100644 --- a/modules/apps/transfer/types/errors.go +++ b/modules/apps/transfer/types/errors.go @@ -15,4 +15,5 @@ var ( ErrReceiveDisabled = sdkerrors.Register(ModuleName, 8, "fungible token transfers to this chain are disabled") ErrMaxTransferChannels = sdkerrors.Register(ModuleName, 9, "max transfer channels") ErrInvalidAuthorization = sdkerrors.Register(ModuleName, 10, "invalid transfer authorization") + ErrInvalidMemo = sdkerrors.Register(ModuleName, 11, "invalid memo") ) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index eaf0046b798..289c3eb617a 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -16,6 +16,11 @@ const ( TypeMsgTransfer = "transfer" ) +const ( + MaximumReceiverLength = 2048 // maximum length of the receiver address in bytes (value chosen arbitrarily) + MaximumMemoLength = 32768 // maximum length of the memo in bytes (value chosen arbitrarily) +) + var ( _ sdk.Msg = (*MsgTransfer)(nil) _ legacytx.LegacyMsg = (*MsgTransfer)(nil) @@ -77,6 +82,12 @@ func (msg MsgTransfer) ValidateBasic() error { if strings.TrimSpace(msg.Receiver) == "" { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing recipient address") } + if len(msg.Receiver) > MaximumReceiverLength { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "recipient address must not exceed %d bytes", MaximumReceiverLength) + } + if len(msg.Memo) > MaximumMemoLength { + return sdkerrors.Wrapf(ErrInvalidMemo, "memo must not exceed %d bytes", MaximumMemoLength) + } return ValidateIBCDenom(msg.Token.Denom) } diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 4cb7700697c..be11a9e0df1 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -10,6 +10,7 @@ import ( "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" + ibctesting "github.com/cosmos/ibc-go/v7/testing" ) // define constants used for testing @@ -71,11 +72,13 @@ func TestMsgTransferValidation(t *testing.T) { {"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, {"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, {"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, + {"too long memo", types.NewMsgTransfer(validPort, validChannel, coin, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1)), false}, {"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coin, sender, receiver, timeoutHeight, 0, ""), false}, {"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoin, sender, receiver, timeoutHeight, 0, ""), false}, {"zero coin", types.NewMsgTransfer(validPort, validChannel, zeroCoin, sender, receiver, timeoutHeight, 0, ""), false}, {"missing sender address", types.NewMsgTransfer(validPort, validChannel, coin, emptyAddr, receiver, timeoutHeight, 0, ""), false}, {"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coin, sender, "", timeoutHeight, 0, ""), false}, + {"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coin, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, ""), false}, {"empty coin", types.NewMsgTransfer(validPort, validChannel, sdk.Coin{}, sender, receiver, timeoutHeight, 0, ""), false}, } diff --git a/testing/utils.go b/testing/utils.go index f0091d8e211..38063fd2907 100644 --- a/testing/utils.go +++ b/testing/utils.go @@ -1,6 +1,7 @@ package ibctesting import ( + "math/rand" "testing" abci "github.com/cometbft/cometbft/abci/types" @@ -21,3 +22,12 @@ func ApplyValSetChanges(t *testing.T, valSet *tmtypes.ValidatorSet, valUpdates [ return newVals } + +// GenerateString generates a random string of the given length in bytes +func GenerateString(length uint) string { + bytes := make([]byte, length) + for i := range bytes { + bytes[i] = charset[rand.Intn(len(charset))] + } + return string(bytes) +} diff --git a/testing/values.go b/testing/values.go index fa69c502069..f45d7eefc07 100644 --- a/testing/values.go +++ b/testing/values.go @@ -40,7 +40,8 @@ const ( Title = "title" Description = "description" - LongString = "LoremipsumdolorsitameconsecteturadipiscingeliseddoeiusmodtemporincididuntutlaboreetdoloremagnaaliquUtenimadminimveniamquisnostrudexercitationullamcolaborisnisiutaliquipexeacommodoconsequDuisauteiruredolorinreprehenderitinvoluptateelitsseillumoloreufugiatnullaariaturEcepteurintoccaectupidatatonroidentuntnulpauifficiaeseruntmollitanimidestlaborum" + // character set used for generating a random string in GenerateString + charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" ) var ( From 4c9e0e7147d4dbe742ca6f0d8c46295931ab8857 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 18 Jun 2024 10:49:47 +0200 Subject: [PATCH 2/2] changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9014fa2b95d..fd003b62d9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,9 +42,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking -### Improvements +* (apps/transfer, apps/27-interchain-accounts, app/29-fee) #4992 Set validation for length of string fields. -* (apps/27-interchain-accounts) [\#6436](https://github.com/cosmos/ibc-go/pull/6436) Refactor ICA host keeper instantiation method to avoid panic related to proto files. +### Improvements ### Features