Automated cherry pick of #216

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
 }
 

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

server/services/notify/notifysubscriptions/diff.go

@@ -296,15 +296,15 @@ 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),
 		)
 	}
 
 	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

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"}))
+}

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

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 &notifier{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")
+}