Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check haveibeenpwned API during password reset and account creation #170

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Check haveibeenpwned API during password reset and account creation #170

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions includes/classes/Authentication/Passwords.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ class Passwords {

use Singleton;

/**
* Stores the Have I Been Pwned API URL
*/
const API_URL = 'https://api.pwnedpasswords.com/range/';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Let's use a more descriptive name for this const. We might want to introduce additional APIs in the future.


/**
* Setup hooks
*
Expand Down Expand Up @@ -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' ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

// Should a strong password be enforced for this user?
if ( $user_id ) {

Expand Down Expand Up @@ -374,4 +384,44 @@ public function enforce_for_user( $user_id ) {

return $enforce;
}

/**
* Check if password is secure by querying the Have I Been Pwned API.
*
* @param string $password Password to validate.
*
* @return bool True if password is ok, false if it shows up in a breach.
*/
protected function is_password_secure( $password ): bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (non-blocking): What are your thoughts on allowing engineers to opt-out of this functionality via a const or a filter?

$hash = strtoupper( sha1( $password ) );
$prefix = substr( $hash, 0, 5 );
$suffix = substr( $hash, 5 );

$response = wp_remote_get( self::API_URL . $prefix );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, should we cache this request/response to stop us spamming the API if someone resubmits the same pass multiple times?


// Allow for a failed request to the HIPB API.
if ( is_wp_error( $response ) ) {
return true;
}
Comment on lines +403 to +405
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?


$body = wp_remote_retrieve_body( $response );

// Allow for a failed request to the HIPB API.
if ( is_wp_error( $body ) ) {
return true;
}

$lines = explode( "\r\n", $body );

foreach ( $lines as $line ) {
$parts = explode( ':', $line );

// If the suffix is found in the response, the password may be in a breach.
if ( $parts[0] === $suffix ) {
return false;
}
}

return true;
}
}
Loading