Usage of json_encode() to convert time fractions to microseconds cause precision issues
#709
Comments
|
May you share with us:
? |
|
A stack trace and a sample token would also be quite helpful 👍 |
|
Yup I'm also getting Sample token: Downgrading from version 4.1.3 to 4.1.2 fixed the issue. I use laravel/passport to generate the tokens which uses oauth2-server as the server which generates the tokens using Not sure if this issue sits on the Laravel Passport implementation side (with oauth2-server package) or with this package, but again locking the version to 4.1.2 fixed it for me, and figured I'd pop a message on the issue for anyone else coming across this. After reverting the version, this is the token I have: The only difference is as per the release notes, the timestamps are floats instead of strings now. |
I'm sorry it seems to me the error you provided doesn't come from the token from the example. It is crucial for us to have the exact token that generates the issue: can you provide one of that kind please @RyanTheAllmighty ? |
|
When you are trying to send the $date value in convertDate() function in Parser.php file. It is checking for string but the value is 1616497508.79562, which is float. Then you are trying to convert with json_encode function which return the value in string like "1616497508.79562548484415745842262445584". Now the createFormFormat not able to convert it into date and through the mentioned error. |
|
The example you are providing doesn't match my tries: https://3v4l.org/ukgjM Can you give us the original JWT token? |
|
Maybe this is the cause/solution: https://3v4l.org/imDlQ The re-encoded value (using JSON) has a different precision (goes beyond microsecond precision by 1 decimal place). |
|
I think you got that. You are trying to cast the value as string. |
|
An alternative would be keeping the Can someone send a PR against v4.0.x? We should have the necessary tests in place to guard against issues. |
|
My question is: how did more than 6 decimal places end up into the token in the first place? |
I'm asking myself the same question... but we can only tell that based on sample tokens. |
|
Hence my question to @RyanTheAllmighty who seems issuing the token from |
|
https://3v4l.org/Xu3QH can be used to quickly detect the occurrence on a sample token btw. |
|
I'd suggest a bare |
|
|
|
if you are using laravel/passport , so the problem come from that library , because it used firebase/jwt but when you generate token with this library and parse it , the are no issue , just to know at the moment you have but to have more then 6 digits , i dont think it possible , all dates are formated by DateTimeImmutable , Just to know i tried 1millions of 6 digits with the new patch , so i think it will be from ~user code. i think you entred a long float number ~ / convertDate (return round float - find by @MarijnKoesen ) to the claims array |
$claims[$claim] = $this->convertDate(is_string($date) ? $date : json_encode($date, JSON_THROW_ON_ERROR));DateTimeImmutable::createFromFormat does not support more then 6 digits to format a date I believe that number_format will resolve this issue 🙏 |
|
I'm pretty sure that |
|
If anyone does not understand why its not working Read this: Conclusion: @lcobucci yeah , we need to know who generated the float with this long decimal sizes , @sachchida1993 can you show us that part of code if you can find it ! thx |
|
If im not wrong , DateTimeImmutable::createFromFormat format a string timestamp by cutting that string into 2 parts seconds + microseconds thats why the microseconds was never rounded , because DateTimeImmutable::createFromFormat does not round those numbers (cutted strings) ah , now its look very logic , so when DateTimeImmutable::createFromFormat recive a float, its convert it to string that make it round to 6 digits _ U.u valid format :at least '.' + 1 digit present with max [0-9]*6 digits |
|
Pretty sure this line is to blame |
That's what I assume as well. However, this gives me an infinite loop (terrible code, I know): <?php
do {
$date = new DateTimeImmutable();
$time = (float) $date->format('U.u');
$formatted = json_encode($time, JSON_THROW_ON_ERROR);
$decimalSeparatorPosition = strpos($formatted, '.');
if ($decimalSeparatorPosition === false) { continue; }
$microseconds = substr($formatted, $decimalSeparatorPosition + 1);
var_dump($microseconds, $formatted, number_format($time, 6, '.', ''));
echo PHP_EOL;
} while (strlen($microseconds) <= 6);
echo 'Issue found in:';
var_dump($date);I have time now, so I can patch and release stuff but I still would like to see a token that has the problem. |
|
i dont think this test will throw any issue |
I'm shooting in the dark without having a token that presents the issue. String manipulation of the encoded float is the only way I can go about reproducing the problem. I'm patching the thing nevertheless... |
lcobucci
changed the title
json_encode() to convert time fractions to microseconds cause precision issues
|
@lcobucci dont forgot the test i did before : https://3v4l.org/XKHV4 (all the 6 digits possible) Else to be more sur , use this more fast & full testing , then randomizing with repeated values $i=0;
do {
$i++;
$timestring = "1616536852." . str_pad("$i", 6, "0", STR_PAD_LEFT);
$time = (float) $timestring;
$formatted = json_encode($time, JSON_THROW_ON_ERROR);
$decimalSeparatorPosition = strpos($formatted, '.');
if ($decimalSeparatorPosition === false) {
continue;
}
$microseconds = substr($formatted, $decimalSeparatorPosition + 1);
} while (strlen($microseconds) <= 6);
echo 'Issue found in if its not i> 1 000 001:';
var_dump($timestring, $microseconds, $formatted, number_format($time, 10, '.', ''));
|
|
i will do another test with encoding and decoding jwt generated by this library , |
|
This is interesting, though, the following test passes with no error: /**
* @test
* @dataProvider timeFractionConversions
*/
public function floatsDoNotCauseParsingErrors(float $issuedAt, string $timeFraction): void
{
$encoder = new JoseEncoder();
$headers = $encoder->base64UrlEncode($encoder->jsonEncode(['typ' => 'JWT', 'alg' => 'none']));
$claims = $encoder->base64UrlEncode($encoder->jsonEncode(['iat' => $issuedAt]));
$config = Configuration::forUnsecuredSigner();
$parsedToken = $config->parser()->parse($headers . '.' . $claims . '.');
self::assertInstanceOf(Plain::class, $parsedToken);
self::assertSame($timeFraction, $parsedToken->claims()->get('iat')->format('U.u'));
}
public function timeFractionConversions(): iterable
{
yield [1616481863.528781890869140625, '1616481863.528782'];
yield [1616497608.0510409, '1616497608.051041'];
} |
|
Yeah I wasn't sure if this was caused by this library itself or one of the other packages I use for the oauth2 server handling. I figured since the downgrade fixed it, it could be here. I've dived into the packages I use and they seem to not be doing anything other than passing the JWT onto this package, but it's possible with all the abstractions in the packages that I missed a step. I don't have an example JWT anymore unfortunately. Looks like the token I gave wasn't one that was erroring. It seems as though this was happening when one of the timestamps went into 7 precision digits. All the stacktraces in my logs all mention failing to parse date formats all with 7 precision digits, and from memory this was happening on every single JWT token that I was getting back (again not sure whose end this is on, AFAIK the generation was on this packages side): These were the specific starts of the stacktraces I have in my logs: If it's not the case that the issue sits here, I'll investigate the downstream packages further for the cause. A reverting of the version solves the immediate issue for me so I'm not in a rush thankfully. |
|
Adding |
|
Example of generating and parsing tests <?php
use Lcobucci\JWT\Configuration;
use Lcobucci\JWT\Signer\Hmac\Sha256;
use Lcobucci\JWT\Signer\Key\InMemory;
use Lcobucci\JWT\UnencryptedToken;
require './vendor/autoload.php';
$config = Configuration::forSymmetricSigner(
// You may use any HMAC variations (256, 384, and 512)
new Sha256(),
// replace the value below with a key of your own!
InMemory::plainText('test')
// You may also override the JOSE encoder/decoder if needed by providing extra arguments here
);
assert($config instanceof Configuration);
function gtoken($config)
{
$now = new DateTimeImmutable();
return $config->builder()
// Configures the issuer (iss claim)
->issuedBy('http://example.com')
// Configures the audience (aud claim)
->permittedFor('http://example.org')
// Configures the id (jti claim)
->identifiedBy('4f1g23a12aa')
// Configures the time that the token was issue (iat claim)
->issuedAt($now)
// Configures the time that the token can be used (nbf claim)
->canOnlyBeUsedAfter($now->modify('+'.rand(0, 999999999).' msec'))
// Configures the expiration time of the token (exp claim)
->expiresAt($now->modify('+'.rand(0, 999999999).' msec'))
// Configures a new claim, called "uid"
->withClaim('uid', 1)
// Configures a new header, called "foo"
->withHeader('foo', 'bar')
// Builds a new token
->getToken($config->signer(), $config->signingKey())
->toString();
}
function pToken($config, $tkn)
{
$token = $config->parser()->parse($tkn);
assert($token instanceof UnencryptedToken);
return $token;
}
do {
// generate random token
$token = gtoken($config);
// parse
$parsed = pToken($config, $token);
$timestring = $parsed->claims()->get('iat')->format('U.u');
if (strpos($timestring, '.') === false) {
continue;
}
$timefloat = (float) $timestring;
$formatted = json_encode($timefloat, JSON_THROW_ON_ERROR);
$decimalSeparatorPosition = strpos($formatted, '.');
if ($decimalSeparatorPosition === false) {
continue;
}
$microseconds = substr($formatted, $decimalSeparatorPosition + 1);
} while (strlen($microseconds) <= 6 && $timestring === (str_pad(json_encode($timefloat), 17, '0')));
echo 'Issue found in if its not i> 1 000 001:';
var_dump($token, $timestring, $microseconds, $formatted, number_format($timefloat, 6, '.', ''));I ran it for a while and found no problems 🤔. |
|
the are no way that this library will generate more then 6 decimals 🤔
the only issue we have , the parser of 7 decimals |
I couldn't find either... but #710 should mitigate the problem. Can we get some reviews? |
|
Alright, releasing new patches now. I'd still love it if you could send a sample token that presents the issue @sachchida1993. |
message: "Value is not in the allowed date format: 1616481863.528781890869140625"
The text was updated successfully, but these errors were encountered: