Remove CSRF vulnerability from comment module
GiorgosK - May 30, 2006 - 12:10
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | needs work |
Description
could be offered as a link where the comment is displayed (next to the "delete edit reply" links)
Normally one has to see the content of the comment in order to approve it so one has to edit it first and then check publish and then save the comment. It would be very convenient to have an "approve" link next to the "delete edit reply" links for fast approval.
Sorry if Project and Component are not the appropriate ones.
#1
This feature request is still valid in Drupal 6 beta2. To approve an individual comment you still have to click the "Edit" link, then click on the "Administration" fieldset title to open it, click the "Published" radio button, then scroll to the bottom of the page and click the "Submit" button. Phew!
So a one-click approval link would really be handy, if it's possible.
Redirecting to the comment.module issue queue. I guess this is for 7.x-dev because of the code freeze?
#2
After using my Drupal blog I have noticed that this is a feature that would be very useful. I searched and found this issue so I have attached a patch to it rather than starting a new issue.
This is my first patch related to the comment module. It may be a bit rough around the edges.
#3
#4
Advised by chx to use comment_load.
#5
chx reviewed in IRC.
This includes a modified test to take this into account.
#6
Excellent patch. With tests and everything. :-)
#7
Great stuff!
+ $items['comment/approve'] = array(+ 'title' => 'Approve to comment',
sounds strange, like granting a permission to a user. Isn't it 'Approve a comment' ?
#8
Yes, I think I was trying to write "the."
#9
still applies with offset.
#10
Committed to CVS HEAD. Marking fixed ... thanks.
#11
Sorry, this is a CSRF vulnerability. Either we need a confirm form or some sort of cron_key-like mechanism to allow this sort of one-click convenience to work without opening a security hole.
#12
Not following you.
Need to be logged in with correct privileges:
<?php$items['comment/approve'] = array(
'title' => 'Approve a comment',
'page callback' => 'comment_approve',
'page arguments' => array(2),
'access arguments' => array('administer comments'),
'type' => MENU_CALLBACK,
);
?>
And link won't even be displayed unless you can administer comments anyway as they are considered unpublished.
If anything now that I look at it should the url be 'comment/approve/%' ?
#13
#12 - @see http://en.wikipedia.org/wiki/CSRF please. No actions should ever be taken through an insecure GET request.
#14
Believe I have it working, but I have to leave so post later.
#15
This should do it.
Updated test so that it take the confirmation form into account and it passes. Had 1 fail (as it should) before.
#16
Why not use %comment instead of just %? Then you get comment_load() called automatically.
#17
Sure, never used it before. Was just adding % so that it would be consistent and wouldn't allow comment/approve to be called.
Patch includes minor change.
#18
Still applies, ping.
#19
The patch looks good, except that these lines are completely unneeded, because confirm_form() has already set
$form['#redirect']:<?php+ $comment = comment_load($form_state['values']['cid']);
+ $form_state['redirect'] = "node/$comment->nid";
?>
#20
Checked into, but that doesn't seem to be the case.
* @param $path* The page to go to if the user denies the action.
* Can be either a drupal path, or an array with the keys 'path', 'query', 'fragment'.
And it never sets #redirect, as it also doesn't work if I remove the lines in question.
#21
Patch still applies and ran comment test which returned all passes.
#22
The last submitted patch failed testing.
#23
Re-rolled, DB TNG was the cause of failed to apply.
#24
"You can unpublish comment" should be "You can unpublish this comment". Otherwise this looks good - except I'd rather see us use tokenised links to avoid CRSF for one click operations rather than confirm forms. That probably needs centralising though.
#25
Addressed #24.
#26
Sending for re-test.
#27
Why:
+ else {+ drupal_set_message(t('The comment you are approving does not exist.'), 'error');
+ drupal_goto();
+ }
and not a 404?
Otherwise looks good.
#28
The last submitted patch failed testing.
#29
@webchick rediscovered the CSRF vulnerability in #606608: Use proper menu router paths for comment/*. We should probably consolidate efforts here, but note there is a more up-to-date patch at http://drupal.org/node/606608#comment-2240294
She also pointed out that the access callback for this link is not correct (you should not be able to approve a comment that is already approved).
I also agree with @catch - it seems like the entire reason this was introduced was to have a convenient one-click approve link, so adding a confirmation form hurts that experience. We don't really need a confirmation form, just a token. Seems like we could get away without centralizing it - this isn't that common of link that people will be generating outside of a few special modules, and it's not hard for them to use the same token generation code as the comment module would use (and they'd learn to do that pretty quickly, since if they didn't, their links wouldn't work :)