Make WordPress Core

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: ltuspe's profile ltuspe Owned by: jdeep's profile jdeep
Milestone: Future Release Priority: normal
Severity: major Version: 5.8
Component: Mail 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 @SirLouen
5 months ago

  • Keywords needs-patch good-first-bug has-test-info added
  • Severity changed from normal to major

Reproduction Report

Description

✅ This report validates that the issue can still be reproduced.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.29
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 138.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Micro Email Testing 1.0.0
    • Test Reports 1.2.0

Reproduction Steps

  1. Create a new regular user (for example, Editor level)
  2. Now change the email to a regular email like foo@…
  3. ✅ You will receive an Email Change Request email to that address
  4. Now change the email to an email with an apostrophe
  5. 🐞 Email is never sent.

Actual Results

  1. ✅ Error condition occurs (reproduced).

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:

  1. First issue is here:

add_magic_quotes wrongly adds the backslash to the email, rendering it useless.

  1. Second issue is in what $_POST content is being received send_confirmation_on_profile_email function. The $_POST['email'] has been sanitized in the process.

Solutions can vary.

For the first issue, maybe unslashing after add_magic_quotes could be OK'ish. But personally, I think that massively applying add_magic_quotes to the whole $user array 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_email simply 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-bug to be worked in

Last edited 5 months ago by SirLouen (previous) (diff)

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
  • Root Cause: WordPress's add_magic_quotes() function adds slashes to all $_POST data and user arrays, but the affected email functions were using this slashed data directly without calling wp_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

  1. Create a user account with a normal email address (e.g., user@example.com)
  2. Log in and go to ProfileAccount Management
  3. Change email to one with an apostrophe (e.g., o'connor@example.com)
  4. Click Update Profile
  5. 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

  1. Complete the email change confirmation process from Test Case 1
  2. Click the confirmation link in the email
  3. 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

### Test Case 3: Password Change Notification

  1. Create or use a user account with an email containing special characters (e.g., user's.email@example.com)
  2. Change the user's password (either through profile or admin)
  3. 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

---

#3 @SirLouen
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:

  1. Why add_magic_quotes is being added to the whole $user array?
  2. Why in send_confirmation_on_profile_email the 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.

#4 follow-up: @jdeep
4 months ago

I would like to pick this up if it is still available.

#5 in reply to: ↑ 4 @SirLouen
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: @jdeep
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: @SirLouen
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_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?

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: @jdeep
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_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?

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?

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 be o\'reilly@example.com
  • $userdata['user_email'] will be o'reilly@example.com

thus triggering the email to be sent even though email ids are exactly same.

#9 in reply to: ↑ 8 @SirLouen
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 @jdeep
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 @jdeep
4 months ago

@SirLouen, So here is what I am planning to implement:

  1. Do not blindly use add_magic_quotes() on entire $user here. Instead use addslashes on 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:
    • ID
    • user_status
    • use_ssl
  • Boolean-like strings
    • rich_editing
    • syntax_highlighting
    • comment_shortcuts
    • show_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.

  1. Use sanitize_email instead of esc_html here. Otherwise emails (like o'connor@example.com) with special characters would become o&#039;connor@example.com.
  1. Lastly un-slash before sending emails in send_confirmation_on_profile_email.

Any inputs from you side?

#12 @SirLouen
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

This commit fixes problems associated with emails containing valid special characters. See #54416 for more details.

Trac ticket: #54416

#14 follow-up: @mindctrl
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 @jdeep
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.

#16 follow-up: @jdeep
4 months ago

@mindctrl @SirLouen
Added unit tests.

@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.

@jdeep commented on PR #9522:


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 @mindctrl
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 @SirLouen
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 &#039 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:

  1. 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_user with 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).
  1. Second, once the email is sent and the email change is pending in _new_email metadata, the process of moving from _new_email to user_email through the send_confirmation_on_profile_email, it's where, currently, its not being correctly sanitized resulting in the email like: o&#039connor@example.com, instead of o'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.

Last edited 4 months ago by SirLouen (previous) (diff)

#21 @SirLouen
4 months ago

  • Owner set to jdeep
  • Status changed from new to assigned

#22 @jdeep
3 months ago

#50072 was marked as a duplicate.

#23 @SirLouen
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

https://i.imgur.com/eiAJevj.png

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:

  1. Get back into the logic
  2. 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 @SirLouen
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).

Last edited 3 months ago by SirLouen (previous) (diff)

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.

Note: See TracTickets for help on using tickets.