Fix usage of non JSON numeric values for time fractions (without having precision issues) by yassinrais · Pull Request #706 · lcobucci/jwt

yassinrais · 2021-03-19T00:52:13Z

🖋 The problem of precession of timestamp floats is solved by using a json_encode instead of (string).

This solves the float round problem when we encode/parse floats .

yassinrais

Only int and float are accepted as returns from a converted date.

zerkms · 2021-03-19T07:41:20Z

After reading all other discussions (and code for this PR) I have an impression there is no understanding on what's happening:

php's float64 (and literally every other's programming language implementation of IEEE754) fails to represent precisely more than 53 bits of significand precision.

This leads to "rounding" problem (which in fact is just lack of precision in the float64 ieee754 type, nothing really "rounds").

You cannot solve it in userland using built-in numeric types only.

What MUST be done though: date fields MUST be encoded as numbers, it should not even be an option, it's REQUIRED by the spec. Ideally, all changes that led to making the generated jwt non-complaint should be reverted, because there is no way or need to "fix" it, since it only broke everything even "worse".

What can be done if one wants precision over 53 bits of significand precision they need:

Bring a custom type that allows it (no built in php types do)
Implement json de-/serialisation algorithm that would employ that type (json_decode/json_encode cannot do that)
A custom type for datetime should be implemented (the built-in php data types suffer from the same "problem" as every other numeric php type)

And once again: just shuffling (float) into random places will not change anything.

This solves the float round problem when we encode/parse floats .

The problem with that "problem" is that it never was formalised: some arbitrary values were chosen as a "proof" of having the "issue" and then the "solution" was optimised to fit only that limited problem. If the library wants to solve the problem with "rounding" floats - then it should be first defined to what point. At this point it's not obvious why losing 18th significant digit is worse than losing 15th one.

yassinrais · 2021-03-19T12:45:20Z

After reading all other discussions (and code for this PR) I have an impression there is no understanding on what's happening

If you read my old PR #702, the goal was to keep the 6 decimal digits same from a timestamp after encoding/decoding Json. If you think my solution has a problem or won't work, can you show me a case or example and explain why?

This leads to "rounding" problem (which in fact is just lack of precision in the float64 ieee754 type, nothing really "rounds").

Im talking only about the 6 digits of timestamp and not talking about floats

At this point it's not obvious why losing 18th significant digit is worse than losing 15th one.

Can you please explain what you mean by this?

Also what do you belive that can be changed to fix this "the bug is there and the referred PR is closed"

lcobucci

We're missing the test that guarantees that precision of time fractions is kept after the parsing - using that json_encode() logic.

Without that we're just asserting that the claims are now using floats - which doesn't address the concern we had before.

I'll add the test and rebase your branch. Either we prove that your approach works or that @zerkms is correct.

yassinrais · 2021-03-19T13:47:07Z

I'll add the test and rebase your branch. Either we prove that your approach works or that @zerkms is correct.

Looking forward to hearing from you 👌.

lcobucci · 2021-03-19T14:24:11Z

@zerkms do you think I'm missing something on 484430f? Just to make it 100% transparent the scope of this is to make sure the precision up to milliseconds is respected.

lcobucci · 2021-03-19T18:14:35Z

@Ocramius would you also have a look at this, please?

lcobucci · 2021-03-19T18:15:30Z

-            $claims[$claim] = $this->convertDate((string) $claims[$claim]);
+            $date = $claims[$claim];
+
+            $claims[$claim] = $this->convertDate(is_string($date) ? $date : json_encode($date, JSON_THROW_ON_ERROR));


To make this explicit as well, the check is only needed for compatibility with previously issued tokens...

Slamdunk

Awesome solution, but I think we should keep integer type for integer timestamps

yassinrais

Awesome solution, but I think we should keep integer type for integer timestamps

Yes, i remember now , That's why I left the integers and floats as returns. , because if you count, many use this library will got some problems about existing tokens that contains only integers. 👏🏻

lcobucci · 2021-03-19T19:36:27Z

I'll handle that int stuff. Thanks for the review @Slamdunk

zerkms · 2021-03-19T19:51:01Z

If you read my old PR #702, the goal was to keep the 6 decimal digits same from a timestamp after encoding/decoding Json. If you think my solution has a problem or won't work, can you show me a case or example and explain why?

Your implementation would work, I just thought this: #618 (comment) was a problem.

Im talking only about the 6 digits of timestamp and not talking about floats

Timestamp is encoded as a float.

Ocramius · 2021-03-19T19:52:09Z

@lcobucci I was kinda glancing over this earlier today, and I don't see massive problems with the patch itself. It seems fine code-wise, but I don't understand the rationale behind it: mostly lacking reference to issues connected to this. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Fri, Mar 19, 2021 at 7:14 PM Luís Cobucci ***@***.***> wrote: @Ocramius <https://github.com/Ocramius> would you also have a look at this, please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#706 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEHHKPAFNCDBP5OWXF3TEOIBZANCNFSM4ZNZFXFQ> .

