WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 months ago

#47980 closed defect (bug) (fixed)

New wp_validate_redirect() removes domain in some circumstances.

Reported by: rconde Owned by: SergeyBiryukov
Milestone: 5.2.4 Priority: normal
Severity: critical Version: 5.2.3
Component: General Keywords: has-patch commit fixed-major
Focuses: Cc:
PR Number:

Description

Last change to wp_validate_redirect() (5.2.3) breaks the redirect in some cases. I've checked on other sites that runs linux and this doesn't happen. This is happening on XAMPP for Windows.

In my case everything was working fine until the 5.2.3 update and now I get:

https://wp-login.php/?loggedout=true
https://wp-login.php/?checkemail=confirm
https://wp-login.php/?checkemail=registered

As you can see, the domain is missing.

I've found the code causing this:

https://github.com/WordPress/WordPress/commit/c86ee39ff4c1a79b93c967eb88522f5c09614a28

Commenting this code everything works fine again.

I've done a clean install of WordPress 5.2.3 to check if it's something that I've modified on my end but on the clean install it's still happening.

So definitely a bug.

Attachments (3)

pluggable.php.diff (671 bytes) - added by Mat Lipe 3 months ago.
Patch the \wp-includes\pluggable.php file
47980.diff (2.7 KB) - added by peterwilsoncc 2 months ago.
47890.2.diff (2.8 KB) - added by SergeyBiryukov 2 months ago.

Download all attachments as: .zip

Change History (30)

#1 follow-up: @whyisjake
3 months ago

Hi there @rconde,

Do you have any other filters/actions/plugins that are adding to those actions?

#2 follow-up: @jmmathc
3 months ago

Can confirm it happens on Windows when logging out. I'd say it's because dirname returns backslashes on Windows, and they're not stripped correctly.

I've temporarily patched my servers with this fix, on the line that left trims the slash from $path:

$location = '/' . ltrim( $path . '/', '/\\' ) . $location;

#3 in reply to: ↑ 1 @rconde
3 months ago

Replying to whyisjake:

Hi there @rconde,

Do you have any other filters/actions/plugins that are adding to those actions?

It was on a fresh install on Windows.

As @jmmathc as said, it's a bug on backslashes on Windows.

#4 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 5.2.4

#5 @johnbillion
3 months ago

  • Focuses accessibility removed
  • Keywords needs-patch added
  • Version changed from trunk to 5.2.3

#6 @peterwilsoncc
3 months ago

#47995 was marked as a duplicate.

This ticket was mentioned in Slack in #forums by macmanx. View the logs.


3 months ago

#8 in reply to: ↑ 2 ; follow-up: @x2l2
3 months ago

Replying to jmmathc:

Can confirm it happens on Windows when logging out. I'd say it's because dirname returns backslashes on Windows, and they're not stripped correctly.

I've temporarily patched my servers with this fix, on the line that left trims the slash from $path:

$location = '/' . ltrim( $path . '/', '/\\' ) . $location;

I had the same redirect problem when try to login, but the server is not windows

I try this change in wp-inculdes/pluggable.php but doesnt works for me

Last edited 3 months ago by x2l2 (previous) (diff)

#9 in reply to: ↑ 8 @rconde
3 months ago

Try commenting this code in 'wp-includes/pluggable.php' and see if this works for you:

https://github.com/WordPress/WordPress/commit/c86ee39ff4c1a79b93c967eb88522f5c09614a28

This is equal to revert back to previous version of WP as this is the only change pluggable.php had on the latest update.

Also, it would be nice to know what system configuration do you have if it's not Windows.

@Mat Lipe
3 months ago

Patch the \wp-includes\pluggable.php file

#10 @Mat Lipe
3 months ago

  • Keywords has-patch added; needs-patch removed

I have tested against various environments and was able to recreate the issue as well as fix it with this patch.

#11 @davidbaumwald
3 months ago

#48017 was marked as a duplicate.

#12 follow-up: @Sixes
3 months ago

