Fix DateTime claim value type recognition for ISO8601 strings with and without trailing zeros #3373
Conversation
…d without trailing zeros
|
@microsoft-github-policy-service agree company="WiseTech Global" |
1 similar comment
|
@microsoft-github-policy-service agree company="WiseTech Global" |
There was a problem hiding this comment.
Pull request overview
This PR fixes DateTime claim value type recognition for ISO8601 strings by removing the exact string match requirement, allowing all parseable DateTime strings to be classified as DateTime claims rather than just those with specific trailing zero patterns.
Key Changes:
- Modified
GetStringClaimValueTypeto useDateTime.TryParsewithDateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversalflags instead of requiring exact string matching - Removed the logic that converted parsed DateTime back to string for comparison
- Added tests for ISO8601 strings with and without trailing zeros
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs |
Simplified DateTime detection logic to accept any parseable DateTime string instead of requiring exact format match |
test/Microsoft.IdentityModel.JsonWebTokens.Tests/JwtTokenUtilitiesTests.cs |
Added test verifying trailing zeros are handled correctly in ISO8601 strings |
test/System.IdentityModel.Tokens.Jwt.Tests/JwtPayloadTest.cs |
Added test verifying JwtPayload handles trailing zeros correctly in ISO8601 strings |
| return ClaimValueTypes.String; | ||
|
|
||
| if (DateTime.TryParse(str, out DateTime dateTimeValue)) | ||
| if (DateTime.TryParse(str, null, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal, out _)) |
There was a problem hiding this comment.
The change to use DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal is broader than just handling trailing zeros. This will now classify ANY parseable DateTime string as a DateTime claim, including:
- Date-only strings like "2019-11-15" (previously String, now DateTime)
- Ambiguous formats like "01/02/2023"
- Relative date strings that DateTime.TryParse might accept
This is a significant behavioral change beyond the trailing zeros issue described in the PR. Consider:
- Adding comprehensive tests for various date formats to document the new behavior
- Verifying this doesn't break existing applications that expect certain date strings to remain as String type
- Evaluating if
DateTimeStyles.RoundtripKind(used in JsonSerializerPrimitives.cs line 151) would be more appropriate to maintain consistency and only accept ISO8601 roundtrip formats
| if (DateTime.TryParse(str, null, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal, out _)) | |
| if (DateTime.TryParseExact(str, "O", CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out _)) |
| [Fact] | ||
| public void GetStringClaimValueType_ReturnsDateTime_ForIso8601WithOrWithoutTrailingZeros() | ||
| { | ||
| var iso8601WithTrailingZeros = "2025-11-26T09:30:45.1234560Z"; | ||
| var iso8601WithoutTrailingZeros = "2025-11-26T09:30:45.123456Z"; | ||
| var resultWithTrailingZeros = JwtTokenUtilities.GetStringClaimValueType(iso8601WithTrailingZeros); | ||
| var resultWithoutTrailingZeros = JwtTokenUtilities.GetStringClaimValueType(iso8601WithoutTrailingZeros); | ||
| Assert.Equal(ClaimValueTypes.DateTime, resultWithTrailingZeros); | ||
| Assert.Equal(ClaimValueTypes.DateTime, resultWithoutTrailingZeros); | ||
| } |
There was a problem hiding this comment.
Consider adding test cases for edge cases beyond trailing zeros, such as:
- Date-only strings: "2019-11-15"
- Various ISO8601 formats: "2019-11-15T14:31:21Z", "2019-11-15T14:31:21+00:00"
- Non-ISO8601 formats that DateTime.TryParse accepts: "11/15/2019", "Nov 15, 2019"
- Strings that should remain as String type
This would document the new behavior comprehensively and ensure the change works as intended across different scenarios.
| [Fact] | ||
| public void TestReturnsDateTime_ForIso8601WithOrWithoutTrailingZeros() | ||
| { | ||
| var iso8601WithTrailingZeros = "2025-11-26T09:30:45.1234560Z"; | ||
| var iso8601WithoutTrailingZeros = "2025-11-26T09:30:45.123456Z"; | ||
| var jwtpayload = new JwtPayload | ||
| { | ||
| { "dateWithTrailingZeros", iso8601WithTrailingZeros }, | ||
| { "dateWithoutTrailingZeros", iso8601WithoutTrailingZeros } | ||
| }; | ||
|
|
||
| var claimWithTrailingZeros = jwtpayload.Claims.First(c => c.Type == "dateWithTrailingZeros"); | ||
| var claimWithoutTrailingZeros = jwtpayload.Claims.First(c => c.Type == "dateWithoutTrailingZeros"); | ||
| Assert.Equal(ClaimValueTypes.DateTime, claimWithTrailingZeros.ValueType); | ||
| Assert.Equal(ClaimValueTypes.DateTime, claimWithoutTrailingZeros.ValueType); | ||
| } |
There was a problem hiding this comment.
Consider adding test cases for edge cases beyond trailing zeros, such as:
- Date-only strings: "2019-11-15"
- Various ISO8601 formats with different timezone indicators
- Non-ISO8601 formats that DateTime.TryParse accepts
- Strings that should remain as String type
This would document the new behavior comprehensively and ensure the change works as intended across different scenarios.
1 participant
Background
Currently, the method
GetStringClaimValueTypereturnsClaimValueTypes.DateTimeonly if the input string matches exactly the output ofDateTime.ToUniversalTime().ToString("o", ...).This leads to inconsistent behavior for ISO8601 UTC strings with or without trailing zeros, e.g.:
Fix
With this change, all strings that can be successfully parsed as a valid DateTime will return
ClaimValueTypes.DateTime, regardless of trailing zeroes.Impact
This fix ensures that semantically equivalent ISO8601 UTC time strings are consistently recognized as DateTime claims.
Tests