Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#44901 closed enhancement (fixed)

Remove unneeded WP_Error in confirmaction

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: garrett-eclipse's profile garrett-eclipse
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch commit
Focuses: Cc:

Description

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

Attachments (5)

44901.diff (839 bytes) - added by garrett-eclipse 6 years ago.
Initial Code Restructuring
44901.2.diff (63.2 KB) - added by garrett-eclipse 6 years ago.
44901.3.diff (63.2 KB) - added by garrett-eclipse 6 years ago.
44901.4.diff (840 bytes) - added by garrett-eclipse 6 years ago.
Attempting manual upload
44901.5.diff (3.8 KB) - added by garrett-eclipse 6 years ago.
Updated patch to place periods on 'Invalid key', and switch error to 'Missing confirm key.'

Download all attachments as: .zip

Change History (27)

#1 @birgire
6 years ago

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?

#2 follow-up: @garrett-eclipse
6 years ago

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.

#3 in reply to: ↑ 2 @birgire
6 years ago

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?

#4 @garrett-eclipse
6 years ago

  • Summary changed from Die earlier if confirm_key missing, no need to execute wp_validate_user_request_key to Remove unneeded WP_Error in confirmaction

Good call thanks @birgire

#5 @SergeyBiryukov
6 years ago

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

#6 @desrosj
6 years ago

  • Keywords needs-patch added

@garrett-eclipse
6 years ago

Initial Code Restructuring

#7 @garrett-eclipse
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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

#8 @pento
6 years ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


6 years ago

#10 @garrett-eclipse
6 years ago

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.

#11 @birgire
6 years ago

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
6 years ago

Attempting manual upload

#12 @garrett-eclipse
6 years ago

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.

#13 @garrett-eclipse
6 years ago

  • Milestone changed from Future Release to 5.2
  • Owner changed from SergeyBiryukov to garrett-eclipse
  • Status changed from reviewing to accepted

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

#14 @birgire
6 years ago

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

#15 @birgire
6 years ago

  • Keywords needs-testing removed

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


Before:


After:


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
6 years ago

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

#16 @garrett-eclipse
6 years ago

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

#17 @garrett-eclipse
6 years ago

  • Keywords needs-testing added

#18 @birgire
6 years ago

  • Keywords needs-testing removed

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.

#19 @garrett-eclipse
6 years ago

  • Keywords commit added

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

#20 @desrosj
6 years ago

In 44930:

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

Props garrett-eclipse, birgire.
See #44901.

#21 @desrosj
6 years ago

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

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.

#22 @desrosj
6 years ago

In 44932:

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

See #44901.

Note: See TracTickets for help on using tickets.