MM-26425: Make URL parsing for image proxy more robust (#16197)

* MM-26425: Make URL parsing for image proxy more robust

- We don't bypass protocol relative URLs.
- We don't bypass hostnames with a similar prefix.

https: //mattermost.atlassian.net/browse/MM-26425

```release-note
NONE
```

* fix tests and incorporate review comments

* Handle opaque URLs

* Fix path tests

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

app/post_metadata_test.go

@@ -560,6 +560,8 @@ func TestPreparePostForClientWithImageProxy(t *testing.T) {
 			*cfg.ImageProxySettings.RemoteImageProxyOptions = "foo"
 		})
 
+		th.Server.ImageProxy = imageproxy.MakeImageProxy(th.Server, th.Server.HTTPService, th.Server.Log)
+
 		return th
 	}
 

app/post_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock"
+	"github.com/mattermost/mattermost-server/v5/services/imageproxy"
 	"github.com/mattermost/mattermost-server/v5/services/searchengine/mocks"
 	"github.com/mattermost/mattermost-server/v5/store/storetest"
 	storemocks "github.com/mattermost/mattermost-server/v5/store/storetest/mocks"
@@ -471,60 +472,71 @@ func TestImageProxy(t *testing.T) {
 		*cfg.ServiceSettings.SiteURL = "http://mymattermost.com"
 	})
 
+	th.Server.ImageProxy = imageproxy.MakeImageProxy(th.Server, th.Server.HTTPService, th.Server.Log)
+
 	for name, tc := range map[string]struct {
-		ProxyType       string
+		ProxyType              string
-		ProxyURL        string
+		ProxyURL               string
-		ProxyOptions    string
+		ProxyOptions           string
-		ImageURL        string
+		ImageURL               string
-		ProxiedImageURL string
+		ProxiedImageURL        string
+		ProxiedRemovedImageURL string
 	}{
 		"atmos/camo": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
+			ProxyType:              model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
-			ProxyURL:        "https://127.0.0.1",
+			ProxyURL:               "https://127.0.0.1",
-			ProxyOptions:    "foo",
+			ProxyOptions:           "foo",
-			ImageURL:        "http://mydomain.com/myimage",
+			ImageURL:               "http://mydomain.com/myimage",
-			ProxiedImageURL: "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage",
+			ProxiedRemovedImageURL: "http://mydomain.com/myimage",
+			ProxiedImageURL:        "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage",
 		},
 		"atmos/camo_SameSite": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
+			ProxyType:              model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
-			ProxyURL:        "https://127.0.0.1",
+			ProxyURL:               "https://127.0.0.1",
-			ProxyOptions:    "foo",
+			ProxyOptions:           "foo",
-			ImageURL:        "http://mymattermost.com/myimage",
+			ImageURL:               "http://mymattermost.com/myimage",
-			ProxiedImageURL: "http://mymattermost.com/myimage",
+			ProxiedRemovedImageURL: "http://mymattermost.com/myimage",
+			ProxiedImageURL:        "http://mymattermost.com/myimage",
 		},
 		"atmos/camo_PathOnly": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
+			ProxyType:              model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
-			ProxyURL:        "https://127.0.0.1",
+			ProxyURL:               "https://127.0.0.1",
-			ProxyOptions:    "foo",
+			ProxyOptions:           "foo",
-			ImageURL:        "/myimage",
+			ImageURL:               "/myimage",
-			ProxiedImageURL: "/myimage",
+			ProxiedRemovedImageURL: "http://mymattermost.com/myimage",
+			ProxiedImageURL:        "http://mymattermost.com/myimage",
 		},
 		"atmos/camo_EmptyImageURL": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
+			ProxyType:              model.IMAGE_PROXY_TYPE_ATMOS_CAMO,
-			ProxyURL:        "https://127.0.0.1",
+			ProxyURL:               "https://127.0.0.1",
-			ProxyOptions:    "foo",
+			ProxyOptions:           "foo",
-			ImageURL:        "",
+			ImageURL:               "",
-			ProxiedImageURL: "",
+			ProxiedRemovedImageURL: "",
+			ProxiedImageURL:        "",
 		},
 		"local": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_LOCAL,
