Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ics import failure when VEVENT includes X-APPLE-STRUCTURED-LOCATION #3905

Closed
flexibeast opened this issue Jan 22, 2022 · 16 comments
Closed

ics import failure when VEVENT includes X-APPLE-STRUCTURED-LOCATION #3905

flexibeast opened this issue Jan 22, 2022 · 16 comments
Labels
0. to triage Pending approval or rejection bug

Comments

@flexibeast
Copy link

Calendar 3.0.5 on NC 22.2.3 failed to import an .ics file, sent from a recent iPhone, with the UI errors:

No valid files found, aborting import

and

file.ics could not be parsed

with the nextcloud.log error:

"Sabre\\VObject\\ParseException","Message":"This parser only supports VCARD and VCALENDAR files"

Bisecting the file revealed that the presence of the X-APPLE-STRUCTURED-LOCATION property within the VEVENT component was the cause; without this property, the file was imported successfully.

@flexibeast flexibeast added 0. to triage Pending approval or rejection bug labels Jan 22, 2022
@max65482
Copy link
Contributor

max65482 commented Feb 3, 2022

I just successfully imported an .ics file that includes one X-APPLE-STRUCTURED-LOCATION property:

X-APPLE-STRUCTURED-LOCATION;VALUE=URI;X-ADDRESS="Berlin, Deutschland";X-AP
 PLE-MAPKIT-HANDLE=CAESkAEIrk0Q0ozqrpusuYyyARoSCZ3dtd81Q0pAESBumLIdvSpAIikK
 C0RldXRzY2hsYW5kEgJERRoGQmVybGluKgZCZXJsaW4yBkJlcmxpbioTQmVybGluIEhhdXB0Ym
 FobmhvZjIGQmVybGluMgtEZXV0c2NobGFuZDgvUAFaEwoRCNKM6q6brLmMsgEYrk2QAwE=;X-A
 PPLE-RADIUS=301.0391264594954;X-APPLE-REFERENCEFRAME=1;X-TITLE=Berlin Haup
 tbahnhof:13.369367

There are some reports about colon/semicolon mixups and missing escape characters with apple devices:
collective/icalendar#116
https://discussions.apple.com/thread/250761617

Could you share a sample property that causes the error?

@max65482
Copy link
Contributor

max65482 commented Feb 3, 2022

What Apple is doing is "interesting", anyway. Properties that start with X- are for "experimental use" according to RFC5545. Logically, X-APPLE-STRUCTURED-LOCATION supplements the regular LOCATION property.

