diff --git a/README.md b/README.md index 9c60d6e8..89bbee6e 100644 --- a/README.md +++ b/README.md @@ -879,5 +879,6 @@ _all admin calls require auth and admin privilege_ * User can edit comments in 5 mins (configurable) window after creation. * User ID hashed and prefixed by oauth provider name to avoid collisions and potential abuse. * All avatars resized and cached locally to prevent rate limiters from oauth providers, part of [go-pkgz/auth](https://github.com/go-pkgz/auth) functionality. -* Images can be proxied (`IMG_PROXY=true`) to prevent mixed http/https. +* Images can be proxied (`IMAGE_PROXY_HTTP2HTTPS=true`) to prevent mixed http/https. +* All images can be proxied and saved (`IMAGE_PROXY_CACHE_EXTERNAL=true`) instead of serving from original location. Beware, images which are posted with this parameter enabled will be served from proxy even after it will be disabled. * Docker build uses [publicly available](https://github.com/umputun/baseimage) base images. diff --git a/backend/app/rest/proxy/image.go b/backend/app/rest/proxy/image.go index 8f85b915..9029490b 100644 --- a/backend/app/rest/proxy/image.go +++ b/backend/app/rest/proxy/image.go @@ -84,15 +84,6 @@ func (p Image) replace(commentHTML string, imgs []string) string { // Handler returns http handler respond to proxied request func (p Image) Handler(w http.ResponseWriter, r *http.Request) { - if !p.HTTP2HTTPS && !p.CacheExternal { - // TODO: we might need to find a better way to handle it. If admin enables caching/proxy and disables it later on - // all comments that got converted will lose their images. We can't just return a redirect (it will open an ability - // to redirect anywhere). We can probably continue proxying these images (but need to make sure this behavior is - // documented) or, better, provide a way to migrate back converted comments. - http.Error(w, "none of the proxy features are enabled", http.StatusNotImplemented) - return - } - src, err := base64.URLEncoding.DecodeString(r.URL.Query().Get("src")) if err != nil { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't decode image url", rest.ErrDecode) @@ -106,9 +97,8 @@ func (p Image) Handler(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't parse image url "+imgURL, rest.ErrAssetNotFound) return } - if p.CacheExternal { - img, _ = p.ImageService.Load(imgID) - } + // try to load from cache for case it was saved when CacheExternal was enabled + img, _ = p.ImageService.Load(imgID) if img == nil { img, err = p.downloadImage(context.Background(), imgURL) if err != nil { diff --git a/backend/app/rest/proxy/image_test.go b/backend/app/rest/proxy/image_test.go index 919d3461..439da549 100644 --- a/backend/app/rest/proxy/image_test.go +++ b/backend/app/rest/proxy/image_test.go @@ -96,7 +96,13 @@ func TestImage_Replace(t *testing.T) { } func TestImage_Routes(t *testing.T) { - img := Image{HTTP2HTTPS: true, RemarkURL: "https://demo.remark42.com", RoutePath: "/api/v1/proxy"} + imageStore := image.MockStore{} + img := Image{ + HTTP2HTTPS: true, + RemarkURL: "https://demo.remark42.com", + RoutePath: "/api/v1/proxy", + ImageService: image.NewService(&imageStore, image.ServiceParams{}), + } ts := httptest.NewServer(http.HandlerFunc(img.Handler)) defer ts.Close() @@ -105,21 +111,50 @@ func TestImage_Routes(t *testing.T) { encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img1.png")) + // no image supposed to be cached + imageStore.On("Load", mock.Anything).Times(2).Return(nil, nil) + resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) require.NoError(t, err) - assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, "1462", resp.Header["Content-Length"][0]) assert.Equal(t, "image/png", resp.Header["Content-Type"][0]) encodedImgURL = base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/no-such-image.png")) resp, err = http.Get(ts.URL + "/?src=" + encodedImgURL) require.NoError(t, err) - assert.Equal(t, 404, resp.StatusCode) + assert.Equal(t, http.StatusNotFound, resp.StatusCode) encodedImgURL = base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "bad encoding")) resp, err = http.Get(ts.URL + "/?src=" + encodedImgURL) require.NoError(t, err) - assert.Equal(t, 400, resp.StatusCode) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) +} + +func TestImage_DisabledCachingAndHTTP2HTTPS(t *testing.T) { + imageStore := image.MockStore{} + img := Image{ + RemarkURL: "https://demo.remark42.com", + RoutePath: "/api/v1/proxy", + ImageService: image.NewService(&imageStore, image.ServiceParams{}), + } + + ts := httptest.NewServer(http.HandlerFunc(img.Handler)) + defer ts.Close() + httpSrv := imgHTTPTestsServer(t) + defer httpSrv.Close() + + encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img1.png")) + + imageStore.On("Load", mock.Anything).Once().Return(nil, nil) + + resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, "1462", resp.Header["Content-Length"][0]) + assert.Equal(t, "image/png", resp.Header["Content-Type"][0]) + + imageStore.AssertCalled(t, "Load", mock.Anything) } func TestImage_RoutesCachingImage(t *testing.T) { @@ -145,7 +180,7 @@ func TestImage_RoutesCachingImage(t *testing.T) { resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) require.Nil(t, err) - assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, "1462", resp.Header["Content-Length"][0]) assert.Equal(t, "image/png", resp.Header["Content-Type"][0]) @@ -176,7 +211,7 @@ func TestImage_RoutesUsingCachedImage(t *testing.T) { resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) require.Nil(t, err) - assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, "256", resp.Header["Content-Length"][0]) assert.Equal(t, "text/plain; charset=utf-8", resp.Header["Content-Type"][0], "if you save text you receive text/plain in response, that's only fair option you got") @@ -185,7 +220,14 @@ func TestImage_RoutesUsingCachedImage(t *testing.T) { } func TestImage_RoutesTimedOut(t *testing.T) { - img := Image{HTTP2HTTPS: true, RemarkURL: "https://demo.remark42.com", RoutePath: "/api/v1/proxy", Timeout: 50 * time.Millisecond} + imageStore := image.MockStore{} + img := Image{ + HTTP2HTTPS: true, + RemarkURL: "https://demo.remark42.com", + RoutePath: "/api/v1/proxy", + Timeout: 50 * time.Millisecond, + ImageService: image.NewService(&imageStore, image.ServiceParams{}), + } ts := httptest.NewServer(http.HandlerFunc(img.Handler)) defer ts.Close() @@ -193,9 +235,13 @@ func TestImage_RoutesTimedOut(t *testing.T) { defer httpSrv.Close() encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img-slow.png")) + + // no image supposed to be cached + imageStore.On("Load", mock.Anything).Once().Return(nil, nil) + resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) require.NoError(t, err) - assert.Equal(t, 404, resp.StatusCode) + assert.Equal(t, http.StatusNotFound, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) t.Log(string(b))