Skip to content

Commit 025ce8d

Browse files
Rajat-Dabademattermost-build
Rajat-Dabade
and
mattermost-build
authored
Added Validation for block patch (#56)
* Added Validation for block patch * linter fixes * Updated code * linter fixes * Added validation for create block * linter fixes * validation for attachment id * linter fixes * review comments * required changes after merging main --------- Co-authored-by: Mattermost Build <[email protected]>
1 parent 47f29e1 commit 025ce8d

File tree

8 files changed

+388
-24
lines changed

8 files changed

+388
-24
lines changed

‎server/api/blocks.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,11 @@ func (a *API) handlePostBlocks(w http.ResponseWriter, r *http.Request) {
250250
hasContents = true
251251
}
252252

253+
if err = block.IsValid(); err != nil {
254+
a.errorResponse(w, r, err)
255+
return
256+
}
257+
253258
if block.CreateAt < 1 {
254259
message := fmt.Sprintf("invalid createAt for block id %s", block.ID)
255260
a.errorResponse(w, r, model.NewErrBadRequest(message))

‎server/app/blocks.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,10 @@ func (a *App) PatchBlock(blockID string, blockPatch *model.BlockPatch, modifiedB
6262
}
6363

6464
func (a *App) PatchBlockAndNotify(blockID string, blockPatch *model.BlockPatch, modifiedByID string, disableNotify bool) (*model.Block, error) {
65+
if err := model.ValidateBlockPatch(blockPatch); err != nil {
66+
return nil, err
67+
}
68+
6569
oldBlock, err := a.store.GetBlock(blockID)
6670
if err != nil {
6771
return nil, err
@@ -99,6 +103,12 @@ func (a *App) PatchBlockAndNotify(blockID string, blockPatch *model.BlockPatch,
99103
}
100104

101105
func (a *App) PatchBlocks(teamID string, blockPatches *model.BlockPatchBatch, modifiedByID string) error {
106+
for _, patch := range blockPatches.BlockPatches {
107+
err := model.ValidateBlockPatch(&patch)
108+
if err != nil {
109+
return err
110+
}
111+
}
102112
return a.PatchBlocksAndNotify(teamID, blockPatches, modifiedByID, false)
103113
}
104114

‎server/app/files.go‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -202,14 +202,34 @@ func (a *App) CopyAndUpdateCardFiles(boardID, userID string, blocks []*model.Blo
202202
blockPatches := make([]model.BlockPatch, 0)
203203
for _, block := range blocks {
204204
if block.Type == model.TypeImage || block.Type == model.TypeAttachment {
205-
if fileID, ok := block.Fields["fileId"].(string); ok {
206-
blockIDs = append(blockIDs, block.ID)
207-
blockPatches = append(blockPatches, model.BlockPatch{
208-
UpdatedFields: map[string]interface{}{
209-
"fileId": newFileNames[fileID],
210-
},
211-
DeletedFields: []string{"attachmentId"},
212-
})
205+
if fileID, ok := block.Fields[model.BlockFieldFileId].(string); ok {
206+
if err = model.ValidateFileId(fileID); err == nil {
207+
blockIDs = append(blockIDs, block.ID)
208+
blockPatches = append(blockPatches, model.BlockPatch{
209+
UpdatedFields: map[string]interface{}{
210+
model.BlockFieldFileId: newFileNames[fileID],
211+
},
212+
DeletedFields: []string{model.BlockFieldAttachmentId},
213+
})
214+
} else {
215+
errMessage := fmt.Sprintf("invalid characters in block with key: %s, %s", block.Fields[model.BlockFieldFileId], err)
216+
return model.NewErrBadRequest(errMessage)
217+
}
218+
}
219+
220+
if attachmentID, ok := block.Fields[model.BlockFieldAttachmentId].(string); ok {
221+
if err = model.ValidateFileId(attachmentID); err == nil {
222+
blockIDs = append(blockIDs, block.ID)
223+
blockPatches = append(blockPatches, model.BlockPatch{
224+
UpdatedFields: map[string]interface{}{
225+
model.BlockFieldAttachmentId: newFileNames[attachmentID],
226+
},
227+
DeletedFields: []string{model.BlockFieldFileId},
228+
})
229+
} else {
230+
errMessage := fmt.Sprintf("invalid characters in block with key: %s, %s", block.Fields[model.BlockFieldAttachmentId], err)
231+
return model.NewErrBadRequest(errMessage)
232+
}
213233
}
214234
}
215235
}
@@ -256,6 +276,11 @@ func (a *App) CopyCardFiles(sourceBoardID string, copiedBlocks []*model.Block, a
256276
}
257277
}
258278

279+
if err = model.ValidateFileId(fileID); err != nil {
280+
errMessage := fmt.Sprintf("Could not validate file ID while duplicating board with fileId: %s", fileID)
281+
return nil, model.NewErrBadRequest(errMessage)
282+
}
283+
259284
// create unique filename
260285
ext := filepath.Ext(fileID)
261286
fileInfoID := utils.NewID(utils.IDTypeNone)

‎server/app/files_test.go‎

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,52 @@ func TestCopyAndUpdateCardFiles(t *testing.T) {
524524
Schema: 1,
525525
Type: "image",
526526
Title: "",
527-
Fields: map[string]interface{}{"fileId": "7fileName.jpg"},
527+
Fields: map[string]interface{}{"fileId": "7fileName1234567890987654321.jpg"},
528+
CreateAt: 1680725585250,
529+
UpdateAt: 1680725585250,
530+
DeleteAt: 0,
531+
BoardID: "boardID",
532+
}
533+
534+
validImageBlock := &model.Block{
535+
ID: "validImageBlock",
536+
ParentID: "c3zqnh6fsu3f4mr6hzq9hizwske",
537+
CreatedBy: "6k6ynxdp47dujjhhojw9nqhmyh",
538+
ModifiedBy: "6k6ynxdp47dujjhhojw9nqhmyh",
539+
Schema: 1,
540+
Type: "image",
541+
Title: "",
542+
Fields: map[string]interface{}{"fileId": "7xhwgf5r15fr3dryfozf1dmy41r9.png"},
543+
CreateAt: 1680725585250,
544+
UpdateAt: 1680725585250,
545+
DeleteAt: 0,
546+
BoardID: "boardID",
547+
}
548+
549+
invalidShortFileIDBlock := &model.Block{
550+
ID: "invalidShortFileIDBlock",
551+
ParentID: "c3zqnh6fsu3f4mr6hzq9hizwske",
552+
CreatedBy: "6k6ynxdp47dujjhhojw9nqhmyh",
553+
ModifiedBy: "6k6ynxdp47dujjhhojw9nqhmyh",
554+
Schema: 1,
555+
Type: "image",
556+
Title: "",
557+
Fields: map[string]interface{}{"fileId": "7short.png"},
558+
CreateAt: 1680725585250,
559+
UpdateAt: 1680725585250,
560+
DeleteAt: 0,
561+
BoardID: "boardID",
562+
}
563+
564+
emptyFileBlock := &model.Block{
565+
ID: "emptyFileBlock",
566+
ParentID: "c3zqnh6fsu3f4mr6hzq9hizwske",
567+
CreatedBy: "6k6ynxdp47dujjhhojw9nqhmyh",
568+
ModifiedBy: "6k6ynxdp47dujjhhojw9nqhmyh",
569+
Schema: 1,
570+
Type: "image",
571+
Title: "",
572+
Fields: map[string]interface{}{"fileId": ""},
528573
CreateAt: 1680725585250,
529574
UpdateAt: 1680725585250,
530575
DeleteAt: 0,
@@ -553,4 +598,70 @@ func TestCopyAndUpdateCardFiles(t *testing.T) {
553598

554599
assert.NotEqual(t, testPath, imageBlock.Fields["fileId"])
555600
})
601+
602+
t.Run("Valid file ID", func(t *testing.T) {
603+
fileInfo := &mm_model.FileInfo{
604+
Id: "validImageBlock",
605+
Path: testPath,
606+
}
607+
th.Store.EXPECT().GetBoard("boardID").Return(&model.Board{ID: "boardID", IsTemplate: false}, nil)
608+
th.Store.EXPECT().GetFileInfo("xhwgf5r15fr3dryfozf1dmy41r9").Return(fileInfo, nil)
609+
th.Store.EXPECT().SaveFileInfo(fileInfo).Return(nil)
610+
th.Store.EXPECT().PatchBlocks(gomock.Any(), "userID").Return(nil)
611+
612+
mockedFileBackend := &mocks.FileBackend{}
613+
th.App.filesBackend = mockedFileBackend
614+
mockedFileBackend.On("CopyFile", mock.Anything, mock.Anything).Return(nil)
615+
616+
err := th.App.CopyAndUpdateCardFiles("boardID", "userID", []*model.Block{validImageBlock}, false)
617+
assert.NoError(t, err)
618+
})
619+
620+
t.Run("Invalid file ID length", func(t *testing.T) {
621+
th.Store.EXPECT().GetBoard("boardID").Return(&model.Board{ID: "boardID", IsTemplate: false}, nil)
622+
err := th.App.CopyAndUpdateCardFiles("boardID", "userID", []*model.Block{invalidShortFileIDBlock}, false)
623+
assert.ErrorIs(t, err, model.NewErrBadRequest("Invalid Block ID"))
624+
})
625+
626+
t.Run("Empty file ID", func(t *testing.T) {
627+
th.Store.EXPECT().GetBoard("boardID").Return(&model.Board{ID: "boardID", IsTemplate: false}, nil)
628+
err := th.App.CopyAndUpdateCardFiles("boardID", "userID", []*model.Block{emptyFileBlock}, false)
629+
assert.ErrorIs(t, err, model.NewErrBadRequest("Block ID cannot be empty"))
630+
})
631+
}
632+
633+
func TestCopyCardFiles(t *testing.T) {
634+
app := &App{}
635+
636+
t.Run("ValidFileID", func(t *testing.T) {
637+
sourceBoardID := "sourceBoardID"
638+
copiedBlocks := []*model.Block{
639+
{
640+
Type: model.TypeImage,
641+
Fields: map[string]interface{}{"fileId": "validFileID"},
642+
BoardID: "destinationBoardID",
643+
},
644+
}
645+
646+
newFileNames, err := app.CopyCardFiles(sourceBoardID, copiedBlocks, false)
647+
648+
assert.NoError(t, err)
649+
assert.NotNil(t, newFileNames)
650+
})
651+
652+
t.Run("InvalidFileID", func(t *testing.T) {
653+
sourceBoardID := "sourceBoardID"
654+
copiedBlocks := []*model.Block{
655+
{
656+
Type: model.TypeImage,
657+
Fields: map[string]interface{}{"fileId": "../../../../../filePath"},
658+
BoardID: "destinationBoardID",
659+
},
660+
}
661+
662+
newFileNames, err := app.CopyCardFiles(sourceBoardID, copiedBlocks, false)
663+
664+
assert.Error(t, err)
665+
assert.Nil(t, newFileNames)
666+
})
556667
}

‎server/app/import.go‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ func (a *App) ImportArchive(r io.Reader, opt model.ImportArchiveOptions) error {
8282
boardMap[dir] = board
8383
default:
8484
// import file/image; dir is the old board id
85+
8586
board, ok := boardMap[dir]
8687
if !ok {
8788
a.logger.Warn("skipping orphan image in archive",
@@ -208,6 +209,9 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (*mo
208209
if err2 := json.Unmarshal(archiveLine.Data, &block); err2 != nil {
209210
return nil, fmt.Errorf("invalid board block in archive line %d: %w", lineNum, err2)
210211
}
212+
if err := block.IsValid(); err != nil {
213+
return nil, err
214+
}
211215
block.ModifiedBy = userID
212216
block.UpdateAt = now
213217
board, err := a.blockToBoard(block, opt)
@@ -221,6 +225,9 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (*mo
221225
if err2 := json.Unmarshal(archiveLine.Data, &block); err2 != nil {
222226
return nil, fmt.Errorf("invalid block in archive line %d: %w", lineNum, err2)
223227
}
228+
if err := block.IsValid(); err != nil {
229+
return nil, err
230+
}
224231
block.ModifiedBy = userID
225232
block.UpdateAt = now
226233
block.BoardID = boardID
@@ -263,6 +270,18 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (*mo
263270
return nil, fmt.Errorf("error inserting archive blocks: %w", err)
264271
}
265272

273+
if err := a.addUserToNewBoard(boardsAndBlocks, opt, boardMembers); err != nil {
274+
return nil, err
275+
}
276+
277+
// find new board id
278+
for _, board := range boardsAndBlocks.Boards {
279+
return board, nil
280+
}
281+
return nil, fmt.Errorf("missing board in archive: %w", model.ErrInvalidBoardBlock)
282+
}
283+
284+
func (a *App) addUserToNewBoard(boardsAndBlocks *model.BoardsAndBlocks, opt model.ImportArchiveOptions, boardMembers []*model.BoardMember) error {
266285
// add users to all the new boards (if not the fake system user).
267286
for _, board := range boardsAndBlocks.Boards {
268287
// make sure an admin user gets added
@@ -272,7 +291,7 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (*mo
272291
SchemeAdmin: true,
273292
}
274293
if _, err2 := a.AddMemberToBoard(adminMember); err2 != nil {
275-
return nil, fmt.Errorf("cannot add adminMember to board: %w", err2)
294+
return fmt.Errorf("cannot add adminMember to board: %w", err2)
276295
}
277296
for _, boardMember := range boardMembers {
278297
bm := &model.BoardMember{
@@ -287,16 +306,11 @@ func (a *App) ImportBoardJSONL(r io.Reader, opt model.ImportArchiveOptions) (*mo
287306
Synthetic: boardMember.Synthetic,
288307
}
289308
if _, err2 := a.AddMemberToBoard(bm); err2 != nil {
290-
return nil, fmt.Errorf("cannot add member to board: %w", err2)
309+
return fmt.Errorf("cannot add member to board: %w", err2)
291310
}
292311
}
293312
}
294-
295-
// find new board id
296-
for _, board := range boardsAndBlocks.Boards {
297-
return board, nil
298-
}
299-
return nil, fmt.Errorf("missing board in archive: %w", model.ErrInvalidBoardBlock)
313+
return nil
300314
}
301315

302316
// fixBoardsandBlocks allows the caller of `ImportArchive` to modify or filters boards and blocks being

‎server/model/base.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package model
22

33
import (
4-
"errors"
5-
64
mmModel "github.com/mattermost/mattermost/server/public/model"
75
)
86

@@ -11,8 +9,8 @@ const (
119
)
1210

1311
var (
14-
errEmptyId = errors.New("ID cannot be empty")
15-
errInvalidId = errors.New("invalid ID")
12+
errEmptyId = NewErrBadRequest("Block ID cannot be empty")
13+
errInvalidId = NewErrBadRequest("Invalid Block ID")
1614
)
1715

1816
func IsValidId(id string) error {

0 commit comments

Comments
 (0)