From 5d3ba7483bad86942aab433a7ccc045614dc284c Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 24 Oct 2023 15:08:14 +0200 Subject: [PATCH] [MM-53057] Disallow malformed channel names for DMs (#24404) --- .../store/searchtest/file_info_layer.go | 6 +-- server/channels/store/searchtest/helper.go | 6 +-- .../channels/store/searchtest/post_layer.go | 6 +-- .../channels/store/storetest/channel_store.go | 45 +++++++++++-------- server/channels/store/storetest/post_store.go | 8 ++-- server/channels/store/storetest/user_store.go | 2 +- .../services/slackimport/slackimport_test.go | 4 +- server/public/model/bot_test.go | 6 +-- server/public/model/channel.go | 3 ++ server/public/pluginapi/post_test.go | 2 +- 10 files changed, 49 insertions(+), 39 deletions(-) diff --git a/server/channels/store/searchtest/file_info_layer.go b/server/channels/store/searchtest/file_info_layer.go index 3ac205c78a..a33619668c 100644 --- a/server/channels/store/searchtest/file_info_layer.go +++ b/server/channels/store/searchtest/file_info_layer.go @@ -196,7 +196,7 @@ func TestSearchFileInfoStore(t *testing.T, s store.Store, testEngine *SearchTest } func testFileInfoSearchFileInfosIncludingDMs(t *testing.T, th *SearchTestHelper) { - direct, err := th.createDirectChannel(th.Team.Id, "direct-"+th.Team.Id, "direct-"+th.Team.Id, []*model.User{th.User, th.User2}) + direct, err := th.createDirectChannel(th.Team.Id, "direct-"+th.Team.Id, []*model.User{th.User, th.User2}) require.NoError(t, err) defer th.deleteChannel(direct) @@ -237,7 +237,7 @@ func testFileInfoSearchFileInfosIncludingDMs(t *testing.T, th *SearchTestHelper) } func testFileInfoSearchFileInfosWithPagination(t *testing.T, th *SearchTestHelper) { - direct, err := th.createDirectChannel(th.Team.Id, "direct", "direct", []*model.User{th.User, th.User2}) + direct, err := th.createDirectChannel(th.Team.Id, "direct", []*model.User{th.User, th.User2}) require.NoError(t, err) defer th.deleteChannel(direct) @@ -651,7 +651,7 @@ func testFileInfoSearchOrExcludeFileInfosInChannel(t *testing.T, th *SearchTestH } func testFileInfoSearchOrExcludeFileInfosInDMGM(t *testing.T, th *SearchTestHelper) { - direct, err := th.createDirectChannel(th.Team.Id, "direct", "direct", []*model.User{th.User, th.User2}) + direct, err := th.createDirectChannel(th.Team.Id, "direct", []*model.User{th.User, th.User2}) require.NoError(t, err) defer th.deleteChannel(direct) diff --git a/server/channels/store/searchtest/helper.go b/server/channels/store/searchtest/helper.go index de37fdb32e..e3d0600e7a 100644 --- a/server/channels/store/searchtest/helper.go +++ b/server/channels/store/searchtest/helper.go @@ -275,10 +275,10 @@ func (th *SearchTestHelper) createChannel(teamID, name, displayName, purpose str return channel, nil } -func (th *SearchTestHelper) createDirectChannel(teamID, name, displayName string, users []*model.User) (*model.Channel, error) { +func (th *SearchTestHelper) createDirectChannel(teamID, displayName string, users []*model.User) (*model.Channel, error) { channel := &model.Channel{ TeamId: teamID, - Name: name, + Name: model.GetDMNameFromIds(users[0].Id, users[1].Id), DisplayName: displayName, Type: model.ChannelTypeDirect, } @@ -290,7 +290,7 @@ func (th *SearchTestHelper) createDirectChannel(teamID, name, displayName string m2 := &model.ChannelMember{} m2.ChannelId = channel.Id - m2.UserId = users[0].Id + m2.UserId = users[1].Id m2.NotifyProps = model.GetDefaultChannelNotifyProps() channel, err := th.Store.Channel().SaveDirectChannel(channel, m1, m2) diff --git a/server/channels/store/searchtest/post_layer.go b/server/channels/store/searchtest/post_layer.go index 859ffb003f..116e92b282 100644 --- a/server/channels/store/searchtest/post_layer.go +++ b/server/channels/store/searchtest/post_layer.go @@ -288,7 +288,7 @@ func TestSearchPostStore(t *testing.T, s store.Store, testEngine *SearchTestEngi } func testSearchPostsIncludingDMs(t *testing.T, th *SearchTestHelper) { - direct, err := th.createDirectChannel(th.Team.Id, "direct", "direct", []*model.User{th.User, th.User2}) + direct, err := th.createDirectChannel(th.Team.Id, "direct", []*model.User{th.User, th.User2}) require.NoError(t, err) defer th.deleteChannel(direct) @@ -310,7 +310,7 @@ func testSearchPostsIncludingDMs(t *testing.T, th *SearchTestHelper) { } func testSearchPostsWithPagination(t *testing.T, th *SearchTestHelper) { - direct, err := th.createDirectChannel(th.Team.Id, "direct", "direct", []*model.User{th.User, th.User2}) + direct, err := th.createDirectChannel(th.Team.Id, "direct", []*model.User{th.User, th.User2}) require.NoError(t, err) defer th.deleteChannel(direct) @@ -675,7 +675,7 @@ func testSearchOrExcludePostsInChannel(t *testing.T, th *SearchTestHelper) { } func testSearchOrExcludePostsInDMGM(t *testing.T, th *SearchTestHelper) { - direct, err := th.createDirectChannel(th.Team.Id, "direct", "direct", []*model.User{th.User, th.User2}) + direct, err := th.createDirectChannel(th.Team.Id, "direct", []*model.User{th.User, th.User2}) require.NoError(t, err) defer th.deleteChannel(direct) diff --git a/server/channels/store/storetest/channel_store.go b/server/channels/store/storetest/channel_store.go index 45d3212033..49a38237c6 100644 --- a/server/channels/store/storetest/channel_store.go +++ b/server/channels/store/storetest/channel_store.go @@ -172,7 +172,7 @@ func testChannelStoreSave(t *testing.T, ss store.Store) { require.Error(t, nErr, "should be unique name") o1.Id = "" - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect _, nErr = ss.Channel().Save(&o1, -1) require.Error(t, nErr, "should not be able to save direct channel") @@ -209,7 +209,7 @@ func testChannelStoreSaveDirectChannel(t *testing.T, ss store.Store, s SqlStore) o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "Name" - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect u1 := &model.User{} @@ -272,7 +272,7 @@ func testChannelStoreSaveDirectChannel(t *testing.T, ss store.Store, s SqlStore) // Save yourself Direct Message o1.Id = "" o1.DisplayName = "Myself" - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect _, nErr = ss.Channel().SaveDirectChannel(&o1, &m1, &m1) require.NoError(t, nErr, "couldn't save direct channel", nErr) @@ -446,7 +446,7 @@ func testChannelStoreGet(t *testing.T, ss store.Store, s SqlStore) { o2 := model.Channel{} o2.TeamId = model.NewId() o2.DisplayName = "Direct Name" - o2.Name = NewTestId() + o2.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o2.Type = model.ChannelTypeDirect m1 := model.ChannelMember{} @@ -548,7 +548,7 @@ func testChannelStoreGetChannelsByIds(t *testing.T, ss store.Store) { o2 := model.Channel{} o2.TeamId = model.NewId() o2.DisplayName = "Direct Name" - o2.Name = "bb" + model.NewId() + o2.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o2.Type = model.ChannelTypeDirect o3 := model.Channel{} @@ -580,8 +580,7 @@ func testChannelStoreGetChannelsByIds(t *testing.T, ss store.Store) { r1, err := ss.Channel().GetChannelsByIds([]string{o1.Id, o2.Id}, false) require.NoError(t, err, err) require.Len(t, r1, 2, "invalid returned channels, expected 2 and got "+strconv.Itoa(len(r1))) - require.Equal(t, channelToJSON(t, &o1), channelToJSON(t, r1[0])) - require.Equal(t, channelToJSON(t, &o2), channelToJSON(t, r1[1])) + require.ElementsMatch(t, []*model.Channel{&o1, &o2}, r1) }) t.Run("Get 1 existing and 1 not existing channel", func(t *testing.T) { @@ -589,16 +588,14 @@ func testChannelStoreGetChannelsByIds(t *testing.T, ss store.Store) { r2, err := ss.Channel().GetChannelsByIds([]string{o1.Id, nonexistentId}, false) require.NoError(t, err, err) require.Len(t, r2, 1, "invalid returned channels, expected 1 and got "+strconv.Itoa(len(r2))) - require.Equal(t, channelToJSON(t, &o1), channelToJSON(t, r2[0]), "invalid returned channel") + require.ElementsMatch(t, []*model.Channel{&o1}, r2, "invalid returned channel") }) t.Run("Get 2 existing and 1 deleted channel", func(t *testing.T) { r1, err := ss.Channel().GetChannelsByIds([]string{o1.Id, o2.Id, o3.Id}, true) require.NoError(t, err, err) require.Len(t, r1, 3, "invalid returned channels, expected 3 and got "+strconv.Itoa(len(r1))) - require.Equal(t, channelToJSON(t, &o1), channelToJSON(t, r1[0])) - require.Equal(t, channelToJSON(t, &o2), channelToJSON(t, r1[1])) - require.Equal(t, channelToJSON(t, &o3), channelToJSON(t, r1[2])) + require.ElementsMatch(t, []*model.Channel{&o1, &o2, &o3}, r1) }) } @@ -640,7 +637,7 @@ func testGetChannelsWithTeamDataByIds(t *testing.T, ss store.Store) { c2 := model.Channel{} c2.TeamId = t1.Id c2.DisplayName = "Direct Name" - c2.Name = "bb" + model.NewId() + c2.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) c2.Type = model.ChannelTypeDirect c3 := model.Channel{} @@ -671,10 +668,20 @@ func testGetChannelsWithTeamDataByIds(t *testing.T, ss store.Store) { res, err := ss.Channel().GetChannelsWithTeamDataByIds([]string{c1.Id, c2.Id}, false) require.NoError(t, err) require.Len(t, res, 2) - assert.Equal(t, res[0].Id, c1.Id) - assert.Equal(t, res[0].TeamName, t1.Name) - assert.Equal(t, res[1].Id, c2.Id) - assert.Equal(t, res[1].TeamName, "") + + if res[0].Id == c1.Id { + assert.Equal(t, res[0].TeamName, t1.Name) + + assert.Equal(t, res[1].Id, c2.Id) + assert.Equal(t, res[1].TeamName, "") + } else if res[0].Id == c2.Id { + assert.Equal(t, res[0].TeamName, "") + + assert.Equal(t, res[1].Id, c1.Id) + assert.Equal(t, res[1].TeamName, t1.Name) + } else { + assert.Fail(t, "unknown channel id") + } } func testChannelStoreGetForPost(t *testing.T, ss store.Store) { @@ -7469,7 +7476,7 @@ func testChannelStoreExportAllDirectChannels(t *testing.T, ss store.Store, s Sql o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "Name" + model.NewId() - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect userIds := []string{model.NewId(), model.NewId(), model.NewId()} @@ -7526,7 +7533,7 @@ func testChannelStoreExportAllDirectChannelsExcludePrivateAndPublic(t *testing.T o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "The Direct Channel" + model.NewId() - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect o2 := model.Channel{} @@ -7588,7 +7595,7 @@ func testChannelStoreExportAllDirectChannelsDeletedChannel(t *testing.T, ss stor o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "Different Name" + model.NewId() - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect u1 := &model.User{} diff --git a/server/channels/store/storetest/post_store.go b/server/channels/store/storetest/post_store.go index caa995939e..50f4a86fa3 100644 --- a/server/channels/store/storetest/post_store.go +++ b/server/channels/store/storetest/post_store.go @@ -3055,7 +3055,7 @@ func testPostStoreGetFlaggedPostsForTeam(t *testing.T, ss store.Store, s SqlStor c2 := &model.Channel{} c2.DisplayName = "DMChannel1" - c2.Name = NewTestId() + c2.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) c2.Type = model.ChannelTypeDirect m1 := &model.ChannelMember{} @@ -4410,7 +4410,7 @@ func testPostStoreGetDirectPostParentsForExportAfter(t *testing.T, ss store.Stor o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "Name" - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect u1 := &model.User{} @@ -4464,7 +4464,7 @@ func testPostStoreGetDirectPostParentsForExportAfterDeleted(t *testing.T, ss sto o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "Name" - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect u1 := &model.User{} @@ -4530,7 +4530,7 @@ func testPostStoreGetDirectPostParentsForExportAfterBatched(t *testing.T, ss sto o1 := model.Channel{} o1.TeamId = teamId o1.DisplayName = "Name" - o1.Name = NewTestId() + o1.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) o1.Type = model.ChannelTypeDirect var postIds []string diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go index 4a1426aa9a..a866762223 100644 --- a/server/channels/store/storetest/user_store.go +++ b/server/channels/store/storetest/user_store.go @@ -2398,7 +2398,7 @@ func testUserUnreadCount(t *testing.T, ss store.Store) { c2 := model.Channel{} c2.TeamId = teamId c2.DisplayName = "Unread Direct" - c2.Name = "unread-direct-" + model.NewId() + c2.Name = model.GetDMNameFromIds(NewTestId(), NewTestId()) c2.Type = model.ChannelTypeDirect u1 := &model.User{} diff --git a/server/platform/services/slackimport/slackimport_test.go b/server/platform/services/slackimport/slackimport_test.go index 9ba7f4c624..407ebce088 100644 --- a/server/platform/services/slackimport/slackimport_test.go +++ b/server/platform/services/slackimport/slackimport_test.go @@ -344,7 +344,7 @@ func TestOldImportChannel(t *testing.T) { // ch := th.CreateDmChannel(u1) ch := &model.Channel{ Type: model.ChannelTypeDirect, - Name: "test-channel", + Name: model.GetDMNameFromIds(u1.Id, u2.Id), } users := map[string]*model.User{ u2.Id: u2, @@ -364,7 +364,7 @@ func TestOldImportChannel(t *testing.T) { t.Run("No panic on direct channel with 1 member", func(t *testing.T) { ch := &model.Channel{ Type: model.ChannelTypeDirect, - Name: "test-channel", + Name: model.GetDMNameFromIds(u1.Id, u1.Id), } users := map[string]*model.User{ u1.Id: u1, diff --git a/server/public/model/bot_test.go b/server/public/model/bot_test.go index 0a402ff312..a5056b91c9 100644 --- a/server/public/model/bot_test.go +++ b/server/public/model/bot_test.go @@ -676,7 +676,7 @@ func TestIsBotChannel(t *testing.T) { { Name: "a direct channel with another user", Channel: &Channel{ - Name: "user1__user2", + Name: GetDMNameFromIds("user1", "user2"), Type: ChannelTypeDirect, }, Expected: false, @@ -684,7 +684,7 @@ func TestIsBotChannel(t *testing.T) { { Name: "a direct channel with the name containing the bot's ID first", Channel: &Channel{ - Name: "botUserID__user2", + Name: GetDMNameFromIds("botUserID", "user2"), Type: ChannelTypeDirect, }, Expected: true, @@ -692,7 +692,7 @@ func TestIsBotChannel(t *testing.T) { { Name: "a direct channel with the name containing the bot's ID second", Channel: &Channel{ - Name: "user1__botUserID", + Name: GetDMNameFromIds("user1", "botUserID"), Type: ChannelTypeDirect, }, Expected: true, diff --git a/server/public/model/channel.go b/server/public/model/channel.go index 1f44a3e50f..511cf7197a 100644 --- a/server/public/model/channel.go +++ b/server/public/model/channel.go @@ -339,6 +339,9 @@ func (o *Channel) GetOtherUserIdForDM(userId string) string { } userIds := strings.Split(o.Name, "__") + if len(userIds) != 2 { + return "" + } var otherUserId string diff --git a/server/public/pluginapi/post_test.go b/server/public/pluginapi/post_test.go index 8c9aa0ff3d..85eb693f9e 100644 --- a/server/public/pluginapi/post_test.go +++ b/server/public/pluginapi/post_test.go @@ -634,7 +634,7 @@ func TestShouldProcessMessage(t *testing.T) { channelID := "1" api := setupAPI() channel := model.Channel{ - Name: "user1__" + expectedBotID, + Name: model.GetDMNameFromIds("user1", expectedBotID), Type: model.ChannelTypeDirect, } api.On("GetChannel", channelID).Return(&channel, nil)