From bce202cb713cbce46967a9267bae420bbe814c14 Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Tue, 27 Jun 2023 11:56:03 +0300 Subject: [PATCH] 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 5dfd402e2679e6da32073aacb863867e6e64ee04) Co-authored-by: Miguel de la Cruz --- server/api/api_test.go | 2 + server/model/block.go | 37 ++++++++ server/model/error.go | 30 ++++-- server/services/store/sqlstore/blocks.go | 22 +---- .../services/store/sqlstore/legacy_blocks.go | 2 +- server/services/store/storetests/blocks.go | 95 +++++++++++++++++++ .../store/storetests/boards_and_blocks.go | 71 +++++++++++++- 7 files changed, 231 insertions(+), 28 deletions(-) diff --git a/server/api/api_test.go b/server/api/api_test.go index 27639a107db..74b90ffc8f6 100644 --- a/server/api/api_test.go +++ b/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"}, diff --git a/server/model/block.go b/server/model/block.go index 9e1658ec66a..538c64856ea 100644 --- a/server/model/block.go +++ b/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 { diff --git a/server/model/error.go b/server/model/error.go index 71af6cf4574..d85f599c1b5 100644 --- a/server/model/error.go +++ b/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: diff --git a/server/services/store/sqlstore/blocks.go b/server/services/store/sqlstore/blocks.go index 306815a18fd..a22daddb7a4 100644 --- a/server/services/store/sqlstore/blocks.go +++ b/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) diff --git a/server/services/store/sqlstore/legacy_blocks.go b/server/services/store/sqlstore/legacy_blocks.go index 842f2d258eb..418daa97a5e 100644 --- a/server/services/store/sqlstore/legacy_blocks.go +++ b/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) diff --git a/server/services/store/storetests/blocks.go b/server/services/store/storetests/blocks.go index bbd01ef72e2..50e659ac85f 100644 --- a/server/services/store/storetests/blocks.go +++ b/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) diff --git a/server/services/store/storetests/boards_and_blocks.go b/server/services/store/storetests/boards_and_blocks.go index f780005e63a..e5b715353d3 100644 --- a/server/services/store/storetests/boards_and_blocks.go +++ b/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{