🐛 Fix invalid image URLs not being cached and causing timeouts (#8986)

refs #8868

* 📐  Use request util in image-size
- swapped the usage of `got` for requests with the request util

* 💄  Use catch predicates
- Uses catch predicates instead of conditionals in `getImageSizeFromUrl`
- Return `NotFoundError` if applicable in `getImageSizeFromFilePath` as the caller function `cachedImageSizeFromUrl` is differentiating those between this error and others.

* 🐛  Fixed ImageObject URL & simplify no protocol URL logic

- Using `ImageObject` as a global var resulted in having the `url` property being the same for all requests coming in.
- The logic that checked for an existing protocol (e. g. gravatar URLs) was overly complicated. Refactored it to be more simple.
- Passing the correct value to `fetchDimensionsFromBuffer` as the population of `imageObject.url` happens there. These are used in our structured data and need to be full URLs (in case of locally stored files) or the original URL (in case of URLs missing the protocol)
- Added two more debug logs in `getCachedImageSizeFromUrl` so it's logged when an image is added to the cache even tho it was returned as error.

* 👀  Differentiate error codes between request and storage

* 🔥  Remove not needed `Promise.resolve()`

We're always resolving the result in `getCachedImageSizeFromUrl`, so there's no need to return the values with a `Promise.resolve()`. The caller fn uses waits for the Promises to be fulfilled.

* ☂️  Wrap already rejected predicate errors in catch all

* Use errorDetails instead of context

* ☂️  Support /assets/ image paths

- adds a guard that checks the image URL for `/assets/` in the beginning and passes a completed URL to the request util to try and fetch the image size
- adds tests

core/server/utils/cached-image-size-from-url.js

@@ -1,5 +1,4 @@
 var debug = require('ghost-ignition').debug('utils:image-size-cache'),
-    Promise = require('bluebird'),
     imageSize = require('./image-size'),
     logging = require('../logging'),
     errors = require('../errors'),
@@ -25,20 +24,26 @@ function getCachedImageSizeFromUrl(url) {
 
             debug('Cached image:', url);
 
-            return Promise.resolve(imageSizeCache[url]);
+            return imageSizeCache[url];
         }).catch(errors.NotFoundError, function () {
+            debug('Cached image (not found):', url);
             // in case of error we just attach the url
-            return Promise.resolve(imageSizeCache[url] = url);
+            imageSizeCache[url] = url;
+
+            return imageSizeCache[url];
         }).catch(function (err) {
+            debug('Cached image (error):', url);
             logging.error(err);
 
             // in case of error we just attach the url
-            return Promise.resolve(imageSizeCache[url] = url);
+            imageSizeCache[url] = url;
+
+            return imageSizeCache[url];
         });
     }
     debug('Read image from cache:', url);
     // returns image size from cache
-    return Promise.resolve(imageSizeCache[url]);
+    return imageSizeCache[url];
 }
 
 module.exports = getCachedImageSizeFromUrl;

core/server/utils/image-size.js

@@ -2,15 +2,13 @@ var debug = require('ghost-ignition').debug('utils:image-size'),
     sizeOf = require('image-size'),
     url = require('url'),
     Promise = require('bluebird'),
-    got = require('got'),
+    request = require('../utils/request'),
     utils = require('../utils'),
     errors = require('../errors'),
     config = require('../config'),
     storage = require('../adapters/storage'),
     _ = require('lodash'),
     storageUtils = require('../adapters/storage/utils'),
-    dimensions,
-    imageObject = {},
     getImageSizeFromUrl,
     getImageSizeFromFilePath;
 
