Make WordPress Core

Opened 5 years ago

Last modified 2 months ago

#53348 reviewing defect (bug)

No form to log in when visiting wp-login.php with a given query string

Reported by: henrywright's profile henry.wright Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: needs-unit-tests early has-patch
Focuses: Cc:

Description

When I visit the wp-login.php page with specific query strings, I get a blank page. I don't get a form to log in.

The query strings that cause the blank page are

  • wp-login.php?action=checkemail
  • wp-login.php?checkemail=foo
  • wp-login.php?checkemail=bar
  • wp-login.php?checkemail=baz
  • Note though, wp-login.php?checkemail=confirm does give me a form

Attachments (1)

53348.diff (645 bytes) - added by henry.wright 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @henry.wright
5 years ago

Actually, wp-login.php?checkemail=confirm doesn't give me a form like I said in this ticket description. It gives me a message to check my email for a password reset link. This is expected behaviour

#2 @henry.wright
5 years ago

Further, when I visit wp-login.php?action=checkemail, I get 2 PHP debug notices

Notice
: Undefined index: checkemail in
/var/www/html/wp-login.php
on line
1143
Notice
: Undefined index: checkemail in
/var/www/html/wp-login.php
on line
1153

#3 @henry.wright
5 years ago

In summary

I did some testing on a fresh local install using both action and checkemail attributes in the wp-login.php query string.

For the action attribute, the postpass and checkemail values result in blank pages:

  • wp-login.php?action=postpass
  • wp-login.php?action=checkemail

When the value is checkemail, I get 2 debug notices related to a undefined indexes in wp-login.php

Undefined index: checkemail in /var/www/html/wp-login.php

For the checkemail attribute, every value aside from confirm and registered results in blank pages:

  • wp-login.php?checkemail=foo
  • wp-login.php?checkemail=bar

This ticket was mentioned in PR #1339 on WordPress/wordpress-develop by davidkryzaniak.


5 years ago
#4

  • Keywords has-patch added

Added else to wp-login.php. This is just in case $_GET['checkemail'] is set to something we were not expecting.

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

#5 @henry.wright
5 years ago

Thanks for the patch @davidkryzaniak. The else statement does provide a solution to the blank page. However, the 2 debug notices aren't fixed by the patch

#7 @henry.wright
5 years ago

Something like this is also necessary to fix the debug notices

if ( isset( $_GET['checkemail'] ) && 'confirm' === $_GET['checkemail'] ) {
    // ...
} elseif ( isset( $_GET['checkemail'] ) && 'registered' === $_GET['checkemail'] ) {
    // ...
} else {
    // ...
}

#8 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @henry.wright
5 years ago

Thinking about the solution for this. Although the patch solves the problem, displaying an error when an invalid query string is supplied isn't consistent with existing behaviour.

Currently when there is an invalid query such as action=abcdef, the login form is displayed. Login form display seems to be the default or "fallback"

@henry.wright
5 years ago

#10 @henry.wright
5 years ago

53348.diff is a solution which uses the login form as the default case when the query string isn't "valid"

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


5 years ago

#13 @audrasjb
5 years ago

  • Keywords needs-testing added
  • Milestone changed from 5.9 to 6.0

As per today's bug scrub, let's move this ticket for 6.0 consideration to ensure it's properly tested. Some testing info are already available above in this ticket.

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


4 years ago

#15 @costdev
4 years ago

  • Keywords needs-unit-tests early added; needs-testing removed
  • Milestone changed from 6.0 to Future Release

Test Report

Environment

  • Server: Apache (Linux)
  • WordPress: 6.0-alpha-52448-src
  • Browser: Chrome 100.0.4896.75
  • OS: Windows 10
  • Theme: Twenty Twenty
  • Plugins: None activated

Steps

  1. Navigate to wp-login.php?checkemail=foo. Notice that there is no login form. ✅
  2. Apply 53348.diff. Notice that the login form appears. ✅

Results

  1. 53348.diff successfully resolves the issue.

Notes

  • This would benefit from unit tests that set the $_GET['checkemail'] value and test did_action( 'login_init' ), for example. Adding needs-unit-tests.
  • Per the discussion in the bug scrub, moving this to Future Release as it was felt that it's too late in the 6.0 cycle to touch the login form and marking as early.

#16 @sheldorofazeroth
2 months ago

  • Keywords needs-patch added; has-patch removed

The existing PR is 4 years old. Therefore, raising a new PR which covers some additional cases.

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


2 months ago
#17

  • Keywords has-patch added; needs-patch removed

Core Trac: https://core.trac.wordpress.org/ticket/53348

Problem

Visiting wp-login.php with certain query strings results in a blank page instead of the login form. The affected URLs are:

wp-login.php?action=checkemail
wp-login.php?checkemail=foo (any value other than confirm or registered)
Additionally, visiting wp-login.php?action=checkemail triggers two PHP notices:

Notice: Undefined index: checkemail in wp-login.php on line 1217
Notice: Undefined index: checkemail in wp-login.php on line 1227

Root Cause

In the checkemail switch case, $_GETcheckemail? was accessed directly without an isset() check. When checkemail is not present in the query string (e.g. ?action=checkemail), PHP fires an "Undefined index" notice for both the if and elseif comparisons.

Furthermore, there was no else fallback for unexpected or missing checkemail values. When neither 'confirm' nor 'registered' matched, the code called login_header() and login_footer() with an empty $errors object — rendering a page with no content, no form, and no message.

Solution

Two targeted changes inside the checkemail switch case in [vscode-webview://06h8mmh3l0rub6ucnbbpeqvqebdb26rk7krdfbk8ba35ibg73ftq/src/wp-login.php src/wp-login.php]:

Added isset() guards before each $_GETcheckemail? comparison to eliminate the PHP notices:

if ( isset( $_GETcheckemail? ) && 'confirm' === $_GETcheckemail? ) {

} elseif ( isset( $_GETcheckemail? ) && 'registered' === $_GETcheckemail? ) {
Added an else fallback that redirects to the login URL for all unexpected or missing checkemail values:

} else {

wp_redirect( wp_login_url() );
exit;

}
This is consistent with existing behaviour — when an unrecognised ?action= value is supplied, WordPress falls back to the login form (line 509–510). No user-supplied input is used in the redirect, so there is no open redirect vulnerability.

Note: See TracTickets for help on using tickets.