From 9c00e87c746ada429eb36ea5c7a7d5855424db8c Mon Sep 17 00:00:00 2001 From: Alexander Peters Date: Wed, 2 Nov 2022 17:16:32 +0100 Subject: [PATCH] Introduce AcceptListStargateQuerier (#1069) * Stargate query enable * Remove initialized whitelists * Roman's review * Minor improvement * Add tests * Add testings and codec * Fix lint * Fix test * Fix from code review * Refactor Stargate querier init * Fix typo Co-authored-by: mattverse --- x/wasm/keeper/options.go | 2 +- x/wasm/keeper/query_plugins.go | 66 +++++++- x/wasm/keeper/query_plugins_test.go | 227 ++++++++++++++++++++++++++-- x/wasm/keeper/reflect_test.go | 8 +- 4 files changed, 283 insertions(+), 20 deletions(-) diff --git a/x/wasm/keeper/options.go b/x/wasm/keeper/options.go index 349c4f34b5..4ae520d353 100644 --- a/x/wasm/keeper/options.go +++ b/x/wasm/keeper/options.go @@ -58,7 +58,7 @@ func WithQueryHandlerDecorator(d func(old WasmVMQueryHandler) WasmVMQueryHandler } // WithQueryPlugins is an optional constructor parameter to pass custom query plugins for wasmVM requests. -// This option expects the default `QueryHandler` set an should not be combined with Option `WithQueryHandler` or `WithQueryHandlerDecorator`. +// This option expects the default `QueryHandler` set and should not be combined with Option `WithQueryHandler` or `WithQueryHandlerDecorator`. func WithQueryPlugins(x *QueryPlugins) Option { return optsFn(func(k *Keeper) { q, ok := k.wasmVMQueryHandler.(QueryPlugins) diff --git a/x/wasm/keeper/query_plugins.go b/x/wasm/keeper/query_plugins.go index 14b7922dd6..a7ca465a7f 100644 --- a/x/wasm/keeper/query_plugins.go +++ b/x/wasm/keeper/query_plugins.go @@ -3,10 +3,12 @@ package keeper import ( "encoding/json" "errors" + "fmt" abci "github.com/tendermint/tendermint/abci/types" baseapp "github.com/Finschia/finschia-sdk/baseapp" + "github.com/Finschia/finschia-sdk/codec" sdk "github.com/Finschia/finschia-sdk/types" sdkerrors "github.com/Finschia/finschia-sdk/types/errors" distributiontypes "github.com/Finschia/finschia-sdk/x/distribution/types" @@ -107,7 +109,7 @@ func DefaultQueryPlugins( Custom: CustomQuerierImpl(queryRouter), IBC: IBCQuerier(wasm, channelKeeper), Staking: StakingQuerier(staking, distKeeper), - Stargate: StargateQuerier(queryRouter), + Stargate: RejectStargateQuerier(), Wasm: WasmQuerier(wasm), } } @@ -302,9 +304,47 @@ func IBCQuerier(wasm contractMetaDataSource, channelKeeper types.ChannelKeeper) } } -func StargateQuerier(queryRouter GRPCQueryRouter) func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { - return func(ctx sdk.Context, msg *wasmvmtypes.StargateQuery) ([]byte, error) { - return nil, wasmvmtypes.UnsupportedRequest{Kind: "Stargate queries are disabled."} +// RejectStargateQuerier rejects all stargate queries +func RejectStargateQuerier() func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { + return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { + return nil, wasmvmtypes.UnsupportedRequest{Kind: "Stargate queries are disabled"} + } +} + +// AcceptedStargateQueries define accepted Stargate queries as a map with path as key and response type as value. +// For example: +// acceptList["/cosmos.auth.v1beta1.Query/Account"]= &authtypes.QueryAccountResponse{} +type AcceptedStargateQueries map[string]codec.ProtoMarshaler + +// AcceptListStargateQuerier supports a preconfigured set of stargate queries only. +// All arguments must be non nil. +// +// Warning: Chains need to test and maintain their accept list carefully. +// There were critical consensus breaking issues in the past with non-deterministic behaviour in the SDK. +// +// This queries can be set via WithQueryPlugins option in the wasm keeper constructor: +// WithQueryPlugins(&QueryPlugins{Stargate: AcceptListStargateQuerier(acceptList, queryRouter, codec)}) +func AcceptListStargateQuerier(acceptList AcceptedStargateQueries, queryRouter GRPCQueryRouter, codec codec.Codec) func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { + return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { + protoResponse, accepted := acceptList[request.Path] + if !accepted { + return nil, wasmvmtypes.UnsupportedRequest{Kind: fmt.Sprintf("'%s' path is not allowed from the contract", request.Path)} + } + + route := queryRouter.Route(request.Path) + if route == nil { + return nil, wasmvmtypes.UnsupportedRequest{Kind: fmt.Sprintf("No route to query '%s'", request.Path)} + } + + res, err := route(ctx, abci.RequestQuery{ + Data: request.Data, + Path: request.Path, + }) + if err != nil { + return nil, err + } + + return ConvertProtoToJSONMarshal(codec, protoResponse, res.Value) } } @@ -551,6 +591,24 @@ func ConvertSdkCoinToWasmCoin(coin sdk.Coin) wasmvmtypes.Coin { } } +// ConvertProtoToJSONMarshal unmarshals the given bytes into a proto message and then marshals it to json. +// This is done so that clients calling stargate queries do not need to define their own proto unmarshalers, +// being able to use response directly by json marshalling, which is supported in cosmwasm. +func ConvertProtoToJSONMarshal(cdc codec.Codec, protoResponse codec.ProtoMarshaler, bz []byte) ([]byte, error) { + // unmarshal binary into stargate response data structure + err := cdc.Unmarshal(bz, protoResponse) + if err != nil { + return nil, sdkerrors.Wrap(err, "to proto") + } + + bz, err = cdc.MarshalJSON(protoResponse) + if err != nil { + return nil, sdkerrors.Wrap(err, "to json") + } + + return bz, nil +} + var _ WasmVMQueryHandler = WasmVMQueryHandlerFn(nil) // WasmVMQueryHandlerFn is a helper to construct a function based query handler. diff --git a/x/wasm/keeper/query_plugins_test.go b/x/wasm/keeper/query_plugins_test.go index d7b9a8122f..376af2998b 100644 --- a/x/wasm/keeper/query_plugins_test.go +++ b/x/wasm/keeper/query_plugins_test.go @@ -1,19 +1,33 @@ -package keeper +package keeper_test import ( + "encoding/hex" "encoding/json" + "fmt" "testing" + "time" + "github.com/gogo/protobuf/proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" dbm "github.com/tendermint/tm-db" + "github.com/Finschia/finschia-sdk/codec" + codectypes "github.com/Finschia/finschia-sdk/codec/types" + "github.com/Finschia/finschia-sdk/crypto/keys/ed25519" "github.com/Finschia/finschia-sdk/store" sdk "github.com/Finschia/finschia-sdk/types" sdkerrors "github.com/Finschia/finschia-sdk/types/errors" + "github.com/Finschia/finschia-sdk/types/query" + authtypes "github.com/Finschia/finschia-sdk/x/auth/types" + banktypes "github.com/Finschia/finschia-sdk/x/bank/types" + stakingtypes "github.com/Finschia/finschia-sdk/x/staking/types" channeltypes "github.com/Finschia/ibc-go/v3/modules/core/04-channel/types" wasmvmtypes "github.com/Finschia/wasmvm/types" + "github.com/Finschia/wasmd/app" + "github.com/Finschia/wasmd/x/wasm/keeper" "github.com/Finschia/wasmd/x/wasm/keeper/wasmtesting" "github.com/Finschia/wasmd/x/wasm/types" ) @@ -307,8 +321,8 @@ func TestIBCQuerier(t *testing.T) { } for name, spec := range specs { t.Run(name, func(t *testing.T) { - h := IBCQuerier(spec.wasmKeeper, spec.channelKeeper) - gotResult, gotErr := h(sdk.Context{}, RandomAccountAddress(t), spec.srcQuery) + h := keeper.IBCQuerier(spec.wasmKeeper, spec.channelKeeper) + gotResult, gotErr := h(sdk.Context{}, keeper.RandomAccountAddress(t), spec.srcQuery) require.True(t, spec.expErr.Is(gotErr), "exp %v but got %#+v", spec.expErr, gotErr) if spec.expErr != nil { return @@ -324,10 +338,10 @@ func TestBankQuerierBalance(t *testing.T) { }} ctx := sdk.Context{} - q := BankQuerier(mock) + q := keeper.BankQuerier(mock) gotBz, gotErr := q(ctx, &wasmvmtypes.BankQuery{ Balance: &wasmvmtypes.BalanceQuery{ - Address: RandomBech32AccountAddress(t), + Address: keeper.RandomBech32AccountAddress(t), Denom: "ALX", }, }) @@ -344,9 +358,9 @@ func TestBankQuerierBalance(t *testing.T) { } func TestContractInfoWasmQuerier(t *testing.T) { - myValidContractAddr := RandomBech32AccountAddress(t) - myCreatorAddr := RandomBech32AccountAddress(t) - myAdminAddr := RandomBech32AccountAddress(t) + myValidContractAddr := keeper.RandomBech32AccountAddress(t) + myCreatorAddr := keeper.RandomBech32AccountAddress(t) + myAdminAddr := keeper.RandomBech32AccountAddress(t) var ctx sdk.Context specs := map[string]struct { @@ -433,7 +447,7 @@ func TestContractInfoWasmQuerier(t *testing.T) { } for name, spec := range specs { t.Run(name, func(t *testing.T) { - q := WasmQuerier(spec.mock) + q := keeper.WasmQuerier(spec.mock) gotBz, gotErr := q(ctx, spec.req) if spec.expErr { require.Error(t, gotErr) @@ -464,17 +478,82 @@ func TestQueryErrors(t *testing.T) { } for name, spec := range specs { t.Run(name, func(t *testing.T) { - mock := WasmVMQueryHandlerFn(func(ctx sdk.Context, caller sdk.AccAddress, request wasmvmtypes.QueryRequest) ([]byte, error) { + mock := keeper.WasmVMQueryHandlerFn(func(ctx sdk.Context, caller sdk.AccAddress, request wasmvmtypes.QueryRequest) ([]byte, error) { return nil, spec.src }) ctx := sdk.Context{}.WithGasMeter(sdk.NewInfiniteGasMeter()).WithMultiStore(store.NewCommitMultiStore(dbm.NewMemDB())) - q := NewQueryHandler(ctx, mock, sdk.AccAddress{}, NewDefaultWasmGasRegister()) + q := keeper.NewQueryHandler(ctx, mock, sdk.AccAddress{}, keeper.NewDefaultWasmGasRegister()) _, gotErr := q.Query(wasmvmtypes.QueryRequest{}, 1) assert.Equal(t, spec.expErr, gotErr) }) } } +func TestAcceptListStargateQuerier(t *testing.T) { + wasmApp := app.SetupWithEmptyStore(t) + ctx := wasmApp.NewUncachedContext(false, tmproto.Header{ChainID: "foo", Height: 1, Time: time.Now()}) + wasmApp.StakingKeeper.SetParams(ctx, stakingtypes.DefaultParams()) + + addrs := app.AddTestAddrs(wasmApp, ctx, 2, sdk.NewInt(1_000_000)) + accepted := keeper.AcceptedStargateQueries{ + "/cosmos.auth.v1beta1.Query/Account": &authtypes.QueryAccountResponse{}, + "/no/route/to/this": &authtypes.QueryAccountResponse{}, + } + + marshal := func(pb proto.Message) []byte { + b, err := proto.Marshal(pb) + require.NoError(t, err) + return b + } + + specs := map[string]struct { + req *wasmvmtypes.StargateQuery + expErr bool + expResp string + }{ + "in accept list - success result": { + req: &wasmvmtypes.StargateQuery{ + Path: "/cosmos.auth.v1beta1.Query/Account", + Data: marshal(&authtypes.QueryAccountRequest{Address: addrs[0].String()}), + }, + expResp: fmt.Sprintf(`{"account":{"@type":"/cosmos.auth.v1beta1.BaseAccount","address":%q,"pub_key":null,"account_number":"1","sequence":"0"}}`, addrs[0].String()), + }, + "in accept list - error result": { + req: &wasmvmtypes.StargateQuery{ + Path: "/cosmos.auth.v1beta1.Query/Account", + Data: marshal(&authtypes.QueryAccountRequest{Address: sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address()).String()}), + }, + expErr: true, + }, + "not in accept list": { + req: &wasmvmtypes.StargateQuery{ + Path: "/cosmos.bank.v1beta1.Query/AllBalances", + Data: marshal(&banktypes.QueryAllBalancesRequest{Address: addrs[0].String()}), + }, + expErr: true, + }, + "unknown route": { + req: &wasmvmtypes.StargateQuery{ + Path: "/no/route/to/this", + Data: marshal(&banktypes.QueryAllBalancesRequest{Address: addrs[0].String()}), + }, + expErr: true, + }, + } + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + q := keeper.AcceptListStargateQuerier(accepted, wasmApp.GRPCQueryRouter(), wasmApp.AppCodec()) + gotBz, gotErr := q(ctx, spec.req) + if spec.expErr { + require.Error(t, gotErr) + return + } + require.NoError(t, gotErr) + assert.JSONEq(t, spec.expResp, string(gotBz), string(gotBz)) + }) + } +} + type mockWasmQueryKeeper struct { GetContractInfoFn func(ctx sdk.Context, contractAddress sdk.AccAddress) *types.ContractInfo QueryRawFn func(ctx sdk.Context, contractAddress sdk.AccAddress, key []byte) []byte @@ -536,3 +615,129 @@ func (m bankKeeperMock) GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk } return m.GetAllBalancesFn(ctx, addr) } + +func TestConvertProtoToJSONMarshal(t *testing.T) { + testCases := []struct { + name string + queryPath string + protoResponseStruct codec.ProtoMarshaler + originalResponse string + expectedProtoResponse codec.ProtoMarshaler + expectedError bool + }{ + { + name: "successful conversion from proto response to json marshalled response", + queryPath: "/cosmos.bank.v1beta1.Query/AllBalances", + originalResponse: "0a090a036261721202333012050a03666f6f", + protoResponseStruct: &banktypes.QueryAllBalancesResponse{}, + expectedProtoResponse: &banktypes.QueryAllBalancesResponse{ + Balances: sdk.NewCoins(sdk.NewCoin("bar", sdk.NewInt(30))), + Pagination: &query.PageResponse{ + NextKey: []byte("foo"), + }, + }, + }, + { + name: "invalid proto response struct", + queryPath: "/cosmos.bank.v1beta1.Query/AllBalances", + originalResponse: "0a090a036261721202333012050a03666f6f", + protoResponseStruct: &authtypes.QueryAccountResponse{}, + expectedError: true, + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { + originalVersionBz, err := hex.DecodeString(tc.originalResponse) + require.NoError(t, err) + appCodec := app.MakeEncodingConfig().Marshaler + + jsonMarshalledResponse, err := keeper.ConvertProtoToJSONMarshal(appCodec, tc.protoResponseStruct, originalVersionBz) + if tc.expectedError { + require.Error(t, err) + return + } + require.NoError(t, err) + + // check response by json marshalling proto response into json response manually + jsonMarshalExpectedResponse, err := appCodec.MarshalJSON(tc.expectedProtoResponse) + require.NoError(t, err) + require.JSONEq(t, string(jsonMarshalledResponse), string(jsonMarshalExpectedResponse)) + }) + } +} + +// TestDeterministicJsonMarshal tests that we get deterministic JSON marshalled response upon +// proto struct update in the state machine. +func TestDeterministicJsonMarshal(t *testing.T) { + testCases := []struct { + name string + originalResponse string + updatedResponse string + queryPath string + responseProtoStruct codec.ProtoMarshaler + expectedProto func() codec.ProtoMarshaler + }{ + /** + * + * Origin Response + * 0a530a202f636f736d6f732e617574682e763162657461312e426173654163636f756e74122f0a2d636f736d6f7331346c3268686a6e676c3939367772703935673867646a6871653038326375367a7732706c686b + * + * Updated Response + * 0a530a202f636f736d6f732e617574682e763162657461312e426173654163636f756e74122f0a2d636f736d6f7331646a783375676866736d6b6135386676673076616a6e6533766c72776b7a6a346e6377747271122d636f736d6f7331646a783375676866736d6b6135386676673076616a6e6533766c72776b7a6a346e6377747271 + // Origin proto + message QueryAccountResponse { + // account defines the account of the corresponding address. + google.protobuf.Any account = 1 [(cosmos_proto.accepts_interface) = "AccountI"]; + } + // Updated proto + message QueryAccountResponse { + // account defines the account of the corresponding address. + google.protobuf.Any account = 1 [(cosmos_proto.accepts_interface) = "AccountI"]; + // address is the address to query for. + string address = 2; + } + */ + { + "Query Account", + "0a530a202f636f736d6f732e617574682e763162657461312e426173654163636f756e74122f0a2d636f736d6f733166387578756c746e3873717a687a6e72737a3371373778776171756867727367366a79766679", + "0a530a202f636f736d6f732e617574682e763162657461312e426173654163636f756e74122f0a2d636f736d6f733166387578756c746e3873717a687a6e72737a3371373778776171756867727367366a79766679122d636f736d6f733166387578756c746e3873717a687a6e72737a3371373778776171756867727367366a79766679", + "/cosmos.auth.v1beta1.Query/Account", + &authtypes.QueryAccountResponse{}, + func() codec.ProtoMarshaler { + account := authtypes.BaseAccount{ + Address: "cosmos1f8uxultn8sqzhznrsz3q77xwaquhgrsg6jyvfy", + } + accountResponse, err := codectypes.NewAnyWithValue(&account) + require.NoError(t, err) + return &authtypes.QueryAccountResponse{ + Account: accountResponse, + } + }, + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { + appCodec := app.MakeEncodingConfig().Marshaler + + originVersionBz, err := hex.DecodeString(tc.originalResponse) + require.NoError(t, err) + jsonMarshalledOriginalBz, err := keeper.ConvertProtoToJSONMarshal(appCodec, tc.responseProtoStruct, originVersionBz) + require.NoError(t, err) + + newVersionBz, err := hex.DecodeString(tc.updatedResponse) + require.NoError(t, err) + jsonMarshalledUpdatedBz, err := keeper.ConvertProtoToJSONMarshal(appCodec, tc.responseProtoStruct, newVersionBz) + require.NoError(t, err) + + // json marshalled bytes should be the same since we use the same proto struct for unmarshalling + require.Equal(t, jsonMarshalledOriginalBz, jsonMarshalledUpdatedBz) + + // raw build also make same result + jsonMarshalExpectedResponse, err := appCodec.MarshalJSON(tc.expectedProto()) + require.NoError(t, err) + require.Equal(t, jsonMarshalledUpdatedBz, jsonMarshalExpectedResponse) + }) + } +} diff --git a/x/wasm/keeper/reflect_test.go b/x/wasm/keeper/reflect_test.go index 96e0d89b21..c6050b504d 100644 --- a/x/wasm/keeper/reflect_test.go +++ b/x/wasm/keeper/reflect_test.go @@ -386,10 +386,10 @@ func TestReflectInvalidStargateQuery(t *testing.T) { }) require.NoError(t, err) - // make a query on the chain, should be blacklisted + // make a query on the chain, should not be whitelisted _, err = keeper.QuerySmart(ctx, contractAddr, protoQueryBz) require.Error(t, err) - require.Contains(t, err.Error(), "Stargate queries are disabled") + require.Contains(t, err.Error(), "Unsupported query") // now, try to build a protobuf query protoRequest = wasmvmtypes.QueryRequest{ @@ -406,7 +406,7 @@ func TestReflectInvalidStargateQuery(t *testing.T) { // make a query on the chain, should be blacklisted _, err = keeper.QuerySmart(ctx, contractAddr, protoQueryBz) require.Error(t, err) - require.Contains(t, err.Error(), "Stargate queries are disabled") + require.Contains(t, err.Error(), "Unsupported query") // and another one protoRequest = wasmvmtypes.QueryRequest{ @@ -423,7 +423,7 @@ func TestReflectInvalidStargateQuery(t *testing.T) { // make a query on the chain, should be blacklisted _, err = keeper.QuerySmart(ctx, contractAddr, protoQueryBz) require.Error(t, err) - require.Contains(t, err.Error(), "Stargate queries are disabled") + require.Contains(t, err.Error(), "Unsupported query") } type reflectState struct {