From 55791a8c6465831aad431110f44bf90594eb84b5 Mon Sep 17 00:00:00 2001
From: Daniel Lockyer <hi@daniellockyer.com>
Date: Tue, 12 Mar 2024 15:56:40 +0100
Subject: [PATCH] Switched to throwing error upon failed image processing

ref https://linear.app/tryghost/issue/ENG-740/http-500-error-when-image-processing-fails
refs https://github.com/TryGhost/Ghost/commit/4aad551c72cba5e927b7ac4ee529af2f30b049e8

- upon further discussion, we've decided it's better to throw an error
  in this case because the uploaded image is deemed invalid and storing
  it on the filesystem might cause more issues with resizing/further
  processing in the future
- this commit implements that and alters the tests
---
 ghost/core/core/server/api/endpoints/images.js | 16 +++++++++++-----
 .../admin/__snapshots__/images.test.js.snap    | 18 ++++++++++++++++++
 ghost/core/test/e2e-api/admin/images.test.js   |  9 ++++++++-
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/ghost/core/core/server/api/endpoints/images.js b/ghost/core/core/server/api/endpoints/images.js
index e2c2671858..140e88bd1e 100644
--- a/ghost/core/core/server/api/endpoints/images.js
+++ b/ghost/core/core/server/api/endpoints/images.js
@@ -1,8 +1,10 @@
 /* eslint-disable ghost/ghost-custom/max-api-complexity */
-const storage = require('../../adapters/storage');
-const imageTransform = require('@tryghost/image-transform');
-const config = require('../../../shared/config');
 const path = require('path');
+const errors = require('@tryghost/errors');
+const imageTransform = require('@tryghost/image-transform');
+
+const storage = require('../../adapters/storage');
+const config = require('../../../shared/config');
 
 module.exports = {
     docName: 'images',
@@ -36,8 +38,12 @@ module.exports = {
                 try {
                     await imageTransform.resizeFromPath(options);
                 } catch (err) {
-                    // If the image processing fails, we just want to store the original image
-                    return store.save(frame.file);
+                    // If the image processing fails, we don't want to store the image because it's corrupted/invalid
+                    throw new errors.BadRequestError({
+                        message: 'Image processing failed',
+                        context: err.message,
+                        help: 'Please verify that the image is valid'
+                    });
                 }
 
                 // Store the processed/optimized image
diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap
index 2bc56cbaf0..a48fd4a2db 100644
--- a/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap
+++ b/ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap
@@ -72,6 +72,24 @@ Object {
 }
 `;
 
+exports[`Images API Does not return HTTP 500 when image processing fails 1: [body] 1`] = `
+Object {
+  "errors": Array [
+    Object {
+      "code": null,
+      "context": "Image processing failed Image processing failed",
+      "details": null,
+      "ghostErrorCode": null,
+      "help": "Please verify that the image is valid",
+      "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
+      "message": "Request not understood error, cannot upload image.",
+      "property": null,
+      "type": "BadRequestError",
+    },
+  ],
+}
+`;
+
 exports[`Images API Will error when filename is too long 1: [body] 1`] = `
 Object {
   "errors": Array [
diff --git a/ghost/core/test/e2e-api/admin/images.test.js b/ghost/core/test/e2e-api/admin/images.test.js
index e5f08322de..3baf808a59 100644
--- a/ghost/core/test/e2e-api/admin/images.test.js
+++ b/ghost/core/test/e2e-api/admin/images.test.js
@@ -308,7 +308,14 @@ describe('Images API', function () {
         const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
         const fileContents = await fs.readFile(originalFilePath);
 
+        const loggingStub = sinon.stub(logging, 'error');
         await uploadImageRequest({fileContents, filename: 'test.png', contentType: 'image/png'})
-            .expectStatus(201);
+            .expectStatus(400)
+            .matchBodySnapshot({
+                errors: [{
+                    id: anyErrorId
+                }]
+            });
+        sinon.assert.calledOnce(loggingStub);
     });
 });