Improved comments API security (#15065)

refs https://github.com/TryGhost/Team/issues/1688

* Added missing/failing tests
* Refactored comments BREAD into service
* Ensured member_id is not writable, it should come from auth only
* Ensured one cannot reply to a reply
* Ensured the parent_id is not writable on edit
This commit is contained in:
Fabien 'egg' O'Carroll 2022-07-25 10:41:33 +01:00 committed by GitHub
parent ee5753a6b7
commit b3471ab439
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 563 additions and 37 deletions

View File

@ -3,7 +3,7 @@ const tpl = require('@tryghost/tpl');
const errors = require('@tryghost/errors');
const models = require('../../models');
const db = require('../../data/db');
const service = require('../../services/comments');
const commentsService = require('../../services/comments');
const ALLOWED_INCLUDES = ['post', 'member', 'likes', 'replies'];
const UNSAFE_ATTRS = ['status'];
@ -52,17 +52,8 @@ module.exports = {
}
},
permissions: true,
query(frame) {
return models.Comment.findOne(frame.data, frame.options)
.then((model) => {
if (!model) {
return Promise.reject(new errors.NotFoundError({
message: tpl(messages.commentNotFound)
}));
}
return model;
});
async query(frame) {
return await commentsService.api.getCommentByID(frame.data.id, frame.options);
}
},
@ -83,17 +74,21 @@ module.exports = {
}
},
permissions: true,
query(frame) {
return models.Comment.edit(frame.data.comments[0], frame.options)
.then((model) => {
if (!model) {
return Promise.reject(new errors.NotFoundError({
message: tpl(messages.commentNotFound)
}));
}
async query(frame) {
if (frame.data.comments[0].status === 'deleted') {
return await commentsService.api.deleteComment(
frame.options.id,
frame?.options?.context?.member?.id,
frame.options
);
}
return model;
});
return await commentsService.api.editCommentContent(
frame.options.id,
frame?.options?.context?.member?.id,
frame.data.comments[0].html,
frame.options
);
}
},
@ -116,20 +111,24 @@ module.exports = {
permissions: {
unsafeAttrs: UNSAFE_ATTRS
},
query(frame) {
// TODO: move to comment service
async query(frame) {
const data = frame.data.comments[0];
if (frame.options?.context?.member?.id) {
data.member_id = frame.options.context.member.id;
// todo: add validation that the parent comment is on the same post, and not deleted
return models.Comment.add(data, frame.options);
} else {
return Promise.reject(new errors.NotFoundError({
message: tpl(messages.memberNotFound)
}));
if (data.parent_id) {
return await commentsService.api.replyToComment(
data.parent_id,
frame.options.context.member.id,
data.html,
frame.options
);
}
return await commentsService.api.commentOnPost(
data.post_id,
frame.options.context.member.id,
data.html,
frame.options
);
}
},
@ -260,7 +259,7 @@ module.exports = {
}));
}
await service.api.reportComment(frame.options.id, frame.options?.context?.member);
await commentsService.api.reportComment(frame.options.id, frame.options?.context?.member);
}
}
};

View File

