Reject seconds values that would cause overflow

[jonalbertini]

If seconds used with long max value, there will be an overflow because you put the value (long) multiplied by 1000 in another long...
in my case i got a negative value as result and the Math.max then return 0....

as I was expecting to have the max ClockSkew, I got the Min....

jjwt/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java

Line 134 in dacdb2c

this.allowedClockSkewMillis = Math.max(0, seconds * MILLISECONDS_PER_SECOND);

[lhazlewood]

I don't quite understand the problem. You shouldn't have overflow with any relevant value, correct? Clock skew should ideally be no more than 5 to 10 minutes at most (e.g. 300-600 seconds). Multiplying 300 * 1000 shouldn't ever cause overflow.

I'm curious - how did this happen? What was your input value?

[jonalbertini]

Hello, the problem is not in what the clock skew should be. you can't predict the "use cases". you deliver a function, doing something , with an Input and an effect. In my case , the framework I use, in TEST cases use the setAllowedClockSkew to allow expired token. It seems that's the only way to allow it. so we put the max long Value in input of your function. but the behaviour is not the one we want... you expose a function with a long in input, and multiply this input by 1000 . this value is then stored in a long. even il we do abstraction of what the function does, it's an issue. if you assume that there's a max value allowed you have to specify it, document it, and code some verifications. if you do so and really limit the clock skew to the "ideally" value. you may expose a setting to avoid the clock skew control for tests. my input value il the long max positive value. best regards, you already di an awesome job with this lib. we use it a lot in production. thank you . Le ven. 27 mars 2020 à 21:57, Les Hazlewood <notifications@github.com> a écrit :

…

I don't quite understand the problem. You shouldn't have overflow with any relevant value, correct? Clock skew should ideally be no more than 5 to 10 minutes at most (e.g. 360 seconds). Multiplying 360 * 1000 shouldn't ever cause overflow. I'm curious - how did this happen? What was your input value? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#583 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACFGRG5SNS47CIRKYKX33W3RJUHJXANCNFSM4LULHDUA> .

[lhazlewood]

Test cases written without any insight into why an API exists, or to use the API in an unintended manner, will cause problems like this. In this case, these test cases are effectively using a feature of the library to avoid the designed purpose of the method - to account for clock skew during parsing. This method does not exist to allow parsing expired tokens, so the test is using it incorrectly.

If anything, a nicer solution is probably to reject any value greater than (Long.MAX_VALUE / 1000) to guarantee that overflow is not possible.

Until #308 is incorporated after 1.0 is released - where we can use a java.time.Duration instance instead of just milliseconds - enforcing an upper bound is probably the cleanest solution because it doesn't introduce another temporary method that will be immediately thrown out when we can use a Duration instead.

Thank you for the feedback and the issue!