Clean up lib-comment and fix security checks on editing and deleting of comments

Geeklog-Core / geeklog

Geeklog - The Secure CMS.

https://www.geeklog.net

24 stars 19 forks source link

Clean up lib-comment and fix security checks on editing and deleting of comments #1023

Closed eSilverStrike closed 2 years ago

eSilverStrike commented 4 years ago

While fixing #1022 I notice several things that need cleaning up.

Remember these items have to work for when the comment editor is on the same page as the plugin content and not (config option comment_on_same_page).

For some reason things like editing and deleting a few extra url variables. For example here is an edit link

https://www.geeklog.net/article.php?cmt_mode=edit&cmt_cid=13335&story=20010908202400207&cmt_type=article&mode=nested&order=ASC&cpage=1#commenteditform

Since this is for a comment that has the editor on the page everything is needed except cmt_type and the story variable.

it is the same with delete and probably other actions. The only time these variables are needed is when creating a new comment.

Also things like viewing a comment directly url looks like this:

https://www.geeklog.net/comment.php?mode=view&cid=13335

Not sure why we need a mode for viewing, it should be like :https://www.geeklog.net/comment.php?cid=13335

Also clicking on the link as an anonymous user will bring up the comment editor with that comment in the actual editor. It also brings up the delete button. It doesn't works since it seems to rely on the gl token but we should also add some extra security so this does not happen (which making sure the config options comment_edit and comment_edittime still work properly). This is also with delete. Proper security is checked when it is decided to place the link to delete a comment on the page but the actual command the only security that is checked is the token.

Comment also need to make sure it's url variables are unique (see #1019) so it doesn't interfere with plugins since comments are attached to plugin items. All comment url variables should start with cmt_

Also remember we need to check edit and delete links on comment submissions and the admin lists forms as well since I believe they use the same functions.

Remember if we change any url variables that change the way a comment is linked to by an outside site we need to setup 301 redirects so the old way still works for external sites who may have already linked to the content.

eSilverStrike commented 4 years ago

Last commit only fixed Edit and delete links don't include type and sid variable anymore. Everything else still needs updating. Also if 301 redirect system setup see article.php as it should have one added for the original way articles handled multiple pages (using mode instead of page url variable)

eSilverStrike commented 4 years ago

Also should confirm comments cannot be displayed or added by visitors that do not have access to read the item (like article or staticpage) plus all the other checks required.

Maybe need to add an extra comment function that checks permissions only? Currently the comment functions for the Likes system uses PLG_getItemInfo to check read permissions as nothing will be returned if user has no access:

if (!empty(PLG_getItemInfo($type, $sid, 'url'))) {

Should also update the permission check for view mode of comment (and elsewhere as needed) so everything uses the same permissions check.

eSilverStrike commented 4 years ago

The last commit fixed security issues with comments and some cleanup) and issue #1035 (new functions to fix comment moderation and deleting existing comments)

Many changes to lib-comments.php (while trying to keep everything backwards compatible) including (I am probably missing a few):

New commentlogin.thtml template file displayed when $_CONF[‘comment_on_same_page’] = true and $_CONF[‘commentsloginrequired’] = true
Fixes to PLG_commentDelete to allow Boolean to be returned so Admin Comments list could delete multiple comments at once. Old version of plugindeletecomment foo will depreciate by Geeklog v3.0.0
Updated Articles, Staticpages, and Polls to support new version of PLG_commentDelete
Fixed Polls to display message when comment goes to submission queue
SPAM-X Mass Delete Comments will now delete more than one
Moderation Submission Queue for Comments updated to just use CMT_approveModeration which calls CMT_rebuildTree and PLG_approveCommentSubmission (this is new) so plugins like articles can do some extra work
CMT_rebuildTree fixed to include type along with id
Fixed $_CONF[‘comment_edit’] so Anonymous users could not edit comments and regular users from editing any comments and not just their own for a certain time. Users also cannot delete their own comments
If fields missing on comment edit form and message is displayed about it to user, comment preview is included so anchor point still works for when comments on the same page
When Comment is previewed, edited, deleting or submitted extra security checks are done. If user is not supposed to be doing this due to lack of permissions or comment not exisit then 404 error happens. Also now check if user has read permissions on item comment is form
Fix for comment notifications when moving from commentsubmission table to comments table

eSilverStrike commented 4 years ago

@mystralkk The last commit for this issue fixed a lot of things and tightened up security for the comment form. I tried to test all parts of the comment system. I think it is mostly done but I still need to retest everything (it takes a while). If possible can you test things as well since I am sure I missed a few things.

This is very time consuming and difficult to test since there are so many different config options and the comment editor works on own page and through content items page. This is my testing process:

Need to test all cases with Anonymous User, Regular User, and User with ‘comment.moderate’ permissions (like root user).

Need to test Add, Preview, Edit and Delete of Comments and Comment Submissions. As well as comment order and mode on comment bar. Also need to make sure this stuff works when comments span multiple pages or for example when articles also span multiple pages (and when comments do as well). Need to do security checks here as well. For example making sure not only the edit link is not shown for a user that can edit but also taking that edit link (or preview, or submit link) and trying it with a different user who is logged in to see if they have access)

Need to test Admin Comment Moderation Form, Comment Admin List, Article Comments, Polls Comments, Staticpage Comments

Need to test with all combinations of these config options:

$_CONF[‘commentsloginrequired’] – Must be a user to post a comment $_CONF[‘commentsubmission’] – All comments go through submission queue unless user has ‘comment.submit’ permission $_CONF[‘comment_edit’] – Allows user to edit own comment for a certain time period $_CONF[‘comment_on_same_page’] – IMPORTANT – Comment Editor Appears on same page of content or on own form. $_CONF[‘show_comments_at_replying’] – Should only affect things when $_CONF[‘comment_on_same_page’] = false

