feat: add opt-in support for millisecond timestamps - #644
Conversation
Some non-compliant token issuers emit the iat/nbf/exp claims in milliseconds rather than the seconds mandated by RFC 7519, which causes decode() to reject the token with a date far in the future (see googleapis#539). Add a JWT::$useMillisecondTimestamps flag (default false). When enabled, the reference time defaults to milliseconds (microtime(true) * 1000) so claims are compared in the same unit, and the BeforeValidException date messages are rendered from the correct second value.
There was a problem hiding this comment.
Thanks for putting this together! The implementation looks solid, but there is a critical edge case regarding 32-bit systems and the ExpiredException class that should be addressed in this PR.
Because millisecond timestamps (e.g., 1710000000000) exceed the 32-bit integer limit (PHP_INT_MAX is 2147483647), any code casting these timestamps to (int) will trigger an integer overflow and cause validation to fail completely on 32-bit PHP systems.
This creates a cascading issue with ExpiredException.php, which currently enforces a strict int type hint.
To ensure this new feature works reliably across all environments without causing static analysis errors or TypeErrors, I highly recommend implementing the in-line suggestions and the following:
Update ExpiredException.php to accept floats:
If a user (or the library itself) passes a float to avoid 32-bit integer overflow, ExpiredException::setTimestamp(int $timestamp) will trigger a fatal TypeError in PHP 8.1+ on 32-bit systems. Since the library requires PHP ^8.0, you can safely use union types here:
// src/ExpiredException.php
public function setTimestamp(int|float $timestamp): void
{
$this->timestamp = $timestamp;
}
public function getTimestamp(): int|float|null
{
return $this->timestamp;
}Address review feedback on googleapis#644: millisecond timestamps exceed PHP_INT_MAX on 32-bit systems, so casting to (int) overflows and breaks validation. - Use round() instead of an (int) cast so the reference time stays a float in millisecond mode. - Widen ExpiredException::$timestamp and its accessors to int|float. - Widen JWT::$timestamp docblock to int|float|null. - Update tests to build millisecond timestamps with round().
|
Thanks for the careful review, @cy-yun — good catch on the 32-bit overflow. Pushed a fix:
|
Address review feedback on googleapis#644: millisecond timestamps exceed PHP_INT_MAX on 32-bit systems, so casting to (int) overflows and breaks validation. - Use round() instead of an (int) cast so the reference time stays a float in millisecond mode. - Widen ExpiredException::$timestamp and its accessors to the int|float union type. - Widen JWT::$timestamp docblock to int|float|null. - Update tests to build millisecond timestamps with round().
d25ed03 to
0f09955
Compare
saifulferoz
left a comment
There was a problem hiding this comment.
All review comments have been addressed. Could you please re-review the PR?
cy-yun
left a comment
There was a problem hiding this comment.
Great work! I'm approving this because the implementation is solid and solves the issue nicely. I've left a couple of minor, non-blocking suggestions for documentation and test coverage that you can apply if you'd like before this gets merged.
Non-blocking follow-ups from the googleapis#644 review: - Note in the JWT::$leeway docblock that it must be expressed in milliseconds when JWT::$useMillisecondTimestamps is enabled. - Add testValidTokenWithMillisecondTimestampsAndExplicitOverrides to verify explicit JWT::$timestamp and JWT::$leeway overrides interact correctly with millisecond mode.
Added the leeway-unit note to the JWT::$leeway docblock. Co-authored-by: Charlotte Y <38296042+cy-yun@users.noreply.github.com>
bshaffer
left a comment
There was a problem hiding this comment.
I don't know to what extent we want to support features for APIs which blatantly disregard the JWT spec. I would like to look into this and see what other libraries are doing. If other libraries are doing this in PHP, then maybe use those instead.
| * @var int | ||
| */ | ||
| public static $leeway = 0; | ||
| public static $leeway = 0; |
|
I am closing this as WONT FIX. We do not want to support a feature which messes with the validation of access tokens in a non-compliant way. This could have serious security implications and result in unexpected behavior. |
Some non-compliant token issuers emit the iat/nbf/exp claims in milliseconds rather than the seconds mandated by RFC 7519, which causes decode() to reject the token with a date far in the future (see #539).
Add a JWT::$useMillisecondTimestamps flag (default false). When enabled, the reference time defaults to milliseconds (microtime(true) * 1000) so claims are compared in the same unit, and the BeforeValidException date messages are rendered from the correct second value.