From 7950122ffee1aa6000b51c10ba1abcb798b8ac72 Mon Sep 17 00:00:00 2001
From: Daniel Lockyer <hi@daniellockyer.com>
Date: Thu, 2 May 2024 17:14:11 +0200
Subject: [PATCH] Protected against deleting non-existent image during upload

fix https://linear.app/tryghost/issue/SLO-93/undefined-path-error-with-bad-image-upload

- in the event we receive a request to upload an image, that doesn't
  contain an image, we still try and unlink the files
- this is a dangling promise, so it doesn't cause an explicit HTTP
  error, but it does show up as a console error
- fixed it by checking for the path, and early returning if it doesn't
  exist
- also added a test that would fail without this
---
 .../core/server/web/api/middleware/upload.js  |  8 +++++++-
 .../admin/__snapshots__/images.test.js.snap   | 18 ++++++++++++++++++
 ghost/core/test/e2e-api/admin/images.test.js  | 19 +++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/ghost/core/core/server/web/api/middleware/upload.js b/ghost/core/core/server/web/api/middleware/upload.js
index 99afc4cd3c..65d7ac608c 100644
--- a/ghost/core/core/server/web/api/middleware/upload.js
+++ b/ghost/core/core/server/web/api/middleware/upload.js
@@ -45,7 +45,13 @@ const messages = {
 const enabledClear = config.get('uploadClear') || true;
 const upload = multer({dest: os.tmpdir()});
 
-const deleteSingleFile = file => fs.unlink(file.path).catch(err => logging.error(err));
+const deleteSingleFile = (file) => {
+    if (!file.path) {
+        return;
+    }
+
+    fs.unlink(file.path).catch(err => logging.error(err));
+};
 
 const single = name => function singleUploadFunction(req, res, next) {
     const singleUpload = upload.single(name);
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 a48fd4a2db..7bb101fc84 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
@@ -90,6 +90,24 @@ Object {
 }
 `;
 
+exports[`Images API Does not try to delete non-existing file upon invalid request 1: [body] 1`] = `
+Object {
+  "errors": Array [
+    Object {
+      "code": null,
+      "context": null,
+      "details": null,
+      "ghostErrorCode": null,
+      "help": null,
+      "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
+      "message": "Please select an image.",
+      "property": null,
+      "type": "ValidationError",
+    },
+  ],
+}
+`;
+
 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 3baf808a59..518f9a29bc 100644
--- a/ghost/core/test/e2e-api/admin/images.test.js
+++ b/ghost/core/test/e2e-api/admin/images.test.js
@@ -1,6 +1,7 @@
 const {agentProvider, fixtureManager, matchers} = require('../../utils/e2e-framework');
 const FormData = require('form-data');
 const p = require('path');
+const fsExtra = require('fs-extra');
 const {promises: fs} = require('fs');
 const assert = require('assert/strict');
 const config = require('../../../core/shared/config');
@@ -302,6 +303,24 @@ describe('Images API', function () {
         clock.restore();
     });
 
+    it('Does not try to delete non-existing file upon invalid request', async function () {
+        sinon.stub(logging, 'error');
+        const unlinkStub = sinon.stub(fsExtra, 'unlink');
+
+        await agent
+            .post('/images/upload/')
+            .body(JSON.stringify({}))
+            .expectStatus(422)
+            .matchBodySnapshot({
+                errors: [{
+                    id: anyErrorId
+                }]
+            });
+
+        // We didn't upload an image, so we shouldn't have called unlink
+        sinon.assert.notCalled(unlinkStub);
+    });
+
     it('Does not return HTTP 500 when image processing fails', async function () {
         sinon.stub(imageTransform, 'resizeFromPath').rejects(new Error('Image processing failed'));