MM-13838 Bypass the HTTP client when getting image dimensions from the image proxy (#10208)

* MM-13838 Bypass the HTTP client when getting image dimensions from the proxy * Add additional log messages to debug failing test * Fix unit test to work on Jenkins
2019-02-04 12:43:30 -05:00
--- 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
--- 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) {
--- 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
--- 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
--- a/services/imageproxy/error.go
+++ 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
+}
--- 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 {
--- 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

--- 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"