Skip to content

Commit 7574333

Browse files
srdtrkaljo242
srdtrk
and
aljo242
authored
imp: add extra validation to ica msgs (#8734)
* imp: add extra validation * chore: lint * imp: error msg * test: fix tests * docs: document the validation change --------- Co-authored-by: Alex | Cosmos Labs <alex@cosmoslabs.io>
1 parent 8e48512 commit 7574333

File tree

3 files changed

+159
-19
lines changed

3 files changed

+159
-19
lines changed

‎docs/docs/02-apps/02-interchain-accounts/07-tx-encoding.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,7 @@ The proto3 JSON encoding presents an alternative encoding technique for `CosmosT
5656
```
5757

5858
Here, the `"messages"` array is populated with transactions. Each transaction is represented as a JSON object with the `@type` field denoting the transaction type and the remaining fields representing the transaction's attributes.
59+
60+
:::warning
61+
When utilizing proto3 JSON encoding, we have extra validations to ensure the integrity of the encoded data. Specifically, after decoding the `CosmosTx` from JSON, we re-encode it back to JSON and compare it with the original input. If non-formatting discrepancies arise, an error is returned. This validation step ensures that the JSON representation remains consistent throughout the encoding and decoding processes. This additional check means that all optional fields must be explicitly set in the JSON input, all enums and integers must be represented as strings.
62+
:::

‎modules/apps/27-interchain-accounts/host/keeper/relay_test.go‎

Lines changed: 132 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -566,7 +566,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
566566
expErr error
567567
}{
568568
{
569-
"interchain account successfully executes an arbitrary message type using the * (allow all message types) param",
569+
"success: interchain account successfully executes an arbitrary message type using the * (allow all message types) param",
570570
func(icaAddress string) {
571571
proposal, err := govtypesv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govtypesv1.DefaultStartingProposalID, s.chainA.GetContext().BlockTime(), s.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
572572
s.Require().NoError(err)
@@ -581,8 +581,9 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
581581
{
582582
"@type": "/cosmos.gov.v1.MsgVote",
583583
"voter": "` + icaAddress + `",
584-
"proposal_id": 1,
585-
"option": 1
584+
"proposal_id": "1",
585+
"option": "VOTE_OPTION_YES",
586+
"metadata": ""
586587
}
587588
]
588589
}`)
@@ -601,7 +602,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
601602
nil,
602603
},
603604
{
604-
"interchain account successfully executes banktypes.MsgSend",
605+
"success: interchain account successfully executes banktypes.MsgSend",
605606
func(icaAddress string) {
606607
msgBytes := []byte(`{
607608
"messages": [
@@ -626,7 +627,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
626627
nil,
627628
},
628629
{
629-
"interchain account successfully executes govtypesv1.MsgSubmitProposal",
630+
"success: interchain account successfully executes govtypesv1.MsgSubmitProposal",
630631
func(icaAddress string) {
631632
msgBytes := []byte(`{
632633
"messages": [
@@ -655,7 +656,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
655656
nil,
656657
},
657658
{
658-
"interchain account successfully executes govtypesv1.MsgVote",
659+
"success: interchain account successfully executes govtypesv1.MsgVote",
659660
func(icaAddress string) {
660661
proposal, err := govtypesv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govtypesv1.DefaultStartingProposalID, s.chainA.GetContext().BlockTime(), s.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
661662
s.Require().NoError(err)
@@ -670,8 +671,9 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
670671
{
671672
"@type": "/cosmos.gov.v1.MsgVote",
672673
"voter": "` + icaAddress + `",
673-
"proposal_id": 1,
674-
"option": 1
674+
"proposal_id": "1",
675+
"option": "VOTE_OPTION_YES",
676+
"metadata": ""
675677
}
676678
]
677679
}`)
@@ -688,7 +690,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
688690
nil,
689691
},
690692
{
691-
"interchain account successfully executes govtypesv1.MsgSubmitProposal, govtypesv1.MsgDeposit, and then govtypesv1.MsgVote sequentially",
693+
"success: interchain account successfully executes govtypesv1.MsgSubmitProposal, govtypesv1.MsgDeposit, and then govtypesv1.MsgVote sequentially",
692694
func(icaAddress string) {
693695
msgBytes := []byte(`{
694696
"messages": [
@@ -704,15 +706,16 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
704706
},
705707
{
706708
"@type": "/cosmos.gov.v1.MsgDeposit",
707-
"proposal_id": 1,
709+
"proposal_id": "1",
708710
"depositor": "` + icaAddress + `",
709711
"amount": [{ "denom": "stake", "amount": "10000000" }]
710712
},
711713
{
712714
"@type": "/cosmos.gov.v1.MsgVote",
713715
"voter": "` + icaAddress + `",
714-
"proposal_id": 1,
715-
"option": 1
716+
"proposal_id": "1",
717+
"option": "VOTE_OPTION_YES",
718+
"metadata": ""
716719
}
717720
]
718721
}`)
@@ -729,7 +732,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
729732
nil,
730733
},
731734
{
732-
"interchain account successfully executes transfertypes.MsgTransfer",
735+
"success: interchain account successfully executes transfertypes.MsgTransfer",
733736
func(icaAddress string) {
734737
transferPath := ibctesting.NewTransferPath(s.chainB, s.chainC)
735738

@@ -744,9 +747,12 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
744747
"token": { "denom": "stake", "amount": "100" },
745748
"sender": "` + icaAddress + `",
746749
"receiver": "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk",
747-
"timeout_height": { "revision_number": 1, "revision_height": 100 },
748-
"timeout_timestamp": 0,
749-
"memo": ""
750+
"timeout_height": { "revision_number": "1", "revision_height": "100" },
751+
"timeout_timestamp": "0",
752+
"memo": "",
753+
"encoding": "",
754+
"use_aliasing": false
755+
750756
}
751757
]
752758
}`)
@@ -763,7 +769,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
763769
nil,
764770
},
765771
{
766-
"unregistered sdk.Msg",
772+
"failure: unregistered sdk.Msg",
767773
func(icaAddress string) {
768774
msgBytes := []byte(`{"messages":[{}]}`)
769775
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
@@ -779,7 +785,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
779785
ibcerrors.ErrInvalidType,
780786
},
781787
{
782-
"message type not allowed banktypes.MsgSend",
788+
"failure: message type not allowed banktypes.MsgSend",
783789
func(icaAddress string) {
784790
msgBytes := []byte(`{
785791
"messages": [
@@ -804,7 +810,7 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
804810
ibcerrors.ErrUnauthorized,
805811
},
806812
{
807-
"unauthorised: signer address is not the interchain account associated with the controller portID",
813+
"failure: signer address is not the interchain account associated with the controller portID",
808814
func(icaAddress string) {
809815
msgBytes := []byte(`{
810816
"messages": [
@@ -828,6 +834,113 @@ func (s *KeeperTestSuite) TestJSONOnRecvPacket() {
828834
},
829835
ibcerrors.ErrInvalidType,
830836
},
837+
{
838+
"failure: missing optional metadata field",
839+
func(icaAddress string) {
840+
proposal, err := govtypesv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govtypesv1.DefaultStartingProposalID, s.chainA.GetContext().BlockTime(), s.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
841+
s.Require().NoError(err)
842+
843+
err = s.chainB.GetSimApp().GovKeeper.SetProposal(s.chainB.GetContext(), proposal)
844+
s.Require().NoError(err)
845+
err = s.chainB.GetSimApp().GovKeeper.ActivateVotingPeriod(s.chainB.GetContext(), proposal)
846+
s.Require().NoError(err)
847+
848+
msgBytes := []byte(`{
849+
"messages": [
850+
{
851+
"@type": "/cosmos.gov.v1.MsgVote",
852+
"voter": "` + icaAddress + `",
853+
"proposal_id": "1",
854+
"option": "VOTE_OPTION_YES"
855+
}
856+
]
857+
}`)
858+
// this is the way cosmwasm encodes byte arrays by default
859+
// golang doesn't use this encoding by default, but it can still deserialize:
860+
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
861+
862+
packetData = []byte(`{
863+
"type": 1,
864+
"data":` + byteArrayString + `
865+
}`)
866+
867+
params := types.NewParams(true, []string{"*"})
868+
s.chainB.GetSimApp().ICAHostKeeper.SetParams(s.chainB.GetContext(), params)
869+
},
870+
ibcerrors.ErrInvalidType,
871+
},
872+
{
873+
"failure: alternative int representation of enum field",
874+
func(icaAddress string) {
875+
proposal, err := govtypesv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govtypesv1.DefaultStartingProposalID, s.chainA.GetContext().BlockTime(), s.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
876+
s.Require().NoError(err)
877+
878+
err = s.chainB.GetSimApp().GovKeeper.SetProposal(s.chainB.GetContext(), proposal)
879+
s.Require().NoError(err)
880+
err = s.chainB.GetSimApp().GovKeeper.ActivateVotingPeriod(s.chainB.GetContext(), proposal)
881+
s.Require().NoError(err)
882+
883+
msgBytes := []byte(`{
884+
"messages": [
885+
{
886+
"@type": "/cosmos.gov.v1.MsgVote",
887+
"voter": "` + icaAddress + `",
888+
"proposal_id": "1",
889+
"option": 1,
890+
"metadata": ""
891+
}
892+
]
893+
}`)
894+
// this is the way cosmwasm encodes byte arrays by default
895+
// golang doesn't use this encoding by default, but it can still deserialize:
896+
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
897+
898+
packetData = []byte(`{
899+
"type": 1,
900+
"data":` + byteArrayString + `
901+
}`)
902+
903+
params := types.NewParams(true, []string{"*"})
904+
s.chainB.GetSimApp().ICAHostKeeper.SetParams(s.chainB.GetContext(), params)
905+
},
906+
ibcerrors.ErrInvalidType,
907+
},
908+
{
909+
"failure: alternative integer field representation",
910+
func(icaAddress string) {
911+
proposal, err := govtypesv1.NewProposal([]sdk.Msg{getTestProposalMessage()}, govtypesv1.DefaultStartingProposalID, s.chainA.GetContext().BlockTime(), s.chainA.GetContext().BlockTime(), "test proposal", "title", "Description", sdk.AccAddress(interchainAccountAddr), false)
912+
s.Require().NoError(err)
913+
914+
err = s.chainB.GetSimApp().GovKeeper.SetProposal(s.chainB.GetContext(), proposal)
915+
s.Require().NoError(err)
916+
err = s.chainB.GetSimApp().GovKeeper.ActivateVotingPeriod(s.chainB.GetContext(), proposal)
917+
s.Require().NoError(err)
918+
919+
msgBytes := []byte(`{
920+
"messages": [
921+
{
922+
"@type": "/cosmos.gov.v1.MsgVote",
923+
"voter": "` + icaAddress + `",
924+
"proposal_id": 1,
925+
"option": "VOTE_OPTION_YES",
926+
"metadata": ""
927+
}
928+
]
929+
}`)
930+
// this is the way cosmwasm encodes byte arrays by default
931+
// golang doesn't use this encoding by default, but it can still deserialize:
932+
byteArrayString := strings.Join(strings.Fields(fmt.Sprint(msgBytes)), ",") //nolint:staticcheck
933+
934+
packetData = []byte(`{
935+
"type": 1,
936+
"data":` + byteArrayString + `
937+
}`)
938+
939+
params := types.NewParams(true, []string{"*"})
940+
s.chainB.GetSimApp().ICAHostKeeper.SetParams(s.chainB.GetContext(), params)
941+
},
942+
ibcerrors.ErrInvalidType,
943+
},
831944
}
832945

833946
for _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} {

‎modules/apps/27-interchain-accounts/types/codec.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package types
22

33
import (
4+
"encoding/json"
5+
"reflect"
6+
47
"github.com/cosmos/gogoproto/proto"
58

69
errorsmod "cosmossdk.io/errors"
@@ -92,6 +95,15 @@ func DeserializeCosmosTx(cdc codec.Codec, data []byte, encoding string) ([]sdk.M
9295
if err := cdc.UnmarshalJSON(data, &cosmosTx); err != nil {
9396
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal CosmosTx with proto3 json: %v", err)
9497
}
98+
reconstructedData, err := cdc.MarshalJSON(&cosmosTx)
99+
if err != nil {
100+
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot remarshal CosmosTx with proto3 json: %v", err)
101+
}
102+
if isEqual, err := equalJSON(data, reconstructedData); err != nil {
103+
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot compare original and reconstructed JSON: %v", err)
104+
} else if !isEqual {
105+
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "original and reconstructed JSON objects do not match, original: %s, reconstructed: %s", string(data), string(reconstructedData))
106+
}
95107
default:
96108
return nil, errorsmod.Wrapf(ErrInvalidCodec, "unsupported encoding format %s", encoding)
97109
}
@@ -109,3 +121,14 @@ func DeserializeCosmosTx(cdc codec.Codec, data []byte, encoding string) ([]sdk.M
109121

110122
return msgs, nil
111123
}
124+
125+
func equalJSON(a, b []byte) (bool, error) {
126+
var x, y any
127+
if err := json.Unmarshal(a, &x); err != nil {
128+
return false, err
129+
}
130+
if err := json.Unmarshal(b, &y); err != nil {
131+
return false, err
132+
}
133+
return reflect.DeepEqual(x, y), nil
134+
}

0 commit comments

Comments
 (0)