#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: |
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)
Change History (27)
#2
follow-up:
↓ 3
@
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
@
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
@
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
@
6 years ago
- Milestone changed from Awaiting Review to 4.9.9
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#7
@
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
This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.
6 years ago
#10
@
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
@
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
.
#12
@
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
@
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
#15
@
6 years ago
- Keywords needs-testing removed
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.
@
6 years ago
Updated patch to place periods on 'Invalid key', and switch error to 'Missing confirm key.'
#16
@
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
#18
@
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.
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 ifconfirm_key
is missing, because it's wrapped inside the if check:or did I maybe misunderstand your suggestion?