From 7c930fa88b88fbdde8b728a804c6324d0806caa9 Mon Sep 17 00:00:00 2001 From: Eli Young Date: Thu, 1 Apr 2021 13:05:26 -0700 Subject: [PATCH] If autolinking would make posts too long, don't Substitutions that increase message length have the potential to grow a post past the ability of the database to store it. This manifests to users as their posts inexplicably failing. To avoid this fail state, we check to see if the post-substitution message is too long and, if so, don't apply the changes. Unfortunately, the actual maximum post size is not currently accessible to plugins. In the meantime, we use the smallest value for which the server guarantees support, which is exposed through model.POST_MESSAGE_MAX_RUNES_V1 and is in practice 4000 runes. --- server/autolinkplugin/plugin.go | 6 ++ server/autolinkplugin/plugin_test.go | 87 +++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/server/autolinkplugin/plugin.go b/server/autolinkplugin/plugin.go index c338e545..92db84f4 100644 --- a/server/autolinkplugin/plugin.go +++ b/server/autolinkplugin/plugin.go @@ -5,6 +5,7 @@ import ( "net/http" "strings" "sync" + "unicode/utf8" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/plugin" @@ -208,6 +209,11 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post, return nil, "" } + if runes := utf8.RuneCountInString(postText); runes > model.POST_MESSAGE_MAX_RUNES_V1 { + p.API.LogWarn("Rewritten message would be too long, skipping", "rewrittenLength", runes) + return nil, "" + } + post.Message = postText } diff --git a/server/autolinkplugin/plugin_test.go b/server/autolinkplugin/plugin_test.go index 5aa8d81a..b5b24af0 100644 --- a/server/autolinkplugin/plugin_test.go +++ b/server/autolinkplugin/plugin_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/mattermost/mattermost-server/v5/model" @@ -305,6 +306,9 @@ func TestSpecialCases(t *testing.T) { Pattern: "(example)", Template: "test", Scope: []string{"other-team/town-square"}, + }, autolink.Autolink{ + Pattern: "(toolong)", + Template: strings.Repeat("1", model.POST_MESSAGE_MAX_RUNES_V1+1), }) validConfig := Config{ EnableAdminCommand: false, @@ -331,6 +335,8 @@ func TestSpecialCases(t *testing.T) { api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil) api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil) + api.On("LogWarn", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("int")) + testUser := model.User{ IsBot: false, } @@ -344,115 +350,156 @@ func TestSpecialCases(t *testing.T) { var tests = []struct { inputMessage string expectedMessage string + expectedIgnored bool }{ { "hello ``` Mattermost ``` goodbye", "hello ``` Mattermost ``` goodbye", + false, }, { "hello\n```\nMattermost\n```\ngoodbye", "hello\n```\nMattermost\n```\ngoodbye", + false, }, { "Mattermost ``` Mattermost ``` goodbye", "[Mattermost](https://mattermost.com) ``` Mattermost ``` goodbye", + false, }, { "``` Mattermost ``` Mattermost", "``` Mattermost ``` [Mattermost](https://mattermost.com)", + false, }, { "Mattermost ``` Mattermost ```", "[Mattermost](https://mattermost.com) ``` Mattermost ```", + false, }, { "Mattermost ``` Mattermost ```\n\n", "[Mattermost](https://mattermost.com) ``` Mattermost ```\n\n", + false, }, { "hello ` Mattermost ` goodbye", "hello ` Mattermost ` goodbye", + false, }, { "hello\n`\nMattermost\n`\ngoodbye", "hello\n`\nMattermost\n`\ngoodbye", + false, }, { "Mattermost ` Mattermost ` goodbye", "[Mattermost](https://mattermost.com) ` Mattermost ` goodbye", + false, }, { "` Mattermost ` Mattermost", "` Mattermost ` [Mattermost](https://mattermost.com)", + false, }, { "Mattermost ` Mattermost `", "[Mattermost](https://mattermost.com) ` Mattermost `", + false, }, { "Mattermost ` Mattermost `\n\n", "[Mattermost](https://mattermost.com) ` Mattermost `\n\n", + false, }, { "hello ``` Mattermost ``` goodbye ` Mattermost ` end", "hello ``` Mattermost ``` goodbye ` Mattermost ` end", + false, }, { "hello\n```\nMattermost\n```\ngoodbye ` Mattermost ` end", "hello\n```\nMattermost\n```\ngoodbye ` Mattermost ` end", + false, }, { "Mattermost ``` Mattermost ``` goodbye ` Mattermost ` end", "[Mattermost](https://mattermost.com) ``` Mattermost ``` goodbye ` Mattermost ` end", + false, }, { "``` Mattermost ``` Mattermost", "``` Mattermost ``` [Mattermost](https://mattermost.com)", + false, }, { "```\n` Mattermost `\n```\nMattermost", "```\n` Mattermost `\n```\n[Mattermost](https://mattermost.com)", + false, }, { " Mattermost", " [Mattermost](https://mattermost.com)", + false, }, { " Mattermost", " Mattermost", + false, }, { " ```\nMattermost\n ```", " ```\n[Mattermost](https://mattermost.com)\n ```", + false, }, { "` ``` `\nMattermost\n` ``` `", "` ``` `\n[Mattermost](https://mattermost.com)\n` ``` `", + false, }, { "Mattermost \n Mattermost", "[Mattermost](https://mattermost.com) \n [Mattermost](https://mattermost.com)", + false, }, { "[Mattermost](https://mattermost.com)", "[Mattermost](https://mattermost.com)", + false, }, { "[ Mattermost ](https://mattermost.com)", "[ Mattermost ](https://mattermost.com)", + false, }, { "[ Mattermost ][1]\n\n[1]: https://mattermost.com", "[ Mattermost ][1]\n\n[1]: https://mattermost.com", + false, }, { "![ Mattermost ](https://mattermost.com/example.png)", "![ Mattermost ](https://mattermost.com/example.png)", + false, }, { "![ Mattermost ][1]\n\n[1]: https://mattermost.com/example.png", "![ Mattermost ][1]\n\n[1]: https://mattermost.com/example.png", + false, }, { "foo!bar\nExample\nfoo!bar Mattermost", "fb\n[Example](https://example.com)\nfb [Mattermost](https://mattermost.com)", + false, }, { "foo!bar", "fb", + false, }, { "foo!barfoo!bar", "foo!barfoo!bar", + false, }, { "foo!bar & foo!bar", "fb & fb", + false, }, { "foo!bar & foo!bar\nfoo!bar & foo!bar\nfoo!bar & foo!bar", "fb & fb\nfb & fb\nfb & fb", + false, }, { "https://mattermost.atlassian.net/browse/MM-12345", "[MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", + false, }, { "Welcome https://mattermost.atlassian.net/browse/MM-12345", "Welcome [MM-12345](https://mattermost.atlassian.net/browse/MM-12345)", + false, }, { "text https://mattermost.atlassian.net/browse/MM-12345 other text", "text [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) other text", + false, }, { "check out MM-12345 too", "check out [MM-12345](https://mattermost.atlassian.net/browse/MM-12345) too", + false, + }, { + "toolong", + "toolong", + true, }, } @@ -465,9 +512,15 @@ func TestSpecialCases(t *testing.T) { Message: tt.inputMessage, } - rpost, _ := p.MessageWillBePosted(&plugin.Context{}, post) + rpost, err := p.MessageWillBePosted(&plugin.Context{}, post) - assert.Equal(t, tt.expectedMessage, rpost.Message) + if tt.expectedIgnored { + assert.Nil(t, rpost) + } else { + assert.Equal(t, tt.expectedMessage, rpost.Message) + } + + assert.Equal(t, "", err) } { // user updates the modified post but with no changes @@ -476,9 +529,15 @@ func TestSpecialCases(t *testing.T) { Message: tt.expectedMessage, } - rpost, _ := p.MessageWillBeUpdated(&plugin.Context{}, post, post) + rpost, err := p.MessageWillBeUpdated(&plugin.Context{}, post, post) - assert.Equal(t, tt.expectedMessage, rpost.Message) + if tt.expectedIgnored { + assert.Nil(t, rpost) + } else { + assert.Equal(t, tt.expectedMessage, rpost.Message) + } + + assert.Equal(t, "", err) } { // user updates the modified post and sets it back to the original text @@ -490,9 +549,15 @@ func TestSpecialCases(t *testing.T) { Message: tt.inputMessage, } - rpost, _ := p.MessageWillBeUpdated(&plugin.Context{}, originalPost, post) + rpost, err := p.MessageWillBeUpdated(&plugin.Context{}, originalPost, post) + + if tt.expectedIgnored { + assert.Nil(t, rpost) + } else { + assert.Equal(t, tt.expectedMessage, rpost.Message) + } - assert.Equal(t, tt.expectedMessage, rpost.Message) + assert.Equal(t, "", err) } { // user updates an empty post to the original text @@ -502,9 +567,15 @@ func TestSpecialCases(t *testing.T) { Message: tt.inputMessage, } - rpost, _ := p.MessageWillBeUpdated(&plugin.Context{}, post, emptyPost) + rpost, err := p.MessageWillBeUpdated(&plugin.Context{}, post, emptyPost) + + if tt.expectedIgnored { + assert.Nil(t, rpost) + } else { + assert.Equal(t, tt.expectedMessage, rpost.Message) + } - assert.Equal(t, tt.expectedMessage, rpost.Message) + assert.Equal(t, "", err) } }) }