sabre-io / dav

sabre/dav is a CalDAV, CardDAV and WebDAV framework for PHP
http://sabre.io
BSD 3-Clause "New" or "Revised" License
1.51k stars 344 forks source link

comma and semicolon must be escaped in values #83

Closed evert closed 11 years ago

evert commented 11 years ago

Original author: lars.kne...@gmail.com (September 10, 2011 08:32:44)

According to RFC 2426 (see: http://www.rfc-ref.org/RFC-TEXTS/2426/chapter2.html#sub5 ) comma and semicolon must be escaped in values. And it seems like colons also have to get escaped.

I think the correct place to do this should be Sabre_VObject_Property::serialize(). Sabre_VObject_Parameter::serialize() is escaping them.

Is it missing in Sabre_VObject_Property::serialize() by intent?

Original issue: http://code.google.com/p/sabredav/issues/detail?id=145

evert commented 11 years ago

From evert...@gmail.com on September 10, 2011 10:06:28: Yes this was intended. It's not as simple as just escaping it everywhere. Take the N: property for example, it's values are escaped by comma's.

So the only way for me to do this is to build in more intelligence for every VCARD/iCalendar property and specify what type of values they may support; only then I can properly determine wether or not a comma should be escaped in certain cases.

I'd like to do this eventually, but for the time I'm keeping this a low-priority feature request.

evert commented 11 years ago

From lars.kne...@gmail.com on September 10, 2011 11:09:47: We could set an array as value. Then we can loop of the single values, escape them as need and concatenate the values. Lists can be supported with multidimensional array.

I'll try to implement a proposal, which solves my problem and maybe can help you finding a generic solution.

evert commented 11 years ago

From evert...@gmail.com on September 10, 2011 12:43:26: That makes sense, but it would be very important to me to keep the api as intuitive as it is, so keep in mind i may not accept a patch for pure aesthetic reasons

evert commented 11 years ago

From lars.kne...@gmail.com on September 10, 2011 17:20:05: While trying to solve the problem I digged a little deeper and stumbled into another problem.

In Sabre_VObject_Reader::readLine there is this line:

$propertyValue = stripcslashes($matches['value']);

This makes parsing this line impossible:

;wer;Pickhuben 2;Hamburg;Hamburg\; abced;20457;DE

because after stripcslahes the semicolon not escaped anymore. This way the string can't be parsed correctly later anymore, as there is one semicolon to much now.

How do you parse such lines?

evert commented 11 years ago

From lars.kne...@gmail.com on September 10, 2011 18:51:43: I digged deeper again. And from my point of view the logic is asymmetric. This makes it hard to find a good solution.

The counter part to Sabre_VObject_Property::serialize() is located in Sabre_VObject_Reader::readline().

I would expect that Sabre_VObject_Property has a unserialize function which knows how to decode a given value.

Now we have the situation that in the readline function other slashes get stripped, while in the serialize function only 2 of them get added again.

First the logic which slashes get added and removed is asymmetric. And then it's asymmetric in which class the slashes got removed and added.

Just thoughts... :-)

evert commented 11 years ago

From lars.kne...@gmail.com on September 10, 2011 21:14:18: I solved the problem for me. And from my point of view, it should match the current api very well.

I moved the code to add / remove the slashes for :,\n and \ to two static functions in Sabre_VObject_Property. They add and remove the slashes for the same characters. Now the code is symmetric for adding and removing slashes.

The handling for compound values, got moved to two other static functions(splitCompoundValues and concatCompoundValues).

Now we can do following.

We can strip the slashes for :, \n and \ in the Sabre_VObject_Reader class.

When we like to split the compound values we can call Sabre_VObject_Property::splitCompoundValues($value); to get an array of the splitted values.

We can still set single values via Sabre_VObject_Property->value.

We can set compound values via new Sabre_VObject_Property('ORG', Sabre_VObject_Property::concatCompoundValues(array($value1, $value2))); which also adds the slashes for commas and semicolons as needed.

Sabre_VObject_Property::serialize() then only needs to add the slashes for :, \n and \ and everything if fine(at least from my point of view).

With these modifications everything is working for me and all my problems are solved.

Find the patches attached.

evert commented 11 years ago

From evert...@gmail.com on September 12, 2011 12:27:52: Lars: you are totally correct that the implementation was incorrect. I'll incorporate the patches in a following release.

evert commented 11 years ago

From lars.kne...@gmail.com on September 14, 2011 07:27:41: Ok!

evert commented 11 years ago

From evert...@gmail.com on September 21, 2011 11:58:59: I can't add this to the next release yet (happening today). I want to review this further, but simply don't have the time right now. So this change will have to happen at a slightly later point in time.

evert commented 11 years ago

From mscher...@intermesh.nl on December 09, 2011 10:13:42: Great that this will be in 1.6.

Additionally something goes wrong with parameters.

A contact can have ADR;TYPE=HOME,WORK:

The , in this parameter will be escaped which shouldn't happen.

evert commented 11 years ago

From evert...@gmail.com on December 09, 2011 17:02:14: Issue 177 has been merged into this issue.

evert commented 11 years ago

From evert...@gmail.com on August 23, 2012 10:13:16: Way late, but this is pretty much fixed in the new VObject library:

https://github.com/evert/sabre-vobject

It's not 100% nailed down yet, but it will be released with v2.0.1. This also means it will now definitely be shipped with SabreDAV 1.7.

Sorry for the massive delay, and thanks for the contributions.