rianjs / ical.net

ical.NET - an open source iCal library for .NET
MIT License
776 stars 230 forks source link

invalid UTC-OFFSET from iCloud prevent parsing the feed #245

Open djoul6 opened 7 years ago

djoul6 commented 7 years ago

When an iCal feed contains UTC-Offset with 4 numbers, there is a parsing exception (feed received from iCloud calendars). Here is an example:

TZOFFSETFROM:+1730
TZOFFSETTO:+1730

As recently fixed (https://github.com/rianjs/ical.net/commit/76025b3afb9e3c366d76dc4e4a3656cd908fec04), the parsing is now done on 6 characters (hhmmss)

TimeSpan.TryParseExact(rawOffset, "hhmmss", CultureInfo.InvariantCulture, out ts)

But the seconds are optionnals, according to the rfc 5545:

time-numzone = ("+" / "-") time-hour time-minute [time-second]

rianjs commented 7 years ago

There's no place on Earth that has a 17 hour, 30 minute UTC offset.

https://en.wikipedia.org/wiki/List_of_UTC_time_offsets

djoul6 commented 7 years ago

I completely agree with you, but it's what we get from iCloud calendars ! (including multiple feeds) We can make a manual replace (before parsing), to "hard-fix" it like this:

TZOFFSETFROM:+001730
TZOFFSETTO:+001730

I've checked multiple iCloud feed, and this value (1730) seems to be always with the "BMT" Timezone, here is a sample:

END:VTIMEZONE
BEGIN:VTIMEZONE
TZID:Europe/Brussels
X-LIC-LOCATION:Europe/Brussels
BEGIN:STANDARD
DTSTART:18800101T000000
RDATE;VALUE=DATE-TIME:18800101T000000
TZNAME:BMT
TZOFFSETFROM:+1730
TZOFFSETTO:+1730
END:STANDARD

But some others invalid values also exists from iCloud feeds:

BEGIN:VTIMEZONE
TZID:Europe/Berlin
X-LIC-LOCATION:Europe/Berlin
BEGIN:STANDARD
DTSTART:18930401T000000
RDATE;VALUE=DATE-TIME:18930401T000000
TZNAME:CEST
TZOFFSETFROM:+5328
TZOFFSETTO:+0100
END:STANDARD

This was also discussed on this issue: https://github.com/rianjs/ical.net/issues/147

After investigations, this issue from iCloud feed is not new, and others projects also got problem to deal with it: https://github.com/collective/icalendar/issues/155

rianjs commented 7 years ago

If there's one thing that's certain, it's that Apple won't bother to fix their bug.

If there's another thing that's certain, it's that Apple is rubbish at writing cloud services.

bymem commented 7 years ago

Having the same problem. with the offset being

Offset must be less than 24 hours, was +5020

is there any news on how to fix it?

rianjs commented 7 years ago

+5020 means a UTC offset of 50 hours and 20 minutes. That's not a real offset.

The problem is that the data is bad, so you should fix the data; I don't intend to allow nonsense inputs like that, sorry.

bymem commented 7 years ago

But that is directly from a icloud calander feed, i don't have control over that.

djoul6 commented 7 years ago

Can't it be a pragmatic solution to ignore this kind of invalid data instead of throwing an exception ?

rianjs commented 7 years ago

Can't it be a pragmatic solution to ignore this kind of invalid data instead of throwing an exception ?

When exceptional situations occur, we should throw exceptions. Wildly bad offsets are exceptional. The right thing to do is to throw.

It's your job to catch and figure out what you want to do when that happens.

But that is directly from a icloud calander feed, i don't have control over that.

Sure you do. You can check for bad offsets in your code, and do something about them. A library can't possibly know what every application developer will want to do with an exceptional situation. That's why the library throws -- to put the ball back in the app developers court to let them figure out what the right course of action is.

rianjs commented 7 years ago

Well you two have an anonymous person who advocated on your behalf, and since he sits near me, he's harder for me to ignore. :P

By way of explanation: behind the scenes, ical.net uses a DateTimeOffset to parse offsets. The limit for that type is +/-14 hours, otherwise it throws, as you've seen. Expecting you to parse your data interchange format before you parse your data interchange format does seem a little unfair.

So I'll think about this a bit more...

aluxnimm commented 7 years ago

I guess this was the reason why the original DDay.iCal parsed it manually with some regex.

djoul6 commented 7 years ago

Even if the exception is catched, it will continue to fail the feed parsing, and then it's clearly not a possible solution to simply catch exception. Of course we can adapt your code to implement it in a "future-proof" way, but it will be overkill to fork the project, just for that.

Avoid throwing exception is not an option to do bad code, but it could be a starting point of reflexion to imagine a better solution, including the management of this kind of partially incorrect feed ! For example it could be a configuration defining the parsing as "strict" (as actually) or not, or to allow the library to override the "DateTimeOffset" parser, by exposing the parser factory or any other technical entry point, like that the library can continue to be used as-is, and the application able to override the built-in parsing mechanism.

minichma commented 6 years ago

The IANA tzdb says about Guam:

# Guam
# Zone  NAME        GMTOFF  RULES   FORMAT  [UNTIL]
Zone    Pacific/Guam    -14:21:00 - LMT 1844 Dec 31
             9:39:00 -  LMT 1901        # Agana
...

so before 1845 -14:21 seems to have been a valid offset in Guam (at least IANA says so).

libical's vzic utility makes TZOFFSETFROM:-1421 out of this data.

For Manila it says

# Zone  NAME        GMTOFF  RULES   FORMAT  [UNTIL]
Zone    Asia/Manila -15:56:00 - LMT 1844 Dec 31

...