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 | Owned by: | 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)
#2
in reply to:
↑ 1
@
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!
#6
@
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
@
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
@
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
@
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:
- Split the form in the UI also to request specifically either username -or- email (via different fields).
- 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 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.