Check haveibeenpwned API during password reset and account creation #170
Conversation
| $prefix = substr( $hash, 0, 5 ); | ||
| $suffix = substr( $hash, 5 ); | ||
|
|
||
| $response = wp_remote_get( self::API_URL . $prefix ); |
There was a problem hiding this comment.
issue: As per https://haveibeenpwned.com/API/v3#UserAgent, we need to specify the User Agent. The docs state:
The user agent should accurately describe the nature of the API consumer such that it can be clearly identified in the request. Not doing so may result in the request being blocked.
I think specifying 10up Experience WordPress Plugin should be sufficient, thoughts?
There was a problem hiding this comment.
Additionally, should we cache this request/response to stop us spamming the API if someone resubmits the same pass multiple times?
| * | ||
| * @return bool True if password is ok, false if it shows up in a breach. | ||
| */ | ||
| protected function is_password_secure( $password ): bool { |
There was a problem hiding this comment.
thought (non-blocking): What are your thoughts on allowing engineers to opt-out of this functionality via a const or a filter?
| @@ -307,6 +312,11 @@ public function validate_strong_password( $errors, $user_data ) { | |||
| return $errors; | |||
| } | |||
|
|
|||
| // Validate the password against the Have I Been Pwned API. | |||
| if ( ! $this->is_password_secure( $password ) && is_wp_error( $errors ) ) { | |||
| $errors->add( 'password_reset_error', __( '<strong>ERROR:</strong> The password entered may have been included in a data breach and is not considered safe to use. Please choose another.', 'tenup' ) ); | |||
There was a problem hiding this comment.
thought (non-blocking): Do we want to trigger an error here, or should we add a warning to the screen pre-update? Alternatively, should there be a checkbox, like the weak password box, that lets them bypass this if they really want to use that password?
| /** | ||
| * Stores the Have I Been Pwned API URL | ||
| */ | ||
| const API_URL = 'https://api.pwnedpasswords.com/range/'; |
There was a problem hiding this comment.
suggestion: Let's use a more descriptive name for this const. We might want to introduce additional APIs in the future.
| if ( is_wp_error( $response ) ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
question (non-blocking): Remond me, does wp_remote_get() always return a WP_Error for non-200 responses? If not, should we also check wp_remote_retrieve_response_code()?
Description of the Change
Verify new password against the Have I Been Pwned API during password change (on profile or via reset process)
Closes #77
How to test the Change
Changing password through profile
london123)disabledattribute on the Update Profile button - this is added through JS when a "weak" password is identifiedReset password process
london123)disabledattribute on the Save Password button - this is added through JS when a "weak" password is identifiedChangelog Entry
Credits
Props @jamesmorrison
Checklist: