Remove unneeded WP_Error in confirmaction

[garrett-eclipse]

In the 'confirmaction' case found in wp-login.php if confirm_key is missing from the request it fails but not before running all the tests in wp_validate_user_request_key. We can modify the codeblock to check the confirm_key right away and die.

Code I'm referring to;
​https://github.com/WordPress/WordPress/blob/c952f8f98f5b34210c0bf16033c935408ac1a64e/wp-login.php#L867-L882

Suggested update moving the wp_die for the confirm_key check to the top;

<?php
if ( ! isset( $_GET['request_id'] ) ) {
        wp_die( __( 'Invalid request.' ) );
}

if ( ! isset( $_GET['confirm_key'] ) ) {
        wp_die( __( 'Missing Confirm Key.' ) );
}

$request_id = (int) $_GET['request_id'];
$key        = sanitize_text_field( wp_unslash( $_GET['confirm_key'] ) );
$result     = wp_validate_user_request_key( $request_id, $key );

if ( is_wp_error( $result ) ) {
        wp_die( $result );
}

Cheers

[birgire]

Hi @garrett-eclipse, your code rearrangements looks more readable imho, but from the code link, it seems that wp_validate_user_request_key() does not run if confirm_key is missing, because it's wrapped inside the if check:

if ( isset( $_GET['confirm_key'] ) ) {
	$key    = sanitize_text_field( wp_unslash( $_GET['confirm_key'] ) );
	$result = wp_validate_user_request_key( $request_id, $key );
} else {
	$result = new WP_Error( 'invalid_key', __( 'Invalid key' ) );
}

or did I maybe misunderstand your suggestion?

[garrett-eclipse]

Thanks @birgire

You're right that's my bad, my initial intentions was to avoid the extra WP_Error object that wasn't needed. But then thought the change would be better accepted if it was also avoiding the other function call but I guess it already did and I just rushed the description.

So mostly aside from the readability is dropping the need for the extra WP_Error object if the confirm_key doesn't exist.

[birgire]

Replying to garrett-eclipse:

Thanks @birgire

You're right that's my bad, my initial intentions was to avoid the extra WP_Error object that wasn't needed. But then thought the change would be better accepted if it was also avoiding the other function call but I guess it already did and I just rushed the description.

So mostly aside from the readability is dropping the need for the extra WP_Error object if the confirm_key doesn't exist.

aha no problem, thanks for the clarification, what about adjusting the title of the ticket, that it's about dropping that extra WP_Error object?

[garrett-eclipse]

Good call thanks @birgire

[garrett-eclipse]

Initial Code Restructuring

[garrett-eclipse]

Alright finally setup my local environment here so I can start patching things.
@desrosj or @SergeyBiryukov would you mind reviewing/testing before we mark to commit
Thank you

[garrett-eclipse]

Uploaded new patch to fix the isset so it uses confirm_key properly, was a copy-paste error w/ request_id being checked twice in 44901.diff. Otherwise this applies cleanly.

[birgire]

Thanks for the update @garrett-eclipse

I noticed that the patch in 44901.2.diff​ is quite large, as it contains many changes to package-lock.json.

[garrett-eclipse]

Attempting manual upload

[garrett-eclipse]

Thanks @birgire I'm not sure what is going on with grunt upload_patch but none of the .diff files in my working directory hold anything excess. Just renamed and manually uploaded the corrected file. Will have to look at my other patches from tonight and make sure none of the rest went awry.

[garrett-eclipse]

Hi @birgire I was hoping you could retest w/ patch 44901.4.diff, it's still applying nicely to trunk so am moving into 5.2 as we should be able to wrap it up together. Cheers

[birgire]

@garrett-eclipse sure, I will test it before the bug scrub next Monday.

[birgire]

I tested this successfully. Here's how I tested it:

• Create an export request.
• Remove the confirm_key in ​https://example.com/wp-login.php?action=confirmaction&request_id=123&confirm_key=... and open it in the browser.

Before:

