Make WordPress Core

Opened 16 years ago

Last modified 8 weeks ago

#10931 assigned enhancement

Verify Comment Email Addresses of Registered Users

Reported by: mtdewvirus's profile mtdewvirus Owned by: adamsilverstein's profile adamsilverstein
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Comments Keywords: needs-docs early has-patch has-unit-tests changes-requested
Focuses: Cc:

Description

When leaving a comment with an email address of a registered user, WordPress should force the visitor to login or change the email address in the comment form.

Anyone can impersonate a blog's user if they know the user's email address.

Attachments (5)

wp-comments-post.php.diff (713 bytes) - added by scribu 16 years ago.
wp-comments-post.2.diff (668 bytes) - added by ShaneF 16 years ago.
10931.diff (1.4 KB) - added by greuben 15 years ago.
10931-2.diff (2.1 KB) - added by greuben 15 years ago.
10931-testing-PR8114.webm (826.4 KB) - added by audrasjb 11 months ago.
Screen capture of PR8114 result: works fine.

Download all attachments as: .zip

Change History (82)

#1 @filosofo
16 years ago

Duplicate of #8646 ?

#2 @Denis-de-Bernardy
16 years ago

Yeah. I closed the old one. See #8646, #2543, #1598.

#3 @scribu
16 years ago

  • Milestone changed from Unassigned to 2.9
  • Version set to 2.8.4

#4 @Denis-de-Bernardy
16 years ago

  • Version changed from 2.8.4 to 1.5

#5 @scribu
16 years ago

  • Keywords changed from comments,email to comments email
  • Owner set to scribu
  • Status changed from new to accepted

#6 @scribu
16 years ago

  • Keywords has-patch added; comments email removed

#7 @ryan
16 years ago

How about get_user_by('email', $comment_author_email)?

#8 @ShaneF
16 years ago

  • Keywords commit added

Works as designed. Changed format to of what ryan had above.

#9 @ryan
16 years ago

Having a means to go back would be nice. Relevant discussion here:

http://core.trac.wordpress.org/ticket/4332#comment:15

#10 @ryan
16 years ago

Maybe this should wait until we can offer a login form instead of just a die or go back. See #11172 for the planned login_form() function which would help here.

#11 @ryan
16 years ago

  • Milestone changed from 2.9 to 3.0
  • Type changed from defect (bug) to task (blessed)

Postponing to 3.0 where we're planning to standardize the login and comment form stuff. That work will make it much easier to offer a good interaction here.

#12 @scribu
16 years ago

  • Cc scribu@… added

#13 @Denis-de-Bernardy
16 years ago

  • Version changed from 1.5 to 2.8.4

#14 @mattrude
16 years ago

  • Cc m@… added

#15 @westi
16 years ago

  • Cc westi added
  • Keywords has-patch commit removed

I agree that it will be better for this to take advantage of #10931 - going to remove commit/has_patch for now as I think it is better to have an implementation which offers a login form so people can continue on and actually comment.

#16 @dd32
16 years ago

I agree that it will be better for this to take advantage of #10931

I agree as well, Without it, This ticket wouldnt exist! :)

#17 follow-up: @hakre
16 years ago

I think westi had some other ticket in mind... but which one?

#18 in reply to: ↑ 17 @westi
16 years ago

Replying to hakre:

I think westi had some other ticket in mind... but which one?

I did #11172 - i.e. display a login form.

#19 follow-up: @rmccue
16 years ago

  • Cc me@… added
  • Keywords needs-patch added

We have a login form thanks to #11172 now, so this ticket appears to be ready for a proper patch.

#20 in reply to: ↑ 19 @nacin
16 years ago

  • Milestone changed from 3.0 to 3.1

Replying to rmccue:

We have a login form thanks to #11172 now, so this ticket appears to be ready for a proper patch.

Too late for 3.0 though. Moving to 3.1.

Remains a blessed task.

#21 @scribu
16 years ago

  • Owner scribu deleted
  • Status changed from accepted to assigned

If I don't come up with an updated patch, go on without me... :)

#23 @mdawaffe
16 years ago

This solution is incomplete. If we're going to prevent impersonation, we need to implement CSRF protection for all logged in commentors. The patch on #13791 does this. The proposed code there is hook based, so it's all configurable/extendable. It's also more complicated.

If we go with this method, we'll need to pull in the CSRF stuff from #13791.

#24 @nacin
15 years ago

  • Milestone changed from Awaiting Triage to Future Release
  • Type changed from task (blessed) to enhancement

#25 @scribu
15 years ago

Instead of preventing the user from commenting, I think a better solution would be to accept the comment, but send it to the moderation queue.

There, the comment could have a notification displayed, to the effect of "Possible impersonation".

#26 @scribu
15 years ago

This could be done by storing a custom field on the comment.

#27 @westi
15 years ago

  • Keywords 3.2-early added
  • Milestone changed from Future Release to Awaiting Triage

I'd like us to consider implementing this in 3.2

#28 @westi
15 years ago

  • Milestone changed from Awaiting Triage to Future Release

Use Future Release to constrain 3.2-early

@greuben
15 years ago

#29 @greuben
15 years ago

  • Keywords has-patch added; needs-patch removed

The patch only moves the comment to moderation if it is not spam and email is registered one.

@greuben
15 years ago

#30 @scribu
15 years ago

  • Keywords changed from has-patch, 3.2-early to has-patch 3.2-early

Found out there's a plugin for this: http://wordpress.org/extend/plugins/impostercide/

#31 follow-up: @dd32
15 years ago

I don't think moving to spam is the ideal method here, Sure, some people will be spamming or attempting to use someone elses details.. but there's also the case of people who are unable to differentiate logging in, and commenting anonymously.

My idea workflow would be for the comment to be marked pending, followed by the commentor being given a login screen, if they can login as that user, the comment is marked as them (and set to approved) and they're redirected back to the posting..

#32 in reply to: ↑ 31 @greuben
15 years ago

Replying to dd32:

I don't think moving to spam is the ideal method here, Sure, some people will be spamming or attempting to use someone elses details.. but there's also the case of people who are unable to differentiate logging in, and commenting anonymously.

My idea workflow would be for the comment to be marked pending, followed by the commentor being given a login screen, if they can login as that user, the comment is marked as them (and set to approved) and they're redirected back to the posting..

10931-2.diff does not move to spam, it moves the comment to moderation queue unless Akismet( or other spam plugins ) mark the comment as spam...

#33 follow-up: @dd32
15 years ago

Sorry, misread the patch as spam rather than pending/moderation queue.

I do think that giving the user some kind of feedback of "Hey, your comment has been sent to the queue due to the email address being in use.." would be beneficial to many users who might not "get it"

#34 in reply to: ↑ 33 @greuben
15 years ago

Replying to dd32:

I do think that giving the user some kind of feedback of "Hey, your comment has been sent to the queue due to the email address being in use.." would be beneficial to many users who might not "get it"

For that, we can set
$comment_approved = -1 or 2;

and comments loop in themes (or wp_list_comments) should do

if( $comment_approved = -1 or 2 ){
echo "blah blah...";
}

#35 @DrewAPicture
14 years ago

#20165 Closed as dup/related.

#36 @jane
14 years ago

  • Keywords needs-refresh added; 3.2-early removed

I would much prefer to force a login than push to moderation.

#37 @nacin
14 years ago

One thing to keep in mind; if a user is suspended/"deleted" or not yet activated, or has lost their password, forcing a login may prevent a comment from occurring temporarily or permanently.

Also, forcing a login here is actually pretty difficult from a technical perspective.

I think a push to moderation would be lame as well. Just pointing out some potential challenges.

#38 @jane
14 years ago

@nacin: I would also be fine with this being in a plugin. :)

#39 @Ipstenu
14 years ago

  • Cc ipstenu@… added

#40 @mark-k
13 years ago

It is not only the impersonation possibility that is problematic, but also that the end result of commenting while logged in and while logged off might be different. When logged off author stylig will not be applied and the comment author URL will not be set to the website field in the user's profile.

Why not have something like

if user not logged in but email matches an active user {
    store comment in spam queue 
    dispatch cron event to be executed an hour later
    redirect to login form with redirect_to set to an admin URL in which the comment can be approved
    once approved duplicate the comment as if it was submitted by the user while he is logged in, remove the original from spam and process the new one
    then redirect to the post in which the comment was made.
}

in the cron event {
  if comment is still marked as spam send a mail to its author telling him to contact the admin about approving that comment
}

Comment initialy marked as spam since the damage of impersonation is probably higher then the damage of one comment being lost.

#41 @cais
13 years ago

  • Cc edward.caissie@… added

#42 @chriscct7
10 years ago

  • Version changed from 2.8.4 to 2.8

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


10 years ago

This ticket was mentioned in Slack in #core by kraft. View the logs.


10 years ago

This ticket was mentioned in Slack in #core-comments by boone. View the logs.


