From bfb9bfebdc509889c49d83da3adec0e232fb5444 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 | 14 ++++++++++----
 .../admin/__snapshots__/images.test.js.snap    | 18 ++++++++++++++++++
 ghost/core/test/e2e-api/admin/images.test.js   |  9 ++++++++-
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/ghost/core/core/server/api/endpoints/images.js b/ghost/core/core/server/api/endpoints/images.js
index e2c26718587a..140e88bd1ec8 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 path = require('path');
+const errors = require('@tryghost/errors');
 const imageTransform = require('@tryghost/image-transform');
+
+const storage = require('../../adapters/storage');
 const config = require('../../../shared/config');
-const path = require('path');
 
 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 2bc56cbaf001..a48fd4a2dbd2 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 e5f08322de7d..3baf808a59df 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);
     });
 });