Opened 4 years ago
Last modified 3 months ago
#54416 assigned defect (bug)
Some WordPress generated emails escape special chars in the email address while other emails do not.
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | Future Release | Priority: | normal |
| Severity: | major | Version: | 5.8 |
| Component: | Keywords: | good-first-bug has-test-info has-patch changes-requested has-unit-tests | |
| Focuses: | Cc: |
Description
When a user's email address contains an apostrophe, WordPress will escape the apostrophe in the email address for some WordPress generated emails, while it will not for other WordPress generated emails.
For example o'connor@… will be escaped to o\'connor@… for the 'Email Changed' and 'Password Changed' emails that are sent to the user.
For other emails, such as the 'User Lost Password' email, the email address is not escaped and is sent properly.
Change History (25)
#1
@
5 months ago
- Keywords needs-patch good-first-bug has-test-info added
- Severity changed from normal to major
This ticket was mentioned in PR #9283 on WordPress/wordpress-develop by @kush123.
5 months ago
#2
- Keywords has-patch added; needs-patch removed
# Fix Email Escaping Bug in WordPress Notifications
## Overview
This pull request fixes a critical bug where WordPress was incorrectly escaping special characters (like apostrophes) in email addresses for certain notifications, resulting in malformed email addresses with backslashes (e.g., o\'connor@example.com instead of o'connor@example.com).
This addresses the inconsistent email handling where some notifications worked correctly while others failed validation or delivery for email addresses containing special characters.
## Trac Ticket: #54416
---
## Issue Fixed
### Email Addresses With Special Characters Show Backslashes in Notifications
- Problem:
Email addresses containing special characters (apostrophes, quotes, etc.) were being escaped in certain WordPress notifications:
- Email change confirmations: Used slashed
$_POST['email']data directly - Email change notifications: Used slashed
$user['user_email']data directly - Password change notifications: Used slashed
$user['user_email']data directly - Result: Recipients received emails with backslashes (
o\'connor@example.com) or emails failed validation entirely
- Email change confirmations: Used slashed
- Root Cause:
WordPress's
add_magic_quotes()function adds slashes to all$_POSTdata and user arrays, but the affected email functions were using this slashed data directly without callingwp_unslash()first.
- Solution:
Applied
wp_unslash()to all email address usages in the affected functions to ensure clean, properly formatted email addresses in all notifications.
---
## Code Changes
### 1. Email Change Confirmation (send_confirmation_on_profile_email)
// Before (buggy)
if ( ! is_email( $_POST['email'] ) ) {
// Email validation fails for addresses with apostrophes
}
wp_mail( $_POST['email'], $subject, $content );
// Sends to: o\'connor@example.com
// After (fixed)
if ( ! is_email( wp_unslash( $_POST['email'] ) ) ) {
// Email validation works correctly
}
wp_mail( wp_unslash( $_POST['email'] ), $subject, $content );
// Sends to: o'connor@example.com
### 2. Email Change Notification (wp_update_user)
// Before (buggy)
$email_change_email = array(
'to' => $user['user_email'], // Contains backslashes
);
$content = str_replace( '###EMAIL###', $user['user_email'], $content );
// Results in: o\'connor@example.com
// After (fixed)
$email_change_email = array(
'to' => wp_unslash( $user['user_email'] ), // Clean email
);
$content = str_replace( '###EMAIL###', wp_unslash( $user['user_email'] ), $content );
// Results in: o'connor@example.com
### 3. Password Change Notification (wp_update_user)
// Before (buggy)
$pass_change_email = array(
'to' => $user['user_email'], // Contains backslashes
);
$content = str_replace( '###EMAIL###', $user['user_email'], $content );
// Results in: o\'connor@example.com
// After (fixed)
$pass_change_email = array(
'to' => wp_unslash( $user['user_email'] ), // Clean email
);
$content = str_replace( '###EMAIL###', wp_unslash( $user['user_email'] ), $content );
// Results in: o'connor@example.com
---
## Testing Instructions
### Test Case 1: Email Change Confirmation
- Create a user account with a normal email address (e.g.,
user@example.com) - Log in and go to Profile → Account Management
- Change email to one with an apostrophe (e.g.,
o'connor@example.com) - Click Update Profile
- Expected:
- Email validation should pass (no error message)
- Confirmation email should be sent to
o'connor@example.com(without backslashes) - Email content should display
o'connor@example.com(without backslashes)
### Test Case 2: Email Change Notification
- Complete the email change confirmation process from Test Case 1
- Click the confirmation link in the email
- Expected:
- Notification email should be sent to the OLD email address (
user@example.com) - Email content should mention the new email as
o'connor@example.com(without backslashes) - Email should be properly delivered and readable
- Notification email should be sent to the OLD email address (
### Test Case 3: Password Change Notification
- Create or use a user account with an email containing special characters (e.g.,
user's.email@example.com) - Change the user's password (either through profile or admin)
- Expected:
- Password change notification should be sent to
user's.email@example.com(without backslashes) - Email content should display
user's.email@example.com(without backslashes) - Email should be properly delivered and readable
- Password change notification should be sent to
---
#3
@
4 months ago
- Keywords needs-patch added; has-patch removed
Hey @kush123
Sorry for late reviewing this patch but I've been a little busy lately in GB
This said, I hope you read my report. Specially the last paragraph. Basically what you have done in this ticket is exactly what I was advising against.
I don't really like this solution. We have to go a little deeper into the problem. As I said there are two main problems:
- Why
add_magic_quotesis being added to the whole$userarray? - Why in
send_confirmation_on_profile_emailthe form post result is coming slashed?
Could we sort any of these two before happening instead of fixing them after happening?
If you have some time, please review this patch again from scratch.
#5
in reply to:
↑ 4
@
4 months ago
- Milestone changed from Awaiting Review to Future Release
Replying to jdeep:
I would like to pick this up if it is still available.
Sure @jdeep go on, please, take into consideration all the elements that have been already commented, this is an easy bug, but not because it's necesarily easy to implement, but because it has a lot of information available for so.
#6
follow-up:
↓ 7
@
4 months ago
@SirLouen
Why in send_confirmation_on_profile_email the form post result is coming slashed?
This happens because ALL superglobals are slashed whenever WordPress loads.
Rather than rectifying this slash-ed email later on, we should refactor add_magic_quotes to maybe have some whitelist fields which does not need to be slashed. But this kind of refactor may break existing code if not thought of all the cases properly. It would also come with the overhead of maintaining this whitelist.
But again this would be a much cleaner solution than un-slashing it later on.
What are your thoughts on it?
#7
in reply to:
↑ 6
;
follow-up:
↓ 8
@
4 months ago
Replying to jdeep:
@SirLouen
Why in send_confirmation_on_profile_email the form post result is coming slashed?
This happens because ALL superglobals are slashed whenever WordPress loads.
Rather than rectifying this slash-ed email later on, we should refactor
add_magic_quotesto maybe have some whitelist fields which does not need to be slashed. But this kind of refactor may break existing code if not thought of all the cases properly. It would also come with the overhead of maintaining this whitelist.
But again this would be a much cleaner solution than un-slashing it later on.
What are your thoughts on it?
One of my questions for the first part is, why slahsing a whole array of fields with add_magic_quotes if we only need quotes for 1 field of such array?
For the second part, maybe the only way to unslash is to target directly the field in send_confirmation_on_profile_email but I just want to make sure that we cannot do this more prematurely.
#8
in reply to:
↑ 7
;
follow-up:
↓ 9
@
4 months ago
Replying to SirLouen:
Replying to jdeep:
@SirLouen
Why in send_confirmation_on_profile_email the form post result is coming slashed?
This happens because ALL superglobals are slashed whenever WordPress loads.
Rather than rectifying this slash-ed email later on, we should refactor
add_magic_quotesto maybe have some whitelist fields which does not need to be slashed. But this kind of refactor may break existing code if not thought of all the cases properly. It would also come with the overhead of maintaining this whitelist.
But again this would be a much cleaner solution than un-slashing it later on.
What are your thoughts on it?
One of my questions for the first part is, why slahsing a whole array of fields with
add_magic_quotesif we only need quotes for 1 field of such array?
You are right. I delved into it a bit more and found that this line is indeed buggy. Even assuming we hotfix it by un-slashing before before sending the mail, it will cause another unexpected behaviour where if the user's email has a character which causes a backslash to be added, then clicking the Update Profile button in User > Profile will trigger an email to be sent even if the email hasn't changed! This happens because of this line:
<?php if ( isset( $userdata['user_email'] ) && $user['user_email'] !== $userdata['user_email'] ) { $send_email_change_email = apply_filters( 'send_email_change_email', true, $user, $userdata ); }
$user['user_email'] has been slashed because of $user = add_magic_quotes( $user ). However, $userdata['user_email'] is unslashed before passing into wp_update_user.
Therefore, if an email is o'reilly@example.com stored in the database,
$user['user_email']will beo\'reilly@example.com$userdata['user_email']will beo'reilly@example.com
thus triggering the email to be sent even though email ids are exactly same.
#9
in reply to:
↑ 8
@
4 months ago
Replying to jdeep:
thus triggering the email to be sent even though email ids are exactly same.
Looks right to me. This ticket in fact is two folded, but the two problems despite they target the same issue they are technically unrelated. The second part with send_confirmation_on_profile_email is completely different issue.
#10
@
4 months ago
@SirLouen,
For the second part, I believe there is no way to prevent slashing prematurely and the only way to do it is to un-slash in send_confirmation_on_profile_email. On a related note, Is there any reason of escaping an email address like it is done here? This breaks email addresses with special characters. Shouldn't sanitize email be a better choice?
#11
@
4 months ago
@SirLouen, So here is what I am planning to implement:
- Do not blindly use
add_magic_quotes()on entire$userhere. Instead useaddslasheson the required fields only to maintain backward compatibility.
<?php // Escape data pulled from DB. // $user = add_magic_quotes( $user ); $user['display_name'] = wp_slash( $user['display_name'] ); $user['first_name'] = wp_slash( $user['first_name'] ); $user['last_name'] = wp_slash( $user['last_name'] ); $user['description'] = wp_slash( $user['description'] ); $user['nickname'] = wp_slash( $user['nickname'] );
Compared to the the previous code, in this change, we intentionally omit slash-ing the following fields:
- Numeric fields:
IDuser_statususe_ssl
- Boolean-like strings
rich_editingsyntax_highlightingcomment_shortcutsshow_admin_bar_front
- Special format fields
user_registered(date format)user_pass(hashed password)
- Fields which should not be slashed
user_email
There are some fields which I am not sure if we should omit slashing. Like user_url, user_login and user_nicename. But maybe for now we can slash them as well since current version slashes everything and everything works. Will need to research a bit more on this to see how not slashing these fields may cause unexpected behaviour somewhere else.
- Use
sanitize_emailinstead ofesc_htmlhere. Otherwise emails (likeo'connor@example.com) with special characters would becomeo'connor@example.com.
- Lastly un-slash before sending emails in
send_confirmation_on_profile_email.
Any inputs from you side?
#12
@
4 months ago
@jdeep at first, it looks good. I will review the code anyway. If possible send a Github PR instead of a .diff file
This ticket was mentioned in PR #9522 on WordPress/wordpress-develop by @jdeep.
4 months ago
#13
- Keywords has-patch added; needs-patch removed
#14
follow-up:
↓ 15
@
4 months ago
- Keywords needs-unit-tests added
There are two PRs on this ticket but both lack phpunit tests. I think we need tests to demonstrate this works as intended and to prevent future regressions.
#15
in reply to:
↑ 14
@
4 months ago
Replying to mindctrl:
There are two PRs on this ticket but both lack phpunit tests. I think we need tests to demonstrate this works as intended and to prevent future regressions.
Right. I will send patches for unit tests to make sure all of it works.
@mindctrl commented on PR #9522:
4 months ago
#17
@JDeepD if you update your fork with the latest trunk changes and then rebase your PR, the test suite will run as intended.
4 months ago
#18
@JDeepD if you update your fork with the latest trunk changes and then rebase your PR, the test suite will run as intended.
Done
#19
@
4 months ago
- Keywords has-unit-tests dev-feedback added; needs-unit-tests removed
@jdeep thanks for updating the patch with tests. I'm requesting some feedback from core devs to see if this can be added to a milestone.
#20
in reply to:
↑ 16
@
4 months ago
- Keywords changes-requested added; has-unit-tests dev-feedback removed
Replying to jdeep:
Added unit tests.
I wrote, two days ago, a pretty long review, but I forgot to send it because i got distracted with the WCUS CD ☠️
So here I am, trying to remember everything and writing it again:
The patches look good to me. But I have a little concern that I did not consider in my first report.
We are going to divide these into two parts. Lets start first with the confirmed email change for the send_confirmation_on_profile_email part (the second part of my report)
The sanitization looks correct, and the email gets stored as it should ✅
The problem is the test is not actually covering the problem. The test is half ok, because its testing the email sending itself and paradoxically, it could serve to cover the other problem. But in this case, what we are sorting in user-edit.php is the fact that, when mail gets confirmed the result is not sanitized currently, ending with the unsanitized HTML encoding ' for the single quote and this is not covered in this test.
For the other section, after rechecking the code, I have completely forgotten that the whole $user object is being passed to the email filter hook. Given this scenario, it appears that there is no other cleaner alternative, that simply slashing the entire array (with add_magic_quotes), and then unslashing just the email, as proposed in the first patch. Now I clearly see that it doesn't make any sense to manually slash a big chunk of the whole array. It's true that the array has 20+ items, and in this case, we are laser targeting only 8 items, which is better for performance, but the improvement is negligible and makes the code way dirtier, since most of the elements in the array are booleans or basic data types. On my mind, I thought that only 1 or 2 items should be slashed (username and display name), but technically, every single item could be profited from in the filter and not passing them as they are being passed now will cause a BC, so I believe its better to simply slash them all and then unslash the email as the original version.
Now, talking about the test, I can't really see the utility: why are you testing wp_mail? It seems it's completely unrelated to what we should be testing here.
So basically none of the unit tests are valid.
The tests should be testing this:
- First, the problem only happens when doing this with an regular account, not admin, because admins don't go through the email confirmation pattern. So basically we should be testing the email trigger
wp_update_userwith a lower privileged user, not with admin (because despite you can see that the email is formed in the function, the email is not sent because of this).
- Second, once the email is sent and the email change is pending in
_new_emailmetadata, the process of moving from_new_emailtouser_emailthrough thesend_confirmation_on_profile_email, it's where, currently, its not being correctly sanitized resulting in the email like:o'connor@example.com, instead ofo'connor@example.com. This is what we should be testing here.
Yes, you could add more tests, but first you should check the coverage and see if you could add some more tests for uncovered sections (or a data provider). But for now, lets focus to cover the specific use-case that is bringing this, ticket.
Ping me if you happen to sort this.
#23
@
3 months ago
@jdeep I've been debugging to understand how to build this unit test and I've noted something very interesting
When we trigger the email change with o'connor@example.com
It comes to this section https://github.com/SirLouen/wordpress-develop/blob/9ca38ce47b7a8a9c9e916e6aff39ec772ccbba55/src/wp-includes/user.php#L3813-L3822
Which obviously triggers the error
But the error is not actually displayed.
Contrarily if you simply add whatever email, for example: "whateveremail".
It will actually trigger the error as expected
The big difference is that a completely wrong email like "whateveremail" will get to
https://github.com/SirLouen/wordpress-develop/blob/9ca38ce47b7a8a9c9e916e6aff39ec772ccbba55/src/wp-admin/user-edit.php#L252-L259
The key is exactly here:
https://github.com/SirLouen/wordpress-develop/blob/9ca38ce47b7a8a9c9e916e6aff39ec772ccbba55/src/wp-admin/includes/user.php#L80-L82
That comes from here
https://github.com/SirLouen/wordpress-develop/blob/9ca38ce47b7a8a9c9e916e6aff39ec772ccbba55/src/wp-admin/user-edit.php#L171-L172
The difference betweeen o'connor@example.com and whateveremail is that in that line, the email is sanitized and the user is edited and accepted correctly, but the email confirmation is not sent and there is no "lock" because there is an error in the send_confirmation_on_profile_email which is entirely ignored.
So here we have two bugs: The fact that the apostrophes should be sanitized, and the fact that no error is hindering the update of the email in the database.
So for the Unit Test, its as simple as checking that there are no errors (global $errors)
There is another problem: the email you send to this section in the test, gets perfectly sanitized.
https://github.com/SirLouen/wordpress-develop/blob/9ca38ce47b7a8a9c9e916e6aff39ec772ccbba55/src/wp-includes/user.php#L3813-L3822
Its not the same as when you send via the password form in the user edit screen.
So it will not trigger the error
Here we have two options:
- Get back into the logic
- Send the poorly sanitized email like:
o\'connor@example.com(which I prefer, because its less code and goes straight to the point)
So this first test can be as simple as:
public function test_send_confirmation_on_profile_email_with_special_chars() {
global $errors;
$user = self::factory()->user->create_and_get(
array(
'role' => 'subscriber',
'user_email' => 'before@example.com',
)
);
$_POST['email'] = "o\'connor@example.com";
$_POST['user_id'] = $user->ID;
wp_set_current_user( $user->ID );
do_action( 'personal_options_update' );
$this->assertFalse( $errors->has_errors(), 'Email with apostrophes should be sanitized and validated' );
}
Now we need to fix the bug I'm talking about (the fact that the error is not checked timely before updating the user) and the second test for the add_magic_quotes and confirmed email change part.
I'm going to leave this easy bug to you. Also you need to expand my test above, to confirm that an email that has triggered any of the two errors send_confirmation_on_profile_email will not update any values in the database, basically 2 lines more to check for this on a similar fashion as what you did before (like checking to see if user meta _new_email is or not there, it should not be there without the apostrophes fix because the error should avoid the user update).
On my side, I will start analysing the second unit test.
#24
@
3 months ago
For the second test, I'm not a super expert on PHPUnit for WordPress, but I believe that it's impossible to re-bootstrap admin UI, at best only includes from wp-admin/includes.
As I've sent you in, the GH review wp_update_user was receiving from:
https://github.com/SirLouen/wordpress-develop/blob/9ca38ce47b7a8a9c9e916e6aff39ec772ccbba55/src/wp-admin/user-edit.php#L119
The email wrongly sanitized before the patch. Now that it's receiving the correct email, wp_update_user is working perfectly, it appears that no extra changes needed.
For this reason, for the second test the only way to actually test this, is by using Playwright. The big problem here is that for this test, we need to play with user meta like _new_email and unfortunately, this is not registered in REST meta for user, hence we cannot set values on the fly unless we do use some shenanigans AFAIK. So the test is not as straightforward as it should. The other option is doing all the steps from the beginning (editing the email in a step, picking the hash after, and going through finally). I will give a thought to this later, but still since the metadata is not registered in the API I'm not sure how it could be retrieved easily without similar shenanigans.
I was alternatively thinking of creating a test to check for the email send confirmation, but this test test_send_confirmation_on_profile_email_html_entities_decoded is already testing for this, and as I say, the wp_update_user doesn't really have any issue with apostrophes. If we would like to test, we would better add a dataProvider to test_send_confirmation_on_profile_email_html_entities_decoded` with the apostrophed-email, but I really think it's unnecessary.
Unless anyone can think of a better solution, it seems that for the second test, we should better leave it unchecked (nothing new under the sun anyway).
This ticket was mentioned in PR #10001 on WordPress/wordpress-develop by @SirLouen.
3 months ago
#25
- Keywords has-unit-tests added
This is the continuation of #54416 given that @JDeepD is going to be AFK for a while.

Reproduction Report
Description
✅ This report validates that the issue can still be reproduced.
Environment
Reproduction Steps
Email Change Requestemail to that addressActual Results
Additional Comments
This problem first appeared here [23554] when they massively applied slashing (good old times when these mammut changes were possible). Now this has become a double folded problem:
add_magic_quoteswrongly adds the backslash to the email, rendering it useless.$_POSTcontent is being receivedsend_confirmation_on_profile_emailfunction. The$_POST['email']has been sanitized in the process.Solutions can vary.
For the first issue, maybe unslashing after
add_magic_quotescould be OK'ish. But personally, I think that massively applyingadd_magic_quotesto the whole$userarray is not the best idea and elements requiring should be slashed accordingly. But the quick fix of just unslashing the email could work. For the patch creator, depending if you want to be a pro and do and explain the whole research of what should be unslashed and what not, or a newb and just go with the easy fix.The second one can be trickier. Obviously, unslashing in
send_confirmation_on_profile_emailsimply works, but it's a little dirty. Ideally, we should look where it was slashed in the first place and working around it.With all this information, I think it could be a nice
good-first-bugto be worked in