Make WordPress Core

Opened 6 years ago

Last modified 4 years ago

#44683 reviewing enhancement

Export and Erase personal data - emails sent to wrong address if username is an email address which is different from the actual email address

Reported by: subrataemfluence's profile subrataemfluence Owned by: xkon's profile xkon
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: 2nd-opinion
Focuses: Cc:

Description

This issue might had been already discussed in another ticket but I was unable to find it.

When a user has set his username to an email address (may not be a valid one), then WordPress will be forced to send the verification request to wrong one.

Here is an example:

Username: fake@bbbb.com

For the above, the following code block won't execute:

if ( ! is_email( $username_or_email_address ) ) {
 ...
}

but this line will be executed:

$email_address = $username_or_email_address;

Which means email is now supposed to be sent at fake@bbbb.com.

I would suggest even a username looks to have a valid email address pattern like above, rather than directly assign it against $email_address variable, we might take an extra step to find the real email address attached to this account before sending the request.

Here is a suggestion:

<?php
if ( ! is_email( $username_or_email_address ) ) {
   ...
} else {
   $user = get_user_by( 'login', $username_or_email_address );
   if ( $user instanceof WP_User ) {
      $email_address = $user->user_email;
   } else {
      $email_address = $username_or_email_address;
   }
}

Please let me know if this makes any sense!

Change History (12)

#1 follow-up: @knutsp
6 years ago

This may get wrong when username is, or looks like, an email address?

Usernames that looks like email address should be prohibited, but that's another ticket.

#2 in reply to: ↑ 1 @subrataemfluence
6 years ago

Replying to knutsp:

This may get wrong when username is, or looks like, an email address?

Exactly! Problem starts when username is an email address or look alike.

Usernames that looks like email address should be prohibited, but that's another ticket.

Yes!

#3 @subrataemfluence
6 years ago

  • Component changed from Gallery to Privacy

#4 @pento
6 years ago

  • Version trunk deleted

#5 @garrett-eclipse
6 years ago

  • Version set to 4.9.6

#6 @desrosj
6 years ago

  • Keywords GDPR removed
  • Owner set to desrosj
  • Status changed from new to assigned

Thanks for reporting this, @subrataemfluence. I am assigning to myself to investigate further. Also, for future reference, the gdpr keyword is no longer used. Using the Privacy componennt or privacy focus is sufficient.

#7 @desrosj
6 years ago

  • Keywords needs-unit-tests added

If you are able to, @subrataemfluence, writing a unit test that proves this is not working correctly would be helpful.

#8 @garrett-eclipse
6 years ago

  • Keywords 2nd-opinion needs-unit-tests removed
  • Owner changed from desrosj to garrett-eclipse
  • Status changed from assigned to reviewing

Thanks @subrataemfluence

This is related to #44347 and I feel needs to be addressed there. I was able to reproduce the issue though but I feel it makes more sense to introduce rigid controls over emails as usernames than it does to attempt to anticipate it within the tools.

It's going to get confusing and potentially leak data to the wrong user when usernames are emails but are of other users.
One thing I found potentially concerning was with registration open I was able to signup a user using my admin email as their username.

I agree with @knutsp that usernames should prohibit the user of email addresses. or at worst limit the username to be an exact match of their email. I'm posting a note to the other ticket and leaving this open for the time being in case those changes require anything to be done to the Privacy tools.

#9 @xkon
4 years ago

  • Milestone changed from Awaiting Review to 5.7
  • Owner changed from garrett-eclipse to xkon

I do agree that the usernames should be cross-checked for existing emails as well during registration so this isn't something that the Privacy component is supposed to handle practically.

From my tests also accidental leaking of information didn't occur as well.

To make it easier to replicate & explain for others reading we have 2 users:

Name: Test, Username: test_user, Email: test@user.com
Name: Fake, Username: test@user.com, Email: fake@user.com

When you create an Export request for test@user.com this is actually used as an email directly so all e-mails will go to test@user.com.

So the issue here is that the Test user might not have actually requested the export (it might've been the Fake user instead) so they will randomly receive an export confirmation, which won't be nice obviously.

I don't mind us altering the code a bit to always go for actual e-mails but we have to take under account that the admin form asks for "Username or email address" and that's why it's working like this. So by simply adding extra checks it might end up being even more confusing towards admins of what was actually used.

If we want to check for actual usernames vs actual email addresses I would prefer to either:

  1. Split the form in the UI also to request specifically either username -or- email (via different fields).
  2. Request only e-mails that will be checked directly to $user->user_email (usernames won't be used anywhere) since everything is communicated via E-mails this might make more sense.

I'd like more input on this from others though before continuing with any decision.

This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-privacy by hellofromtonya. View the logs.


4 years ago

#12 @xkon
4 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.7 to Future Release

Moving to future release as 5.7 Beta 1 is close and we need to iterate further here to see what's the best approach.

Note: See TracTickets for help on using tickets.