WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 22 months ago

#40728 closed defect (bug) (fixed)

Added urlencode on wp_lostpassword_url()

Reported by: adhun Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch
Focuses: administration Cc:

Description

I found that the function wp_lostpassword_url() was not encoding the redirect URL which was resulting in 404 error in some instances. To fix the issue, urlencode has been added to the function.

Attachments (1)

40728.diff (842 bytes) - added by adhun 2 years ago.
Patch File for urlencode on wp_lostpassword_url( )

Download all attachments as: .zip

Change History (14)

@adhun
2 years ago

Patch File for urlencode on wp_lostpassword_url( )

#1 @adhun
2 years ago

  • Keywords has-patch added

#2 @adhun
2 years ago

  • Severity changed from normal to critical

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


2 years ago

#4 follow-up: @jnylen0
2 years ago

which was resulting in 404 error in some instances

Can you be more specific here? How can we reproduce this breakage?

#5 in reply to: ↑ 4 @adhun
2 years ago

Replying to jnylen0:

which was resulting in 404 error in some instances

Can you be more specific here? How can we reproduce this breakage?

Yes, Steps to reproduce the issue

if you use wp_lostpassword_url(home_url()) on http://example.com it will generate a url like the following

http://example.com/wp-login.php?action=lostpassword&redirect_to=http://example.com

so the url parameter here contains slashes which not safe on all kind of servers.
so when we add urlencode for the redirect url will be

http://example.com/wp-login.php?action=lostpassword&redirect_to=http%3A%2F%2Fexample.com

and it is safe for all kind of servers.

#6 follow-up: @jnylen0
2 years ago

not safe on all kind of servers.

On what server setups does this break?

#7 in reply to: ↑ 6 @adhun
2 years ago

Replying to jnylen0:

not safe on all kind of servers.

On what server setups does this break?

I was working on a shared hosting with Apache Version 2.2.32, PHP Version 5.6.30.
It was throwing 404 error when I tried to access a URL with a slash on GET parameter list.
When I applied urlencode() for the URL passing through GET parameter it fixed the issue.

For instance, in a query string, the ampersand (&) is used as a separator between key-value pairs. If you were to put an ampersand into one of those values, it would look like the separator between the end of a value and the beginning of the next key. So for special characters like this, we use percent encoding so that we can be sure that the data is unambiguously encoded.

#8 @jnylen0
2 years ago

  • Milestone changed from Awaiting Review to 4.8.1
  • Severity changed from critical to normal

Ok, thanks for the clarification. Which shared host is this? It would help to know how common this problem is.

In any case, I'm setting this ticket back to normal as this is a broken server configuration. / characters are specifically allowed in query strings per RFC 3986:

The query component is indicated by the first question mark ("?") character and terminated by a number sign ("#") character or by the end of the URI.

query = *( pchar / "/" / "?" )

The characters slash ("/") and question mark ("?") may represent data within the query component. Beware that some older, erroneous implementations may not handle such data correctly when it is used as the base URI for relative references (Section 5.1), apparently because they fail to distinguish query data from path data when looking for hierarchical separators.

Even though this should work everywhere, it obviously doesn't work sometimes, and this is noted in the RFC, so I don't see the harm in adding urlencode. Milestoning for 4.8.1 for discussion.

#9 @adhun
2 years ago

I found issue on the https://www.hostdime.com/ shared hosting
I think it's a good idea to keep the urlencode() since the users have less access to server configuration on shared hosting.

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


23 months ago

#11 @jbpaul17
23 months ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

#12 @johnbillion
22 months ago

  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version 4.8 deleted

This actually has nothing to do with server configuration. The URL should be encoded as per @adhun's patch, otherwise any query parameters that are included in the URL that's passed to wp_lostpassword_url() will be treated as query parameters on the lost password URL.

@adhun's patch is correct, and mirrors behaviour in other functions such as wp_logout_url() and wp_login_url().

#13 @johnbillion
22 months ago

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

In 41121:

Login and Registration: Correctly encode the redirect location URL when it's passed as a query parameter to the lost password URL.

Props adhun

Fixes #40728

Note: See TracTickets for help on using tickets.