Regression: Anonymous users can post comments in the name of registered users

Hah, wow.

I raised this quiestion somewhere in issue about comment form but this was ignored so following

If I remember correctly, 'post comments' is the base permission. 'post comments without approval' does nothing if the user role does not have 'post comments'.

Attached patch fixes this issue by checking the "is_anonyous" property set in the form declaration before assigning a uid.

Obviously test coverage is lacking here, so this should go back to needs work if/when it gets fixed.

Added empty() to conditional for consistency and to avoid a fatal error.

First condition is entirely different in #8 and #9 ;)

Since I believe that comment module itself is defining ->is_anonymous, we don't really need the empty() here. Every module should be able to trust that the properties it sets itself actually exist.

However, we are additionally missing adjustment for #6.1, and we are missing tests to prevent this bug in the future.

OK, reverted to patch #8, updated for #6.1 ("naive approach"), still needs tests.

+++ modules/comment/comment.module	12 Jul 2010 18:39:38 -0000
@@ -2057,36 +2057,34 @@ function comment_form_validate($form, &$
+    if ($form_state['values']['mail']) {
+      if (!valid_email_address($form_state['values']['mail'])) {
...
+    if ($form_state['values']['homepage']) {
+      if (!valid_url($form_state['values']['homepage'], TRUE)) {

Next to tests, I don't understand why these conditions are written out separately...?

Powered by Dreditor.

re #14:
this patch changed the indentation of several nested conditionals, but didn't try to optimize them (to avoid killing kittens).

In response to #7 comment. The actual issue for this is #438224: "Post comments without approval" permission name is completely misleading, though that issue does not affect the security. :-)

This module does not appear on the D7 beta blocker section of the community initiative page. Should it be?

#12: 845774-anonymous-comment-masquerade.patch queued for re-testing.

#12: 845774-anonymous-comment-masquerade.patch queued for re-testing.

This is a pretty nasty bug. How are we doing on those tests? :)

Also, can someone explain why the logic here differs from http://api.drupal.org/api/function/comment_validate/6 ? Don't we basically want a straight-over port?

It'd be interesting to figure out how this bug got introduced. Hrm.

Removing tag.

No response from comment #17.

This is quite a beta blocker actually. It's a big security risk.

Rerolled #12 with the following test:

    // Post anonymous comment using an existing username.
    $masquerade_comment = $this->postComment($this->node, $this->randomName(), $this->randomName(), array('name' => $this->admin_user->name));
    $masquerade_comment = comment_load($masquerade_comment['id']);
    $anonymous_user = drupal_anonymous_user();
    $this->assertEqual($masquerade_comment['uid'], $anonymous_user->uid, t('Anonymous comment correctly assigned to anonymous user.'));

Does this test look sufficient?

Also, the title was a little bit misleading. Anonymous can never become an auth user (as the term "masquerade" implies), but can post a comment that ends up being owned by an auth user.

Let's fix this properly, please. This code dates back to Drupal 4.7... http://api.drupal.org/api/function/comment_validate/4.7

Didn't touch the tests.

I applied this patch and kicked the tires a bit ... when I try and use a registered user's name I (correctly) get an error message, but I also get two different fields being highlighted.

This one should pass.

@bleen18: Are you sure that this odd behavior doesn't exist without this patch, too?

I'm not sure how else to produce an error on the "Your Name" field without this patch...

@sun - one can masquerade before your patch is applied so no error is thrown at all. With your patch, apparently both name fields in 2 different forms are highlighted in red. Thats an unrelated FAPI bug that we are uncovering?

Yes, that's correct. To manually replicate that unrelated Form API bug in current HEAD:

0) Allow anonymous users to "post comments".
1) Configure Article content type to require contact information from anonymous.
2) Create an article, with comments open.
3) Log out, go to the comment form, don't enter a name, submit.

Result identical to screenshot in #27:

I believe that the patch in #766146: Support multiple forms with same $form_id on the same page contains a single line change that fixes this bug (though the scope of the issue is larger).

Effectively making this patch RTBC, perhaps?

For reference, this is the single line change from over there:

