[wp-trac] [WordPress Trac] #54416: Some WordPress generated emails escape special chars in the email address while other emails do not.

[WordPress Trac]

#54416: Some WordPress generated emails escape special chars in the email address
while other emails do not.
-------------------------------------------------+-------------------------
 Reporter:  ltuspe                               |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  Future
                                                 |  Release
Component:  Mail                                 |     Version:  5.8
 Severity:  major                                |  Resolution:
 Keywords:  good-first-bug has-test-info has-    |     Focuses:
  patch changes-requested                        |
-------------------------------------------------+-------------------------
Changes (by SirLouen):

 * keywords:  good-first-bug has-test-info has-patch has-unit-tests dev-
     feedback => good-first-bug has-test-info has-patch changes-requested


Comment:

 Replying to [comment:16 jdeep]:
 > @mindctrl  @SirLouen
 > 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 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 [[if ( isset( $userdata['user_email'] ) && $user['user_email']
 !== $userdata['user_email'] ) {](https://github.com/SirLouen/wordpress-
 develop/blob/1ab39ce269a0dd718f6e547162403c5eb11219ba/src/wp-
 includes/user.php#L2728-L2729) because of this]).

 2. 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 at example.com`, instead of `o'connor at 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/54416#comment:20>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform