Merge pull request cosmos#562 from CosmWasm/560-reject-invalid-attrib…

…utes Reject invalid attributes
notional-labs · Jul 27, 2021 · b2adcc4 · b2adcc4
2 parents 86d96fd + d540e00
commit b2adcc4
Show file tree

Hide file tree

Showing 5 changed files with 138 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 ## [Unreleased](https://github.com/CosmWasm/wasmd/tree/HEAD)
 
+**Implemented Enhancements:**
+- Reject invalid events/attributes returned from contracts [\#560](https://github.com/CosmWasm/wasmd/pull/560)
+
 [Full Changelog](https://github.com/CosmWasm/wasmd/compare/v0.17.0...HEAD)
 
 ## [v0.17.0](https://github.com/CosmWasm/wasmd/tree/v0.17.0) (2021-05-26)

diff --git a/x/wasm/keeper/events.go b/x/wasm/keeper/events.go
@@ -5,16 +5,20 @@ import (
 	"github.com/CosmWasm/wasmd/x/wasm/types"
 	wasmvmtypes "github.com/CosmWasm/wasmvm/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"strings"
 )
 
 // newWasmModuleEvent creates with wasm module event for interacting with the given contract. Adds custom attributes
 // to this event.
-func newWasmModuleEvent(customAttributes []wasmvmtypes.EventAttribute, contractAddr sdk.AccAddress) sdk.Events {
-	attrs := contractSDKEventAttributes(customAttributes, contractAddr)
+func newWasmModuleEvent(customAttributes []wasmvmtypes.EventAttribute, contractAddr sdk.AccAddress) (sdk.Events, error) {
+	attrs, err := contractSDKEventAttributes(customAttributes, contractAddr)
+	if err != nil {
+		return nil, err
+	}
 
 	// each wasm invocation always returns one sdk.Event
-	return sdk.Events{sdk.NewEvent(types.WasmModuleEventType, attrs...)}
+	return sdk.Events{sdk.NewEvent(types.WasmModuleEventType, attrs...)}, nil
 }
 
 // returns true when a wasm module event was emitted for this contract already
@@ -34,28 +38,42 @@ func hasWasmModuleEvent(ctx sdk.Context, contractAddr sdk.AccAddress) bool {
 const eventTypeMinLength = 2
 
 // newCustomEvents converts wasmvm events from a contract response to sdk type events
-func newCustomEvents(evts wasmvmtypes.Events, contractAddr sdk.AccAddress) sdk.Events {
+func newCustomEvents(evts wasmvmtypes.Events, contractAddr sdk.AccAddress) (sdk.Events, error) {
 	events := make(sdk.Events, 0, len(evts))
 	for _, e := range evts {
-		if len(e.Type) <= eventTypeMinLength {
-			continue
+		typ := strings.TrimSpace(e.Type)
+		if len(typ) <= eventTypeMinLength {
+			return nil, sdkerrors.Wrap(types.ErrInvalidEvent, fmt.Sprintf("Event type too short: '%s'", typ))
+		}
+		attributes, err := contractSDKEventAttributes(e.Attributes, contractAddr)
+		if err != nil {
+			return nil, err
 		}
-		attributes := contractSDKEventAttributes(e.Attributes, contractAddr)
-		events = append(events, sdk.NewEvent(fmt.Sprintf("%s%s", types.CustomContractEventPrefix, e.Type), attributes...))
+		events = append(events, sdk.NewEvent(fmt.Sprintf("%s%s", types.CustomContractEventPrefix, typ), attributes...))
 	}
-	return events
+	return events, nil
 }
 
 // convert and add contract address issuing this event
-func contractSDKEventAttributes(customAttributes []wasmvmtypes.EventAttribute, contractAddr sdk.AccAddress) []sdk.Attribute {
+func contractSDKEventAttributes(customAttributes []wasmvmtypes.EventAttribute, contractAddr sdk.AccAddress) ([]sdk.Attribute, error) {
 	attrs := []sdk.Attribute{sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddr.String())}
 	// append attributes from wasm to the sdk.Event
 	for _, l := range customAttributes {
-		// FIXME: do we want to error here on invalid events
+		// ensure key and value are non-empty (and trim what is there)
+		key := strings.TrimSpace(l.Key)
+		if len(key) == 0 {
+			return nil, sdkerrors.Wrap(types.ErrInvalidEvent, fmt.Sprintf("Empty attribute key. Value: %s", l.Value))
+		}
+		value := strings.TrimSpace(l.Value)
+		// TODO: check if this is legal in the SDK - if it is, we can remove this check
+		if len(value) == 0 {
+			return nil, sdkerrors.Wrap(types.ErrInvalidEvent, fmt.Sprintf("Empty attribute value. Key: %s", key))
+		}
 		// and reserve all _* keys for our use (not contract)
-		if len(l.Key) > 0 && len(l.Value) > 0 && !strings.HasPrefix(l.Key, types.AttributeReservedPrefix) {
-			attrs = append(attrs, sdk.NewAttribute(l.Key, l.Value))
+		if strings.HasPrefix(key, types.AttributeReservedPrefix) {
+			return nil, sdkerrors.Wrap(types.ErrInvalidEvent, fmt.Sprintf("Attribute key starts with reserved prefix %s: '%s'", types.AttributeReservedPrefix, key))
 		}
+		attrs = append(attrs, sdk.NewAttribute(key, value))
 	}
-	return attrs
+	return attrs, nil
 }
diff --git a/x/wasm/keeper/events_test.go b/x/wasm/keeper/events_test.go
@@ -53,8 +53,9 @@ func TestHasWasmModuleEvent(t *testing.T) {
 func TestNewCustomEvents(t *testing.T) {
 	myContract := RandomAccountAddress(t)
 	specs := map[string]struct {
-		src wasmvmtypes.Events
-		exp sdk.Events
+		src     wasmvmtypes.Events
+		exp     sdk.Events
+		isError bool
 	}{
 		"all good": {
 			src: wasmvmtypes.Events{{
@@ -100,58 +101,106 @@ func TestNewCustomEvents(t *testing.T) {
 			exp: sdk.Events{sdk.NewEvent("wasm-foo",
 				sdk.NewAttribute("_contract_address", myContract.String()))},
 		},
-		"min length not reached": {
+		"error on short event type": {
 			src: wasmvmtypes.Events{{
 				Type: "f",
 			}},
-			exp: sdk.Events{},
+			isError: true,
 		},
-		"overwrite _contract_address": {
+		"error on _contract_address": {
 			src: wasmvmtypes.Events{{
 				Type:       "foo",
 				Attributes: []wasmvmtypes.EventAttribute{{Key: "_contract_address", Value: RandomBech32AccountAddress(t)}},
 			}},
-			exp: sdk.Events{sdk.NewEvent("wasm-foo",
-				sdk.NewAttribute("_contract_address", myContract.String()))},
+			isError: true,
 		},
-		"ignore reserved prefix": {
+		"error on reserved prefix": {
 			src: wasmvmtypes.Events{{
 				Type: "wasm",
 				Attributes: []wasmvmtypes.EventAttribute{
 					{Key: "_reserved", Value: "is skipped"},
 					{Key: "normal", Value: "is used"}},
 			}},
-			exp: sdk.Events{sdk.NewEvent("wasm-wasm",
-				sdk.NewAttribute("_contract_address", myContract.String()),
-				sdk.NewAttribute("normal", "is used"))},
+			isError: true,
 		},
-		"ignore empty attributes": {
+		"error on empty value": {
 			src: wasmvmtypes.Events{{
 				Type: "boom",
 				Attributes: []wasmvmtypes.EventAttribute{
 					{Key: "some", Value: "data"},
 					{Key: "key", Value: ""},
+				},
+			}},
+			isError: true,
+		},
+		"error on empty key": {
+			src: wasmvmtypes.Events{{
+				Type: "boom",
+				Attributes: []wasmvmtypes.EventAttribute{
+					{Key: "some", Value: "data"},
 					{Key: "", Value: "value"},
 				},
 			}},
-			exp: sdk.Events{sdk.NewEvent("wasm-boom",
+			isError: true,
+		},
+		"error on whitespace type": {
+			src: wasmvmtypes.Events{{
+				Type: "    f   ",
+				Attributes: []wasmvmtypes.EventAttribute{
+					{Key: "some", Value: "data"},
+				},
+			}},
+			isError: true,
+		},
+		"error on only whitespace key": {
+			src: wasmvmtypes.Events{{
+				Type: "boom",
+				Attributes: []wasmvmtypes.EventAttribute{
+					{Key: "some", Value: "data"},
+					{Key: "\n\n\n\n", Value: "value"},
+				},
+			}},
+			isError: true,
+		},
+		"error on only whitespace value": {
+			src: wasmvmtypes.Events{{
+				Type: "boom",
+				Attributes: []wasmvmtypes.EventAttribute{
+					{Key: "some", Value: "data"},
+					{Key: "myKey", Value: " \t\r\n"},
+				},
+			}},
+			isError: true,
+		},
+		"strip out whitespace": {
+			src: wasmvmtypes.Events{{
+				Type:       "  food\n",
+				Attributes: []wasmvmtypes.EventAttribute{{Key: "my Key", Value: "\tmyVal"}},
+			}},
+			exp: sdk.Events{sdk.NewEvent("wasm-food",
 				sdk.NewAttribute("_contract_address", myContract.String()),
-				sdk.NewAttribute("some", "data"))},
+				sdk.NewAttribute("my Key", "myVal"))},
 		},
 	}
 	for name, spec := range specs {
 		t.Run(name, func(t *testing.T) {
-			gotEvent := newCustomEvents(spec.src, myContract)
-			assert.Equal(t, spec.exp, gotEvent)
+			gotEvent, err := newCustomEvents(spec.src, myContract)
+			if spec.isError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, spec.exp, gotEvent)
+			}
 		})
 	}
 }
 
 func TestNewWasmModuleEvent(t *testing.T) {
 	myContract := RandomAccountAddress(t)
 	specs := map[string]struct {
-		src []wasmvmtypes.EventAttribute
-		exp sdk.Events
+		src     []wasmvmtypes.EventAttribute
+		exp     sdk.Events
+		isError bool
 	}{
 		"all good": {
 			src: []wasmvmtypes.EventAttribute{{Key: "myKey", Value: "myVal"}},
@@ -171,16 +220,34 @@ func TestNewWasmModuleEvent(t *testing.T) {
 			exp: sdk.Events{sdk.NewEvent("wasm",
 				sdk.NewAttribute("_contract_address", myContract.String()))},
 		},
-		"overwrite _contract_address": {
-			src: []wasmvmtypes.EventAttribute{{Key: "_contract_address", Value: RandomBech32AccountAddress(t)}},
+		"error on _contract_address": {
+			src:     []wasmvmtypes.EventAttribute{{Key: "_contract_address", Value: RandomBech32AccountAddress(t)}},
+			isError: true,
+		},
+		"error on whitespace key": {
+			src:     []wasmvmtypes.EventAttribute{{Key: "  ", Value: "value"}},
+			isError: true,
+		},
+		"error on whitespace value": {
+			src:     []wasmvmtypes.EventAttribute{{Key: "key", Value: "\n\n\n"}},
+			isError: true,
+		},
+		"strip whitespace": {
+			src: []wasmvmtypes.EventAttribute{{Key: "   my-real-key    ", Value: "\n\n\nsome-val\t\t\t"}},
 			exp: sdk.Events{sdk.NewEvent("wasm",
-				sdk.NewAttribute("_contract_address", myContract.String()))},
+				sdk.NewAttribute("_contract_address", myContract.String()),
+				sdk.NewAttribute("my-real-key", "some-val"))},
 		},
 	}
 	for name, spec := range specs {
 		t.Run(name, func(t *testing.T) {
-			gotEvent := newWasmModuleEvent(spec.src, myContract)
-			assert.Equal(t, spec.exp, gotEvent)
+			gotEvent, err := newWasmModuleEvent(spec.src, myContract)
+			if spec.isError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, spec.exp, gotEvent)
+			}
 		})
 	}
 }
diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go
@@ -771,10 +771,18 @@ func (k *Keeper) handleContractResponse(
 	ctx.GasMeter().ConsumeGas(attributeGasCost, "Custom contract event attributes")
 	// emit all events from this contract itself
 	if len(attrs) != 0 || !hasWasmModuleEvent(ctx, contractAddr) {
-		ctx.EventManager().EmitEvents(newWasmModuleEvent(attrs, contractAddr))
+		wasmEvents, err := newWasmModuleEvent(attrs, contractAddr)
+		if err != nil {
+			return nil, err
+		}
+		ctx.EventManager().EmitEvents(wasmEvents)
 	}
 	if len(evts) > 0 {
-		ctx.EventManager().EmitEvents(newCustomEvents(evts, contractAddr))
+		customEvents, err := newCustomEvents(evts, contractAddr)
+		if err != nil {
+			return nil, err
+		}
+		ctx.EventManager().EmitEvents(customEvents)
 	}
 	return k.wasmVMResponseHandler.Handle(ctx, contractAddr, ibcPort, msgs, data)
 }

diff --git a/x/wasm/types/errors.go b/x/wasm/types/errors.go
@@ -66,4 +66,7 @@ var (
 
 	// ErrUnknownMsg error by a message handler to show that it is not responsible for this message type
 	ErrUnknownMsg = sdkErrors.Register(DefaultCodespace, 20, "unknown message from the contract")
+
+	// ErrInvalidEvent error if an attribute/event from the contract is invalid
+	ErrInvalidEvent = sdkErrors.Register(DefaultCodespace, 21, "invalid event")
 )