Skip to content
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

Encode timestamps as float instead of string #618

Conversation

StephenBeirlaen
Copy link

Currently all timestamps are rendered as strings.
Some JWT parsers like auth0/node-jsonwebtoken and jwt.io complain about timestamps formatted as a string. This PR changes this to encode timestamps as float instead of string values.

@StephenBeirlaen StephenBeirlaen changed the title Patch float timestamps Encode timestamps as float instead of string Dec 23, 2020
Some JWT parsers like auth0/node-jsonwebtoken and jwt.io complain about timestamps formatted as a string.
@Slamdunk
Copy link
Collaborator

Hi, I'm not sure if this should be considered a Bugfix or a BC Break: this library would work seamlessly, but other could not. Let's wait for @lcobucci reply.

In the meantime you can implement your own Formatter as explained in https://github.com/lcobucci/jwt/blob/4.0.0/docs/extending-the-library.md#claims-formatter

@StephenBeirlaen
Copy link
Author

Thanks for your comment. I found my way to the explanation on how to implement my own Formatter, but I am using this library indirectly through league/oauth2-server which is depended on by trikoder/oauth2-bundle. This bundle does not allow me to configure the builder, so I would need to make changes there.

But since I would argue that string-encoded timestamps are the exception to the rule, the more straightforward option was to change this at the source (this library).

Therefore it is my suggestion to make int/float encoding the default behaviour, since I believe that this will cover the majority of the use-cases.
In the case that you expect floats to be encoded as a string, you would then need a custom formatter, rather than otherwise.

@lcobucci
Copy link
Owner

I didn't see any issue on jwt.io: https://jwt.io/#debugger-io?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoiMTUxNjIzOTAyMi4xNDk1ODIifQ.ALrWTFUa7CqfKjX-38X6bYvYSTzWOP1V8kGYDssr6P4

Nevertheless, this is a BC-break that I completely disagree with. Using floating points will certainly lead to rounding problems, which is what you don't want to have when you need the microseconds precision.

