575782 - Lightning 1.0 beta 2 does not work with non-ASCII characters in event names

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Bernd Kuemmerlen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.4) Gecko/20100608 Lightning/1.0b2 Thunderbird/3.1

I have updated from Thunderbird 3.0 with Lightning 1.0b1 to Thunderbird 3.1 with Lightning 1.0b2, using a CalDav provider (DavMail 3.6.6-1032 talking to an Exchange 2003 server).

Since the update, not all calendar entries are displayed anymore. CalDav logs "HTTP/1.1 404 Not Found" for some calendar entries. The entries all seem to contain Umlaut characters or the € sign.

CalDav log:
<D:response><D:href>/users/[e-mail]/calendar/Osterferien%20(Baden-W%3Frttemberg)-16.EML</D:href><D:propstat><D:status>HTTP/1.1 404 Not Found</D:status></D:propstat></D:response>

Lightning log:
Warning: CalDAV: Unexpected response, status: undefined, href: /users/[e-mail]/calendar/Osterferien (Baden-W?rttemberg)-16.EML calendar-data:

There are about 10 Warnings like this in the Lightning log, followed by a (very long) "CalDAC: recv" section, then the following error:

Error: Assert failed: unexepcted endBatch!
2: [file:///C:/Users/bkuemmerlen/AppData/Roaming/Thunderbird/Profiles/ba9sao7p.Empty%20Test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/modules/calProviderUtils.jsm:647] cPB_endBatch
3: [file:///C:/Users/bkuemmerlen/AppData/Roaming/Thunderbird/Profiles/ba9sao7p.Empty%20Test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/modules/calUtils.jsm -> file:///C:/Users/bkuemmerlen/AppData/Roaming/Thunderbird/Profiles/ba9sao7p.Empty%20Test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calDavRequestHandlers.js:797] mg_onStopRequest

Source File: file:///C:/Users/bkuemmerlen/AppData/Roaming/Thunderbird/Profiles/ba9sao7p.Empty%20Test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/modules/calUtils.jsm -> file:///C:/Users/bkuemmerlen/AppData/Roaming/Thunderbird/Profiles/ba9sao7p.Empty%20Test/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js
Line: 975

Warning: CalDAV: Get failed: multiget error
Warning: There has been an error reading data for calendar: MMS.  However, this error is believed to be minor, so the program will attempt to continue. Error code: 0x80004005. Description: multiget error
Warning: There has been an error reading data for calendar: MMS.  However, this error is believed to be minor, so the program will attempt to continue. Error code: READ_FAILED. Description: 

After this, the calendar displays a few entries, but a lot of entries are not displayed (as confirmed through the Outlook Web Interface). Next to the Calendar the icon for "The calendar XXX is momentarily not available" is displayed.

Reproducible: Always

Steps to Reproduce:
1. Create a CalDav calendar connection to a local DavMail server
2. Open Calendar

Actual Results:  
The Calendar does not display all events on the server, only a subset. Warnings and errors are displayed in the Error Console.

Expected Results:  
All events should be displayed in the Calendar.

This has been tested with a completely new clean profile. It is 100% reproducible (i.e. I can not get it to work) with Lightning 1.0 beta2.

Using Lightning 1.0 beta 1 on Thunderbird 3.1, all Calendar entries load fine, but after restarting Thunderbird I get a message that the Calendar could not be loaded, and I have to delete and recreate the CalDav calendar.

Comment 1

[Bernd Kuemmerlen]

Changing a few of the calendar entries to not contain Umlauts anymore removes them from the Warning log in Lightning, but others appear instead.

Comment 2

[Bernd Kuemmerlen]

The same issue can be reproduced in 
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.4) Gecko/20100610 Sunbird/1.0b2

Comment 3

[Simon Vaillancourt]

Is it possible to have a test account on a DavMail server so I can reproduce the issue? You can email me the credentials, thanks.

Comment 4

[Bernd Kuemmerlen]

Sorry, since this is an Exchange 2003 server over which I have no control, I can not provide a test account.

Comment 5

[Simon Vaillancourt]

In that case I think the full logs would be useful:

1) Go in tools->options->advanced (General tab) and "Config Editor..."
2) Add or set these preference names to "true":
calendar.debug.log
calendar.debug.log.verbose
3) Install the Console2 addon(To copy all the logs at step 5)
4) Reproduce the issue
5) Tools->Error console->Right click->Select All,  Right click->Copy
6) Either mail the logs to me or add them as attachment to this bug (if your meetings contain no sensitive info)

You can wait a week or so before doing that as I think there are other similar bugs I have to look at that might also resolve this issue. 

Thanks.

Comment 6

[Bernd Kuemmerlen]

Logs have been sent by private e-mail

Comment 8

[Simon Vaillancourt]

Attachment: Encode path before sending multiget request

Make sure the path is encoded when building the multiget request.

I could not test this on a DavMail server since I don't have a test account, so please test on as many different servers as you can.

Comment 9

[Bernd Kuemmerlen]

I have tested this patch against 
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.4) Gecko/20100610
Sunbird/1.0b2
and it does indeed fix the issue for me!

Thanks a lot!

Comment 10

[Bernd Kuemmerlen]

Now also tested successfully in 
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.4)
Gecko/20100608 Lightning/1.0b2 Thunderbird/3.1

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This seems like a quite critical bug, we should consider doing a chemspill release for this. I've seen quite a few caldav regressions after the release, we should get as many of them fixed as possible.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 456281 [details] [diff] [review]
Encode path before sending multiget request

