emersion / go-ical

An iCalendar library for Go
MIT License
52 stars 10 forks source link

Expose escaping #6

Closed Dynom closed 3 years ago

Dynom commented 3 years ago

Hi,

Prop.SetText() applies value escaping through Prop.SetTextList(). However it's currently too opinionated to use a general value escaping. I propose to open up the escaping, te be re-used for more complex situations:

// Potentially problematic, since none of the parameters or the value is escaped.
e.Props.Set(&ical.Prop{
    Name: ical.PropOrganizer,
    Params: ical.Params{
        ical.ParamCommonName: []string{organiser.Name},
    },
    Value: fmt.Sprintf("MAILTO:%s", organizer.EmailAddress),
})

Now we need to duplicate the escaping functionality to make sure that the value passed here is also properly escaped. Ideally I would like to see an ical.EscapeValue() which does so that there can be consistent escaping.

func EscapeValue(s string) string {

    var sb strings.Builder
    sb.Grow(len(s))

    for _, r := range s {
        switch r {
        default:
            sb.WriteRune(r)

        case '\\', ';', ',':
            sb.WriteByte('\\')
            sb.WriteRune(r)
        case '\n':
            sb.WriteString(`\n`)
        // Should \r's be written at all, or is it safe to skip these?
        case '\r':
            continue

        }
    }

    return sb.String()
}
emersion commented 3 years ago

I don't really want to require users to manually escape values. go-ical should transparently do it.

Dynom commented 3 years ago

Hi @emersion,

That'd be even better, but does this mean all values are now already escaped? So basically I don't need to apply any escaping to any value which is set on on a Prop?

emersion commented 3 years ago

I wonder. What does the RFC ABNF say?

Dynom commented 3 years ago

Your guess is as good as mine.

I've been reading up on it and I'm not sure on some accounts. e.g.: is a semicolon ; allowed after a colon : or does it need to be escaped? Currently in the go-ical implementation it isn't, I'm not sure if it should be. I'd love a reference implementation, but I'm not at that point yet, I had hoped you already had an answer on it :-)