@@ -22,7 +20,11 @@ var debug = require('ghost-ignition').debug('utils:image-size'),
 function isLocalImage(imagePath) {
     imagePath = utils.url.urlFor('image', {image: imagePath}, true);
 
-    return imagePath.match(new RegExp('^' + utils.url.urlJoin(utils.url.urlFor('home', true), utils.url.getSubdir(), '/', utils.url.STATIC_IMAGE_URL_PREFIX)));
+    if (imagePath) {
+        return imagePath.match(new RegExp('^' + utils.url.urlJoin(utils.url.urlFor('home', true), utils.url.getSubdir(), '/', utils.url.STATIC_IMAGE_URL_PREFIX)));
+    } else {
+        return false;
+    }
 }
 
 /**
@@ -32,7 +34,11 @@ function isLocalImage(imagePath) {
  */
 function fetchDimensionsFromBuffer(options) {
     var buffer = options.buffer,
-        imagePath = options.imagePath;
+        imagePath = options.imagePath,
+        imageObject = {},
+        dimensions;
+
+    imageObject.url = imagePath;
 
     try {
         // Using the Buffer rather than an URL requires to use sizeOf synchronously.
@@ -84,25 +90,29 @@ function fetchDimensionsFromBuffer(options) {
  */
 getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) {
     var requestOptions,
+        parsedUrl,
         timeout = config.get('times:getImageSizeTimeoutInMS') || 10000;
 
-    imageObject.url = imagePath;
-
     if (isLocalImage(imagePath)) {
         // don't make a request for a locally stored image
-        return Promise.resolve(getImageSizeFromFilePath(imagePath));
+        return getImageSizeFromFilePath(imagePath);
+    }
+
+    // CASE: pre 1.0 users were able to use an asset path for their blog logo
+    if (imagePath.match(/^\/assets/)) {
+        imagePath = utils.url.urlJoin(utils.url.urlFor('home', true), utils.url.getSubdir(), '/', imagePath);
     }
 
+    parsedUrl = url.parse(imagePath);
+
     // check if we got an url without any protocol
-    if (imagePath.indexOf('http') === -1) {
-        // our gravatar urls start with '//' in that case add 'http:'
-        if (imagePath.indexOf('//') === 0) {
-            // it's a gravatar url
-            imagePath = 'http:' + imagePath;
-        }
+    if (!parsedUrl.protocol) {
+        // CASE: our gravatar URLs start with '//' and we need to add 'http:'
+        // to make the request work
+        imagePath = 'http:' + imagePath;
     }
 
-    imagePath = url.parse(imagePath);
+    debug('requested imagePath:', imagePath);
     requestOptions = {
         headers: {
             'User-Agent': 'Mozilla/5.0'
@@ -111,39 +121,49 @@ getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) {
         encoding: null
     };
 
-    return got(
+    return request(
         imagePath,
         requestOptions
     ).then(function (response) {
-        debug('Image fetched (URL):', imagePath.href);
+        debug('Image fetched (URL):', imagePath);
 
         return fetchDimensionsFromBuffer({
             buffer: response.body,
-            imagePath: imagePath.href
+            // we need to return the URL that's accessible for network requests as this imagePath
+            // value will be used as the URL for structured data
+            imagePath: parsedUrl.href
         });
+    }).catch({code: 'URL_MISSING_INVALID'}, function (err) {
+        return Promise.reject(new errors.InternalServerError({
+            message: err.message,
+            code: 'IMAGE_SIZE_URL',
+            statusCode: err.statusCode,
+            context: err.url || imagePath
+        }));
+    }).catch({code: 'ETIMEDOUT'}, {statusCode: 408}, function (err) {
+        return Promise.reject(new errors.InternalServerError({
+            message: 'Request timed out.',
+            code: 'IMAGE_SIZE_URL',
+            statusCode: err.statusCode,
+            context: err.url || imagePath
+        }));
+    }).catch({code: 'ENOENT'}, {statusCode: 404}, function (err) {
+        return Promise.reject(new errors.NotFoundError({
+            message: 'Image not found.',
+            code: 'IMAGE_SIZE_URL',
+            statusCode: err.statusCode,
+            context: err.url || imagePath
+        }));
     }).catch(function (err) {
-        if (err.statusCode === 404) {
-            return Promise.reject(new errors.NotFoundError({
-                message: 'Image not found.',
-                code: 'IMAGE_SIZE',
-                statusCode: err.statusCode,
-                context: err.url || imagePath.href || imagePath
-            }));
-        } else if (err.code === 'ETIMEDOUT') {
-            return Promise.reject(new errors.InternalServerError({
-                message: 'Request timed out.',
-                code: 'IMAGE_SIZE',
-                statusCode: err.statusCode,
-                context: err.url || imagePath.href || imagePath
-            }));
-        } else {
-            return Promise.reject(new errors.InternalServerError({
-                message: 'Unknown Request error.',
-                code: 'IMAGE_SIZE',
-                statusCode: err.statusCode,
-                context: err.url || imagePath.href || imagePath
-            }));
+        if (err instanceof errors.GhostError) {
+            return Promise.reject(err);
         }
+        return Promise.reject(new errors.InternalServerError({
+            message: 'Unknown Request error.',
+            code: 'IMAGE_SIZE_URL',
+            statusCode: err.statusCode,
+            context: err.url || imagePath
+        }));
     });
 };
 
@@ -165,27 +185,48 @@ getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) {
  * @returns {object} imageObject or error
  */
 getImageSizeFromFilePath = function getImageSizeFromFilePath(imagePath) {
+    var filePath;
+
     imagePath = utils.url.urlFor('image', {image: imagePath}, true);
-    imageObject.url = imagePath;
 
-    imagePath = storageUtils.getLocalFileStoragePath(imagePath);
+    // get the storage readable filePath
+    filePath = storageUtils.getLocalFileStoragePath(imagePath);
 
     return storage.getStorage()
-        .read({path: imagePath})
+        .read({path: filePath})
         .then(function readFile(buf) {
-            debug('Image fetched (storage):', imagePath);
+            debug('Image fetched (storage):', filePath);
 
             return fetchDimensionsFromBuffer({
                 buffer: buf,
+                // we need to return the URL that's accessible for network requests as this imagePath
+                // value will be used as the URL for structured data
                 imagePath: imagePath
             });
-        })
-        .catch(function (err) {
+        }).catch({code: 'ENOENT'}, function (err) {
+            return Promise.reject(new errors.NotFoundError({
+                message: err.message,
+                code: 'IMAGE_SIZE_STORAGE',
+                err: err,
+                context: filePath,
+                errorDetails: {
+                    originalPath: imagePath,
+                    reqFilePath: filePath
+                }
+            }));
+        }).catch(function (err) {
+            if (err instanceof errors.GhostError) {
+                return Promise.reject(err);
+            }
             return Promise.reject(new errors.InternalServerError({
                 message: err.message,
-                code: 'IMAGE_SIZE',
+                code: 'IMAGE_SIZE_STORAGE',
                 err: err,
-                context: imagePath
+                context: filePath,
+                errorDetails: {
+                    originalPath: imagePath,
+                    reqFilePath: filePath
+                }
             }));
         });
 };

core/test/unit/utils/cached-image-size-from-url_spec.js

@@ -22,7 +22,9 @@ describe('getCachedImageSizeFromUrl', function () {
     });
 
     it('should read from cache, if dimensions for image are fetched already', function (done) {
-        var url = 'http://mysite.com/content/image/mypostcoverimage.jpg';
+        var url = 'http://mysite.com/content/image/mypostcoverimage.jpg',
+            cachedImagedSizeResult,
+            imageSizeSpy;
 
         sizeOfStub.returns(new Promise.resolve({
             width: 50,
@@ -32,7 +34,10 @@ describe('getCachedImageSizeFromUrl', function () {
 
         getCachedImageSizeFromUrl.__set__('imageSize.getImageSizeFromUrl', sizeOfStub);
 
-        getCachedImageSizeFromUrl(url).then(function () {
+        imageSizeSpy = getCachedImageSizeFromUrl.__get__('imageSize.getImageSizeFromUrl');
+
+        cachedImagedSizeResult = Promise.resolve(getCachedImageSizeFromUrl(url));
+        cachedImagedSizeResult.then(function () {
             // first call to get result from `getImageSizeFromUrl`
             cachedImagedSize = getCachedImageSizeFromUrl.__get__('imageSizeCache');
             should.exist(cachedImagedSize);
@@ -41,9 +46,13 @@ describe('getCachedImageSizeFromUrl', function () {
             cachedImagedSize[url].width.should.be.equal(50);
             should.exist(cachedImagedSize[url].height);
             cachedImagedSize[url].height.should.be.equal(50);
+
             // second call to check if values get returned from cache
-            getCachedImageSizeFromUrl(url).then(function () {
+            cachedImagedSizeResult = Promise.resolve(getCachedImageSizeFromUrl(url));
+            cachedImagedSizeResult.then(function () {
                 cachedImagedSize = getCachedImageSizeFromUrl.__get__('imageSizeCache');
+                imageSizeSpy.calledOnce.should.be.true();
+                imageSizeSpy.calledTwice.should.be.false();
                 should.exist(cachedImagedSize);
                 cachedImagedSize.should.have.property(url);
                 should.exist(cachedImagedSize[url].width);
@@ -57,14 +66,15 @@ describe('getCachedImageSizeFromUrl', function () {
     });
 
     it('can handle image-size errors', function (done) {
-        var url = 'http://mysite.com/content/image/mypostcoverimage.jpg';
+        var url = 'http://mysite.com/content/image/mypostcoverimage.jpg',
+            cachedImagedSizeResult;
 
         sizeOfStub.returns(new Promise.reject('error'));
 
         getCachedImageSizeFromUrl.__set__('imageSize.getImageSizeFromUrl', sizeOfStub);
 
-        getCachedImageSizeFromUrl(url)
-            .then(function () {
+        cachedImagedSizeResult = Promise.resolve(getCachedImageSizeFromUrl(url));
+        cachedImagedSizeResult.then(function () {
                 cachedImagedSize = getCachedImageSizeFromUrl.__get__('imageSizeCache');
                 should.exist(cachedImagedSize);
                 cachedImagedSize.should.have.property(url);

core/test/unit/utils/image-size_spec.js

@@ -139,6 +139,45 @@ describe('Image Size', function () {
             }).catch(done);
         });
 
+        it('[success] should return image dimensions asset path images', function (done) {
+            var url = '/assets/img/logo.png?v=d30c3d1e41',
+                urlForStub,
+                urlGetSubdirStub,
+                expectedImageObject =
+                {
+                    height: 100,
+                    url: 'http://myblog.com/assets/img/logo.png?v=d30c3d1e41',
+                    width: 100
+                };
+
+            urlForStub = sandbox.stub(utils.url, 'urlFor');
+            urlForStub.withArgs('home').returns('http://myblog.com/');
+            urlGetSubdirStub = sandbox.stub(utils.url, 'getSubdir');
+            urlGetSubdirStub.returns('');
+
+            requestMock = nock('http://myblog.com')
+                .get('/assets/img/logo.png?v=d30c3d1e41')
+                .reply(200, {
+                    body: '<Buffer 2c be a4 40 f7 87 73 1e 57 2c c1 e4 0d 79 03 95 42 f0 42 2e 41 95 27 c9 5c 35 a7 71 2c 09 5a 57 d3 04 1e 83 03 28 07 96 b0 c8 88 65 07 7a d1 d6 63 50>'
+                });
+
+            sizeOfStub = sandbox.stub();
+            sizeOfStub.returns({width: 100, height: 100, type: 'svg'});
+            imageSize.__set__('sizeOf', sizeOfStub);
+
+            result = imageSize.getImageSizeFromUrl(url).then(function (res) {
+                requestMock.isDone().should.be.true();
+                should.exist(res);
+                should.exist(res.width);
+                res.width.should.be.equal(expectedImageObject.width);
+                should.exist(res.height);
+                res.height.should.be.equal(expectedImageObject.height);
+                should.exist(res.url);
+                res.url.should.be.equal(expectedImageObject.url);
+                done();
+            }).catch(done);
+        });
+
         it('[success] should return image dimensions for gravatar images request', function (done) {
             var url = '//www.gravatar.com/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=mm&r=x',
                 expectedImageObject =