From 7b0b0d86091504c01c0e014d3b4d11543ee2d734 Mon Sep 17 00:00:00 2001 From: Christopher Poile Date: Thu, 17 Aug 2023 18:23:39 -0400 Subject: [PATCH] MM-53764 - Fix: Improve limits on Opengraph Data Cache (#24177) * enforce strict opengraph cache entry size limit * move json marshalling and error checking into parsOpenGraphMetadata fn * fix linting * fix potential nil deref * Revert "fix potential nil deref" This reverts commit 095bcd496e9b8d97ca63d3a99e2e46eec58ef7ae. * Revert "fix linting" This reverts commit f3e1f7b27634ca0567fd0fef01ac23f5e39009a7. * Revert "move json marshalling and error checking into parsOpenGraphMetadata fn" This reverts commit ba9a1e13b0fb9d4579077b7b37666e950a5fb6ed. * Revert "enforce strict opengraph cache entry size limit" This reverts commit d1de4a8fa40e58ac85c85f66de27ec5aca884b18. * remove /opengraph api endpoint * i18n * removing unneeded action and reducer --- api/Makefile | 1 - api/v4/source/opengraph.yaml | 38 --------- server/channels/api4/api.go | 5 -- server/channels/api4/openGraph.go | 41 ---------- server/channels/api4/openGraph_test.go | 77 ------------------- server/i18n/en.json | 4 - server/public/model/client4.go | 19 ----- .../src/action_types/posts.ts | 1 - .../src/actions/posts.test.ts | 30 -------- .../mattermost-redux/src/actions/posts.ts | 23 ------ .../src/reducers/entities/posts.ts | 7 -- webapp/platform/client/src/client4.ts | 7 -- 12 files changed, 253 deletions(-) delete mode 100644 api/v4/source/opengraph.yaml delete mode 100644 server/channels/api4/openGraph.go delete mode 100644 server/channels/api4/openGraph_test.go diff --git a/api/Makefile b/api/Makefile index 0c78f2973d..387da3e31a 100644 --- a/api/Makefile +++ b/api/Makefile @@ -41,7 +41,6 @@ build-v4: node_modules playbooks @cat $(V4_SRC)/schemes.yaml >> $(V4_YAML) @cat $(V4_SRC)/service_terms.yaml >> $(V4_YAML) @cat $(V4_SRC)/sharedchannels.yaml >> $(V4_YAML) - @cat $(V4_SRC)/opengraph.yaml >> $(V4_YAML) @cat $(V4_SRC)/reactions.yaml >> $(V4_YAML) @cat $(V4_SRC)/actions.yaml >> $(V4_YAML) @cat $(V4_SRC)/bots.yaml >> $(V4_YAML) diff --git a/api/v4/source/opengraph.yaml b/api/v4/source/opengraph.yaml deleted file mode 100644 index 3cde79d4be..0000000000 --- a/api/v4/source/opengraph.yaml +++ /dev/null @@ -1,38 +0,0 @@ - /api/v4/opengraph: - post: - tags: - - OpenGraph - summary: Get open graph metadata for url - description: > - Get Open Graph Metadata for a specif URL. Use the Open Graph protocol to - get some generic metadata about a URL. Used for creating link previews. - - - __Minimum server version__: 3.10 - - - ##### Permissions - - No permission required but must be logged in. - operationId: OpenGraph - requestBody: - content: - application/json: - schema: - type: object - required: - - url - properties: - url: - type: string - description: The URL to get Open Graph Metadata. - required: true - responses: - "200": - description: Open Graph retrieval successful - content: - application/json: - schema: - $ref: "#/components/schemas/OpenGraph" - "501": - $ref: "#/components/responses/NotImplemented" diff --git a/server/channels/api4/api.go b/server/channels/api4/api.go index 868dd8d2c2..d20fdd50fb 100644 --- a/server/channels/api4/api.go +++ b/server/channels/api4/api.go @@ -81,8 +81,6 @@ type Routes struct { OAuthApps *mux.Router // 'api/v4/oauth/apps' OAuthApp *mux.Router // 'api/v4/oauth/apps/{app_id:[A-Za-z0-9]+}' - OpenGraph *mux.Router // 'api/v4/opengraph' - SAML *mux.Router // 'api/v4/saml' Compliance *mux.Router // 'api/v4/compliance' Cluster *mux.Router // 'api/v4/cluster' @@ -240,8 +238,6 @@ func Init(srv *app.Server) (*API, error) { api.BaseRoutes.ReactionByNameForPostForUser = api.BaseRoutes.PostForUser.PathPrefix("/reactions/{emoji_name:[A-Za-z0-9\\_\\-\\+]+}").Subrouter() - api.BaseRoutes.OpenGraph = api.BaseRoutes.APIRoot.PathPrefix("/opengraph").Subrouter() - api.BaseRoutes.Roles = api.BaseRoutes.APIRoot.PathPrefix("/roles").Subrouter() api.BaseRoutes.Schemes = api.BaseRoutes.APIRoot.PathPrefix("/schemes").Subrouter() @@ -294,7 +290,6 @@ func Init(srv *app.Server) (*API, error) { api.InitEmoji() api.InitOAuth() api.InitReaction() - api.InitOpenGraph() api.InitPlugin() api.InitRole() api.InitScheme() diff --git a/server/channels/api4/openGraph.go b/server/channels/api4/openGraph.go deleted file mode 100644 index a66d62016f..0000000000 --- a/server/channels/api4/openGraph.go +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package api4 - -import ( - "net/http" - - "github.com/mattermost/mattermost/server/public/model" - "github.com/mattermost/mattermost/server/public/shared/mlog" -) - -func (api *API) InitOpenGraph() { - api.BaseRoutes.OpenGraph.Handle("", api.APISessionRequired(getOpenGraphMetadata)).Methods("POST") -} - -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) - return - } - - props := model.StringInterfaceFromJSON(r.Body) - - url := "" - ok := false - if url, ok = props["url"].(string); url == "" || !ok { - c.SetInvalidParam("url") - return - } - - buf, err := c.App.GetOpenGraphMetadata(url) - if err != nil { - mlog.Warn("GetOpenGraphMetadata request failed", - mlog.String("requestURL", url), - mlog.Err(err)) - w.Write([]byte(`{"url": ""}`)) - return - } - w.Write(buf) -} diff --git a/server/channels/api4/openGraph_test.go b/server/channels/api4/openGraph_test.go deleted file mode 100644 index 52e65c0dbc..0000000000 --- a/server/channels/api4/openGraph_test.go +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package api4 - -import ( - "context" - "fmt" - "net/http" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/require" - - "github.com/mattermost/mattermost/server/public/model" -) - -func TestGetOpenGraphMetadata(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() - - client := th.Client - - enableLinkPreviews := *th.App.Config().ServiceSettings.EnableLinkPreviews - allowedInternalConnections := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections - defer func() { - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableLinkPreviews = enableLinkPreviews }) - th.App.UpdateConfig(func(cfg *model.Config) { - cfg.ServiceSettings.AllowedUntrustedInternalConnections = &allowedInternalConnections - }) - }() - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableLinkPreviews = true }) - th.App.UpdateConfig(func(cfg *model.Config) { - *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost,127.0.0.1" - }) - - ogDataCacheMissCount := 0 - - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ogDataCacheMissCount++ - - if r.URL.Path == "/og-data/" { - fmt.Fprintln(w, ` - - - - - `) - } else if r.URL.Path == "/no-og-data/" { - fmt.Fprintln(w, ``) - } - })) - - for _, data := range [](map[string]any){ - {"path": "/og-data/", "title": "Test Title", "cacheMissCount": 1}, - {"path": "/no-og-data/", "title": "", "cacheMissCount": 2}, - - // Data should be cached for following - {"path": "/og-data/", "title": "Test Title", "cacheMissCount": 2}, - {"path": "/no-og-data/", "title": "", "cacheMissCount": 2}, - } { - - openGraph, _, err := client.OpenGraph(context.Background(), ts.URL+data["path"].(string)) - require.NoError(t, err) - - require.Equalf(t, openGraph["title"], data["title"].(string), - "OG data title mismatch for path \"%s\".") - - require.Equal(t, ogDataCacheMissCount, data["cacheMissCount"].(int), - "Cache miss count didn't match.") - } - - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableLinkPreviews = false }) - _, resp, err := client.OpenGraph(context.Background(), ts.URL+"/og-data/") - require.Error(t, err) - CheckNotImplementedStatus(t, resp) -} diff --git a/server/i18n/en.json b/server/i18n/en.json index 8881c43a5e..2c398abc4c 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -2391,10 +2391,6 @@ "other": "{{.Count}} images sent: {{.Filenames}}" } }, - { - "id": "api.post.link_preview_disabled.app_error", - "translation": "Link previews have been disabled by the system administrator." - }, { "id": "api.post.patch_post.can_not_update_post_in_deleted.error", "translation": "Can not update a post in a deleted channel." diff --git a/server/public/model/client4.go b/server/public/model/client4.go index f8a251bfa4..dfe1dd8bf6 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -467,10 +467,6 @@ func (c *Client4) oAuthAppRoute(appId string) string { return fmt.Sprintf("/oauth/apps/%v", appId) } -func (c *Client4) openGraphRoute() string { - return "/opengraph" -} - func (c *Client4) jobsRoute() string { return "/jobs" } @@ -6779,21 +6775,6 @@ func (c *Client4) GetSupportedTimezone(ctx context.Context) ([]string, *Response return timezones, BuildResponse(r), nil } -// Open Graph Metadata Section - -// OpenGraph return the open graph metadata for a particular url if the site have the metadata. -func (c *Client4) OpenGraph(ctx context.Context, url string) (map[string]string, *Response, error) { - requestBody := make(map[string]string) - requestBody["url"] = url - - r, err := c.DoAPIPost(ctx, c.openGraphRoute(), MapToJSON(requestBody)) - if err != nil { - return nil, BuildResponse(r), err - } - defer closeBody(r) - return MapFromJSON(r.Body), BuildResponse(r), nil -} - // Jobs Section // GetJob gets a single job. diff --git a/webapp/channels/src/packages/mattermost-redux/src/action_types/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/action_types/posts.ts index b8ace1976e..4dd642c173 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/action_types/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/action_types/posts.ts @@ -47,7 +47,6 @@ export default keyMirror({ RECEIVED_REACTION: null, RECEIVED_REACTIONS: null, REACTION_DELETED: null, - RECEIVED_OPEN_GRAPH_METADATA: null, ADD_MESSAGE_INTO_HISTORY: null, RESET_HISTORY_INDEX: null, diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts index 4589bbbaa3..b3ebf5037f 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts @@ -1335,36 +1335,6 @@ describe('Actions.Posts', () => { expect(state.entities.emojis.nonExistentEmoji.has(missingEmojiName)).toBeTruthy(); }); - it('getOpenGraphMetadata', async () => { - const {dispatch, getState} = store; - - const url = 'https://mattermost.com'; - const docs = 'https://docs.mattermost.com/'; - - nock(Client4.getBaseRoute()). - post('/opengraph'). - reply(200, {type: 'article', url: 'https://mattermost.com/', title: 'Mattermost private cloud messaging', description: 'Open source, private cloud\nSlack-alternative, \nWorkplace messaging for web, PCs and phones.'}); - await dispatch(Actions.getOpenGraphMetadata(url)); - - nock(Client4.getBaseRoute()). - post('/opengraph'). - reply(200, {type: '', url: '', title: '', description: ''}); - await dispatch(Actions.getOpenGraphMetadata(docs)); - - nock(Client4.getBaseRoute()). - post('/opengraph'). - reply(200, undefined); - await dispatch(Actions.getOpenGraphMetadata(docs)); - - const state = getState(); - const metadata = state.entities.posts.openGraph; - expect(metadata).toBeTruthy(); - expect(metadata[url]).toBeTruthy(); - if (metadata[docs]) { - throw new Error('unexpected metadata[docs]'); - } - }); - it('doPostAction', async () => { nock(Client4.getBaseRoute()). post('/posts/posth67ja7ntdkek6g13dp3wka/actions/action7ja7ntdkek6g13dp3wka'). diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts index 90867ccc0e..48767e59f0 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts @@ -1269,29 +1269,6 @@ export function addPostReminder(userId: string, postId: string, timestamp: numbe }; } -export function getOpenGraphMetadata(url: string) { - return async (dispatch: DispatchFunc, getState: GetStateFunc) => { - let data; - try { - data = await Client4.getOpenGraphMetadata(url); - } catch (error) { - forceLogoutIfNecessary(error, dispatch, getState); - dispatch(logError(error)); - return {error}; - } - - if (data && (data.url || data.type || data.title || data.description)) { - dispatch({ - type: PostTypes.RECEIVED_OPEN_GRAPH_METADATA, - data, - url, - }); - } - - return {data}; - }; -} - export function doPostAction(postId: string, actionId: string, selectedOption = '') { return doPostActionWithCookie(postId, actionId, '', selectedOption); } diff --git a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts index b4e4df5082..ec5899f1fc 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts @@ -1357,13 +1357,6 @@ function storeAcknowledgementsForPost(state: any, post: Post) { export function openGraph(state: RelationOneToOne> = {}, action: GenericAction) { switch (action.type) { - case PostTypes.RECEIVED_OPEN_GRAPH_METADATA: { - const nextState = {...state}; - nextState[action.url] = action.data; - - return nextState; - } - case PostTypes.RECEIVED_NEW_POST: case PostTypes.RECEIVED_POST: { const post = action.data; diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index d42cb0deb1..222e2c1224 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -2214,13 +2214,6 @@ export default class Client4 { return this.searchFilesWithParams(teamId, {terms, is_or_search: isOrSearch}); }; - getOpenGraphMetadata = (url: string) => { - return this.doFetch( - `${this.getBaseRoute()}/opengraph`, - {method: 'post', body: JSON.stringify({url})}, - ); - }; - doPostAction = (postId: string, actionId: string, selectedOption = '') => { return this.doPostActionWithCookie(postId, actionId, '', selectedOption); };