WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#36322 closed defect (bug) (fixed)

Password reset form fails when email address includes apostrophes.

Reported by: dcavins Owned by: boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch
Focuses: administration Cc:
PR Number:

Description

If a user's email address contains an apostrophe, the get_user_by() lookup fails because it's checking an email address that's been slashed to travel via $_POST. The conservative fix (patch attached) is to add wp_unslash to retrieve_password(), which is in the spirit of a related fix for adding users via the dashboard, r29966.

I also wonder about adding wp_unslash() to get_user_by( 'email' ) generally so that this problem is fixed everywhere, but the unintended consequences of that change could be bigger than I imagine.

Thanks!

Attachments (2)

36322.01.patch (677 bytes) - added by dcavins 4 years ago.
Un-slash posted email addresses before attempting get_user_by.
36322.tests.patch (2.2 KB) - added by gitlost 4 years ago.
Unit test loading just the PHP functions.

Download all attachments as: .zip

Change History (13)

@dcavins
4 years ago

Un-slash posted email addresses before attempting get_user_by.

#1 @dcavins
4 years ago

  • Keywords has-patch added

#2 @johnbillion
4 years ago

  • Component changed from Users to Login and Registration
  • Keywords 4.6-early added
  • Milestone changed from Awaiting Review to Future Release
  • Version trunk deleted

Thanks for the patch!

#3 @adamsilverstein
4 years ago

  • Keywords needs-unit-tests added

#4 @adamsilverstein
4 years ago

Good catch @dcavins! A unit test validating the issue and fix would be helpful here.

#5 @dcavins
4 years ago

I'd love to write a unit test for this, but I can't figure out how to load wp-login.php at the time of the test. The function I'm testing, retrieve_password(), isn't normally available. @boonebgorges suggests that it may not be possible to test this function given the current testing setup.

#6 @johnbillion
4 years ago

Yeah this is probably untestable given the current state of wp-login.php.

@gitlost
4 years ago

Unit test loading just the PHP functions.

#7 @gitlost
4 years ago

There's a unit test that uses an eval() to load just the PHP functions in "wp-login.php".

#8 @johnbillion
4 years ago

  • Keywords 4.6-early needs-unit-tests removed
  • Milestone changed from Future Release to 4.6

#10 @boonebgorges
3 years ago

@gitlost Thank you for your heroic efforts at making wp-login testable. However, I think we would be far better served by (a) moving login-related functions to a different file (see #35829 and friends), and (b) adopting a proper system for browsers tests (see #34693). The current fix is simple enough that we'll forgo the tests.

#11 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 37474:

During password reset, user-submitted login/email should be stripslashed.

This prevents errors when an email address contains an apostrophe. See [29966]
for similar treatment of a related problem.

Props dcavins.
Fixes #36322.

Note: See TracTickets for help on using tickets.