>-            let locpath = this.itemsNeedFetching.pop();
>+            // Rebuild the path to make sure it is encoded properly
>+            let locpath = this.calendar.ensurePath(this.itemsNeedFetching.pop());
>+            locpath = makeURL(this.baseUri.prePath + locpath).path;
>             queryXml.D::prop += <D:href xmlns:D={D}>{locpath}</D:href>;
Can it be the case that whatever is in this.itemsNeedFetching.pop() is already a full uri? (i.e the server advertises full uris rather than relative) In this case locpath will probably be http://.../http://..../...


r=philipp if I am mistaking, but I really think we should be more robust here.

Comment 13

[Martin Bálint]

This patch made events with non-ascii characters downloaded.
However, there is a similar problem, when dismissing such event.

E.g.
PUT /users/[e-mail]/calendar/test%20+%C4%9B%C5%A1%C4%8D.EML

but event name is actually "test +ěšč", so correctly urlencoding is
PUT /users/[e-mail]/calendar/test%20%2B%C4%9B%C5%A1%C4%8D.EML

Comment 14

[Martin Bálint]

If characters + and # are present in event name, such event cannot be dismissed.
In DavMail logs, I can see 404.

Comment 15

[Simon Vaillancourt]

@Philipp: whether the server returns a full uri or not does not matter because we call ensurePath() on the uri so we "extract" only the path information from it and we then rebuild it to make sure it is properly encoded.

@Martin: The encoding is done by nsIURI which follows the rules of rfc2396/3986(Section 2.2), even if +  and # are described as "reserved" characters they do not need to be encoded since they are not delimiters in the context described. I believe this specific case is a server bug: The server should unescape this request path properly before matching it with a server resource. In other words, it shouldn't matter if the client percent encodes + or # in the requests, the server should be able to match it anyways.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #15)
> @Philipp: whether the server returns a full uri or not does not matter because
> we call ensurePath() on the uri so we "extract" only the path information from
> it and we then rebuild it to make sure it is properly encoded.
Yes, I've taken another look at the code and ensurePath is called as you've mentioned. 


> 
> @Martin: The encoding is done by nsIURI which follows the rules of
> rfc2396/3986(Section 2.2), even if +  and # are described as "reserved"
I've talked with Simon, and we've come to the conclusion this is a server bug. As Simon mentioned, # and + are not delimiter characters in the caldav context, so they don't need to be escaped. The workaround is easy for now, so please file a bug with davmail, feel free to reference this bug.

Comment 17

[Simon Vaillancourt]

Actually after re-reading the RFCs with second set of eyes(Thanks Bern!) I've come to the conclusion that we should encode the # and + since their value is not equivalent to their % encoding.. and also it is stated elsewhere that their values must be encoded IF they have no delimiter role in the URL, I will look at the available mozilla functions and might end up writing my own.

3.2.3 URI Comparison
http://tools.ietf.org/html/rfc2616#section-3.2.3 

Characters other than those in the "reserved" and "unsafe" sets (see RFC 2396 [42]) are equivalent to their ""%" HEX HEX" encoding. 

http://tools.ietf.org/html/rfc3986 
reserved = gen-delims / sub-delims
gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@"
sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
/ "*" / "+" / "," / ";" / "="

Comment 18

[Simon Vaillancourt]

Attachment: patch v2

This patch makes the caldav provider encode all "unsafe" characters. I've also changed the webdav sync token name property in order to trigger a refresh so that paths that are not encoded differently will get picked up correctly. 

This change affects pretty much all requests done by the caldav provider so please test on as many servers as you can.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 492906 [details] [diff] [review]
patch v2

> 
>     makeUri: function caldav_makeUri(aInsertString, aBaseUri) {
>         let baseUri = aBaseUri || this.calendarUri;
>-        let spec = baseUri.spec + (aInsertString || "");
>-        if (this.mUriParams) {
>-            spec += this.mUriParams;
>-        }
>-        return makeURL(spec);

>+        let decodedSpec = baseUri.prePath + this.ensureDecodedPath(baseUri.path) + (aInsertString || "");
>+        let url = cal.makeURL(baseUri.prePath + this.ensureEncodedPath(decodedSpec) + (this.mUriParams || ""));
I don't quite understand: the decodedSpec contains baseUri.prePath, then the url again prepends baseUri.prePath? Doesn't this cause a double prePath ?


>+        cal.LOG("CalDAV: makeUri: aInsertString=" + aInsertString + ", baseUri=" + (baseUri ? baseUri.spec : "undefined")+", final url=" + url.spec);
We might want to get rid of this before checkin, this might happen potentially very often. Or at least move it to verbose caldav logging.


>+        cal.WARN("Spec could not be parsed, returning as-is: "+ aSpec);
This should be prefixed with CalDAV:


>+        var uriComponents = aString.split("/");



>+        for (var i = 0 ; i < uriComponents.length ; i++ ) {
>+            uriComponents[i] = encodeURIComponent(uriComponents[i]);
>+        }
uriComponents = uriComponents.map(encodeURIComponent);

or even:

return uriComponents.map(encodeURIComponent).join("/");


I've tested this with sogo and it seems to work. I unfortunately don't have other servers at hand.
r=philipp with comments considered.

Comment 20

[Simon Vaillancourt]

pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/42371756f234

Comment 21

[Simon Vaillancourt]

backported to comm-1.9.2:
http://hg.mozilla.org/releases/comm-1.9.2/rev/678d78f024ca