Fixed handling malformed image + media upload requests

fix https://linear.app/tryghost/issue/SLO-94/unexpected-field-when-given-broken-image-upload-request

- in the event the body of an image or media upload request is malformed
  (broken metadata / blob or something), we get a MulterError and this
  bubbles up as an InternalServerError and spits out a HTTP 500
- we can capture this and return a BadRequestError, as it's the client's
  fault for not providing the correct body
- this implements that and adds breaking tests

ghost/core/core/server/web/api/middleware/upload.js

@@ -58,6 +58,12 @@ const single = name => function singleUploadFunction(req, res, next) {
 
     singleUpload(req, res, (err) => {
         if (err) {
+            if (err instanceof multer.MulterError) {
+                return next(new errors.BadRequestError({
+                    err
+                }));
+            }
+
             return next(err);
         }
         if (enabledClear) {
@@ -94,6 +100,12 @@ const media = (fileName, thumbName) => function mediaUploadFunction(req, res, ne
 
     mediaUpload(req, res, (err) => {
         if (err) {
+            if (err instanceof multer.MulterError) {
+                return next(new errors.BadRequestError({
+                    err
+                }));
+            }
+
             return next(err);
         }
 

ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap

@@ -108,6 +108,24 @@ Object {
 }
 `;
 
+exports[`Images API Errors when image request body is broken 1: [body] 1`] = `
+Object {
+  "errors": Array [
+    Object {
+      "code": "LIMIT_UNEXPECTED_FILE",
+      "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": "The request could not be understood.",
+      "property": null,
+      "type": "BadRequestError",
+    },
+  ],
+}
+`;
+
 exports[`Images API Will error when filename is too long 1: [body] 1`] = `
 Object {
   "errors": Array [

ghost/core/test/e2e-api/admin/images.test.js

@@ -321,6 +321,29 @@ describe('Images API', function () {
         sinon.assert.notCalled(unlinkStub);
     });
 
+    it('Errors when image request body is broken', async function () {
+        // Manually construct a broken request body
+        const blob = await fetch('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==').then(res => res.blob());
+        const brokenPayload = '--boundary\r\nContent-Disposition: form-data; name=\"image\"; filename=\"example.png\"\r\nContent-Type: image/png\r\n\r\n';
+
+        // eslint-disable-next-line no-undef
+        const brokenDataBlob = await (new Blob([brokenPayload, blob.slice(0, blob.size / 2)], {
+            type: 'multipart/form-data; boundary=boundary'
+        })).text();
+
+        sinon.stub(logging, 'error');
+        await agent
+            .post('/images/upload/')
+            .header('Content-Type', 'multipart/form-data; boundary=boundary')
+            .body(brokenDataBlob)
+            .expectStatus(400)
+            .matchBodySnapshot({
+                errors: [{
+                    id: anyErrorId
+                }]
+            });
+    });
+
     it('Does not return HTTP 500 when image processing fails', async function () {
         sinon.stub(imageTransform, 'resizeFromPath').rejects(new Error('Image processing failed'));
 

ghost/core/test/e2e-api/admin/media.test.js

@@ -130,6 +130,28 @@ describe('Media API', function () {
             res.body.errors[0].message.should.match(/select a valid media file/gi);
             sinon.assert.calledOnce(loggingStub);
         });
+
+        it('Errors when media request body is broken', async function () {
+            // Manually construct a broken request body
+
+            // Note: still using png mime type here but it doesn't matter because we're sending an invalid
+            // request body anyway
+            const blob = await fetch('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==').then(res => res.blob());
+            const brokenPayload = '--boundary\r\nContent-Disposition: form-data; name=\"image\"; filename=\"example.png\"\r\nContent-Type: image/png\r\n\r\n';
+
+            // eslint-disable-next-line no-undef
+            const brokenDataBlob = await (new Blob([brokenPayload, blob.slice(0, blob.size / 2)], {
+                type: 'multipart/form-data; boundary=boundary'
+            })).text();
+
+            sinon.stub(logging, 'error');
+            const res = await request.post(localUtils.API.getApiQuery('media/upload'))
+                .set('Content-Type', 'multipart/form-data; boundary=boundary')
+                .send(brokenDataBlob)
+                .expect(400);
+
+            res.body.errors[0].message.should.match(/The request could not be understood./gi);
+        });
     });
 
     describe('media/thumbnail/upload', function () {