IntenseWebs/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2025-02-25 18:55:24 -06:00
Code Issues Packages Projects Releases Wiki Activity

Restrict post metadata to allow for potentially unsafe links (#26098)

Browse Source 
* Restrict post metadata to allow for potentially unsafe links

* Enhance tests to test tests.

* Restrict prop to only be active if set to 'true'

* Adress feedback.

* Fix existing test using invalid permalink.

* Fix more tests
This commit is contained in:
Christopher Speller 2024-02-22 13:31:24 -08:00 committed by GitHub
parent 45750dbfc6
commit 729950ef03
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 96 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
server/channels/app/post_metadata.go
View File

@ -35,6 +35,8 @@ type linkMetadataCache struct {
const MaxMetadataImageSize = MaxOpenGraphResponseSize
const UnsafeLinksPostProp = "unsafe_links"
func (s *Server) initPostMetadata() {
// Dump any cached links if the proxy settings have changed so image URLs can be updated
s.platform.AddConfigListener(func(before, after *model.Config) {
@ -169,11 +171,20 @@ func (a *App) getEmbedsAndImages(c request.CTX, post *model.Post, isNewPost bool
post.Metadata = &model.PostMetadata{}
}
if post.Metadata.Embeds == nil {
post.Metadata.Embeds = []*model.PostEmbed{}
}
// Embeds and image dimensions
firstLink, images := a.getFirstLinkAndImages(post.Message)
if post.Metadata.Embeds == nil {
post.Metadata.Embeds = []*model.PostEmbed{}
if unsafeLinksProp := post.GetProp(UnsafeLinksPostProp); unsafeLinksProp != nil {
if prop, ok := unsafeLinksProp.(string); ok && prop == "true" {
images = []string{}
if !looksLikeAPermalink(firstLink, *a.Config().ServiceSettings.SiteURL) {
return post
}
}
}
if embed, err := a.getEmbedForPost(c, post, firstLink, isNewPost); err != nil {
@ -581,8 +592,12 @@ func (a *App) getImagesInMessageAttachments(post *model.Post) []string {
}
func looksLikeAPermalink(url, siteURL string) bool {
expression := fmt.Sprintf(`^(%s).*(/pl/)[a-z0-9]{26}$`, siteURL)
matched, err := regexp.MatchString(expression, strings.TrimSpace(url))
path, hasPrefix := strings.CutPrefix(strings.TrimSpace(url), siteURL)
if !hasPrefix {
return false
}
path = strings.TrimPrefix(path, "/")
matched, err := regexp.MatchString(`^[0-9a-z_-]{1,64}/pl/[a-z0-9]{26}$`, path)
if err != nil {
mlog.Warn("error matching regex", mlog.Err(err))
}

79
server/channels/app/post_metadata_test.go
View File

@ -487,6 +487,76 @@ func TestPreparePostForClient(t *testing.T) {
})
})
t.Run("opengraph unsafe links", func(t *testing.T) {
th := setup(t)
defer th.TearDown()
noAccessServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
assert.Fail(t, "acessed server")
}))
for _, tc := range []struct {
name string
link string
notImplmented bool
}{
{
name: "normal link",
link: "%s",
},
{
name: "normal image",
link: "%s/test-image1.png",
},
{
name: "markdown",
link: "[markdown](%s) link",
// This is because markdown links are not currently supported in the opengraph fetching code
// if you just implmented this, remove the `notImplmented` field
notImplmented: true,
},
{
name: "markdown image",
link: "![markdown](%s/test-image1.png) link",
},
} {
t.Run(tc.name, func(t *testing.T) {
t.Run("prop set", func(t *testing.T) {
prepost := &model.Post{
UserId: th.BasicUser.Id,
ChannelId: th.BasicChannel.Id,
Message: `Bla bla bla: ` + fmt.Sprintf(tc.link, noAccessServer.URL),
}
prepost.AddProp(UnsafeLinksPostProp, "true")
post, err := th.App.CreatePost(th.Context, prepost, th.BasicChannel, false, true)
require.Nil(t, err)
clientPost := th.App.PreparePostForClient(th.Context, post, false, false, false)
assert.Len(t, clientPost.Metadata.Embeds, 0)
assert.Len(t, clientPost.Metadata.Images, 0)
})
if !tc.notImplmented {
t.Run("prop not set", func(t *testing.T) {
prepost := &model.Post{
UserId: th.BasicUser.Id,
ChannelId: th.BasicChannel.Id,
Message: `Bla bla bla: ` + fmt.Sprintf(tc.link, server.URL),
}
post, err := th.App.CreatePost(th.Context, prepost, th.BasicChannel, false, true)
require.Nil(t, err)
clientPost := th.App.PreparePostForClient(th.Context, post, false, false, false)
assert.Greater(t, len(clientPost.Metadata.Embeds)+len(clientPost.Metadata.Images), 0)
})
}
})
}
})
t.Run("message attachment embed", func(t *testing.T) {
th := setup(t)
defer th.TearDown()
@ -1319,7 +1389,7 @@ func TestGetImagesForPost(t *testing.T) {
defer th.TearDown()
ogURL := "https://example.com/index.html"
imageURL := th.App.GetSiteURL() + "/pl/qwertyuiopasdfghjklzxcvbnm"
imageURL := th.App.GetSiteURL() + "/team/pl/qwertyuiopasdfghjklzxcvbnm"
post := &model.Post{
Id: "qwertyuiopasdfghjklzxcvbnm",
@ -2504,7 +2574,7 @@ func TestGetLinkMetadata(t *testing.T) {
*cfg.ServiceSettings.SiteURL = server.URL
})
requestURL := server.URL + "/pl/5rpoy4o3nbgwjm7gs4cm71h6ho"
requestURL := server.URL + "/team/pl/5rpoy4o3nbgwjm7gs4cm71h6ho"
timestamp := int64(1547510400000)
_, _, _, err := th.App.getLinkMetadata(th.Context, requestURL, timestamp, true, "")
@ -2706,6 +2776,7 @@ func TestLooksLikeAPermalink(t *testing.T) {
expect bool
}{
"happy path": {input: fmt.Sprintf("%s/private-core/pl/dppezk51jp8afbhwxf1jpag66r", siteURLWithSubpath), siteURL: siteURLWithSubpath, expect: true},
"happy path redirect": {input: fmt.Sprintf("%s/_redirect/pl/dppezk51jp8afbhwxf1jpag66r", siteURLWithSubpath), siteURL: siteURLWithSubpath, expect: true},
"looks nothing like a permalink": {input: "foobar", siteURL: siteURLWithSubpath, expect: false},
"link has no subpath": {input: fmt.Sprintf("%s/private-core/pl/dppezk51jp8afbhwxf1jpag66r", "http://localhost:8065"), siteURL: siteURLWithSubpath, expect: false},
"without port": {input: fmt.Sprintf("%s/private-core/pl/dppezk51jp8afbhwxf1jpag66r", "http://localhost/foo"), siteURL: siteURLWithSubpath, expect: false},
@ -2732,6 +2803,10 @@ func TestContainsPermalink(t *testing.T) {
const siteURLWithSubpath = "http://localhost:8065/foo"
th.App.UpdateConfig(func(cfg *model.Config) {
*cfg.ServiceSettings.SiteURL = siteURLWithSubpath
})
testCases := []struct {
Description string
Post *model.Post