MM-69978: strengthen validation of card property data

server/api/cards.go

@@ -297,6 +297,11 @@ func (a *API) handlePatchCard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if err = patch.CheckValid(); err != nil {
+		a.errorResponse(w, r, model.NewErrBadRequest(err.Error()))
+		return
+	}
+
 	auditRec := a.makeAuditRecord(r, "patchCard", audit.Fail)
 	defer a.audit.LogRecord(audit.LevelModify, auditRec)
 	auditRec.AddMeta("boardID", card.BoardID)

server/integrationtests/board_test.go

@@ -1033,6 +1033,42 @@ func TestPatchBoard(t *testing.T) {
 		require.Nil(t, rBoard)
 	})
 
+	t.Run("a patch with a malformed card property should be rejected", func(t *testing.T) {
+		th := SetupTestHelperPluginMode(t)
+		defer th.TearDown()
+
+		clients := setupClients(th)
+		th.Client = clients.TeamMember
+
+		teamID := mmModel.NewId()
+		user1 := th.GetUser1()
+
+		newBoard := &model.Board{
+			Title:  "title",
+			Type:   model.BoardTypeOpen,
+			TeamID: teamID,
+			CardProperties: []map[string]any{
+				{"id": "property-id", "name": "Note", "type": "text", "options": []any{}},
+			},
+		}
+		board, err := th.Server.App().CreateBoard(newBoard, user1.ID, true)
+		require.NoError(t, err)
+
+		patch := &model.BoardPatch{
+			UpdatedCardProperties: []map[string]any{
+				{"id": "property-id", "name": map[string]any{}, "type": "text", "options": []any{}},
+			},
+		}
+
+		rBoard, resp := th.Client.PatchBoard(board.ID, patch)
+		th.CheckBadRequest(resp)
+		require.Nil(t, rBoard)
+
+		dbBoard, err := th.Server.App().GetBoard(board.ID)
+		require.NoError(t, err)
+		require.Equal(t, "Note", dbBoard.CardProperties[0]["name"])
+	})
+
 	t.Run("valid patch on a board with permissions", func(t *testing.T) {
 		th := SetupTestHelperPluginMode(t)
 		defer th.TearDown()

server/integrationtests/cards_test.go

@@ -263,6 +263,36 @@ func TestPatchCard(t *testing.T) {
 		require.Error(t, resp.Error)
 		require.Nil(t, cardNew)
 	})
+
+	t.Run("a patch with a malformed property value should be rejected", func(t *testing.T) {
+		th := SetupTestHelperPluginMode(t)
+		defer th.TearDown()
+
+		clients := setupClients(th)
+		th.Client = clients.TeamMember
+		teamID := mmModel.NewId()
+		_, cards := th.CreateBoardAndCards(teamID, model.BoardTypeOpen, 1)
+		card := cards[0]
+
+		propertyID := ""
+		for id := range card.Properties {
+			propertyID = id
+			break
+		}
+		require.NotEmpty(t, propertyID)
+
+		patch := &model.CardPatch{
+			UpdatedProperties: map[string]any{propertyID: map[string]any{}},
+		}
+
+		patchedCard, resp := th.Client.PatchCard(card.ID, patch, false)
+		th.CheckBadRequest(resp)
+		require.Nil(t, patchedCard)
+
+		fetchedCard, resp := th.Client.GetCard(card.ID)
+		th.CheckOK(resp)
+		require.Equal(t, card.Properties[propertyID], fetchedCard.Properties[propertyID])
+	})
 }
 
 func TestGetCard(t *testing.T) {

server/model/block.go

@@ -23,6 +23,7 @@ const (
 	BlockFieldsMaxRunes    = 800000
 	BlockFieldFileId       = "fileId"
 	BlockFieldAttachmentId = "attachmentId"
+	BlockFieldProperties   = "properties"
 )
 
 var (
@@ -199,7 +200,7 @@ func (b *Block) baseValidations() error {
 		}
 	}
 
-	if propsIface, present := b.Fields["properties"]; present {
+	if propsIface, present := b.Fields[BlockFieldProperties]; present {
 		if _, ok := propsIface.(map[string]interface{}); !ok {
 			return ErrBlockPropertiesInvalidType
 		}
@@ -264,6 +265,17 @@ func validateUpdatedFields(fields map[string]interface{}) error {
 			}
 		}
 
+		if key == BlockFieldProperties {
+			props, ok := value.(map[string]interface{})
+			if !ok {
+				return NewErrBadRequest(ErrBlockPropertiesInvalidType.Error())
+			}
+
+			if err := ValidateCardPropertyValues(props); err != nil {
+				return NewErrBadRequest(err.Error())
+			}
+		}
+
 		if nestedMap, ok := value.(map[string]interface{}); ok {
 			if err := validateUpdatedFields(nestedMap); err != nil {
 				return err

server/model/board.go

@@ -329,7 +329,7 @@ func (p *BoardPatch) Patch(board *Board) *Board {
 		// existing ones or add them
 		for _, newprop := range p.UpdatedCardProperties {
 			id, ok := newprop["id"].(string)
-			if !ok {
+			if !ok || ValidateCardPropertyTemplate(newprop) != nil {
 				// bad new property, skipping
 				continue
 			}
@@ -377,7 +377,7 @@ func (p *BoardPatch) IsValid() error {
 		return InvalidBoardErr{"invalid-channel-id"}
 	}
 
-	return nil
+	return ValidateCardPropertyTemplates(p.UpdatedCardProperties)
 }
 
 type InvalidBoardErr struct {
@@ -403,6 +403,10 @@ func (b *Board) IsValid() error {
 		return InvalidBoardErr{"invalid-channel-id"}
 	}
 
+	if err := ValidateCardPropertyTemplates(b.CardProperties); err != nil {
+		return err
+	}
+
 	return b.baseValidation()
 }
 

server/model/card.go

@@ -137,7 +137,7 @@ func (c *Card) CheckValid() error {
 	if c.UpdateAt == 0 {
 		return ErrInvalidCard{"UpdateAt"}
 	}
-	return nil
+	return ValidateCardPropertyValues(c.Properties)
 }
 
 // CardPatch is a patch for modifying cards
@@ -181,6 +181,10 @@ func (p *CardPatch) Patch(card *Card) *Card {
 	// if there are properties marked for update, we replace the
 	// existing ones or add them
 	for propID, propVal := range p.UpdatedProperties {
+		if !IsValidCardPropertyValue(propVal) {
+			continue
+		}
+
 		card.Properties[propID] = propVal
 	}
 
@@ -192,7 +196,7 @@ func (p *CardPatch) CheckValid() error {
 	if p.Icon != nil && uniseg.GraphemeClusterCount(*p.Icon) > 1 {
 		return ErrInvalidCard{"Icon can have only one grapheme"}
 	}
-	return nil
+	return ValidateCardPropertyValues(p.UpdatedProperties)
 }
 
 // Card2Block converts a card to block using a shallow copy. Not needed once cards are first class entities.

server/model/card_property.go

@@ -0,0 +1,118 @@
+// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package model
+
+import "fmt"
+
+type ErrInvalidCardProperty struct {
+	msg string
+}
+
+func NewErrInvalidCardProperty(msg string) ErrInvalidCardProperty {
+	return ErrInvalidCardProperty{msg: msg}
+}
+
+func (e ErrInvalidCardProperty) Error() string {
+	return fmt.Sprintf("invalid card property, %s", e.msg)
+}
+
+// Card property templates and values are persisted as free-form JSON and
+// rendered directly by the clients, which only handle strings and arrays of
+// strings. Anything else is rejected before it reaches the database.
+func IsValidCardPropertyValue(value any) bool {
+	switch v := value.(type) {
+	case nil, string, []string:
+		return true
+	case []any:
+		for _, item := range v {
+			if _, ok := item.(string); !ok {
+				return false
+			}
+		}
+		return true
+	default:
+		return false
+	}
+}
+
+func ValidateCardPropertyValues(values map[string]any) error {
+	for propertyID, value := range values {
+		if !IsValidCardPropertyValue(value) {
+			return NewErrInvalidCardProperty(fmt.Sprintf("value of property %q must be a string or an array of strings", propertyID))
+		}
+	}
+
+	return nil
+}
+
+func ValidateCardPropertyTemplate(template map[string]any) error {
+	id, ok := template["id"].(string)
+	if !ok || id == "" {
+		return NewErrInvalidCardProperty("id must be a non empty string")
+	}
+
+	for _, field := range []string{"name", "type"} {
+		value, exists := template[field]
+		if !exists {
+			continue
+		}
+
+		if _, ok := value.(string); !ok {
+			return NewErrInvalidCardProperty(fmt.Sprintf("%s of property %q must be a string", field, id))
+		}
+	}
+
+	return validateCardPropertyOptions(id, template["options"])
+}
+
+func ValidateCardPropertyTemplates(templates []map[string]any) error {
+	for _, template := range templates {
+		if err := ValidateCardPropertyTemplate(template); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func validateCardPropertyOptions(propertyID string, options any) error {
+	invalidErr := NewErrInvalidCardProperty(fmt.Sprintf("options of property %q must be a list of objects with string fields", propertyID))
+
+	switch opts := options.(type) {
+	case nil:
+		return nil
+	case []any:
+		for _, option := range opts {
+			optionMap, ok := option.(map[string]any)
+			if !ok || !isValidCardPropertyOption(optionMap) {
+				return invalidErr
+			}
+		}
+	case []map[string]any:
+		for _, option := range opts {
+			if !isValidCardPropertyOption(option) {
+				return invalidErr
+			}
+		}
+	default:
+		return invalidErr
+	}
+
+	return nil
+}
+
+func isValidCardPropertyOption(option map[string]any) bool {
+	for _, field := range []string{"id", "value", "color"} {
+		value, exists := option[field]
+		if !exists {
+			continue
+		}
+
+		if _, ok := value.(string); !ok {
+			return false
+		}
+	}
+
+	return true
+}