MM-69978: strengthen validation of card property data

server/api/blocks.go

@@ -258,6 +258,11 @@ func (a *API) handlePostBlocks(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 
+		if err = model.ValidateBlockProperties(block); err != nil {
+			a.errorResponse(w, r, model.NewErrBadRequest(err.Error()))
+			return
+		}
+
 		if block.CreateAt < 1 {
 			message := fmt.Sprintf("invalid createAt for block id %s", block.ID)
 			a.errorResponse(w, r, model.NewErrBadRequest(message))

server/api/boards.go

@@ -379,6 +379,11 @@ func (a *API) handlePatchBoard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if patch == nil {
+		a.errorResponse(w, r, model.NewErrBadRequest("missing board patch"))
+		return
+	}
+
 	if err = patch.IsValid(); err != nil {
 		a.errorResponse(w, r, model.NewErrBadRequest(err.Error()))
 		return

server/api/boards_and_blocks.go

@@ -142,6 +142,11 @@ func (a *API) handleCreateBoardsAndBlocks(w http.ResponseWriter, r *http.Request
 			a.errorResponse(w, r, model.NewErrBadRequest(message))
 			return
 		}
+
+		if err = model.ValidateBlockProperties(block); err != nil {
+			a.errorResponse(w, r, model.NewErrBadRequest(err.Error()))
+			return
+		}
 	}
 
 	// IDs of boards and blocks are used to confirm that they're

server/api/cards.go

@@ -297,6 +297,16 @@ func (a *API) handlePatchCard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if patch == nil {
+		a.errorResponse(w, r, model.NewErrBadRequest("missing card patch"))
+		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/app/blocks.go

@@ -121,16 +121,16 @@ func (a *App) PatchBlockAndNotify(blockID string, blockPatch *model.BlockPatch,
 }
 
 func (a *App) PatchBlocks(teamID string, blockPatches *model.BlockPatchBatch, modifiedByID string) error {
-	for _, patch := range blockPatches.BlockPatches {
-		err := model.ValidateBlockPatch(&patch)
-		if err != nil {
-			return err
-		}
-	}
 	return a.PatchBlocksAndNotify(teamID, blockPatches, modifiedByID, false)
 }
 
 func (a *App) PatchBlocksAndNotify(teamID string, blockPatches *model.BlockPatchBatch, modifiedByID string, disableNotify bool) error {
+	for i := range blockPatches.BlockPatches {
+		if err := model.ValidateBlockPatch(&blockPatches.BlockPatches[i]); err != nil {
+			return err
+		}
+	}
+
 	oldBlocks, err := a.store.GetBlocksByIDs(blockPatches.BlockIDs)
 	if err != nil {
 		return err

server/app/boards_and_blocks.go

@@ -80,6 +80,16 @@ func (a *App) CreateBoardsAndBlocks(bab *model.BoardsAndBlocks, userID string, a
 }
 
 func (a *App) PatchBoardsAndBlocks(pbab *model.PatchBoardsAndBlocks, userID string) (*model.BoardsAndBlocks, error) {
+	for _, patch := range pbab.BlockPatches {
+		if patch == nil {
+			continue
+		}
+
+		if err := model.ValidateBlockPatch(patch); err != nil {
+			return nil, err
+		}
+	}
+
 	oldBlocks, err := a.store.GetBlocksByIDs(pbab.BlockIDs)
 	if err != nil {
 		return nil, err

server/integrationtests/blocks_test.go

@@ -4,6 +4,8 @@
 package integrationtests
 
 import (
+	"fmt"
+	"net/http"
 	"testing"
 	"time"
 
@@ -171,6 +173,24 @@ func TestPostBlock(t *testing.T) {
 		require.NotNil(t, block4)
 		require.Equal(t, "Updated title", block4.Title)
 	})
+
+	t.Run("Create a block with a malformed card property", func(t *testing.T) {
+		block := &model.Block{
+			ID:       utils.NewID(utils.IDTypeBlock),
+			BoardID:  board.ID,
+			CreateAt: 1,
+			UpdateAt: 1,
+			Type:     model.TypeCard,
+			Fields: map[string]interface{}{
+				"properties": map[string]interface{}{"property-id": map[string]interface{}{}},
+			},
+		}
+
+		newBlocks, resp := th.Client.InsertBlocks(board.ID, []*model.Block{block}, false)
+		require.Error(t, resp.Error)
+		require.Equal(t, http.StatusBadRequest, resp.StatusCode)
+		require.Empty(t, newBlocks)
+	})
 }
 
 func TestPatchBlock(t *testing.T) {
@@ -273,6 +293,30 @@ func TestPatchBlock(t *testing.T) {
 		require.Equal(t, "test value 2", updatedBlock.Fields["test2"])
 		require.Equal(t, nil, updatedBlock.Fields["test3"])
 	})
+
+	t.Run("Patch a block with a malformed card property", func(t *testing.T) {
+		blockPatch := &model.BlockPatch{
+			UpdatedFields: map[string]interface{}{
+				"properties": map[string]interface{}{"property-id": map[string]interface{}{}},
+			},
+		}
+
+		_, resp := th.Client.PatchBlock(board.ID, blockID, blockPatch, false)
+		require.Error(t, resp.Error)
+		require.Equal(t, http.StatusBadRequest, resp.StatusCode)
+	})
+
+	t.Run("Patch a batch of blocks with a malformed card property", func(t *testing.T) {
+		batch := fmt.Sprintf(
+			`{"block_ids":["%s"],"block_patches":[{"updatedFields":{"properties":{"property-id":{}}}}]}`,
+			blockID,
+		)
+
+		res, err := th.Client.DoAPIPatch("/boards/"+board.ID+"/blocks", batch)
+		require.Error(t, err)
+		require.NotNil(t, res)
+		require.Equal(t, http.StatusBadRequest, res.StatusCode)
+	})
 }
 
 func TestDeleteBlock(t *testing.T) {

server/integrationtests/board_test.go

@@ -5,6 +5,7 @@ package integrationtests
 
 import (
 	"encoding/json"
+	"net/http"
 	"os"
 	"sort"
 	"testing"
@@ -1033,6 +1034,66 @@ func TestPatchBoard(t *testing.T) {
 		require.Nil(t, rBoard)
 	})
 
+	t.Run("an empty patch body 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,
+		}
+		board, err := th.Server.App().CreateBoard(newBoard, user1.ID, true)
+		require.NoError(t, err)
+
+		res, err := th.Client.DoAPIPatch("/boards/"+board.ID, "null")
+		require.Error(t, err)
+		require.NotNil(t, res)
+		require.Equal(t, http.StatusBadRequest, res.StatusCode)
+	})
+
+	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/boards_and_blocks_test.go

@@ -700,6 +700,63 @@ func TestPatchBoardsAndBlocks(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, newTitle, rBlock2.Title)
 	})
+
+	t.Run("patches should be rejected if a block patch has a malformed card property", func(t *testing.T) {
+		th := SetupTestHelperPluginMode(t)
+		defer th.TearDown()
+
+		clients := setupClients(th)
+		th.Client = clients.TeamMember
+		teamID := mmModel.NewId()
+		userID := th.GetUser1().ID
+		initialTitle := "initial title"
+		newTitle := "new other title"
+
+		newBoard := &model.Board{
+			Title:  initialTitle,
+			TeamID: teamID,
+			Type:   model.BoardTypeOpen,
+		}
+		board, err := th.Server.App().CreateBoard(newBoard, userID, true)
+		require.NoError(t, err)
+		require.NotNil(t, board)
+
+		newBlock := &model.Block{
+			ID:      "block-id-1",
+			BoardID: board.ID,
+			Title:   initialTitle,
+			Fields:  map[string]interface{}{"properties": map[string]interface{}{"property-id": "82%"}},
+		}
+		require.NoError(t, th.Server.App().InsertBlock(newBlock, userID))
+		block, err := th.Server.App().GetBlockByID("block-id-1")
+		require.NoError(t, err)
+		require.NotNil(t, block)
+
+		pbab := &model.PatchBoardsAndBlocks{
+			BoardIDs: []string{board.ID},
+			BoardPatches: []*model.BoardPatch{
+				{Title: &newTitle},
+			},
+			BlockIDs: []string{block.ID},
+			BlockPatches: []*model.BlockPatch{
+				{UpdatedFields: map[string]interface{}{
+					"properties": map[string]interface{}{"property-id": map[string]interface{}{}},
+				}},
+			},
+		}
+
+		bab, resp := th.Client.PatchBoardsAndBlocks(pbab)
+		th.CheckBadRequest(resp)
+		require.Nil(t, bab)
+
+		// nothing should have been updated
+		rBoard, err := th.Server.App().GetBoard(board.ID)
+		require.NoError(t, err)
+		require.Equal(t, initialTitle, rBoard.Title)
+		rBlock, err := th.Server.App().GetBlockByID(block.ID)
+		require.NoError(t, err)
+		require.Equal(t, map[string]interface{}{"property-id": "82%"}, rBlock.Fields["properties"])
+	})
 }
 
 func TestDeleteBoardsAndBlocks(t *testing.T) {

server/integrationtests/cards_test.go

@@ -5,6 +5,7 @@ package integrationtests
 
 import (
 	"fmt"
+	"net/http"
 	"strconv"
 	"testing"
 
@@ -263,6 +264,52 @@ func TestPatchCard(t *testing.T) {
 		require.Error(t, resp.Error)
 		require.Nil(t, cardNew)
 	})
+
+	t.Run("an empty patch body 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]
+
+		res, err := th.Client.DoAPIPatch("/cards/"+card.ID, "null")
+		require.Error(t, err)
+		require.NotNil(t, res)
+		require.Equal(t, http.StatusBadRequest, res.StatusCode)
+	})
+
+	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) {