[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:  jdeep
     Type:  defect (bug)                         |      Status:  assigned
 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                        |
-------------------------------------------------+-------------------------

Comment (by SirLouen):

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

 [[Image(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 at 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 at 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 at example.com',
                 )
         );

         $_POST['email']   = "o\'connor at 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.

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