Unify and enhance block validation (#4790) (#4793)

* Adds limit check for block titles

* Adds limit check for the aggregation of the fields

* Fix linter

* Adds tests

* Fix err check method order

(cherry picked from commit 5dfd402)

Co-authored-by: Miguel de la Cruz <[email protected]>

server/api/api_test.go

@@ -31,6 +31,8 @@ func TestErrorResponse(t *testing.T) {
 		{"ErrInvalidCategory", model.NewErrInvalidCategory("open"), http.StatusBadRequest, "open"},
 		{"ErrBoardMemberIsLastAdmin", model.ErrBoardMemberIsLastAdmin, http.StatusBadRequest, "no admins"},
 		{"ErrBoardIDMismatch", model.ErrBoardIDMismatch, http.StatusBadRequest, "Board IDs do not match"},
+		{"ErrBlockTitleSizeLimitExceeded", model.ErrBlockTitleSizeLimitExceeded, http.StatusBadRequest, "block title size limit exceeded"},
+		{"ErrBlockFieldsSizeLimitExceeded", model.ErrBlockFieldsSizeLimitExceeded, http.StatusBadRequest, "block fields size limit exceeded"},
 
 		// unauthorized
 		{"ErrUnauthorized", model.NewErrUnauthorized("not enough permissions"), http.StatusUnauthorized, "not enough permissions"},

server/model/block.go

@@ -2,12 +2,26 @@ package model
 
 import (
 	"encoding/json"
+	"errors"
 	"io"
 	"strconv"
+	"unicode/utf8"
 
 	"github.com/mattermost/focalboard/server/services/audit"
 )
 
+const (
+	BlockTitleMaxBytes  = 65535                  // Maximum size of a TEXT column in MySQL
+	BlockTitleMaxRunes  = BlockTitleMaxBytes / 4 // Assume a worst-case representation
+	BlockFieldsMaxRunes = 800000
+)
+
+var (
+	ErrBlockEmptyBoardID            = errors.New("boardID is empty")
+	ErrBlockTitleSizeLimitExceeded  = errors.New("block title size limit exceeded")
+	ErrBlockFieldsSizeLimitExceeded = errors.New("block fields size limit exceeded")
+)
+
 // Block is the basic data unit
 // swagger:model
 type Block struct {
@@ -124,6 +138,29 @@ func BlocksFromJSON(data io.Reader) []*Block {
 	return blocks
 }
 
+// IsValid checks the block for errors before inserting, and makes
+// sure it complies with the requirements of a valid block.
+func (b *Block) IsValid() error {
+	if b.BoardID == "" {
+		return ErrBlockEmptyBoardID
+	}
+
+	if utf8.RuneCountInString(b.Title) > BlockTitleMaxRunes {
+		return ErrBlockTitleSizeLimitExceeded
+	}
+
+	fieldsJSON, err := json.Marshal(b.Fields)
+	if err != nil {
+		return err
+	}
+
+	if utf8.RuneCountInString(string(fieldsJSON)) > BlockFieldsMaxRunes {
+		return ErrBlockFieldsSizeLimitExceeded
+	}
+
+	return nil
+}
+
 // LogClone implements the `mlog.LogCloner` interface to provide a subset of Block fields for logging.
 func (b *Block) LogClone() interface{} {
 	return struct {

server/model/error.go

@@ -166,7 +166,9 @@ func (ni *ErrNotImplemented) Error() string {
 // - model.ErrAuthParam
 // - model.ErrInvalidCategory
 // - model.ErrBoardMemberIsLastAdmin
-// - model.ErrBoardIDMismatch.
+// - model.ErrBoardIDMismatch
+// - model.ErrBlockTitleSizeLimitExceeded
+// - model.ErrBlockFieldsSizeLimitExceeded.
 func IsErrBadRequest(err error) bool {
 	if err == nil {
 		return false
@@ -178,14 +180,14 @@ func IsErrBadRequest(err error) bool {
 		return true
 	}
 
-	// check if this is a model.ErrAuthParam
-	var ap *ErrAuthParam
-	if errors.As(err, &ap) {
+	// check if this is a model.ErrViewsLimitReached
+	if errors.Is(err, ErrViewsLimitReached) {
 		return true
 	}
 
-	// check if this is a model.ErrViewsLimitReached
-	if errors.Is(err, ErrViewsLimitReached) {
+	// check if this is a model.ErrAuthParam
+	var ap *ErrAuthParam
+	if errors.As(err, &ap) {
 		return true
 	}
 
@@ -195,13 +197,23 @@ func IsErrBadRequest(err error) bool {
 		return true
 	}
 
-	// check if this is a model.ErrBoardIDMismatch
+	// check if this is a model.ErrBoardMemberIsLastAdmin
 	if errors.Is(err, ErrBoardMemberIsLastAdmin) {
 		return true
 	}
 
-	// check if this is a model.ErrBoardMemberIsLastAdmin
-	return errors.Is(err, ErrBoardIDMismatch)
+	// check if this is a model.ErrBoardIDMismatch
+	if errors.Is(err, ErrBoardIDMismatch) {
+		return true
+	}
+
+	// check if this is a model.ErrBlockTitleSizeLimitExceeded
+	if errors.Is(err, ErrBlockTitleSizeLimitExceeded) {
+		return true
+	}
+
+	// check if this is a model.ErrBlockTitleSizeLimitExceeded
+	return errors.Is(err, ErrBlockFieldsSizeLimitExceeded)
 }
 
 // IsErrUnauthorized returns true if `err` is or wraps one of:

server/services/store/sqlstore/blocks.go

@@ -20,18 +20,6 @@ const (
 	descClause     = " DESC "
 )
 
-type ErrEmptyBoardID struct{}
-
-func (re ErrEmptyBoardID) Error() string {
-	return "boardID is empty"
-}
-
-type ErrLimitExceeded struct{ max int }
-
-func (le ErrLimitExceeded) Error() string {
-	return fmt.Sprintf("limit exceeded (max=%d)", le.max)
-}
-
 func (s *SQLStore) timestampToCharField(name string, as string) string {
 	switch s.dbType {
 	case model.MysqlDBType:
@@ -240,8 +228,8 @@ func (s *SQLStore) blocksFromRows(rows *sql.Rows) ([]*model.Block, error) {
 }
 
 func (s *SQLStore) insertBlock(db sq.BaseRunner, block *model.Block, userID string) error {
-	if block.BoardID == "" {
-		return ErrEmptyBoardID{}
+	if err := block.IsValid(); err != nil {
+		return fmt.Errorf("error validating block %s: %w", block.ID, err)
 	}
 
 	fieldsJSON, err := json.Marshal(block.Fields)
@@ -348,8 +336,8 @@ func (s *SQLStore) patchBlocks(db sq.BaseRunner, blockPatches *model.BlockPatchB
 
 func (s *SQLStore) insertBlocks(db sq.BaseRunner, blocks []*model.Block, userID string) error {
 	for _, block := range blocks {
-		if block.BoardID == "" {
-			return ErrEmptyBoardID{}
+		if err := block.IsValid(); err != nil {
+			return fmt.Errorf("error validating block %s: %w", block.ID, err)
 		}
 	}
 	for i := range blocks {
@@ -1018,7 +1006,7 @@ func (s *SQLStore) deleteBlockChildren(db sq.BaseRunner, boardID string, parentI
 
 func (s *SQLStore) undeleteBlockChildren(db sq.BaseRunner, boardID string, parentID string, modifiedBy string) error {
 	if boardID == "" {
-		return ErrEmptyBoardID{}
+		return model.ErrBlockEmptyBoardID
 	}
 
 	where := fmt.Sprintf("board_id='%s'", boardID)

server/services/store/sqlstore/legacy_blocks.go

@@ -162,7 +162,7 @@ func (s *SQLStore) getLegacyBlock(db sq.BaseRunner, workspaceID string, blockID
 //nolint:unused
 func (s *SQLStore) insertLegacyBlock(db sq.BaseRunner, workspaceID string, block *model.Block, userID string) error {
 	if block.BoardID == "" {
-		return ErrEmptyBoardID{}
+		return model.ErrBlockEmptyBoardID
 	}
 
 	fieldsJSON, err := json.Marshal(block.Fields)

server/services/store/storetests/blocks.go

@@ -3,6 +3,7 @@ package storetests
 import (
 	"math"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -142,6 +143,35 @@ func testInsertBlock(t *testing.T, store store.Store) {
 		require.Len(t, blocks, initialCount+1)
 	})
 
+	t.Run("block with title too large", func(t *testing.T) {
+		block := &model.Block{
+			ID:         "id-test",
+			BoardID:    boardID,
+			ModifiedBy: userID,
+			Title:      strings.Repeat("A", model.BlockTitleMaxRunes+1),
+		}
+
+		err := store.InsertBlock(block, "user-id-1")
+		require.ErrorIs(t, err, model.ErrBlockTitleSizeLimitExceeded)
+	})
+
+	t.Run("block with aggregated fields size too large", func(t *testing.T) {
+		block := &model.Block{
+			ID:         "id-test",
+			BoardID:    boardID,
+			ModifiedBy: userID,
+			Fields: map[string]any{
+				"one":   strings.Repeat("1", model.BlockFieldsMaxRunes/4),
+				"two":   strings.Repeat("2", model.BlockFieldsMaxRunes/4),
+				"three": strings.Repeat("3", model.BlockFieldsMaxRunes/4),
+				"four":  strings.Repeat("4", model.BlockFieldsMaxRunes/4),
+			},
+		}
+
+		err := store.InsertBlock(block, "user-id-2")
+		require.ErrorIs(t, err, model.ErrBlockFieldsSizeLimitExceeded)
+	})
+
 	t.Run("insert new block", func(t *testing.T) {
 		block := &model.Block{
 			BoardID: testBoardID,
@@ -184,6 +214,71 @@ func testInsertBlock(t *testing.T, store store.Store) {
 		require.Equal(t, "New Title", newBlock.Title)
 	})
 
+	t.Run("update existing block with title too large", func(t *testing.T) {
+		block := &model.Block{
+			ID:        "id-3",
+			BoardID:   "board-id-1",
+			CreatedBy: "user-id-3",
+			Title:     "New Title",
+		}
+
+		// inserting
+		err := store.InsertBlock(block, "user-id-3")
+		require.NoError(t, err)
+
+		// created by populated from user id for new blocks
+		require.Equal(t, "user-id-3", block.CreatedBy)
+
+		// hack to avoid multiple, quick updates to a card
+		// violating block_history composite primary key constraint
+		time.Sleep(1 * time.Millisecond)
+
+		// updating
+		newBlock := &model.Block{
+			ID:        "id-3",
+			BoardID:   "board-id-1",
+			CreatedBy: "user-id-3",
+			Title:     strings.Repeat("A", model.BlockTitleMaxRunes+1),
+		}
+		err = store.InsertBlock(newBlock, "user-id-3")
+		require.ErrorIs(t, err, model.ErrBlockTitleSizeLimitExceeded)
+	})
+
+	t.Run("update existing block with aggregated fields size too large", func(t *testing.T) {
+		block := &model.Block{
+			ID:        "id-3",
+			BoardID:   "board-id-1",
+			CreatedBy: "user-id-3",
+			Title:     "New Title",
+		}
+
+		// inserting
+		err := store.InsertBlock(block, "user-id-3")
+		require.NoError(t, err)
+
+		// created by populated from user id for new blocks
+		require.Equal(t, "user-id-3", block.CreatedBy)
+
+		// hack to avoid multiple, quick updates to a card
+		// violating block_history composite primary key constraint
+		time.Sleep(1 * time.Millisecond)
+
+		// updating
+		newBlock := &model.Block{
+			ID:        "id-3",
+			BoardID:   "board-id-1",
+			CreatedBy: "user-id-3",
+			Fields: map[string]any{
+				"one":   strings.Repeat("1", model.BlockFieldsMaxRunes/4),
+				"two":   strings.Repeat("2", model.BlockFieldsMaxRunes/4),
+				"three": strings.Repeat("3", model.BlockFieldsMaxRunes/4),
+				"four":  strings.Repeat("4", model.BlockFieldsMaxRunes/4),
+			},
+		}
+		err = store.InsertBlock(newBlock, "user-id-3")
+		require.ErrorIs(t, err, model.ErrBlockFieldsSizeLimitExceeded)
+	})
+
 	createdAt, err := time.Parse(time.RFC822, "01 Jan 90 01:00 IST")
 	assert.NoError(t, err)
 

server/services/store/storetests/boards_and_blocks.go

@@ -2,6 +2,7 @@ package storetests
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
@@ -141,6 +142,30 @@ func testCreateBoardsAndBlocks(t *testing.T, store store.Store) {
 		require.Empty(t, bab)
 		require.Empty(t, members)
 	})
+
+	t.Run("should apply block size limits", func(t *testing.T) {
+		// one of the blocks is invalid as it has a title too large
+		newBab := &model.BoardsAndBlocks{
+			Boards: []*model.Board{
+				{ID: "board-id-7", TeamID: teamID, Type: model.BoardTypeOpen},
+				{ID: "board-id-8", TeamID: teamID, Type: model.BoardTypePrivate},
+				{ID: "board-id-9", TeamID: teamID, Type: model.BoardTypeOpen},
+			},
+			Blocks: []*model.Block{
+				{ID: "block-id-5", BoardID: "board-id-7", Type: model.TypeCard},
+				{ID: "block-id-6", BoardID: "board-id-8", Type: model.TypeCard, Title: strings.Repeat("A", model.BlockTitleMaxRunes+1)},
+			},
+		}
+
+		bab, err := store.CreateBoardsAndBlocks(newBab, userID)
+		require.ErrorIs(t, err, model.ErrBlockTitleSizeLimitExceeded)
+		require.Nil(t, bab)
+
+		bab, members, err := store.CreateBoardsAndBlocksWithAdmin(newBab, userID)
+		require.ErrorIs(t, err, model.ErrBlockTitleSizeLimitExceeded)
+		require.Empty(t, bab)
+		require.Empty(t, members)
+	})
 }
 
 func testPatchBoardsAndBlocks(t *testing.T, store store.Store) {
@@ -190,7 +215,7 @@ func testPatchBoardsAndBlocks(t *testing.T, store store.Store) {
 		require.Error(t, err)
 		require.Nil(t, bab)
 
-		// check that things have changed
+		// check that things have not changed
 		rBoard, err := store.GetBoard("board-id-1")
 		require.NoError(t, err)
 		require.Equal(t, initialTitle, rBoard.Title)
@@ -200,6 +225,50 @@ func testPatchBoardsAndBlocks(t *testing.T, store store.Store) {
 		require.Equal(t, initialTitle, rBlock.Title)
 	})
 
+	t.Run("should apply block size limits", func(t *testing.T) {
+		if store.DBType() == model.SqliteDBType {
+			t.Skip("No transactions support int sqlite")
+		}
+
+		initialTitle := "initial title"
+		newTitle := strings.Repeat("A", model.BlockTitleMaxRunes+1)
+
+		board := &model.Board{
+			ID:     "board-id-1",
+			Title:  initialTitle,
+			TeamID: teamID,
+			Type:   model.BoardTypeOpen,
+		}
+		_, err := store.InsertBoard(board, userID)
+		require.NoError(t, err)
+
+		block := &model.Block{
+			ID:      "block-id-1",
+			BoardID: "board-id-1",
+			Title:   initialTitle,
+		}
+		require.NoError(t, store.InsertBlock(block, userID))
+
+		// apply the patches
+		pbab := &model.PatchBoardsAndBlocks{
+			BlockIDs: []string{"block-id-1"},
+			BlockPatches: []*model.BlockPatch{
+				{Title: &newTitle},
+			},
+		}
+
+		time.Sleep(10 * time.Millisecond)
+
+		bab, err := store.PatchBoardsAndBlocks(pbab, userID)
+		require.ErrorIs(t, err, model.ErrBlockTitleSizeLimitExceeded)
+		require.Nil(t, bab)
+
+		// check that things have not changed
+		rBlock, err := store.GetBlock("block-id-1")
+		require.NoError(t, err)
+		require.Equal(t, initialTitle, rBlock.Title)
+	})
+
 	t.Run("patch boards and blocks", func(t *testing.T) {
 		newBab := &model.BoardsAndBlocks{
 			Boards: []*model.Board{