Follow up to CBOR date handling fix

[vcsjones]

This pull request fixed an issue for CBOR encoded dates where the wrong calendar is used for reading and writing dates #92539

We should decide on three follow up tasks

1. This is a breaking change. Do we want to document it?
2. This is a bug when encoding to a standard. Do we want to backport it?
3. Do we need a compat switch?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-formats-cbor, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This pull request fixed an issue for CBOR encoded dates where the wrong calendar is used for reading and writing dates #92539

We should decide on two follow up tasks

1. This is a breaking change. Do we want to document it?
2. This is a bug when encoding to a standard. Do we want to backport it?
3. Do we need a compat switch?

Author:	vcsjones
Assignees:	-
Labels:	area-System.Formats.Cbor
Milestone:	-

[filipnavara]

I think a backport is in order, especially since .NET 8 is a LTS release and this library may start getting used more widely.

[eiriktsarpalis]

Seems like a breaking change document is in order, but I don't think the change meets the bar for servicing at this point.

[ghost]

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

[filipnavara]

Seems like a breaking change document is in order, but I don't think the change meets the bar for servicing at this point.

Some cultures default to non-Gregorian calendars, so this produces silently corrupted data on machines in those cultures. The fix is unlikely to break any scenario that already worked correctly, and I think it's very much low-risk. Also, side effect of the bug causes one of the CBOR tests fail with a stack overflow error in some endless loop.

[eiriktsarpalis]

I defer to the @dotnet/area-system-formats-cbor owners and @jeffhandley for making a call on this. In my view this is a bug that doesn't meet the bar for servicing.

[vcsjones]

My 2c:

It's a globalization bug that adversely affects the use of the CBOR date APIs for non-Gregorian calendars, and the work arounds can be intrusive.

In other calendars it is just going to write something clearly incorrect. Consider:

using System.Formats.Cbor;
using System.Globalization;

var culture = (CultureInfo)new CultureInfo("he-IL").Clone();
culture.DateTimeFormat.Calendar = new HebrewCalendar();
Thread.CurrentThread.CurrentCulture = culture;

CborWriter writer = new();
writer.WriteDateTimeOffset(DateTimeOffset.Now);
byte[] encoded = writer.Encode();
Console.WriteLine(Convert.ToHexString(encoded));

This will encode as C07829D7AAD7A9D7A422D7932DD790272DD79922D7905431303A31393A31332E3636343832392D30343A3030

Where the date is literally encoded as תשפ"ד-א'-י"אT10:20:02.494098-04:00.

Another interesting case is the Thai Buddhist calendar, where dates are in the "future" when interpreted as Gregorian dates, and is the case for the default Thai calendar:

using System.Formats.Cbor;
using System.Globalization;

Thread.CurrentThread.CurrentCulture = new CultureInfo("th-TH");

CborWriter writer = new();
writer.WriteDateTimeOffset(DateTimeOffset.Now);
byte[] encoded = writer.Encode();

Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;
CborReader reader = new(encoded);
Console.WriteLine(reader.ReadDateTimeOffset());

This will parse as 09/26/2566 10:33:35 -04:00.

I would be in favor of servicing this. At best it's buggy that gives non-Gregorian cultures friction when trying to interoperate with a standard. At worst it's subtle security bug (consider some kind of CBOR token where an exp is centuries in the future by accident).

[iamcarbon]

Having recently contributed code to move the Fido2.NET library (a .NET foundation project with 742.6K downloads) to System.Formats.Cbor, learning that the WriteDateTimeOffset method is culture sensitive causes me concern. Given the possible security implications of writing a non-Georgian date, I would also like support the case for back-porting.

[danmoseley]

As well as the correctness and security arguments there is also the reasoning that given we are going to make a breaking change, it is better to do it in .NET 8 so that the population that begin using it in the 3 years of .NET 8 support are not broken.

[danmoseley]

Setting this to 8 since if I understand correctly, this issue concerns 8. Feel free to change back.

[jeffhandley]

I also think porting this into .NET 8 is valuable. The fix meets the servicing bar (data corruption), so it would be better to get this into .NET 8 GA than to introduce it through servicing.

In the breaking change documentation, we need to incorporate recommendations for how to handle data that was persisted incorrectly.

[eiriktsarpalis]

we need to incorporate recommendations for how to handle data that was persisted incorrectly.

This suggests to me that we might want to relax the ReadDateTimeOffset method so that previously persisted values are tolerated. In other words, replacing DateTimeOffset.TryParseExact with DateTimeOffset.TryParse. @vcsjones thoughts?

[filipnavara]

In other words, replacing DateTimeOffset.TryParseExact with DateTimeOffset.TryParse.

That won't help. The information about the calendar is simply not there, so you observe a value with a time shift (usually a few centuries of time shift) but otherwise formatted correctly.

In cases where you know which incorrect calendar was used, you can convert the date back (unless it goes out of representable bounds). You can likely also make a pretty broad validation of the date ranges to identify some of the erroneous values.

[eiriktsarpalis]

I see, in which case we declare this a breaking change and advice users to manually call ReadTag()/ReadTextString() methods and parse the date as they see fit.

Would we want to introduce an AppContext switch that restores the original behavior or is it too brittle to preserve even as opt-in behavior? My impression is that it isn't necessary given that it's an out-of-band package that users can upgrade to independently of the sdk.

[filipnavara]

I see, in which case we declare this a breaking change and advice users to manually call ReadTag()/ReadTextString() methods and parse the date as they see fit.

Yep, that seems like a reasonable advice if presented with all the relevant context.

Would we want to introduce an AppContext switch that restores the original behavior or is it too brittle to preserve even as opt-in behavior?

I would say "no". If there's way to read the broken data with existing API surface I don't think we should encourage reading or producing the malformed data in any other way.

[eiriktsarpalis]

Breaking change issue filed: dotnet/docs#37378