fix: reject a non-numeric clockTolerance instead of silently ignoring it - #1045
Open
afonsojanu wants to merge 1 commit into
Open
fix: reject a non-numeric clockTolerance instead of silently ignoring it#1045afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
verify() already validates that clockTimestamp is a number, but never did the same check for clockTolerance. Passing a string there (something you can easily do by accident, since maxAge does accept strings) turns the internal exp/nbf comparisons into string concatenation, and Infinity or NaN break the same arithmetic in their own way. In all three cases the tolerance effectively becomes unbounded, so an already-expired token gets accepted as valid no matter how old it is. This mirrors the existing clockTimestamp check and throws a JsonWebTokenError up front instead of letting the comparison quietly do the wrong thing.
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
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.
Closes #606.
verify() checks that
clockTimestampis a number, but there was never an equivalent check forclockTolerance. If you pass a string there, which is easy to do by accident sincemaxAgeaccepts strings, the exp/nbf comparisons turn into string concatenation instead of arithmetic. Infinity and NaN break the same comparisons in their own way. All three cases end up making the tolerance effectively unbounded, so a token that's actually expired still verifies fine.Repro before this change:
The fix just validates
clockTolerancethe same wayclockTimestampalready is, and throws aJsonWebTokenErrorinstead of letting it silently do the wrong thing. Added tests covering the string, Infinity and NaN cases, plus one confirming a normal numeric tolerance still works as before.There was an earlier attempt at this back in #611, but it never got merged and had its own bug (the validation lived inside a
.forEachcallback, soreturning from it didn't actually stopverify(), it just kept going). This is a fresh, smaller fix that doesn't have that problem.