Open Bug 605201 Opened 14 years ago Updated 3 years ago

Updating an event results in a PUT to the wrong CalDav URL

Categories

(Calendar :: Provider: CalDAV, defect)

Lightning 1.5
defect
Not set
major

Tracking

(Not tracked)

People

(Reporter: jdc, Unassigned)

References

Details

(Whiteboard: [CalDAV server: DAViCal][calconnect31-haspatch])

Attachments

(2 files, 2 obsolete files)

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.
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.
OS: Windows XP → All
Jonathan could you please provide the URL rewrites you have setup in DAViCal.
RewriteEngine On
RewriteCond %{REQUEST_URI} !^/$
RewriteCond %{REQUEST_URI} !\.(php|css|js|png|gif|jpg)
RewriteRule ^(.*)$ /caldav.php/$1  [NC,L]
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.
Whiteboard: [CalDAV server: DAViCal]
Version: unspecified → Lightning 1.0b2
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).
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 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. ;)
Attachment #532960 - Flags: review?(philipp)
Assignee: nobody → jb-mozilla
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.
Assignee: jb-mozilla → nobody
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.
Attachment #532960 - Attachment description: Patch against 1.0b2 which might solve this → [after beta] Patch against 1.0b2 which might solve this
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.
Attachment #532960 - Attachment is obsolete: true
Attachment #532960 - Flags: review?(philipp)
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.
Attachment #548147 - Attachment description: Updated patch matching the 1.0b4 code → [after beta] Updated patch matching the 1.0b4 code
Attachment #548147 - Flags: review?(philipp)
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 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
Attachment #548147 - Flags: review?(philipp) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [CalDAV server: DAViCal] → [needs updated patch] [CalDAV server: DAViCal]
Attached patch Updated patch against current stable (obsolete) — — Splinter Review
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.
Someone told me it would be good to tell which version we're on: Thunderbird 13.01 and lightning 1.5.1.
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.
Attachment #637023 - Flags: review?(philipp)
Assignee: nobody → kwadronaut+bugzilla.mozilla
Status: NEW → ASSIGNED
Hardware: x86 → All
Whiteboard: [needs updated patch] [CalDAV server: DAViCal] → [CalDAV server: DAViCal]
Version: Lightning 1.0b2 → Lightning 1.5
+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.
rgnkjnva@aol.com: Lightning 1.5 is available with Thunderbird 13.0 (and 1.6 with TB 14.0).
Whiteboard: [CalDAV server: DAViCal] → [CalDAV server: DAViCal][calconnect25]
Attachment #637023 - Attachment is obsolete: true
Attachment #637023 - Flags: review?(philipp)
Attached patch Fix - v3 — — Splinter Review
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
Attachment #686757 - Flags: review+
Pushed to comm-central changeset 83ac919fe88d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2
Depends on: 849797
Patch was backed out due to regressions in bug 849797 for Lightning 2.2, Lightning 2.3, and Lightning 2.4 branches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
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).
Assignee: kwadronaut+bugzilla.mozilla → nobody
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.
Target Milestone: 2.2 → ---
Whiteboard: [CalDAV server: DAViCal][calconnect25] → [CalDAV server: DAViCal][calconnect31]
Whiteboard: [CalDAV server: DAViCal][calconnect31] → [CalDAV server: DAViCal][calconnect31-haspatch]
Thunderbird 52.3.0
Lightning 5.4.3
DaviCal with rewrite rules.

Bug still exists. Will it be fixed?
Almost year passed by and no answer (
I found this article - http://codeverge.com/mozilla.support.calendar/lightning-1.0b2-caldav-problem-with/1513822
Patch for that version is included.
Maybe this help to fix this odd bug with 8 years lifetime and release fixed Lightning version?
I am also affected by this bug with Thunderbird 52.9.1 (64-bit) and lightning 5.4.9.1 .

The link provided by Dron is similar to
https://groups.google.com/forum/#!topic/mozilla.support.calendar/2cNId3kT6y8
The solution and patch proposed there used to work for me years ago, but not anymore. This patch patches the function addTargetCalendarItem in calDavCalendar.js, which is not used for modifying events (anymore). So (probably) there is another bug in the code regardig the handling of CalDAV-URLs in another function.

This is what I experience when trying to modify an event:
Everything works just fine for calendars like this: https://calendar.example.com/calendar/caldav.php/username/calendar
Modifications do not work for calendars like this (addtitions and deletions are still OK): https://calendar.example.com/username/calendar

For the latter kind of calendar I can see the following in ther Thunderbird Error Console, when I modify an item:
> PUT https://calendar.example.com/username/calendar/.php/username/calendar/778d0468-e6c3-456e-8bcc-ec92adffc15f.ics [HTTP/1.1 405 Method Not Allowed 1904ms]

The correct URL should be: https://calendar.example.com/username/calendar/778d0468-e6c3-456e-8bcc-ec92adffc15f.ics
This behavior is 100% reproducible and happens to all my accounts (following the abovementioned calendar URL pattern).

This is the call stack, as far as I can see:
> authSuccess file:///usr/lib/lightning/components/calDavCalendar.js:381:13
> calDavCalendar.prototype.sendHttpRequest file:///usr/lib/lightning/components/calDavCalendar.js:398:13
> calDavCalendar.prototype.doModifyItem file:///usr/lib/lightning/components/calDavCalendar.js:842:9
> calDavCalendar.prototype.modifyItem file:///usr/lib/lightning/components/calDavCalendar.js:740:16
> calCachedCalendar.prototype.modifyItem/flagListener.onOperationComplete file:///usr/lib/lightning/calendar-js/calCachedCalendar.js:719:21
> calStorageCalendar.prototype.getItemOfflineFlag/listener.handleCompletion file:///usr/lib/lightning/components/calStorageCalendar.js:959:21

Regards,
Ferdinand Rau
Status: REOPENED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: