bugfix: 'kid' not in given key list #129
Conversation
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.
|
1 similar comment
|
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! |
|
CLAs look good, thanks! |
| @@ -98,6 +98,9 @@ public static function decode($jwt, $key, $allowed_algs = array()) | |||
| } | |||
| 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'); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
| @@ -98,6 +98,9 @@ public static function decode($jwt, $key, $allowed_algs = array()) | |||
| } | |||
| if (is_array($key) || $key instanceof \ArrayAccess) { | |||
| if (isset($header->kid)) { | |||
| if(!isset($key[$header->kid])) { | |||
There was a problem hiding this comment.
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 Thanks. |
There was a problem hiding this comment.
I don't think this implementation is accordingly to the spec. Spec claims that kid should be optional https://tools.ietf.org/html/rfc7515#section-4.1.4
|
@palminha The |
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.