From 4474ca1a1df67a6b678922c8681cf52fd662e60a Mon Sep 17 00:00:00 2001
From: Hannah Wolfe <github.erisds@gmail.com>
Date: Thu, 10 Aug 2017 14:31:52 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20Fixed=20Infinite=20404s=20for?=
 =?UTF-8?q?=20images=20(#8869)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

refs #8868

- Improve the error returned from local file store
- Use the new code to differentiate between static & non-static errors
---
 .../adapters/storage/LocalFileStorage.js      |  6 ++++-
 core/server/middleware/error-handler.js       | 24 +++++++++++++++----
 core/server/translations/en.json              |  1 +
 core/test/functional/routes/frontend_spec.js  |  2 +-
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/core/server/adapters/storage/LocalFileStorage.js b/core/server/adapters/storage/LocalFileStorage.js
index 2d836d1458..0e40954952 100644
--- a/core/server/adapters/storage/LocalFileStorage.js
+++ b/core/server/adapters/storage/LocalFileStorage.js
@@ -84,7 +84,11 @@ class LocalFileStore extends StorageBase {
             return serveStatic(self.storagePath, {maxAge: utils.ONE_YEAR_MS, fallthrough: false})(req, res, function (err) {
                 if (err) {
                     if (err.statusCode === 404) {
-                        return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')}));
+                        return next(new errors.NotFoundError({
+                            message: i18n.t('errors.errors.imageNotFound'),
+                            code: 'STATIC_FILE_NOT_FOUND',
+                            property: err.path
+                        }));
                     }
 
                     return next(new errors.GhostError({err: err}));
diff --git a/core/server/middleware/error-handler.js b/core/server/middleware/error-handler.js
index bf726bfe0a..8e8b956879 100644
--- a/core/server/middleware/error-handler.js
+++ b/core/server/middleware/error-handler.js
@@ -71,22 +71,32 @@ _private.JSONErrorRenderer = function JSONErrorRenderer(err, req, res, /*jshint
     });
 };
 
-_private.HTMLErrorRenderer = function HTMLErrorRender(err, req, res, /*jshint unused:false */ next) {
+_private.HTMLErrorRenderer = function HTMLErrorRender(err, req, res, next) {
+    // If the error code is explicitly set to STATIC_FILE_NOT_FOUND,
+    // Skip trying to render an HTML error, and move on to the basic error renderer
+    // I looked at doing this with accepts headers, but the internet is a crazy place...
+    // A better long term solution might be to do this based on extension
+    if (err.code === 'STATIC_FILE_NOT_FOUND') {
+        return next(err);
+    }
+
     var templateData = {
         message: err.message,
         code: err.statusCode,
         errorDetails: err.errorDetails || []
-    };
+    },
+    template = templates.error(err.statusCode);
 
     // It can be that something went wrong with the theme or otherwise loading handlebars
     // This ensures that no matter what res.render will work here
     if (_.isEmpty(req.app.engines)) {
+        template = 'error';
         req.app.engine('hbs', _private.createHbsEngine());
         req.app.set('view engine', 'hbs');
         req.app.set('views', config.get('paths').defaultViews);
     }
 
-    res.render(templates.error(err.statusCode), templateData, function renderResponse(err, html) {
+    res.render(template, templateData, function renderResponse(err, html) {
         if (!err) {
             return res.send(html);
         }
@@ -103,6 +113,10 @@ _private.HTMLErrorRenderer = function HTMLErrorRender(err, req, res, /*jshint un
     });
 };
 
+_private.BasicErorRenderer = function BasicErrorRenderer(err, req, res, /*jshint unused:false */ next) {
+    return res.send(res.statusCode + ' ' + err.message);
+};
+
 errorHandler.resourceNotFound = function resourceNotFound(req, res, next) {
     // TODO, handle unknown resources & methods differently, so that we can also produce
     // 405 Method Not Allowed
@@ -124,7 +138,9 @@ errorHandler.handleHTMLResponse = [
     // Make sure the error can be served
     _private.prepareError,
     // Render the error using HTML format
-    _private.HTMLErrorRenderer
+    _private.HTMLErrorRenderer,
+    // Fall back to basic if HTML is not explicitly accepted
+    _private.BasicErorRenderer
 ];
 
 module.exports = errorHandler;
diff --git a/core/server/translations/en.json b/core/server/translations/en.json
index 099ad765aa..b7ea676241 100644
--- a/core/server/translations/en.json
+++ b/core/server/translations/en.json
@@ -473,6 +473,7 @@
             "whilstTryingToRender": "whilst trying to render an error page for the error: ",
             "renderingErrorPage": "Rendering Error Page",
             "caughtProcessingError": "Ghost caught a processing error in the middleware layer.",
+            "imageNotFound": "Image not found",
             "pageNotFound": "Page not found",
             "resourceNotFound": "Resource not found"
         }
diff --git a/core/test/functional/routes/frontend_spec.js b/core/test/functional/routes/frontend_spec.js
index d5bfb36610..c0e022bc8c 100644
--- a/core/test/functional/routes/frontend_spec.js
+++ b/core/test/functional/routes/frontend_spec.js
@@ -179,7 +179,7 @@ describe('Frontend Routing', function () {
                     request.get('/content/images/some/file/that/doesnt-exist.jpg')
                         .expect('Cache-Control', testUtils.cacheRules['private'])
                         .expect(404)
-                        .expect(/Page not found/)
+                        .expect(/404 Image not found/)
                         .end(doEnd(done));
                 });
             });