I am seeing the same issue on a Ubuntu server (18.04.3 LTS). The WordPress install is version 5.2.3 on a multi-site setup.

I have tried removing the section at https://github.com/WordPress/WordPress/commit/c86ee39ff4c1a79b93c967eb88522f5c09614a28 and this makes no difference. Clearly adding a backslash to the ltrim() statement also has no effect.

In any case, does ltrim() really take three parameters? According to the php manual:

 ltrim ( string $str [, string $character_mask ] ) : string

The only other odd thing about this setup is that Fear of Landing redirects through Cloudflare.com.

Any suggestions as to how to get round this issue? Currently none of my users can log in.

Edit: Having checked further, it seems that wp_validate_redirect() is not actually being called. Also this may be a different issue as the user is (sometimes) getting:

ERROR: Cookies are blocked or not supported by your browser. You must enable cookies to use WordPress.

Last edited 3 months ago by Sixes (previous) (diff)

#13 in reply to: ↑ 12 @rconde
3 months ago

Replying to Sixes:

I am seeing the same issue on a Ubuntu server (18.04.3 LTS). The WordPress install is version 5.2.3 on a multi-site setup.

I have tried removing the section at https://github.com/WordPress/WordPress/commit/c86ee39ff4c1a79b93c967eb88522f5c09614a28 and this makes no difference. Clearly adding a backslash to the ltrim() statement also has no effect.

In any case, does ltrim() really take three parameters? According to the php manual:

 ltrim ( string $str [, string $character_mask ] ) : string

The only other odd thing about this setup is that Fear of Landing redirects through Cloudflare.com.

Any suggestions as to how to get round this issue? Currently none of my users can log in.

Edit: Having checked further, it seems that wp_validate_redirect() is not actually being called. Also this may be a different issue as the user is (sometimes) getting:

ERROR: Cookies are blocked or not supported by your browser. You must enable cookies to use WordPress.

This seems unrelated to this ticket. Fortunately you have mentioned Cloudflare...

I can tell you what is happening:

wp-login.php creates php cookie 'wordpress_test_cookie' when you access this page.

So I guess that you have configured Cloudflare incorrectly, so when you access wp-login.php you are getting a cached page from Cloudflare, not from your server, hence your server is not creating any cookie because the request is not getting the page from your server but from cloudflare.

Try setting Cloudflare development mode ON and try to log in and see if the problem persists. Please set the development mode On, wait at least 1 minute, reload wp-login.php and try.

If this fixes the problem, its your Cloudflare config, not WordPress.

Then the fix for you is to create a page rule in Cloudflare under 'Page Rules' -> Create page rule -> in the url field insert https://fearoflanding.com/*.php* and then click add a setting and select 'Cache level' -> Bypass and save and deploy.

Hope this fixes your problem.

#14 @Sixes
3 months ago

Thank you so much for that response @rconde, it was very helpful.

However it doesn't seem to have fixed the problem. I have now turned on development mode and cleared the Cloudflare cache. I'm still getting the same issue.

Digging a little further using curl, I find that WordPress appears to be setting a slew of cookies on login but these all have the domain .blog.me.uk which is the "parent" domain for the Wordpress install.For example:

set-cookie: wordpress_test_cookie=WP+Cookie+check; path=/; domain=.blog.me.uk; secure

As I understand it, this cookie will not be sent back whilst accessing the https://fearoflanding.com domain. I assume it is the lack of this cookie (or one of the other cookies with the same domain) that is causing the issue.

Any more ideas? I really appreciate the help.

#15 @Sixes
3 months ago

Well, oddly just uncommenting the line define( 'SUNRISE', 'on' ); in wp-config.php solved the problem. The cookies now all have the correct domain.

I have no idea why.

Thanks again @rconde for the advice.

#16 @SergeyBiryukov
3 months ago

#48042 was marked as a duplicate.

#17 @justinahinon
3 months ago

Hi,

Want to point out that the same issue happen when trying to reset a site password after I've enter the username and click on Reset.

