Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: backport #4992 to release/v7.6.x #6625

Merged
merged 3 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this because this will be released in v7.5.1 first.

### Improvements

### Features

Expand Down
15 changes: 14 additions & 1 deletion modules/apps/27-interchain-accounts/controller/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func (suite *TypesTestSuite) TestValidateAccountAddress() {
},
{
"address is too long",
ibctesting.LongString,
ibctesting.GenerateString(uint(types.DefaultMaxAddrLength) + 1),
false,
},
}
Expand Down
6 changes: 6 additions & 0 deletions modules/apps/29-fee/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}

Expand Down
7 changes: 7 additions & 0 deletions modules/apps/29-fee/types/msgs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions modules/apps/transfer/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
11 changes: 11 additions & 0 deletions modules/apps/transfer/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

Expand Down
3 changes: 3 additions & 0 deletions modules/apps/transfer/types/msgs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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},
}

Expand Down
10 changes: 10 additions & 0 deletions testing/utils.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ibctesting

import (
"math/rand"
"testing"

abci "github.com/cometbft/cometbft/abci/types"
Expand All @@ -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)
}
3 changes: 2 additions & 1 deletion testing/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ const (
Title = "title"
Description = "description"

LongString = "LoremipsumdolorsitameconsecteturadipiscingeliseddoeiusmodtemporincididuntutlaboreetdoloremagnaaliquUtenimadminimveniamquisnostrudexercitationullamcolaborisnisiutaliquipexeacommodoconsequDuisauteiruredolorinreprehenderitinvoluptateelitsseillumoloreufugiatnullaariaturEcepteurintoccaectupidatatonroidentuntnulpauifficiaeseruntmollitanimidestlaborum"
// character set used for generating a random string in GenerateString
charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
)

var (
Expand Down
Loading