WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 8 months ago

Last modified 8 months ago

#44901 closed enhancement (fixed)

Remove unneeded WP_Error in confirmaction

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

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 14 months ago.
Initial Code Restructuring
44901.2.diff (63.2 KB) - added by garrett-eclipse 10 months ago.
44901.3.diff (63.2 KB) - added by garrett-eclipse 10 months ago.
44901.4.diff (840 bytes) - added by garrett-eclipse 10 months ago.
Attempting manual upload
44901.5.diff (3.8 KB) - added by garrett-eclipse 9 months 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
15 months 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
15 months 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
15 months 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
15 months 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
15 months ago

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

#6 @desrosj
14 months ago

  • Keywords needs-patch added

@garrett-eclipse
14 months ago

Initial Code Restructuring

#7 @garrett-eclipse
14 months 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
14 months ago

  • Milestone changed from 4.9.9 to Future Release

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


14 months ago

#10 @garrett-eclipse
10 months 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
10 months 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
10 months ago

Attempting manual upload

#12 @garrett-eclipse
10 months 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
9 months 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
9 months ago

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

#15 @birgire
9 months 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
9 months ago

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

#16 @garrett-eclipse
9 months 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
9 months ago

  • Keywords needs-testing added

#18 @birgire
8 months 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
8 months ago

  • Keywords commit added

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

#20 @desrosj
8 months ago

In 44930:

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

Props garrett-eclipse, birgire.
See #44901.

#21 @desrosj
8 months 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
8 months ago

In 44932:

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

See #44901.

Note: See TracTickets for help on using tickets.