Skip to content

Follow up to CBOR date handling fix #92595

New issue
New issue
Closed
Closed
Follow up to CBOR date handling fix#92595
Assignees
eiriktsarpalis
Labels
area-System.Formats.Cborbreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.needs-breaking-change-doc-createdBreaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Milestone
8.0.0

Description

@vcsjones
vcsjones
opened on Sep 25, 2023

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?

Activity

  1. vcsjones
    added
    area-System.Formats.Cbor
    on Sep 25, 2023
  2. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Sep 25, 2023
  4. vcsjones
    added this to the 9.0.0 milestone on Sep 25, 2023
  5. ghost removed
    untriagedNew issue has not been triaged by the area owner
    on Sep 25, 2023

  6. filipnavara commented on Sep 25, 2023

    @filipnavara
    filipnavara
    on Sep 25, 2023
    Member

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

  7. eiriktsarpalis commented on Sep 26, 2023

    @eiriktsarpalis
    eiriktsarpalis
    on Sep 26, 2023
    Member

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

  8. eiriktsarpalis
    added
    breaking-changeIssue or PR that represents a breaking API or functional change over a previous release.
    on Sep 26, 2023
  9. ghost added
    needs-breaking-change-doc-createdBreaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
    on Sep 26, 2023

  11. filipnavara commented on Sep 26, 2023

    @filipnavara
    filipnavara
    on Sep 26, 2023
    Member

    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.

  12. eiriktsarpalis commented on Sep 26, 2023

    @eiriktsarpalis
    eiriktsarpalis
    on Sep 26, 2023
    Member

    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.

  13. vcsjones commented on Sep 26, 2023

    @vcsjones
    vcsjones
    on Sep 26, 2023
    MemberAuthor

    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).

  14. iamcarbon commented on Sep 26, 2023

    @iamcarbon
    iamcarbon
    on Sep 26, 2023

    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.

  15. danmoseley commented on Sep 27, 2023

    @danmoseley
    danmoseley
    on Sep 27, 2023
    Contributor

    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.

  16. danmoseley
    modified the milestones: 9.0.0, 8.0.0 on Sep 27, 2023

  17. danmoseley commented on Sep 27, 2023

    @danmoseley
    danmoseley
    on Sep 27, 2023
    Contributor

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

  18. eiriktsarpalis
    self-assigned this
    on Sep 28, 2023

  19. jeffhandley commented on Oct 2, 2023

    @jeffhandley
    jeffhandley
    on Oct 2, 2023
    Member

    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.

  20. eiriktsarpalis commented on Oct 3, 2023

    @eiriktsarpalis
    eiriktsarpalis
    on Oct 3, 2023
    Member

    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?

  21. filipnavara commented on Oct 3, 2023

    @filipnavara
    filipnavara
    on Oct 3, 2023
    Member

    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.

  22. eiriktsarpalis commented on Oct 3, 2023

    @eiriktsarpalis
    eiriktsarpalis
    on Oct 3, 2023
    Member

    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.

  23. filipnavara commented on Oct 3, 2023

    @filipnavara
    filipnavara
    on Oct 3, 2023
    Member

    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.

  24. eiriktsarpalis commented on Oct 4, 2023

    @eiriktsarpalis
    eiriktsarpalis
    on Oct 4, 2023
    Member

    Breaking change issue filed: dotnet/docs#37378

  25. eiriktsarpalis
    closed this as completedon Oct 4, 2023
  26. ghost locked as resolved and limited conversation to collaborators on Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-System.Formats.Cborbreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.needs-breaking-change-doc-createdBreaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions