Make WordPress Core

Opened 3 years ago

Closed 5 months ago

#52529 closed defect (bug) (fixed)

Non-slashed $old_user_data->user_email in wp_insert_user causes user_activation_key to be unset

Reported by: emirpprime's profile emirpprime Owned by: rajinsharwar's profile rajinsharwar
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch has-unit-tests needs-testing
Focuses: Cc:

Description

WordPress now allows apostrophes in email addresses, but the forgot password process for these users fails.

$data['user_activation_key'] in wp_insert_user() get's cleared when the $user_email !== $old_user_data->user_email comparison fails.
This is because $old_user_data is the "clean" data from the database, however wp_update_user() calls add_magic_quotes() on the user data it passes, causing apostrophes in email addresses to be slashed.

Process fails silently as there is no error when $data['user_activation_key'] is cleared in this flow, even though it is required for the forgot password system to function.

To replicate - register a user with an apostrophe in their email, use the lost password system from wp-login.php. An email will be sent/received, but the reset link is deemed invalid as there is no user_activation_key in the db.

Patch (about to be) attached to slash email only in $old_user_data. While this fixes the bug, it does raise the question of whether get_password_reset_key() should validate that the key is created directly as wp_update_user() errors don't give the granularity to tell us.

Attachments (1)

52529.patch (912 bytes) - added by emirpprime 3 years ago.

Download all attachments as: .zip

Change History (25)

@emirpprime
3 years ago

#1 @hellofromTonya
3 years ago

  • Keywords has-patch needs-unit-tests added
  • Version changed from trunk to 2.2

#2 @desrosj
3 years ago

#53699 was marked as a duplicate.

#3 @kebbet
17 months ago

#58003 was marked as a duplicate.

#4 @kebbet
17 months ago

#58002 was marked as a duplicate.

#5 @rajinsharwar
12 months ago

  • Keywords needs-dev-note added
  • Milestone changed from Awaiting Review to 6.4
  • Version 2.2 deleted

Putting this for 6.4

This ticket was mentioned in PR #5037 on WordPress/wordpress-develop by @rajinsharwar.


12 months ago
#6

  • Keywords has-unit-tests added; needs-unit-tests removed

Unit tests for resetting email with an apostrophe

Trac ticket: https://core.trac.wordpress.org/ticket/52529

#7 @rajinsharwar
12 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Reverting the units tests, need to check further for failures.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


10 months ago

#10 @rajinsharwar
10 months ago

  • Keywords needs-dev-note removed

As per the discussions on the Bugs Scrub of today (09/28/2023), the current patch was proposed to be tested if it was working correctly, and if it was working fine with the current upstream.

After testing the current patch, I found that the patch is working as expected, and I am able to reset the password for a user who has an apostrophe in his/her email.

@oglekler @mukesh27 Do you think we need unit tests for this scenario?

#11 @oglekler
10 months ago

  • Keywords has-unit-tests changes-requested added; needs-unit-tests removed

@rajinsharwar you don't have an apostrophe in your unit test. I think it needs unit test, and you just need to adjust yours a bit (to add this apostrophe into the email), reopen the PR and add needs-testing. It looks like this PR is almost ready, so let's try to finish it. Thank you!

This ticket was mentioned in PR #5432 on WordPress/wordpress-develop by @fnpen.


10 months ago
#12

Setting a new password using the reset link is impossible for the reason that the user's email contains an apostrophe.

#13 @oglekler
10 months ago

  • Keywords needs-testing added; changes-requested removed

#14 @fnpen
10 months ago

I added the fix and test which covers the change, but I found the new issues, which appear in the user editing form. But it's a separate problem another part of the code.

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


10 months ago

#16 @rajinsharwar
10 months ago

Hi @fnpen, thank you so much for your unit tests :)

On the thing you mentioned about the errors that are appearing in the user editing form, would you mind sharing a screenshot or reference of the error?

This ticket was mentioned in Slack in #core by rajinsharwar. View the logs.


10 months ago

#18 @nicolefurlan
10 months ago

@rajinsharwar do you mind if I add you as the owner of this ticket so that you can usher it through and make sure it gets addressed? :)

#19 @rajinsharwar
10 months ago

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

Hi @nicolefurlan, sure no prob, I will assign it to myself.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


10 months ago

#21 @nicolefurlan
10 months ago

  • Milestone changed from 6.4 to 6.5

This ticket was discussed in today's bug scrub.

@fnpen could you create another ticket with your findings regarding the user editing form?

Bumping this to 6.5. @rajinsharwar if you feel that you can get this tested and committed in time, feel free to move it back to 6.4!

Props @hellofromTonya and @oglekler

#22 @rajinsharwar
7 months ago

  • Keywords good-first-bug added

The current patch of https://github.com/WordPress/wordpress-develop/pull/5432 looks good! If we can get some test reports on this, we can get this merged before Beta 1.

#23 @swissspidy
5 months ago

  • Keywords good-first-bug removed

#24 @swissspidy
5 months ago

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

In 57711:

Login and Registration: Slash email address when updating an existing user.

Addresses an issue with password reset keys when the email address contains special characters such as apostrophes.

Props emirpprime, rajinsharwar, fnpen, hellofromTonya, oglekler, nicolefurlan.
Fixes #52529.

Note: See TracTickets for help on using tickets.