Insufficient input validation in GitHub plugin API causes denial of service
Description
Mattermost versions 11.6.x <= 11.6.0, 11.5.x <= 11.5.3, 11.4.x <= 11.4.4, 10.11.x <= 10.11.14 fail to validate user-supplied input in API request handlers which allows an authenticated attacker to crash the plugin process via a crafted HTTP request to the PR details endpoint.. Mattermost Advisory ID: MMSA-2026-00638
Affected products
1- Range: 11.6.x <= 11.6.0, 11.5.x <= 11.5.3, 11.4.x <= 11.4.4, 10.11.x <= 10.11.14
Patches
11a07e4b05ff82Added nil checks (#35755) (#36134)
3 files changed · +250 −7
server/channels/app/channel.go+6 −6 modified@@ -1580,6 +1580,12 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return err } + deleteAt := model.GetMillis() + + if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { + return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + if user != nil { T := i18n.GetUserTranslations(user.Locale) @@ -1635,12 +1641,6 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return model.NewAppError("DeleteChannel", "app.post_persistent_notification.delete_by_channel.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - deleteAt := model.GetMillis() - - if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { - return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - a.Srv().Platform().InvalidateCacheForChannel(channel) var message *model.WebSocketEvent
server/channels/app/post_persistent_notification.go+26 −1 modified@@ -163,13 +163,25 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos return err } + var postsForPersistentNotificationCleanup []*model.Post + for _, post := range posts { channel := channelsMap[post.ChannelId] + if channel == nil { + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue + } + team := teamsMap[channel.TeamId] // GMs and DMs don't belong to any team if channel.IsGroupOrDirect() { team = &model.Team{} + } else if team == nil { + // cleanup persistent notification for posts with missing teams when they are not DM or GM + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue } + profileMap := channelProfileMap[channel.Id] // Ensure the sender is always in the profile map: for example, system admins can post @@ -210,6 +222,14 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos } } + if len(postsForPersistentNotificationCleanup) > 0 { + for _, post := range postsForPersistentNotificationCleanup { + if appErr := a.DeletePersistentNotification(request.EmptyContext(a.Log()), post); appErr != nil { + a.Log().Warn("Failed to delete persistent notification for post", mlog.String("post_id", post.Id), mlog.String("channel_id", post.ChannelId), mlog.Err(appErr)) + } + } + } + return nil } @@ -219,9 +239,14 @@ func (a *App) persistentNotificationsAuxiliaryData(channelsMap map[string]*model channelKeywords := make(map[string]MentionKeywords, len(channelsMap)) channelNotifyProps := make(map[string]map[string]model.StringMap, len(channelsMap)) for _, c := range channelsMap { + team := teamsMap[c.TeamId] + if team == nil && !c.IsGroupOrDirect() { + continue + } + // In DM, notifications can't be send to any 3rd person. if c.Type != model.ChannelTypeDirect { - groups, err := a.getGroupsAllowedForReferenceInChannel(c, teamsMap[c.TeamId]) + groups, err := a.getGroupsAllowedForReferenceInChannel(c, team) if err != nil { return nil, nil, nil, nil, errors.Wrapf(err, "failed to get profiles for channel %s", c.Id) }
server/channels/app/post_persistent_notification_test.go+218 −0 modified@@ -192,6 +192,224 @@ func TestDeletePersistentNotification(t *testing.T) { }) } +func TestForEachPersistentNotificationPost(t *testing.T) { + mainHelper.Parallel(t) + + t.Run("should cleanup posts whose channel no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + profileMap := map[string]*model.User{user1.Id: user1} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + + // post1 belongs to an existing channel; post2 belongs to a deleted/missing channel + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user1.Id} + post2 := &model.Post{Id: "pid2", ChannelId: "deleted-channel-id", Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Only return channel for post1; post2's channel is missing + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + // DeletePersistentNotification mocks - the cleanup path calls GetSingle then Delete + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid channel) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should cleanup posts whose team no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + // channelWithMissingTeam has a TeamId that won't be in teamsMap + channelWithMissingTeam := &model.Channel{Id: "chid2", TeamId: "deleted-team-id", Type: model.ChannelTypeOpen} + + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user2.Id} + post2 := &model.Post{Id: "pid2", ChannelId: channelWithMissingTeam.Id, Message: "hello @user-1", UserId: user2.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Both channels exist, but only one team exists + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel, channelWithMissingTeam}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + // Only return the team for channel, not for channelWithMissingTeam + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid team) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up due to missing team + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should not cleanup DM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + dmChannel := &model.Channel{Id: "dm-chid", TeamId: "", Type: model.ChannelTypeDirect, Name: model.GetDMNameFromIds(user1.Id, user2.Id)} + + post1 := &model.Post{Id: "pid1", ChannelId: dmChannel.Id, Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{dmChannel}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the DM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — DMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) + + t.Run("should not cleanup GM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + user3 := &model.User{Id: "uid3", Username: "user-3"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2, user3.Id: user3} + gmChannel := &model.Channel{Id: "gm-chid", TeamId: "", Type: model.ChannelTypeGroup} + + post1 := &model.Post{Id: "pid1", ChannelId: gmChannel.Id, Message: "hello @user-2", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{gmChannel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the GM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — GMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) +} + func TestSendPersistentNotifications(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t)
3b21498788a7Added nil checks (#35755) (#36133)
3 files changed · +250 −7
server/channels/app/channel.go+6 −6 modified@@ -1605,6 +1605,12 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return err } + deleteAt := model.GetMillis() + + if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { + return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + if user != nil { T := i18n.GetUserTranslations(user.Locale) @@ -1660,12 +1666,6 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return model.NewAppError("DeleteChannel", "app.post_persistent_notification.delete_by_channel.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - deleteAt := model.GetMillis() - - if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { - return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - a.Srv().Platform().InvalidateCacheForChannel(channel) var message *model.WebSocketEvent
server/channels/app/post_persistent_notification.go+26 −1 modified@@ -163,13 +163,25 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos return err } + var postsForPersistentNotificationCleanup []*model.Post + for _, post := range posts { channel := channelsMap[post.ChannelId] + if channel == nil { + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue + } + team := teamsMap[channel.TeamId] // GMs and DMs don't belong to any team if channel.IsGroupOrDirect() { team = &model.Team{} + } else if team == nil { + // cleanup persistent notification for posts with missing teams when they are not DM or GM + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue } + profileMap := channelProfileMap[channel.Id] // Ensure the sender is always in the profile map: for example, system admins can post @@ -210,6 +222,14 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos } } + if len(postsForPersistentNotificationCleanup) > 0 { + for _, post := range postsForPersistentNotificationCleanup { + if appErr := a.DeletePersistentNotification(request.EmptyContext(a.Log()), post); appErr != nil { + a.Log().Warn("Failed to delete persistent notification for post", mlog.String("post_id", post.Id), mlog.String("channel_id", post.ChannelId), mlog.Err(appErr)) + } + } + } + return nil } @@ -219,9 +239,14 @@ func (a *App) persistentNotificationsAuxiliaryData(channelsMap map[string]*model channelKeywords := make(map[string]MentionKeywords, len(channelsMap)) channelNotifyProps := make(map[string]map[string]model.StringMap, len(channelsMap)) for _, c := range channelsMap { + team := teamsMap[c.TeamId] + if team == nil && !c.IsGroupOrDirect() { + continue + } + // In DM, notifications can't be send to any 3rd person. if c.Type != model.ChannelTypeDirect { - groups, err := a.getGroupsAllowedForReferenceInChannel(c, teamsMap[c.TeamId]) + groups, err := a.getGroupsAllowedForReferenceInChannel(c, team) if err != nil { return nil, nil, nil, nil, errors.Wrapf(err, "failed to get profiles for channel %s", c.Id) }
server/channels/app/post_persistent_notification_test.go+218 −0 modified@@ -192,6 +192,224 @@ func TestDeletePersistentNotification(t *testing.T) { }) } +func TestForEachPersistentNotificationPost(t *testing.T) { + mainHelper.Parallel(t) + + t.Run("should cleanup posts whose channel no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + profileMap := map[string]*model.User{user1.Id: user1} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + + // post1 belongs to an existing channel; post2 belongs to a deleted/missing channel + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user1.Id} + post2 := &model.Post{Id: "pid2", ChannelId: "deleted-channel-id", Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Only return channel for post1; post2's channel is missing + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + // DeletePersistentNotification mocks - the cleanup path calls GetSingle then Delete + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid channel) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should cleanup posts whose team no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + // channelWithMissingTeam has a TeamId that won't be in teamsMap + channelWithMissingTeam := &model.Channel{Id: "chid2", TeamId: "deleted-team-id", Type: model.ChannelTypeOpen} + + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user2.Id} + post2 := &model.Post{Id: "pid2", ChannelId: channelWithMissingTeam.Id, Message: "hello @user-1", UserId: user2.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Both channels exist, but only one team exists + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel, channelWithMissingTeam}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + // Only return the team for channel, not for channelWithMissingTeam + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid team) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up due to missing team + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should not cleanup DM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + dmChannel := &model.Channel{Id: "dm-chid", TeamId: "", Type: model.ChannelTypeDirect, Name: model.GetDMNameFromIds(user1.Id, user2.Id)} + + post1 := &model.Post{Id: "pid1", ChannelId: dmChannel.Id, Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{dmChannel}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the DM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — DMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) + + t.Run("should not cleanup GM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + user3 := &model.User{Id: "uid3", Username: "user-3"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2, user3.Id: user3} + gmChannel := &model.Channel{Id: "gm-chid", TeamId: "", Type: model.ChannelTypeGroup} + + post1 := &model.Post{Id: "pid1", ChannelId: gmChannel.Id, Message: "hello @user-2", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{gmChannel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the GM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — GMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) +} + func TestSendPersistentNotifications(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t)
2f2dabe523f8Added nil checks (#35755) (#36132)
3 files changed · +249 −7
server/channels/app/channel.go+5 −6 modified@@ -1605,6 +1605,11 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return err } + deleteAt := model.GetMillis() + + if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { + return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } if user != nil { T := i18n.GetUserTranslations(user.Locale) @@ -1660,12 +1665,6 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return model.NewAppError("DeleteChannel", "app.post_persistent_notification.delete_by_channel.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - deleteAt := model.GetMillis() - - if err := a.Srv().Store().Channel().Delete(channel.Id, deleteAt); err != nil { - return model.NewAppError("DeleteChannel", "app.channel.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } - a.Srv().Platform().InvalidateCacheForChannel(channel) var message *model.WebSocketEvent
server/channels/app/post_persistent_notification.go+26 −1 modified@@ -163,13 +163,25 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos return err } + var postsForPersistentNotificationCleanup []*model.Post + for _, post := range posts { channel := channelsMap[post.ChannelId] + if channel == nil { + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue + } + team := teamsMap[channel.TeamId] // GMs and DMs don't belong to any team if channel.IsGroupOrDirect() { team = &model.Team{} + } else if team == nil { + // cleanup persistent notification for posts with missing teams when they are not DM or GM + postsForPersistentNotificationCleanup = append(postsForPersistentNotificationCleanup, post) + continue } + profileMap := channelProfileMap[channel.Id] // Ensure the sender is always in the profile map: for example, system admins can post @@ -210,6 +222,14 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos } } + if len(postsForPersistentNotificationCleanup) > 0 { + for _, post := range postsForPersistentNotificationCleanup { + if appErr := a.DeletePersistentNotification(request.EmptyContext(a.Log()), post); appErr != nil { + a.Log().Warn("Failed to delete persistent notification for post", mlog.String("post_id", post.Id), mlog.String("channel_id", post.ChannelId), mlog.Err(appErr)) + } + } + } + return nil } @@ -219,9 +239,14 @@ func (a *App) persistentNotificationsAuxiliaryData(channelsMap map[string]*model channelKeywords := make(map[string]MentionKeywords, len(channelsMap)) channelNotifyProps := make(map[string]map[string]model.StringMap, len(channelsMap)) for _, c := range channelsMap { + team := teamsMap[c.TeamId] + if team == nil && !c.IsGroupOrDirect() { + continue + } + // In DM, notifications can't be send to any 3rd person. if c.Type != model.ChannelTypeDirect { - groups, err := a.getGroupsAllowedForReferenceInChannel(c, teamsMap[c.TeamId]) + groups, err := a.getGroupsAllowedForReferenceInChannel(c, team) if err != nil { return nil, nil, nil, nil, errors.Wrapf(err, "failed to get profiles for channel %s", c.Id) }
server/channels/app/post_persistent_notification_test.go+218 −0 modified@@ -192,6 +192,224 @@ func TestDeletePersistentNotification(t *testing.T) { }) } +func TestForEachPersistentNotificationPost(t *testing.T) { + mainHelper.Parallel(t) + + t.Run("should cleanup posts whose channel no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + profileMap := map[string]*model.User{user1.Id: user1} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + + // post1 belongs to an existing channel; post2 belongs to a deleted/missing channel + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user1.Id} + post2 := &model.Post{Id: "pid2", ChannelId: "deleted-channel-id", Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Only return channel for post1; post2's channel is missing + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + // DeletePersistentNotification mocks - the cleanup path calls GetSingle then Delete + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid channel) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should cleanup posts whose team no longer exists", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + team := &model.Team{Id: "tid"} + channel := &model.Channel{Id: "chid", TeamId: team.Id, Type: model.ChannelTypeOpen} + // channelWithMissingTeam has a TeamId that won't be in teamsMap + channelWithMissingTeam := &model.Channel{Id: "chid2", TeamId: "deleted-team-id", Type: model.ChannelTypeOpen} + + post1 := &model.Post{Id: "pid1", ChannelId: channel.Id, Message: "hello @user-1", UserId: user2.Id} + post2 := &model.Post{Id: "pid2", ChannelId: channelWithMissingTeam.Id, Message: "hello @user-1", UserId: user2.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + // Both channels exist, but only one team exists + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{channel, channelWithMissingTeam}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + // Only return the team for channel, not for channelWithMissingTeam + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{team}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + mockPostPersistentNotification.On("GetSingle", post2.Id).Return(&model.PostPersistentNotifications{PostId: post2.Id}, nil) + mockPostPersistentNotification.On("Delete", []string{post2.Id}).Return(nil) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1, post2}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should only be called for post1 (valid team) + assert.Equal(t, []string{"pid1"}, fnCalled) + // post2 persistent notification should have been cleaned up due to missing team + mockPostPersistentNotification.AssertCalled(t, "Delete", []string{post2.Id}) + }) + + t.Run("should not cleanup DM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2} + dmChannel := &model.Channel{Id: "dm-chid", TeamId: "", Type: model.ChannelTypeDirect, Name: model.GetDMNameFromIds(user1.Id, user2.Id)} + + post1 := &model.Post{Id: "pid1", ChannelId: dmChannel.Id, Message: "hello", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{dmChannel}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the DM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — DMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) + + t.Run("should not cleanup GM posts that have no team", func(t *testing.T) { + th := SetupWithStoreMock(t) + + user1 := &model.User{Id: "uid1", Username: "user-1"} + user2 := &model.User{Id: "uid2", Username: "user-2"} + user3 := &model.User{Id: "uid3", Username: "user-3"} + profileMap := map[string]*model.User{user1.Id: user1, user2.Id: user2, user3.Id: user3} + gmChannel := &model.Channel{Id: "gm-chid", TeamId: "", Type: model.ChannelTypeGroup} + + post1 := &model.Post{Id: "pid1", ChannelId: gmChannel.Id, Message: "hello @user-2", UserId: user1.Id} + + mockStore := th.App.Srv().Store().(*storemocks.Store) + + mockChannel := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannel) + mockChannel.On("GetChannelsByIds", mock.Anything, mock.Anything).Return([]*model.Channel{gmChannel}, nil) + mockChannel.On("GetAllChannelMembersNotifyPropsForChannel", mock.Anything, mock.Anything).Return(map[string]model.StringMap{}, nil) + + mockTeam := storemocks.TeamStore{} + mockStore.On("Team").Return(&mockTeam) + mockTeam.On("GetMany", mock.Anything).Return([]*model.Team{}, nil) + + mockUser := storemocks.UserStore{} + mockStore.On("User").Return(&mockUser) + mockUser.On("GetAllProfilesInChannel", mock.Anything, mock.Anything, mock.Anything).Return(profileMap, nil) + + mockGroup := storemocks.GroupStore{} + mockStore.On("Group").Return(&mockGroup) + mockGroup.On("GetGroups", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Group{}, nil) + + mockPostPersistentNotification := storemocks.PostPersistentNotificationStore{} + mockStore.On("PostPersistentNotification").Return(&mockPostPersistentNotification) + + th.App.Srv().SetLicense(getLicWithSkuShortName(model.LicenseShortSkuProfessional)) + cfg := th.App.Config() + *cfg.ServiceSettings.PostPriority = true + *cfg.ServiceSettings.AllowPersistentNotifications = true + + fnCalled := []string{} + err := th.App.forEachPersistentNotificationPost([]*model.Post{post1}, func(post *model.Post, _ *model.Channel, _ *model.Team, _ *MentionResults, _ model.UserMap, _ map[string]map[string]model.StringMap) error { + fnCalled = append(fnCalled, post.Id) + return nil + }) + require.NoError(t, err) + + // The callback should be called for the GM post even though there's no team + assert.Equal(t, []string{"pid1"}, fnCalled) + // Delete should NOT have been called — GMs don't need a team + mockPostPersistentNotification.AssertNotCalled(t, "Delete", mock.Anything) + }) +} + func TestSendPersistentNotifications(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t)
436b103174afFixed URL validation for integration actions (#35857) (#36089)
2 files changed · +203 −11
server/channels/app/integration_action.go+18 −11 modified@@ -331,16 +331,7 @@ func (a *App) DoActionRequest(rctx request.CTX, rawURL string, body []byte) (*ht req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json") - // Allow access to plugin routes for action buttons - var httpClient *http.Client - subpath, _ := utils.GetSubpathFromConfig(a.Config()) - siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL) - if inURL.Hostname() == siteURL.Hostname() && strings.HasPrefix(inURL.Path, path.Join(subpath, "plugins")) { - req.Header.Set(model.HeaderAuth, "Bearer "+rctx.Session().Token) - httpClient = a.HTTPService().MakeClient(true) - } else { - httpClient = a.HTTPService().MakeClient(false) - } + httpClient := a.getPostActionClient(rctx, inURL, req) resp, httpErr := httpClient.Do(req) if httpErr != nil { @@ -354,6 +345,20 @@ func (a *App) DoActionRequest(rctx request.CTX, rawURL string, body []byte) (*ht return resp, nil } +func (a *App) getPostActionClient(rctx request.CTX, inURL *url.URL, req *http.Request) *http.Client { + // Allow access to plugin routes for action buttons + var httpClient *http.Client + subpath, _ := utils.GetSubpathFromConfig(a.Config()) + siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL) + if inURL.Hostname() == siteURL.Hostname() && strings.HasPrefix(path.Clean(inURL.Path), path.Join(subpath, "plugins")) { + req.Header.Set(model.HeaderAuth, "Bearer "+rctx.Session().Token) + httpClient = a.HTTPService().MakeClient(true) + } else { + httpClient = a.HTTPService().MakeClient(false) + } + return httpClient +} + type LocalResponseWriter struct { data []byte headers http.Header @@ -387,13 +392,15 @@ func (ch *Channels) doPluginRequest(rctx request.CTX, method, rawURL string, val if err != nil { return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "", http.StatusBadRequest).Wrap(err) } - result := strings.Split(inURL.Path, "/") + result := strings.Split(path.Clean(inURL.Path), "/") if len(result) < 2 { return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "err=Unable to find pluginId", http.StatusBadRequest) } + if result[0] != "plugins" { return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "err=plugins not in path", http.StatusBadRequest) } + pluginID := result[1] path := strings.TrimPrefix(inURL.Path, "plugins/"+pluginID)
server/channels/app/integration_action_test.go+185 −0 modified@@ -1621,6 +1621,105 @@ func TestDoActionRequest(t *testing.T) { }) } +func TestGetPostActionClient(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + tests := []struct { + name string + siteURL string + subpath string + requestURL string + expectAuth bool + }{ + { + name: "same host with plugin path gets auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/plugins/myplugin/action", + expectAuth: true, + }, + { + name: "same host with non-plugin path does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/api/v4/posts", + expectAuth: false, + }, + { + name: "different host with plugin path does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://evil.com/plugins/myplugin/action", + expectAuth: false, + }, + { + name: "different host same port does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://attacker.com:8065/plugins/myplugin/action", + expectAuth: false, + }, + { + name: "path traversal to reach plugins does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/api/../../plugins/myplugin", + expectAuth: true, // path.Clean normalizes to /plugins/myplugin + }, + { + name: "path traversal escaping plugins does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/plugins/../api/v4/posts", + expectAuth: false, // path.Clean normalizes to /api/v4/posts + }, + { + name: "subpath with plugin path gets auth", + siteURL: "http://localhost:8065/mattermost", + subpath: "/mattermost", + requestURL: "http://localhost:8065/mattermost/plugins/myplugin/action", + expectAuth: true, + }, + { + name: "subpath without subpath prefix does not get auth", + siteURL: "http://localhost:8065/mattermost", + subpath: "/mattermost", + requestURL: "http://localhost:8065/plugins/myplugin/action", + expectAuth: false, // plugins path doesn't include subpath + }, + { + name: "empty path does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/", + expectAuth: false, + }, + { + name: "plugins as query param does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/api?path=plugins/myplugin", + expectAuth: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.siteURL + }) + + inURL, err := url.Parse(tc.requestURL) + require.NoError(t, err) + + req, err := http.NewRequest("POST", tc.requestURL, nil) + require.NoError(t, err) + + _ = th.App.getPostActionClient(th.Context, inURL, req) + + if tc.expectAuth { + assert.NotEmpty(t, req.Header.Get(model.HeaderAuth), "expected auth header to be set") + assert.Contains(t, req.Header.Get(model.HeaderAuth), "Bearer ") + } else { + assert.Empty(t, req.Header.Get(model.HeaderAuth), "expected no auth header") + } + }) + } +} + func TestDoLocalRequest(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) @@ -1816,4 +1915,90 @@ func TestDoPluginRequest(t *testing.T) { require.NotNil(t, resp) body, _ = io.ReadAll(resp.Body) assert.Equal(t, "param multiple not correct", string(body)) + + t.Run("should handle URLs with path traversals", func(t *testing.T) { + tests := []struct { + name string + rawURL string + expectErr bool + errDetail string + }{ + { + name: "path traversal to escape plugins directory", + rawURL: "/plugins/../../../etc/passwd", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "path traversal with encoded slashes", + rawURL: "/plugins/..%2F..%2F..%2Fetc%2Fpasswd", + expectErr: true, // url.Parse decodes %2F, path.Clean normalizes traversal + errDetail: "plugins not in path", + }, + { + name: "double dot in plugin path", + rawURL: "/plugins/../plugins/myplugin/action", + expectErr: false, // path.Clean normalizes this back to plugins/myplugin/action + }, + { + name: "path traversal without leading slash", + rawURL: "plugins/../../../etc/passwd", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "only plugins with no plugin ID", + rawURL: "/plugins/", + expectErr: true, + errDetail: "Unable to find pluginId", + }, + { + name: "just plugins no trailing slash", + rawURL: "/plugins", + expectErr: true, + errDetail: "Unable to find pluginId", + }, + { + name: "non-plugins path", + rawURL: "/api/v4/users", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "path traversal via dot segments after plugin ID", + rawURL: "/plugins/myplugin/../../etc/passwd", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "backslash traversal attempt", + rawURL: "/plugins/myplugin/..\\..\\etc\\passwd", + expectErr: false, // backslashes are not path separators in URL paths; treated as literal + }, + { + name: "null byte injection attempt", + rawURL: "/plugins/myplugin\x00/action", + expectErr: true, // url.Parse rejects URLs with null bytes + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp, appErr := th.App.doPluginRequest(th.Context, "GET", tc.rawURL, nil, nil) + if tc.expectErr { + require.NotNil(t, appErr, "expected error for URL: %s", tc.rawURL) + if tc.errDetail != "" { + assert.Contains(t, appErr.DetailedError, tc.errDetail) + } + } else { + // Should not return an app error from path validation; + // may still get a 404 if the plugin doesn't exist, which is fine. + assert.Nil(t, appErr, "unexpected error for URL: %s - %v", tc.rawURL, appErr) + if resp != nil { + resp.Body.Close() + } + } + }) + } + }) }
610a28e9faa9Automated cherry pick of #35562 (#36095)
3 files changed · +297 −0
server/channels/api4/team.go+38 −0 modified@@ -604,6 +604,10 @@ func getTeamMember(c *Context, w http.ResponseWriter, r *http.Request) { return } + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + team.SanitizeRoleData(c.AppContext.Session().UserId) + } + if err := json.NewEncoder(w).Encode(team); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) } @@ -642,6 +646,13 @@ func getTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range members { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembers", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -681,6 +692,13 @@ func getTeamMembersForUser(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + for _, m := range members { + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), m.TeamId, model.PermissionManageTeamRoles) { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembersForUser", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -724,6 +742,13 @@ func getTeamMembersByIds(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range members { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembersByIds", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -830,6 +855,10 @@ func addTeamMember(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventObjectType("team_member") // TODO verify this is the final state. should it be the team instead? auditRec.Success() + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + tm.SanitizeRoleData(c.AppContext.Session().UserId) + } + w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(tm); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) @@ -984,6 +1013,15 @@ func addTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range membersWithErrors { + if m.Member != nil { + m.Member.SanitizeRoleData(currentUserId) + } + } + } + var ( js []byte err error
server/channels/api4/team_test.go+248 −0 modified@@ -4574,3 +4574,251 @@ func TestInvalidateAllEmailInvites(t *testing.T) { CheckOKStatus(t, res) }) } + +func setupTeamWithAdminAndMember(t *testing.T, th *TestHelper) *model.Client4 { + t.Helper() + th.UpdateUserToTeamAdmin(th.BasicUser2, th.BasicTeam) + require.Nil(t, th.App.Srv().InvalidateAllCaches()) + teamAdminClient := th.CreateClient() + _, _, err := teamAdminClient.Login(context.Background(), th.BasicUser2.Email, th.BasicUser2.Password) + require.NoError(t, err) + return teamAdminClient +} + +func assertRoleDataSanitized(t *testing.T, m *model.TeamMember) { + t.Helper() + assert.Empty(t, m.Roles) + assert.Empty(t, m.ExplicitRoles) + assert.False(t, m.SchemeAdmin) + assert.False(t, m.SchemeGuest) + assert.False(t, m.SchemeUser) + assert.Equal(t, int64(-1), m.DeleteAt) +} + +func TestGetTeamMembersRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId != th.BasicUser.Id { + assertRoleDataSanitized(t, m) + } + } + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "current user not found in members") + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "target user not found in members") + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser2.Id { + assert.True(t, m.SchemeAdmin) + return + } + } + require.Fail(t, "team admin not found in members") + }) +} + +func TestGetTeamMemberRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + member, _, err := th.Client.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + assertRoleDataSanitized(t, member) + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + member, _, err := th.Client.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeUser) + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + member, _, err := teamAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeUser) + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + member, _, err := th.SystemAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeAdmin) + }) +} + +func TestGetTeamMembersByIdsRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser2.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assertRoleDataSanitized(t, members[0]) + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser2.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeAdmin) + }) +} + +func TestAddTeamMemberRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("team admin adding user sees full role data in response", func(t *testing.T) { + newUser := th.CreateUser() + tm, _, err := teamAdminClient.AddTeamMember(context.Background(), th.BasicTeam.Id, newUser.Id) + require.NoError(t, err) + assert.True(t, tm.SchemeUser) + }) + + t.Run("non-admin adding user sees sanitized role data in response", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions() + defer th.RestoreDefaultRolePermissions(defaultRolePermissions) + th.AddPermissionToRole(model.PermissionAddUserToTeam.Id, model.TeamUserRoleId) + + newUser := th.CreateUser() + tm, _, err := th.Client.AddTeamMember(context.Background(), th.BasicTeam.Id, newUser.Id) + require.NoError(t, err) + assertRoleDataSanitized(t, tm) + }) +} + +func TestAddTeamMembersRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("team admin adding users sees full role data in response", func(t *testing.T) { + newUser := th.CreateUser() + members, _, err := teamAdminClient.AddTeamMembers(context.Background(), th.BasicTeam.Id, []string{newUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("non-admin adding users sees sanitized role data in response", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions() + defer th.RestoreDefaultRolePermissions(defaultRolePermissions) + th.AddPermissionToRole(model.PermissionAddUserToTeam.Id, model.TeamUserRoleId) + + newUser := th.CreateUser() + members, _, err := th.Client.AddTeamMembers(context.Background(), th.BasicTeam.Id, []string{newUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assertRoleDataSanitized(t, members[0]) + }) +} + +func TestGetTeamMembersForUserRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("user sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersForUser(context.Background(), th.BasicUser.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + assert.True(t, m.SchemeUser) + } + }) + + t.Run("non-admin cannot see role data of another user", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions() + defer th.RestoreDefaultRolePermissions(defaultRolePermissions) + th.AddPermissionToRole(model.PermissionReadOtherUsersTeams.Id, model.SystemUserRoleId) + + members, _, err := th.Client.GetTeamMembersForUser(context.Background(), th.BasicUser2.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + assertRoleDataSanitized(t, m) + } + }) + + t.Run("team admin sees full role data for other user in managed team", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions() + defer th.RestoreDefaultRolePermissions(defaultRolePermissions) + th.AddPermissionToRole(model.PermissionReadOtherUsersTeams.Id, model.SystemUserRoleId) + + members, _, err := teamAdminClient.GetTeamMembersForUser(context.Background(), th.BasicUser.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + if m.TeamId == th.BasicTeam.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "basic team membership not found") + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembersForUser(context.Background(), th.BasicUser2.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + if m.TeamId == th.BasicTeam.Id { + assert.True(t, m.SchemeAdmin) + return + } + } + require.Fail(t, "basic team membership not found") + }) +}
server/public/model/team_member.go+11 −0 modified@@ -142,3 +142,14 @@ func (o *TeamMember) PreUpdate() { func (o *TeamMember) GetRoles() []string { return strings.Fields(o.Roles) } + +func (o *TeamMember) SanitizeRoleData(currentUserId string) { + if o.UserId != currentUserId { + o.Roles = "" + o.ExplicitRoles = "" + o.SchemeAdmin = false + o.SchemeGuest = false + o.SchemeUser = false + o.DeleteAt = -1 + } +}
667dffe31dbbImproved processing of attachments (#35854) (#36103)
3 files changed · +36 −1
server/channels/app/slack.go+1 −1 modified@@ -91,7 +91,7 @@ func (a *App) ProcessSlackText(text string) string { // documented here: https://api.slack.com/docs/attachments func (a *App) ProcessSlackAttachments(attachments []*model.SlackAttachment) []*model.SlackAttachment { var nonNilAttachments = model.StringifySlackFieldValue(attachments) - for _, attachment := range attachments { + for _, attachment := range nonNilAttachments { attachment.Pretext = a.ProcessSlackText(attachment.Pretext) attachment.Text = a.ProcessSlackText(attachment.Text) attachment.Title = a.ProcessSlackText(attachment.Title)
server/channels/app/slack_test.go+27 −0 modified@@ -6,6 +6,8 @@ package app import ( "testing" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost/server/public/model" ) @@ -33,6 +35,31 @@ func TestProcessSlackText(t *testing.T) { } } +func TestProcessMessageAttachmentsWithNilEntries(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + defer th.TearDown() + + attachments := []*model.SlackAttachment{ + nil, + { + Pretext: "pretext", + Text: "text", + Title: "title", + }, + nil, + { + Pretext: "pretext2", + Text: "text2", + }, + } + + result := th.App.ProcessSlackAttachments(attachments) + require.Len(t, result, 2) + require.Equal(t, "pretext", result[0].Pretext) + require.Equal(t, "pretext2", result[1].Pretext) +} + func TestProcessSlackAnnouncement(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic()
server/channels/app/webhook.go+8 −0 modified@@ -125,6 +125,14 @@ func (a *App) TriggerWebhook(c request.CTX, payload *model.OutgoingWebhookPayloa go func() { defer wg.Done() + defer func() { + if r := recover(); r != nil { + logger.Error("Recovered from panic in outgoing webhook goroutine", + mlog.String("url", url), + mlog.Any("panic", r), + ) + } + }() var accessToken *model.OutgoingOAuthConnectionToken
7526844c5052Fixed URL validation for integration actions (#35857) (#36108)
2 files changed · +203 −11
server/channels/app/integration_action.go+18 −11 modified@@ -331,16 +331,7 @@ func (a *App) DoActionRequest(c request.CTX, rawURL string, body []byte) (*http. req.Header.Set("Content-Type", "application/json") req.Header.Set("Accept", "application/json") - // Allow access to plugin routes for action buttons - var httpClient *http.Client - subpath, _ := utils.GetSubpathFromConfig(a.Config()) - siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL) - if inURL.Hostname() == siteURL.Hostname() && strings.HasPrefix(inURL.Path, path.Join(subpath, "plugins")) { - req.Header.Set(model.HeaderAuth, "Bearer "+c.Session().Token) - httpClient = a.HTTPService().MakeClient(true) - } else { - httpClient = a.HTTPService().MakeClient(false) - } + httpClient := a.getPostActionClient(c, inURL, req) resp, httpErr := httpClient.Do(req) if httpErr != nil { @@ -354,6 +345,20 @@ func (a *App) DoActionRequest(c request.CTX, rawURL string, body []byte) (*http. return resp, nil } +func (a *App) getPostActionClient(rctx request.CTX, inURL *url.URL, req *http.Request) *http.Client { + // Allow access to plugin routes for action buttons + var httpClient *http.Client + subpath, _ := utils.GetSubpathFromConfig(a.Config()) + siteURL, _ := url.Parse(*a.Config().ServiceSettings.SiteURL) + if inURL.Hostname() == siteURL.Hostname() && strings.HasPrefix(path.Clean(inURL.Path), path.Join(subpath, "plugins")) { + req.Header.Set(model.HeaderAuth, "Bearer "+rctx.Session().Token) + httpClient = a.HTTPService().MakeClient(true) + } else { + httpClient = a.HTTPService().MakeClient(false) + } + return httpClient +} + type LocalResponseWriter struct { data []byte headers http.Header @@ -387,13 +392,15 @@ func (ch *Channels) doPluginRequest(c request.CTX, method, rawURL string, values if err != nil { return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "", http.StatusBadRequest).Wrap(err) } - result := strings.Split(inURL.Path, "/") + result := strings.Split(path.Clean(inURL.Path), "/") if len(result) < 2 { return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "err=Unable to find pluginId", http.StatusBadRequest) } + if result[0] != "plugins" { return nil, model.NewAppError("doPluginRequest", "api.post.do_action.action_integration.app_error", nil, "err=plugins not in path", http.StatusBadRequest) } + pluginID := result[1] path := strings.TrimPrefix(inURL.Path, "plugins/"+pluginID)
server/channels/app/integration_action_test.go+185 −0 modified@@ -1198,6 +1198,105 @@ func TestPostActionRelativePluginURL(t *testing.T) { }) } +func TestGetPostActionClient(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + tests := []struct { + name string + siteURL string + subpath string + requestURL string + expectAuth bool + }{ + { + name: "same host with plugin path gets auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/plugins/myplugin/action", + expectAuth: true, + }, + { + name: "same host with non-plugin path does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/api/v4/posts", + expectAuth: false, + }, + { + name: "different host with plugin path does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://evil.com/plugins/myplugin/action", + expectAuth: false, + }, + { + name: "different host same port does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://attacker.com:8065/plugins/myplugin/action", + expectAuth: false, + }, + { + name: "path traversal to reach plugins does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/api/../../plugins/myplugin", + expectAuth: true, // path.Clean normalizes to /plugins/myplugin + }, + { + name: "path traversal escaping plugins does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/plugins/../api/v4/posts", + expectAuth: false, // path.Clean normalizes to /api/v4/posts + }, + { + name: "subpath with plugin path gets auth", + siteURL: "http://localhost:8065/mattermost", + subpath: "/mattermost", + requestURL: "http://localhost:8065/mattermost/plugins/myplugin/action", + expectAuth: true, + }, + { + name: "subpath without subpath prefix does not get auth", + siteURL: "http://localhost:8065/mattermost", + subpath: "/mattermost", + requestURL: "http://localhost:8065/plugins/myplugin/action", + expectAuth: false, // plugins path doesn't include subpath + }, + { + name: "empty path does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/", + expectAuth: false, + }, + { + name: "plugins as query param does not get auth", + siteURL: "http://localhost:8065", + requestURL: "http://localhost:8065/api?path=plugins/myplugin", + expectAuth: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = tc.siteURL + }) + + inURL, err := url.Parse(tc.requestURL) + require.NoError(t, err) + + req, err := http.NewRequest("POST", tc.requestURL, nil) + require.NoError(t, err) + + _ = th.App.getPostActionClient(th.Context, inURL, req) + + if tc.expectAuth { + assert.NotEmpty(t, req.Header.Get(model.HeaderAuth), "expected auth header to be set") + assert.Contains(t, req.Header.Get(model.HeaderAuth), "Bearer ") + } else { + assert.Empty(t, req.Header.Get(model.HeaderAuth), "expected no auth header") + } + }) + } +} + func TestDoPluginRequest(t *testing.T) { mainHelper.Parallel(t) th := Setup(t) @@ -1303,4 +1402,90 @@ func TestDoPluginRequest(t *testing.T) { require.NotNil(t, resp) body, _ = io.ReadAll(resp.Body) assert.Equal(t, "param multiple not correct", string(body)) + + t.Run("should handle URLs with path traversals", func(t *testing.T) { + tests := []struct { + name string + rawURL string + expectErr bool + errDetail string + }{ + { + name: "path traversal to escape plugins directory", + rawURL: "/plugins/../../../etc/passwd", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "path traversal with encoded slashes", + rawURL: "/plugins/..%2F..%2F..%2Fetc%2Fpasswd", + expectErr: true, // url.Parse decodes %2F, path.Clean normalizes traversal + errDetail: "plugins not in path", + }, + { + name: "double dot in plugin path", + rawURL: "/plugins/../plugins/myplugin/action", + expectErr: false, // path.Clean normalizes this back to plugins/myplugin/action + }, + { + name: "path traversal without leading slash", + rawURL: "plugins/../../../etc/passwd", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "only plugins with no plugin ID", + rawURL: "/plugins/", + expectErr: true, + errDetail: "Unable to find pluginId", + }, + { + name: "just plugins no trailing slash", + rawURL: "/plugins", + expectErr: true, + errDetail: "Unable to find pluginId", + }, + { + name: "non-plugins path", + rawURL: "/api/v4/users", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "path traversal via dot segments after plugin ID", + rawURL: "/plugins/myplugin/../../etc/passwd", + expectErr: true, + errDetail: "plugins not in path", + }, + { + name: "backslash traversal attempt", + rawURL: "/plugins/myplugin/..\\..\\etc\\passwd", + expectErr: false, // backslashes are not path separators in URL paths; treated as literal + }, + { + name: "null byte injection attempt", + rawURL: "/plugins/myplugin\x00/action", + expectErr: true, // url.Parse rejects URLs with null bytes + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp, appErr := th.App.doPluginRequest(th.Context, "GET", tc.rawURL, nil, nil) + if tc.expectErr { + require.NotNil(t, appErr, "expected error for URL: %s", tc.rawURL) + if tc.errDetail != "" { + assert.Contains(t, appErr.DetailedError, tc.errDetail) + } + } else { + // Should not return an app error from path validation; + // may still get a 404 if the plugin doesn't exist, which is fine. + assert.Nil(t, appErr, "unexpected error for URL: %s - %v", tc.rawURL, appErr) + if resp != nil { + resp.Body.Close() + } + } + }) + } + }) }
f6760151c4a7Support Elasticsearch v9 (for v10.11) (#35925)
8 files changed · +182 −11
.github/workflows/server-ci-template.yml+24 −0 modified@@ -264,6 +264,30 @@ jobs: datasource: mmuser:mostest@tcp(mysql:3306)/mattermost_test?charset=utf8mb4&multiStatements=true&maxAllowedPacket=4194304 drivername: mysql logsartifact: mysql-server-test-logs + test-elasticsearch-v8: + name: Elasticsearch v8 Compatibility + needs: check-mattermost-vet + uses: ./.github/workflows/server-test-template.yml + secrets: inherit + with: + name: Elasticsearch v8 Compatibility + datasource: postgres://mmuser:mostest@postgres:5432/mattermost_test?sslmode=disable&connect_timeout=10 + drivername: postgres + logsartifact: elasticsearch-v8-server-test-logs + elasticsearch-version: "8.9.0" + test-target: "test-server-elasticsearch" + test-elasticsearch-v7: + name: Elasticsearch v7 Compatibility + needs: check-mattermost-vet + uses: ./.github/workflows/server-test-template.yml + secrets: inherit + with: + name: Elasticsearch v7 Compatibility + datasource: postgres://mmuser:mostest@postgres:5432/mattermost_test?sslmode=disable&connect_timeout=10 + drivername: postgres + logsartifact: elasticsearch-v7-server-test-logs + elasticsearch-version: "7.17.29" + test-target: "test-server-elasticsearch" test-coverage: # Skip coverage generation for cherry-pick PRs into release branches. if: ${{ github.event_name != 'pull_request' || !startsWith(github.event.pull_request.base.ref, 'release-') }}
.github/workflows/server-test-template.yml+15 −2 modified@@ -22,6 +22,14 @@ on: required: false type: boolean default: false + elasticsearch-version: + required: false + type: string + default: "9.0.0" + test-target: + required: false + type: string + default: "test-server" # -- Test sharding inputs (leave defaults for non-sharded callers) -- shard-index: required: false @@ -75,6 +83,8 @@ jobs: echo "${{ inputs.name }}" > server/test-name echo "${{ github.event.pull_request.number }}" > server/pr-number - name: Run docker compose + env: + ELASTICSEARCH_VERSION: ${{ inputs.elasticsearch-version }} run: | cd server/build docker compose --ansi never run --rm start_dependencies @@ -143,11 +153,11 @@ jobs: env: BUILD_IMAGE: mattermost/mattermost-build-server:${{ steps.go.outputs.GO_VERSION }} run: | - if [[ ${{ github.ref_name }} == 'master' && ${{ inputs.fullyparallel }} != true ]]; then + if [[ ${{ github.ref_name }} == 'master' && ${{ inputs.fullyparallel }} != true && "${{ inputs.test-target }}" == "test-server" ]]; then export RACE_MODE="-race" fi - TEST_TARGET="test-server${RACE_MODE}" + TEST_TARGET="${{ inputs.test-target }}${RACE_MODE}" BUILD_NUMBER="${GITHUB_HEAD_REF}-${GITHUB_RUN_ID}" DOCKER_CMD="make ${TEST_TARGET}" @@ -186,8 +196,10 @@ jobs: disable_search: true files: server/cover.out - name: Stop docker compose + if: ${{ always() }} run: | cd server/build + docker compose --ansi never logs --no-color > ../../docker-compose.log 2>&1 docker compose --ansi never stop - name: Archive logs if: ${{ always() }} @@ -200,4 +212,5 @@ jobs: server/cover.out server/test-name server/pr-number + docker-compose.log
server/build/docker-compose.common.yml+5 −1 modified@@ -87,7 +87,11 @@ services: LDAP_DOMAIN: "mm.test.com" LDAP_ADMIN_PASSWORD: "mostest" elasticsearch: - image: "mattermostdevelopment/mattermost-elasticsearch:8.9.0" + build: + context: . + dockerfile: ./Dockerfile.elasticsearch + args: + ELASTICSEARCH_VERSION: ${ELASTICSEARCH_VERSION:-9.0.0} networks: - mm-test environment:
server/build/Dockerfile.elasticsearch+4 −0 added@@ -0,0 +1,4 @@ +ARG ELASTICSEARCH_VERSION=9.0.0 +FROM docker.elastic.co/elasticsearch/elasticsearch:${ELASTICSEARCH_VERSION} + +RUN /usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-icu analysis-nori analysis-kuromoji analysis-smartcn
server/enterprise/elasticsearch/elasticsearch/check_version_test.go+109 −0 added@@ -0,0 +1,109 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.enterprise for license information. + +package elasticsearch + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + elastic "github.com/elastic/go-elasticsearch/v8" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestClient(t *testing.T, handler http.Handler) *elastic.TypedClient { + t.Helper() + ts := httptest.NewServer(handler) + t.Cleanup(ts.Close) + + client, err := elastic.NewTypedClient(elastic.Config{ + Addresses: []string{ts.URL}, + }) + require.NoError(t, err) + return client +} + +func infoHandler(version string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Elastic-Product", "Elasticsearch") + fmt.Fprintf(w, `{"cluster_name":"test","version":{"number":%q,"build_flavor":"default","build_hash":"abc","build_date":"2024-01-01","build_snapshot":false,"build_type":"docker","lucene_version":"9.0.0","minimum_wire_compatibility_version":"7.0.0","minimum_index_compatibility_version":"7.0.0"}}`, version) + } +} + +func TestCheckVersion(t *testing.T) { + tests := []struct { + name string + version string + wantVersion string + wantMajor int + wantErrID string + }{ + { + name: "ES 8 is supported", + version: "8.9.0", + wantVersion: "8.9.0", + wantMajor: 8, + }, + { + name: "ES 9 is supported", + version: "9.0.0", + wantVersion: "9.0.0", + wantMajor: 9, + }, + { + name: "ES 7 is supported", + version: "7.17.0", + wantVersion: "7.17.0", + wantMajor: 7, + }, + { + name: "ES 6 is too old", + version: "6.8.0", + wantErrID: "ent.elasticsearch.min_version.app_error", + }, + { + name: "ES 10 is too new", + version: "10.0.0", + wantErrID: "ent.elasticsearch.max_version.app_error", + }, + { + name: "invalid version string", + version: "invalid", + wantErrID: "ent.elasticsearch.start.parse_server_version.app_error", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := newTestClient(t, infoHandler(tc.version)) + version, major, appErr := checkVersion(client, nil) + if tc.wantErrID != "" { + require.NotNil(t, appErr) + assert.Equal(t, tc.wantErrID, appErr.Id) + } else { + require.Nil(t, appErr) + assert.Equal(t, tc.wantVersion, version) + assert.Equal(t, tc.wantMajor, major) + } + }) + } +} + +func TestCheckVersionConnectionError(t *testing.T) { + ts := httptest.NewServer(http.NotFoundHandler()) + ts.Close() // close immediately to force connection error + + client, err := elastic.NewTypedClient(elastic.Config{ + Addresses: []string{ts.URL}, + MaxRetries: 0, + }) + require.NoError(t, err) + + _, _, appErr := checkVersion(client, nil) + require.NotNil(t, appErr) + assert.Equal(t, "ent.elasticsearch.start.get_server_version.app_error", appErr.Id) +}
server/enterprise/elasticsearch/elasticsearch/elasticsearch.go+11 −7 modified@@ -28,7 +28,8 @@ import ( "github.com/elastic/go-elasticsearch/v8/typedapi/types/enums/sortorder" ) -const elasticsearchMaxVersion = 8 +const elasticsearchMinVersion = 7 +const elasticsearchMaxVersion = 9 var ( purgeIndexListAllowedIndexes = []string{common.IndexBaseChannels} @@ -106,7 +107,7 @@ func (es *ElasticsearchInterfaceImpl) Start() *model.AppError { return appErr } - version, major, appErr := checkMaxVersion(es.client, es.Platform.Config()) + version, major, appErr := checkVersion(es.client, es.Platform.Config()) if appErr != nil { return appErr } @@ -1245,7 +1246,7 @@ func (es *ElasticsearchInterfaceImpl) TestConfig(rctx request.CTX, cfg *model.Co return appErr } - _, _, appErr = checkMaxVersion(client, cfg) + _, _, appErr = checkVersion(client, cfg) if appErr != nil { return appErr } @@ -1830,19 +1831,22 @@ func (es *ElasticsearchInterfaceImpl) DeleteFilesBatch(rctx request.CTX, endTime return nil } -func checkMaxVersion(client *elastic.TypedClient, cfg *model.Config) (string, int, *model.AppError) { +func checkVersion(client *elastic.TypedClient, cfg *model.Config) (string, int, *model.AppError) { resp, err := client.API.Core.Info().Do(context.Background()) if err != nil { - return "", 0, model.NewAppError("Elasticsearch.checkMaxVersion", "ent.elasticsearch.start.get_server_version.app_error", map[string]any{"Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusInternalServerError).Wrap(err) + return "", 0, model.NewAppError("Elasticsearch.checkVersion", "ent.elasticsearch.start.get_server_version.app_error", map[string]any{"Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusInternalServerError).Wrap(err) } major, _, _, esErr := common.GetVersionComponents(resp.Version.Int) if esErr != nil { - return "", 0, model.NewAppError("Elasticsearch.checkMaxVersion", "ent.elasticsearch.start.parse_server_version.app_error", map[string]any{"Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusInternalServerError).Wrap(err) + return "", 0, model.NewAppError("Elasticsearch.checkVersion", "ent.elasticsearch.start.parse_server_version.app_error", map[string]any{"Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusInternalServerError).Wrap(esErr) } + if major < elasticsearchMinVersion { + return "", 0, model.NewAppError("Elasticsearch.checkVersion", "ent.elasticsearch.min_version.app_error", map[string]any{"Version": major, "MinVersion": elasticsearchMinVersion, "Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusBadRequest) + } if major > elasticsearchMaxVersion { - return "", 0, model.NewAppError("Elasticsearch.checkMaxVersion", "ent.elasticsearch.max_version.app_error", map[string]any{"Version": major, "MaxVersion": elasticsearchMaxVersion, "Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusBadRequest) + return "", 0, model.NewAppError("Elasticsearch.checkVersion", "ent.elasticsearch.max_version.app_error", map[string]any{"Version": major, "MaxVersion": elasticsearchMaxVersion, "Backend": model.ElasticsearchSettingsESBackend}, "", http.StatusBadRequest) } return resp.Version.Int, major, nil }
server/i18n/en.json+4 −0 modified@@ -8228,6 +8228,10 @@ "id": "ent.elasticsearch.max_version.app_error", "translation": "{{.Backend}} version {{.Version}} is higher than max supported version of {{.MaxVersion}}" }, + { + "id": "ent.elasticsearch.min_version.app_error", + "translation": "{{.Backend}} version {{.Version}} is lower than min supported version of {{.MinVersion}}" + }, { "id": "ent.elasticsearch.not_started.error", "translation": "{{.Backend}} is not started"
server/Makefile+10 −1 modified@@ -1,4 +1,4 @@ -.PHONY: build package run stop run-client run-server run-node run-haserver stop-haserver stop-client stop-server restart restart-server restart-client restart-haserver start-docker update-docker clean-dist clean nuke check-style check-client-style check-server-style check-unit-tests test dist run-client-tests setup-run-client-tests cleanup-run-client-tests test-client build-linux build-osx build-windows package-prep package-linux package-osx package-windows internal-test-web-client vet run-server-for-web-client-tests diff-config prepackaged-plugins prepackaged-binaries test-server test-server-ee test-server-quick test-server-race test-mmctl-unit test-mmctl-e2e test-mmctl test-mmctl-coverage mmctl-build mmctl-docs new-migration migrations-extract test-public mocks-public +.PHONY: build package run stop run-client run-server run-node run-haserver stop-haserver stop-client stop-server restart restart-server restart-client restart-haserver start-docker update-docker clean-dist clean nuke check-style check-client-style check-server-style check-unit-tests test dist run-client-tests setup-run-client-tests cleanup-run-client-tests test-client build-linux build-osx build-windows package-prep package-linux package-osx package-windows internal-test-web-client vet run-server-for-web-client-tests diff-config prepackaged-plugins prepackaged-binaries test-server test-server-ee test-server-elasticsearch test-server-quick test-server-race test-mmctl-unit test-mmctl-e2e test-mmctl test-mmctl-coverage mmctl-build mmctl-docs new-migration migrations-extract test-public mocks-public ROOT := $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) @@ -476,6 +476,15 @@ test-server-ee: check-prereqs-enterprise start-docker gotestsum ## Runs EE tests @echo Running only EE tests $(GOBIN)/gotestsum --packages="$(EE_PACKAGES)" -- $(GOFLAGS) -timeout=20m +ES_PACKAGES=$(shell $(GO) list ./enterprise/elasticsearch/...) + +test-server-elasticsearch: export GOTESTSUM_FORMAT := $(GOTESTSUM_FORMAT) +test-server-elasticsearch: export GOTESTSUM_JUNITFILE := $(GOTESTSUM_JUNITFILE) +test-server-elasticsearch: export GOTESTSUM_JSONFILE := $(GOTESTSUM_JSONFILE) +test-server-elasticsearch: check-prereqs-enterprise start-docker gotestsum ## Runs Elasticsearch tests. + @echo Running only Elasticsearch tests + $(GOBIN)/gotestsum --rerun-fails=3 --packages="$(ES_PACKAGES)" -- $(GOFLAGS) -timeout=20m + test-server-quick: export GOTESTSUM_FORMAT := $(GOTESTSUM_FORMAT) test-server-quick: export GOTESTSUM_JUNITFILE := $(GOTESTSUM_JUNITFILE) test-server-quick: export GOTESTSUM_JSONFILE := $(GOTESTSUM_JSONFILE)
ffee10a61081Bump Boards FIPS version to v9.2.4 (#36165) (#36168)
1 file changed · +1 −1
server/Makefile+1 −1 modified@@ -183,7 +183,7 @@ PLUGIN_PACKAGES += mattermost-plugin-channel-export-v1.3.0 ifeq ($(FIPS_ENABLED),true) PLUGIN_PACKAGES = mattermost-plugin-playbooks-v2.8.0%2Bc4449ac-fips PLUGIN_PACKAGES += mattermost-plugin-agents-v1.7.2%2B866e2dd-fips - PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.2%2B4282c63-fips + PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.4%2B5855fe1-fips endif EE_PACKAGES=$(shell $(GO) list $(BUILD_ENTERPRISE_DIR)/...)
292d4b7ea15bBump Boards FIPS version to v9.2.4 (#36165) (#36170)
1 file changed · +1 −1
server/Makefile+1 −1 modified@@ -174,7 +174,7 @@ PLUGIN_PACKAGES += mattermost-plugin-channel-export-v1.3.0 ifeq ($(FIPS_ENABLED),true) PLUGIN_PACKAGES = mattermost-plugin-playbooks-v2.8.0%2Bc4449ac-fips PLUGIN_PACKAGES += mattermost-plugin-agents-v1.7.2%2B866e2dd-fips - PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.2%2B4282c63-fips + PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.4%2B5855fe1-fips endif EE_PACKAGES=$(shell $(GO) list $(BUILD_ENTERPRISE_DIR)/...)
fff6ab3a5851Bump Boards FIPS version to v9.2.4 (#36165) (#36169)
1 file changed · +1 −1
server/Makefile+1 −1 modified@@ -176,7 +176,7 @@ PLUGIN_PACKAGES += mattermost-plugin-channel-export-v1.3.0 ifeq ($(FIPS_ENABLED),true) PLUGIN_PACKAGES = mattermost-plugin-playbooks-v2.8.0%2Bc4449ac-fips PLUGIN_PACKAGES += mattermost-plugin-agents-v1.7.2%2B866e2dd-fips - PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.2%2B4282c63-fips + PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.4%2B5855fe1-fips endif EE_PACKAGES=$(shell $(GO) list $(BUILD_ENTERPRISE_DIR)/...)
Vulnerability mechanics
Root cause
"Missing nil pointer checks when dereferencing channel and team objects in persistent notification processing allows a nil dereference panic."
Attack vector
An authenticated attacker sends a crafted HTTP request to the PR details endpoint that references a post whose channel or team has been deleted. Because the server does not validate that the channel and team objects exist before dereferencing them, the nil pointer dereference causes a panic that crashes the plugin process. The attacker must be authenticated and the server must have persistent notifications enabled (PostPriority and AllowPersistentNotifications config settings). The advisory does not specify the exact request shape, but the patch shows the crash occurs in the `forEachPersistentNotificationPost` code path triggered by the PR details endpoint.
Affected code
The vulnerability is in the `forEachPersistentNotificationPost` function in `server/channels/app/post_persistent_notification.go`. When iterating over posts, the code looked up `channel := channelsMap[post.ChannelId]` and `team := teamsMap[channel.TeamId]` without checking if `channel` or `team` were nil. A nil channel dereference would cause a panic, crashing the plugin process. The `persistentNotificationsAuxiliaryData` function in the same file had a similar nil dereference when accessing `teamsMap[c.TeamId]` without a nil check.
What the fix does
The patch adds nil checks before dereferencing `channel` and `team` in `forEachPersistentNotificationPost` [patch_id=1693045]. If `channel` is nil (post references a deleted channel), the post is added to a cleanup list and skipped. If `team` is nil and the channel is not a DM or GM, the post is also cleaned up and skipped. After the loop, orphaned persistent notification records are deleted. Similarly, `persistentNotificationsAuxiliaryData` now skips channels whose team is nil unless the channel is a group or direct message. The `DeleteChannel` function was also reordered to delete the channel from the store before attempting to clean up persistent notifications, preventing a race where the channel still exists in the store during cleanup.
Preconditions
- authAttacker must be authenticated to the Mattermost server
- configServer must have PostPriority and AllowPersistentNotifications config settings enabled
- inputA post must reference a channel or team that has been deleted (or the attacker must be able to trigger such a state)
Generated on May 23, 2026. Inputs: CWE entries + fix-commit diffs from this CVE's patches. Citations validated against bundle.
References
1- mattermost.com/security-updatesmitrevendor-advisory
News mentions
0No linked articles in our index yet.