Skip to content

fix: validate numeric type of iat, nbf and exp claims in encode#634

Merged
bshaffer merged 3 commits into
googleapis:mainfrom
guillaumedelre:fix/pr-453-numeric-claims
May 11, 2026
Merged

fix: validate numeric type of iat, nbf and exp claims in encode#634
bshaffer merged 3 commits into
googleapis:mainfrom
guillaumedelre:fix/pr-453-numeric-claims

Conversation

@guillaumedelre
Copy link
Copy Markdown

@guillaumedelre guillaumedelre commented May 10, 2026

The decode() path already rejected non-numeric iat/nbf/exp values, but encode() accepted any type and silently produced an invalid token. Add the same is_numeric guards in encode() and cover them with unit tests.

Closes #453

The decode() path already rejected non-numeric iat/nbf/exp values, but
encode() accepted any type and silently produced an invalid token. Add
the same is_numeric guards in encode() and cover them with unit tests.

Signed-off-by: Guillaume Delré <delre.guillaume@gmail.com>
@guillaumedelre
Copy link
Copy Markdown
Author

Hey @bshaffer, this PR fixes #453 — happy to adjust if anything looks off.

@bshaffer bshaffer added next major version do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 11, 2026
@bshaffer
Copy link
Copy Markdown
Collaborator

@guillaumedelre thank you for your contribution! Please see my comments here: #453 (review)

Covers the case where iat/nbf/exp are passed as numeric strings
(e.g. (string) time()), which must remain accepted to avoid a
breaking change.

Signed-off-by: Guillaume Delré <delre.guillaume@gmail.com>
@guillaumedelre
Copy link
Copy Markdown
Author

Hi @bshaffer, thanks for the feedback on #453!

I'm aware of the breaking change concern with is_int — that's exactly why this PR takes a different approach. It uses is_numeric() instead, which accepts both integers and numeric strings, so existing code passing (string) time() continues to work without exceptions.

I've also added an explicit regression test covering that exact scenario:

$payload = [
    'message' => 'abc',
    'iat' => (string) time(),
    'exp' => (string) (time() + 20),
    'nbf' => (string) (time() - 20),
];

$encoded = JWT::encode($payload, $key, 'HS256');
$decoded = JWT::decode($encoded, $keyObj); // no exception thrown

So this should be mergeable without a major version bump. Happy to adjust if you'd like a different approach.

@bshaffer
Copy link
Copy Markdown
Collaborator

@guillaumedelre I see, I was a bit confused by your PR description and referencing the other PR as "fixed".

This looks good to me then!

@bshaffer bshaffer removed next major version do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 11, 2026
@guillaumedelre
Copy link
Copy Markdown
Author

@bshaffer Sorry, I think I might have gotten a bit mixed up.

@bshaffer bshaffer merged commit 958e422 into googleapis:main May 11, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants