Report out-of-range NumericDate claims distinctly from non-numeric ones#774
Open
arpitjain099 wants to merge 1 commit into
Open
Report out-of-range NumericDate claims distinctly from non-numeric ones#774arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
exp, nbf and iat are NumericDate claims, which per RFC 7519 are JSON numbers and may be written in scientific notation. getInstantFromSeconds gated on canConvertToLong() and threw "contained a non-numeric date value" when it was false. For a numeric value that overflows a long (for example 1.733162101e+26 from issue auth0#706) that message is wrong: the value is numeric, just not representable as a NumericDate. Split the two failure modes so a genuinely non-numeric value keeps the existing message, while a numeric value that does not fit a long throws a message naming the value and the real reason. In-range scientific values such as 1.7e9 continue to decode to the correct instant. Fixes auth0#706 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #706
What is actually happening
While looking into this I found that in-range scientific notation already decodes correctly. A claim like
{"exp": 1.7e9}is parsed by Jackson as aDoubleNodewhosecanConvertToLong()is true, andgetInstantFromSecondsalready handles it and returnsInstant.ofEpochSecond(1_700_000_000).The value in the report,
1.733162101e+26, is different: it is about1.7e26seconds (a year around5.5e18), which overflowslongand is not representable as a NumericDate or anInstant. So it genuinely cannot be decoded to a date. The defect is thatPayloadDeserializer.getInstantFromSecondsreported it asThe claim 'exp' contained a non-numeric date value, which is misleading: the value is numeric, it is just out of range.Fix
Split the two failure modes in
getInstantFromSeconds:non-numeric date valuemessage.long: a new, accurate message,The claim '<name>' value (<value>) is out of the range representable as a NumericDate.In-range scientific-notation values continue to decode unchanged. Clamping an absurd overflow value to some wrong year would be worse than a clear rejection, so the out-of-range case is rejected with the precise error.
Tests
Added to
PayloadDeserializerTest:shouldGetInstantWhenParsingScientificNotationNode:1.7e9decodes toInstant.ofEpochSecond(1_700_000_000)(documents that in-range scientific notation works).shouldThrowOutOfRangeWhenNumericDateExceedsLong:1.733162101e+26raises the new out-of-range message.The existing non-numeric and normal-integer tests still pass. The directly affected suites (
PayloadDeserializerTest,JsonNodeClaimTest,PayloadImplTest,JWTVerifierTest,JWTDecoderTest) run green. I ran these on JDK 11 because the repo's Gradle wrapper (6.9.2) rejects newer JDKs; CI will exercise the full version matrix.Scope
This is narrower than the title might suggest: scientific notation is already supported for representable values, so this PR fixes the misleading error for out-of-range values and adds the missing test coverage rather than adding a new feature. A separate pre-existing edge (a plain integer that passes
canConvertToLong()but still overflowsInstant.ofEpochSecond) is orthogonal to this issue and not addressed here.