bitfireAT / ical4android

Allows usage of iCalendar files with the Android calendar provider
GNU General Public License v3.0
18 stars 10 forks source link

Crash when using `SMH` suffix with durations #117

Closed ArnyminerZ closed 9 months ago

ArnyminerZ commented 9 months ago

We have FixInvalidDayOffsetPreprocessor in ical4android, which handles cases when PT is not properly formatted for triggers and durations, however, it doesn't contemplate the S suffix, as stated in the Java Documentation for "seconds", so for example:

BEGIN:VALARM
TRIGGER:-P5S
ACTION:DISPLAY
END:VALARM

crashes the app.

Internal DAVx5 reference: https://github.com/bitfireAT/davx5/issues/482 Ticket reference: https://bitfire-at.zammad.com/#ticket/zoom/3057

ArnyminerZ commented 9 months ago

The issue here is that we are specifying seconds (S suffix), and as stated in the documentation:

The ASCII letter "T" must occur before the first occurrence, if any, of an hour, minute or second section.

So that is clearly missing the T, so the correct duration would be -PT5S, which if we try to parse it, doesn't give any issues:

Duration.parse("-PT5S")

However, I've been taking a look at FixInvalidDayOffsetPreprocessor and it isn't exactly this case. So I don't know if we should extend the regex and functions, which would make them much more complicated, or just create a new StreamProcessor for cases like this.

This processor would have to detect cases where seconds, minutes or hours are specified without the T, and add it, so it would be a pretty simple one.

@sunkup what do you think? I believe that even though it's quite more code, it would be more readable and maintainable if we create a new StreamProcessor for this, since the cases handled by FixInvalidDayOffsetPreprocessor as stated in https://github.com/bitfireAT/ical4android/issues/77, even though they are related to the T prefix, it's not really the same case.

sunkup commented 9 months ago

How often does this occur? Meaning how many providers/servers do this wrong? I am asking because, even if it seems like a small fix, we can not possibly repair all existing wrongdoings of all providers.

I only found one ticket in zammad mentioning one problematic server: "calcurse/baikal" (calcurse.org). I might be wrong, but I believe not too many people are using this server and it seems they have already fixed the time formatting issue? At least I can't find the file which the user in the ticket mentions: src/local.c in their repository.

I think we should consider adding this in case it pops up again with a different provider or if more calcurse users report it.

ArnyminerZ commented 9 months ago

I've only seen that case, and I agree that we shouldn't cover all the formatting errors for all the server providers. So either we consider this case, or we can stale this until it happens again, or directly tell the user that the server must be updated or something

rfc2822 commented 9 months ago

Addressed on server side here: https://github.com/lfos/calcurse/issues/480

I wouldn't add another stream preprecessor for that, because it's a really rare problem and operating with regex on incoming iCalendars is really some "last resort" thing that I'd only do for problems that occur often and/or are very important.