diff --git a/app/post_metadata.go b/app/post_metadata.go index 9bc36ba22f..d0065452e5 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -346,18 +346,34 @@ func (a *App) getLinkMetadata(requestURL string, timestamp int64, isNewPost bool return nil, nil, err } - request.Header.Add("Accept", "text/html, image/*") + var body io.ReadCloser + var contentType string - client := a.HTTPService.MakeClient(false) - client.Timeout = time.Duration(*a.Config().ExperimentalSettings.LinkMetadataTimeoutMilliseconds) * time.Millisecond + if (request.URL.Scheme+"://"+request.URL.Host) == a.GetSiteURL() && request.URL.Path == "/api/v4/image" { + // /api/v4/image requires authentication, so bypass the API by hitting the proxy directly + body, contentType, err = a.ImageProxy.GetImageDirect(a.ImageProxy.GetUnproxiedImageURL(request.URL.String())) + } else { + request.Header.Add("Accept", "text/html, image/*") - res, err := client.Do(request) + client := a.HTTPService.MakeClient(false) + client.Timeout = time.Duration(*a.Config().ExperimentalSettings.LinkMetadataTimeoutMilliseconds) * time.Millisecond + + var res *http.Response + res, err = client.Do(request) + + if res != nil { + body = res.Body + contentType = res.Header.Get("Content-Type") + } + } + + if body != nil { + defer body.Close() + } if err == nil { - defer res.Body.Close() - // Parse the data - og, image, err = a.parseLinkMetadata(requestURL, res.Body, res.Header.Get("Content-Type")) + og, image, err = a.parseLinkMetadata(requestURL, body, contentType) } // Write back to cache and database, even if there was an error and the results are nil diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index 81b347e475..ccd44ee8f5 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -19,6 +19,8 @@ import ( "github.com/dyatlov/go-opengraph/opengraph" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/services/httpservice" + "github.com/mattermost/mattermost-server/services/imageproxy" "github.com/mattermost/mattermost-server/utils/testutils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -1064,8 +1066,6 @@ func TestGetLinkMetadata(t *testing.T) { return th } - th := Setup().InitBasic() - defer th.TearDown() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { params := r.URL.Query() @@ -1510,6 +1510,11 @@ func TestGetLinkMetadata(t *testing.T) { defer th.TearDown() // Fake the SiteURL to have the relative URL resolve to the external server + oldSiteURL := *th.App.Config().ServiceSettings.SiteURL + defer th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.SiteURL = oldSiteURL + }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = server.URL }) @@ -1522,6 +1527,45 @@ func TestGetLinkMetadata(t *testing.T) { assert.NotNil(t, img) assert.Nil(t, err) }) + + t.Run("should error on local addresses other than the image proxy", func(t *testing.T) { + th := setup() + defer th.TearDown() + + // Disable AllowedUntrustedInternalConnections since it's turned on for the previous tests + oldAllowUntrusted := *th.App.Config().ServiceSettings.AllowedUntrustedInternalConnections + oldSiteURL := *th.App.Config().ServiceSettings.SiteURL + defer th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = oldAllowUntrusted + *cfg.ServiceSettings.SiteURL = oldSiteURL + }) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "" + *cfg.ServiceSettings.SiteURL = "http://mattermost.example.com" + *cfg.ImageProxySettings.Enable = true + *cfg.ImageProxySettings.ImageProxyType = "local" + }) + + requestURL := server.URL + "/image?height=200&width=300&name=" + t.Name() + timestamp := int64(1547510400000) + + og, img, err := th.App.getLinkMetadata(requestURL, timestamp, false) + assert.Nil(t, og) + assert.Nil(t, img) + assert.NotNil(t, err) + assert.IsType(t, &url.Error{}, err) + assert.Equal(t, httpservice.AddressForbidden, err.(*url.Error).Err) + + requestURL = th.App.GetSiteURL() + "/api/v4/image?url=" + url.QueryEscape(requestURL) + + // Note that this request still fails while testing because the request made by the image proxy is blocked + og, img, err = th.App.getLinkMetadata(requestURL, timestamp, false) + assert.Nil(t, og) + assert.Nil(t, img) + assert.NotNil(t, err) + assert.IsType(t, imageproxy.Error{}, err) + }) } func TestResolveMetadataURL(t *testing.T) { diff --git a/services/imageproxy/atmos_camo.go b/services/imageproxy/atmos_camo.go index ca98f85339..f66ea033e3 100644 --- a/services/imageproxy/atmos_camo.go +++ b/services/imageproxy/atmos_camo.go @@ -7,6 +7,7 @@ import ( "crypto/hmac" "crypto/sha1" "encoding/hex" + "io" "net/http" "strings" ) @@ -25,6 +26,23 @@ func (backend *AtmosCamoBackend) GetImage(w http.ResponseWriter, r *http.Request http.Redirect(w, r, backend.GetProxiedImageURL(imageURL), http.StatusFound) } +func (backend *AtmosCamoBackend) GetImageDirect(imageURL string) (io.ReadCloser, string, error) { + req, err := http.NewRequest("GET", backend.GetProxiedImageURL(imageURL), nil) + if err != nil { + return nil, "", Error{err} + } + + client := backend.proxy.HTTPService.MakeClient(false) + + resp, err := client.Do(req) + if err != nil { + return nil, "", Error{err} + } + + // Note that we don't do any additional validation of the received data since we expect the image proxy to do that + return resp.Body, resp.Header.Get("Content-Type"), nil +} + func (backend *AtmosCamoBackend) GetProxiedImageURL(imageURL string) string { cfg := *backend.proxy.ConfigService.Config() siteURL := *cfg.ServiceSettings.SiteURL diff --git a/services/imageproxy/atmos_camo_test.go b/services/imageproxy/atmos_camo_test.go index 154d0c0087..ec9133f76e 100644 --- a/services/imageproxy/atmos_camo_test.go +++ b/services/imageproxy/atmos_camo_test.go @@ -4,20 +4,24 @@ package imageproxy import ( + "io/ioutil" "net/http" "net/http/httptest" "testing" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/services/httpservice" "github.com/mattermost/mattermost-server/utils/testutils" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func makeTestAtmosCamoProxy() *ImageProxy { configService := &testutils.StaticConfigService{ Cfg: &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: model.NewString("https://mattermost.example.com"), + SiteURL: model.NewString("https://mattermost.example.com"), + AllowedUntrustedInternalConnections: model.NewString("127.0.0.1"), }, ImageProxySettings: model.ImageProxySettings{ Enable: model.NewBool(true), @@ -28,7 +32,7 @@ func makeTestAtmosCamoProxy() *ImageProxy { }, } - return MakeImageProxy(configService, nil) + return MakeImageProxy(configService, httpservice.MakeHTTPService(configService)) } func TestAtmosCamoBackend_GetImage(t *testing.T) { @@ -46,10 +50,42 @@ func TestAtmosCamoBackend_GetImage(t *testing.T) { assert.Equal(t, proxiedURL, resp.Header.Get("Location")) } +func TestAtmosCamoBackend_GetImageDirect(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "max-age=2592000, private") + w.Header().Set("Content-Type", "image/png") + w.Header().Set("Content-Length", "10") + + w.WriteHeader(http.StatusOK) + w.Write([]byte("1111111111")) + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestAtmosCamoProxy() + proxy.ConfigService.(*testutils.StaticConfigService).Cfg.ImageProxySettings.RemoteImageProxyURL = model.NewString(mock.URL) + + body, contentType, err := proxy.GetImageDirect("https://example.com/image.png") + + assert.Nil(t, err) + assert.Equal(t, "image/png", contentType) + + require.NotNil(t, body) + respBody, _ := ioutil.ReadAll(body) + 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 diff --git a/services/imageproxy/error.go b/services/imageproxy/error.go new file mode 100644 index 0000000000..3a46f64a31 --- /dev/null +++ b/services/imageproxy/error.go @@ -0,0 +1,8 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package imageproxy + +type Error struct { + error +} diff --git a/services/imageproxy/imageproxy.go b/services/imageproxy/imageproxy.go index d694862310..103dcf285c 100644 --- a/services/imageproxy/imageproxy.go +++ b/services/imageproxy/imageproxy.go @@ -4,6 +4,8 @@ package imageproxy import ( + "errors" + "io" "net/http" "sync" @@ -12,6 +14,8 @@ import ( "github.com/mattermost/mattermost-server/services/httpservice" ) +var ErrNotEnabled = Error{errors.New("imageproxy.ImageProxy: image proxy not enabled")} + // An ImageProxy is the public interface for Mattermost's image proxy. An instance of ImageProxy should be created // using MakeImageProxy which requires a configService and an HTTPService provided by the server. type ImageProxy struct { @@ -30,6 +34,9 @@ type ImageProxyBackend interface { // GetImage provides a proxied image in response to an HTTP request. GetImage(w http.ResponseWriter, r *http.Request, imageURL string) + // 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 @@ -97,6 +104,18 @@ func (proxy *ImageProxy) GetImage(w http.ResponseWriter, r *http.Request, imageU proxy.backend.GetImage(w, r, imageURL) } +// GetImageDirect takes the URL of an image and returns the image along with its content type. +func (proxy *ImageProxy) GetImageDirect(imageURL string) (io.ReadCloser, string, error) { + proxy.lock.RLock() + defer proxy.lock.RUnlock() + + if proxy.backend == nil { + return nil, "", ErrNotEnabled + } + + return proxy.backend.GetImageDirect(imageURL) +} + // 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 { diff --git a/services/imageproxy/local.go b/services/imageproxy/local.go index 3308ba27d2..84425f708e 100644 --- a/services/imageproxy/local.go +++ b/services/imageproxy/local.go @@ -4,7 +4,11 @@ package imageproxy import ( + "errors" + "io" + "io/ioutil" "net/http" + "net/http/httptest" "net/url" "strings" "time" @@ -27,6 +31,8 @@ var imageContentTypes = []string{ "image/x-quicktime", "image/x-rgb", "image/x-xbitmap", "image/x-xpixmap", "image/x-xwindowdump", } +var ErrLocalRequestFailed = Error{errors.New("imageproxy.LocalBackend: failed to request proxied image")} + type LocalBackend struct { proxy *ImageProxy @@ -68,6 +74,24 @@ func (backend *LocalBackend) GetImage(w http.ResponseWriter, r *http.Request, im backend.impl.ServeHTTP(w, req) } +func (backend *LocalBackend) GetImageDirect(imageURL string) (io.ReadCloser, string, error) { + // The interface to the proxy only exposes a ServeHTTP method, so fake a request to it + req, err := http.NewRequest(http.MethodGet, "/"+imageURL, nil) + if err != nil { + return nil, "", Error{err} + } + + recorder := httptest.NewRecorder() + + backend.impl.ServeHTTP(recorder, req) + + if recorder.Code != http.StatusOK { + return nil, "", ErrLocalRequestFailed + } + + 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 diff --git a/services/imageproxy/local_test.go b/services/imageproxy/local_test.go index b2e84abb97..b07473b2ba 100644 --- a/services/imageproxy/local_test.go +++ b/services/imageproxy/local_test.go @@ -166,6 +166,134 @@ func TestLocalBackend_GetImage(t *testing.T) { }) } +func TestLocalBackend_GetImageDirect(t *testing.T) { + t.Run("image", func(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "max-age=2592000, private") + w.Header().Set("Content-Type", "image/png") + w.Header().Set("Content-Length", "10") + + w.WriteHeader(http.StatusOK) + w.Write([]byte("1111111111")) + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestLocalProxy() + + body, contentType, err := proxy.GetImageDirect(mock.URL + "/image.png") + + assert.Nil(t, err) + assert.Equal(t, "image/png", contentType) + + respBody, _ := ioutil.ReadAll(body) + assert.Equal(t, []byte("1111111111"), respBody) + }) + + t.Run("not an image", func(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotAcceptable) + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestLocalProxy() + + body, contentType, err := proxy.GetImageDirect(mock.URL + "/file.pdf") + + assert.NotNil(t, err) + assert.Equal(t, "", contentType) + assert.Equal(t, ErrLocalRequestFailed, err) + assert.Nil(t, body) + }) + + t.Run("not an image, but remote server ignores accept header", func(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "max-age=2592000, private") + w.Header().Set("Content-Type", "application/pdf") + w.Header().Set("Content-Length", "10") + + w.WriteHeader(http.StatusOK) + w.Write([]byte("1111111111")) + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestLocalProxy() + + body, contentType, err := proxy.GetImageDirect(mock.URL + "/file.pdf") + + assert.NotNil(t, err) + assert.Equal(t, "", contentType) + assert.Equal(t, ErrLocalRequestFailed, err) + assert.Nil(t, body) + }) + + t.Run("not found", func(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestLocalProxy() + + body, contentType, err := proxy.GetImageDirect(mock.URL + "/image.png") + + assert.NotNil(t, err) + assert.Equal(t, "", contentType) + assert.Equal(t, ErrLocalRequestFailed, err) + assert.Nil(t, body) + }) + + t.Run("other server error", func(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestLocalProxy() + + body, contentType, err := proxy.GetImageDirect(mock.URL + "/image.png") + + assert.NotNil(t, err) + assert.Equal(t, "", contentType) + assert.Equal(t, ErrLocalRequestFailed, err) + assert.Nil(t, body) + }) + + t.Run("timeout", func(t *testing.T) { + wait := make(chan bool, 1) + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-wait + }) + + mock := httptest.NewServer(handler) + defer mock.Close() + + proxy := makeTestLocalProxy() + + // Modify the timeout to be much shorter than the default 30 seconds + proxy.backend.(*LocalBackend).impl.Timeout = time.Millisecond + + body, contentType, err := proxy.GetImageDirect(mock.URL + "/image.png") + + assert.NotNil(t, err) + assert.Equal(t, "", contentType) + assert.Equal(t, ErrLocalRequestFailed, err) + assert.Nil(t, body) + + 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"