From 2959b53d9815bbd44a5c48edf3d2866f80782044 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 4 Sep 2018 08:33:29 -0400 Subject: [PATCH] MM-11272 Add OpenGraph and image dimension metadata to posts (#9313) * Move OpenGraph code into its own file * Move OpenGraph image proxying to app layer * Move test file code out of api4 package * MM-11272 Add OpenGraph and image dimension metadata to posts --- api4/apitestlib.go | 17 -- api4/brand_test.go | 4 +- api4/file_test.go | 17 +- api4/openGraph.go | 23 --- api4/team_test.go | 9 +- api4/user_test.go | 3 +- app/app.go | 2 + app/opengraph.go | 116 +++++++++++++ app/opengraph_test.go | 129 +++++++++++++++ app/post.go | 83 ---------- app/post_metadata.go | 236 ++++++++++++++++++++++++--- app/post_metadata_test.go | 308 ++++++++++++++++++++++++++++++++++- app/post_test.go | 120 -------------- i18n/en.json | 4 + model/post.go | 4 +- utils/testutils/testutils.go | 29 ++++ 16 files changed, 819 insertions(+), 285 deletions(-) create mode 100644 app/opengraph.go create mode 100644 app/opengraph_test.go create mode 100644 utils/testutils/testutils.go diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 9976932a9a..bae6e8678c 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -4,7 +4,6 @@ package api4 import ( - "bytes" "fmt" "io" "io/ioutil" @@ -746,22 +745,6 @@ func CheckInternalErrorStatus(t *testing.T, resp *model.Response) { } } -func readTestFile(name string) ([]byte, error) { - path, _ := utils.FindDir("tests") - file, err := os.Open(filepath.Join(path, name)) - if err != nil { - return nil, err - } - defer file.Close() - - data := &bytes.Buffer{} - if _, err := io.Copy(data, file); err != nil { - return nil, err - } else { - return data.Bytes(), nil - } -} - // Similar to s3.New() but allows initialization of signature v2 or signature v4 client. // If signV2 input is false, function always returns signature v4. // diff --git a/api4/brand_test.go b/api4/brand_test.go index 37169d99a6..4d627f9441 100644 --- a/api4/brand_test.go +++ b/api4/brand_test.go @@ -6,6 +6,8 @@ package api4 import ( "net/http" "testing" + + "github.com/mattermost/mattermost-server/utils/testutils" ) func TestGetBrandImage(t *testing.T) { @@ -29,7 +31,7 @@ func TestUploadBrandImage(t *testing.T) { defer th.TearDown() Client := th.Client - data, err := readTestFile("test.png") + data, err := testutils.ReadTestFile("test.png") if err != nil { t.Fatal(err) } diff --git a/api4/file_test.go b/api4/file_test.go index 607e54dd7c..0bead4d41a 100644 --- a/api4/file_test.go +++ b/api4/file_test.go @@ -12,6 +12,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + "github.com/mattermost/mattermost-server/utils/testutils" ) func TestUploadFileAsMultipart(t *testing.T) { @@ -25,7 +26,7 @@ func TestUploadFileAsMultipart(t *testing.T) { var uploadInfo *model.FileInfo var data []byte var err error - if data, err = readTestFile("test.png"); err != nil { + if data, err = testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else if fileResp, resp := Client.UploadFile(data, channel.Id, "test.png"); resp.Error != nil { t.Fatal(resp.Error) @@ -138,7 +139,7 @@ func TestUploadFileAsRequestBody(t *testing.T) { var uploadInfo *model.FileInfo var data []byte var err error - if data, err = readTestFile("test.png"); err != nil { + if data, err = testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else if fileResp, resp := Client.UploadFileAsRequestBody(data, channel.Id, "test.png"); resp.Error != nil { t.Fatal(resp.Error) @@ -263,7 +264,7 @@ func TestGetFile(t *testing.T) { fileId := "" var sent []byte var err error - if sent, err = readTestFile("test.png"); err != nil { + if sent, err = testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else { fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") @@ -378,7 +379,7 @@ func TestGetFileThumbnail(t *testing.T) { fileId := "" var sent []byte var err error - if sent, err = readTestFile("test.png"); err != nil { + if sent, err = testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else { fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") @@ -437,7 +438,7 @@ func TestGetFileLink(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.PublicLinkSalt = model.NewId() }) fileId := "" - if data, err := readTestFile("test.png"); err != nil { + if data, err := testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else { fileResp, resp := Client.UploadFile(data, channel.Id, "test.png") @@ -506,7 +507,7 @@ func TestGetFilePreview(t *testing.T) { fileId := "" var sent []byte var err error - if sent, err = readTestFile("test.png"); err != nil { + if sent, err = testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else { fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") @@ -559,7 +560,7 @@ func TestGetFileInfo(t *testing.T) { fileId := "" var sent []byte var err error - if sent, err = readTestFile("test.png"); err != nil { + if sent, err = testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else { fileResp, resp := Client.UploadFile(sent, channel.Id, "test.png") @@ -632,7 +633,7 @@ func TestGetPublicFile(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.PublicLinkSalt = GenerateTestId() }) fileId := "" - if data, err := readTestFile("test.png"); err != nil { + if data, err := testutils.ReadTestFile("test.png"); err != nil { t.Fatal(err) } else { fileResp, resp := Client.UploadFile(data, channel.Id, "test.png") diff --git a/api4/openGraph.go b/api4/openGraph.go index af0989ab95..143c0279ba 100644 --- a/api4/openGraph.go +++ b/api4/openGraph.go @@ -6,7 +6,6 @@ package api4 import ( "net/http" - "github.com/dyatlov/go-opengraph/opengraph" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" ) @@ -27,22 +26,6 @@ func (api *API) InitOpenGraph() { }) } -func OpenGraphDataWithProxyAddedToImageURLs(ogdata *opengraph.OpenGraph, toProxyURL func(string) string) *opengraph.OpenGraph { - for _, image := range ogdata.Images { - var url string - if image.SecureURL != "" { - url = image.SecureURL - } else { - url = image.URL - } - - image.URL = "" - image.SecureURL = toProxyURL(url) - } - - return ogdata -} - func getOpenGraphMetadata(c *Context, w http.ResponseWriter, r *http.Request) { if !*c.App.Config().ServiceSettings.EnableLinkPreviews { c.Err = model.NewAppError("getOpenGraphMetadata", "api.post.link_preview_disabled.app_error", nil, "", http.StatusNotImplemented) @@ -65,12 +48,6 @@ func getOpenGraphMetadata(c *Context, w http.ResponseWriter, r *http.Request) { } og := c.App.GetOpenGraphMetadata(url) - - // If image proxy enabled modify open graph data to feed though proxy - if toProxyURL := c.App.ImageProxyAdder(); toProxyURL != nil { - og = OpenGraphDataWithProxyAddedToImageURLs(og, toProxyURL) - } - ogJSON, err := og.ToJSON() openGraphDataCache.AddWithExpiresInSecs(props["url"], ogJSON, 3600) // Cache would expire after 1 hour if err != nil { diff --git a/api4/team_test.go b/api4/team_test.go index 8f1027cc5b..e82e05cfc7 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -17,6 +17,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/services/mailservice" "github.com/mattermost/mattermost-server/utils" + "github.com/mattermost/mattermost-server/utils/testutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1813,7 +1814,7 @@ func TestImportTeam(t *testing.T) { t.Run("ImportTeam", func(t *testing.T) { var data []byte var err error - data, err = readTestFile("Fake_Team_Import.zip") + data, err = testutils.ReadTestFile("Fake_Team_Import.zip") if err != nil && len(data) == 0 { t.Fatal("Error while reading the test file.") } @@ -1874,7 +1875,7 @@ func TestImportTeam(t *testing.T) { t.Run("WrongPermission", func(t *testing.T) { var data []byte var err error - data, err = readTestFile("Fake_Team_Import.zip") + data, err = testutils.ReadTestFile("Fake_Team_Import.zip") if err != nil && len(data) == 0 { t.Fatal("Error while reading the test file.") } @@ -2029,7 +2030,7 @@ func TestSetTeamIcon(t *testing.T) { Client := th.Client team := th.BasicTeam - data, err := readTestFile("test.png") + data, err := testutils.ReadTestFile("test.png") if err != nil { t.Fatal(err) } @@ -2109,7 +2110,7 @@ func TestRemoveTeamIcon(t *testing.T) { team := th.BasicTeam th.LoginTeamAdmin() - data, _ := readTestFile("test.png") + data, _ := testutils.ReadTestFile("test.png") Client.SetTeamIcon(team.Id, data) _, resp := Client.RemoveTeamIcon(team.Id) diff --git a/api4/user_test.go b/api4/user_test.go index f7970128f5..b3572052c0 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -12,6 +12,7 @@ import ( "github.com/mattermost/mattermost-server/app" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + "github.com/mattermost/mattermost-server/utils/testutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -2265,7 +2266,7 @@ func TestSetProfileImage(t *testing.T) { Client := th.Client user := th.BasicUser - data, err := readTestFile("test.png") + data, err := testutils.ReadTestFile("test.png") if err != nil { t.Fatal(err) } diff --git a/app/app.go b/app/app.go index e3a919bba4..ea56b047df 100644 --- a/app/app.go +++ b/app/app.go @@ -262,6 +262,8 @@ func New(options ...Option) (outApp *App, outErr error) { handlers: make(map[string]webSocketHandler), } + app.InitPostMetadata() + return app, nil } diff --git a/app/opengraph.go b/app/opengraph.go new file mode 100644 index 0000000000..7066706736 --- /dev/null +++ b/app/opengraph.go @@ -0,0 +1,116 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package app + +import ( + "io" + "net/url" + + "github.com/dyatlov/go-opengraph/opengraph" + "github.com/mattermost/mattermost-server/mlog" + "golang.org/x/net/html/charset" +) + +func (a *App) GetOpenGraphMetadata(requestURL string) *opengraph.OpenGraph { + res, err := a.HTTPClient(false).Get(requestURL) + if err != nil { + mlog.Error("GetOpenGraphMetadata request failed", mlog.String("requestURL", requestURL), mlog.Any("err", err)) + return nil + } + defer consumeAndClose(res) + + return a.ParseOpenGraphMetadata(requestURL, res.Body, res.Header.Get("Content-Type")) +} + +func (a *App) ParseOpenGraphMetadata(requestURL string, body io.Reader, contentType string) *opengraph.OpenGraph { + og := opengraph.NewOpenGraph() + + body = forceHTMLEncodingToUTF8(body, contentType) + + if err := og.ProcessHTML(body); err != nil { + mlog.Error("ParseOpenGraphMetadata processing failed", mlog.String("requestURL", requestURL), mlog.Any("err", err)) + } + + makeOpenGraphURLsAbsolute(og, requestURL) + + // If image proxy enabled modify open graph data to feed though proxy + if toProxyURL := a.ImageProxyAdder(); toProxyURL != nil { + og = OpenGraphDataWithProxyAddedToImageURLs(og, toProxyURL) + } + + // The URL should be the link the user provided in their message, not a redirected one. + if og.URL != "" { + og.URL = requestURL + } + + return og +} + +func forceHTMLEncodingToUTF8(body io.Reader, contentType string) io.Reader { + r, err := charset.NewReader(body, contentType) + if err != nil { + mlog.Error("forceHTMLEncodingToUTF8 failed to convert", mlog.String("contentType", contentType), mlog.Any("err", err)) + return body + } + return r +} + +func makeOpenGraphURLsAbsolute(og *opengraph.OpenGraph, requestURL string) { + parsedRequestURL, err := url.Parse(requestURL) + if err != nil { + mlog.Warn("makeOpenGraphURLsAbsolute failed to parse url", mlog.String("requestURL", requestURL), mlog.Any("err", err)) + return + } + + makeURLAbsolute := func(resultURL string) string { + if resultURL == "" { + return resultURL + } + + parsedResultURL, err := url.Parse(resultURL) + if err != nil { + mlog.Warn("makeOpenGraphURLsAbsolute failed to parse result", mlog.String("requestURL", requestURL), mlog.Any("err", err)) + return resultURL + } + + if parsedResultURL.IsAbs() { + return resultURL + } + + return parsedRequestURL.ResolveReference(parsedResultURL).String() + } + + og.URL = makeURLAbsolute(og.URL) + + for _, image := range og.Images { + image.URL = makeURLAbsolute(image.URL) + image.SecureURL = makeURLAbsolute(image.SecureURL) + } + + for _, audio := range og.Audios { + audio.URL = makeURLAbsolute(audio.URL) + audio.SecureURL = makeURLAbsolute(audio.SecureURL) + } + + for _, video := range og.Videos { + video.URL = makeURLAbsolute(video.URL) + video.SecureURL = makeURLAbsolute(video.SecureURL) + } +} + +func OpenGraphDataWithProxyAddedToImageURLs(ogdata *opengraph.OpenGraph, toProxyURL func(string) string) *opengraph.OpenGraph { + for _, image := range ogdata.Images { + var url string + if image.SecureURL != "" { + url = image.SecureURL + } else { + url = image.URL + } + + image.URL = "" + image.SecureURL = toProxyURL(url) + } + + return ogdata +} diff --git a/app/opengraph_test.go b/app/opengraph_test.go new file mode 100644 index 0000000000..78e9de17d0 --- /dev/null +++ b/app/opengraph_test.go @@ -0,0 +1,129 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package app + +import ( + "strings" + "testing" + + "github.com/dyatlov/go-opengraph/opengraph" +) + +func BenchmarkForceHTMLEncodingToUTF8(b *testing.B) { + HTML := ` + + + + + + + ` + ContentType := "text/html; utf-8" + + b.Run("with converting", func(b *testing.B) { + for i := 0; i < b.N; i++ { + r := forceHTMLEncodingToUTF8(strings.NewReader(HTML), ContentType) + + og := opengraph.NewOpenGraph() + og.ProcessHTML(r) + } + }) + + b.Run("without converting", func(b *testing.B) { + for i := 0; i < b.N; i++ { + og := opengraph.NewOpenGraph() + og.ProcessHTML(strings.NewReader(HTML)) + } + }) +} + +func TestMakeOpenGraphURLsAbsolute(t *testing.T) { + for name, tc := range map[string]struct { + HTML string + RequestURL string + URL string + ImageURL string + }{ + "absolute URLs": { + HTML: ` + + + + + + `, + RequestURL: "https://example.com", + URL: "https://example.com/apps/mattermost", + ImageURL: "https://images.example.com/image.png", + }, + "URLs starting with /": { + HTML: ` + + + + + + `, + RequestURL: "http://example.com", + URL: "http://example.com/apps/mattermost", + ImageURL: "http://example.com/image.png", + }, + "HTTPS URLs starting with /": { + HTML: ` + + + + + + `, + RequestURL: "https://example.com", + URL: "https://example.com/apps/mattermost", + ImageURL: "https://example.com/image.png", + }, + "missing image URL": { + HTML: ` + + + + + `, + RequestURL: "http://example.com", + URL: "http://example.com/apps/mattermost", + ImageURL: "", + }, + "relative URLs": { + HTML: ` + + + + + + `, + RequestURL: "http://example.com/content/index.html", + URL: "http://example.com/content/index.html", + ImageURL: "http://example.com/resources/image.png", + }, + } { + t.Run(name, func(t *testing.T) { + og := opengraph.NewOpenGraph() + if err := og.ProcessHTML(strings.NewReader(tc.HTML)); err != nil { + t.Fatal(err) + } + + makeOpenGraphURLsAbsolute(og, tc.RequestURL) + + if og.URL != tc.URL { + t.Fatalf("incorrect url, expected %v, got %v", tc.URL, og.URL) + } + + if len(og.Images) > 0 { + if og.Images[0].URL != tc.ImageURL { + t.Fatalf("incorrect image url, expected %v, got %v", tc.ImageURL, og.Images[0].URL) + } + } else if tc.ImageURL != "" { + t.Fatalf("missing image url, expected %v, got nothing", tc.ImageURL) + } + }) + } +} diff --git a/app/post.go b/app/post.go index 368053c3bf..bd9153d680 100644 --- a/app/post.go +++ b/app/post.go @@ -9,15 +9,11 @@ import ( "encoding/hex" "encoding/json" "fmt" - "io" "net/http" "net/url" "path" "strings" - "github.com/dyatlov/go-opengraph/opengraph" - "golang.org/x/net/html/charset" - "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" @@ -798,85 +794,6 @@ func (a *App) GetFileInfosForPost(postId string, readFromMaster bool) ([]*model. return infos, nil } -func (a *App) GetOpenGraphMetadata(requestURL string) *opengraph.OpenGraph { - og := opengraph.NewOpenGraph() - - res, err := a.HTTPService.MakeClient(false).Get(requestURL) - if err != nil { - mlog.Error(fmt.Sprintf("GetOpenGraphMetadata request failed for url=%v with err=%v", requestURL, err.Error())) - return og - } - defer consumeAndClose(res) - - contentType := res.Header.Get("Content-Type") - body := forceHTMLEncodingToUTF8(res.Body, contentType) - - if err := og.ProcessHTML(body); err != nil { - mlog.Error(fmt.Sprintf("GetOpenGraphMetadata processing failed for url=%v with err=%v", requestURL, err.Error())) - } - - makeOpenGraphURLsAbsolute(og, requestURL) - - // The URL should be the link the user provided in their message, not a redirected one. - if og.URL != "" { - og.URL = requestURL - } - - return og -} - -func forceHTMLEncodingToUTF8(body io.Reader, contentType string) io.Reader { - r, err := charset.NewReader(body, contentType) - if err != nil { - mlog.Error(fmt.Sprintf("forceHTMLEncodingToUTF8 failed to convert for contentType=%v with err=%v", contentType, err.Error())) - return body - } - return r -} - -func makeOpenGraphURLsAbsolute(og *opengraph.OpenGraph, requestURL string) { - parsedRequestURL, err := url.Parse(requestURL) - if err != nil { - mlog.Warn(fmt.Sprintf("makeOpenGraphURLsAbsolute failed to parse url=%v", requestURL)) - return - } - - makeURLAbsolute := func(resultURL string) string { - if resultURL == "" { - return resultURL - } - - parsedResultURL, err := url.Parse(resultURL) - if err != nil { - mlog.Warn(fmt.Sprintf("makeOpenGraphURLsAbsolute failed to parse result url=%v", resultURL)) - return resultURL - } - - if parsedResultURL.IsAbs() { - return resultURL - } - - return parsedRequestURL.ResolveReference(parsedResultURL).String() - } - - og.URL = makeURLAbsolute(og.URL) - - for _, image := range og.Images { - image.URL = makeURLAbsolute(image.URL) - image.SecureURL = makeURLAbsolute(image.SecureURL) - } - - for _, audio := range og.Audios { - audio.URL = makeURLAbsolute(audio.URL) - audio.SecureURL = makeURLAbsolute(audio.SecureURL) - } - - for _, video := range og.Videos { - video.URL = makeURLAbsolute(video.URL) - video.SecureURL = makeURLAbsolute(video.SecureURL) - } -} - func (a *App) DoPostAction(postId, actionId, userId, selectedOption string) *model.AppError { pchan := a.Srv.Store.Post().GetSingle(postId) cchan := a.Srv.Store.Channel().GetForPost(postId) diff --git a/app/post_metadata.go b/app/post_metadata.go index dd7e66774d..b14c9a1c92 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -4,12 +4,32 @@ package app import ( + "image" + "io" + "net/http" "strings" "github.com/dyatlov/go-opengraph/opengraph" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/utils" + "github.com/mattermost/mattermost-server/utils/markdown" ) +const LINK_CACHE_SIZE = 10000 +const LINK_CACHE_DURATION = 3600 + +var linkCache = utils.NewLru(LINK_CACHE_SIZE) + +func (a *App) InitPostMetadata() { + // Dump any cached links if the proxy settings have changed so image URLs can be updated + a.AddConfigListener(func(before, after *model.Config) { + if (before.ServiceSettings.ImageProxyType != after.ServiceSettings.ImageProxyType) || + (before.ServiceSettings.ImageProxyURL != after.ServiceSettings.ImageProxyType) { + linkCache.Purge() + } + }) +} + func (a *App) PreparePostListForClient(originalList *model.PostList) (*model.PostList, *model.AppError) { list := &model.PostList{ Posts: make(map[string]*model.Post), @@ -31,15 +51,15 @@ func (a *App) PreparePostListForClient(originalList *model.PostList) (*model.Pos func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *model.AppError) { post := originalPost.Clone() - var err *model.AppError - needReactionCounts := post.ReactionCounts == nil needEmojis := post.Emojis == nil - needImageDimensions := post.ImageDimensions == nil needOpenGraphData := post.OpenGraphData == nil + needImageDimensions := post.ImageDimensions == nil + // Get reactions to post var reactions []*model.Reaction if needReactionCounts || needEmojis { + var err *model.AppError reactions, err = a.GetReactionsForPost(post.Id) if err != nil { return post, err @@ -50,15 +70,7 @@ func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *mode post.ReactionCounts = model.CountReactions(reactions) } - if post.FileInfos == nil { - fileInfos, err := a.GetFileInfosForPost(post.Id, false) - if err != nil { - return post, err - } - - post.FileInfos = fileInfos - } - + // Get emojis for post if needEmojis { emojis, err := a.getCustomEmojisForPost(post.Message, reactions) if err != nil { @@ -68,23 +80,79 @@ func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *mode post.Emojis = emojis } + // Get files for post + if post.FileInfos == nil { + fileInfos, err := a.GetFileInfosForPost(post.Id, false) + if err != nil { + return post, err + } + + post.FileInfos = fileInfos + } + + // Proxy image links in post post = a.PostWithProxyAddedToImageURLs(post) - if needImageDimensions || needOpenGraphData { - if needImageDimensions { - post.ImageDimensions = []*model.PostImageDimensions{} + // Get OpenGraph and image metadata + if needOpenGraphData || needImageDimensions { + err := a.preparePostWithOpenGraphAndImageMetadata(post, needOpenGraphData, needImageDimensions) + if err != nil { + return post, err } - - if needOpenGraphData { - post.OpenGraphData = []*opengraph.OpenGraph{} - } - - // TODO } return post, nil } +func (a *App) preparePostWithOpenGraphAndImageMetadata(post *model.Post, needOpenGraphData, needImageDimensions bool) *model.AppError { + var appError *model.AppError + + if needOpenGraphData { + post.OpenGraphData = []*opengraph.OpenGraph{} + } + + if needImageDimensions { + post.ImageDimensions = []*model.PostImageDimensions{} + } + + firstLink, images := getFirstLinkAndImages(post.Message) + + // Look at the first link to see if it's a web page or an image + if firstLink != "" { + og, dimensions, err := a.getLinkMetadata(firstLink, true) + if err != nil { + // Keep going so that one bad link doesn't prevent other image dimensions from being sent to the client + appError = model.NewAppError("PreparePostForClient", "app.post.metadata.link.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + if needOpenGraphData { + post.OpenGraphData = append(post.OpenGraphData, og) + } + + if needImageDimensions { + post.ImageDimensions = append(post.ImageDimensions, dimensions) + } + } + + if needImageDimensions { + // And dimensions for other images + for _, image := range images { + _, dimensions, err := a.getLinkMetadata(image, true) + if err != nil { + // Keep going so that one bad link doesn't prevent other image dimensions from being sent to the client + appError = model.NewAppError("PreparePostForClient", "app.post.metadata.link.app_error", nil, err.Error(), http.StatusInternalServerError) + continue + } + + if dimensions != nil { + post.ImageDimensions = append(post.ImageDimensions, dimensions) + } + } + } + + return appError +} + func (a *App) getCustomEmojisForPost(message string, reactions []*model.Reaction) ([]*model.Emoji, *model.AppError) { if !*a.Config().ServiceSettings.EnableCustomEmoji { // Only custom emoji are returned @@ -109,3 +177,129 @@ func (a *App) getCustomEmojisForPost(message string, reactions []*model.Reaction return a.GetMultipleEmojiByName(names) } + +// Given a string, returns the first autolinked URL in the string as well as an array of all Markdown +// images of the form ![alt text](image url). Note that this does not return Markdown links of the +// form [text](url). +func getFirstLinkAndImages(str string) (string, []string) { + firstLink := "" + images := []string{} + + markdown.Inspect(str, func(blockOrInline interface{}) bool { + switch v := blockOrInline.(type) { + case *markdown.Autolink: + if firstLink == "" { + firstLink = v.Destination() + } + case *markdown.InlineImage: + images = append(images, v.Destination()) + case *markdown.ReferenceImage: + images = append(images, v.ReferenceDefinition.Destination()) + } + + return true + }) + + if len(images) > 1 { + images = model.RemoveDuplicateStrings(images) + } + + return firstLink, images +} + +func (a *App) getLinkMetadata(requestURL string, useCache bool) (*opengraph.OpenGraph, *model.PostImageDimensions, error) { + // Check cache + if useCache { + og, dimensions, ok := getLinkMetadataFromCache(requestURL) + + if ok { + return og, dimensions, nil + } + } + + // Make request for a web page or an image + request, err := http.NewRequest("GET", requestURL, nil) + if err != nil { + return nil, nil, err + } + + request.Header.Add("Accept", "text/html, image/*") + + res, err := a.HTTPClient(false).Do(request) // TODO figure out a way to mock out the client for testing + if err != nil { + return nil, nil, err + } + defer consumeAndClose(res) + + // Parse the data + og, dimensions, err := a.parseLinkMetadata(requestURL, res.Body, res.Header.Get("Content-Type")) + + // Write back to cache + if useCache { + cacheLinkMetadata(requestURL, og, dimensions) + } + + return og, dimensions, err +} + +func getLinkMetadataFromCache(requestURL string) (*opengraph.OpenGraph, *model.PostImageDimensions, bool) { + cached, ok := linkCache.Get(requestURL) + if !ok { + return nil, nil, false + } + + switch v := cached.(type) { + case *opengraph.OpenGraph: + return v, nil, true + case *model.PostImageDimensions: + return nil, v, true + default: + return nil, nil, true + } +} + +func cacheLinkMetadata(requestURL string, og *opengraph.OpenGraph, dimensions *model.PostImageDimensions) { + var val interface{} + if og != nil { + val = og + } else if dimensions != nil { + val = dimensions + } + + linkCache.AddWithExpiresInSecs(requestURL, val, LINK_CACHE_DURATION) +} + +func (a *App) parseLinkMetadata(requestURL string, body io.Reader, contentType string) (*opengraph.OpenGraph, *model.PostImageDimensions, error) { + if strings.HasPrefix(contentType, "image") { + dimensions, err := parseImageDimensions(requestURL, body) + return nil, dimensions, err + } else if strings.HasPrefix(contentType, "text/html") { + og := a.ParseOpenGraphMetadata(requestURL, body, contentType) + + // The OpenGraph library and Go HTML library don't error for malformed input, so check that at least + // one of these required fields exists before returning the OpenGraph data + if og.Title != "" || og.Type != "" || og.URL != "" { + return og, nil, nil + } else { + return nil, nil, nil + } + } else { + // Not an image or web page with OpenGraph information + return nil, nil, nil + } +} + +func parseImageDimensions(requestURL string, body io.Reader) (*model.PostImageDimensions, error) { + config, _, err := image.DecodeConfig(body) + if err != nil { + return nil, err + } + + dimensions := &model.PostImageDimensions{ + URL: requestURL, + Width: config.Width, + Height: config.Height, + } + + return dimensions, nil +} diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index c1102a2424..232ca366e5 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -4,11 +4,16 @@ package app import ( + "bytes" "fmt" + "io" + "strings" "testing" "time" + "github.com/dyatlov/go-opengraph/opengraph" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/utils/testutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -167,8 +172,55 @@ func TestPreparePostForClient(t *testing.T) { assert.ElementsMatch(t, []*model.Emoji{emoji1, emoji2, emoji3}, clientPost.Emojis, "should've populated post.Emojis") }) + t.Run("markdown image dimensions", func(t *testing.T) { + th := setup() + defer th.TearDown() + + post, err := th.App.CreatePost(&model.Post{ + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: "This is ![our logo](https://github.com/hmhealey/test-files/raw/master/logoVertical.png) and ![our icon](https://github.com/hmhealey/test-files/raw/master/icon.png)", + }, th.BasicChannel, false) + require.Nil(t, err) + + clientPost, err := th.App.PreparePostForClient(post) + require.Nil(t, err) + + assert.Len(t, clientPost.ImageDimensions, 2) + assert.Equal(t, &model.PostImageDimensions{ + URL: "https://github.com/hmhealey/test-files/raw/master/logoVertical.png", + Width: 1068, + Height: 552, + }, clientPost.ImageDimensions[0]) + assert.Equal(t, &model.PostImageDimensions{ + URL: "https://github.com/hmhealey/test-files/raw/master/icon.png", + Width: 501, + Height: 501, + }, clientPost.ImageDimensions[1]) + }) + t.Run("linked image dimensions", func(t *testing.T) { - // TODO + th := setup() + defer th.TearDown() + + post, err := th.App.CreatePost(&model.Post{ + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: `This is our logo: https://github.com/hmhealey/test-files/raw/master/logoVertical.png + And this is our icon: https://github.com/hmhealey/test-files/raw/master/icon.png`, + }, th.BasicChannel, false) + require.Nil(t, err) + + clientPost, err := th.App.PreparePostForClient(post) + require.Nil(t, err) + + // Reminder that only the first link gets dimensions + assert.Len(t, clientPost.ImageDimensions, 1) + assert.Equal(t, &model.PostImageDimensions{ + URL: "https://github.com/hmhealey/test-files/raw/master/logoVertical.png", + Width: 1068, + Height: 552, + }, clientPost.ImageDimensions[0]) }) t.Run("proxy linked images", func(t *testing.T) { @@ -179,7 +231,32 @@ func TestPreparePostForClient(t *testing.T) { }) t.Run("opengraph", func(t *testing.T) { - // TODO + th := setup() + defer th.TearDown() + + post, err := th.App.CreatePost(&model.Post{ + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: `This is our web page: https://github.com/hmhealey/test-files`, + }, th.BasicChannel, false) + require.Nil(t, err) + + clientPost, err := th.App.PreparePostForClient(post) + require.Nil(t, err) + + assert.Len(t, clientPost.OpenGraphData, 1) + assert.Equal(t, &opengraph.OpenGraph{ + Description: "Contribute to hmhealey/test-files development by creating an account on GitHub.", + SiteName: "GitHub", + Title: "hmhealey/test-files", + Type: "object", + URL: "https://github.com/hmhealey/test-files", + Images: []*opengraph.Image{ + { + URL: "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4", + }, + }, + }, clientPost.OpenGraphData[0]) }) t.Run("opengraph image dimensions", func(t *testing.T) { @@ -187,7 +264,10 @@ func TestPreparePostForClient(t *testing.T) { }) t.Run("proxy opengraph images", func(t *testing.T) { - // TODO + th := setup() + defer th.TearDown() + + testProxyOpenGraphImage(t, th, false) }) } @@ -213,7 +293,10 @@ func TestPreparePostForClientWithImageProxy(t *testing.T) { }) t.Run("proxy opengraph images", func(t *testing.T) { - // TODO + th := setup() + defer th.TearDown() + + testProxyOpenGraphImage(t, th, true) }) } @@ -233,7 +316,9 @@ func testProxyLinkedImage(t *testing.T, th *TestHelper, shouldProxy bool) { require.Nil(t, err) clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + if err != nil && err.Id != "app.post.metadata.link.app_error" { + t.Fatal(err) + } if shouldProxy { assert.Equal(t, post.Message, fmt.Sprintf(postTemplate, imageURL), "should not have mutated original post") @@ -243,6 +328,35 @@ func testProxyLinkedImage(t *testing.T, th *TestHelper, shouldProxy bool) { } } +func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) { + post, err := th.App.CreatePost(&model.Post{ + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: `This is our web page: https://github.com/hmhealey/test-files`, + }, th.BasicChannel, false) + require.Nil(t, err) + + clientPost, err := th.App.PreparePostForClient(post) + require.Nil(t, err) + + image := &opengraph.Image{} + if shouldProxy { + image.SecureURL = "https://127.0.0.1/b2ef6ef4890a0107aa80ba33b3011fd51f668303/68747470733a2f2f61766174617273312e67697468756275736572636f6e74656e742e636f6d2f752f333237373331303f733d34303026763d34" + } else { + image.URL = "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4" + } + + assert.Len(t, clientPost.OpenGraphData, 1) + assert.Equal(t, &opengraph.OpenGraph{ + Description: "Contribute to hmhealey/test-files development by creating an account on GitHub.", + SiteName: "GitHub", + Title: "hmhealey/test-files", + Type: "object", + URL: "https://github.com/hmhealey/test-files", + Images: []*opengraph.Image{image}, + }, clientPost.OpenGraphData[0]) +} + func TestGetCustomEmojisForPost_Message(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -354,3 +468,187 @@ func TestGetCustomEmojisForPost(t *testing.T) { assert.Nil(t, err, "failed to get emojis for post") assert.ElementsMatch(t, emojis, []*model.Emoji{emoji1, emoji2}, "received incorrect emojis") } + +func TestGetFirstLinkAndImages(t *testing.T) { + for name, testCase := range map[string]struct { + Input string + ExpectedFirstLink string + ExpectedImages []string + }{ + "no links or images": { + Input: "this is a string", + ExpectedFirstLink: "", + ExpectedImages: []string{}, + }, + "http link": { + Input: "this is a http://example.com", + ExpectedFirstLink: "http://example.com", + ExpectedImages: []string{}, + }, + "www link": { + Input: "this is a www.example.com", + ExpectedFirstLink: "http://www.example.com", + ExpectedImages: []string{}, + }, + "image": { + Input: "this is a ![our logo](http://example.com/logo)", + ExpectedFirstLink: "", + ExpectedImages: []string{"http://example.com/logo"}, + }, + "multiple images": { + Input: "this is a ![our logo](http://example.com/logo) and ![their logo](http://example.com/logo2) and ![my logo](http://example.com/logo3)", + ExpectedFirstLink: "", + ExpectedImages: []string{"http://example.com/logo", "http://example.com/logo2", "http://example.com/logo3"}, + }, + "multiple images with duplicate": { + Input: "this is a ![our logo](http://example.com/logo) and ![their logo](http://example.com/logo2) and ![my logo which is their logo](http://example.com/logo2)", + ExpectedFirstLink: "", + ExpectedImages: []string{"http://example.com/logo", "http://example.com/logo2"}, + }, + "reference image": { + Input: `this is a ![our logo][logo] + +[logo]: http://example.com/logo`, + ExpectedFirstLink: "", + ExpectedImages: []string{"http://example.com/logo"}, + }, + "image and link": { + Input: "this is a https://example.com and ![our logo](https://example.com/logo)", + ExpectedFirstLink: "https://example.com", + ExpectedImages: []string{"https://example.com/logo"}, + }, + "markdown links (not returned)": { + Input: `this is a [our page](http://example.com) and [another page][] + +[another page]: http://www.exaple.com/another_page`, + ExpectedFirstLink: "", + ExpectedImages: []string{}, + }, + } { + t.Run(name, func(t *testing.T) { + firstLink, images := getFirstLinkAndImages(testCase.Input) + + assert.Equal(t, firstLink, testCase.ExpectedFirstLink) + assert.Equal(t, images, testCase.ExpectedImages) + }) + } +} + +func TestParseLinkMetadata(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + imageURL := "http://example.com/test.png" + file, err := testutils.ReadTestFile("test.png") + require.Nil(t, err) + + ogURL := "https://example.com/hello" + html := ` + + + + + + + ` + + makeImageReader := func() io.Reader { + return bytes.NewReader(file) + } + + makeOpenGraphReader := func() io.Reader { + return strings.NewReader(html) + } + + t.Run("image", func(t *testing.T) { + og, dimensions, err := th.App.parseLinkMetadata(imageURL, makeImageReader(), "image/png") + assert.Nil(t, err) + + assert.Nil(t, og) + assert.Equal(t, &model.PostImageDimensions{ + URL: imageURL, + Width: 408, + Height: 336, + }, dimensions) + }) + + t.Run("malformed image", func(t *testing.T) { + og, dimensions, err := th.App.parseLinkMetadata(imageURL, makeOpenGraphReader(), "image/png") + assert.NotNil(t, err) + + assert.Nil(t, og) + assert.Nil(t, dimensions) + }) + + t.Run("opengraph", func(t *testing.T) { + og, dimensions, err := th.App.parseLinkMetadata(ogURL, makeOpenGraphReader(), "text/html; charset=utf-8") + assert.Nil(t, err) + + assert.NotNil(t, og) + assert.Equal(t, og.Title, "Hello, World!") + assert.Equal(t, og.Type, "object") + assert.Equal(t, og.URL, ogURL) + assert.Nil(t, dimensions) + }) + + t.Run("malformed opengraph", func(t *testing.T) { + og, dimensions, err := th.App.parseLinkMetadata(ogURL, makeImageReader(), "text/html; charset=utf-8") + assert.Nil(t, err) + + assert.Nil(t, og) + assert.Nil(t, dimensions) + }) + + t.Run("neither", func(t *testing.T) { + og, dimensions, err := th.App.parseLinkMetadata("http://example.com/test.wad", strings.NewReader("garbage"), "application/x-doom") + assert.Nil(t, err) + + assert.Nil(t, og) + assert.Nil(t, dimensions) + }) +} + +func TestParseImageDimensions(t *testing.T) { + for name, testCase := range map[string]struct { + FileName string + URL string + ExpectedWidth int + ExpectedHeight int + ExpectError bool + }{ + "png": { + FileName: "test.png", + URL: "https://example.com/test.png", + ExpectedWidth: 408, + ExpectedHeight: 336, + }, + "animated gif": { + FileName: "testgif.gif", + URL: "http://example.com/test.gif?foo=bar", + ExpectedWidth: 118, + ExpectedHeight: 118, + }, + "not an image": { + FileName: "README.md", + URL: "https://example.com/test.png", + ExpectError: true, + }, + } { + t.Run(name, func(t *testing.T) { + file, err := testutils.ReadTestFile(testCase.FileName) + require.Nil(t, err) + + dimensions, err := parseImageDimensions(testCase.URL, bytes.NewReader(file)) + if testCase.ExpectError { + require.NotNil(t, err) + } else { + require.Nil(t, err) + + require.NotNil(t, dimensions) + require.Equal(t, testCase.URL, dimensions.URL) + require.Equal(t, testCase.ExpectedWidth, dimensions.Width) + require.Equal(t, testCase.ExpectedHeight, dimensions.Height) + } + }) + } +} diff --git a/app/post_test.go b/app/post_test.go index 2820d6e027..0fbe550ae8 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -7,12 +7,10 @@ import ( "fmt" "net/http" "net/http/httptest" - "strings" "sync/atomic" "testing" "time" - "github.com/dyatlov/go-opengraph/opengraph" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -483,124 +481,6 @@ func TestImageProxy(t *testing.T) { } } -func BenchmarkForceHTMLEncodingToUTF8(b *testing.B) { - HTML := ` - - - - - - - ` - ContentType := "text/html; utf-8" - - b.Run("with converting", func(b *testing.B) { - for i := 0; i < b.N; i++ { - r := forceHTMLEncodingToUTF8(strings.NewReader(HTML), ContentType) - - og := opengraph.NewOpenGraph() - og.ProcessHTML(r) - } - }) - - b.Run("without converting", func(b *testing.B) { - for i := 0; i < b.N; i++ { - og := opengraph.NewOpenGraph() - og.ProcessHTML(strings.NewReader(HTML)) - } - }) -} - -func TestMakeOpenGraphURLsAbsolute(t *testing.T) { - for name, tc := range map[string]struct { - HTML string - RequestURL string - URL string - ImageURL string - }{ - "absolute URLs": { - HTML: ` - - - - - - `, - RequestURL: "https://example.com", - URL: "https://example.com/apps/mattermost", - ImageURL: "https://images.example.com/image.png", - }, - "URLs starting with /": { - HTML: ` - - - - - - `, - RequestURL: "http://example.com", - URL: "http://example.com/apps/mattermost", - ImageURL: "http://example.com/image.png", - }, - "HTTPS URLs starting with /": { - HTML: ` - - - - - - `, - RequestURL: "https://example.com", - URL: "https://example.com/apps/mattermost", - ImageURL: "https://example.com/image.png", - }, - "missing image URL": { - HTML: ` - - - - - `, - RequestURL: "http://example.com", - URL: "http://example.com/apps/mattermost", - ImageURL: "", - }, - "relative URLs": { - HTML: ` - - - - - - `, - RequestURL: "http://example.com/content/index.html", - URL: "http://example.com/content/index.html", - ImageURL: "http://example.com/resources/image.png", - }, - } { - t.Run(name, func(t *testing.T) { - og := opengraph.NewOpenGraph() - if err := og.ProcessHTML(strings.NewReader(tc.HTML)); err != nil { - t.Fatal(err) - } - - makeOpenGraphURLsAbsolute(og, tc.RequestURL) - - if og.URL != tc.URL { - t.Fatalf("incorrect url, expected %v, got %v", tc.URL, og.URL) - } - - if len(og.Images) > 0 { - if og.Images[0].URL != tc.ImageURL { - t.Fatalf("incorrect image url, expected %v, got %v", tc.ImageURL, og.Images[0].URL) - } - } else if tc.ImageURL != "" { - t.Fatalf("missing image url, expected %v, got nothing", tc.ImageURL) - } - }) - } -} - func TestMaxPostSize(t *testing.T) { t.Parallel() diff --git a/i18n/en.json b/i18n/en.json index 70b36a1738..d13d0c052f 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3146,6 +3146,10 @@ "id": "app.plugin.upload_disabled.app_error", "translation": "Plugins and/or plugin uploads have been disabled." }, + { + "id": "app.post.metadata.link.app_error", + "translation": "Failed to get metadata for a link in a post." + }, { "id": "app.role.check_roles_exist.role_not_found", "translation": "The provided role does not exist" diff --git a/model/post.go b/model/post.go index 3bd74a1000..a64391d4da 100644 --- a/model/post.go +++ b/model/post.go @@ -93,8 +93,8 @@ type Post struct { type PostImageDimensions struct { URL string `json:"url"` - Width int64 `json:"width"` - Height int64 `json:"height"` + Width int `json:"width"` + Height int `json:"height"` } type PostEphemeral struct { diff --git a/utils/testutils/testutils.go b/utils/testutils/testutils.go new file mode 100644 index 0000000000..777d0aa15b --- /dev/null +++ b/utils/testutils/testutils.go @@ -0,0 +1,29 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package testutils + +import ( + "bytes" + "io" + "os" + "path/filepath" + + "github.com/mattermost/mattermost-server/utils" +) + +func ReadTestFile(name string) ([]byte, error) { + path, _ := utils.FindDir("tests") + file, err := os.Open(filepath.Join(path, name)) + if err != nil { + return nil, err + } + defer file.Close() + + data := &bytes.Buffer{} + if _, err := io.Copy(data, file); err != nil { + return nil, err + } else { + return data.Bytes(), nil + } +}