10up / 10up-experience

The 10up Experience plugin configures WordPress to better protect and inform clients, aligned to 10up’s best practices.
GNU General Public License v2.0
129 stars 27 forks source link

Password reset triggers fatal error #80

Closed oscarssanchez closed 1 year ago

oscarssanchez commented 4 years ago

Describe the bug

It looks like resetting the password can trigger a fatal error sometimes. Pasting the output of the error log:

#4 in /chroot/home/client/client.com/html/wp-content/plugins/10up-experience/vendor/bjeavons/zxcvbn-php/src/Matchers/SequenceMatch.php on line 40
#3 /chroot/home/client/client.com/html/wp-content/plugins/10up-experience/includes/classes/Authentication/Passwords.php(266): TenUpExperience\Authentication\Passwords->validate_strong_password(Object(WP_Error), Object(stdClass))
#2 /chroot/home/client/client.com/html/wp-content/plugins/10up-experience/includes/classes/Authentication/Passwords.php(321): ZxcvbnPhp\Zxcvbn->passwordStrength('ptjFN0BLCIxvDaI...')
#1 /chroot/home/client/client.com/html/wp-content/plugins/10up-experience/vendor/bjeavons/zxcvbn-php/src/Zxcvbn.php(73): ZxcvbnPhp\Matcher->getMatches('ptjFN0BLCIxvDaI...', Array)
#0 /chroot/home/client/client.com/html/wp-content/plugins/10up-experience/vendor/bjeavons/zxcvbn-php/src/Matcher.php(39): ZxcvbnPhp\Matchers\SequenceMatch::match('ptjFN0BLCIxvDaI...', Array)
Stack trace:
[02-Nov-2020 19:29:55 UTC] PHP Fatal error:  Uncaught Error: Call to undefined function ZxcvbnPhp\Matchers\mb_ord() in /chroot/home/client/client.com/html/wp-content/plugins/10up-experience/vendor/bjeavons/zxcvbn-php/src/Matchers/SequenceMatch.php:40

Steps to Reproduce

  1. Go to user page
  2. Reset password (I tried with an administrator and a subscriber)
  3. It works for subscriber
  4. It triggered a fatal error with the administrator account

Expected behavior

Password reset should not trigger the error

Screenshots

Environment information

Additional context

bengreeley commented 3 years ago

@oscarssanchez I noticed this on another site and discovered the mbstring extension wasn't installed on the environment, which was causing the issue. It looks like the PHP dependency Zxcvbn relies on this extension. That dependency is used to determine the password strength, so if we were to change out the method to one that doesn't use mbstring, it would resolve the issue. Otherwise, we'll need to indicate that the plugin reliesd on the mbstring PHP extension.

cc @tlovett1

tlovett1 commented 3 years ago

Can't we just check if that function exists and, if not, bail on that functionality?

bengreeley commented 2 years ago

@tlovett1 Checking for a mbstring function sounds like a good short-term approach to prevent the error. The downside is the 10up Exp strong password functionality won't work on websites that don't have the mbstring PHP extension installed so that would be good to have documented so the dependencies are understood .

tlovett1 commented 2 years ago

Yea, definitely needs to be documented but I feel like 90%+ websites have that extension. Can you put together a PR?

MARQAS commented 2 years ago

We are experiencing a fatal error while trying to reset a password after updating the site to PHP8. Here's what the log says:

"NOTICE: PHP message: PHP Parse error:  syntax error, unexpected token

"match", expecting variable in

/var/www/html/client/public/wp-content/plugins/10up-experience/vendor/bjeavons/zxcvbn-php/src/Matcher.php

on line 92"
darylldoyle commented 1 year ago

This seems to have been resolved via #98, so closing this issue.