Skip to content

Commit

Permalink
If autolinking would make posts too long, don't
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@elyscape
elyscape committed Apr 1, 2021
1 parent 681b494 commit 7c930fa
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 8 deletions.
6 changes: 6 additions & 0 deletions server/autolinkplugin/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down
87 changes: 79 additions & 8 deletions server/autolinkplugin/plugin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"

"github.com/mattermost/mattermost-server/v5/model"
Expand Down Expand Up @@ -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,
Expand All @@ -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,
}
Expand All @@ -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,
},
}

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
}
})
}
Expand Down

0 comments on commit 7c930fa

Please sign in to comment.