@Slamdunk created a PR that puts the UNIX timestamp formatter in the lib (#617), that should be the path forward.

If you really need to use floats for your tokens you should use your own claim formatter.

@StephenBeirlaen
Copy link
Author

Just wanted to add that jwt.io does parse the JWT, but marks dates as invalid. Try hovering over the iat in your example.

@Slamdunk
Copy link
Collaborator

Slamdunk commented Dec 24, 2020

I am using this library indirectly through league/oauth2-server which is depended on by trikoder/oauth2-bundle. This bundle does not allow me to configure the builder

I guess you are referring to https://github.com/thephpleague/oauth2-server/blob/622eaa1f28eb4a2dea0cfc7e4f5280fac794e83c/src/Entities/Traits/AccessTokenTrait.php#L63

You should then push a PR there to enable custom formatters to be passed to the builder

@softwarespot
Copy link

softwarespot commented Feb 21, 2021

The number precision could have still been stored as a number e.g. 1613412329.537179 is a valid JSON number.

JSON.stringify({exp: 1613412329.537179})
"{"exp":1613412329.537179}" 

But, the specification is clear that this must be a number and says that there can be "leeway", but it's in minutes, not milliseconds.

Implementers MAY provide for some small leeway, usually no more than
   a few minutes, to account for clock skew.  Its value MUST be a number
   containing a NumericDate value.  Use of this claim is OPTIONAL.

The real issue is that other packages in the wild e.g. I am using a common Go package "jwt-go" are designed using the specification, so when I found out my code wasn't working with newer tokens (we have a service written in PHP, that generates these tokens), I spent a few hours tracking down the problem, to eventually just implementing my own custom formatter (at that point, I thought the Go code was at fault, until I tracked it down to here). I am wondering how many others will be affected by this specification change?

I also get why you won't fix due to backwards compatibility, but then it would be good to mention in the documentation that they are strings, not numbers and warn others that this package is not spec compliant.

Thanks for considering

@lcobucci
Copy link
Owner

lcobucci commented Feb 21, 2021

As I mentioned before, using floats to represent microseconds will present rounding/precision issues (on PHP < 8.0). Here are a few examples:

var_dump((float) '1613938511.017448'); // float(1613938511.0174)
var_dump((float) '1613938511.023691'); // float(1613938511.0237)
var_dump((float) '1613938511.018045'); // float(1613938511.018)

Since we still have to support PHP 7.4 (for now), I've opted for accuracy and consistency. I believe that when you need microsecond precision, you must not have the value being changed.

I'll happily merge a new formatter that uses floating points instead of strings and that can/will become the default one on the next major release - and users are well aware of the possible caveats of this implementation.

then it would be good to mention in the documentation that they are strings, not numbers and warn others that this package is not spec compliant.

IMHO the formatter doesn't invalidate compliance - especially because there're ways to mitigate the issue. However, I do reckon that documentation can be improved.

@softwarespot would you like to contribute to these two things? Doc improvements can be sent to branch 4.0.x and the new implementation to 4.2.x 👍

@softwarespot
Copy link

Thanks for your reply @lcobucci. I am not a PHP developer anymore, so I have completely forgotten these little details with floating point precision in PHP. So thanks for the reminder.

But sure, I can do the documentation changes and the new implementation, would that be a major version change?

@lcobucci
Copy link
Owner

would that be a major version change?

No, it would be a minor one because we'd be providing an alternative implementation to live side-by-side.

@corbosman
Copy link

I know there have been several issues about this already, and that the current plan seems to be to change the exp etc fields to a float, but that still breaks the spec. Here is what the spec says:

"Its value MUST be a number containing a NumericDate value".

Then if you check what a NumericDate is, that is explained as well in the same spec:

" 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
"

It's a bit unclear what they mean with "other than that non-integer values can be represented.", but if you check POSIX.1, it never mentions milliseconds, and specifically mentions seconds everywhere.

Regardless how one interprets this (personally I think it's pretty clear they want this number to be in seconds), basically every single one of our interfaces with other languages/libs broke.

@lcobucci
Copy link
Owner

lcobucci commented Mar 18, 2021

@corbosman the library already provides all the necessary things for you to use seconds - as long as you configure things. I'm not sure what else you expect to happen.

@corbosman
Copy link

Sure, if you use this library directly, but I use it through laravel passport which does not provide much in the form of configuration. I guess I just don't understand why you'd choose to deviate from the spec breaking any compatibility with the rest of the JWT world by default. Why not opt to let people configure microseconds if they really need it.

Anyways, I'm not expecting anything, I'll find a way to work around it.

@lcobucci
Copy link
Owner

@corbosman worst case scenario can be new DateTimeImmutable('@' . time()), right?

Also, the usage of microseconds isn't mentioned on the JWT RFC because it's covered by the RFC 3339 (as time-secfrac). So, we're not deviating from the spec here.

The usage of strings instead of floats is explained above and will be kept as that until we can prove we won't have rounding issues like these: https://3v4l.org/NCNT7

Why not opt to let people configure microseconds if they really need it.

That's is a valid request, honestly. A BC-break, but a valid request.
We can surely ship a v5 and go back on the decision of making that the default. It's slightly frustrating for me, though, since we went through several pre-release versions and didn't hear a thing from anybody.

@corbosman
Copy link

I already maintain a package that allows people to add custom claims to JWT tokens in Laravel Passport (https://github.com/corbosman/laravel-passport-claims). What I ended up doing is extending that package to also allow custom formatters. Thanks for at least listening, and maybe you'll end up switching the default. If not, at least I can now configure the UnixTimestampDates formatter (this is not normally possible in Laravel).

@zerkms
Copy link

zerkms commented Mar 18, 2021

@lcobucci it's a RFC7519 violation still, the library should not generate invalid tokens. It only allows JSON numeric values (either int or any other valid numeric representation).

@lcobucci
Copy link
Owner

@zerkms I understand you point, let's find a solution for the rounding issue instead of flooding this with unnecessary messages?

There's a PR trying to address that, if it provides the necessary assertions we'll be changing things.

@zerkms
Copy link

zerkms commented Mar 19, 2021

Inability to deserialise data with higher precision than the language built-in types can represent (eg: php's float) is not a library's problem, but a language's "problem".

See another extreme example: in php strings are limited by the size of 2Gb, yet - lcobucci/jwt won't try to solve the limitation in case if somebody wanted to store an iat with 3 billion digits after the decimal point.

To summarise: decoding json and representing values accurately is a programming language domain, not this library domain.

Repository owner locked and limited conversation to collaborators Mar 19, 2021
@lcobucci
Copy link
Owner

Hello folks, thanks to @yassinrais we managed to fix this compatibility issue whilst still preserving the microseconds precision. We already released new versions to solve the problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants