1
0
mirror of https://github.com/mattermost/focalboard.git synced 2024-12-21 13:38:56 +02:00

Cherry pick enhance validation 7.8 (#4821)

* Unify and enhance block validation (#4790)

* Adds limit check for block titles

* Adds limit check for the aggregation of the fields

* Fix linter

* Adds tests

* Fix err check method order

* Updates import to read with a scanner (#4788)

* Updates import to read with a scanner

* Fix linter

* Fix migration test
This commit is contained in:
Miguel de la Cruz 2023-07-28 12:54:38 +02:00 committed by GitHub
parent d8af5c3975
commit 9ec1e42f34
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 251 additions and 49 deletions

View File

@ -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"},

View File

@ -20,12 +20,14 @@ import (
)
const (
archiveVersion = 2
legacyFileBegin = "{\"version\":1"
archiveVersion = 2
legacyFileBegin = "{\"version\":1"
importMaxFileSize = 1024 * 1024 * 70
)
var (
errBlockIsNotABoard = errors.New("block is not a board")
errBlockIsNotABoard = errors.New("block is not a board")
errSizeLimitExceeded = errors.New("size limit exceeded")
)
// ImportArchive imports an archive containing zero or more boards, plus all
@ -129,7 +131,8 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (str
Blocks: make([]*model.Block, 0, 10),
Boards: make([]*model.Board, 0, 10),
}
lineReader := bufio.NewReader(r)
lineReader := &io.LimitedReader{R: r, N: importMaxFileSize + 1}
scanner := bufio.NewScanner(lineReader)
userID := opt.ModifiedBy
if userID == model.SingleUser {
@ -141,8 +144,12 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (str
lineNum := 1
firstLine := true
for {
line, errRead := readLine(lineReader)
for scanner.Scan() {
if lineReader.N <= 0 {
return "", fmt.Errorf("error parsing archive line %d: %w", lineNum, errSizeLimitExceeded)
}
line := bytes.TrimSpace(scanner.Bytes())
if len(line) != 0 {
var skip bool
if firstLine {
@ -209,14 +216,10 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (str
firstLine = false
}
}
}
if errRead != nil {
if errors.Is(errRead, io.EOF) {
break
}
return "", fmt.Errorf("error reading archive line %d: %w", lineNum, errRead)
}
lineNum++
if errRead := scanner.Err(); errRead != nil {
return "", fmt.Errorf("error reading archive line %d: %w", lineNum, errRead)
}
// loop to remove the people how are not part of the team and system
@ -438,9 +441,3 @@ func parseVersionFile(r io.Reader) (int, error) {
}
return header.Version, nil
}
func readLine(r *bufio.Reader) ([]byte, error) {
line, err := r.ReadBytes('\n')
line = bytes.TrimSpace(line)
return line, err
}

View File

@ -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 {

View File

@ -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:

View File

@ -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 {
@ -1022,7 +1010,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)

View File

@ -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)

View File

@ -10,9 +10,11 @@ func Test40FixFileinfoSoftDeletes(t *testing.T) {
th, tearDown := SetupPluginTestHelper(t)
defer tearDown()
th.f.MigrateToStep(39).
// Migrations are 38 and 39 for tests not to fail, as migration 39
// doesn't exists for this release
th.f.MigrateToStep(38).
ExecFile("./fixtures/test40FixFileinfoSoftDeletes.sql").
MigrateToStep(40)
MigrateToStep(39)
type FileInfo struct {
Id string

View File

@ -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)

View File

@ -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{