WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 8 months ago

#44601 closed defect (bug) (fixed)

Missing vallidations in function get_password_reset_key()

Reported by: edocev Owned by: pento
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.9.7
Component: Users Keywords: has-patch needs-refresh
Focuses: Cc:

Description

In the get_password_reset_key() function, with parameter WP_User $user

were missing validations if the user exists, if it's set and if it's an object of the class WP_User.

Attachments (2)

44601.diff (675 bytes) - added by edocev 14 months ago.
44601.2.diff (639 bytes) - added by edocev 14 months ago.

Download all attachments as: .zip

Change History (6)

@edocev
14 months ago

#1 follow-up: @SergeyBiryukov
14 months ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 5.0

Hi @edocev, welcome to WordPress Trac! Thanks for the patch. A few notes:

  • Validation should be done at the beginning of the function, otherwise it still generates a few notices if invalid data is passed.
  • The isset() check seems redundant, as $user is a required parameter. PHP already throws a warning if it's not passed.
  • The empty() check is redundant as well, instanceof WP_User should be enough.
  • No need for a DocBlock here, as this piece of code is neither a hook nor a separate function.
  • The error message could be a bit more clear, something like this:
    if ( ! ( $user instanceof WP_User ) ) {
    	return new WP_Error( 'invalid_user_data', __( 'Invalid user data.' ) );
    }
    

@edocev
14 months ago

#2 in reply to: ↑ 1 @edocev
14 months ago

Replying to SergeyBiryukov:

Hi @edocev, welcome to WordPress Trac! Thanks for the patch. A few notes:

  • Validation should be done at the beginning of the function, otherwise it still generates a few notices if invalid data is passed.
  • The isset() check seems redundant, as $user is a required parameter. PHP already throws a warning if it's not passed.
  • The empty() check is redundant as well, instanceof WP_User should be enough.
  • No need for a DocBlock here, as this piece of code is neither a hook nor a separate function.
  • The error message could be a bit more clear, something like this:
    if ( ! ( $user instanceof WP_User ) ) {
    	return new WP_Error( 'invalid_user_data', __( 'Invalid user data.' ) );
    }
    

Hello, thank you for the feedback! I've fixed the code and send the updated patch.

#3 @pento
11 months ago

  • Milestone changed from 5.0 to 5.1

#4 @pento
8 months ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 44602:

Users: Check that a valid user is passed to get_password_reset_key().

Props edocev.
Fixes #44601.

Note: See TracTickets for help on using tickets.