From dce6cb601f15f27cb35fb37b6863ee4626df6d01 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 6 May 2019 09:22:37 -0400 Subject: [PATCH] MM-14686 Send all image proxy requests through /api/v4/image (#10775) * MM-14686 Implement /api/v4/image when proxy is disabled * MM-14686 Send all image proxy requests through /api/v4/image * Update unit tests --- api4/image.go | 11 ++-- api4/image_test.go | 3 +- app/post_metadata_test.go | 4 +- app/post_test.go | 11 ++-- services/imageproxy/atmos_camo.go | 28 +-------- services/imageproxy/atmos_camo_test.go | 54 ---------------- services/imageproxy/imageproxy.go | 37 ++++++----- services/imageproxy/imageproxy_test.go | 86 ++++++++++++++++++++++++++ services/imageproxy/local.go | 31 ---------- services/imageproxy/local_test.go | 76 ----------------------- 10 files changed, 128 insertions(+), 213 deletions(-) create mode 100644 services/imageproxy/imageproxy_test.go diff --git a/api4/image.go b/api4/image.go index 4a51c3cedb..fb82a77861 100644 --- a/api4/image.go +++ b/api4/image.go @@ -12,10 +12,11 @@ func (api *API) InitImage() { } func getImage(c *Context, w http.ResponseWriter, r *http.Request) { - if !*c.App.Config().ImageProxySettings.Enable { - http.NotFound(w, r) - return - } + url := r.URL.Query().Get("url") - c.App.ImageProxy.GetImage(w, r, r.URL.Query().Get("url")) + if *c.App.Config().ImageProxySettings.Enable { + c.App.ImageProxy.GetImage(w, r, url) + } else { + http.Redirect(w, r, url, http.StatusFound) + } } diff --git a/api4/image_test.go b/api4/image_test.go index a1eadc67c4..e06fca8d5f 100644 --- a/api4/image_test.go +++ b/api4/image_test.go @@ -38,7 +38,8 @@ func TestGetImage(t *testing.T) { resp, err := th.Client.HttpClient.Do(r) require.NoError(t, err) - assert.Equal(t, http.StatusNotFound, resp.StatusCode) + assert.Equal(t, http.StatusFound, resp.StatusCode) + assert.Equal(t, imageURL, resp.Header.Get("Location")) }) t.Run("atmos/camo", func(t *testing.T) { diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index 9f799056b3..6d64afc9d4 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -456,7 +456,7 @@ func TestPreparePostForClientWithImageProxy(t *testing.T) { func testProxyLinkedImage(t *testing.T, th *TestHelper, shouldProxy bool) { postTemplate := "![foo](%v)" imageURL := "http://mydomain.com/myimage" - proxiedImageURL := "https://127.0.0.1/f8dace906d23689e8d5b12c3cefbedbf7b9b72f5/687474703a2f2f6d79646f6d61696e2e636f6d2f6d79696d616765" + proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage" post := &model.Post{ UserId: th.BasicUser.Id, @@ -498,7 +498,7 @@ func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) { image := og.Images[0] if shouldProxy { assert.Equal(t, "", image.URL, "image URL should not be set with proxy") - assert.Equal(t, "https://127.0.0.1/b2ef6ef4890a0107aa80ba33b3011fd51f668303/68747470733a2f2f61766174617273312e67697468756275736572636f6e74656e742e636f6d2f752f333237373331303f733d34303026763d34", image.SecureURL, "secure image URL should be sent through proxy") + assert.Equal(t, "http://mymattermost.com/api/v4/image?url=https%3A%2F%2Favatars1.githubusercontent.com%2Fu%2F3277310%3Fs%3D400%26v%3D4", image.SecureURL, "secure image URL should be sent through proxy") } else { assert.Equal(t, "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4", image.URL, "image URL should be set") assert.Equal(t, "", image.SecureURL, "secure image URL should not be set") diff --git a/app/post_test.go b/app/post_test.go index cbbd81756f..40a59b07c8 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -475,7 +475,7 @@ func TestImageProxy(t *testing.T) { ProxyURL: "https://127.0.0.1", ProxyOptions: "foo", ImageURL: "http://mydomain.com/myimage", - ProxiedImageURL: "https://127.0.0.1/f8dace906d23689e8d5b12c3cefbedbf7b9b72f5/687474703a2f2f6d79646f6d61696e2e636f6d2f6d79696d616765", + ProxiedImageURL: "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage", }, "atmos/camo_SameSite": { ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, @@ -671,6 +671,7 @@ func TestCreatePost(t *testing.T) { defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = "http://mymattermost.com" *cfg.ExperimentalSettings.DisablePostMetadata = true *cfg.ImageProxySettings.Enable = true *cfg.ImageProxySettings.ImageProxyType = "atmos/camo" @@ -679,7 +680,7 @@ func TestCreatePost(t *testing.T) { }) imageURL := "http://mydomain.com/myimage" - proxiedImageURL := "https://127.0.0.1/f8dace906d23689e8d5b12c3cefbedbf7b9b72f5/687474703a2f2f6d79646f6d61696e2e636f6d2f6d79696d616765" + proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage" post := &model.Post{ ChannelId: th.BasicChannel.Id, @@ -699,6 +700,7 @@ func TestPatchPost(t *testing.T) { defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = "http://mymattermost.com" *cfg.ExperimentalSettings.DisablePostMetadata = true *cfg.ImageProxySettings.Enable = true *cfg.ImageProxySettings.ImageProxyType = "atmos/camo" @@ -707,7 +709,7 @@ func TestPatchPost(t *testing.T) { }) imageURL := "http://mydomain.com/myimage" - proxiedImageURL := "https://127.0.0.1/f8dace906d23689e8d5b12c3cefbedbf7b9b72f5/687474703a2f2f6d79646f6d61696e2e636f6d2f6d79696d616765" + proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage" post := &model.Post{ ChannelId: th.BasicChannel.Id, @@ -748,6 +750,7 @@ func TestUpdatePost(t *testing.T) { defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = "http://mymattermost.com" *cfg.ExperimentalSettings.DisablePostMetadata = true *cfg.ImageProxySettings.Enable = true *cfg.ImageProxySettings.ImageProxyType = "atmos/camo" @@ -756,7 +759,7 @@ func TestUpdatePost(t *testing.T) { }) imageURL := "http://mydomain.com/myimage" - proxiedImageURL := "https://127.0.0.1/f8dace906d23689e8d5b12c3cefbedbf7b9b72f5/687474703a2f2f6d79646f6d61696e2e636f6d2f6d79696d616765" + proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage" post := &model.Post{ ChannelId: th.BasicChannel.Id, diff --git a/services/imageproxy/atmos_camo.go b/services/imageproxy/atmos_camo.go index f66ea033e3..46b358c934 100644 --- a/services/imageproxy/atmos_camo.go +++ b/services/imageproxy/atmos_camo.go @@ -23,11 +23,11 @@ func makeAtmosCamoBackend(proxy *ImageProxy) *AtmosCamoBackend { } func (backend *AtmosCamoBackend) GetImage(w http.ResponseWriter, r *http.Request, imageURL string) { - http.Redirect(w, r, backend.GetProxiedImageURL(imageURL), http.StatusFound) + http.Redirect(w, r, backend.getAtmosCamoImageURL(imageURL), http.StatusFound) } func (backend *AtmosCamoBackend) GetImageDirect(imageURL string) (io.ReadCloser, string, error) { - req, err := http.NewRequest("GET", backend.GetProxiedImageURL(imageURL), nil) + req, err := http.NewRequest("GET", backend.getAtmosCamoImageURL(imageURL), nil) if err != nil { return nil, "", Error{err} } @@ -43,7 +43,7 @@ func (backend *AtmosCamoBackend) GetImageDirect(imageURL string) (io.ReadCloser, return resp.Body, resp.Header.Get("Content-Type"), nil } -func (backend *AtmosCamoBackend) GetProxiedImageURL(imageURL string) string { +func (backend *AtmosCamoBackend) getAtmosCamoImageURL(imageURL string) string { cfg := *backend.proxy.ConfigService.Config() siteURL := *cfg.ServiceSettings.SiteURL proxyURL := *cfg.ImageProxySettings.RemoteImageProxyURL @@ -64,25 +64,3 @@ func getAtmosCamoImageURL(imageURL, siteURL, proxyURL, options string) string { return proxyURL + "/" + digest + "/" + hex.EncodeToString([]byte(imageURL)) } - -func (backend *AtmosCamoBackend) GetUnproxiedImageURL(proxiedURL string) string { - proxyURL := *backend.proxy.ConfigService.Config().ImageProxySettings.RemoteImageProxyURL + "/" - - if !strings.HasPrefix(proxiedURL, proxyURL) { - return proxiedURL - } - - path := proxiedURL[len(proxyURL):] - - slash := strings.IndexByte(path, '/') - if slash == -1 { - return proxiedURL - } - - decoded, err := hex.DecodeString(path[slash+1:]) - if err != nil { - return proxiedURL - } - - return string(decoded) -} diff --git a/services/imageproxy/atmos_camo_test.go b/services/imageproxy/atmos_camo_test.go index 833e70004e..eee2470ccf 100644 --- a/services/imageproxy/atmos_camo_test.go +++ b/services/imageproxy/atmos_camo_test.go @@ -76,22 +76,6 @@ func TestAtmosCamoBackend_GetImageDirect(t *testing.T) { assert.Equal(t, []byte("1111111111"), respBody) } -func TestAtmosCamoBackend_GetProxiedImageURL(t *testing.T) { - imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" - proxiedURL := "http://images.example.com/5b6f6661516bc837b89b54566eb619d14a5c3eca/687474703a2f2f7777772e6d61747465726d6f73742e6f72672f77702d636f6e74656e742f75706c6f6164732f323031362f30332f6c6f676f486f72697a6f6e74616c2e706e67" - - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - }) - - mock := httptest.NewServer(handler) - defer mock.Close() - - proxy := makeTestAtmosCamoProxy() - - // Most of this logic is tested in TestGetAtmosCamoImageURL - assert.Equal(t, proxiedURL, proxy.GetProxiedImageURL(imageURL)) -} - func TestGetAtmosCamoImageURL(t *testing.T) { imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" proxiedURL := "http://images.example.com/5b6f6661516bc837b89b54566eb619d14a5c3eca/687474703a2f2f7777772e6d61747465726d6f73742e6f72672f77702d636f6e74656e742f75706c6f6164732f323031362f30332f6c6f676f486f72697a6f6e74616c2e706e67" @@ -155,41 +139,3 @@ func TestGetAtmosCamoImageURL(t *testing.T) { } } - -func TestAtmosCamoBackend_GetUnproxiedImageURL(t *testing.T) { - imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" - proxiedURL := "http://images.example.com/5b6f6661516bc837b89b54566eb619d14a5c3eca/687474703a2f2f7777772e6d61747465726d6f73742e6f72672f77702d636f6e74656e742f75706c6f6164732f323031362f30332f6c6f676f486f72697a6f6e74616c2e706e67" - - proxy := makeTestAtmosCamoProxy() - - for _, test := range []struct { - Name string - Input string - Expected string - }{ - { - Name: "should remove proxy", - Input: proxiedURL, - Expected: imageURL, - }, - { - Name: "should not remove proxy from a relative image", - Input: "/static/logo.png", - Expected: "/static/logo.png", - }, - { - Name: "should not remove proxy from an image on the Mattermost server", - Input: "https://mattermost.example.com/static/logo.png", - Expected: "https://mattermost.example.com/static/logo.png", - }, - { - Name: "should not remove proxy from a non-proxied image", - Input: imageURL, - Expected: imageURL, - }, - } { - t.Run(test.Name, func(t *testing.T) { - assert.Equal(t, test.Expected, proxy.GetUnproxiedImageURL(test.Input)) - }) - } -} diff --git a/services/imageproxy/imageproxy.go b/services/imageproxy/imageproxy.go index 64562187fc..bee78d4c2f 100644 --- a/services/imageproxy/imageproxy.go +++ b/services/imageproxy/imageproxy.go @@ -7,6 +7,8 @@ import ( "errors" "io" "net/http" + "net/url" + "strings" "sync" "github.com/mattermost/mattermost-server/mlog" @@ -39,13 +41,6 @@ type ImageProxyBackend interface { // GetImageDirect returns a proxied image along with its content type. GetImageDirect(imageURL string) (io.ReadCloser, string, error) - - // GetProxiedImageURL returns the URL to access a given image through the image proxy, whether the image proxy is - // running externally or as part of the Mattermost server itself. - GetProxiedImageURL(imageURL string) string - - // GetUnproxiedImageURL returns the original URL of an image from one that has been directed at the image proxy. - GetUnproxiedImageURL(proxiedURL string) string } func MakeImageProxy(configService configservice.ConfigService, httpService httpservice.HTTPService, logger *mlog.Logger) *ImageProxy { @@ -123,24 +118,36 @@ func (proxy *ImageProxy) GetImageDirect(imageURL string) (io.ReadCloser, string, // GetProxiedImageURL takes the URL of an image and returns a URL that can be used to view that image through the // image proxy. func (proxy *ImageProxy) GetProxiedImageURL(imageURL string) string { - proxy.lock.RLock() - defer proxy.lock.RUnlock() + return getProxiedImageURL(imageURL, *proxy.ConfigService.Config().ServiceSettings.SiteURL) +} - if proxy.backend == nil { +func getProxiedImageURL(imageURL, siteURL string) string { + if imageURL == "" || imageURL[0] == '/' || strings.HasPrefix(imageURL, siteURL) { return imageURL } - return proxy.backend.GetProxiedImageURL(imageURL) + return siteURL + "/api/v4/image?url=" + url.QueryEscape(imageURL) } // GetUnproxiedImageURL takes the URL of an image on the image proxy and returns the original URL of the image. func (proxy *ImageProxy) GetUnproxiedImageURL(proxiedURL string) string { - proxy.lock.RLock() - defer proxy.lock.RUnlock() + return getUnproxiedImageURL(proxiedURL, *proxy.ConfigService.Config().ServiceSettings.SiteURL) +} - if proxy.backend == nil { +func getUnproxiedImageURL(proxiedURL, siteURL string) string { + if !strings.HasPrefix(proxiedURL, siteURL+"/api/v4/image?url=") { return proxiedURL } - return proxy.backend.GetUnproxiedImageURL(proxiedURL) + parsed, err := url.Parse(proxiedURL) + if err != nil { + return proxiedURL + } + + u := parsed.Query()["url"] + if len(u) == 0 { + return proxiedURL + } + + return u[0] } diff --git a/services/imageproxy/imageproxy_test.go b/services/imageproxy/imageproxy_test.go new file mode 100644 index 0000000000..719dca73dd --- /dev/null +++ b/services/imageproxy/imageproxy_test.go @@ -0,0 +1,86 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package imageproxy + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGetProxiedImageURL(t *testing.T) { + siteURL := "https://mattermost.example.com" + + imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" + proxiedURL := "https://mattermost.example.com/api/v4/image?url=http%3A%2F%2Fwww.mattermost.org%2Fwp-content%2Fuploads%2F2016%2F03%2FlogoHorizontal.png" + + for _, test := range []struct { + Name string + Input string + Expected string + }{ + { + Name: "should proxy an image", + Input: imageURL, + Expected: proxiedURL, + }, + { + Name: "should not proxy a relative image", + Input: "/static/logo.png", + Expected: "/static/logo.png", + }, + { + Name: "should not proxy an image on the Mattermost server", + Input: "https://mattermost.example.com/static/logo.png", + Expected: "https://mattermost.example.com/static/logo.png", + }, + { + Name: "should not proxy an image that has already been proxied", + Input: proxiedURL, + Expected: proxiedURL, + }, + } { + t.Run(test.Name, func(t *testing.T) { + assert.Equal(t, test.Expected, getProxiedImageURL(test.Input, siteURL)) + }) + } +} + +func TestGetUnproxiedImageURL(t *testing.T) { + siteURL := "https://mattermost.example.com" + + imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" + proxiedURL := "https://mattermost.example.com/api/v4/image?url=http%3A%2F%2Fwww.mattermost.org%2Fwp-content%2Fuploads%2F2016%2F03%2FlogoHorizontal.png" + + for _, test := range []struct { + Name string + Input string + Expected string + }{ + { + Name: "should remove proxy", + Input: proxiedURL, + Expected: imageURL, + }, + { + Name: "should not remove proxy from a relative image", + Input: "/static/logo.png", + Expected: "/static/logo.png", + }, + { + Name: "should not remove proxy from an image on the Mattermost server", + Input: "https://mattermost.example.com/static/logo.png", + Expected: "https://mattermost.example.com/static/logo.png", + }, + { + Name: "should not remove proxy from a non-proxied image", + Input: imageURL, + Expected: imageURL, + }, + } { + t.Run(test.Name, func(t *testing.T) { + assert.Equal(t, test.Expected, getUnproxiedImageURL(test.Input, siteURL)) + }) + } +} diff --git a/services/imageproxy/local.go b/services/imageproxy/local.go index e25d86475d..0ffc4dc025 100644 --- a/services/imageproxy/local.go +++ b/services/imageproxy/local.go @@ -10,7 +10,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "strings" "time" "github.com/mattermost/mattermost-server/mlog" @@ -105,33 +104,3 @@ func (backend *LocalBackend) GetImageDirect(imageURL string) (io.ReadCloser, str return ioutil.NopCloser(recorder.Body), recorder.Header().Get("Content-Type"), nil } - -func (backend *LocalBackend) GetProxiedImageURL(imageURL string) string { - siteURL := *backend.proxy.ConfigService.Config().ServiceSettings.SiteURL - - if imageURL == "" || imageURL[0] == '/' || strings.HasPrefix(imageURL, siteURL) { - return imageURL - } - - return siteURL + "/api/v4/image?url=" + url.QueryEscape(imageURL) -} - -func (backend *LocalBackend) GetUnproxiedImageURL(proxiedURL string) string { - siteURL := *backend.proxy.ConfigService.Config().ServiceSettings.SiteURL - - if !strings.HasPrefix(proxiedURL, siteURL+"/api/v4/image?url=") { - return proxiedURL - } - - parsed, err := url.Parse(proxiedURL) - if err != nil { - return proxiedURL - } - - u := parsed.Query()["url"] - if len(u) == 0 { - return proxiedURL - } - - return u[0] -} diff --git a/services/imageproxy/local_test.go b/services/imageproxy/local_test.go index 8ff50401fb..6d1b46293a 100644 --- a/services/imageproxy/local_test.go +++ b/services/imageproxy/local_test.go @@ -293,79 +293,3 @@ func TestLocalBackend_GetImageDirect(t *testing.T) { wait <- true }) } - -func TestLocalBackend_GetProxiedImageURL(t *testing.T) { - imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" - proxiedURL := "https://mattermost.example.com/api/v4/image?url=http%3A%2F%2Fwww.mattermost.org%2Fwp-content%2Fuploads%2F2016%2F03%2FlogoHorizontal.png" - - proxy := makeTestLocalProxy() - - for _, test := range []struct { - Name string - Input string - Expected string - }{ - { - Name: "should proxy image", - Input: imageURL, - Expected: proxiedURL, - }, - { - Name: "should not proxy a relative image", - Input: "/static/logo.png", - Expected: "/static/logo.png", - }, - { - Name: "should not proxy an image on the Mattermost server", - Input: "https://mattermost.example.com/static/logo.png", - Expected: "https://mattermost.example.com/static/logo.png", - }, - { - Name: "should not proxy an image that has already been proxied", - Input: proxiedURL, - Expected: proxiedURL, - }, - } { - t.Run(test.Name, func(t *testing.T) { - assert.Equal(t, test.Expected, proxy.GetProxiedImageURL(test.Input)) - }) - } -} - -func TestLocalBackend_GetUnproxiedImageURL(t *testing.T) { - imageURL := "http://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png" - proxiedURL := "https://mattermost.example.com/api/v4/image?url=http%3A%2F%2Fwww.mattermost.org%2Fwp-content%2Fuploads%2F2016%2F03%2FlogoHorizontal.png" - - proxy := makeTestLocalProxy() - - for _, test := range []struct { - Name string - Input string - Expected string - }{ - { - Name: "should remove proxy", - Input: proxiedURL, - Expected: imageURL, - }, - { - Name: "should not remove proxy from a relative image", - Input: "/static/logo.png", - Expected: "/static/logo.png", - }, - { - Name: "should not remove proxy from an image on the Mattermost server", - Input: "https://mattermost.example.com/static/logo.png", - Expected: "https://mattermost.example.com/static/logo.png", - }, - { - Name: "should not remove proxy from a non-proxied image", - Input: imageURL, - Expected: imageURL, - }, - } { - t.Run(test.Name, func(t *testing.T) { - assert.Equal(t, test.Expected, proxy.GetUnproxiedImageURL(test.Input)) - }) - } -}