From e2d1af17dea0d8ae351789c2308bae4296ca9d64 Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Tue, 7 Apr 2020 16:56:07 -0400 Subject: [PATCH] MM-23489 Fix poor Unicode handling in display names (#14214) Filter blacklisted Unicode characters from: user: first name, last name, nickname, bot description, username team: name, display name, description, company name channel: name, display name --- api4/channel_test.go | 58 ++++++++++++++++++++++++++++++++++++ api4/team_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++-- api4/user_test.go | 63 +++++++++++++++++++++++++++++++++++++++ model/channel.go | 5 ++++ model/team.go | 9 ++++++ model/user.go | 16 ++++++++++ model/utils.go | 45 ++++++++++++++++++++++++++++ model/utils_test.go | 31 +++++++++++++++++++ 8 files changed, 296 insertions(+), 2 deletions(-) diff --git a/api4/channel_test.go b/api4/channel_test.go index 2ae21aa341..d54d605346 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -317,6 +317,64 @@ func TestPatchChannel(t *testing.T) { CheckForbiddenStatus(t, resp) } +func TestChannelUnicodeNames(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + Client := th.Client + team := th.BasicTeam + + t.Run("create channel unicode", func(t *testing.T) { + channel := &model.Channel{ + Name: "\u206cenglish\u206dchannel", + DisplayName: "The \u206cEnglish\u206d Channel", + Type: model.CHANNEL_OPEN, + TeamId: team.Id} + + rchannel, resp := Client.CreateChannel(channel) + CheckNoError(t, resp) + CheckCreatedStatus(t, resp) + + require.Equal(t, "englishchannel", rchannel.Name, "bad unicode should be filtered from name") + require.Equal(t, "The English Channel", rchannel.DisplayName, "bad unicode should be filtered from display name") + }) + + t.Run("update channel unicode", func(t *testing.T) { + channel := &model.Channel{ + DisplayName: "Test API Name", + Name: GenerateTestChannelName(), + Type: model.CHANNEL_OPEN, + TeamId: team.Id, + } + channel, _ = Client.CreateChannel(channel) + + channel.Name = "\u206ahistorychannel" + channel.DisplayName = "UFO's and \ufff9stuff\ufffb." + + newChannel, resp := Client.UpdateChannel(channel) + CheckNoError(t, resp) + + require.Equal(t, "historychannel", newChannel.Name, "bad unicode should be filtered from name") + require.Equal(t, "UFO's and stuff.", newChannel.DisplayName, "bad unicode should be filtered from display name") + }) + + t.Run("patch channel unicode", func(t *testing.T) { + patch := &model.ChannelPatch{ + Name: new(string), + DisplayName: new(string), + Header: new(string), + Purpose: new(string), + } + *patch.Name = "\u206ecommunitychannel\u206f" + *patch.DisplayName = "Natalie Tran's \ufffcAwesome Channel" + + channel, resp := Client.PatchChannel(th.BasicChannel.Id, patch) + CheckNoError(t, resp) + + require.Equal(t, "communitychannel", channel.Name, "bad unicode should be filtered from name") + require.Equal(t, "Natalie Tran's Awesome Channel", channel.DisplayName, "bad unicode should be filtered from display name") + }) +} + func TestCreateDirectChannel(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/api4/team_test.go b/api4/team_test.go index ae18c00eda..f52650337e 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -459,6 +459,73 @@ func TestPatchTeamSanitization(t *testing.T) { }) } +func TestTeamUnicodeNames(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + Client := th.Client + + t.Run("create team unicode", func(t *testing.T) { + team := &model.Team{ + Name: GenerateTestUsername(), + DisplayName: "Some\u206c Team", + Description: "A \ufffatest\ufffb channel.", + CompanyName: "\ufeffAcme Inc\ufffc", + Type: model.TEAM_OPEN} + rteam, resp := Client.CreateTeam(team) + CheckNoError(t, resp) + CheckCreatedStatus(t, resp) + + require.Equal(t, "Some Team", rteam.DisplayName, "bad unicode should be filtered from display name") + require.Equal(t, "A test channel.", rteam.Description, "bad unicode should be filtered from description") + require.Equal(t, "Acme Inc", rteam.CompanyName, "bad unicode should be filtered from company name") + }) + + t.Run("update team unicode", func(t *testing.T) { + team := &model.Team{ + DisplayName: "Name", + Description: "Some description", + CompanyName: "Bad Company", + Name: model.NewRandomTeamName(), + Email: "success+" + model.NewId() + "@simulator.amazonses.com", + Type: model.TEAM_OPEN} + team, _ = Client.CreateTeam(team) + + team.DisplayName = "\u206eThe Team\u206f" + team.Description = "A \u17a3great\u17d3 team." + team.CompanyName = "\u206aAcme Inc" + uteam, resp := Client.UpdateTeam(team) + CheckNoError(t, resp) + + require.Equal(t, "The Team", uteam.DisplayName, "bad unicode should be filtered from display name") + require.Equal(t, "A great team.", uteam.Description, "bad unicode should be filtered from description") + require.Equal(t, "Acme Inc", uteam.CompanyName, "bad unicode should be filtered from company name") + }) + + t.Run("patch team unicode", func(t *testing.T) { + team := &model.Team{ + DisplayName: "Name", + Description: "Some description", + CompanyName: "Some company name", + Name: model.NewRandomTeamName(), + Email: "success+" + model.NewId() + "@simulator.amazonses.com", + Type: model.TEAM_OPEN} + team, _ = Client.CreateTeam(team) + + patch := &model.TeamPatch{} + + patch.DisplayName = model.NewString("Goat\u206e Team") + patch.Description = model.NewString("\ufffaGreat team.") + patch.CompanyName = model.NewString("\u202bAcme Inc\u202c") + + rteam, resp := Client.PatchTeam(team.Id, patch) + CheckNoError(t, resp) + + require.Equal(t, "Goat Team", rteam.DisplayName, "bad unicode should be filtered from display name") + require.Equal(t, "Great team.", rteam.Description, "bad unicode should be filtered from description") + require.Equal(t, "Acme Inc", rteam.CompanyName, "bad unicode should be filtered from company name") + }) +} + func TestRegenerateTeamInviteId(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -2366,7 +2433,7 @@ func TestInviteUsersToTeam(t *testing.T) { "TeamDisplayName": th.BasicTeam.DisplayName, "SiteName": th.App.ClientConfig()["SiteName"]}) - //Check if the email was send to the rigth email address + //Check if the email was send to the right email address for _, email := range emailList { var resultsMailbox mailservice.JSONMessageHeaderInbucket err := mailservice.RetryInbucket(5, func() error { @@ -2489,7 +2556,7 @@ func TestInviteGuestsToTeam(t *testing.T) { "TeamDisplayName": th.BasicTeam.DisplayName, "SiteName": th.App.ClientConfig()["SiteName"]}) - //Check if the email was send to the rigth email address + //Check if the email was send to the right email address for _, email := range emailList { var resultsMailbox mailservice.JSONMessageHeaderInbucket err := mailservice.RetryInbucket(5, func() error { diff --git a/api4/user_test.go b/api4/user_test.go index 415dda8647..e2d133291a 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -1567,6 +1567,69 @@ func TestPatchUser(t *testing.T) { CheckNoError(t, resp) } +func TestUserUnicodeNames(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + Client := th.Client + + t.Run("create user unicode", func(t *testing.T) { + user := model.User{ + Email: th.GenerateTestEmail(), + FirstName: "Andrew\u202e", + LastName: "\ufeffWiggin", + Nickname: "Ender\u2028 Wiggin", + Password: "hello1", + Username: "\ufeffwiggin77", + Roles: model.SYSTEM_ADMIN_ROLE_ID + " " + model.SYSTEM_USER_ROLE_ID} + + ruser, resp := Client.CreateUser(&user) + CheckNoError(t, resp) + CheckCreatedStatus(t, resp) + + _, _ = Client.Login(user.Email, user.Password) + + require.Equal(t, "wiggin77", ruser.Username, "Bad Unicode not filtered from username") + require.Equal(t, "Andrew Wiggin", ruser.GetDisplayName(model.SHOW_FULLNAME), "Bad Unicode not filtered from displayname") + require.Equal(t, "Ender Wiggin", ruser.Nickname, "Bad Unicode not filtered from nickname") + }) + + t.Run("update user unicode", func(t *testing.T) { + user := th.CreateUser() + Client.Login(user.Email, user.Password) + + user.Username = "wiggin\ufff9" + user.Nickname = "Ender\u0340 \ufffcWiggin" + user.FirstName = "Andrew\ufff9" + user.LastName = "Wig\u206fgin" + + ruser, resp := Client.UpdateUser(user) + CheckNoError(t, resp) + + require.Equal(t, "wiggin", ruser.Username, "bad unicode should be filtered from username") + require.Equal(t, "Ender Wiggin", ruser.Nickname, "bad unicode should be filtered from nickname") + require.Equal(t, "Andrew Wiggin", ruser.GetDisplayName(model.SHOW_FULLNAME), "bad unicode should be filtered from display name") + }) + + t.Run("patch user unicode", func(t *testing.T) { + user := th.CreateUser() + Client.Login(user.Email, user.Password) + + patch := &model.UserPatch{} + patch.Nickname = model.NewString("\U000E0000Ender\u206d Wiggin\U000E007F") + patch.FirstName = model.NewString("\U0001d173Andrew\U0001d17a") + patch.LastName = model.NewString("\u2028Wiggin\u2029") + + ruser, resp := Client.PatchUser(user.Id, patch) + CheckNoError(t, resp) + CheckUserSanitization(t, ruser) + + require.Equal(t, "Ender Wiggin", ruser.Nickname, "Bad unicode should be filtered from nickname") + require.Equal(t, "Andrew", ruser.FirstName, "Bad unicode should be filtered from first name") + require.Equal(t, "Wiggin", ruser.LastName, "Bad unicode should be filtered from last name") + require.Equal(t, "Andrew Wiggin", ruser.GetDisplayName(model.SHOW_FULLNAME), "Bad unicode should be filtered from display name") + }) +} + func TestUpdateUserAuth(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/model/channel.go b/model/channel.go index b10352434c..5dd5e5a0e3 100644 --- a/model/channel.go +++ b/model/channel.go @@ -230,6 +230,9 @@ func (o *Channel) PreSave() { o.Id = NewId() } + o.Name = SanitizeUnicode(o.Name) + o.DisplayName = SanitizeUnicode(o.DisplayName) + o.CreateAt = GetMillis() o.UpdateAt = o.CreateAt o.ExtraUpdateAt = 0 @@ -237,6 +240,8 @@ func (o *Channel) PreSave() { func (o *Channel) PreUpdate() { o.UpdateAt = GetMillis() + o.Name = SanitizeUnicode(o.Name) + o.DisplayName = SanitizeUnicode(o.DisplayName) } func (o *Channel) IsGroupOrDirect() bool { diff --git a/model/team.go b/model/team.go index 137a8f0b8a..ba92b51d22 100644 --- a/model/team.go +++ b/model/team.go @@ -203,6 +203,11 @@ func (o *Team) PreSave() { o.CreateAt = GetMillis() o.UpdateAt = o.CreateAt + o.Name = SanitizeUnicode(o.Name) + o.DisplayName = SanitizeUnicode(o.DisplayName) + o.Description = SanitizeUnicode(o.Description) + o.CompanyName = SanitizeUnicode(o.CompanyName) + if len(o.InviteId) == 0 { o.InviteId = NewId() } @@ -210,6 +215,10 @@ func (o *Team) PreSave() { func (o *Team) PreUpdate() { o.UpdateAt = GetMillis() + o.Name = SanitizeUnicode(o.Name) + o.DisplayName = SanitizeUnicode(o.DisplayName) + o.Description = SanitizeUnicode(o.Description) + o.CompanyName = SanitizeUnicode(o.CompanyName) } func IsReservedTeamName(s string) bool { diff --git a/model/user.go b/model/user.go index eed5e84093..d499d774a1 100644 --- a/model/user.go +++ b/model/user.go @@ -329,6 +329,11 @@ func (u *User) PreSave() { u.AuthData = nil } + u.Username = SanitizeUnicode(u.Username) + u.FirstName = SanitizeUnicode(u.FirstName) + u.LastName = SanitizeUnicode(u.LastName) + u.Nickname = SanitizeUnicode(u.Nickname) + u.Username = NormalizeUsername(u.Username) u.Email = NormalizeEmail(u.Email) @@ -362,10 +367,21 @@ func (u *User) PreSave() { // PreUpdate should be run before updating the user in the db. func (u *User) PreUpdate() { + u.Username = SanitizeUnicode(u.Username) + u.FirstName = SanitizeUnicode(u.FirstName) + u.LastName = SanitizeUnicode(u.LastName) + u.Nickname = SanitizeUnicode(u.Nickname) + u.BotDescription = SanitizeUnicode(u.BotDescription) + u.Username = NormalizeUsername(u.Username) u.Email = NormalizeEmail(u.Email) u.UpdateAt = GetMillis() + u.FirstName = SanitizeUnicode(u.FirstName) + u.LastName = SanitizeUnicode(u.LastName) + u.Nickname = SanitizeUnicode(u.Nickname) + u.BotDescription = SanitizeUnicode(u.BotDescription) + if u.AuthData != nil && *u.AuthData == "" { u.AuthData = nil } diff --git a/model/utils.go b/model/utils.go index 053af33e6f..aee04a068e 100644 --- a/model/utils.go +++ b/model/utils.go @@ -647,3 +647,48 @@ func AsStringBoolMap(list []string) map[string]bool { } return listMap } + +// SanitizeUnicode will remove undesirable Unicode characters from a string. +func SanitizeUnicode(s string) string { + return strings.Map(filterBlacklist, s) +} + +// filterBlacklist returns `r` if it is not in the blacklist, otherwise drop (-1). +// Blacklist is taken from https://www.w3.org/TR/unicode-xml/#Charlist +func filterBlacklist(r rune) rune { + const drop = -1 + switch r { + case '\u0340', '\u0341': // clones of grave and acute; deprecated in Unicode + return drop + case '\u17A3', '\u17D3': // obsolete characters for Khmer; deprecated in Unicode + return drop + case '\u2028', '\u2029': // line and paragraph separator + return drop + case '\u202A', '\u202B', '\u202C', '\u202D', '\u202E': // BIDI embedding controls + return drop + case '\u206A', '\u206B': // activate/inhibit symmetric swapping; deprecated in Unicode + return drop + case '\u206C', '\u206D': // activate/inhibit Arabic form shaping; deprecated in Unicode + return drop + case '\u206E', '\u206F': // activate/inhibit national digit shapes; deprecated in Unicode + return drop + case '\uFFF9', '\uFFFA', '\uFFFB': // interlinear annotation characters + return drop + case '\uFEFF': // byte order mark + return drop + case '\uFFFC': // object replacement character + return drop + } + + // Scoping for musical notation + if r >= 0x0001D173 && r <= 0x0001D17A { + return drop + } + + // Language tag code points + if r >= 0x000E0000 && r <= 0x000E007F { + return drop + } + + return r +} diff --git a/model/utils_test.go b/model/utils_test.go index 5457688eac..ae49acba9d 100644 --- a/model/utils_test.go +++ b/model/utils_test.go @@ -4,6 +4,7 @@ package model import ( + "bytes" "fmt" "net/http" "reflect" @@ -736,3 +737,33 @@ func checkNowhereNil(t *testing.T, name string, value interface{}) bool { return true } } + +func TestSanitizeUnicode(t *testing.T) { + buf := bytes.Buffer{} + buf.WriteString("Hello") + buf.WriteRune(0x1d173) + buf.WriteRune(0x1d17a) + buf.WriteString(" there.") + + musicArg := buf.String() + musicWant := "Hello there." + + tests := []struct { + name string + arg string + want string + }{ + {name: "empty string", arg: "", want: ""}, + {name: "ascii only", arg: "Hello There", want: "Hello There"}, + {name: "allowed unicode", arg: "Ādam likes Iñtërnâtiônàližætiøn", want: "Ādam likes Iñtërnâtiônàližætiøn"}, + {name: "allowed unicode escaped", arg: "\u00eaI like hats\u00e2", want: "êI like hatsâ"}, + {name: "blacklist char, don't reverse string", arg: "\u202E2resu", want: "2resu"}, + {name: "blacklist chars, scoping musical notation", arg: musicArg, want: musicWant}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeUnicode(tt.arg) + assert.Equal(t, tt.want, got) + }) + } +}