Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#44672 closed defect (bug) (fixed)

REST-API: invalid email on lowercase/uppercase change

Reported by: fuchsws's profile fuchsws Owned by: desrosj's profile desrosj
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.9.7
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

When changing an email adresse from lower to uppercase (and vice versa) the error "rest_user_invalid_email" is thrown. This is wrong and it should work as in edit_user().

Examples:

[REST-API] change email from "UserName@domain.com" to "username@…" results in ERROR

[WP-admin] change email from "UserName@domain.com" to "username@…" results in SUCCESS

Both functions use different types of comparisons, while edit_user() is correct and REST-API wrong:

WP_REST_Users_Controller > update_item():

<?php
if ( email_exists( $request['email'] ) && $request['email'] !== $user->user_email )

wp-admin/includes/user.php > edit_user()

<?php
$user->user_email = sanitize_text_field( wp_unslash( $_POST['email'] ) );

[...]

elseif ( ( $owner_id = email_exists($user->user_email) ) && ( !$update || ( $owner_id != $user->ID ) ) )

Attachments (3)

44672.diff (1.9 KB) - added by rachelbaker 7 years ago.
Standardize case of email comparison check and add test
44672.2.diff (2.5 KB) - added by desrosj 6 years ago.
44672.3.diff (2.6 KB) - added by desrosj 6 years ago.

Download all attachments as: .zip

Change History (18)

#1 @SergeyBiryukov
7 years ago

  • Keywords needs-patch needs-unit-tests added

#2 @subrataemfluence
7 years ago

Can we simply unslash and then sanitize the value like

<?php
if ( email_exists( sanitize_text_field( wp_unslash( $request['email'] ) ) ) && sanitize_text_field( wp_unslash( $request['email'] ) ) !== $user->user_email ) {
   return new WP_Error( 'rest_user_invalid_email', __( 'Invalid email address.' ), array( 'status' => 400 ) );
}

@rachelbaker
7 years ago

Standardize case of email comparison check and add test

#3 @rachelbaker
7 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
  • Milestone changed from Awaiting Review to 5.0

In 44672.diff I modified the comparison that checks if a provided user email address should be allowed to change in a PUT request to transform the comparison check of current email address <> provided value to always check lowercase strings.

This feels a bit hacky, but will result in allowing a user to change:
Editor@… --> editor@… successfully matching WP-admin.

#4 @rachelbaker
6 years ago

  • Keywords commit added

#5 @danielbachhuber
6 years ago

  • Milestone changed from 5.0 to 5.1

Bumping to 5.1 because this isn't critical for the 5.0 release.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

#7 @desrosj
6 years ago

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

@desrosj
6 years ago

#8 @desrosj
6 years ago

  • Keywords commit removed

44672.2.diff is a different approach that is more aligned with the check performed in edit_user(). email_exists() is a case insensitive lookup.

I also added a test to ensure a user cannot change their email to one that is assigned to another user if the casing is changed.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

#11 @danielbachhuber
6 years ago

44672.2.diff looks good to me. For precision, can we make sure test_update_item_existing_email_case_not_own has an explicit assertion for the 'rest_user_invalid_email' error code too?

#12 @kadamwhite
6 years ago

Looks pretty good, although the assignment in this conditional

if ( ( $owner_id = email_exists( $request['email'] ) ) && $owner_id !== $id ) {

does not thrill me. I'd like to propose adding a comment to make it clear we're borrowing verbatim, or find a way to clarify the logic flow.

@desrosj
6 years ago

#13 @desrosj
6 years ago

  • Keywords commit added

44672.3.diff moves the assignment outside of the conditional and adds an explicit assertion for the rest_user_invalid_email code to test_update_item_existing_email_case_not_own.

#14 @desrosj
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44641:

REST API: Allow a user to change the letter casing of their email.

When a PUT request is performed to update a user, a rest_user_invalid_email error is incorrectly being returned when the email exists with different letter casing, even if it belongs to the user being updated. email_exists() performs a case insensitive lookup, but the conditional statement following that lookup was performing a strict comparison between the new email and the user’s current email.

This changes that comparison to instead compare the user ID returned by email_exists() with the user ID being updated. This more closely matches the logic used in edit_user() and allows a user to change the letter casing of their email.

Props fuchsws, rachelbaker, desrosj.
Fixes #44672.

#15 @desrosj
6 years ago

In 44642:

Tests: Do not include the delimiter parameter in ucwords() calls.

This parameter is not supported in PHP < 5.4. Introduced in [44641].

See #44672.

Note: See TracTickets for help on using tickets.