10 years ago

#46 @playjoy
8 years ago

I wanted to report this issue (for bbPress).
I'm amazed the issue still exists after 8 years it was first reported...

#47 @SergeyBiryukov
6 years ago

#49527 was marked as a duplicate.

#48 @bookdude13
6 years ago

Renewing interest in this. The main issues still to handle appear to be:

  • How to deal with expired nonces in cached pages
  • How to handle comments by anonymous or logged-out users (are they treated as anonymous, or are they treated as "normal" after a successful login?)

For expired nonces, one of these solutions could be explored.

For comments by logged-out users, some type of caching, queuing, or extra verification before the post seems to be the consensus?

#49 @farhadvn
5 years ago

i used a function for this problem! Of course, it may not be up to WordPress standards, but I hope it helps

function useremailcheck($approved){
 // example function
$email = filter_var($_POST["email"],FILTER_SANITIZE_EMAIL) ?: null;
// this SANITIZE is after wordpress validation, so this filter is not make sense, however wordpress devs can delete or replace it with a standard wordpress var.
if(is_user_logged_in() === false
// check if 1. user is not loggedin
 && isset($email)
 // 2. email is set
&& get_user_by("email",$email) !== false
// 3. we have an user with this email
&& $approved != "spam"
// 4. not marked as spam before
){
 $approved = 0;
// Force pending even if comments are open to all
 }
  return $approved;
}

add_action("pre_comment_approved","useremailcheck",999);

#50 @azaozz
14 months ago

  • Keywords needs-patch added; has-patch needs-refresh removed
  • Milestone changed from Future Release to 6.8
  • Version 2.8 deleted

Lets revisit this for 6.8.

Thinking that the idea to keep comments from existing users that are not logged-in in the moderation queue seems best. Such comments can be labelled in a specific way on the Comments screen so they would draw more attention.

One caveat with this idea may be comments by users that cannot moderate comments. In this case they may have to ask somebody else to approve their comments.

Alternatively the approach from https://core.trac.wordpress.org/attachment/ticket/10931/wp-comments-post.2.diff makes the most sense as it deals with this case as a "commenting error".

This ticket was mentioned in PR #8114 on WordPress/wordpress-develop by @rinkalpagdar.


11 months ago
#51

  • Keywords has-patch added; needs-patch removed

Fixes #10931

@audrasjb
11 months ago

Screen capture of PR8114 result: works fine.

#52 @audrasjb
11 months ago

  • Keywords 2nd-opinion added

Hello there,

I attached a screencast of PR8114 above. At a glance, it do was it should do.

Yet I have two proposals:

  1. I think we should remove the "or use a different email address" part of the message to avoid encouraging registered users to just remove one character from the email to send their comment with having to log in. Yep: most people are lazy :)
  2. Should we prevent comments made with registered emails from being in the moderation queue? While I think it would sound logical given we want them to log in first, but I don't want us to facilitate spamming, so this should be tested with all the way people car send comments and not only the interface.

What do you think?

#53 @audrasjb
11 months ago

  • Keywords needs-unit-tests added

AH. Also we'll probably need some unit tests for this PR @rinkalpagdar.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


11 months ago

#55 @audrasjb
11 months ago

  • Keywords needs-docs added

#56 @johnbillion
11 months ago

  • Keywords needs-patch added; has-patch 2nd-opinion removed

The approach in PR 8114 goes against what several commenters on this ticket have indicated is their preference (including most recently @azaozz), which is to push the comment into the moderation queue rather than blocking it entirely. The PR are also introduces another means for an attacker to attempt to discover whether any given email address belongs to a registered user. This is technically not a functional change because it can be performed via wp-login.php, but it's definitely less than desirable. Placing such a comment into the moderation queue, along with an explanation of why it's there, is much preferable.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


10 months ago

#58 @audrasjb
10 months ago

  • Keywords early added
  • Milestone changed from 6.8 to 6.9

Moving to milestone 6.9 as this ticket lost its momentum and didn't get a new patch. Adding early keyword too.

This ticket was mentioned in PR #8443 on WordPress/wordpress-develop by @ankitkumarshah.


9 months ago
#59

  • Keywords has-patch added; needs-patch removed

Trac Ticket: #10931

#60 @adamsilverstein
9 months ago

A unit test would be nice here.

#61 @adamsilverstein
8 months ago

  • Owner set to adamsilverstein

#62 @adamsilverstein
6 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#63 @adamsilverstein
6 months ago

  • Keywords commit added

Thanks for the PR and unit tests @ankitkumarshah; looks good to me. Unless anyone raises outstanding concerns, I'm going to get this committed.

@ankitkumarshah commented on PR #8443:


6 months ago
#64

Hi @adamsilverstein,
Thank you for the review. I have made the necessary changes. Please review the PR at your convenience. Thank you. 🙌

#65 @ankitkumarshah
6 months ago

Hey @adamsilverstein,
Thank you for the feedback. I’ve made the necessary changes as requested. Please feel free to let me know if there’s anything else you’d like me to address.

Last edited 6 months ago by ankitkumarshah (previous) (diff)

#66 @rollybueno
5 months ago

  • Keywords needs-testing added; commit removed

#67 @gulamdastgir04
5 months ago

I have tested this PR in WordPress Playground:

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8443

Environment

  • WordPress: 6.9-alpha-20250606.060013
  • PHP: 8.3.0-dev
  • Server: PHP.wasm
  • Database: WP_SQLite_Driver (Server: 5.5 / Client: 3.40.1)
  • Browser: Chrome 138.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

#68 @rollybueno
5 months ago

Reproduction Report

Description

This report validates whether a guest commenter can impersonate a registered user by using their email address in the comment form.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Query Monitor 3.18.0
    • Test Reports 1.2.0

Actual Results

A comment submitted by a guest using the email of the registered administrator is accepted without requiring login, allowing impersonation.

✅ Error condition occurs (reproduced).

Additional Notes

  • ❗❗❗ I have tested this using Author Role but having difficulty to reproduce first, turns out I only able to reproduce when impersonating admin email, but it failed on Author role.
  • I only found out after going through on each roles.
  • No warning or login prompt is shown when the email matches administrator email.

Supplemental Artifacts

Admin email:
https://i.imgur.com/lApDCVR.png

Comment passing through when using admin email. This without going to the moderation queue first. This is the main bug:
https://i.imgur.com/mxBXB60.png

Comment blocked for moderation when using Author role:
https://i.imgur.com/Yk6oXbH.png

#69 @rollybueno
5 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8443

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Query Monitor 3.18.0
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

Additional Notes

Supplemental Artifacts

✅ Re-using administrator email since I was able to reproduce this earlier:
https://i.imgur.com/olOPxrU.png
✅ Test the test unit. I changed the role with the default WP roles and re-run on each update:
https://i.imgur.com/0AG7MVX.png
✅ Confirming that I get same result of before and after video screencast from https://github.com/WordPress/wordpress-develop/pull/8443#issue-2892040401

#70 @rollybueno
5 months ago

  • Keywords needs-testing removed

Tested this using latest patch from https://github.com/WordPress/wordpress-develop/pull/8443/, it's good to go. Hey @adamsilverstein, you can re-add the commit keyword.

#71 @mindctrl
5 months ago

A concern I have with the current PR is that there is nothing in the wp-admin UI informing the admin/moderator that the comment in the queue was submitted by a logged out user who provided the email of an existing user.

The PR adjusts the email text via the comment_moderation_text filter, but if email notifications are disabled then the admin gets no notification at all of the potentially problematic nature of the comment.

#72 @oglekler
4 months ago

I share concerns with @mindctrl

I wonder if the more correct approach is to ask the user to log in and then return the user to the comment form. This way, we will not need to worry if the comment user is genuine or not. Another option is to add comment meta with the comment_moderation_text, but in this case it will be easy to miss, so I believe offering authorization will be the right approach, it will stop bogus comments from legitimate users and will be more convenient for moderation purposes.

#73 @adamsilverstein
4 months ago

  • Keywords changes-requested added

I wonder if the more correct approach is to ask the user to log in and then return the user to the comment form.

That sounds right to me @oglekler and aligns with the original ticket request:

When leaving a comment with an email address of a registered user, WordPress should force the visitor to login or change the email address in the comment form.

@ankitkumarshah are you able to update your PR with this suggested change?

This ticket was mentioned in Slack in #core by wildworks. View the logs.


2 months ago

#75 @wildworks
2 months ago

This ticket was featured on today's 6.9 Bug Scrub.

@ankitkumarshah, do you have the bandwidth to refresh the PR?

This ticket was mentioned in Slack in #core by welcher. View the logs.


8 weeks ago

#77 @welcher
8 weeks ago

  • Milestone changed from 6.9 to 7.0

6.9 Beta 1 is tomorrow and there hasn't been movement on this so I will punt it to 7.0

Note: See TracTickets for help on using tickets.