+++ includes/form.inc	27 Jul 2010 22:03:26 -0000
@@ -3403,7 +3415,7 @@ function _form_set_class(&$element, $cla
-  if (form_get_error($element)) {
+  if (!empty($element['#validated']) && form_get_error($element)) {
     $element['#attributes']['class'][] = 'error';
   }

Tested manually. Successfully fixes that bug. Unrelated to this patch, rather related to aforementioned issue + patch, but could as well be extracted into an own issue.

Sounds like a plan.

Why this patch does not contain tests?

Highlighting of username in login block still there - screenshot attached

@andypost: Not sure what you mean. Patch in #28 adds test assertions to not break this in the future. #31 clarified that duplicate form error highlighting is unrelated to this issue.

@sun, sorry missed this hunk

So, once this patch is committed, if an anonymous user tries to post a comment under the name of Mike (which is his name), but there is a registered user named Mike, then he can't? That seems like a strange way to handle it to me since the anonymous commenter isn't "masquerading" as the registered user...he's just entering his name. My expected behavior would be the let him/her use whatever name is desired and validate on email addresses instead. Am I the only one?

I never realized it was like this in D6 as well.

@mcrittenden: That's the way how it's supposed to work. We can revisit the behavior for D8.

A couple points ...

1) RTBC ++ ... I'm satisfied with sun's response to the problem I pointed out in #27 (that issue that can be handled in #766146: Support multiple forms with same $form_id on the same page) AND the code works as advertised.

2) @mcrittended: In your example it's true that "Mike" is not being malicious, but it isn't about "Mike's" intent, its about what other users will see. Imagine if I could comment on d.o. as "webchick" - that would cause quite a lot of confusion. The "not verified" message would help, but massive confusion would still abound. That said, if you have a good alternative you should open a new (D8) issue.

I'm not sure about this patch. We used to put "(not verified)" behind the name of anonymous users to address this problem. In other words, this could be made a theme issue.

Yeah, I'm fine with marking this as 'By Design'. The 'not verified' is clear enough, even in the 'webchick' case described earlier.

History note: the 'not verified' text refers (i think) to the fact that the email address has not been verified via clicking on a link in the welcome email. This used to be mandatory for all accounts, but we made verification optional in D6 ... I can't really think of better text than 'not verified' though.

@Dries: That was always the case for anonymous users and is not changed by this patch. We also used to prevent anonymous users to post/incarnate as a registered user, which is what this patch is preventing.

That said, Comment and Node module and other parts of core are not prepared for anonymous author names equaling registered user names. Various locations will grant wrong permissions, display bogus links, or otherwise confuse the hell out of the system. That's something we can revisit and improve for D8.

For D7, we need to restore the expected functionality (which seems to be the same at least since Drupal 4.7).

In other words: I agree that we may want to reconsider this behavior and validation, but until #40 and August 9, 2010, this issue is about fixing a security issue, not about doing an unplanned API change that affects many parts of core and contrib.

In addition, I wouldn't entirely agree that this is a theming issue... I'd ask for links to some popular example sites where comments from authenticated users and anonymous users are displayed sufficiently differently, so that you immediately recognize that comment #10 by "Dries" is not the same author as comment #12, equally, by "Dries", and which of both is tha real Dries. Such a change would require further in-depth discussion and, consequently, changes to all core themes.

More precisely, with that suggestion, we're scratching the doomed and evil surface of #585838: Consider a generic $entity->user property

We'd need a lot of new tests to ensure that every core module can cope with anonymous usernames potentially being identical to registered usernames.

Someone with proper permissions, add this too:

http://drupal.org/community-initiatives/drupal-core

#28: drupal.comment-author-validate.28.patch queued for re-testing.

Assigning to me to better keep track of this issue.

This patch is still ready to go in. Review issues mentioned in #40 and #41 are bogus or talk about API/behavior changes that are not related to this patch and should be discussed in a non-critical task or feature request.

Patch looks fine to me as well. I think at this stage we should restore the regression from D6 and leave changing the behaviour to another issue.

Comin' at you LIVE from Drupalcon Copenhagen Core Developer Summit!

Checked this over again and looks good, comes with test coverage, and I verified that this restores the behaviour that Drupal 6 had. I think there was confusion on Dries's part because this issue has somehow become the lightning rod for "Every remotely janky thing that Comment module does," which is quite a large list. ;)

Drumrolllll... Committed to HEAD!

fixed? ;)