bugfix: 'kid' not in given key list by stampycode · Pull Request #129

bugfix: 'kid' not in given key list by stampycode · Pull Request #129 · firebase/php-jwt

if 'kid' value is not found in the given key map, should throw an exception.
Instead, it was outputting a php warning for using an undefined index, resulting in a null key.

if 'kid' value is not found in the given key map, should throw an exception.
Instead, it was outputting a php warning for using an undefined index, resulting in a null key.

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).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

		if (is_array($key) \|\| $key instanceof \ArrayAccess) {
		if (isset($header->kid)) {
		if(!isset($key[$header->kid])) {
		throw new UnexpectedValueException('"kid" not found in key map, unable to lookup correct key');

Choose a reason for hiding this comment

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

The kid not found in the map indicates an invalid kid, so a better error message would be simply Invalid "kid". Another option would be to set the value of $key to null here, so the OpenSSL unable to verify data error is thrown. I prefer the first option, however.

Choose a reason for hiding this comment

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

I'd prefer to have a distinct exception here, as the kid not being present in the list can then be used to trigger the key list being re-downloaded from the source, and updating the locally cached key list. I have a suspicion the key-list download endpoint is deliberately slow to encourage developers the cache the list and update periodically...

		}
		if (is_array($key) \|\| $key instanceof \ArrayAccess) {
		if (isset($header->kid)) {
		if(!isset($key[$header->kid])) {

Choose a reason for hiding this comment

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

We need a space after if

Any chance this could be merged? When using this lib to validate tokens provided by Firebase Custom Authentication, public keys are cycled regularly. If a JWT is presented which specifies a kid no longer in the current list of public keys, the lib will throw an ErrorException rather than an UnexpectedValueException.

Thanks.

Choose a reason for hiding this comment

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

@palminha The kid field is optional for the JWT spec, but it is used in this implementation, so it is required here. Requiring a kid is an application-specific requirement. If the kid field is not populated then the JWT is valid according to the spec, but is missing the information required to complete the request.