diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000000..f8f28176e1 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,2 @@ +service: + golangci-lint-version: 1.19.1 \ No newline at end of file diff --git a/README.md b/README.md index 01700ff2b0..afa54efab2 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,8 @@ _this is the recommended way to run remark42_ | restricted-words | RESTRICTED_WORDS | | words banned in comments (can use `*`), _multi_ | | edit-time | EDIT_TIME | `5m` | edit window | | read-age | READONLY_AGE | | read-only age of comments, days | -| img-proxy | IMG_PROXY | `false` | enable http->https proxy for images | +| image-proxy.http2https | | `false` | enable http->https proxy for images |≈ +| imgage-proxy.cache-external | IMAGE_PROXY_CACHE_EXTERNAL | `false` | enable caching external images to current image storage | | emoji | EMOJI | `false` | enable emoji support | | simple-view | SIMPLE_VIEW | `false` | minimized UI with basic info only | | port | REMARK_PORT | `8080` | web server port | @@ -204,6 +205,7 @@ _this is the recommended way to run remark42_ | auth.email.passwd | smtp.password | AUTH_EMAIL_PASSWD | SMTP_PASSWORD | | smtp password | 1.5.0 | | auth.email.tls | smtp.tls | AUTH_EMAIL_TLS | SMTP_TLS | `false` | enable TLS | 1.5.0 | | auth.email.timeout | smtp.timeout | AUTH_EMAIL_TIMEOUT | SMTP_TIMEOUT | `10s` | smtp timeout | 1.5.0 | +| img-proxy | image-proxy.http2https | IMG_PROXY | IMAGE_PROXY_HTTP2HTTPS | `false` | enable http->https proxy for images | 1.5.0 | ##### Required parameters diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 091fa3037f..2e12601739 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -42,37 +42,38 @@ import ( // ServerCommand with command line flags and env type ServerCommand struct { - Store StoreGroup `group:"store" namespace:"store" env-namespace:"STORE"` - Avatar AvatarGroup `group:"avatar" namespace:"avatar" env-namespace:"AVATAR"` - Cache CacheGroup `group:"cache" namespace:"cache" env-namespace:"CACHE"` - Admin AdminGroup `group:"admin" namespace:"admin" env-namespace:"ADMIN"` - Notify NotifyGroup `group:"notify" namespace:"notify" env-namespace:"NOTIFY"` - SMTP SmtpGroup `group:"smtp" namespace:"smtp" env-namespace:"SMTP"` - Image ImageGroup `group:"image" namespace:"image" env-namespace:"IMAGE"` - SSL SSLGroup `group:"ssl" namespace:"ssl" env-namespace:"SSL"` - Stream StreamGroup `group:"stream" namespace:"stream" env-namespace:"STREAM"` - - Sites []string `long:"site" env:"SITE" default:"remark" description:"site names" env-delim:","` - AnonymousVote bool `long:"anon-vote" env:"ANON_VOTE" description:"enable anonymous votes (works only with VOTES_IP enabled)"` - AdminPasswd string `long:"admin-passwd" env:"ADMIN_PASSWD" default:"" description:"admin basic auth password"` - BackupLocation string `long:"backup" env:"BACKUP_PATH" default:"./var/backup" description:"backups location"` - MaxBackupFiles int `long:"max-back" env:"MAX_BACKUP_FILES" default:"10" description:"max backups to keep"` - ImageProxy bool `long:"img-proxy" env:"IMG_PROXY" description:"enable image proxy"` - MaxCommentSize int `long:"max-comment" env:"MAX_COMMENT_SIZE" default:"2048" description:"max comment size"` - MaxVotes int `long:"max-votes" env:"MAX_VOTES" default:"-1" description:"maximum number of votes per comment"` - RestrictVoteIP bool `long:"votes-ip" env:"VOTES_IP" description:"restrict votes from the same ip"` - DurationVoteIP time.Duration `long:"votes-ip-time" env:"VOTES_IP_TIME" default:"5m" description:"same ip vote duration"` - LowScore int `long:"low-score" env:"LOW_SCORE" default:"-5" description:"low score threshold"` - CriticalScore int `long:"critical-score" env:"CRITICAL_SCORE" default:"-10" description:"critical score threshold"` - PositiveScore bool `long:"positive-score" env:"POSITIVE_SCORE" description:"enable positive score only"` - ReadOnlyAge int `long:"read-age" env:"READONLY_AGE" default:"0" description:"read-only age of comments, days"` - EditDuration time.Duration `long:"edit-time" env:"EDIT_TIME" default:"5m" description:"edit window"` - Port int `long:"port" env:"REMARK_PORT" default:"8080" description:"port"` - WebRoot string `long:"web-root" env:"REMARK_WEB_ROOT" default:"./web" description:"web root directory"` - UpdateLimit float64 `long:"update-limit" env:"UPDATE_LIMIT" default:"0.5" description:"updates/sec limit"` - RestrictedWords []string `long:"restricted-words" env:"RESTRICTED_WORDS" description:"words prohibited to use in comments" env-delim:","` - EnableEmoji bool `long:"emoji" env:"EMOJI" description:"enable emoji"` - SimpleView bool `long:"simpler-view" env:"SIMPLE_VIEW" description:"minimal comment editor mode"` + Store StoreGroup `group:"store" namespace:"store" env-namespace:"STORE"` + Avatar AvatarGroup `group:"avatar" namespace:"avatar" env-namespace:"AVATAR"` + Cache CacheGroup `group:"cache" namespace:"cache" env-namespace:"CACHE"` + Admin AdminGroup `group:"admin" namespace:"admin" env-namespace:"ADMIN"` + Notify NotifyGroup `group:"notify" namespace:"notify" env-namespace:"NOTIFY"` + SMTP SmtpGroup `group:"smtp" namespace:"smtp" env-namespace:"SMTP"` + Image ImageGroup `group:"image" namespace:"image" env-namespace:"IMAGE"` + SSL SSLGroup `group:"ssl" namespace:"ssl" env-namespace:"SSL"` + Stream StreamGroup `group:"stream" namespace:"stream" env-namespace:"STREAM"` + ImageProxy ImageProxyGroup `group:"image-proxy" namespace:"image-proxy" env-namespace:"IMAGE_PROXY"` + + Sites []string `long:"site" env:"SITE" default:"remark" description:"site names" env-delim:","` + AnonymousVote bool `long:"anon-vote" env:"ANON_VOTE" description:"enable anonymous votes (works only with VOTES_IP enabled)"` + AdminPasswd string `long:"admin-passwd" env:"ADMIN_PASSWD" default:"" description:"admin basic auth password"` + BackupLocation string `long:"backup" env:"BACKUP_PATH" default:"./var/backup" description:"backups location"` + MaxBackupFiles int `long:"max-back" env:"MAX_BACKUP_FILES" default:"10" description:"max backups to keep"` + LegacyImageProxy bool `long:"img-proxy" env:"IMG_PROXY" description:"[deprecated, use image-proxy.http2https] enable image proxy"` + MaxCommentSize int `long:"max-comment" env:"MAX_COMMENT_SIZE" default:"2048" description:"max comment size"` + MaxVotes int `long:"max-votes" env:"MAX_VOTES" default:"-1" description:"maximum number of votes per comment"` + RestrictVoteIP bool `long:"votes-ip" env:"VOTES_IP" description:"restrict votes from the same ip"` + DurationVoteIP time.Duration `long:"votes-ip-time" env:"VOTES_IP_TIME" default:"5m" description:"same ip vote duration"` + LowScore int `long:"low-score" env:"LOW_SCORE" default:"-5" description:"low score threshold"` + CriticalScore int `long:"critical-score" env:"CRITICAL_SCORE" default:"-10" description:"critical score threshold"` + PositiveScore bool `long:"positive-score" env:"POSITIVE_SCORE" description:"enable positive score only"` + ReadOnlyAge int `long:"read-age" env:"READONLY_AGE" default:"0" description:"read-only age of comments, days"` + EditDuration time.Duration `long:"edit-time" env:"EDIT_TIME" default:"5m" description:"edit window"` + Port int `long:"port" env:"REMARK_PORT" default:"8080" description:"port"` + WebRoot string `long:"web-root" env:"REMARK_WEB_ROOT" default:"./web" description:"web root directory"` + UpdateLimit float64 `long:"update-limit" env:"UPDATE_LIMIT" default:"0.5" description:"updates/sec limit"` + RestrictedWords []string `long:"restricted-words" env:"RESTRICTED_WORDS" description:"words prohibited to use in comments" env-delim:","` + EnableEmoji bool `long:"emoji" env:"EMOJI" description:"enable emoji"` + SimpleView bool `long:"simpler-view" env:"SIMPLE_VIEW" description:"minimal comment editor mode"` Auth struct { TTL struct { @@ -104,6 +105,11 @@ type ServerCommand struct { CommonOpts } +type ImageProxyGroup struct { + HTTP2HTTPS bool `long:"http2https" env:"HTTP2HTTPS" description:"enable HTTP->HTTPS proxy"` + CacheExternal bool `long:"cache-external" env:"CACHE_EXTERNAL" description:"enable caching for external images"` +} + // AuthGroup defines options group for auth params type AuthGroup struct { CID string `long:"cid" env:"CID" description:"OAuth client ID"` @@ -294,6 +300,10 @@ func (s *ServerCommand) HandleDeprecatedFlags() (result []DeprecatedFlag) { s.SMTP.TimeOut = s.Auth.Email.TimeOut result = append(result, DeprecatedFlag{Old: "auth.email.timeout", New: "smtp.timeout", RemoveVersion: "1.7.0"}) } + if s.LegacyImageProxy && !s.ImageProxy.HTTP2HTTPS { + s.ImageProxy.HTTP2HTTPS = s.LegacyImageProxy + result = append(result, DeprecatedFlag{Old: "img-proxy", New: "image-proxy.http2https", RemoveVersion: "1.7.0"}) + } return result } @@ -379,7 +389,13 @@ func (s *ServerCommand) newServerApp() (*serverApp, error) { emailNotifications = false // email notifications are not available in this case } - imgProxy := &proxy.Image{Enabled: s.ImageProxy, RoutePath: "/api/v1/img", RemarkURL: s.RemarkURL} + imgProxy := &proxy.Image{ + HTTP2HTTPS: s.ImageProxy.HTTP2HTTPS, + CacheExternal: s.ImageProxy.CacheExternal, + RoutePath: "/api/v1/img", + RemarkURL: s.RemarkURL, + ImageService: imageService, + } emojiFmt := store.CommentConverterFunc(func(text string) string { return text }) if s.EnableEmoji { emojiFmt = func(text string) string { return emoji.Sprint(text) } diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 9997f210ce..9ef42a932d 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -868,19 +868,19 @@ func TestRest_SavePictureCtrl(t *testing.T) { body, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) assert.Equal(t, 1462, len(body)) - assert.Equal(t, "image/png", resp.Header.Get("Content-Type")) + assert.Equal(t, "image/*", resp.Header.Get("Content-Type")) id = savePic("picture.gif") resp, err = http.Get(fmt.Sprintf("%s/api/v1/picture/%s", ts.URL, id)) require.NoError(t, err) assert.Equal(t, 200, resp.StatusCode) - assert.Equal(t, "image/gif", resp.Header.Get("Content-Type")) + assert.Equal(t, "image/*", resp.Header.Get("Content-Type")) id = savePic("picture.jpg") resp, err = http.Get(fmt.Sprintf("%s/api/v1/picture/%s", ts.URL, id)) require.NoError(t, err) assert.Equal(t, 200, resp.StatusCode) - assert.Equal(t, "image/jpeg", resp.Header.Get("Content-Type")) + assert.Equal(t, "image/*", resp.Header.Get("Content-Type")) id = savePic("picture.blah") resp, err = http.Get(fmt.Sprintf("%s/api/v1/picture/%s", ts.URL, id)) @@ -939,7 +939,6 @@ func TestRest_CreateWithPictures(t *testing.T) { m := map[string]string{} err = json.Unmarshal(body, &m) assert.NoError(t, err) - assert.Contains(t, m["id"], ".png") return m["id"] } diff --git a/backend/app/rest/proxy/image.go b/backend/app/rest/proxy/image.go index cab9dfcca9..e9c0a25323 100644 --- a/backend/app/rest/proxy/image.go +++ b/backend/app/rest/proxy/image.go @@ -1,10 +1,15 @@ package proxy import ( + "bytes" "context" + "crypto/sha1" // nolint "encoding/base64" + "fmt" "io" + "io/ioutil" "net/http" + "net/url" "strings" "time" @@ -14,36 +19,77 @@ import ( "github.com/pkg/errors" "github.com/umputun/remark/backend/app/rest" + "github.com/umputun/remark/backend/app/store/image" ) // Image extracts image src from comment's html and provides proxy for them // this is needed to keep remark42 running behind of HTTPS serve all images via https type Image struct { - RemarkURL string - RoutePath string - Enabled bool - Timeout time.Duration + RemarkURL string + RoutePath string + HTTP2HTTPS bool + CacheExternal bool + Timeout time.Duration + ImageService *image.Service } -// Convert all img src links without https to proxied links +// Convert img src links to proxied links depends on enabled options func (p Image) Convert(commentHTML string) string { - if !p.Enabled || strings.HasPrefix(p.RemarkURL, "http://") { - return commentHTML + if p.CacheExternal { + imgs, err := p.extract(commentHTML, func(img string) bool { return !strings.HasPrefix(img, p.RemarkURL) }) + if err != nil { + return commentHTML + } + commentHTML = p.replace(commentHTML, imgs) + } + + if p.HTTP2HTTPS && !strings.HasPrefix(p.RemarkURL, "http://") { + imgs, err := p.extract(commentHTML, func(img string) bool { return strings.HasPrefix(img, "http://") }) + if err != nil { + return commentHTML + } + commentHTML = p.replace(commentHTML, imgs) } - imgs, err := p.extract(commentHTML) + return commentHTML +} + +// extract gets all images matching predicate and return list of src +func (p Image) extract(commentHTML string, imgSrcPred func(string) bool) ([]string, error) { + doc, err := goquery.NewDocumentFromReader(strings.NewReader(commentHTML)) if err != nil { - return commentHTML + return nil, errors.Wrap(err, "can't create document") + } + result := []string{} + doc.Find("img").Each(func(i int, s *goquery.Selection) { + if im, ok := s.Attr("src"); ok { + if imgSrcPred(im) { + result = append(result, im) + } + } + }) + return result, nil +} + +// replace img links in commentHTML with route to proxy, base64 encoded original link +func (p Image) replace(commentHTML string, imgs []string) string { + for _, img := range imgs { + encodedImgURL := base64.URLEncoding.EncodeToString([]byte(img)) + resImgURL := p.RemarkURL + p.RoutePath + "?src=" + encodedImgURL + commentHTML = strings.Replace(commentHTML, img, resImgURL, -1) } - return p.replace(commentHTML, imgs) + return commentHTML } // Handler returns http handler respond to proxied request func (p Image) Handler(w http.ResponseWriter, r *http.Request) { - - if !p.Enabled { - http.Error(w, "proxy disabled", http.StatusNotImplemented) + 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 } @@ -53,48 +99,42 @@ func (p Image) Handler(w http.ResponseWriter, r *http.Request) { return } - timeout := 60 * time.Second // default - if p.Timeout > 0 { - timeout = p.Timeout - } - - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - client := http.Client{Timeout: 30 * time.Second} - var resp *http.Response - err = repeater.NewDefault(5, time.Second).Do(ctx, func() error { - var e error - req, e := http.NewRequest("GET", string(src), nil) - if e != nil { - return errors.Wrapf(e, "failed to make request for %s", r.URL.Query().Get("src")) - } - resp, e = client.Do(req.WithContext(ctx)) - return e - }) + imgURL := string(src) + var imgReader io.ReadCloser + imgID, err := cachedImgID(imgURL) if err != nil { - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't get image "+string(src), rest.ErrAssetNotFound) + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't parse image url "+imgURL, rest.ErrAssetNotFound) return } - defer func() { - if e := resp.Body.Close(); e != nil { - log.Printf("[WARN] can't close body, %s", e) + if p.CacheExternal { + imgReader, _, err = p.ImageService.Load(imgID) + if err != nil { + imgReader = nil } - }() - - if resp.StatusCode != http.StatusOK { - w.WriteHeader(resp.StatusCode) - return } - - for k, v := range resp.Header { - if strings.EqualFold(k, "Content-Type") { - w.Header().Set(k, v[0]) + if imgReader == nil { + imgReader, err = p.downloadImage(context.Background(), imgURL) + if err != nil { + rest.SendErrorJSON(w, r, http.StatusNotFound, err, "can't get image "+imgURL, rest.ErrAssetNotFound) + return } - if strings.EqualFold(k, "Content-Length") { - w.Header().Set(k, v[0]) + if p.CacheExternal { + var buf bytes.Buffer + // We need to duplicate data into a new buffer because `cacheImage` would read provider Reader + // and we would need another one to read data for response + p.cacheImage(io.TeeReader(imgReader, &buf), imgID) + if err := imgReader.Close(); err != nil { + log.Printf("[WARN] can't close image reader, %s", err) + } + imgReader = ioutil.NopCloser(&buf) } } + defer func() { + if e := imgReader.Close(); e != nil { + log.Printf("[WARN] can't close image reader, %s", e) + } + }() + // enforce client-side caching etag := `"` + r.URL.Query().Get("src") + `"` w.Header().Set("Etag", etag) @@ -105,36 +145,78 @@ func (p Image) Handler(w http.ResponseWriter, r *http.Request) { return } } - if _, e := io.Copy(w, resp.Body); e != nil { - log.Printf("[WARN] can't copy image stream, %s", e) + + w.Header().Add("Content-Type", "image/*") + _, err = io.Copy(w, imgReader) + if err != nil { + log.Printf("[WARN] can't copy image stream, %s", err) } } -// extract gets all non-https images and return list of src -func (p Image) extract(commentHTML string) ([]string, error) { - doc, err := goquery.NewDocumentFromReader(strings.NewReader(commentHTML)) +// cache image from provided Reader using given ID +func (p Image) cacheImage(r io.Reader, imgID string) { + id, err := p.ImageService.SaveWithID(imgID, r) if err != nil { - return nil, errors.Wrap(err, "can't create document") + log.Printf("[WARN] unable to save image to the storage: %+v", err) } - result := []string{} - doc.Find("img").Each(func(i int, s *goquery.Selection) { - if im, ok := s.Attr("src"); ok { - if strings.HasPrefix(im, "http://") { - result = append(result, im) - } + // In the future we can do something smarter than just committing everything (eg, some kind of LFU/LRU) + if err := p.ImageService.Commit(id); err != nil { + log.Printf("[WARN] unable to commit image %s", imgID) + } +} + +// download an image. Returns a Reader which has to be closed by a caller +func (p Image) downloadImage(ctx context.Context, imgURL string) (io.ReadCloser, error) { + log.Printf("[DEBUG] downloading image %s", imgURL) + + timeout := 60 * time.Second // default + if p.Timeout > 0 { + timeout = p.Timeout + } + + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + client := http.Client{Timeout: 30 * time.Second} + var resp *http.Response + err := repeater.NewDefault(5, time.Second).Do(ctx, func() error { + var e error + req, e := http.NewRequest("GET", imgURL, nil) + if e != nil { + return errors.Wrapf(e, "failed to make request for %s", imgURL) } + resp, e = client.Do(req.WithContext(ctx)) + return e }) - return result, nil -} + if err != nil { + log.Print(err.Error()) + return nil, err + } -// replace img links in commentHTML with route to proxy, base64 encoded original link -func (p Image) replace(commentHTML string, imgs []string) string { + if resp.StatusCode != http.StatusOK { + return nil, errors.Errorf("got unsuccessful response status %d while fetching %s", resp.StatusCode, imgURL) + } - for _, img := range imgs { - encodedImgURL := base64.URLEncoding.EncodeToString([]byte(img)) - resImgURL := p.RemarkURL + p.RoutePath + "?src=" + encodedImgURL - commentHTML = strings.Replace(commentHTML, img, resImgURL, -1) + imgData, err := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, errors.Errorf("unable to read image body") } + return ioutil.NopCloser(bytes.NewBuffer(imgData)), nil +} - return commentHTML +func sha1Str(s string) string { + return fmt.Sprintf("%x", sha1.Sum([]byte(s))) // nolint +} + +// generates ID for a cached image. +// ID would look like: "cached_images/-" +// - would allow us to identify all images from particular site if ever needed +// - would allow us to avoid storing duplicates of the same image +// (as accurate as deduplication based on potentially mutable url can be) +func cachedImgID(imgURL string) (string, error) { + parsedURL, err := url.Parse(imgURL) + if err != nil { + return "", errors.Wrapf(err, "can parse url %s", imgURL) + } + return fmt.Sprintf("cached_images/%s-%s", sha1Str(parsedURL.Hostname()), sha1Str(imgURL)), nil } diff --git a/backend/app/rest/proxy/image_test.go b/backend/app/rest/proxy/image_test.go index 4c4d5e0493..88351ffd01 100644 --- a/backend/app/rest/proxy/image_test.go +++ b/backend/app/rest/proxy/image_test.go @@ -1,8 +1,10 @@ package proxy import ( + "bytes" "encoding/base64" "fmt" + "io" "io/ioutil" "net/http" "net/http/httptest" @@ -12,7 +14,9 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store/image" ) func TestPicture_Extract(t *testing.T) { @@ -46,11 +50,11 @@ func TestPicture_Extract(t *testing.T) { []string{}, }, } - img := Image{Enabled: true} + img := Image{HTTP2HTTPS: true} for i, tt := range tbl { t.Run(strconv.Itoa(i), func(t *testing.T) { - res, err := img.extract(tt.inp) + res, err := img.extract(tt.inp, func(src string) bool { return strings.HasPrefix(src, "http://") }) assert.NoError(t, err) assert.Equal(t, tt.res, res) }) @@ -58,14 +62,14 @@ func TestPicture_Extract(t *testing.T) { } func TestPicture_Replace(t *testing.T) { - img := Image{Enabled: true, RoutePath: "/img"} + img := Image{HTTP2HTTPS: true, RoutePath: "/img"} r := img.replace(` xyz `, []string{"http://radio-t.com/img3.png", "http://images.pexels.com/67636/img4.jpeg"}) assert.Equal(t, ` xyz `, r) } func TestImage_Routes(t *testing.T) { - img := Image{Enabled: true, RemarkURL: "https://demo.remark42.com", RoutePath: "/api/v1/proxy"} + img := Image{HTTP2HTTPS: true, RemarkURL: "https://demo.remark42.com", RoutePath: "/api/v1/proxy"} ts := httptest.NewServer(http.HandlerFunc(img.Handler)) defer ts.Close() @@ -77,9 +81,8 @@ func TestImage_Routes(t *testing.T) { resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) require.NoError(t, err) assert.Equal(t, 200, resp.StatusCode) - t.Logf("%+v", resp.Header) assert.Equal(t, "123", resp.Header["Content-Length"][0]) - assert.Equal(t, "image/png", resp.Header["Content-Type"][0]) + assert.Equal(t, "image/*", 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) @@ -92,8 +95,69 @@ func TestImage_Routes(t *testing.T) { assert.Equal(t, 400, resp.StatusCode) } +func TestImage_Routes_CachingImage(t *testing.T) { + imageStore := image.MockStore{} + img := Image{ + CacheExternal: true, + RemarkURL: "https://demo.remark42.com", + RoutePath: "/api/v1/proxy", + ImageService: &image.Service{Store: &imageStore}, + } + + ts := httptest.NewServer(http.HandlerFunc(img.Handler)) + defer ts.Close() + httpSrv := imgHTTPServer(t) + defer httpSrv.Close() + + imgURL := httpSrv.URL + "/image/img1.png" + encodedImgURL := base64.URLEncoding.EncodeToString([]byte(imgURL)) + + imageStore.On("Load", mock.Anything).Once().Return(nil, int64(0), nil) + imageStore.On("SaveWithID", mock.Anything, mock.Anything).Once().Run(func(args mock.Arguments) { _, _ = ioutil.ReadAll(args.Get(1).(io.Reader)) }).Return("", nil) + imageStore.On("Commit", mock.Anything).Once().Return(nil) + + resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) + require.Nil(t, err) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, "123", resp.Header["Content-Length"][0]) + assert.Equal(t, "image/*", resp.Header["Content-Type"][0]) + + imageStore.AssertCalled(t, "Load", mock.Anything) + imageStore.AssertCalled(t, "SaveWithID", "cached_images/4b84b15bff6ee5796152495a230e45e3d7e947d9-"+sha1Str(imgURL), mock.Anything) + imageStore.AssertCalled(t, "Commit", mock.Anything) +} + +func TestImage_Routes_Using_Cachded_Image(t *testing.T) { + imageStore := image.MockStore{} + img := Image{ + CacheExternal: true, + RemarkURL: "https://demo.remark42.com", + RoutePath: "/api/v1/proxy", + ImageService: &image.Service{Store: &imageStore}, + } + + ts := httptest.NewServer(http.HandlerFunc(img.Handler)) + defer ts.Close() + httpSrv := imgHTTPServer(t) + defer httpSrv.Close() + + encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img1.png")) + + // In order to validate that cached data is used cache "will return" some other data from what http server would + imageReader := ioutil.NopCloser(bytes.NewReader([]byte(fmt.Sprintf("%256s", "X")))) + imageStore.On("Load", mock.Anything).Once().Return(imageReader, int64(256), nil) + + resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) + require.Nil(t, err) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, "256", resp.Header["Content-Length"][0]) + assert.Equal(t, "image/*", resp.Header["Content-Type"][0]) + + imageStore.AssertCalled(t, "Load", mock.Anything) +} + func TestImage_RoutesTimedOut(t *testing.T) { - img := Image{Enabled: true, RemarkURL: "https://demo.remark42.com", RoutePath: "/api/v1/proxy", Timeout: 50 * time.Millisecond} + img := Image{HTTP2HTTPS: true, RemarkURL: "https://demo.remark42.com", RoutePath: "/api/v1/proxy", Timeout: 50 * time.Millisecond} ts := httptest.NewServer(http.HandlerFunc(img.Handler)) defer ts.Close() @@ -103,30 +167,51 @@ func TestImage_RoutesTimedOut(t *testing.T) { encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img-slow.png")) resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL) require.NoError(t, err) - assert.Equal(t, 400, resp.StatusCode) + assert.Equal(t, 404, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) t.Log(string(b)) assert.True(t, strings.Contains(string(b), "deadline exceeded")) } -func TestPicture_Convert(t *testing.T) { - img := Image{Enabled: true, RoutePath: "/img"} +func TestPicture_Convert_ProxyMode(t *testing.T) { + img := Image{HTTP2HTTPS: true, RoutePath: "/img"} r := img.Convert(` xyz `) assert.Equal(t, ` xyz `, r) r = img.Convert(` xyz `) assert.Equal(t, ` xyz `, r) - img = Image{Enabled: true, RoutePath: "/img", RemarkURL: "http://example.com"} + img = Image{HTTP2HTTPS: true, RoutePath: "/img", RemarkURL: "http://example.com"} r = img.Convert(` xyz`) assert.Equal(t, ` xyz`, r, "http:// remark url, no proxy") - img = Image{Enabled: false, RoutePath: "/img"} + img = Image{HTTP2HTTPS: false, RoutePath: "/img"} r = img.Convert(` xyz`) assert.Equal(t, ` xyz`, r, "disabled, no proxy") } +func TestPicture_Convert_CachingMode(t *testing.T) { + img := Image{CacheExternal: true, RoutePath: "/img", RemarkURL: "https://remark42.com"} + r := img.Convert(` xyz `) + assert.Equal(t, ` xyz `, r) + + r = img.Convert(` xyz `) + assert.Equal(t, ` xyz `, r) + + r = img.Convert(``) + assert.Equal(t, ``, r) + + img = Image{CacheExternal: false, RoutePath: "/img", RemarkURL: "https://remark42.com"} + r = img.Convert(``) + assert.Equal(t, ``, r) + + // both Caching and Proxy are enabled + img = Image{CacheExternal: true, HTTP2HTTPS: true, RoutePath: "/img", RemarkURL: "https://remark42.com"} + r = img.Convert(` xyz `) + assert.Equal(t, ` xyz `, r) +} + func imgHTTPServer(t *testing.T) *httptest.Server { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/image/img1.png" { diff --git a/backend/app/store/image/bolt_store.go b/backend/app/store/image/bolt_store.go index eb049b067a..66ba72a6a1 100644 --- a/backend/app/store/image/bolt_store.go +++ b/backend/app/store/image/bolt_store.go @@ -60,16 +60,14 @@ func NewBoltStorage(fileName string, maxSize int, maxHeight int, maxWidth int, o }, nil } -// Save data from reader to staging bucket in DB -func (b *Bolt) Save(fileName string, userID string, r io.Reader) (id string, err error) { +// SaveWithID saves data from reader with given id +func (b *Bolt) SaveWithID(id string, r io.Reader) (string, error) { data, err := readAndValidateImage(r, b.MaxSize) if err != nil { - return "", errors.Wrapf(err, "can't load image %s", fileName) + return "", errors.Wrapf(err, "can't load image with ID %s", id) } - data, _ = resize(data, b.MaxWidth, b.MaxHeight) - - id = path.Join(userID, guid()) + data = resize(data, b.MaxWidth, b.MaxHeight) err = b.db.Update(func(tx *bolt.Tx) error { if err = tx.Bucket([]byte(imagesStagedBktName)).Put([]byte(id), data); err != nil { @@ -88,6 +86,12 @@ func (b *Bolt) Save(fileName string, userID string, r io.Reader) (id string, err return id, err } +// Save data from reader to staging bucket in DB +func (b *Bolt) Save(fileName string, userID string, r io.Reader) (id string, err error) { + id = path.Join(userID, guid()) + return b.SaveWithID(id, r) +} + // Commit file stored in staging bucket by copying it to permanent bucket // Data from staging bucket not removed immediately, but would be removed on cleanup func (b *Bolt) Commit(id string) error { diff --git a/backend/app/store/image/fs_store.go b/backend/app/store/image/fs_store.go index a1c218684a..b800812b27 100644 --- a/backend/app/store/image/fs_store.go +++ b/backend/app/store/image/fs_store.go @@ -36,35 +36,39 @@ type FileSystem struct { } } -// Save data from reader for given file name to local FS, staging directory. Returns id as user/uuid.ext -// Files partitioned across multiple subdirectories and the final path includes part, i.e. /location/user1/03/123-4567.png -func (f *FileSystem) Save(fileName string, userID string, r io.Reader) (id string, err error) { +// SaveWithID saves data from reader with given id +func (f *FileSystem) SaveWithID(id string, r io.Reader) (string, error) { data, err := readAndValidateImage(r, f.MaxSize) if err != nil { - return "", errors.Wrapf(err, "can't load image %s", fileName) + return "", errors.Wrapf(err, "can't load image with ID %s", id) } - data, resized := resize(data, f.MaxWidth, f.MaxHeight) - - id = path.Join(userID, guid()) + filepath.Ext(fileName) // make id as user/uuid.ext + data = resize(data, f.MaxWidth, f.MaxHeight) dst := f.location(f.Staging, id) - if resized { // resized also converted to png - id = strings.TrimSuffix(id, filepath.Ext(id)) + ".png" - dst = f.location(f.Staging, id) - } if err = os.MkdirAll(path.Dir(dst), 0700); err != nil { return "", errors.Wrap(err, "can't make image directory") } if err = ioutil.WriteFile(dst, data, 0600); err != nil { - return "", errors.Wrapf(err, "can't write image file %s", dst) + return "", errors.Wrapf(err, "can't write file") } - log.Printf("[DEBUG] file %s saved for image %s, size=%d", dst, fileName, len(data)) + log.Printf("[DEBUG] file %s saved for image %s, size=%d", dst, id, len(data)) return id, nil } +// Save data from reader for given file name to local FS, staging directory. Returns id as user/uuid +// Files partitioned across multiple subdirectories and the final path includes part, i.e. /location/user1/03/123-4567 +func (f *FileSystem) Save(fileName string, userID string, r io.Reader) (id string, err error) { + id = path.Join(userID, guid()) // make id as user/uuid + finalID, err := f.SaveWithID(id, r) + if err != nil { + err = errors.Wrapf(err, "can't save file %s", fileName) + } + return finalID, err +} + // Commit file stored in staging location by moving it to permanent location func (f *FileSystem) Commit(id string) error { log.Printf("[DEBUG] commit image %s", id) diff --git a/backend/app/store/image/fs_store_test.go b/backend/app/store/image/fs_store_test.go index e1b2f1ba5b..5af64a9bd5 100644 --- a/backend/app/store/image/fs_store_test.go +++ b/backend/app/store/image/fs_store_test.go @@ -47,7 +47,6 @@ func TestFsStore_Save(t *testing.T) { id, err := svc.Save("file1.png", "user1", gopherPNG()) assert.NoError(t, err) assert.Contains(t, id, "user1/") - assert.Contains(t, id, ".png") t.Log(id) img := svc.location(svc.Staging, id) @@ -65,7 +64,6 @@ func TestFsStore_SaveWithResize(t *testing.T) { id, err := svc.Save("file1.png", "user1", gopherPNG()) assert.NoError(t, err) assert.Contains(t, id, "user1/") - assert.Contains(t, id, ".png") t.Log(id) img := svc.location(svc.Staging, id) @@ -87,7 +85,6 @@ func TestFsStore_SaveWithResizeJpeg(t *testing.T) { id, err := svc.Save("circles.jpg", "user1", fh) assert.NoError(t, err) assert.Contains(t, id, "user1/") - assert.Contains(t, id, ".png") t.Log(id) img := svc.location(svc.Staging, id) @@ -109,7 +106,6 @@ func TestFsStore_SaveNoResizeJpeg(t *testing.T) { id, err := svc.Save("circles.jpg", "user1", fh) assert.NoError(t, err) assert.Contains(t, id, "user1/") - assert.Contains(t, id, ".jpg") t.Log(id) img := svc.location(svc.Staging, id) @@ -124,7 +120,7 @@ func TestFsStore_WrongFormat(t *testing.T) { defer teardown() _, err := svc.Save("file1.png", "user1", strings.NewReader("blah blah bad image")) - assert.EqualError(t, err, "can't load image file1.png: file format is not allowed") + assert.Error(t, err) } func TestFsStore_SaveAndCommit(t *testing.T) { diff --git a/backend/app/store/image/image.go b/backend/app/store/image/image.go index 0b9f32ce69..94a2e62950 100644 --- a/backend/app/store/image/image.go +++ b/backend/app/store/image/image.go @@ -30,6 +30,7 @@ import ( // Store defines interface for saving and loading pictures. // Declares two-stage save with commit type Store interface { + SaveWithID(id string, r io.Reader) (string, error) Save(fileName string, userID string, r io.Reader) (id string, err error) // get name and reader and returns ID of stored image Commit(id string) error // move image from staging to permanent Load(id string) (io.ReadCloser, int64, error) // load image by ID. Caller has to close the reader. @@ -139,23 +140,23 @@ func (s *Service) Close() { // resize an image of supported format (PNG, JPG, GIF) to the size of "limit" px of the // biggest side (width or height) preserving aspect ratio. // Returns original data if resizing is not needed or failed. -// If resized the result will be for png format and ok flag will be true. -func resize(data []byte, limitW, limitH int) ([]byte, bool) { +// If resized the result will be for png format +func resize(data []byte, limitW, limitH int) []byte { if data == nil || limitW <= 0 || limitH <= 0 { - return data, false + return data } src, _, err := image.Decode(bytes.NewBuffer(data)) if err != nil { log.Printf("[WARN] can't decode image, %s", err) - return data, false + return data } bounds := src.Bounds() w, h := bounds.Dx(), bounds.Dy() if w <= limitW && h <= limitH || w <= 0 || h <= 0 { log.Printf("[DEBUG] resizing image is smaller that the limit or has 0 size") - return data, false + return data } newW, newH := getProportionalSizes(w, h, limitW, limitH) @@ -165,9 +166,9 @@ func resize(data []byte, limitW, limitH int) ([]byte, bool) { var out bytes.Buffer if err = png.Encode(&out, m); err != nil { log.Printf("[WARN] can't encode resized image to png, %s", err) - return data, false + return data } - return out.Bytes(), true + return out.Bytes() } // getProportionalSizes returns width and height resized by both dimensions proportionally diff --git a/backend/app/store/image/image_mock.go b/backend/app/store/image/image_mock.go index cb4631f86a..47cda23dda 100644 --- a/backend/app/store/image/image_mock.go +++ b/backend/app/store/image/image_mock.go @@ -1,4 +1,5 @@ // Code generated by mockery v1.0.0. DO NOT EDIT. + package image import ( @@ -94,6 +95,27 @@ func (_m *MockStore) Save(fileName string, userID string, r io.Reader) (string, return r0, r1 } +// SaveWithID provides a mock function with given fields: id, r +func (_m *MockStore) SaveWithID(id string, r io.Reader) (string, error) { + ret := _m.Called(id, r) + + var r0 string + if rf, ok := ret.Get(0).(func(string, io.Reader) string); ok { + r0 = rf(id, r) + } else { + r0 = ret.Get(0).(string) + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, io.Reader) error); ok { + r1 = rf(id, r) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // SizeLimit provides a mock function with given fields: func (_m *MockStore) SizeLimit() int { ret := _m.Called() diff --git a/backend/app/store/image/image_test.go b/backend/app/store/image/image_test.go index 215768a4f8..67b70fbb43 100644 --- a/backend/app/store/image/image_test.go +++ b/backend/app/store/image/image_test.go @@ -85,21 +85,18 @@ func TestService_SubmitDelay(t *testing.T) { func TestService_resize(t *testing.T) { // Reader is nil. - resized, ok := resize(nil, 100, 100) + resized := resize(nil, 100, 100) assert.Nil(t, resized) - assert.False(t, ok) // Negative limit error. - resized, ok = resize([]byte("some picture bin data"), -1, -1) + resized = resize([]byte("some picture bin data"), -1, -1) require.NotNil(t, resized) assert.Equal(t, resized, []byte("some picture bin data")) - assert.False(t, ok) // Decode error. - resized, ok = resize([]byte("invalid image content"), 100, 100) + resized = resize([]byte("invalid image content"), 100, 100) assert.NotNil(t, resized) assert.Equal(t, resized, []byte("invalid image content")) - assert.False(t, ok) cases := []struct { file string @@ -114,15 +111,13 @@ func TestService_resize(t *testing.T) { require.NoError(t, err, "can't open test file %s", c.file) // No need for resize, image dimensions are smaller than resize limit. - resized, ok = resize(img, 800, 800) + resized = resize(img, 800, 800) assert.NotNil(t, resized, "file %s", c.file) assert.Equal(t, resized, img) - assert.False(t, ok) // Resizing to half of width. Check resized image format PNG. - resized, ok = resize(img, 400, 400) + resized = resize(img, 400, 400) assert.NotNil(t, resized, "file %s", c.file) - assert.True(t, ok) imgRz, format, err := image.Decode(bytes.NewBuffer(resized)) assert.NoError(t, err, "file %s", c.file)