+			ProxyType:              model.IMAGE_PROXY_TYPE_LOCAL,
-			ImageURL:        "http://mydomain.com/myimage",
+			ImageURL:               "http://mydomain.com/myimage",
-			ProxiedImageURL: "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage",
+			ProxiedRemovedImageURL: "http://mydomain.com/myimage",
+			ProxiedImageURL:        "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage",
 		},
 		"local_SameSite": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_LOCAL,
+			ProxyType:              model.IMAGE_PROXY_TYPE_LOCAL,
-			ImageURL:        "http://mymattermost.com/myimage",
+			ImageURL:               "http://mymattermost.com/myimage",
-			ProxiedImageURL: "http://mymattermost.com/myimage",
+			ProxiedRemovedImageURL: "http://mymattermost.com/myimage",
+			ProxiedImageURL:        "http://mymattermost.com/myimage",
 		},
 		"local_PathOnly": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_LOCAL,
+			ProxyType:              model.IMAGE_PROXY_TYPE_LOCAL,
-			ImageURL:        "/myimage",
+			ImageURL:               "/myimage",
-			ProxiedImageURL: "/myimage",
+			ProxiedRemovedImageURL: "http://mymattermost.com/myimage",
+			ProxiedImageURL:        "http://mymattermost.com/myimage",
 		},
 		"local_EmptyImageURL": {
-			ProxyType:       model.IMAGE_PROXY_TYPE_LOCAL,
+			ProxyType:              model.IMAGE_PROXY_TYPE_LOCAL,
-			ImageURL:        "",
+			ImageURL:               "",
-			ProxiedImageURL: "",
+			ProxiedRemovedImageURL: "",
+			ProxiedImageURL:        "",
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
@@ -547,14 +559,14 @@ func TestImageProxy(t *testing.T) {
 
 			assert.Equal(t, "![foo]("+tc.ImageURL+")", th.App.PostWithProxyRemovedFromImageURLs(post).Message)
 			post.Message = "![foo](" + tc.ProxiedImageURL + ")"
-			assert.Equal(t, "![foo]("+tc.ImageURL+")", th.App.PostWithProxyRemovedFromImageURLs(post).Message)
+			assert.Equal(t, "![foo]("+tc.ProxiedRemovedImageURL+")", th.App.PostWithProxyRemovedFromImageURLs(post).Message)
 
 			if tc.ImageURL != "" {
 				post.Message = "![foo](" + tc.ImageURL + " =500x200)"
 				assert.Equal(t, "![foo]("+tc.ProxiedImageURL+" =500x200)", th.App.PostWithProxyAddedToImageURLs(post).Message)
 				assert.Equal(t, "![foo]("+tc.ImageURL+" =500x200)", th.App.PostWithProxyRemovedFromImageURLs(post).Message)
 				post.Message = "![foo](" + tc.ProxiedImageURL + " =500x200)"
-				assert.Equal(t, "![foo]("+tc.ImageURL+" =500x200)", th.App.PostWithProxyRemovedFromImageURLs(post).Message)
+				assert.Equal(t, "![foo]("+tc.ProxiedRemovedImageURL+" =500x200)", th.App.PostWithProxyRemovedFromImageURLs(post).Message)
 			}
 		})
 	}
@@ -675,6 +687,8 @@ func TestCreatePost(t *testing.T) {
 			*cfg.ImageProxySettings.RemoteImageProxyOptions = "foo"
 		})
 
+		th.Server.ImageProxy = imageproxy.MakeImageProxy(th.Server, th.Server.HTTPService, th.Server.Log)
+
 		imageURL := "http://mydomain.com/myimage"
 		proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage"
 
@@ -755,6 +769,8 @@ func TestPatchPost(t *testing.T) {
 			*cfg.ImageProxySettings.RemoteImageProxyOptions = "foo"
 		})
 
+		th.Server.ImageProxy = imageproxy.MakeImageProxy(th.Server, th.Server.HTTPService, th.Server.Log)
+
 		imageURL := "http://mydomain.com/myimage"
 		proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage"
 
@@ -975,6 +991,8 @@ func TestUpdatePost(t *testing.T) {
 			*cfg.ImageProxySettings.RemoteImageProxyOptions = "foo"
 		})
 
+		th.Server.ImageProxy = imageproxy.MakeImageProxy(th.Server, th.Server.HTTPService, th.Server.Log)
+
 		imageURL := "http://mydomain.com/myimage"
 		proxiedImageURL := "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage"
 

