Last Comment Bug 605201 - Updating an event results in a PUT to the wrong CalDav URL
: Updating an event results in a PUT to the wrong CalDav URL
Status: REOPENED
[CalDAV server: DAViCal][calconnect31...
:
Product: Calendar
Classification: Client Software
Component: Provider: CalDAV (show other bugs)
: Lightning 1.5
: All All
: -- major with 3 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 849797
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-18 11:39 PDT by Jonathan Csanyi
Modified: 2014-09-29 07:57 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[after beta] Patch against 1.0b2 which might solve this (3.11 KB, patch)
2011-05-17 07:52 PDT, Jakob Bohm
no flags Details | Diff | Review
[after beta] Updated patch matching the 1.0b4 code (3.08 KB, patch)
2011-07-25 04:49 PDT, Jakob Bohm
philipp: review+
Details | Diff | Review
Updated patch against current stable (2.58 KB, patch)
2012-06-27 01:17 PDT, kwadronaut+bugzilla.mozilla
no flags Details | Diff | Review
Fix - v3 (2.68 KB, patch)
2012-11-29 12:52 PST, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Review

Description Jonathan Csanyi 2010-10-18 11:39:31 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.41 Safari/534.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100915 Lightning/1.0b2 Thunderbird/3.1.4

I can add new events to my CalDav calendars, but when I try to edit or delete them, the operation fails with a MODIFICATION_FAILED error code.

The calendar I am subscribed to is at /user/calname, but looking at the backend webserver logs (Davical on Apache), I see that the PUT operation is happening at /user/calname/calname/some-id-here. (the calname part is doubled).

The real davical URL for the calendar is /caldav.php/user/calname
If I use this URL, everything works, and the backend web logs indicate the PUT request is being sent to /caldav.php/user/calname/some-id-here as expected.
I've setup the URL rewrites (suggested as part of the davical install) to get the friendlier URLs, and it's only when I use these URLs that I encounter the problem. Subscribing to the calendar and adding new events works as expected.

Reproducible: Always

Steps to Reproduce:
1. Subscribe to a calendar at http://server/user/calname
2. Add a new event
3. Delete or modify the event
Actual Results:  
MODIFICATION_FAILED error is indicated.
Looking at the backend web logs, I see that the PUT request happened on the /user/calname/calname/some-id-here URL

Expected Results:  
Event should be modified as requested.
Looking at the backend web logs, I would expect the PUT request to happen on the /user/calname/some-id-here URL.
Comment 1 Jonathan Csanyi 2010-10-18 11:55:53 PDT
The situation described above works as expected in Evolution, suggesting it's probably not a problem with the DAViCal server or the rewritten URLs themselves.
Comment 2 Felix Möller 2011-02-21 16:55:37 PST
Jonathan could you please provide the URL rewrites you have setup in DAViCal.
Comment 3 Jonathan Csanyi 2011-02-23 11:21:31 PST
RewriteEngine On
RewriteCond %{REQUEST_URI} !^/$
RewriteCond %{REQUEST_URI} !\.(php|css|js|png|gif|jpg)
RewriteRule ^(.*)$ /caldav.php/$1  [NC,L]
Comment 4 Andrew McMillan 2011-02-23 15:37:13 PST
I've seen this kind of thing happening before, and I am pretty sure it is a configuration issue rather than a Lightning issue.

If you set in your config that:

 $c->base_url = "/";

for example, it might work (though that's a hack rather than a correct solution).

The fact that it works with Evolution is not necessarily an indication that Lightning is doing anything wrong.  The two programmes take very different approaches to accessing CalDAV servers.

Something that would be a data point is whether you have caching enabled on the calendar in Lightning.  More information could also be found in examing the log files in Apache if you enable the following settings in DAViCal's configuration:

$c->dbg['request'] = 1;
$c->dbg['response'] = 1;

This will log the full request / response process in DAViCal (including headers) and will probably show that DAViCal is the first thing showing those urls...

Regards,
Andrew McMillan.
Comment 5 Jakob Bohm 2011-05-17 07:42:03 PDT
Confirming that this happens to me too.  I have traced the bug to an overly naive "url cleaning" algorithm in calDavCalendar.js in combination with the fact that URL rewriting does not change URLs embedded in replies, only those in HTTP headers.

I have created a patch against 1.0b2 for our own use, but it has not seen much testing, however I will attach it anyway as it might help (if it isn't broken).
Comment 6 Jakob Bohm 2011-05-17 07:52:57 PDT
Created attachment 532960 [details] [diff] [review]
[after beta] Patch against 1.0b2 which might solve this

This patch changes the URL cleanup/replacement code in function caldav_addTargetCalendarItem() in calDavCalendar.js, so it is less likely to mess up server returned URLs if their embedded base URL is longer/shorter than the user configured calendar URL.

This patch is *very* untested!

Note: I am not a regular Mozilla developer, and I do not have the setup to run the Mozilla test suite or create a test for it, the Bug report above and the comments in the code (including the unchanged comment just before the biggest part of the patch) should inspire some relevant tests.
Comment 7 Martin Schröder [:mschroeder] 2011-05-31 08:32:09 PDT
Comment on attachment 532960 [details] [diff] [review]
[after beta] Patch against 1.0b2 which might solve this

Putting this patch in Philipp's review queue, so it doesn't get lost. ;)
Comment 8 Jakob Bohm 2011-06-10 04:18:12 PDT
Note to Martin: As I said, I am not a regular Mozilla developer and I don't have the time nor training to set up my own Mercurial mirror of the entire Mozilla tree, create Mozilla-format testcase nor learn any of the other procedures to make my patch suitable for release.  I am just a sophisticated end user who submitted a quick and dirty traditional unified-diff patch against a released file in the hope that it might help other affected users and/or help a real Mozilla Developer create a proper fix for inclusion in a later release.

I am removing myself as assignee of the bug so a real Mozilla Developer can pick it up.
Comment 9 Michael Rausch 2011-06-14 01:09:18 PDT
I can confirm everything mentioned before. I am seeing this issue myself for about 11 months now. I also had discovered that everything is working fine in Evolution, but in Thunderbird, using the rewrite URL will produce errors when changing events. Read access is totally fine, and I also can create events. When I try to change something existing however, Thunderbird will report a MODIFICATION_FAILED.

Looking at the error log reveals the following: "CalDAV: Unexpected status on modifying item on mycalendar home: 500"

So the server is returning an error code for "internal server error", which - like Andrew said before - might suggest a server configuration error. Setting '$c->base_url = "/";' in the DAViCal server configuration does not change anything with respect to that error.

I still need to test the patch provided above. Going to give feedback once I have done so.
Comment 10 Jakob Bohm 2011-07-25 04:49:40 PDT
Created attachment 548147 [details] [diff] [review]
[after beta] Updated patch matching the 1.0b4 code

This patch replaces 532960 for users running 1.0b4 and for any devs trying to port this to trunk.  I am keeping the old patch available as an end user workaround for users of 1.0b2, which (despite its beta status) is the shipping version for end users on Tb 3.x, while 1.0b4 is the shipping version for Tb 5.0.

Once again this is a traditional UniDiff patch against componets/calDavCalendar.js in the shipping ligtning .xpi, as I do not have a personal Hg mirror of Mozilla's build environment.
Comment 11 Philipp Kewisch [:Fallen] 2011-07-25 05:20:51 PDT
Comment on attachment 548147 [details] [diff] [review]
[after beta] Updated patch matching the 1.0b4 code

Thanks for the patch. Unfortunately its too risky to take this for the current beta (due very soon), but we'll consider this afterwards. I think this is ok given it can be fixed by server config.
Comment 12 Jakob Bohm 2011-07-25 06:03:23 PDT
Note: For the specific scenario of DaviCal with DaviCal's recommended Apache configuration, the only "server side" workaround is to remove the Apache rewrite rules, thus breaking all non-Lightning clients.

There is however a client side workaround for this scenario: In the Lightning config, type in the URL as https://yourserver.example.com/cal/caldav.php/you@example.com/somecal, rather than the server-preferred https://yourserver.example.com/cal/you@example.com/somecal .

This prevents Lightning from getting confused when temporary URLs sent back from the server included the "caldav.php/" part, but means you will have to reconfigure your client(s) if the server is upgraded to a different CalDav implementation.
Comment 13 Philipp Kewisch [:Fallen] 2012-01-17 04:06:40 PST
Comment on attachment 548147 [details] [diff] [review]
[after beta] Updated patch matching the 1.0b4 code

Review of attachment 548147 [details] [diff] [review]:
-----------------------------------------------------------------

Given this works for most server types, the patch looks fine. Could you whip up a patch with the below nits fixed? I'll do some testing in the meanwhile.

::: lightning.orig/components/calDavCalendar.js
@@ +109,5 @@
> + *
> + * @param s1        First string
> + * @param s2        Second string
> + */
> +function iseithersubstring(s1,s2) {

I think we can make this an inline function where its used and a one-liner, i.e.

function isEitherSubstring(s1, s2) (s1.length < s2.length ? s2.indexOf(s1) ? s1.indexOf(s2)) >= 0;

I'd also prefer the casing as above, i.e isEitherSubstring instead of iseithersubstring.

@@ +920,5 @@
> +        // a valid path that just has additional redundant components,
> +        // like /dav/davical.php/user@example/Calendar
> +        // The original 1.0b4 code would incorrectly mangle this to
> +        // /dav/user@example/Calendar/Calendar and then get a failure back
> +        // from the server.

Lets try to keep comments about old errors out of the code, the first paragraph is fine though.

@@ +931,5 @@
> +           x = uriPathComponents[uriPathComponentLength - 1];
> +        }
> +        x = x.toLowerCase();
> +        let j = resPathComponents.length - 2;
> +	//cal.LOG("Looking for '" + x + "' in '" + path + "'");

remove commented out lines, please
Comment 14 kwadronaut+bugzilla.mozilla 2012-06-27 01:17:41 PDT
Created attachment 637023 [details] [diff] [review]
Updated patch against current stable

Jakob Bohm removed himself from the subscribers list, so it's unlikely you'll get an improved patch from him, I guess. How has your testing been? I'm not so fond of javascript, but I tried to improve the patch with your comments, while still maintaining readability. I think having that renamed function inline is confusing, but feel free to adapt.
I made a diff against the 1.5.1, which works fine for me.

Making a similar note as Jakob Bohm: I'm not a mozilla developer nor do I have a Mozilla build/test environment here.
Comment 15 kwadronaut+bugzilla.mozilla 2012-06-27 23:03:02 PDT
Someone told me it would be good to tell which version we're on: Thunderbird 13.01 and lightning 1.5.1.
Comment 16 Martin Schröder [:mschroeder] 2012-06-30 02:25:58 PDT
Comment on attachment 637023 [details] [diff] [review]
Updated patch against current stable

Please, ask for review next time, so the patch does not stay unnoticed.
Comment 17 rgnkjnva 2012-07-27 14:06:40 PDT
+1 experiencing this same issue on Win7, TBird 12.0, Lightning 1.4 (I don't see 1.5 available). Our calendar paths are getting duplicated if Thunderbird is closed then re-opened. Afterward, we begin to get modify and delete requests with a path of <http request>/Calendar/Calendar which results in 404 due to incorrect path. HOWEVER, note that user can still create new events and doing so seems to 'reset' the path issue thereby allowing users to once again modify and delete events.
Comment 18 Cyril 2012-08-23 14:06:25 PDT
rgnkjnva@aol.com: Lightning 1.5 is available with Thunderbird 13.0 (and 1.6 with TB 14.0).
Comment 19 Philipp Kewisch [:Fallen] 2012-11-29 12:52:41 PST
Created attachment 686757 [details] [diff] [review]
Fix - v3

I've done some slight modifications and style fixes and have tested very simple operations (add/edit/delete) with these caldav servers:

Google,Yahoo,SOGo,Zimbra,Kerio,eGroupware,DAViCal

and it seems to be working.

r=philipp
Comment 20 Philipp Kewisch [:Fallen] 2012-11-29 12:53:45 PST
Pushed to comm-central changeset 83ac919fe88d
Comment 21 Stefan Sitter 2013-03-21 07:34:39 PDT
Patch was backed out due to regressions in bug 849797 for Lightning 2.2, Lightning 2.3, and Lightning 2.4 branches.
Comment 22 Jakob Bohm 2013-06-12 12:45:09 PDT
(In reply to Stefan Sitter from comment #21)
> Patch was backed out due to regressions in bug 849797 for Lightning 2.2,
> Lightning 2.3, and Lightning 2.4 branches.

Let me clarify all this:

1. My original patch was written 2 years ago against Lightning 1.0, and without access to Mozilla's full testsuite.  It is thus very likely that either a referenced variable was renamed by a different patch and/or an untested code path got invoked in a scenario I cannot test.  The patch (as modified by others) needs to be reviewed and tested against the current code base by someone with access to the latest tools and the hazmat training needed for working with Mercury.

2. The fundamental bug here is that Lightning contains code which changes URLs in an *attempt* to work around a bug in some server other than DaviCal.  In doing so the original Lightning 1.0 code would corrupt valid URLs, thus creating invalid requests that get rejected with HTTP error 500.  If Lightning simply accepted whatever URL the server sent it, things would work just fine with DaviCal, so this is most definitely a Lightning bug, not a server problem.

3. Removing the offending "url cleaning" code would fix things for DaviCal users, but might break with whatever buggy server the original "url cleaning" code was trying to work around.  Thus my original patch tried to preserve as much as possible of the apparently intended behavior of the code while avoiding the specific kind of incorrect url changing that caused the issue with DaviCal.  This made the patch rather ugly and complex, which is why I used helper functions such as iseithersubstring() to keep things readable.

4. It has been some time since I tested if the original broken code is still active in Lightning.

5. I am still subscribed to the bug (as an interested and affected end user), but I never wanted to be assigned as the responsible developer and removed that incorrect assignment when I got inundated with requests to make my patch fit the internal rules for Mozilla Corporation employees.
Comment 23 kwadronaut+bugzilla.mozilla 2013-06-12 14:30:22 PDT
Removing myself as assignee

1. I have moved, changed and dumped some of the calendaring servers I was using; so I can't test anymore

2. There is some code in davical too to work around specific client implementation issues, best is that someone tests who is at least aware of such things and/or corner cases

3. No hammocks or other testbeds laying around for Lightning code to be tested

4. I'm still subscribed (as an interested user).
Comment 24 Philipp Kewisch [:Fallen] 2013-06-13 05:22:18 PDT
Don't worry, this being backed out doesn't mean its your fault. We just needed to do it since it wasn't fully working. I'm sure someone will step up eventually and provide a patch that fixes the issue.

I agree the testbed isn't great, especially for caldav server testing. I've always wanted to do something about this, but you know how it is, there are always other things to take care of.

I would imagine some sort of fakeserver that provides the exact responses the server does, along with a creation script that allows mocking a server with one easy python script. This does mean we need to extract all requests from the caldav code somehow so the test script doesn't have to be changed each time a request changes in the .js code. Maybe something worth a student-project.

Note You need to log in before you can comment on or make changes to this bug.