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

Fix usage of non JSON numeric values for time fractions (without having precision issues) #706

Merged
merged 2 commits into from Mar 19, 2021

Conversation

yassinrais
Copy link
Contributor

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

Copy link
Contributor Author

@yassinrais yassinrais left a comment

Choose a reason for hiding this comment

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

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

RFC7519 Page 6 : NumericDate

@zerkms
Copy link

zerkms commented Mar 19, 2021

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:

  1. Bring a custom type that allows it (no built in php types do)
  2. Implement json de-/serialisation algorithm that would employ that type (json_decode/json_encode cannot do that)
  3. 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
Copy link
Contributor Author

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"

Copy link
Owner

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

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.

src/Encoding/MicrosecondBasedDateConversion.php Outdated Show resolved Hide resolved
src/Token/Parser.php Outdated Show resolved Hide resolved
@yassinrais
Copy link
Contributor Author

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 lcobucci force-pushed the 4.0.x branch 2 times, most recently from baed0bf to ba4aa2d Compare March 19, 2021 14:22
@lcobucci
Copy link
Owner

lcobucci commented Mar 19, 2021

@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 lcobucci self-assigned this Mar 19, 2021
@lcobucci lcobucci added the Bug label Mar 19, 2021
@lcobucci 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 lcobucci modified the milestones: 4.1.3, 4.0.2 Mar 19, 2021
@lcobucci
Copy link
Owner

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

$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));
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@Slamdunk Slamdunk left a comment

Choose a reason for hiding this comment

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

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

test/unit/Encoding/MicrosecondBasedDateConversionTest.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@yassinrais yassinrais left a comment

Choose a reason for hiding this comment

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

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. 👏🏻

src/Encoding/MicrosecondBasedDateConversion.php Outdated Show resolved Hide resolved
@lcobucci
Copy link
Owner

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

@zerkms
Copy link

zerkms commented Mar 19, 2021

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
Copy link
Sponsor Collaborator

Ocramius commented Mar 19, 2021 via email

lcobucci and others added 2 commits March 19, 2021 21:00
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
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Owner

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

@yassinrais
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

None yet

5 participants