diff --git a/server/model/block.go b/server/model/block.go index 489cfe794..0bda6bdc0 100644 --- a/server/model/block.go +++ b/server/model/block.go @@ -29,6 +29,7 @@ var ( ErrBlockEmptyBoardID = errors.New("boardID is empty") ErrBlockTitleSizeLimitExceeded = errors.New("block title size limit exceeded") ErrBlockFieldsSizeLimitExceeded = errors.New("block fields size limit exceeded") + ErrBlockPropertiesInvalidType = errors.New("block fields.properties must be a JSON object") ) // Block is the basic data unit @@ -193,6 +194,12 @@ func (b *Block) baseValidations() error { } } + if propsIface, present := b.Fields["properties"]; present { + if _, ok := propsIface.(map[string]interface{}); !ok { + return ErrBlockPropertiesInvalidType + } + } + return nil } diff --git a/server/model/block_test.go b/server/model/block_test.go index b715bb6c1..ab7a34688 100644 --- a/server/model/block_test.go +++ b/server/model/block_test.go @@ -574,6 +574,57 @@ func TestBlockIsValid(t *testing.T) { require.Error(t, err) require.EqualError(t, err, "Invalid Block ID") }) + + t.Run("Should return error when fields.properties is JSON null", func(t *testing.T) { + block := &Block{ + ID: string(utils.IDTypeNone) + mmModel.NewId(), + BoardID: string(utils.IDTypeNone) + mmModel.NewId(), + CreatedBy: string(utils.IDTypeNone) + mmModel.NewId(), + ModifiedBy: string(utils.IDTypeNone) + mmModel.NewId(), + Schema: 1, + Type: TypeCard, + Title: "Block with null properties", + Fields: map[string]interface{}{"properties": nil}, + CreateAt: 1234567890, + UpdateAt: 1234567890, + } + err := block.IsValid() + require.ErrorIs(t, err, ErrBlockPropertiesInvalidType) + }) + + t.Run("Should return error when fields.properties is not a JSON object", func(t *testing.T) { + block := &Block{ + ID: string(utils.IDTypeNone) + mmModel.NewId(), + BoardID: string(utils.IDTypeNone) + mmModel.NewId(), + CreatedBy: string(utils.IDTypeNone) + mmModel.NewId(), + ModifiedBy: string(utils.IDTypeNone) + mmModel.NewId(), + Schema: 1, + Type: TypeCard, + Title: "Block with malformed properties", + Fields: map[string]interface{}{"properties": "not-a-property-map"}, + CreateAt: 1234567890, + UpdateAt: 1234567890, + } + err := block.IsValid() + require.ErrorIs(t, err, ErrBlockPropertiesInvalidType) + }) + + t.Run("Should accept block with properties as map", func(t *testing.T) { + block := &Block{ + ID: string(utils.IDTypeNone) + mmModel.NewId(), + BoardID: string(utils.IDTypeNone) + mmModel.NewId(), + CreatedBy: string(utils.IDTypeNone) + mmModel.NewId(), + ModifiedBy: string(utils.IDTypeNone) + mmModel.NewId(), + Schema: 1, + Type: TypeCard, + Title: "Block with properties map", + Fields: map[string]interface{}{"properties": map[string]interface{}{"a": "b"}}, + CreateAt: 1234567890, + UpdateAt: 1234567890, + } + err := block.IsValid() + require.NoError(t, err) + }) } func TestBlock_IsValidForImport(t *testing.T) { diff --git a/server/services/notify/notifysubscriptions/diff.go b/server/services/notify/notifysubscriptions/diff.go index c95f16c64..e0f0a5741 100644 --- a/server/services/notify/notifysubscriptions/diff.go +++ b/server/services/notify/notifysubscriptions/diff.go @@ -296,7 +296,7 @@ func (dg *diffGenerator) generatePropDiffs(oldBlock, newBlock *model.Block, sche oldProps, err := model.ParseProperties(oldBlock, schema, dg.store) if err != nil { dg.logger.Error("Cannot parse properties for old block", - mlog.String("block_id", oldBlock.ID), + mlog.String("block_id", safeBlockID(oldBlock)), mlog.Err(err), ) } @@ -304,7 +304,7 @@ func (dg *diffGenerator) generatePropDiffs(oldBlock, newBlock *model.Block, sche newProps, err := model.ParseProperties(newBlock, schema, dg.store) if err != nil { dg.logger.Error("Cannot parse properties for new block", - mlog.String("block_id", oldBlock.ID), + mlog.String("block_id", safeBlockID(newBlock)), mlog.Err(err), ) } @@ -352,6 +352,13 @@ func (dg *diffGenerator) generatePropDiffs(oldBlock, newBlock *model.Block, sche return sortPropDiffs(propDiffs) } +func safeBlockID(b *model.Block) string { + if b == nil { + return "" + } + return b.ID +} + func sortPropDiffs(propDiffs []PropDiff) []PropDiff { if len(propDiffs) == 0 { return propDiffs diff --git a/server/services/notify/notifysubscriptions/diff_test.go b/server/services/notify/notifysubscriptions/diff_test.go new file mode 100644 index 000000000..781effb34 --- /dev/null +++ b/server/services/notify/notifysubscriptions/diff_test.go @@ -0,0 +1,17 @@ +// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package notifysubscriptions + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost-plugin-boards/server/model" +) + +func TestSafeBlockID(t *testing.T) { + require.Equal(t, "", safeBlockID(nil), "nil block must yield empty id") + require.Equal(t, "abc", safeBlockID(&model.Block{ID: "abc"})) +} diff --git a/server/services/notify/notifysubscriptions/notifier.go b/server/services/notify/notifysubscriptions/notifier.go index cc6ba79cf..fdd75dc9a 100644 --- a/server/services/notify/notifysubscriptions/notifier.go +++ b/server/services/notify/notifysubscriptions/notifier.go @@ -6,6 +6,7 @@ package notifysubscriptions import ( "errors" "fmt" + "runtime/debug" "sync" "time" @@ -61,10 +62,38 @@ func (n *notifier) start() { if n.done == nil { n.done = make(chan struct{}) - go n.loop() + done := n.done + go n.safeLoop(done) } } +func (n *notifier) safeLoop(done chan struct{}) { + for { + if n.runLoopOnce(n.loop) { + return + } + select { + case <-done: + return + case <-time.After(time.Second * 5): + } + } +} + +func (n *notifier) runLoopOnce(fn func()) (finished bool) { + defer func() { + if r := recover(); r != nil { + n.logger.Error("panic recovered in notification loop", + mlog.Any("panic", r), + mlog.String("stack", string(debug.Stack())), + ) + finished = false + } + }() + fn() + return true +} + func (n *notifier) stop() { n.mux.Lock() defer n.mux.Unlock() diff --git a/server/services/notify/notifysubscriptions/notifier_test.go b/server/services/notify/notifysubscriptions/notifier_test.go new file mode 100644 index 000000000..9ebbcffea --- /dev/null +++ b/server/services/notify/notifysubscriptions/notifier_test.go @@ -0,0 +1,58 @@ +// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package notifysubscriptions + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/shared/mlog" +) + +func newTestNotifier(t *testing.T) *notifier { + t.Helper() + return ¬ifier{logger: mlog.CreateConsoleTestLogger(t)} +} + +func TestRunLoopOnce_NormalReturn(t *testing.T) { + n := newTestNotifier(t) + called := false + finished := n.runLoopOnce(func() { called = true }) + require.True(t, called, "fn should have been invoked") + require.True(t, finished, "runLoopOnce should report finished=true on normal return") +} + +func TestRunLoopOnce_RecoversFromPanic(t *testing.T) { + n := newTestNotifier(t) + require.NotPanics(t, func() { + finished := n.runLoopOnce(func() { panic("boom") }) + require.False(t, finished, "runLoopOnce should report finished=false on panic") + }) +} + +func TestSafeLoop_RestartsAfterPanic(t *testing.T) { + n := newTestNotifier(t) + calls := 0 + fn := func() { + calls++ + if calls < 3 { + panic("transient") + } + } + + deadline := time.After(2 * time.Second) + for { + select { + case <-deadline: + t.Fatalf("safeLoop simulation did not converge; calls=%d", calls) + default: + } + if n.runLoopOnce(fn) { + break + } + } + require.Equal(t, 3, calls, "loop should restart after each panic until normal return") +}