search

alphagov / notifications-php-client

PHP client for the GOV.UK Notify API
https://docs.notifications.service.gov.uk/php.html
MIT License
11 stars 13 forks source link

Bump firebase/php-jwt to ^v6.1 #107

Closed acsauk closed 2 years ago

acsauk commented 2 years ago

What problem does the pull request solve?

Fixes https://security.snyk.io/vuln/SNYK-PHP-FIREBASEPHPJWT-2434829. This is a vulnerability flagged in our repo - https://github.com/ministryofjustice/opg-digideps.

I'm hoping there aren't any other breaking changes in moving from v5 -> v6 but as I can't run the integration tests without valid creds I had to just rely on unit tests (which are passing now). Happy to pair with a Notify dev to get the fix out if required, just let me know.

Checklist

idavidmcdonald commented 2 years ago

Hi @acsauk, thanks so much for this!

I'm not a PHP expert so I want to call out my assumptions and understandings of this PR to get a sense check from you please.

  1. The Notify client itself isn't vulnerable to https://security.snyk.io/vuln/SNYK-PHP-FIREBASEPHPJWT-2434829 given we are only creating JWTs and are not decoding/validating them.
  2. However, users of the Notify client may be using firebase/php-jwt in their own code to decode and validate JWTs and therefore if they are stuck on a version below 6.0.0 because of Notify then their own code may be vulnerable. This is why it is important to make the above change
  3. Version v6.1.0 drops support for PHP 5.3, 5.4, 5.5, 5.6 and 7.0. Therefore requiring^v6.1.0 means that we are potentially forcing users to upgrade their PHP version to >=7.1. Note, 7.3 and below are currently end of life (https://www.php.net/supported-versions.php) but I have no idea how up to date our users applications are.
  4. Requiring users to use version ^v6.1.0 will mean that anyone using firebase/php-jwt in their own code may be required to make changes to it due to the breaking changes introduced in v6.0.0 (https://github.com/firebase/php-jwt/releases?page=1)
  5. Given points 3 and 4, this change may not be backwards compatible for those running PHP < 7.0 or for those who are using firebase/php-jwt in their own code.

If my understanding is correct above, I think this should be a breaking change and we should release it under version 4.0.0 rather than 3.3.0. What do you think about that? Have I missed anything? Note, versioning libraries is hard and not a perfect science.

But apart from that, all the code checks out and I've created a new branch for security reasons that we have run our integration tests against and it looks happy - https://github.com/alphagov/notifications-php-client/pull/108.

acsauk commented 2 years ago

Hey @idavidmcdonald - yeah I would agree with all of your points above and makes sense to make it a major version release given the requirement for PHP 7.1+. Cant see anything you've missed 👍

I'll push the version bump up now!

idavidmcdonald commented 2 years ago

Right, this has now been released from a separate PR. You now have version 4.0.1 available to download (https://packagist.org/packages/alphagov/notifications-php-client). Let us know if you spot any problems.

Note, I spotted an extra thing we missed so that is why there is a version 4.0.1 rather than just 4.0.0.

Thanks for your help in raising this and doing the hard work to make the changes. It is really really appreciated!

Cheers!