#18 @cmagrin
3 months ago

#48148 was marked as a duplicate.

#19 @daxelrod
3 months ago

Noting the original commit which caused this bug was to fix a security issue: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-16220

It would be useful to know what circumstances could lead to an open redirect to ensure a patch for this bug doesn't cause a regression.

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


2 months ago

@peterwilsoncc
2 months ago

#21 follow-up: @peterwilsoncc
2 months ago

The WP unit tests don't include a Windows machine. If someone could run the unit tests on a Windows machine (ie, not in the docker container provided) and report back, that would be most helpful.

In 47980.diff

  • switched to using wp_normalize_path() before left trimming the URL. This ensures the dirname() output is converted to / throughout and keeps path normalization DRY
  • Added some unit tests for validating redirects without hostnames

Test results found here https://github.com/WordPress/wordpress-develop/pull/108/checks

Edit: The Requests library includes remove_dot_segments() which may prove helpful here too, @SergeyBiryukov I'd be interested in your thoughts here.

Last edited 2 months ago by peterwilsoncc (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @SergeyBiryukov
2 months ago

Replying to peterwilsoncc:

In 47980.diff

  • switched to using wp_normalize_path() before left trimming the URL. This ensures the dirname() output is converted to / throughout and keeps path normalization DRY
  • Added some unit tests for validating redirects without hostnames

Looks correct, though the tests appear to pass for me on Windows 10 both before and after the patch.

I could not yet reproduce the original issue either, so it looks like not all Windows configurations are affected. It would be helpful to know the Windows version on the affected installs, the server software, and whether the login URLs were customized in some way.

I'll see if I can come up with tests that fail before the patch and pass after.

#23 in reply to: ↑ 22 ; follow-up: @rconde
2 months ago

I'm the OP of this ticket to give you some insight.

I'm on Windows 10 pro.

Download XAMPP for windows, clean install of xampp, then clean install of WP. Nothing customized. That's all.

I've just tested with and without the patch just in case a Windows hotfix fixed this, and not: Without the patch the original issue persists.

Replying to SergeyBiryukov:

Replying to peterwilsoncc:

In 47980.diff

  • switched to using wp_normalize_path() before left trimming the URL. This ensures the dirname() output is converted to / throughout and keeps path normalization DRY
  • Added some unit tests for validating redirects without hostnames

Looks correct, though the tests appear to pass for me on Windows 10 both before and after the patch.

I could not yet reproduce the original issue either, so it looks like not all Windows configurations are affected. It would be helpful to know the Windows version on the affected installs, the server software, and whether the login URLs were customized in some way.

I'll see if I can come up with tests that fail before the patch and pass after.

#24 in reply to: ↑ 23 @SergeyBiryukov
2 months ago

  • Keywords commit added

Replying to rconde:

Download XAMPP for windows, clean install of xampp, then clean install of WP. Nothing customized. That's all.

Thanks! I was able to reproduce with WordPress installed in a root directory (previously was testing with a subdirectory).

47890.2.diff includes a test case that fails before and passes after the patch. Should be good to go.

#25 @SergeyBiryukov
2 months ago

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

In 46472:

Formatting: In wp_validate_redirect(), normalize the path when validating the location for relative URLs, to account for Windows paths.

Props peterwilsoncc, rconde, jmmathc, mat-lipe, Sixes, justinahinon, cmagrin, daxelrod, SergeyBiryukov.
Fixes #47980.

#26 @SergeyBiryukov
2 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 5.2.4.

#27 @SergeyBiryukov
2 months ago

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

In 46473:

Formatting: In wp_validate_redirect(), normalize the path when validating the location for relative URLs, to account for Windows paths.

Props peterwilsoncc, rconde, jmmathc, mat-lipe, Sixes, justinahinon, cmagrin, daxelrod, SergeyBiryukov.
Merges [46472] to the 5.2 branch.
Fixes #47980.

Note: See TracTickets for help on using tickets.