• ​https://example.com/wp-login.php?action=confirmaction&request_id=123 displays the "Invalid key" wp_die message.
• ​https://example.com/wp-login.php?action=confirmaction&request_id=123&confirm_key=INVALID displays the "Invalid key" wp_die message.

After:

• ​https://example.com/wp-login.php?action=confirmaction&request_id=123 displayes the "Missing confirm key." wp_die message.
• ​https://example.com/wp-login.php?action=confirmaction&request_id=123&confirm_key=INVALID displays the "Invalid key" wp_die message.
• Applying a valid confirm key also works successfully after the change.

The patch changes the existing "Invalid key" to "Missing confirm key." when $_GET['confirm_key'] is not set.

PS:

If the request_id is missing then we get "Invalid key" too. I wonder if that should also be "Missing request ID." when $_GET['request_id'] is not set.?

The "Invalid key" string is also used seven times in check_password_reset_key( $key ) to check the dynamic $key input. I noticed it's missing the ending period, like in "Invalid key.". I guess the reason to use this string in the privacy implementation, was to reuse an existing string.

There's also a missing end period for "Invalid key" and "Invalid action" in wp_validate_user_request_key(), but in there we have "Invalid request." with the ending period.

[garrett-eclipse]

Updated patch to place periods on 'Invalid key', and switch error to 'Missing confirm key.'

[garrett-eclipse]

Thanks @birgire I appreciate the thorough testing and feedback.

I've provided 44901.5.diff to cover your feedback and updated;

• All instances of 'Invalid key' to 'Invalid key.' with ending period.
• Updated the error when $key is empty to state 'Missing confirm key.' which is used in the confirmaction.
• Updated the die message when request_id isn't set on $_GET to 'Missing request ID.'

Can you give it a once over when you have a chance and we'll move this forward.
Cheers

[birgire]

Thanks @garrett-eclipse

I did some more manual testing that worked as expected:

Missing request_id:
​https://example.org/wp-login.php?action=confirmaction&confirm_key=QgaEc8mkBVXZZjvW0vLf
Displays "Missing request ID." as expected.

Missing confirm_key:
​https://example.org/wp-login.php?action=confirmaction&request_id=199
Displays: "Missing confirm key." as expected.

Missing both confirm_key and request_id:
​https://example.org/wp-login.php?action=confirmaction
Displays: "Missing request ID." as expected.

Missing confirmaction action:
​https://example.org/wp-login.php?request_id=199&confirm_key=QgaEc8mkBVXZZjvW0vLf
Displays login screen as expected.

Invalid confirm_key:
​https://example.org/wp-login.php?action=confirmaction&request_id=199&confirm_key=INVALID
Displays: "Invalid key." as expected.

Invalid request_id:
​https://example.org/wp-login.php?action=confirmaction&request_id=INVALID&confirm_key=QgaEc8mkBVXZZjvW0vLf
​https://example.org/wp-login.php?action=confirmaction&request_id=999999999999&confirm_key=QgaEc8mkBVXZZjvW0vLf
Displays: "Invalid request." as expected.

Valid export link:
​https://example.org/wp-login.php?action=confirmaction&request_id=199&confirm_key=QgaEc8mkBVXZZjvW0vLf
Displays: "Thanks for confirming your export request." etc as expected.

Try same export link again:
​https://example.org/wp-login.php?action=confirmaction&request_id=199&confirm_key=QgaEc8mkBVXZZjvW0vLf
Displays: "This link has expired." as expected.

[garrett-eclipse]

Awesome, thanks @birgire for testing. Let's move this forward to get a final review from a committer.

[desrosj]

In 44930:

General: Ensure error messages end with a period for consistency.

Props garrett-eclipse, birgire.
See #44901.

[desrosj]

In 44931:

Privacy: Remove unnecessary WP_Error when handling confirmaction requests.

By reordering the logic when handling the confirmaction action in wp-login.php, the need for a new WP_Error object to be created can be eliminated. The error message can be passed directly into a wp_die() call, matching the other validation errors in related code.

Props garrett-eclipse, birgire.
Fixes #44901.

[desrosj]

In 44932:

Coding Standards: Fix PHPCS issue introduced in [44931].

See #44901.