WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 months ago

#44901 reviewing enhancement

Remove unneeded WP_Error in confirmaction

Reported by: garrett-eclipse Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch needs-testing
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 (1)

44901.diff (839 bytes) - added by garrett-eclipse 4 months ago.
Initial Code Restructuring

Download all attachments as: .zip

Change History (10)

#1 @birgire
5 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
5 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
5 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
5 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
4 months ago

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

#6 @desrosj
4 months ago

  • Keywords needs-patch added

@garrett-eclipse
4 months ago

Initial Code Restructuring

#7 @garrett-eclipse
4 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
4 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.


4 months ago

Note: See TracTickets for help on using tickets.