Need to make sure if Comments are Closed for a Plugin Item that Users that don't have edit access cannot add, new comments or edit them.

eSilverStrike commented 4 years ago

@mystralkk - Still working on this as I found an issue so don't test yet.

I have to add another comment plugin API to determine if user has access to add or edit comments for a particular item. I was using PLG_getItemInfo which if returned nothing then I knew that the user didn't have read access to the item.

The problem is that I forgot about the commentcode that plugins use to disable or close comments for individual articles (or pages, etc..). So a comment can be viewable but since closed cannot edit or add new ones (unless Admin).

I will still use PLG_getItemInfo as a backup option if the plugin doesn't have the new API as it is better than nothing and we need backwards compatibility (until Geeklog 3.0.0)

eSilverStrike commented 4 years ago

Okay mainly with last commit added a plugin api so comments library had a way to check properly what a user can do. See PLG_commentEnabled

Added PLG_commentEnabled API function which allows comment library to find out if comments are enabled and access is allowed for a user by the plugin item. This will be required as of Geeklog v3.0.0 but if missing will fall back to PLG_getItemInfo which works for the most part but only knows if user has access to item and not if comments are closed or disabled for the item.
So if user can see comments for the plugin item and they are not closed they will be able to add new comments (and edit or delete if have permissions).
Likewise if the user doesn’t have access to preview, submit, delete, edit something (or it doesn’t exist) a 404 error will be returned (as long as PLG_commentEnabled is supported by the plugin)
Added some messages to comment bar to indicate if login is required to post and if Comments are closed.
Comment Post button will take into account if posts are not allowed, it will disappear
Admins will always have access though to post, edit, and reply even if comments are closed
Also added some better checks on variables so the comment library does not assume good data. If bad data found 404 error happens

mystralkk commented 4 years ago

Thanks for the update!

2020年2月16日(日) 3:51 Tom notifications@github.com:

Okay mainly with last commit added a plugin api so comments library had a way to check properly what a user can do. See PLG_commentEnabled

Added PLG_commentEnabled API function which allows comment library to find out if comments are enabled and access is allowed for a user by the plugin item. This will be required as of Geeklog v3.0.0 but if missing will fall back to PLG_getItemInfo which works for the most part but only knows if user has access to item and not if comments are closed or disabled for the item.

So if user can see comments for the plugin item and they are not closed they will be able to add new comments (and edit or delete if have permissions).

Likewise if the user doesn’t have access to preview, submit, delete, edit something (or it doesn’t exist) a 404 error will be returned (as long as PLG_commentEnabled is supported by the plugin)

Added some messages to comment bar to indicate if login is required to post and if Comments are closed.

Comment Post button will take into account if posts are not allowed, it will disappear

Admins will always have access though to post, edit, and reply even if comments are closed

Also added some better checks on variables so the comment library does not assume good data. If bad data found 404 error happens

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Geeklog-Core/geeklog/issues/1023?email_source=notifications&email_token=AAPMBELBAMPSEIYTYZJQIZLRDA2UVA5CNFSM4KGZ5TG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL3UJ2Y#issuecomment-586630379, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPMBEOFZNW6N6RMKPARHC3RDA2UVANCNFSM4KGZ5TGQ .

-- mystral-kk http://mystral-kk.net/ mystralkk@gmail.com

eSilverStrike commented 4 years ago

Had to modify the new PLG_commentEnabled. It now returns the comment code to the plugin item. This comment code is based not only the comment code of the actual plugin item but the access the user has to the item. This means things can happen for comments that are not only enabled but closed as well (for example: Report Abuse can be done for both. An edit can happen only when comments are enabled unless the user has Comment Admin permissions).

Also added some Comment Definitions in lib-comment for the comment codes since their numeric values are difficult to remember (ie enabled = 0)

// Possible Comment Codes for Plugin Items
define('COMMENT_CODE_DISABLED', -1); // Comments will not display and no one can add, edit, etc...
define('COMMENT_CODE_ENABLED', 0); // Comments displayed and can be added, edited, etc... if user has access
define('COMMENT_CODE_CLOSED', 1); // Comments displayed but can not be added or edited by any user except admins

I didn't go through all the plugin code to update it to use the new definitions since the lib-comment is not always loaded and some bugs could happen if we didn't test everything again. This should be done at some point in a later version of Geeklog (ie when this issue is worked on again)

eSilverStrike commented 4 years ago

So taking all these changes into account if plugins want to support the latest comment/plugin API they will need to update the functions for:

PLG_commentDelete (new $returnBoolean field added) PLG_getCommentUrlId (new $id field added) PLG_commentEnabled (New Function) PLG_approveCommentSubmission (New Function, only required if have submissions and need to do something after new comment approval)

Here is the Downloads plugin commit with the update that others can use as an example: https://github.com/Geeklog-Plugins/downloads/commit/c7f2ed17b9ef83f2edd68de195193070ea7a8d1d

eSilverStrike commented 4 years ago

So the main thing left to do here for cleanup is updating the names for the comment variables:

Comment also need to make sure it's url variables are unique (see #1019) so it doesn't interfere with plugins since comments are attached to plugin items and the comment editor can appear on the same page as the plugin content. All comment url variables should start with cmt_ even if the comment on the same page config setting is set to false.

If we change any url variables that change the way a comment is linked to by an outside site (ie the Permanent Link) we need to setup 301 redirects so the old way still works for external sites who may have already linked to the content. This will also help search engines not finding duplicate content

eSilverStrike commented 2 years ago

Closing this since security fixes are done. Moved url comment variable renaming to #1019