arran4 / golang-ical

A ICS / ICal parser and serialiser for Golang.
Apache License 2.0
279 stars 72 forks source link

Property spanning multiple lines causes `Malformed Calendar Error` #48

Closed JM-Lemmi closed 2 years ago

JM-Lemmi commented 2 years ago

Iterating over events in an Outlook generated .ics-File creates an invalid memory address or nil pointer dereference-Error.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x4a5bdd]

goroutine 1 [running]:
github.com/arran4/golang-ical.(*Calendar).Events(0x0)
        /workspaces/golang-ical/calendar.go:409 +0x1d
main.main()
        /tmp/test/testing.go:14 +0x89

I have attached an example entry I exported from Outlook as well as my testing code to reproduce the error.

issue48_attachments.zip

arran4 commented 2 years ago

Thanks. I will look at it when I have time. Mind if I use the attached ICS file as part of the tests?

arran4 commented 2 years ago

What is the commit ref you are using?

JM-Lemmi commented 2 years ago

Mind if I use the attached ICS file as part of the tests?

Sure. I can also generate more, with different options if you need more.

What is the commit ref you are using?

I was using efac1f4c as well as 8026c5c4 from shaohme/golang-ical in case it was related to the Date formatting from my O365 locale, but I don't think it is related to that.

JM-Lemmi commented 2 years ago

Also just tried 32b67e2 and it gives the same error

arran4 commented 2 years ago

Also just tried 32b67e2 and it gives the same error

What is the stack trace for this one?

JM-Lemmi commented 2 years ago
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x4a5a1d]

goroutine 1 [running]:
github.com/arran4/golang-ical.(*Calendar).Events(0x0)
        /workspaces/golang-ical/calendar.go:420 +0x1d
main.main()
        /tmp/test/testing.go:14 +0x89
arran4 commented 2 years ago

Okay so: https://github.com/arran4/golang-ical/blob/master/calendar.go#L420

Seems to be on the loop.. I can see in your sample:

package main

import (
    "fmt"
    "os"

    ics "github.com/arran4/golang-ical"
)

func main() {
    addicsfile, _ := os.Open("outlookentry.ics")
    addics, _ := ics.ParseCalendar(addicsfile)
    fmt.Println("Parsed Calendar")
    for _, event := range addics.Events() {
        fmt.Println(event.Id())
    }
}

You're ignoring the error results?

Can you run something like this:

package main

import (
    "fmt"
    "os"
       "log"

    ics "github.com/arran4/golang-ical"
)

func main() {
        log.SetFlag(log.Lshortfile | log.Flags())
    addicsfile, err := os.Open("outlookentry.ics")
        if err != nil { log.Panicf("open: %s", err) }
    addics, err := ics.ParseCalendar(addicsfile)
        if err != nil { log.Panicf("parse: %s", err) }
    fmt.Println("Parsed Calendar")
    for _, event := range addics.Events() {
        fmt.Println(event.Id())
    }
}
JM-Lemmi commented 2 years ago

Ah, I see my mistake in discarding the error. I only started writing in go a week ago, so I'm still learning.

It does give a Malformed calendar Error now.

arran4 commented 2 years ago

It doesn't /look/ malformed but I will have to dig in a bit further I will have a quicker look after work. (it's 9am here.)

JM-Lemmi commented 2 years ago

The icalendar.org validator gives the following error:

Invalid TZID value or missing VTIMEZONE component ("W. Europe Standard Time") near line # 21 Reference: 3.2.19. Time Zone Identifier

But I don't see it? The Error says, that TZID MUST be specified on the "DTSTART", "DTEND", "DUE", "EXDATE", and "RDATE" properties [...].

But line 25 and 27 of my test .ics do include the TZID:

L25> DTEND;TZID="W. Europe Standard Time":20211219T133000
L27> DTSTART;TZID="W. Europe Standard Time":20211219T130000
arran4 commented 2 years ago

It's right: W. Europe Standard Time Is not a valid timezone /id/ it's a descriptor.

image

Try Europe/Lisbon

arran4 commented 2 years ago

Abbreviated too!

JM-Lemmi commented 2 years ago

So this is probably on Microsoft to implement it correctly. But I don't think that they'll get around to something like this anytime soon.

I tried a few different identifiers (Europe/Lisbon, Europe/Berlin, Europe/London, GMT and even America/New_York as given in the example on Reference 3.2.19), but they were all rejected, both by go as well as well as the icalendar validator.

arran4 commented 2 years ago

Oh sorry. I will have to have a better look after work, it could be that it's defining the timezone with that ID.

JM-Lemmi commented 2 years ago

I changed the identifier in L7 to match.

I just tried changing the Name of the Timezone in the Outlook Settings, but it does not show up in the ics file.

Also submitted a Feedback-report to Microsoft.

JM-Lemmi commented 2 years ago

If I leave out the Z at the end of the Date Stamp and the " fromt he TZID, the ical validator will now accept my .ics. Even with the Spaces, the ID seems to be valid.

https://gist.github.com/JM-Lemmi/19167eadbeb9f7f050018c3fa609b645

But golang-ical will still error "Malformed Calendar" on this validated version. I would look into it over the Holdays and see if I can correct the parser to get the validated ics to be accepted.

Would you also be willing to include parsing for the "malformed" Outlook entries? Since I don't have high hopes for MS fixing it any time soon.

JM-Lemmi commented 2 years ago

I have to correct myself. The Z at the end of DTSTAMP and LAST-MODIFIED is actually also valid. The only invalid part in the outlook ics are the " in the TZID.

You can assign this issue to me.

JM-Lemmi commented 2 years ago

I've experimented some more, building the ics part by part to see, what line exactly is at fault for the error and I've identified that UID and X-ALT-DESC following issues as actually causing the Malformed Calendar Error. Both are spread over multiple lines, with the following lines being marked by beginning with a TAB.

The " in TZID are not actually a problem. The icalendar.org-validator was throwing us off the actual problem.

So the actual issue has to do with any Property spanning multiple lines. Should I close this issue and open a new one for that or rename this issue?

JM-Lemmi commented 2 years ago

This fixes the issue for Outlook generated calendars. Google seems to use newline + one space for entries wrapping multiple lines. I will have to fix that too, but the filtering may be a bit more complicated.

arran4 commented 2 years ago

Can you give me some sample data?

JM-Lemmi commented 2 years ago

For example the CTFtime calendar in L6256. But any Google shared calendar will probably have this break somewhere.

The easy regex would be \n (newline + space), but I think this catches more than we want. So maybe \n [^\s], or even \n\s[^\s], which should catch both Google and Outlook.

Though I am not sure, how I can match regex on the bytestream

JM-Lemmi commented 2 years ago

Actually (?!^\s)\n\s, otherwise it would match the non-whitespace character we also want.

JM-Lemmi commented 2 years ago

I created a calendar for Testing purposes that only contains one entry: https://calendar.google.com/calendar/ical/vob796cf5s95rqfhii4n65h40k%40group.calendar.google.com/private-a4300f7f60eb8fad400fe95a820cfc32/basic.ics

JM-Lemmi commented 2 years ago

Actually (?!^\s)\n\s, otherwise it would match the non-whitespace character we also want.

I think I was wrong there. We also want to remove linebreaks if there is a trailing whitespace, as is illustrated in the examples in calendar_test.go

Now in adf0eaa1510ad7a8b5dfe866379fff4949c61c80 i just removed 0x0D0A20.

JM-Lemmi commented 2 years ago

Fixed by e3ae829