From af948553497de999d03569eb3b4fd32077cb93f7 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Wed, 24 Aug 2022 08:28:20 +0100 Subject: [PATCH] Removed bluebird catch predicates from API endpoints refs: https://github.com/TryGhost/Ghost/issues/14882 - I found a common pattern where catch predicates were being used to catch non-existent models in destroy methods, and sometimes elsewhere in the API endpoints - The use of predicates is deprecated, and we're working to remove them from everywhere, so that we can remove bluebird - In order to still handle these errors correctly, we needed a small change to mw-error-handler so that it can detect EmptyResponse errors from bookshelf, as well as 404s Note: there is a small change as a result of this - the context on these errors now says "Resource not found" instead of "{ModelName} not found". - I think this is acceptable for now, as we will be reviewing these errors in more depth later. It's quite easy to make changes, we just have to decide what with proper design input --- .../server/api/endpoints/comments-members.js | 17 ++- .../core/core/server/api/endpoints/invites.js | 10 +- .../core/core/server/api/endpoints/labels.js | 8 +- .../core/core/server/api/endpoints/members.js | 16 +-- ghost/core/core/server/api/endpoints/pages.js | 10 +- ghost/core/core/server/api/endpoints/posts.js | 10 +- .../core/server/api/endpoints/snippets.js | 10 +- ghost/core/core/server/api/endpoints/tags.js | 8 +- .../core/server/api/endpoints/webhooks.js | 21 +-- .../admin/__snapshots__/labels.test.js.snap | 60 +++++++++ .../admin/__snapshots__/members.test.js.snap | 30 +++++ .../admin/__snapshots__/snippets.test.js.snap | 90 +++++++++++++ .../admin/__snapshots__/webhooks.test.js.snap | 120 ++++++++++++++++++ ghost/core/test/e2e-api/admin/invites.test.js | 12 ++ ghost/core/test/e2e-api/admin/labels.test.js | 14 ++ ghost/core/test/e2e-api/admin/members.test.js | 16 ++- .../core/test/e2e-api/admin/snippets.test.js | 14 ++ ghost/core/test/e2e-api/admin/tags.test.js | 10 ++ .../core/test/e2e-api/admin/webhooks.test.js | 35 +++++ .../__snapshots__/comments.test.js.snap | 36 +++++- .../e2e-api/members-comments/comments.test.js | 31 +++-- .../mw-error-handler/lib/mw-error-handler.js | 4 +- 22 files changed, 479 insertions(+), 103 deletions(-) diff --git a/ghost/core/core/server/api/endpoints/comments-members.js b/ghost/core/core/server/api/endpoints/comments-members.js index 582ef3a8eb..49cb949882 100644 --- a/ghost/core/core/server/api/endpoints/comments-members.js +++ b/ghost/core/core/server/api/endpoints/comments-members.js @@ -189,8 +189,6 @@ module.exports = { validation: {}, permissions: true, query(frame) { - frame.options.require = true; - // TODO: move to likes service if (frame.options?.context?.member?.id) { return models.CommentLike.destroy({ @@ -198,12 +196,17 @@ module.exports = { destroyBy: { member_id: frame.options.context.member.id, comment_id: frame.options.id - } + }, + require: true }).then(() => null) - .catch(models.CommentLike.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.likeNotFound) - })); + .catch((err) => { + if (err instanceof models.CommentLike.NotFoundError) { + return Promise.reject(new errors.NotFoundError({ + message: tpl(messages.likeNotFound) + })); + } + + throw err; }); } else { return Promise.reject(new errors.NotFoundError({ diff --git a/ghost/core/core/server/api/endpoints/invites.js b/ghost/core/core/server/api/endpoints/invites.js index 771b6c9ab1..705c09224c 100644 --- a/ghost/core/core/server/api/endpoints/invites.js +++ b/ghost/core/core/server/api/endpoints/invites.js @@ -76,15 +76,7 @@ module.exports = { }, permissions: true, query(frame) { - frame.options.require = true; - - return models.Invite.destroy(frame.options) - .then(() => null) - .catch(models.Invite.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.inviteNotFound) - })); - }); + return models.Invite.destroy({...frame.options, require: true}); } }, diff --git a/ghost/core/core/server/api/endpoints/labels.js b/ghost/core/core/server/api/endpoints/labels.js index 7662aa88e4..4085d59e4a 100644 --- a/ghost/core/core/server/api/endpoints/labels.js +++ b/ghost/core/core/server/api/endpoints/labels.js @@ -150,13 +150,7 @@ module.exports = { }, permissions: true, query(frame) { - return models.Label.destroy(frame.options) - .then(() => null) - .catch(models.Label.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.labelNotFound) - })); - }); + return models.Label.destroy({...frame.options, require: true}); } } }; diff --git a/ghost/core/core/server/api/endpoints/members.js b/ghost/core/core/server/api/endpoints/members.js index 04f160f6b2..9cee71bab7 100644 --- a/ghost/core/core/server/api/endpoints/members.js +++ b/ghost/core/core/server/api/endpoints/members.js @@ -1,6 +1,5 @@ // NOTE: We must not cache references to membersService.api // as it is a getter and may change during runtime. -const Promise = require('bluebird'); const moment = require('moment-timezone'); const errors = require('@tryghost/errors'); const models = require('../../models'); @@ -253,20 +252,11 @@ module.exports = { }, permissions: true, async query(frame) { - frame.options.require = true; - frame.options.cancelStripeSubscriptions = frame.options.cancel; - - await Promise.resolve(membersService.api.members.destroy({ + return membersService.api.members.destroy({ id: frame.options.id - }, frame.options)).catch(models.Member.NotFoundError, () => { - throw new errors.NotFoundError({ - message: tpl(messages.resourceNotFound, { - resource: 'Member' - }) - }); + }, { + ...frame.options, require: true, cancelStripeSubscriptions: frame.options.cancel }); - - return null; } }, diff --git a/ghost/core/core/server/api/endpoints/pages.js b/ghost/core/core/server/api/endpoints/pages.js index 621992b26a..381122b509 100644 --- a/ghost/core/core/server/api/endpoints/pages.js +++ b/ghost/core/core/server/api/endpoints/pages.js @@ -186,15 +186,7 @@ module.exports = { unsafeAttrs: UNSAFE_ATTRS }, query(frame) { - frame.options.require = true; - - return models.Post.destroy(frame.options) - .then(() => null) - .catch(models.Post.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.pageNotFound) - })); - }); + return models.Post.destroy({...frame.options, require: true}); } } }; diff --git a/ghost/core/core/server/api/endpoints/posts.js b/ghost/core/core/server/api/endpoints/posts.js index c97179b245..7d2d15a49e 100644 --- a/ghost/core/core/server/api/endpoints/posts.js +++ b/ghost/core/core/server/api/endpoints/posts.js @@ -192,15 +192,7 @@ module.exports = { unsafeAttrs: unsafeAttrs }, query(frame) { - frame.options.require = true; - - return models.Post.destroy(frame.options) - .then(() => null) - .catch(models.Post.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.postNotFound) - })); - }); + return models.Post.destroy({...frame.options, require: true}); } } }; diff --git a/ghost/core/core/server/api/endpoints/snippets.js b/ghost/core/core/server/api/endpoints/snippets.js index 33615d33f2..b1cd58e588 100644 --- a/ghost/core/core/server/api/endpoints/snippets.js +++ b/ghost/core/core/server/api/endpoints/snippets.js @@ -101,15 +101,7 @@ module.exports = { }, permissions: true, query(frame) { - frame.options.require = true; - - return models.Snippet.destroy(frame.options) - .then(() => null) - .catch(models.Snippet.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.snippetNotFound) - })); - }); + return models.Snippet.destroy({...frame.options, require: true}); } } }; diff --git a/ghost/core/core/server/api/endpoints/tags.js b/ghost/core/core/server/api/endpoints/tags.js index 2de713c0d8..5f3caa9859 100644 --- a/ghost/core/core/server/api/endpoints/tags.js +++ b/ghost/core/core/server/api/endpoints/tags.js @@ -147,13 +147,7 @@ module.exports = { }, permissions: true, query(frame) { - return models.Tag.destroy(frame.options) - .then(() => null) - .catch(models.Tag.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.tagNotFound) - })); - }); + return models.Tag.destroy({...frame.options, require: true}); } } }; diff --git a/ghost/core/core/server/api/endpoints/webhooks.js b/ghost/core/core/server/api/endpoints/webhooks.js index d2ff54279a..f574c7e202 100644 --- a/ghost/core/core/server/api/endpoints/webhooks.js +++ b/ghost/core/core/server/api/endpoints/webhooks.js @@ -78,14 +78,7 @@ module.exports = { } }, query({data, options}) { - return models.Webhook.edit(data.webhooks[0], Object.assign(options, {require: true})) - .catch(models.Webhook.NotFoundError, () => { - throw new errors.NotFoundError({ - message: tpl(messages.resourceNotFound, { - resource: 'Webhook' - }) - }); - }); + return models.Webhook.edit(data.webhooks[0], {...options, require: true}); } }, @@ -130,17 +123,7 @@ module.exports = { } }, query(frame) { - frame.options.require = true; - - return models.Webhook.destroy(frame.options) - .then(() => null) - .catch(models.Webhook.NotFoundError, () => { - return Promise.reject(new errors.NotFoundError({ - message: tpl(messages.resourceNotFound, { - resource: 'Webhook' - }) - })); - }); + return models.Webhook.destroy({...frame.options, require: true}); } } }; diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/labels.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/labels.test.js.snap index f1531b054b..93101418c3 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/labels.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/labels.test.js.snap @@ -184,6 +184,66 @@ Object { } `; +exports[`Labels API Cannot destroy non-existent label 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete label.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Labels API Cannot destroy non-existent label 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "258", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Labels API Cannot destroy nonexistent label 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete label.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Labels API Cannot destroy nonexistent label 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "258", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Labels API Errors when adding label with the same name 1: [body] 1`] = ` Object { "errors": Array [ diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap index bc1b79953b..6a8659e0b7 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snap @@ -3340,6 +3340,36 @@ Object { } `; +exports[`Members API Cannot delete a non-existent member 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete member.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Members API Cannot delete a non-existent member 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "259", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Members API Cannot edit a non-existing id 1: [body] 1`] = ` Object { "errors": Array [ diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/snippets.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/snippets.test.js.snap index ef0ca1cf2b..1244a6b180 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/snippets.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/snippets.test.js.snap @@ -215,3 +215,93 @@ Object { "x-powered-by": "Express", } `; + +exports[`Snippets API Cannot destroy non-existent snippet 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete snippet.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Snippets API Cannot destroy non-existent snippet 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "260", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Snippets API Cannot destroy nonexistent snippet 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete snippet.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Snippets API Cannot destroy nonexistent snippet 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "260", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Snippets API Cannot destroy unknown snippet 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource not found.", + "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": "Resource not found error, cannot delete snippet.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Snippets API Cannot destroy unknown snippet 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "250", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/webhooks.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/webhooks.test.js.snap index 82b2be6893..c1ded3c5e6 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/webhooks.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/webhooks.test.js.snap @@ -76,6 +76,126 @@ Object { } `; +exports[`Webhooks API Cannot delete a non-existent webhook 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete webhook.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Webhooks API Cannot delete a non-existent webhook 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "260", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Webhooks API Cannot delete a nonexistent webhook 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot delete webhook.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Webhooks API Cannot delete a nonexistent webhook 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "260", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Webhooks API Cannot edit a non-existent webhook 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot edit webhook.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Webhooks API Cannot edit a non-existent webhook 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "258", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + +exports[`Webhooks API Cannot edit a nonexistent webhook 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Resource could not be found.", + "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": "Resource not found error, cannot edit webhook.", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Webhooks API Cannot edit a nonexistent webhook 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "258", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Webhooks API Fails nicely when adding a duplicate webhook 1: [body] 1`] = ` Object { "errors": Array [ diff --git a/ghost/core/test/e2e-api/admin/invites.test.js b/ghost/core/test/e2e-api/admin/invites.test.js index a294ca7765..e2b2dfb544 100644 --- a/ghost/core/test/e2e-api/admin/invites.test.js +++ b/ghost/core/test/e2e-api/admin/invites.test.js @@ -102,4 +102,16 @@ describe('Invites API', function () { mailService.GhostMailer.prototype.send.called.should.be.false(); }); + + it('Cannot destroy an non-existent invite', async function () { + await request.del(localUtils.API.getApiQuery(`invites/abcd1234abcd1234abcd1234/`)) + .set('Origin', config.get('url')) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(404) + .expect((res) => { + res.body.errors[0].message.should.eql('Resource not found error, cannot delete invite.'); + }); + + mailService.GhostMailer.prototype.send.called.should.be.false(); + }); }); diff --git a/ghost/core/test/e2e-api/admin/labels.test.js b/ghost/core/test/e2e-api/admin/labels.test.js index 2587435996..c4cc5a008b 100644 --- a/ghost/core/test/e2e-api/admin/labels.test.js +++ b/ghost/core/test/e2e-api/admin/labels.test.js @@ -118,4 +118,18 @@ describe('Labels API', function () { etag: anyEtag }); }); + + it('Cannot destroy non-existent label', async function () { + await agent + .delete('labels/abcd1234abcd1234abcd1234') + .expectStatus(404) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); }); diff --git a/ghost/core/test/e2e-api/admin/members.test.js b/ghost/core/test/e2e-api/admin/members.test.js index 51745bac89..1b62cee0e2 100644 --- a/ghost/core/test/e2e-api/admin/members.test.js +++ b/ghost/core/test/e2e-api/admin/members.test.js @@ -167,7 +167,7 @@ describe('Members API', function () { beforeEach(function () { mockManager.mockStripe(); mockManager.mockMail(); - + // For some reason it is enabled by default? mockManager.mockLabsDisabled('memberAttribution'); }); @@ -1582,6 +1582,20 @@ describe('Members API', function () { }); }); + it('Cannot delete a non-existent member', async function () { + await agent + .delete('/members/abcd1234abcd1234abcd1234') + .expectStatus(404) + .matchBodySnapshot({ + errors: [{ + id: anyUuid + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); + it('Can delete a member without cancelling Stripe Subscription', async function () { let subscriptionCanceled = false; nock('https://api.stripe.com') diff --git a/ghost/core/test/e2e-api/admin/snippets.test.js b/ghost/core/test/e2e-api/admin/snippets.test.js index a989cc961e..356e25e5c7 100644 --- a/ghost/core/test/e2e-api/admin/snippets.test.js +++ b/ghost/core/test/e2e-api/admin/snippets.test.js @@ -136,4 +136,18 @@ describe('Snippets API', function () { etag: anyEtag }); }); + + it('Cannot destroy non-existent snippet', async function () { + await agent + .delete('snippets/abcd1234abcd1234abcd1234') + .expectStatus(404) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); }); diff --git a/ghost/core/test/e2e-api/admin/tags.test.js b/ghost/core/test/e2e-api/admin/tags.test.js index d0fee5a3a8..78237378c3 100644 --- a/ghost/core/test/e2e-api/admin/tags.test.js +++ b/ghost/core/test/e2e-api/admin/tags.test.js @@ -160,4 +160,14 @@ describe('Tag API', function () { should.exist(res.headers['x-cache-invalidate']); res.body.should.eql({}); }); + + it('Can destroy a non-existent tag', async function () { + const res = await request + .del(localUtils.API.getApiQuery(`tags/abcd1234abcd1234abcd1234`)) + .set('Origin', config.get('url')) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(404); + + res.body.errors[0].message.should.eql('Resource not found error, cannot delete tag.'); + }); }); diff --git a/ghost/core/test/e2e-api/admin/webhooks.test.js b/ghost/core/test/e2e-api/admin/webhooks.test.js index 58f211fe08..7ad113803d 100644 --- a/ghost/core/test/e2e-api/admin/webhooks.test.js +++ b/ghost/core/test/e2e-api/admin/webhooks.test.js @@ -102,6 +102,27 @@ describe('Webhooks API', function () { }); }); + it('Cannot edit a non-existent webhook', async function () { + await agent.put('/webhooks/abcd1234abcd1234abcd1234/') + .body({ + webhooks: [{ + name: 'Edit Test', + event: 'member.added', + target_url: 'https://example.com/new-member', + integration_id: 'ignore_me' + }] + }) + .expectStatus(404) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); + it('Can delete a webhook', async function () { await agent .delete(`/webhooks/${createdWebhookId}/`) @@ -111,4 +132,18 @@ describe('Webhooks API', function () { etag: anyEtag }); }); + + it('Cannot delete a non-existent webhook', async function () { + await agent + .delete('/webhooks/abcd1234abcd1234abcd1234/') + .expectStatus(404) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId + }] + }) + .matchHeaderSnapshot({ + etag: anyEtag + }); + }); }); diff --git a/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap b/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap index e0b2f5d2e9..bf90d6617b 100644 --- a/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap +++ b/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap @@ -564,7 +564,7 @@ Object { } `; -exports[`Comments API when authenticated Can remove a like 1: [headers] 1`] = ` +exports[`Comments API when authenticated Can remove a like (unlike) 1: [headers] 1`] = ` Object { "access-control-allow-origin": "*", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", @@ -573,7 +573,7 @@ Object { } `; -exports[`Comments API when authenticated Can remove a like 2: [body] 1`] = ` +exports[`Comments API when authenticated Can remove a like (unlike) 2: [body] 1`] = ` Object { "comments": Array [ Object { @@ -619,7 +619,7 @@ Object { } `; -exports[`Comments API when authenticated Can remove a like 3: [headers] 1`] = ` +exports[`Comments API when authenticated Can remove a like (unlike) 3: [headers] 1`] = ` Object { "access-control-allow-origin": "*", "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", @@ -952,6 +952,36 @@ Object { } `; +exports[`Comments API when authenticated Cannot unlike a comment if it has not been liked 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": "Unable to find like", + "property": null, + "type": "NotFoundError", + }, + ], +} +`; + +exports[`Comments API when authenticated Cannot unlike a comment if it has not been liked 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "*", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "205", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Comments API when authenticated Limits returned replies to 3 1: [body] 1`] = ` Object { "comments": Array [ diff --git a/ghost/core/test/e2e-api/members-comments/comments.test.js b/ghost/core/test/e2e-api/members-comments/comments.test.js index 1c8f0efaed..507a0468db 100644 --- a/ghost/core/test/e2e-api/members-comments/comments.test.js +++ b/ghost/core/test/e2e-api/members-comments/comments.test.js @@ -1,6 +1,6 @@ const assert = require('assert'); const {agentProvider, mockManager, fixtureManager, matchers} = require('../../utils/e2e-framework'); -const {anyEtag, anyObjectId, anyLocationFor, anyISODateTime, anyUuid, anyNumber, anyBoolean} = matchers; +const {anyEtag, anyObjectId, anyLocationFor, anyISODateTime, anyErrorId, anyUuid, anyNumber, anyBoolean} = matchers; const should = require('should'); const models = require('../../../core/server/models'); const moment = require('moment-timezone'); @@ -23,9 +23,9 @@ const commentMatcher = { }; /** - * @param {Object} [options] - * @param {number} [options.replies] - * @returns + * @param {Object} [options] + * @param {number} [options.replies] + * @returns */ function commentMatcherWithReplies(options = {replies: 0}) { return { @@ -338,7 +338,7 @@ describe('Comments API', function () { testReplyId = reply.comments[0].id; } } - + // Check if we have count.replies = 4, and replies.length == 3 await membersAgent .get(`/api/comments/${parentId}/`) @@ -481,8 +481,8 @@ describe('Comments API', function () { }); }); - it('Can remove a like', async function () { - // Create a temporary comment + it('Can remove a like (unlike)', async function () { + // Unlike await membersAgent .delete(`/api/comments/${commentId}/like/`) .expectStatus(204) @@ -491,7 +491,7 @@ describe('Comments API', function () { }) .expectEmptyBody(); - // Check liked + // Check not liked await membersAgent .get(`/api/comments/${commentId}/`) .expectStatus(200) @@ -507,6 +507,21 @@ describe('Comments API', function () { }); }); + it('Cannot unlike a comment if it has not been liked', async function () { + // Remove like + await membersAgent + .delete(`/api/comments/${commentId}/like/`) + //.expectStatus(404) + .matchHeaderSnapshot({ + etag: anyEtag + }) + .matchBodySnapshot({ + errors: [{ + id: anyErrorId + }] + }); + }); + it('Can report a comment', async function () { // Create a temporary comment await membersAgent diff --git a/ghost/mw-error-handler/lib/mw-error-handler.js b/ghost/mw-error-handler/lib/mw-error-handler.js index 99005657b0..bd45f6e489 100644 --- a/ghost/mw-error-handler/lib/mw-error-handler.js +++ b/ghost/mw-error-handler/lib/mw-error-handler.js @@ -62,8 +62,8 @@ module.exports.prepareError = (err, req, res, next) => { } if (!errors.utils.isGhostError(err)) { - // We need a special case for 404 errors - if (err.statusCode && err.statusCode === 404) { + // We need a special case for 404 errors & bookshelf empty errors + if ((err.statusCode && err.statusCode === 404) || err.message === 'EmptyResponse') { err = new errors.NotFoundError({ err: err });