icalendar sanitizer

[tobixen]

In the caldav library, I have a function fixing broken icalendar data. The idea is that the library users should be able to fetch data from one calendar and play with it without other components breaking (particularly the vobject library that was used earlier had a tendency to raise errors on broken icalendar data). Six different observed issues are fixed, at least three of them have been observed recenly.

This function is not specific to the CalDAV library. @SashankBhamidi indicated that it would be useful in the anonymizer project and suggested to implement it in the icalendar library.

The current function takes a string and returns a string. In the icalendar library it would probably be better with a method accepting a string and returning an object. Maybe simply throwing in a scrub=False parameter to the current from_ical-method. One could also throw in possibilities to configure what should be scrubbed, how, scrubber-plugins, etc, but personally I think I favor the "keep it simple stupid" approach.

The current test code uses the vobject library to validae the icalendar code. The vobject will raise errors on all the broken data found except the missing DTSTAMP. However, it would make more sense to create a validator method in the icalendar library. I think it's important to be able to validate both strings (which may be so broken that the from_ical-method will raise errors) and icalendar objects (which may be missing mandatory fields, etc).

So basically everything I have seems to need a complete rewrite before it can be moved to the icalendar library. Oh well, the current implementation is probably not catching all kthe corner cases anyway.

The logic is available here:

https://github.com/python-caldav/caldav/blob/master/caldav/lib/vcal.py

Test code here:

https://github.com/python-caldav/caldav/blob/master/tests/test_vcal.py

[SashankBhamidi]

This would help icalendar-anonymizer. We rely on Calendar.from_ical() and if it fails on broken data, we just crash.

We'd use both:

Scrubbing: Would let us handle broken input gracefully instead of crashing on invalid calendars.

Validation: Right now we can't tell users why their calendar is broken. We just pass through whatever exception from_ical() raises.

API:

I like scrub=False for from_ical(). For validation:

errors = icalendar.validate_string(ics_string)
errors = calendar.validate()

Return list of violations instead of exceptions.

Our workflow would be:

1. Validate input
2. Scrub if needed
3. Parse with from_ical(scrub=True) as fallback
4. Anonymize
5. Validate output

Happy to test once there's a draft.

[niccokunzmann]

I would always scrub it if we cannot parse it or something like that. The users/devs need the minimal choice to do it right - a design choice: hide technical complexity and make it easy to use.

[tobixen]

I'm reading your text over and over again, and realize that it may be interpreted in different ways, at least:

• The icalendar-anonymizer should always scrub data that cannot be parsed
• When we're proposing to add scrubbing to the from_ical, you agree and think the default should be to scrub - from_ical(scrub=True)
• Or perhaps you meant that it shouldn't be scrubbed except if the parsing fails. (and there may be cases where the parsing does not fail, but the icalendar data is still broken - like with the missing DTSTAMP case).

(One design pattern I've used some few places in the CalDAV library is tri-states - False, True and (default) None, and when None is given, the library will try to guess the best behaviour, like sanitizing data only if parsing breaks).

There are limits to what can and should be done - if the parsing fails because the thing doesn't even resemble ical, then a traceback is most likely appropriate.

[niccokunzmann]

Hm. Thinking again:

There are parsing errors - we should recover as much information as possible if we can safely ignore the error. The .ics examples from caldav can be added to icalendar and tests created to parse them successfully.

Then, there is stuff like missing DTSTAMP. we have Event.DTSTAMP and other such methods. One could put a useful default value in there.

Another way to go about it is to add fixing methods. We have e.g. Event.new() now. The idea here is to enable devs to create correct ics files. Then, there is also Calendar.add_missing_timezones() which is there to do a fix.

Maybe, this can be combined: subclasses of Component.fix() will try to remove errors in the components. Subclasses of Component.new() should set the same fields by default that Component.fix() would set.

[niccokunzmann]

This is a package with a slightly different scope, I want to throw in the mix before I get into discussion at some point:
https://icalendar-compatibility.readthedocs.io/en/latest/

[stevepiercy]

I wouldn't trust that guy further than I can throw him. 😉

[stevepiercy]

The current function takes a string and returns a string. In the icalendar library it would probably be better with a method accepting a string and returning an object. Maybe simply throwing in a scrub=False parameter to the current from_ical-method. One could also throw in possibilities to configure what should be scrubbed, how, scrubber-plugins, etc, but personally I think I favor the "keep it simple stupid" approach.

Can you add a definition in the proposal of what a scrub function would actually do, including a concrete example? I have an idea, but it might not align with what y'all have in mind.

[tobixen]

I don't know if "scrub" is the most appropriate word, but the idea is to take some potentially broken ical input, and modify it in any way needed to make valid ical output - adding fake data to it, if needed.

i've named it "fx" in the caldav code. I feel a bit uneasy about it, as the caldav library shouldn't really do anything except taking the exact thing we're getting from the server and push it out to the user - at the other hand, things are likely to blow up with invalid ical, so . So I decided to at least log the diff. However, HomeAssistant users with iCloud calendars went mad at me because I was spamming their logs. So I introduced rate limiting of the logging, but people were still not happy with that, so in the end I accepted a pull-request to download it from an error to a warning.

[stevepiercy]

Thanks for the clarification. I assume that it could also mean removing bad data.

sanitize is a more common term for what you describe. The phrase "scrub a rocket launch" means to cancel it. Also scrubbing usually removes stains, but doesn't add or fix them.

I imagine that there are myriad implementation details. I'd gather the most frequently broken examples now while they're still hot and smoking. It's hard to plan development without some data.

[tobixen]

I've updated the title.

Came to think, there is another decision to be made. My approach is to just consider six different types of deviations observed in the real world and sanitize those. The "better" approach is to try to sanitize any kind of crazy things coming.

The latter is a complex task, maybe it's not even possible to consider all the possible corner cases . This reply was originally intended to include some pseudo-code, I started writing it, but it was harder than I thought, so this reply was left behind forgotten among many other browser tabs :-)

[niccokunzmann]

icalendar is a good place to cope with broken or weird files. People report all sorts of errors, specially when it comes to parsing.