@ -1,3 +1,14 @@
const tpl = require('@tryghost/tpl');
const errors = require('@tryghost/errors');
const messages = {
commentNotFound: 'Comment could not be found',
memberNotFound: 'Unable to find member',
likeNotFound: 'Unable to find like',
alreadyLiked: 'This comment was liked already',
replyToReply: 'Can not reply to a reply'
};
class CommentsService {
constructor({config, logging, models, mailer, settingsCache, urlService, urlUtils}) {
this.config = config;
@ -42,6 +53,142 @@ class CommentsService {
await this.emails.notifiyReport(comment, reporter);
}
/**
* @param {string} id - The ID of the Comment to get
* @param {any} options
*/
async getCommentByID(id, options) {
const model = await this.models.Comment.findOne({id}, options);
if (!model) {
throw new errors.NotFoundError({
messages: tpl(messages.commentNotFound)
});
}
return model;
}
/**
* @param {string} post - The ID of the Post to comment on
* @param {string} member - The ID of the Member to comment as
* @param {string} comment - The HTML content of the Comment
* @param {any} options
*/
async commentOnPost(post, member, comment, options) {
await this.models.Member.findOne({
id: member
}, {
require: true,
...options
});
const model = await this.models.Comment.add({
post_id: post,
member_id: member,
parent_id: null,
html: comment,
status: 'published'
}, options);
return model;
}
/**
* @param {string} parent - The ID of the Comment to reply to
* @param {string} member - The ID of the Member to comment as
* @param {string} comment - The HTML content of the Comment
* @param {any} options
*/
async replyToComment(parent, member, comment, options) {
await this.models.Member.findOne({
id: member
}, {
require: true,
...options
});
const parentComment = await this.getCommentByID(parent, options);
if (!parentComment) {
throw new errors.BadRequestError({
message: tpl(messages.commentNotFound)
});
}
if (parentComment.get('parent_id') !== null) {
throw new errors.BadRequestError({
message: tpl(messages.replyToReply)
});
}
const model = await this.models.Comment.add({
post_id: parentComment.get('post_id'),
member_id: member,
parent_id: parentComment.id,
html: comment,
status: 'published'
}, options);
return model;
}
/**
* @param {string} id - The ID of the Comment to delete
* @param {string} member - The ID of the Member to delete as
* @param {any} options
*/
async deleteComment(id, member, options) {
const existingComment = await this.getCommentByID(id, options);
if (existingComment.get('member_id') !== member) {
throw new errors.NoPermissionError({
// todo fix message
message: tpl(messages.memberNotFound)
});
}
const model = await this.models.Comment.edit({
status: 'deleted'
}, {
id,
require: true,
...options
});
return model;
}
/**
* @param {string} id - The ID of the Comment to edit
* @param {string} member - The ID of the Member to edit as
* @param {string} comment - The new HTML content of the Comment
* @param {any} options
*/
async editCommentContent(id, member, comment, options) {
const existingComment = await this.getCommentByID(id, options);
if (!comment) {
return existingComment;
}
if (existingComment.get('member_id') !== member) {
throw new errors.NoPermissionError({
// todo fix message
message: tpl(messages.memberNotFound)
});
}
const model = await this.models.Comment.edit({
html: comment
}, {
id,
require: true,
...options
});
return model;
}
}
module.exports = CommentsService;

View File

@ -110,6 +110,59 @@ Object {
}
`;
exports[`Comments API when authenticated Can edit a comment on a post 1: [body] 1`] = `
Object {
"comments": Array [
Object {
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"edited_at": null,
"html": "Updated comment",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"liked": Any<Boolean>,
"likes_count": Any<Number>,
"member": Object {
"avatar_image": null,
"bio": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": null,
"uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
},
"replies": Array [
Object {
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"edited_at": null,
"html": "This is a reply",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"liked": Any<Boolean>,
"likes_count": Any<Number>,
"member": Object {
"avatar_image": null,
"bio": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": null,
"uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
},
"status": "published",
},
],
"status": "published",
},
],
}
`;
exports[`Comments API when authenticated Can edit a comment on a post 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": "624",
"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 Can like a comment 1: [body] 1`] = `
Object {
"comments": Array [
@ -256,6 +309,155 @@ Object {
}
`;
exports[`Comments API when authenticated Can not edit a comment as a member who is not you 1: [body] 1`] = `
Object {
"comments": Array [
Object {
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"edited_at": null,
"html": "Illegal comment update",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"liked": Any<Boolean>,
"likes_count": Any<Number>,
"member": Object {
"avatar_image": null,
"bio": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": null,
"uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
},
"replies": Array [
Object {
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"edited_at": null,
"html": "This is a reply",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"liked": Any<Boolean>,
"likes_count": Any<Number>,
"member": Object {
"avatar_image": null,
"bio": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": null,
"uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
},
"status": "published",
},
],
"status": "published",
},
],
}
`;
exports[`Comments API when authenticated Can not edit a comment as a member who is not you 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": "631",
"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 Can not edit a comment post_id 1: [body] 1`] = `
Object {
"comments": Array [
Object {
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
"edited_at": null,
"html": "Updated comment",
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"liked": Any<Boolean>,
"likes_count": Any<Number>,
"member": Object {
"avatar_image": null,
"bio": null,
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
"name": null,
"uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
},
"replies": Array [],
"status": "published",
},
],
}
`;
exports[`Comments API when authenticated Can not edit a comment post_id 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": "326",
"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 Can not edit a comment which does not belong to you 1: [body] 1`] = `
Object {
"errors": Array [
Object {
"code": null,
"context": "You do not have permission to edit comments",
"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": "Permission error, cannot edit comment.",
"property": null,
"type": "NoPermissionError",
},
],
}
`;
exports[`Comments API when authenticated Can not edit a comment which does not belong to you 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": "269",
"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 Can not reply to a reply 1: [body] 1`] = `
Object {
"errors": Array [
Object {
"code": null,
"context": "Can not reply to a reply",
"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": "Request not understood error, cannot save comment.",
"property": null,
"type": "BadRequestError",
},
],
}
`;
exports[`Comments API when authenticated Can not reply to a reply 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": "260",
"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 Can remove a like 1: [headers] 1`] = `
Object {
"access-control-allow-origin": "*",

View File

@ -1,9 +1,10 @@
const assert = require('assert');
const {agentProvider, mockManager, fixtureManager, matchers} = require('../../utils/e2e-framework');
const {anyEtag, anyObjectId, anyLocationFor, anyISODateTime, anyUuid, anyNumber, anyBoolean} = matchers;
const should = require('should');
const models = require('../../../core/server/models');
let membersAgent, member, postId, commentId;
let membersAgent, membersAgent2, member, postId, commentId;
const commentMatcherNoMember = {
id: anyObjectId,
@ -42,6 +43,7 @@ async function sleep(ms) {
describe('Comments API', function () {
before(async function () {
membersAgent = await agentProvider.getMembersAPIAgent();
membersAgent2 = await agentProvider.getMembersAPIAgent();
await fixtureManager.init('posts', 'members', 'comments');
@ -91,6 +93,7 @@ describe('Comments API', function () {
before(async function () {
await membersAgent.loginAs('member@example.com');
member = await models.Member.findOne({email: 'member@example.com'}, {require: true});
await membersAgent2.loginAs('member2@example.com');
});
it('Can comment on a post', async function () {
@ -113,7 +116,7 @@ describe('Comments API', function () {
// Wait for the emails (because this happens async)
await sleep(100);
// Check if author got an email
mockManager.assert.sentEmailCount(1);
mockManager.assert.sentEmail({
@ -292,7 +295,7 @@ describe('Comments API', function () {
mockManager.assert.sentEmail({
subject: '🚩 A comment has been reported on your post',
to: fixtureManager.get('users', 0).email
});
});
});
it('Cannot report a comment twice', async function () {
@ -314,5 +317,180 @@ describe('Comments API', function () {
mockManager.assert.sentEmailCount(0);
});
it('Can edit a comment on a post', async function () {
await membersAgent
.put(`/api/comments/${commentId}`)
.body({comments: [{
html: 'Updated comment'
}]})
.expectStatus(200)
.matchHeaderSnapshot({
etag: anyEtag
})
.matchBodySnapshot({
comments: [commentMatcherWithReply]
});
});
it('Can not edit a comment post_id', async function () {
const anotherPostId = fixtureManager.get('posts', 1).id;
await membersAgent
.put(`/api/comments/${commentId}`)
.body({comments: [{
post_id: anotherPostId
}]});
const {body} = await membersAgent
.get(`/api/comments/?filter=post_id:${anotherPostId}`);
assert(!body.comments.find(comment => comment.id === commentId), 'The comment should not have moved post');
});
it('Can not edit a comment which does not belong to you', async function () {
await membersAgent2
.put(`/api/comments/${commentId}`)
.body({comments: [{
html: 'Illegal comment update'
}]})
.expectStatus(403)
.matchHeaderSnapshot({
etag: anyEtag
})
.matchBodySnapshot({
errors: [{
type: 'NoPermissionError',
id: anyUuid
}]
});
});
it('Can not edit a comment as a member who is not you', async function () {
const memberId = fixtureManager.get('members', 1).id;
await membersAgent
.put(`/api/comments/${commentId}`)
.body({comments: [{
html: 'Illegal comment update',
member_id: memberId
}]});
const {
body: {
comments: [
comment
]
}
} = await membersAgent.get(`/api/comments/${commentId}`)
.expectStatus(200)
.matchHeaderSnapshot({
etag: anyEtag
})
.matchBodySnapshot({
comments: [commentMatcherWithReply]
});
assert(comment.member.id !== memberId);
});
it('Can not reply to a reply', async function () {
const {
body: {
comments: [{
id: parentId
}]
}
} = await membersAgent
.post(`/api/comments/`)
.body({comments: [{
post_id: postId,
html: 'Parent'
}]});
const {
body: {
comments: [{
id: replyId
}]
}
} = await membersAgent
.post(`/api/comments/`)
.body({comments: [{
post_id: postId,
parent_id: parentId,
html: 'Reply'
}]});
await membersAgent
.post(`/api/comments/`)
.body({comments: [{
post_id: postId,
parent_id: replyId,
html: 'Reply to a reply!'
}]})
.expectStatus(400)
.matchHeaderSnapshot({
etag: anyEtag
})
.matchBodySnapshot({
errors: [{
type: 'BadRequestError',
id: anyUuid
}]
});
});
it('Can not edit a replies parent', async function () {
const {
body: {
comments: [{
id: parentId
}]
}
} = await membersAgent
.post(`/api/comments/`)
.body({comments: [{
post_id: postId,
html: 'Parent'
}]});
const {
body: {
comments: [{
id: newParentId
}]
}
} = await membersAgent
.post(`/api/comments/`)
.body({comments: [{
post_id: postId,
html: 'New Parent'
}]});
const {
body: {
comments: [{
id: replyId
}]
}
} = await membersAgent
.post(`/api/comments/`)
.body({comments: [{
post_id: postId,
parent_id: parentId,
html: 'Reply'
}]});
// Attempt to edit the parent
await membersAgent
.put(`/api/comments/${replyId}/`)
.body({comments: [{
parent_id: newParentId,
html: 'Changed parent'
}]});
const {body: {comments: [comment]}} = await membersAgent.get(`api/comments/${newParentId}`);
assert(comment.replies.length === 0, 'The parent comment should not have changed');
});
});
});