From e626dd9353dddc46e8ef45d4201f091889601b5b Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Thu, 11 Jul 2024 09:37:44 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20image=20dimension=20retr?= =?UTF-8?q?ieval=20causing=20Ghost=20requests=20to=20hang=20(#20589)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref https://linear.app/tryghost/issue/ENG-1408/ - added additional safeguards to the image size dimensions probing For some reason that requires further investigation, the probe-image-size package was silently failing (neither resolving nor rejecting) for a particular URL. This was causing Ghost to hang on to serving the request, and after a few of these came in, ultimately caused Ghost to stop being responsive. Rather than trying to patch a dependency, we'll wrap the call to this package and use the same timeout we pass into the package (which is ignored in this particular case) as an additional safeguard. --- ghost/core/core/server/lib/image/ImageSize.js | 18 +++- .../core/core/server/lib/image/ImageUtils.js | 3 +- .../unit/server/lib/image/image-size.test.js | 85 +++++++++++++------ 3 files changed, 78 insertions(+), 28 deletions(-) diff --git a/ghost/core/core/server/lib/image/ImageSize.js b/ghost/core/core/server/lib/image/ImageSize.js index 7c9506b164..fecd52668c 100644 --- a/ghost/core/core/server/lib/image/ImageSize.js +++ b/ghost/core/core/server/lib/image/ImageSize.js @@ -1,6 +1,6 @@ const debug = require('@tryghost/debug')('utils:image-size'); const sizeOf = require('image-size'); -const probeSizeOf = require('probe-image-size'); + const url = require('url'); const path = require('path'); const _ = require('lodash'); @@ -17,13 +17,14 @@ const FETCH_ONLY_FORMATS = [ ]; class ImageSize { - constructor({config, storage, storageUtils, validator, urlUtils, request}) { + constructor({config, storage, storageUtils, validator, urlUtils, request, probe}) { this.config = config; this.storage = storage; this.storageUtils = storageUtils; this.validator = validator; this.urlUtils = urlUtils; this.request = request; + this.probe = probe; this.REQUEST_OPTIONS = { // we need the user-agent, otherwise some https request may fail (e.g. cloudfare) @@ -82,7 +83,18 @@ class ImageSize { })); } - return probeSizeOf(imageUrl, this.NEEDLE_OPTIONS); + // wrap probe-image-size in a promise in case it is unresponsive/the timeout itself doesn't work + return (Promise.race([ + this.probe(imageUrl, this.NEEDLE_OPTIONS), + new Promise((res, rej) => { + setTimeout(() => { + rej(new errors.InternalServerError({ + message: 'Probe unresponsive.', + code: 'IMAGE_SIZE_URL' + })); + }, this.NEEDLE_OPTIONS.response_timeout); + }) + ])); } // download full image then use image-size to get it's dimensions diff --git a/ghost/core/core/server/lib/image/ImageUtils.js b/ghost/core/core/server/lib/image/ImageUtils.js index 08bbb46903..82ca6cadf4 100644 --- a/ghost/core/core/server/lib/image/ImageUtils.js +++ b/ghost/core/core/server/lib/image/ImageUtils.js @@ -2,11 +2,12 @@ const BlogIcon = require('./BlogIcon'); const CachedImageSizeFromUrl = require('./CachedImageSizeFromUrl'); const Gravatar = require('./Gravatar'); const ImageSize = require('./ImageSize'); +const probe = require('probe-image-size'); class ImageUtils { constructor({config, urlUtils, settingsCache, storageUtils, storage, validator, request, cacheStore}) { this.blogIcon = new BlogIcon({config, urlUtils, settingsCache, storageUtils}); - this.imageSize = new ImageSize({config, storage, storageUtils, validator, urlUtils, request}); + this.imageSize = new ImageSize({config, storage, storageUtils, validator, urlUtils, request, probe}); this.cachedImageSizeFromUrl = new CachedImageSizeFromUrl({ getImageSizeFromUrl: this.imageSize.getImageSizeFromUrl.bind(this.imageSize), cache: cacheStore diff --git a/ghost/core/test/unit/server/lib/image/image-size.test.js b/ghost/core/test/unit/server/lib/image/image-size.test.js index 96481c28c6..1ec27f2e8c 100644 --- a/ghost/core/test/unit/server/lib/image/image-size.test.js +++ b/ghost/core/test/unit/server/lib/image/image-size.test.js @@ -5,6 +5,7 @@ const path = require('path'); const errors = require('@tryghost/errors'); const fs = require('fs'); const ImageSize = require('../../../../../core/server/lib/image/ImageSize'); +const probe = require('probe-image-size'); describe('lib/image: image size', function () { // use a 1x1 gif in nock responses because it's really small and easy to work with @@ -18,7 +19,7 @@ describe('lib/image: image size', function () { it('[success] should have an image size function', function () { const imageSize = new ImageSize({config: { get: () => {} - }, tpl: {}, storage: {}, storageUtils: {}, validator: {}, urlUtils: {}, request: {}}); + }, tpl: {}, storage: {}, storageUtils: {}, validator: {}, urlUtils: {}, request: {}, probe}); should.exist(imageSize.getImageSizeFromUrl); should.exist(imageSize.getImageSizeFromStoragePath); }); @@ -42,7 +43,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.true(); @@ -77,7 +78,7 @@ describe('lib/image: image size', function () { }); } return Promise.reject(); - }}); + }, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.false(); @@ -107,7 +108,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.true(); @@ -138,7 +139,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMockNotFound.isDone().should.be.false(); @@ -188,7 +189,7 @@ describe('lib/image: image size', function () { }); } return Promise.reject(); - }}); + }, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.true(); @@ -218,7 +219,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.true(); @@ -254,7 +255,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.true(); @@ -300,7 +301,7 @@ describe('lib/image: image size', function () { }, validator: {}, urlUtils: { urlFor: urlForStub, getSubdir: urlGetSubdirStub - }, request: {}}); + }, request: {}, probe}); imageSize.getImageSizeFromUrl(url).then(function (res) { requestMock.isDone().should.be.false(); @@ -328,7 +329,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url) .catch(function (err) { @@ -366,7 +367,7 @@ describe('lib/image: image size', function () { return Promise.reject(new NotFound()); } return Promise.reject(); - }}); + }}, probe); imageSize.getImageSizeFromUrl(url) .catch(function (err) { @@ -387,7 +388,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => false - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url) .catch(function (err) { @@ -398,7 +399,7 @@ describe('lib/image: image size', function () { }).catch(done); }); - it('[failure] will timeout', function (done) { + it('[failure] will handle responses timing out', function (done) { const url = 'https://static.wixstatic.com/media/355241_d31358572a2542c5a44738ddcb59e7ea.jpg_256'; const requestMock = nock('https://static.wixstatic.com') @@ -409,14 +410,18 @@ describe('lib/image: image size', function () { const imageSize = new ImageSize({config: { get: (key) => { if (key === 'times:getImageSizeTimeoutInMS') { - return 1; + return 10; } } }, tpl: {}, storage: {}, storageUtils: { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, + probe(reqUrl, options) { + // simulate probe the request timing out by probe's option + return probe(reqUrl, {...options, response_timeout: 1}); + }}); imageSize.getImageSizeFromUrl(url) .catch(function (err) { @@ -441,7 +446,7 @@ describe('lib/image: image size', function () { isLocalImage: () => false }, validator: { isURL: () => true - }, urlUtils: {}, request: {}}); + }, urlUtils: {}, request: {}, probe}); imageSize.getImageSizeFromUrl(url) .then(() => { @@ -475,7 +480,7 @@ describe('lib/image: image size', function () { }); } return Promise.reject(); - }}); + }, probe}); imageSize.getImageSizeFromUrl(url) .then(() => { @@ -500,7 +505,7 @@ describe('lib/image: image size', function () { isURL: () => true }, urlUtils: {}, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromUrl(url) .catch(function (err) { @@ -510,6 +515,38 @@ describe('lib/image: image size', function () { done(); }).catch(done); }); + + it('[failure] handles probe being unresponsive', function (done) { + const url = 'http://img.stockfresh.com/files/f/feedough/x/11/1540353_20925115.jpg'; + const requestMock = nock('http://img.stockfresh.com') + .get('/files/f/feedough/x/11/1540353_20925115.jpg') + .reply(200, GIF1x1); + + const imageSize = new ImageSize({config: { + get: (key) => { + if (key === 'times:getImageSizeTimeoutInMS') { + return 1; + } + } + }, tpl: {}, storage: {}, storageUtils: { + isLocalImage: () => false + }, validator: { + isURL: () => true + }, urlUtils: {}, request: {}, + probe(reqUrl, options) { + // simulate probe being unresponsive by making the timeout longer than the request + return probe(reqUrl, {...options, response_timeout: 10}); + }}); + + imageSize.getImageSizeFromUrl(url) + .catch(function (err) { + requestMock.isDone().should.be.true(); + should.exist(err); + err.errorType.should.be.equal('InternalServerError'); + err.message.should.be.equal('Probe unresponsive.'); + done(); + }).catch(done); + }); }); describe('getImageSizeFromStoragePath', function () { @@ -542,7 +579,7 @@ describe('lib/image: image size', function () { getSubdir: urlGetSubdirStub }, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromStoragePath(url).then(function (res) { should.exist(res); @@ -585,7 +622,7 @@ describe('lib/image: image size', function () { getSubdir: urlGetSubdirStub }, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromStoragePath(url).then(function (res) { should.exist(res); @@ -628,7 +665,7 @@ describe('lib/image: image size', function () { getSubdir: urlGetSubdirStub }, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromStoragePath(url).then(function (res) { should.exist(res); @@ -671,7 +708,7 @@ describe('lib/image: image size', function () { getSubdir: urlGetSubdirStub }, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromStoragePath(url).then(function (res) { should.exist(res); @@ -711,7 +748,7 @@ describe('lib/image: image size', function () { getSubdir: urlGetSubdirStub }, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromStoragePath(url) .catch(function (err) { @@ -746,7 +783,7 @@ describe('lib/image: image size', function () { getSubdir: urlGetSubdirStub }, request: () => { return Promise.reject({}); - }}); + }, probe}); imageSize.getImageSizeFromStoragePath(url) .catch(function (err) {