From 3e797124669d767d8e60404cb5f7449777fcc407 Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Mon, 6 May 2024 13:14:09 +0200 Subject: [PATCH] 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 --- .../core/server/web/api/middleware/upload.js | 12 ++++++++++ .../admin/__snapshots__/images.test.js.snap | 18 +++++++++++++++ ghost/core/test/e2e-api/admin/images.test.js | 23 +++++++++++++++++++ ghost/core/test/e2e-api/admin/media.test.js | 22 ++++++++++++++++++ 4 files changed, 75 insertions(+) diff --git a/ghost/core/core/server/web/api/middleware/upload.js b/ghost/core/core/server/web/api/middleware/upload.js index 65d7ac608c..6bcced3ef9 100644 --- a/ghost/core/core/server/web/api/middleware/upload.js +++ b/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); } 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 7bb101fc84..cf70745739 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 @@ -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 [ diff --git a/ghost/core/test/e2e-api/admin/images.test.js b/ghost/core/test/e2e-api/admin/images.test.js index 518f9a29bc..c9c56cbd57 100644 --- a/ghost/core/test/e2e-api/admin/images.test.js +++ b/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')); diff --git a/ghost/core/test/e2e-api/admin/media.test.js b/ghost/core/test/e2e-api/admin/media.test.js index 2113164ca0..e579225bd3 100644 --- a/ghost/core/test/e2e-api/admin/media.test.js +++ b/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 () {