diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index 9730a87fb2..4c2ca457d9 100644 --- a/app/post_metadata_test.go +++ b/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 } diff --git a/app/post_test.go b/app/post_test.go index c3abf9bd7e..196ed71256 100644 --- a/app/post_test.go +++ b/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 - ProxyURL string - ProxyOptions string - ImageURL string - ProxiedImageURL string + ProxyType string + ProxyURL string + ProxyOptions string + ImageURL string + ProxiedImageURL string + ProxiedRemovedImageURL string }{ "atmos/camo": { - ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, - ProxyURL: "https://127.0.0.1", - ProxyOptions: "foo", - ImageURL: "http://mydomain.com/myimage", - ProxiedImageURL: "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage", + ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, + ProxyURL: "https://127.0.0.1", + ProxyOptions: "foo", + ImageURL: "http://mydomain.com/myimage", + 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, - ProxyURL: "https://127.0.0.1", - ProxyOptions: "foo", - ImageURL: "http://mymattermost.com/myimage", - ProxiedImageURL: "http://mymattermost.com/myimage", + ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, + ProxyURL: "https://127.0.0.1", + ProxyOptions: "foo", + ImageURL: "http://mymattermost.com/myimage", + ProxiedRemovedImageURL: "http://mymattermost.com/myimage", + ProxiedImageURL: "http://mymattermost.com/myimage", }, "atmos/camo_PathOnly": { - ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, - ProxyURL: "https://127.0.0.1", - ProxyOptions: "foo", - ImageURL: "/myimage", - ProxiedImageURL: "/myimage", + ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, + ProxyURL: "https://127.0.0.1", + ProxyOptions: "foo", + ImageURL: "/myimage", + ProxiedRemovedImageURL: "http://mymattermost.com/myimage", + ProxiedImageURL: "http://mymattermost.com/myimage", }, "atmos/camo_EmptyImageURL": { - ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, - ProxyURL: "https://127.0.0.1", - ProxyOptions: "foo", - ImageURL: "", - ProxiedImageURL: "", + ProxyType: model.IMAGE_PROXY_TYPE_ATMOS_CAMO, + ProxyURL: "https://127.0.0.1", + ProxyOptions: "foo", + ImageURL: "", + ProxiedRemovedImageURL: "", + ProxiedImageURL: "", }, "local": { - ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, - ImageURL: "http://mydomain.com/myimage", - ProxiedImageURL: "http://mymattermost.com/api/v4/image?url=http%3A%2F%2Fmydomain.com%2Fmyimage", + ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, + ImageURL: "http://mydomain.com/myimage", + 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, - ImageURL: "http://mymattermost.com/myimage", - ProxiedImageURL: "http://mymattermost.com/myimage", + ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, + ImageURL: "http://mymattermost.com/myimage", + ProxiedRemovedImageURL: "http://mymattermost.com/myimage", + ProxiedImageURL: "http://mymattermost.com/myimage", }, "local_PathOnly": { - ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, - ImageURL: "/myimage", - ProxiedImageURL: "/myimage", + ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, + ImageURL: "/myimage", + ProxiedRemovedImageURL: "http://mymattermost.com/myimage", + ProxiedImageURL: "http://mymattermost.com/myimage", }, "local_EmptyImageURL": { - ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, - ImageURL: "", - ProxiedImageURL: "", + ProxyType: model.IMAGE_PROXY_TYPE_LOCAL, + ImageURL: "", + 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" diff --git a/services/imageproxy/atmos_camo.go b/services/imageproxy/atmos_camo.go index 88cd3e2567..d22dafc4ba 100644 --- a/services/imageproxy/atmos_camo.go +++ b/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) -} - -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) { + if imageURL == "" || backend.siteURL == nil { 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) } diff --git a/services/imageproxy/atmos_camo_test.go b/services/imageproxy/atmos_camo_test.go index cc10c0bd72..923040fb77 100644 --- a/services/imageproxy/atmos_camo_test.go +++ b/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)) }) } diff --git a/services/imageproxy/imageproxy.go b/services/imageproxy/imageproxy.go index a6913b4fee..32eaeb131c 100644 --- a/services/imageproxy/imageproxy.go +++ b/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) -} - -func getProxiedImageURL(imageURL, siteURL string) string { - if imageURL == "" || imageURL[0] == '/' || strings.HasPrefix(imageURL, siteURL) { + if imageURL == "" || proxy.siteURL == nil { 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. diff --git a/services/imageproxy/imageproxy_test.go b/services/imageproxy/imageproxy_test.go index 4cc961d21d..eb2ec902c3 100644 --- a/services/imageproxy/imageproxy_test.go +++ b/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)) }) } }