-
-
Notifications
You must be signed in to change notification settings - Fork 825
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.
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
Fix fatal error when firebase/php-jwt library is 'replaced' in composer #28971
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
Approach sounds reasonable to me. For target branch, I think it's fair game for |
I'm happy with 5.70 |
Rebased for 5.70 |
$useKeyObj = version_compare(\Composer\InstalledVersions::getVersion('firebase/php-jwt'), '6', '>='); | ||
// Version 6.x+ has 2 parameters, earlier versions had 3 | ||
$reflection = new \ReflectionMethod('Firebase\JWT\JWT::decode'); | ||
$useKeyObj = ($reflection->getNumberOfParameters() === 2) ?? FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this conditional distinguishes Firbase JWT v5.x from v6.0 -- but then it misinterprets v6.6+.
So if your build has v6.6 (as in the D9 E2E tests), then it misfires and gives it the v5.x data:
Error: Cannot pass parameter 3 by reference in Civi\Crypto\CryptoJwt->decode() (line 92 of /Users/totten/bknix/build/build-0/vendor/civicrm/civicrm-core/Civi/Crypto/CryptoJwt.php)
#0 /Users/totten/bknix/build/build-0/vendor/civicrm/civicrm-core/ext/authx/Civi/Authx/CheckCredential.php(96): Civi\Crypto\CryptoJwt->decode('')
#1 [internal function]: Civi\Authx\CheckCredential->bearerJwt(Object(Civi\Authx\CheckCredentialEvent), 'civi.authx.chec...', Object(Civi\Core\UnoptimizedEventDispatcher))
Overview
#28055 updates recommended php-jwt to 6.x and handles a signature change on
JWT::decode
(old version 3 parameters, new version 2 parameters).The problem is that the composer version check doesn't work if an extension also uses php-jwt (eg. https://github.com/mjwconsult/nz.co.fuzion.civixero/blob/mjw/composer.json#L7) because the check returns version = 0.
This PR performs a more direct check of the function to see if it actually is defined with 2 or 3 parameters which should never fail and is independent of the actual version.
Before
Crash due to library conflicts if firebase/php-jwt is required in more than one place.
After
No crash, breaking changes are inspected and handled.
Technical Details
#28055 was merged in 5.69 so this is a potencial issue from that version.
Comments