Skip to content

Commit

Permalink
Switched to throwing error upon failed image processing
Browse files Browse the repository at this point in the history
ref https://linear.app/tryghost/issue/ENG-740/http-500-error-when-image-processing-fails
refs 4aad551

- 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
  • Loading branch information
daniellockyer committed Mar 12, 2024
1 parent 4aad551 commit bfb9bfe
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 5 deletions.
14 changes: 10 additions & 4 deletions 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',
Expand Down Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions ghost/core/test/e2e-api/admin/__snapshots__/images.test.js.snap
Expand Up @@ -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 [
Expand Down
9 changes: 8 additions & 1 deletion ghost/core/test/e2e-api/admin/images.test.js
Expand Up @@ -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);
});
});

0 comments on commit bfb9bfe

Please sign in to comment.