The RFC-7519 states that the `NumericDate` type is: > JSON numeric value representing the number of seconds from > 1970-01-01T00:00:00Z UTC until the specified UTC date/time, ignoring > leap seconds. Then also mentions that time fractions (as covered by RFC-3339) are supported: > Seconds Since the Epoch", in which each day is accounted for by > exactly 86400 seconds, other than that non-integer values can be > represented. While adding support for time fractions we've interpreted the "non-integer" really as any "non-integer" value, and used strings to guard against precision issues. That causes issues, since a string isn't a "JSON numeric value" according to the JSON specs. We observed that the 6-digit precision is not lost when doing JSON encode/decode operations, this applies that technique to make sure we comply to the specs and have "rounding issues" when dealing with floats.

lcobucci · 2021-03-19T20:15:51Z

@Ocramius I've tried to explain the issue on the commit message: 9a961f4 does it give you more info?

Your implementation would work, I just thought this: #618 (comment) was a problem.

It is only a problem when casting to strings, that was @yassinrais' finding.

lcobucci · 2021-03-19T20:19:12Z

I've applied the changes and extracted the test to a separate place, would you all give a final check? Even though this is a minor BC-break, it's solving a compatibility issue that has been introduced since v4.0 and we should ship it as a patch.

lcobucci · 2021-03-19T21:06:51Z

@yassinrais once again, thank you very much for your help in finding the fix 👍

yassinrais · 2021-03-20T00:52:03Z

@lcobucci you're welcome !
I use it so I'm supposed to help 😁👌
It's great to see the progress made 🚀.

yassinrais commented Mar 19, 2021

View reviewed changes

yassinrais mentioned this pull request Mar 19, 2021

[BUG] Datetime should be a float instead of string #702

Closed

zerkms reviewed Mar 19, 2021

View reviewed changes

Comment thread src/Encoding/MicrosecondBasedDateConversion.php Outdated

lcobucci requested changes Mar 19, 2021

View reviewed changes

Comment thread src/Encoding/MicrosecondBasedDateConversion.php Outdated

Comment thread src/Token/Parser.php Outdated

lcobucci force-pushed the 4.0.x branch 2 times, most recently from baed0bf to ba4aa2d Compare March 19, 2021 14:22

lcobucci self-assigned this Mar 19, 2021

lcobucci added the Bug label Mar 19, 2021

lcobucci changed the title ~~🖋 The problem of precession of timestamp floats is solved by using a json_encode instead of (string)~~ Fix usage of non JSON numeric values for time fractions (without having precision issues) Mar 19, 2021

lcobucci modified the milestones: 4.1.3, 4.0.2 Mar 19, 2021

lcobucci reviewed Mar 19, 2021

View reviewed changes

lcobucci requested review from Ocramius and Slamdunk March 19, 2021 18:15

Slamdunk reviewed Mar 19, 2021

View reviewed changes

Comment thread test/unit/Encoding/MicrosecondBasedDateConversionTest.php Outdated

yassinrais commented Mar 19, 2021

View reviewed changes

Comment thread src/Encoding/MicrosecondBasedDateConversion.php Outdated

lcobucci and others added 2 commits March 19, 2021 21:00

Guard against precision issues on time fractions

ad4729f

lcobucci force-pushed the 4.0.x branch from ba4aa2d to 9a961f4 Compare March 19, 2021 20:14

lcobucci approved these changes Mar 19, 2021

View reviewed changes

Slamdunk approved these changes Mar 19, 2021

View reviewed changes

lcobucci merged commit cefaeb3 into lcobucci:4.0.x Mar 19, 2021

github-actions bot mentioned this pull request Mar 19, 2021

Merge release 4.0.2 into 4.1.x #707

Merged

This was referenced Mar 19, 2021

Encode timestamps as float instead of string #618

Closed

Changed string return type to float according to JWT specs #637

Closed

claims invalid after debugging at <https://jwt.io/> #692

Closed

expiresAt() sets a floating point value #693

Closed

lcobucci mentioned this pull request Mar 26, 2021

Invalid dates in JWT laravel/passport#1430

Closed

Uh oh!

Conversation

yassinrais commented Mar 19, 2021

Uh oh!

yassinrais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zerkms commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yassinrais commented Mar 19, 2021

Uh oh!

lcobucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yassinrais commented Mar 19, 2021

Uh oh!

lcobucci commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcobucci commented Mar 19, 2021

Uh oh!

lcobucci Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

Slamdunk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yassinrais left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lcobucci commented Mar 19, 2021

Uh oh!

zerkms commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius commented Mar 19, 2021 via email

Uh oh!

lcobucci commented Mar 19, 2021

Uh oh!

lcobucci commented Mar 19, 2021

Uh oh!

lcobucci commented Mar 19, 2021

Uh oh!

yassinrais commented Mar 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zerkms commented Mar 19, 2021 •

edited

Loading

lcobucci commented Mar 19, 2021 •

edited

Loading

yassinrais left a comment •

edited

Loading

zerkms commented Mar 19, 2021 •

edited

Loading