-
Notifications
You must be signed in to change notification settings - Fork 191
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
Upgrade firebase/php-jwt to ^6.0 #388
Conversation
The firebase/php-jwt package recently had to make breaking changes to resolve a security flaw in their library. This change upgrades that library and fixes the code that broke with the upgrade. Most notably it changes OAuth2::verifyIdToken to only support a single algorithm to bring it inline with the way Jwt::decode works.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
Working on getting the CLA figured out. |
I think I got the CLA thing figured out 😄 |
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.
I worked on a way to implement this in #391, and I can't find a way to implement it without breaking backwards compatibility.
I think it would be better to break these edge case implementations for security reasons, however.
* @return object | ||
*/ | ||
private function jwtDecode($idToken, $publicKey, $allowedAlgs) | ||
private function jwtDecode($idToken, $publicKey, $allowedAlg) |
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.
unfortunately this is a backwards breaking change.
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.
I thought that might be the case. Thanks for the feedback and for the work done in #391. Ultimately, I just need the issue fixed 😄
Closing in favor of #391, which is a little more robust in handling the BC breaking issues with this change |
The firebase/php-jwt package recently had to make breaking changes to resolve a security flaw in their library. This change upgrades that library and fixes the code that broke with the upgrade.
Most notably it changes OAuth2::verifyIdToken to only support a single algorithm to bring it inline with the way Jwt::decode works.
You can find more information about the fixes to the firebase/php-jwt here:
https://github.com/firebase/php-jwt/releases/tag/v6.0.0
firebase/php-jwt#351
https://security.snyk.io/vuln/SNYK-PHP-FIREBASEPHPJWT-2434829
I also saw there was a renovote bot PR for this but it didn't fix the code so I thought this might be an opportunity to lend a hand.
Hope this helps! Let me know if there's something else I can do to get this merged. Thanks! 😄