services/imageproxy/atmos_camo.go

@@ -9,16 +9,25 @@ import (
 	"encoding/hex"
 	"io"
 	"net/http"
-	"strings"
+	"net/url"
 )
 
 type AtmosCamoBackend struct {
-	proxy *ImageProxy
+	proxy     *ImageProxy
+	siteURL   *url.URL
+	remoteURL *url.URL
 }
 
 func makeAtmosCamoBackend(proxy *ImageProxy) *AtmosCamoBackend {
+	// We deliberately ignore the error because it's from config.json.
+	// The function returns a nil pointer in case of error, and we handle it when it's used.
+	siteURL, _ := url.Parse(*proxy.ConfigService.Config().ServiceSettings.SiteURL)
+	remoteURL, _ := url.Parse(*proxy.ConfigService.Config().ImageProxySettings.RemoteImageProxyURL)
+
 	return &AtmosCamoBackend{
-		proxy: proxy,
+		proxy:     proxy,
+		siteURL:   siteURL,
+		remoteURL: remoteURL,
 	}
 }
 
@@ -45,22 +54,39 @@ func (backend *AtmosCamoBackend) GetImageDirect(imageURL string) (io.ReadCloser,
 
 func (backend *AtmosCamoBackend) getAtmosCamoImageURL(imageURL string) string {
 	cfg := *backend.proxy.ConfigService.Config()
-	siteURL := *cfg.ServiceSettings.SiteURL
-	proxyURL := *cfg.ImageProxySettings.RemoteImageProxyURL
 	options := *cfg.ImageProxySettings.RemoteImageProxyOptions
 
-	return getAtmosCamoImageURL(imageURL, siteURL, proxyURL, options)
+	if imageURL == "" || backend.siteURL == nil {
-}
-
-func getAtmosCamoImageURL(imageURL, siteURL, proxyURL, options string) string {
-	// Don't proxy blank images, relative URLs, absolute URLs on this server, or URLs that are already going through the proxy
-	if imageURL == "" || imageURL[0] == '/' || (siteURL != "" && strings.HasPrefix(imageURL, siteURL)) || strings.HasPrefix(imageURL, proxyURL) {
 		return imageURL
 	}
 
+	// Parse url, return siteURL in case of failure.
+	// Also if the URL is opaque.
+	parsedURL, err := url.Parse(imageURL)
+	if err != nil || parsedURL.Opaque != "" {
+		return backend.siteURL.String()
+	}
+
+	// If host is same as siteURL host/ remoteURL host, return.
+	if parsedURL.Host == backend.siteURL.Host || parsedURL.Host == backend.remoteURL.Host {
+		return parsedURL.String()
+	}
+
+	// Handle protocol-relative URLs.
+	if parsedURL.Scheme == "" {
+		parsedURL.Scheme = backend.siteURL.Scheme
+	}
+
+	// If it's a relative URL, fill up the hostname and scheme and return.
+	if parsedURL.Host == "" {
+		parsedURL.Host = backend.siteURL.Host
+		return parsedURL.String()
+	}
+
+	urlBytes := []byte(parsedURL.String())
 	mac := hmac.New(sha1.New, []byte(options))
-	mac.Write([]byte(imageURL))
+	mac.Write(urlBytes)
 	digest := hex.EncodeToString(mac.Sum(nil))
 
-	return proxyURL + "/" + digest + "/" + hex.EncodeToString([]byte(imageURL))
+	return backend.remoteURL.String() + "/" + digest + "/" + hex.EncodeToString(urlBytes)
 }

services/imageproxy/atmos_camo_test.go

@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/mattermost/mattermost-server/v5/model"
@@ -64,9 +65,19 @@ func TestAtmosCamoBackend_GetImageDirect(t *testing.T) {
 	defer mock.Close()
 
 	proxy := makeTestAtmosCamoProxy()
-	proxy.ConfigService.(*testutils.StaticConfigService).Cfg.ImageProxySettings.RemoteImageProxyURL = model.NewString(mock.URL)
+	parsedURL, err := url.Parse(*proxy.ConfigService.Config().ServiceSettings.SiteURL)
+	require.NoError(t, err)
 
-	body, contentType, err := proxy.GetImageDirect("https://example.com/image.png")
+	remoteURL, err := url.Parse(mock.URL)
+	require.NoError(t, err)
+
+	backend := &AtmosCamoBackend{
+		proxy:     proxy,
+		siteURL:   parsedURL,
+		remoteURL: remoteURL,
+	}
+
+	body, contentType, err := backend.GetImageDirect("https://example.com/image.png")
 
 	assert.Nil(t, err)
 	assert.Equal(t, "image/png", contentType)
@@ -82,7 +93,6 @@ func TestGetAtmosCamoImageURL(t *testing.T) {
 
 	defaultSiteURL := "https://mattermost.example.com"
 	proxyURL := "http://images.example.com"
-	options := "7e5f3fab20b94782b43cdb022a66985ef28ba355df2c5d5da3c9a05e4b697bac"
 
 	for _, test := range []struct {
 		Name     string
@@ -112,7 +122,13 @@ func TestGetAtmosCamoImageURL(t *testing.T) {
 			Name:     "should not proxy a relative image",
 			Input:    "/static/logo.png",
 			SiteURL:  defaultSiteURL,
-			Expected: "/static/logo.png",
+			Expected: "https://mattermost.example.com/static/logo.png",
+		},
+		{
+			Name:     "should bypass opaque URLs",
+			Input:    "http:xyz123?query",
+			SiteURL:  defaultSiteURL,
+			Expected: defaultSiteURL,
 		},
 		{
 			Name:     "should not proxy an image on the Mattermost server",
@@ -132,9 +148,39 @@ func TestGetAtmosCamoImageURL(t *testing.T) {
 			SiteURL:  defaultSiteURL,
 			Expected: proxiedURL,
 		},
+		{
+			Name:     "should not bypass protocol relative URLs",
+			Input:    "//www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png",
+			SiteURL:  "http://mattermost.example.com",
+			Expected: proxiedURL,
+		},
+		{
+			Name:     "should not bypass if the host prefix is same",
+			Input:    "http://www.mattermost.org.example.com/wp-content/uploads/2016/03/logoHorizontal.png",
+			SiteURL:  defaultSiteURL,
+			Expected: "http://images.example.com/99dcf38b8e6110d6e3ebcfb7a2db9ce875bc5c03/687474703a2f2f7777772e6d61747465726d6f73742e6f72672e6578616d706c652e636f6d2f77702d636f6e74656e742f75706c6f6164732f323031362f30332f6c6f676f486f72697a6f6e74616c2e706e67",
+		},
+		{
+			Name:     "should not bypass for user auth URLs",
+			Input:    "http://www.mattermost.org@example.com/wp-content/uploads/2016/03/logoHorizontal.png",
+			SiteURL:  defaultSiteURL,
+			Expected: "http://images.example.com/19deedea7c0b75369f8d2162ee4e7ab36e26ca50/687474703a2f2f7777772e6d61747465726d6f73742e6f7267406578616d706c652e636f6d2f77702d636f6e74656e742f75706c6f6164732f323031362f30332f6c6f676f486f72697a6f6e74616c2e706e67",
+		},
 	} {
 		t.Run(test.Name, func(t *testing.T) {
-			assert.Equal(t, test.Expected, getAtmosCamoImageURL(test.Input, test.SiteURL, proxyURL, options))
+			parsedURL, err := url.Parse(test.SiteURL)
+			require.NoError(t, err)
+
+			remoteURL, err := url.Parse(proxyURL)
+			require.NoError(t, err)
+
+			backend := &AtmosCamoBackend{
+				proxy:     makeTestAtmosCamoProxy(),
+				siteURL:   parsedURL,
+				remoteURL: remoteURL,
+			}
+
+			assert.Equal(t, test.Expected, backend.getAtmosCamoImageURL(test.Input))
 		})
 	}
 

services/imageproxy/imageproxy.go

@@ -29,6 +29,7 @@ type ImageProxy struct {
 
 	Logger *mlog.Logger
 
+	siteURL *url.URL
 	lock    sync.RWMutex
 	backend ImageProxyBackend
 }
@@ -50,6 +51,11 @@ func MakeImageProxy(configService configservice.ConfigService, httpService https
 		Logger:        logger,
 	}
 
+	// We deliberately ignore the error because it's from config.json.
+	// The function returns a nil pointer in case of error, and we handle it when it's used.
+	siteURL, _ := url.Parse(*configService.Config().ServiceSettings.SiteURL)
+	proxy.siteURL = siteURL
+
 	proxy.configListenerId = proxy.ConfigService.AddConfigListener(proxy.OnConfigChange)
 
 	config := proxy.ConfigService.Config()
@@ -118,15 +124,32 @@ 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 {
-	return getProxiedImageURL(imageURL, *proxy.ConfigService.Config().ServiceSettings.SiteURL)
+	if imageURL == "" || proxy.siteURL == nil {
-}
-
-func getProxiedImageURL(imageURL, siteURL string) string {
-	if imageURL == "" || imageURL[0] == '/' || strings.HasPrefix(imageURL, siteURL) {
 		return imageURL
 	}
+	// Parse url, return siteURL in case of failure.
+	// Also if the URL is opaque.
+	parsedURL, err := url.Parse(imageURL)
+	if err != nil || parsedURL.Opaque != "" {
+		return proxy.siteURL.String()
+	}
+	// If host is same as siteURL host, return.
+	if parsedURL.Host == proxy.siteURL.Host {
+		return parsedURL.String()
+	}
 
-	return siteURL + "/api/v4/image?url=" + url.QueryEscape(imageURL)
+	// Handle protocol-relative URLs.
+	if parsedURL.Scheme == "" {
+		parsedURL.Scheme = proxy.siteURL.Scheme
+	}
+
+	// If it's a relative URL, fill up the hostname and return.
+	if parsedURL.Host == "" {
+		parsedURL.Host = proxy.siteURL.Host
+		return parsedURL.String()
+	}
+
+	return proxy.siteURL.String() + "/api/v4/image?url=" + url.QueryEscape(parsedURL.String())
 }
 
 // GetUnproxiedImageURL takes the URL of an image on the image proxy and returns the original URL of the image.

services/imageproxy/imageproxy_test.go

@@ -4,17 +4,23 @@
 package imageproxy
 
 import (
+	"net/url"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestGetProxiedImageURL(t *testing.T) {
 	siteURL := "https://mattermost.example.com"
+	parsedURL, err := url.Parse(siteURL)
+	require.NoError(t, err)
 
 	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 := ImageProxy{siteURL: parsedURL}
+
 	for _, test := range []struct {
 		Name     string
 		Input    string
@@ -28,7 +34,12 @@ func TestGetProxiedImageURL(t *testing.T) {
 		{
 			Name:     "should not proxy a relative image",
 			Input:    "/static/logo.png",
-			Expected: "/static/logo.png",
+			Expected: "https://mattermost.example.com/static/logo.png",
+		},
+		{
+			Name:     "should bypass opaque URLs",
+			Input:    "http:xyz123?query",
+			Expected: siteURL,
 		},
 		{
 			Name:     "should not proxy an image on the Mattermost server",
@@ -40,9 +51,24 @@ func TestGetProxiedImageURL(t *testing.T) {
 			Input:    proxiedURL,
 			Expected: proxiedURL,
 		},
+		{
+			Name:     "should not bypass protocol relative URLs",
+			Input:    "//mattermost.org/static/logo.png",
+			Expected: "https://mattermost.example.com/api/v4/image?url=https%3A%2F%2Fmattermost.org%2Fstatic%2Flogo.png",
+		},
+		{
+			Name:     "should not bypass if the host prefix is same",
+			Input:    "https://mattermost.example.com.anothersite.com/static/logo.png",
+			Expected: "https://mattermost.example.com/api/v4/image?url=https%3A%2F%2Fmattermost.example.com.anothersite.com%2Fstatic%2Flogo.png",
+		},
+		{
+			Name:     "should not bypass for user auth URLs",
+			Input:    "https://mattermost.example.com@anothersite.com/static/logo.png",
+			Expected: "https://mattermost.example.com/api/v4/image?url=https%3A%2F%2Fmattermost.example.com%40anothersite.com%2Fstatic%2Flogo.png",
+		},
 	} {
 		t.Run(test.Name, func(t *testing.T) {
-			assert.Equal(t, test.Expected, getProxiedImageURL(test.Input, siteURL))
+			assert.Equal(t, test.Expected, proxy.GetProxiedImageURL(test.Input))
 		})
 	}
 }