Think of the following situation (similar to #3863): You create an event with an Apple device (includes X-APPLE-STRUCTURED-LOCATION and LOCATION) . Then you modify the location with a non-Apple device. Only LOCATION is updated. The event properties are inconsistent.

Technically, it would be possible to delete X-APPLE-STRUCTURED-LOCATION upon modification of LOCATION to avoid inconsistencies. However, starting to meddle with experimental properties might open Pandora's box. And it's hard to say if all Apple devices will deal with this loss gracefully. @benbucksch, can you comment?

@tcitworld
Copy link
Member

Hopefully Apple may use VLOCATION components with STRUCTURED-DATA properties from RFC 9073 instead in the future.

@benbucksch
Copy link

benbucksch commented Feb 3, 2022

Apple should have used parameters on the location property, instead of using a completely separate property, to avoid exactly that problem of being out of sync when the LOCATION is changed by a non-Apple calendar client. Every other calendar client which changes LOCATION will trigger the same bug, so arguably, Apple needs to fix that on their end. You should file a bug against Apple, but good luck with getting it fixed.

I would special-case this and delete X-APPLE-STRUCTURED-LOCATION when LOCATION is changed. Apple clients should be able to deal with having only LOCATION, just like they should not break on new events that other clients created and which have only the LOCATION property and no X-APPLE-STRUCTURED-LOCATION.

@benbucksch
Copy link

benbucksch commented Feb 3, 2022

However, this bug is not about inconsistencies between LOCATION and X-APPLE-STRUCTURED-LOCATION, but rather parse failures.

I would recommend to look at how NextCloud parses property parameters, and compare that with the spec RFC 5545 Section 3.2. Note the space in the parameter value (which are valid) and the , comma within the " quotes. It's possible that the NextCloud parser is not understanding how to parse the " quotes and tripps over the , comma or a : in the parameter value in " quotes, or over the space in the parameter value without " quotes.

@max65482
Copy link
Contributor

max65482 commented Feb 3, 2022

I opened #3930 for the inconsistency issue. It deletes the X-APPLE-STRUCTURED-LOCATION upon modification. We can continue discussions there.

As for the parsing error, a sample for the reproduction is needed. The snippet above is a valid case.

@flexibeast
Copy link
Author

flexibeast commented Feb 3, 2022

i don't have or use Apple devices myself, but here's a trimmed-down and lightly-redacted version of an .ics recently sent to me from the same Apple device as in my original report:

BEGIN:VCALENDAR
PRODID:-//caldav.icloud.com//CALDAVJ 2206B560//EN
METHOD:REQUEST
VERSION:2.0
BEGIN:VEVENT
DTEND;TZID=Australia/Melbourne:20220203T163000
DTSTART;TZID=Australia/Melbourne:20220203T120000
LOCATION:71 Whitehorse Rd\nDeepdene 3103\nAustralia
X-APPLE-STRUCTURED-LOCATION;VALUE=URI;X-ADDRESS=71 Whitehorse Rd\\nDeepde
 ne 3103\\nAustralia;X-APPLE-ABUID="xxxxxxxxxxxxxxxxxxxxxxx"::;X-APPLE-MA
 PKIT-HANDLE=CAES9gEI2TIaEgmLMOr/wedCwBGPbRlwFiJiQCJ6CglBdXN0cmFsaWESAkFV
 GghWaWN0b3JpYSIDVklDKgpCb3Jvb25kYXJhMghEZWVwZGVuZToEMzEwM0ILQ290aGFtIFdh
 cmRSDVdoaXRlaG9yc2UgUmRaAjcxYhA3MSBXaGl0ZWhvcnNlIFJkigELQ290aGFtIFdhcmQq
 EDcxIFdoaXRlaG9yc2UgUmQyEDcxIFdoaXRlaG9yc2UgUmQyEURlZXBkZW5lIFZJQyAzMTAz
 MglBdXN0cmFsaWE4OUAAUAFaGQoXEhIJizDq/8HnQsARj20ZcBYiYkAY2TI=;X-APPLE-RAD
 IUS=24.73208612096114;X-APPLE-REFERENCEFRAME=1;X-TITLE=71 Whitehorse Rd
Deepdene 3103
Australia:geo:-37.810608,145.065239
DTSTAMP:20220202T043057Z
END:VEVENT
END:VCALENDAR

If i remove the X-APPLE-STRUCTURED-LOCATION block from the preceding, there's no parse error. However, there's still a parse error when the X-APPLE-STRUCTURED-LOCATION block is itself trimmed like so:

BEGIN:VCALENDAR
PRODID:-//caldav.icloud.com//CALDAVJ 2206B560//EN
METHOD:REQUEST
VERSION:2.0
BEGIN:VEVENT
DTEND;TZID=Australia/Melbourne:20220203T163000
DTSTART;TZID=Australia/Melbourne:20220203T120000
LOCATION:71 Whitehorse Rd\nDeepdene 3103\nAustralia
X-APPLE-STRUCTURED-LOCATION;VALUE=URI;X-ADDRESS=71 Whitehorse Rd\\nDeepde
 ne 3103\\nAustralia
DTSTAMP:20220202T043057Z
END:VEVENT
END:VCALENDAR

@max65482
Copy link
Contributor

max65482 commented Feb 4, 2022

Thanks for the sample! I could reproduce the error.

Your second sample doesn't work because it doesn't specify a value (no :value). That seems plausible to me.

Your first sample fails because Deepdene 3103 and Australia:geo:-37.810608,145.065239 are in separate lines without leading space. Thus, they are taken as separate properties although they are part of X-APPLE-STRUCTURED-LOCATION. I would expect the address lines to be separated by \n. Not an actual line break.

This is odd. Can you confirm that you see this in the original as well (not a copy/redaction effect)?

@flexibeast
Copy link
Author

Yes, this is what the original looks like:

 MglBdXN0cmFsaWE4OUAAUAFaGQoXEhIJizDq/8HnQsARj20ZcBYiYkAY2TI=;X-APPLE-RAD
 IUS=24.73208612096114;X-APPLE-REFERENCEFRAME=1;X-TITLE=71 Whitehorse Rd
Deepdene 3103
Australia:geo:-37.810608,145.065239
X-APPLE-TRAVEL-START;ROUTING=CAR;VALUE=URI;X-ADDRESS="

@max65482
Copy link
Contributor

max65482 commented Feb 4, 2022

Ok, then it seems that the creator application failed to comply with RFC 5545. It does not allow newlines (CRLF) in text values. Newlines must be encoded as \N or \n. However, CRLF may be used to break long lines. But then it must be followed by a white-space character.

All in all, it seems like a major encoding error in the application that created the calendar/event. I couldn't find any misbehavior in Nextcloud.

@flexibeast
Copy link
Author

*nod* The creating application is the stock calendaring app on the iPhone. i don't feel NC has any obligation to deal with data that doesn't follow the relevant standards, so i'll just manually munge the data now that you've explained the source of the issue - thanks! i'm happy for this issue to be closed, if appropriate.

@max65482
Copy link
Contributor

max65482 commented Feb 5, 2022

Yes, removing the city and country information (including the newlines) from X-TITLE is a workaround. That's how my iPhone acts in the first place.

You can find multiple reports of decoding issues with Apple's X-APPLE-STRUCTURED-LOCATION. When taking a closer look, you find that these issues are similar among each other, but not exactly the same. That's why I guess it's unrealistic to develop a parser that cleanly supports all of Apple's non-compliance flavors.

@tcitworld
Copy link
Member

The corresponding issue in vObject sabre-io/vobject#402

@tcitworld
Copy link
Member

I have failed to reproduce the issue on both latest iOS and iCloud, so I guess we can close this.

Otherwise, adding OPTION_IGNORE_INVALID_LINES to the parser's options also fixes this, but whether we should use it is debatable.

@neufeind
Copy link

neufeind commented Nov 8, 2023

I have a private icloud-calendar from a user that triggers this behaviour. With the workaround from
sabre-io/vobject#402
to cleanup the feed-body first (stripping out the invalid parts from X-APPLE-STRUCTURED-LOCATION) I was able to import the feed. Maybe ignoring the invalid lines as suggested above also would work.

Currently implemented here in ./apps/dav/lib/CalDAV/WebcalCaching/RefreshWebcalService.php (calling a cleanup before parsing):

@@ -244,6 +294,7 @@
                                case 'text/calendar':
                                default:
                                        try {
+                                               $body = $this->cleanVCalendarBody($body);
                                                $vCalendar = Reader::read($body);
                                        } catch (Exception $ex) {
                                                // In case of a parsing error return null

@neufeind
Copy link

neufeind commented Nov 8, 2023

As you suggested it seems to also work with adding the ignore-option. Maybe that is really a viable solution we could add?

-                                               $vCalendar = Reader::read($body, OPTION_IGNORE_INVALID_LINES);
+                                               $vCalendar = Reader::read($body, Reader::OPTION_IGNORE_INVALID_LINES);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. to triage Pending approval or rejection bug
Projects
None yet
Development

No branches or pull requests

5 participants