diff --git a/server/api/blocks.go b/server/api/blocks.go index 7ba5ab8b4..28a658d9e 100644 --- a/server/api/blocks.go +++ b/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)) diff --git a/server/api/boards.go b/server/api/boards.go index 0a4632383..94d1ba82d 100644 --- a/server/api/boards.go +++ b/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 diff --git a/server/api/boards_and_blocks.go b/server/api/boards_and_blocks.go index ffc20ac4b..800a1a36e 100644 --- a/server/api/boards_and_blocks.go +++ b/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 diff --git a/server/api/cards.go b/server/api/cards.go index 18689ee5c..0fbfa130c 100644 --- a/server/api/cards.go +++ b/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) diff --git a/server/app/blocks.go b/server/app/blocks.go index 050b45144..f976b46f6 100644 --- a/server/app/blocks.go +++ b/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 diff --git a/server/app/boards_and_blocks.go b/server/app/boards_and_blocks.go index 5e2606834..baf3ba010 100644 --- a/server/app/boards_and_blocks.go +++ b/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 diff --git a/server/integrationtests/blocks_test.go b/server/integrationtests/blocks_test.go index 5f2304948..b579e1437 100644 --- a/server/integrationtests/blocks_test.go +++ b/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) { diff --git a/server/integrationtests/board_test.go b/server/integrationtests/board_test.go index 40bae8886..8bf6d3c13 100644 --- a/server/integrationtests/board_test.go +++ b/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() diff --git a/server/integrationtests/boards_and_blocks_test.go b/server/integrationtests/boards_and_blocks_test.go index e91efc162..d3e61f1e3 100644 --- a/server/integrationtests/boards_and_blocks_test.go +++ b/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) { diff --git a/server/integrationtests/cards_test.go b/server/integrationtests/cards_test.go index f5c7d9ba3..5c53b7a1e 100644 --- a/server/integrationtests/cards_test.go +++ b/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) { diff --git a/server/model/block.go b/server/model/block.go index 194c97aac..4d509f817 100644 --- a/server/model/block.go +++ b/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 } @@ -229,6 +230,20 @@ func ValidateFileId(id string) error { } } +func ValidateBlockProperties(block *Block) error { + propsIface, present := block.Fields[BlockFieldProperties] + if !present { + return nil + } + + props, ok := propsIface.(map[string]interface{}) + if !ok { + return ErrBlockPropertiesInvalidType + } + + return ValidateCardPropertyValues(props) +} + func ValidateBlockPatch(patch *BlockPatch) error { // Validate UpdatedFields map if patch.UpdatedFields != nil { @@ -264,6 +279,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 diff --git a/server/model/board.go b/server/model/board.go index 50571e243..4f607dd67 100644 --- a/server/model/board.go +++ b/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() } diff --git a/server/model/card.go b/server/model/card.go index a87e0edf6..79dae0d0c 100644 --- a/server/model/card.go +++ b/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. diff --git a/server/model/card_property.go b/server/model/card_property.go new file mode 100644 index 000000000..43337b93c --- /dev/null +++ b/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 +} diff --git a/server/model/card_property_test.go b/server/model/card_property_test.go new file mode 100644 index 000000000..13a18399e --- /dev/null +++ b/server/model/card_property_test.go @@ -0,0 +1,131 @@ +// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidateCardPropertyValues(t *testing.T) { + testCases := []struct { + name string + values map[string]any + valid bool + }{ + {"string value", map[string]any{"prop-id": "82%"}, true}, + {"array of strings value", map[string]any{"prop-id": []any{"opt-1", "opt-2"}}, true}, + {"empty value", map[string]any{"prop-id": nil}, true}, + {"object value", map[string]any{"prop-id": map[string]any{}}, false}, + {"array with an object value", map[string]any{"prop-id": []any{"opt-1", map[string]any{}}}, false}, + {"number value", map[string]any{"prop-id": float64(82)}, false}, + {"boolean value", map[string]any{"prop-id": true}, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCardPropertyValues(tc.values) + if tc.valid { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +func TestValidateCardPropertyTemplate(t *testing.T) { + testCases := []struct { + name string + template map[string]any + valid bool + }{ + { + "complete template", + map[string]any{ + "id": "prop-id", + "name": "Note", + "type": "text", + "options": []any{map[string]any{"id": "opt-id", "value": "Done", "color": "propColorGreen"}}, + }, + true, + }, + {"only an id", map[string]any{"id": "prop-id"}, true}, + {"missing id", map[string]any{"name": "Note"}, false}, + {"empty id", map[string]any{"id": ""}, false}, + {"object name", map[string]any{"id": "prop-id", "name": map[string]any{}}, false}, + {"object type", map[string]any{"id": "prop-id", "type": map[string]any{}}, false}, + {"object options", map[string]any{"id": "prop-id", "options": map[string]any{}}, false}, + {"option with an object value", map[string]any{"id": "prop-id", "options": []any{map[string]any{"id": "opt-id", "value": map[string]any{}}}}, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCardPropertyTemplate(tc.template) + if tc.valid { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +func TestBoardPatchWithInvalidCardProperties(t *testing.T) { + patch := &BoardPatch{ + UpdatedCardProperties: []map[string]any{ + {"id": "prop-id", "name": map[string]any{}, "type": "text", "options": []any{}}, + }, + } + + t.Run("is rejected by the patch validation", func(t *testing.T) { + require.Error(t, patch.IsValid()) + }) + + t.Run("is not merged into the board", func(t *testing.T) { + board := &Board{ + CardProperties: []map[string]any{ + {"id": "prop-id", "name": "Note", "type": "text", "options": []any{}}, + }, + } + + patched := patch.Patch(board) + require.Equal(t, "Note", patched.CardProperties[0]["name"]) + }) +} + +func TestCardPatchWithInvalidProperties(t *testing.T) { + patch := &CardPatch{ + UpdatedProperties: map[string]any{"prop-id": map[string]any{}}, + } + + t.Run("is rejected by the patch validation", func(t *testing.T) { + require.Error(t, patch.CheckValid()) + }) + + t.Run("is not merged into the card", func(t *testing.T) { + card := &Card{Properties: map[string]any{"prop-id": "82%"}} + + patched := patch.Patch(card) + require.Equal(t, "82%", patched.Properties["prop-id"]) + }) +} + +func TestValidateBlockPatchProperties(t *testing.T) { + t.Run("valid properties", func(t *testing.T) { + patch := &BlockPatch{UpdatedFields: map[string]any{"properties": map[string]any{"prop-id": "82%"}}} + require.NoError(t, ValidateBlockPatch(patch)) + }) + + t.Run("object property value", func(t *testing.T) { + patch := &BlockPatch{UpdatedFields: map[string]any{"properties": map[string]any{"prop-id": map[string]any{}}}} + require.Error(t, ValidateBlockPatch(patch)) + }) + + t.Run("properties is not an object", func(t *testing.T) { + patch := &BlockPatch{UpdatedFields: map[string]any{"properties": "not-an-object"}} + require.Error(t, ValidateBlockPatch(patch)) + }) +} diff --git a/webapp/src/blocks/board.test.ts b/webapp/src/blocks/board.test.ts index 1740d940f..edc0555c4 100644 --- a/webapp/src/blocks/board.test.ts +++ b/webapp/src/blocks/board.test.ts @@ -3,7 +3,7 @@ import {TestBlockFactory} from '../test/testBlockFactory' -import {createPatchesFromBoards, createBoard, IPropertyTemplate, createPatchesFromBoardsAndBlocks} from './board' +import {createPatchesFromBoards, createBoard, IPropertyTemplate, createPatchesFromBoardsAndBlocks, safePropertyString, safePropertyValue} from './board' import {createBlock} from './block' describe('board tests', () => { @@ -118,4 +118,20 @@ describe('board tests', () => { expect(result).toMatchSnapshot() }) }) + + describe('safely coerce malformed card properties', () => { + it('should keep strings and arrays of strings', () => { + expect(safePropertyString('Note')).toBe('Note') + expect(safePropertyValue('82%')).toBe('82%') + expect(safePropertyValue(['opt-1', 'opt-2'])).toEqual(['opt-1', 'opt-2']) + }) + + it('should discard values that are not renderable', () => { + expect(safePropertyString({})).toBe('') + expect(safePropertyString(undefined)).toBe('') + expect(safePropertyValue({})).toBe('') + expect(safePropertyValue(['opt-1', {}])).toBe('') + expect(safePropertyValue(undefined)).toBe('') + }) + }) }) diff --git a/webapp/src/blocks/board.ts b/webapp/src/blocks/board.ts index b9ea389c5..5c6c9164e 100644 --- a/webapp/src/blocks/board.ts +++ b/webapp/src/blocks/board.ts @@ -100,6 +100,21 @@ interface IPropertyTemplate { options: IPropertyOption[] } +// A malformed record must not break the rendering of the board or card using it. +function safePropertyString(value: unknown): string { + return typeof value === 'string' ? value : '' +} + +function safePropertyValue(value: unknown): string | string[] { + if (typeof value === 'string') { + return value + } + if (Array.isArray(value) && value.every((item) => typeof item === 'string')) { + return value + } + return '' +} + function createBoard(board?: Board): Board { const now = Date.now() let cardProperties: IPropertyTemplate[] = [] @@ -332,4 +347,6 @@ export { createPatchesFromBoards, createPatchesFromBoardsAndBlocks, createCardPropertiesPatches, + safePropertyString, + safePropertyValue, } diff --git a/webapp/src/components/cardDetail/cardDetailProperties.tsx b/webapp/src/components/cardDetail/cardDetailProperties.tsx index 8d6cd5bab..3a6b70442 100644 --- a/webapp/src/components/cardDetail/cardDetailProperties.tsx +++ b/webapp/src/components/cardDetail/cardDetailProperties.tsx @@ -4,7 +4,7 @@ import React, {useEffect, useState} from 'react' import {FormattedMessage, useIntl} from 'react-intl' -import {Board, IPropertyTemplate} from '../../blocks/board' +import {Board, IPropertyTemplate, safePropertyString} from '../../blocks/board' import {Card} from '../../blocks/card' import {BoardView} from '../../blocks/boardView' @@ -79,7 +79,7 @@ const CardDetailProperties = (props: Props) => { defaultMessage: 'Are you sure you want to change property "{propertyName}" {customText}? This will affect value(s) across {numOfCards} card(s) in this board, and can result in data loss.', }, { - propertyName: propertyTemplate.name, + propertyName: safePropertyString(propertyTemplate.name), customText: subTextString, numOfCards: affectsNumOfCards, }), @@ -109,10 +109,10 @@ const CardDetailProperties = (props: Props) => { id: 'CardDetailProperty.confirm-delete-subtext', defaultMessage: 'Are you sure you want to delete the property "{propertyName}"? Deleting it will delete the property from all cards in this board.', }, - {propertyName: propertyTemplate.name}), + {propertyName: safePropertyString(propertyTemplate.name)}), confirmButtonText: intl.formatMessage({id: 'CardDetailProperty.delete-action-button', defaultMessage: 'Delete'}), onConfirm: async () => { - const deletingPropName = propertyTemplate.name + const deletingPropName = safePropertyString(propertyTemplate.name) setShowConfirmationDialog(false) try { await mutator.deleteProperty(board, views, cards, propertyTemplate.id) @@ -132,18 +132,19 @@ const CardDetailProperties = (props: Props) => { return (
{board.cardProperties.map((propertyTemplate: IPropertyTemplate) => { + const propertyName = safePropertyString(propertyTemplate.name) return (
- {(props.readonly || !canEditBoardProperties) &&
{propertyTemplate.name}
} + {(props.readonly || !canEditBoardProperties) &&
{propertyName}
} {!props.readonly && canEditBoardProperties && -
+
onPropertyChangeSetAndOpenConfirmationDialog(newType, newName, propertyTemplate)} onDelete={() => onPropertyDeleteSetAndOpenConfirmationDialog(propertyTemplate)} diff --git a/webapp/src/components/propertyValueElement.tsx b/webapp/src/components/propertyValueElement.tsx index 80ec32e4c..79bc9886b 100644 --- a/webapp/src/components/propertyValueElement.tsx +++ b/webapp/src/components/propertyValueElement.tsx @@ -4,7 +4,7 @@ import React from 'react' -import {Board, IPropertyTemplate} from '../blocks/board' +import {Board, IPropertyTemplate, safePropertyValue} from '../blocks/board' import {Card} from '../blocks/card' import propsRegistry from '../properties' @@ -20,10 +20,7 @@ type Props = { const PropertyValueElement = (props: Props): JSX.Element => { const {card, propertyTemplate, readOnly, showEmptyPlaceholder, board} = props - let propertyValue = card.fields.properties[propertyTemplate.id] - if (propertyValue === undefined) { - propertyValue = '' - } + const propertyValue = safePropertyValue(card.fields.properties[propertyTemplate.id]) const property = propsRegistry.get(propertyTemplate.type) const Editor = property.Editor return ( diff --git a/webapp/src/components/table/tableHeaders.tsx b/webapp/src/components/table/tableHeaders.tsx index 4bf2d2759..681c4e193 100644 --- a/webapp/src/components/table/tableHeaders.tsx +++ b/webapp/src/components/table/tableHeaders.tsx @@ -5,7 +5,7 @@ import React, {useCallback, useMemo} from 'react' import {FormattedMessage, useIntl} from 'react-intl' -import {IPropertyTemplate, Board} from '../../blocks/board' +import {IPropertyTemplate, Board, safePropertyString} from '../../blocks/board' import {createBoardView, BoardView, ISortOption} from '../../blocks/boardView' import {Card} from '../../blocks/card' import {Constants} from '../../constants' @@ -139,7 +139,7 @@ const TableHeaders = (props: Props): JSX.Element => { } return ( boolean, spellCheck?: boolean}): JSX.Element => { - const [value, setValue] = useState(props.card.fields.properties[props.propertyTemplate.id || ''] || '') - const onCancel = useCallback(() => setValue(props.propertyValue || ''), [props.propertyValue]) + const [value, setValue] = useState(safePropertyString(props.card.fields.properties[props.propertyTemplate.id || ''])) + const onCancel = useCallback(() => setValue(safePropertyString(props.propertyValue)), [props.propertyValue]) const saveTextProperty = useCallback(() => { - if (value !== (props.card.fields.properties[props.propertyTemplate?.id || ''] || '')) { + if (value !== safePropertyString(props.card.fields.properties[props.propertyTemplate?.id || ''])) { mutator.changePropertyValue(props.board.id, props.card, props.propertyTemplate?.id || '', value) } }, [props.board.id, props.card, props.propertyTemplate?.id, value]) @@ -42,7 +43,7 @@ const BaseTextEditor = (props: PropertyProps & {validator: () => boolean, spellC { return <> } - const [value, setValue] = useState(props.card.fields.properties[props.propertyTemplate.id || ''] || '') + const [value, setValue] = useState(safePropertyString(props.card.fields.properties[props.propertyTemplate.id || ''])) const [isEditing, setIsEditing] = useState(false) - const isEmpty = !(props.propertyValue as string)?.trim() + const propertyValue = safePropertyString(props.propertyValue) + const isEmpty = !propertyValue.trim() const showEditable = !props.readOnly && (isEditing || isEmpty) const editableRef = useRef(null) const intl = useIntl() @@ -33,7 +35,7 @@ const URLProperty = (props: PropertyProps): JSX.Element => { const emptyDisplayValue = props.showEmptyPlaceholder ? intl.formatMessage({id: 'PropertyValueElement.empty', defaultMessage: 'Empty'}) : '' const saveTextProperty = useCallback(() => { - if (value !== (props.card.fields.properties[props.propertyTemplate?.id || ''] || '')) { + if (value !== safePropertyString(props.card.fields.properties[props.propertyTemplate?.id || ''])) { mutator.changePropertyValue(props.board.id, props.card, props.propertyTemplate?.id || '', value) } }, [props.board.id, props.card, props.propertyTemplate?.id, value]) @@ -63,7 +65,7 @@ const URLProperty = (props: PropertyProps): JSX.Element => { className={props.property.valueClassName(props.readOnly)} ref={editableRef} placeholderText={emptyDisplayValue} - value={value as string} + value={value} autoExpand={true} readonly={props.readOnly} onChange={setValue} @@ -73,7 +75,7 @@ const URLProperty = (props: PropertyProps): JSX.Element => { }} onCancel={() => { setIsEditing(false) - setValue(props.propertyValue || '') + setValue(propertyValue) }} onFocus={() => { setIsEditing(true) @@ -83,7 +85,7 @@ const URLProperty = (props: PropertyProps): JSX.Element => { return true } const urlRegexp = /(((.+:(?:\/\/)?)?(?:[-;:&=+$,\w]+@)?[A-Za-z0-9.-]+|(?:www\.|[-;:&=+$,\w]+@)[A-Za-z0-9.-]+)((?:\/[+~%/.\w\-_]*)?\??(?:[-+=&;%@.\w_]*)#?(?:[.!/\\\w]*))?)/ - return urlRegexp.test(value as string) + return urlRegexp.test(value) }} />
@@ -94,12 +96,12 @@ const URLProperty = (props: PropertyProps): JSX.Element => {
event.stopPropagation()} > - {props.propertyValue} + {propertyValue} {!props.readOnly && { icon={} onClick={(e) => { e.stopPropagation() - Utils.copyTextToClipboard(props.propertyValue as string) + Utils.copyTextToClipboard(propertyValue) sendFlashMessage({content: intl.formatMessage({id: 'URLProperty.copiedLink', defaultMessage: 'Copied!'}), severity: 'high'}) }} />