Preserving external images (issue #399) (#447)

* added passing userID to formatters

* added images preserver

* pinned version of golangci-lint

* implemented caching in proxy

* added tests for caching proxy

* removed previous implementation

* removed passing userID into formatter

This reverts commit 05014a5b24190836958377fcd7304294ed02f061.

* made linter happy

* removed storing image extension for FS store

* fixed bug with context cancelation preventing reading downloaded image

* fixed bug with trusting return value and ignorring error

* updated README

* added nolint for sha1

* removed unused resized flag

* added backward compatibility for config

* added handling deprecated cli parameter

* corrected func comment

* made linter happy

* made linter happy

.golangci.yml

@@ -0,0 +1,2 @@
+service:
+  golangci-lint-version: 1.19.1

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               |
 </details>
 
 ##### Required parameters

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"`
+	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"`
-	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"`
+	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) }

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

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)
 	}
 
-	imgs, err := p.extract(commentHTML)
+	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)
+	}
+
+	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
 	}
+	if p.CacheExternal {
+		imgReader, _, err = p.ImageService.Load(imgID)
+		if err != nil {
+			imgReader = nil
+		}
+	}
+	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 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 := resp.Body.Close(); e != nil {
-			log.Printf("[WARN] can't close body, %s", e)
+		if e := imgReader.Close(); e != nil {
+			log.Printf("[WARN] can't close image reader, %s", e)
 		}
 	}()
 
-	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 strings.EqualFold(k, "Content-Length") {
-			w.Header().Set(k, v[0])
-		}
-	}
 	// 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)
-	}
-}
 
-// 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))
+	w.Header().Add("Content-Type", "image/*")
+	_, err = io.Copy(w, imgReader)
 	if err != nil {
-		return nil, errors.Wrap(err, "can't create document")
+		log.Printf("[WARN] can't copy image stream, %s", 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)
-			}
+}
+
+// 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 {
+		log.Printf("[WARN] unable to save image to the storage: %+v", err)
+	}
+	// 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
-}
-
-// 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)
+	if err != nil {
+		log.Print(err.Error())
+		return nil, err
 	}
 
-	return commentHTML
+	if resp.StatusCode != http.StatusOK {
+		return nil, errors.Errorf("got unsuccessful response status %d while fetching %s", resp.StatusCode, imgURL)
+	}
+
+	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
+}
+
+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/<sha1-of-image-url-hostname>-<sha1-of-image-entire-url>"
+// <sha1-of-image-url-hostname> - would allow us to identify all images from particular site if ever needed
+// <sha1-of-image-entire-url> - 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
 }

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(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`,
 		[]string{"http://radio-t.com/img3.png", "http://images.pexels.com/67636/img4.jpeg"})
 	assert.Equal(t, `<img src="/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, 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(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
 	assert.Equal(t, `<img src="/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, r)
 
 	r = img.Convert(`<img src="https://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
 	assert.Equal(t, `<img src="https://radio-t.com/img3.png"/> xyz <img src="/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, r)
 
-	img = Image{Enabled: true, RoutePath: "/img", RemarkURL: "http://example.com"}
+	img = Image{HTTP2HTTPS: true, RoutePath: "/img", RemarkURL: "http://example.com"}
 	r = img.Convert(`<img src="http://radio-t.com/img3.png"/> xyz`)
 	assert.Equal(t, `<img src="http://radio-t.com/img3.png"/> xyz`, r, "http:// remark url, no proxy")
 
-	img = Image{Enabled: false, RoutePath: "/img"}
+	img = Image{HTTP2HTTPS: false, RoutePath: "/img"}
 	r = img.Convert(`<img src="http://radio-t.com/img3.png"/> xyz`)
 	assert.Equal(t, `<img src="http://radio-t.com/img3.png"/> 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(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
+	assert.Equal(t, `<img src="https://remark42.com/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="https://remark42.com/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, r)
+
+	r = img.Convert(`<img src="https://radio-t.com/img3.png"/> xyz <img src="https://images.pexels.com/67636/img4.jpeg">`)
+	assert.Equal(t, `<img src="https://remark42.com/img?src=aHR0cHM6Ly9yYWRpby10LmNvbS9pbWczLnBuZw=="/> xyz <img src="https://remark42.com/img?src=aHR0cHM6Ly9pbWFnZXMucGV4ZWxzLmNvbS82NzYzNi9pbWc0LmpwZWc=">`, r)
+
+	r = img.Convert(`<img src="https://remark42.com/pictures/1.png"/>`)
+	assert.Equal(t, `<img src="https://remark42.com/pictures/1.png"/>`, r)
+
+	img = Image{CacheExternal: false, RoutePath: "/img", RemarkURL: "https://remark42.com"}
+	r = img.Convert(`<img src="http://radio-t.com/img3.png"/>`)
+	assert.Equal(t, `<img src="http://radio-t.com/img3.png"/>`, r)
+
+	// both Caching and Proxy are enabled
+	img = Image{CacheExternal: true, HTTP2HTTPS: true, RoutePath: "/img", RemarkURL: "https://remark42.com"}
+	r = img.Convert(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
+	assert.Equal(t, `<img src="https://remark42.com/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="https://remark42.com/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, 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" {

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 {

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)

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) {

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

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()

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)