-
Notifications
You must be signed in to change notification settings - Fork 367
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
Accept doubles (microseconds) in JWT timestamps when verifying claims. #282
base: master
Are you sure you want to change the base?
Conversation
Also see discussion here lcobucci/jwt#229
* $nbf instead of $exp in not before check. * Parenthesis setting corrected
Hi. Thank you for your contribution. I am ok with the patch. Would you consider writing a simple unit test for it? |
Hello @azmeuk If you will accept that the |
The patch I proposed here should also fix this issue: #287 (comment) Delegating to a third-party JWT library (I suggest using |
we should do a close analysis which jwt lib to use - there is also firebase/php-jwt .... |
My 2 cents: I did just that this week because I'm writing a library implementing the Solid OIDC specification to be released next week during SymfonyLive. I tried to implement Solid OIDC using Lcobucci JWT, Firebase JWT, and finally JWT Framework. As of today, JWT Framework is the only one supporting all features required by Solid OIDC (including OAuth DPoP): JWK, JWKS and ECDSA. |
closes #298 |
Is there still a plan to fix this issue? |
@@ -1027,8 +1027,8 @@ protected function verifyJWTclaims($claims, $accessToken = null) { | |||
return (($this->issuerValidator->__invoke($claims->iss)) | |||
&& (($claims->aud === $this->clientID) || in_array($this->clientID, $claims->aud, true)) | |||
&& (!isset($claims->nonce) || $claims->nonce === $this->getNonce()) | |||
&& ( !isset($claims->exp) || ((gettype($claims->exp) === 'integer') && ($claims->exp >= time() - $this->leeway))) | |||
&& ( !isset($claims->nbf) || ((gettype($claims->nbf) === 'integer') && ($claims->nbf <= time() + $this->leeway))) | |||
&& ( !isset($claims->exp) || ((gettype($claims->exp) === 'integer' || gettype($claims->exp) === 'double') && ($claims->exp >= time() - $this->leeway))) |
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.
It would be cleaner to use in_array for this. As example:
(in_array(gettype($claims->exp), ['double', 'float', 'integer']) && ($claims->exp >= time() - $this->leeway)))
Hi there
When implementing your library we noticed that tokens fail verification.
After some debugging we noticed, that the iobucci/jwt library issues timestamps in milliseconds (double) which fails your claim verification which expects an integer.
There was a discussion at the iobucci/jwt library regarding this topic ending in a wontfix.
RFC7519 says
NumericDate
A JSON numeric value representing the number of seconds from
1970-01-01T00:00:00Z UTC until the specified UTC date/time,
ignoring leap seconds. This is equivalent to the IEEE Std 1003.1,
2013 Edition [POSIX.1] definition "Seconds Since the Epoch", in
which each day is accounted for by exactly 86400 seconds, other
than that non-integer values can be represented. See RFC 3339
[RFC3339] for details regarding date/times in general and UTC in
particular.
In our opinion the definition is not clear as it not says explicit the seconds timestamp MUST be an integer.
It can be read as implicit definition of an integer if "other than that non-integer values" is meant regarding RFC 3339 which is mostly about Date/Time representation as string.
This PR alters your claim verification by allowing integer and double as type for the timestamps.
List of common tasks a pull request require complete