From f00d43342c64e66acc15466aa85fe8534f1bf5d2 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Thu, 13 May 2021 03:05:56 +0200 Subject: [PATCH] validate image existence before post or preview --- backend/app/rest/api/rest_private.go | 14 ++++++++++++ backend/app/rest/api/rest_private_test.go | 26 ++++++++++++++++++++++- backend/app/rest/api/rest_public.go | 15 +++++++++++++ backend/app/rest/api/rest_public_test.go | 21 ++++++++++++++++++ backend/app/rest/api/rest_test.go | 6 +++++- backend/app/rest/httperrors.go | 1 + backend/app/store/image/image.go | 18 ++++++++++------ backend/app/store/image/image_test.go | 6 +++--- 8 files changed, 96 insertions(+), 11 deletions(-) diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 3caad19a2a..3db49cbb3e 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -88,6 +88,20 @@ func (s *private) createCommentCtrl(w http.ResponseWriter, r *http.Request) { } comment = s.commentFormatter.Format(comment) + // check if images are valid + imgIds, err := s.imageService.ExtractPictures(comment.Text) + if err != nil { + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't extract pictures from comment text", rest.ErrCommentValidation) + return + } + for _, id := range imgIds { + _, err = s.imageService.Load(id) + if err != nil { + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't load picture from the comment", rest.ErrImgNotFound) + return + } + } + // check if user blocked if s.dataService.IsBlocked(comment.Locator.SiteID, comment.User.ID) { rest.SendErrorJSON(w, r, http.StatusForbidden, errors.New("rejected"), "user blocked", rest.ErrUserBlocked) diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 12c5ec5fde..c338215664 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -165,6 +165,28 @@ func TestRest_CreateRejected(t *testing.T) { require.Equal(t, http.StatusForbidden, resp.StatusCode, "reject wrong aud") } +func TestRest_CreateWithWrongImage(t *testing.T) { + ts, srv, teardown := startupT(t) + defer teardown() + + // create comment + resp, err := post(t, ts.URL+"/api/v1/comment", fmt.Sprintf(`{"text": "![non-existent.jpg](%s/api/v1/picture/dev_user/bad_picture)", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, srv.RemarkURL)) + assert.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + b, err := ioutil.ReadAll(resp.Body) + assert.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + assert.Contains(t, + string(b), + "{\"code\":20,\"details\":\"can't load picture from the comment\","+ + "\"error\":\"can't get image file for dev_user/bad_picture: can't get image stats for dev_user/bad_picture: stat ", + ) + assert.Contains(t, + string(b), + "/pics-remark42/staging/dev_user/62/bad_picture: no such file or directory\"}\n", + ) +} + func TestRest_CreateWithLazyImage(t *testing.T) { ts, _, teardown := startupT(t) defer teardown() @@ -979,6 +1001,8 @@ func TestRest_CreateWithPictures(t *testing.T) { }, image.ServiceParams{ EditDuration: 100 * time.Millisecond, MaxSize: 2000, + ImageAPI: svc.RemarkURL + "/api/v1/picture/", + ProxyAPI: svc.RemarkURL + "/api/v1/img", }) defer imageService.Close(context.Background()) @@ -1022,7 +1046,7 @@ func TestRest_CreateWithPictures(t *testing.T) { ids[i] = uploadPicture(fmt.Sprintf("pic%d.png", i)) } - text := fmt.Sprintf(`text 123 ![](/api/v1/picture/%s) *xxx* ![](/api/v1/picture/%s) ![](/api/v1/picture/%s)`, ids[0], ids[1], ids[2]) + text := fmt.Sprintf(`text 123 ![](%s/api/v1/picture/%s) *xxx* ![](%s/api/v1/picture/%s) ![](%s/api/v1/picture/%s)`, svc.RemarkURL, ids[0], svc.RemarkURL, ids[1], svc.RemarkURL, ids[2]) body := fmt.Sprintf(`{"text": "%s", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`, text) resp, err := post(t, ts.URL+"/api/v1/comment", body) diff --git a/backend/app/rest/api/rest_public.go b/backend/app/rest/api/rest_public.go index 9371137460..f6c7efb0d0 100644 --- a/backend/app/rest/api/rest_public.go +++ b/backend/app/rest/api/rest_public.go @@ -132,6 +132,21 @@ func (s *public) previewCommentCtrl(w http.ResponseWriter, r *http.Request) { comment = s.commentFormatter.Format(comment) comment.Sanitize() + + // check if images are valid + imgIds, err := s.imageService.ExtractPictures(comment.Text) + if err != nil { + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't extract pictures from comment text", rest.ErrCommentValidation) + return + } + for _, id := range imgIds { + _, err = s.imageService.Load(id) + if err != nil { + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't load picture from the comment", rest.ErrImgNotFound) + return + } + } + render.HTML(w, r, comment.Text) } diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 8c1e987b4d..39326c1205 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -45,6 +45,27 @@ func TestRest_Preview(t *testing.T) { assert.Equal(t, 400, resp.StatusCode) } +func TestRest_PreviewWithWrongImage(t *testing.T) { + ts, srv, teardown := startupT(t) + defer teardown() + + resp, err := post(t, ts.URL+"/api/v1/preview", fmt.Sprintf(`{"text": "![non-existent.jpg](%s/api/v1/picture/dev_user/bad_picture)", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, srv.RemarkURL)) + assert.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + b, err := ioutil.ReadAll(resp.Body) + assert.NoError(t, err) + assert.NoError(t, resp.Body.Close()) + assert.Contains(t, + string(b), + "{\"code\":20,\"details\":\"can't load picture from the comment\","+ + "\"error\":\"can't get image file for dev_user/bad_picture: can't get image stats for dev_user/bad_picture: stat ", + ) + assert.Contains(t, + string(b), + "/pics-remark42/staging/dev_user/62/bad_picture: no such file or directory\"}\n", + ) +} + func TestRest_PreviewWithMD(t *testing.T) { ts, _, teardown := startupT(t) defer teardown() diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go index 240e340a83..6effe33d65 100644 --- a/backend/app/rest/api/rest_test.go +++ b/backend/app/rest/api/rest_test.go @@ -402,6 +402,8 @@ func startupT(t *testing.T) (ts *httptest.Server, srv *Rest, teardown func()) { RestrictedWordsMatcher: restrictedWordsMatcher, } + remarkURL := "https://demo.remark42.com" + srv = &Rest{ DataService: dataStore, Authenticator: auth.NewService(auth.Opts{ @@ -411,13 +413,15 @@ func startupT(t *testing.T) (ts *httptest.Server, srv *Rest, teardown func()) { }), Cache: memCache, WebRoot: tmp, - RemarkURL: "https://demo.remark42.com", + RemarkURL: remarkURL, ImageService: image.NewService(&image.FileSystem{ Location: tmp + "/pics-remark42", Partitions: 100, Staging: tmp + "/pics-remark42/staging", }, image.ServiceParams{ EditDuration: 100 * time.Millisecond, + ImageAPI: remarkURL + "/api/v1/picture/", + ProxyAPI: remarkURL + "/api/v1/img", MaxSize: 10000, }), ImageProxy: &proxy.Image{}, diff --git a/backend/app/rest/httperrors.go b/backend/app/rest/httperrors.go index 3a094b8e31..d5310590b8 100644 --- a/backend/app/rest/httperrors.go +++ b/backend/app/rest/httperrors.go @@ -39,6 +39,7 @@ const ( ErrActionRejected = 17 // general error for rejected actions ErrAssetNotFound = 18 // requested file not found ErrCommentRestrictWords = 19 // restricted words in a comment + ErrImgNotFound = 20 // posted image not found in the storage ) // errTmplData store data for error message diff --git a/backend/app/store/image/image.go b/backend/app/store/image/image.go index c346d4903b..0a3f196ac0 100644 --- a/backend/app/store/image/image.go +++ b/backend/app/store/image/image.go @@ -164,22 +164,28 @@ func (s *Service) ExtractPictures(commentHTML string) (ids []string, err error) } } if strings.Contains(im, s.ProxyAPI) { - proxiedURL, err := url.Parse(im) - if err != nil { + proxiedURL, e := url.Parse(im) + if e != nil { + err = errors.Wrapf(e, "problem parsing %s", im) return } - imgURL, err := base64.URLEncoding.DecodeString(proxiedURL.Query().Get("src")) - if err != nil { + imgURL, e := base64.URLEncoding.DecodeString(proxiedURL.Query().Get("src")) + if e != nil { + err = errors.Wrapf(e, "problem decodind base64 from %s", im) return } - imgID, err := CachedImgID(string(imgURL)) - if err != nil { + imgID, e := CachedImgID(string(imgURL)) + if e != nil { + err = errors.Wrapf(e, "problem retrieving cached URL for %s", im) return } ids = append(ids, imgID) } } }) + if err != nil { + return nil, err + } return ids, nil } diff --git a/backend/app/store/image/image_test.go b/backend/app/store/image/image_test.go index 04e23790fd..5a7b55e255 100644 --- a/backend/app/store/image/image_test.go +++ b/backend/app/store/image/image_test.go @@ -105,20 +105,20 @@ func TestService_ExtractPictures(t *testing.T) { // bad url html = `` ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + require.Error(t, err) require.Empty(t, ids) // bad src html = `` ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + require.Error(t, err) require.Empty(t, ids) // good src with bad content badURL := base64.URLEncoding.EncodeToString([]byte(" http://foo.bar")) html = fmt.Sprintf(``, badURL) ids, err = svc.ExtractPictures(html) - require.NoError(t, err) + require.Error(t, err) require.Empty(t, ids) }