Update exp and nbf validation to use double parsing #89
Conversation
… for 2 reasons: Support dates beyond year 2038, and prevent treating nulls as 0.
|
#89 fix |
src/JWT/JwtValidator.cs
Outdated
| try | ||
| { | ||
| expInt = Convert.ToInt32(expObj); | ||
| expInt = double.Parse(expObj.ToString()); |
There was a problem hiding this comment.
Double.ToString() has current culture issues. Please use Convert.ToDouble() instead
src/JWT/JwtValidator.cs
Outdated
| try | ||
| { | ||
| nbfInt = Convert.ToInt32(nbfObj); | ||
| nbfInt = double.Parse(nbfObj.ToString()); |
tests/JWT.Tests.Core/DecodeTests.cs
Outdated
| var serializer = new JsonNetSerializer(); | ||
| JsonWebToken.JsonSerializer = serializer; | ||
|
|
||
| var post2038 = new DateTime(3000, 1, 1, 0, 0, 0, DateTimeKind.Utc); |
There was a problem hiding this comment.
Should we use 2038/1/1 + 1 second here? To justify variable's name
tests/JWT.Tests.Core/DecodeTests.cs
Outdated
| public void DecodeToObject_Should_Throw_Exception_Before_NotBefore_Becomes_Valid() | ||
| { | ||
| var nbf = (int)(DateTime.UtcNow.AddHours(1) - JwtValidator.UnixEpoch).TotalSeconds; | ||
| var post2038 = new DateTime(3000, 1, 1, 0, 0, 0, DateTimeKind.Utc); |
|
I've reverted back to using Convert and added a separate null check to ensure it doesn't get converted to 0. |
src/JWT/JwtValidator.cs
Outdated
| } | ||
| catch | ||
| { | ||
| throw new SignatureVerificationException("Claim 'exp' must be an integer."); | ||
| throw new SignatureVerificationException(string.Format(claimMustBeDoubleFormat, "exp")); |
There was a problem hiding this comment.
Let's don't extract strings into const variables to keep the source code smaller and more readable
| @@ -67,10 +67,20 @@ public void DecodeToObject_Should_Throw_Exception_On_Invalid_Expiration_Claim() | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void DecodeToObject_Should_Throw_Exception_On_Null_Expiration_Claim() | |||
There was a problem hiding this comment.
Sorry for confusing. Encode/Decode tests are for now-obsolete static API which we'll remove shortly. Can you please move new tests to JwtEncoder/Decoder tests?
|
Reverted the constant change. I knew it would be contentious so made it a separate commit ;) |
|
Looks good to me. Thanks for this work! Let's keep till EoD so @nbarbettini could give a quick look. And I'll merge. Thanks again :) |
|
Thanks for the Y2038 catch, @zemien! One question - why |
|
Hmm good question. I suppose most approaches for calculating epoch in .Net uses the TotalSeconds property which is a double. Logically "seconds since epoch" do not appear to represent fractional seconds, but then again is there a reason to stop someone from specifying 0.5 (Jan 1, 1970 at 00:00:00.500)? |
src/JWT/JwtValidator.cs
Outdated
| } | ||
| catch | ||
| { | ||
| throw new SignatureVerificationException("Claim 'exp' must be an integer."); | ||
| throw new SignatureVerificationException("Claim 'exp' must be a double."); |
There was a problem hiding this comment.
I think "Claim 'exp' must be a number." would remove some potential confusion from the error message.
src/JWT/JwtValidator.cs
Outdated
| int nbfInt; | ||
| if (nbfObj == null) | ||
| { | ||
| throw new SignatureVerificationException("Claim 'nbf' must be a double."); |
src/JWT/JwtValidator.cs
Outdated
| } | ||
| catch | ||
| { | ||
| throw new SignatureVerificationException("Claim 'nbf' must be an integer."); | ||
| throw new SignatureVerificationException("Claim 'nbf' must be a double."); |
src/JWT/JwtValidator.cs
Outdated
| throw new SignatureVerificationException("Claim 'exp' must be a double."); | ||
| } | ||
|
|
||
| double expInt; |
There was a problem hiding this comment.
Nitpick: This should be expValue or just exp.
No reason, I think using double is fine. I forgot that Code looks good! 👍 Sent you a few minor comments. |
…instead of double.
|
Done, please review |
Update exp and nbf validation to be based on strictly-parsed doubles for 2 reasons: Support dates beyond year 2038, and prevent treating nulls as 0.