Last Comment Bug 761178 - Malformed URL can cause Lightning to delete complete calendar file (CalDAV)
: Malformed URL can cause Lightning to delete complete calendar file (CalDAV)
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Provider: CalDAV (show other bugs)
: Lightning 1.8
: All All
: P1 critical with 4 votes (vote)
: 1.9.1
Assigned To: fschlich
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 09:09 PDT by thorsten_h@ymail.com
Modified: 2013-03-10 07:37 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
strip double slashes from aUri.path so that we calculate a correct uriPathComponentLength (848 bytes, patch)
2012-10-24 02:16 PDT, fschlich
no flags Details | Diff | Review
updated patch, including a check in doDeleteItem to prevent deletion of the entire collection (2.08 KB, patch)
2012-11-13 02:07 PST, fschlich
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
philipp: approval‑calendar‑esr+
Details | Diff | Review

Description thorsten_h@ymail.com 2012-06-04 09:09:35 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

- Import new CalDAV-calendar with URL of form "http://[server]/davical/caldav.php/[working group]/abwesenheit//" 
(Note the terminating double slashes)
- Create new appointment
- Duplicate it by copy/paste
- Delete one of them -> works fine
- Delete the other one -> Error message about missing resource

- It might not be necessary to duplicate the entry, but this is how it was reproduced twice.


Actual results:

- Import worked, showing existing entries in the calendar
- When deleting the first entry, Lightning creates a command which causes CalDAV to remove the entire calendar file. The server log reads like this:

[IP] - [username] [04/Jun/2012:11:27:57 +0200] "DELETE /davical/caldav.php/[working group]/abwesenheit// HTTP/1.0" 204 465 "-" "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 Lightning/1.4"

[IP] - [username] [04/Jun/2012:11:27:59 +0200] "DELETE /davical/caldav.php/[working group]/abwesenheit// HTTP/1.0" 404 508 "-" "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 Lightning/1.4"

The problem does not occur when removing one of the terminating slashes of the URL.


Expected results:

Just the two entries should have been deleted, leaving the rest of the data intact.
Comment 1 fschlich 2012-10-22 01:46:41 PDT
This problem has surfaced in other places as well, see for example http://davical-general.89287.n3.nabble.com/Davical-general-Delete-collection-by-accident-with-thunderbird-lightning-td3410709.html

it is not limited to old versions of TB / Lightning, but can be seen with current versions, such as Thunderbird/16.0.1 Lightning/1.8 as well

I notice that when the entire calendar gets deleted, the URI requested is the one of the entire calendar and not of a particular event / "ics file":

    "DELETE //davical/caldav.php/all/Testcal/ HTTP/1.1"

as opposed to

    "DELETE /davical/caldav.php/user/calendar/3b36b087-e59e-4ffd-9378-76f79ae8e179.ics HTTP/1.1"
Comment 2 fschlich 2012-10-23 06:19:38 PDT
to elaborate a bit, this is the entire conversation captured with wireshark:

DELETE //davical/caldav.php/fschlich/calendar/ HTTP/1.1
Host: simbabwe.devel:8081
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121010 Thunderbird/16.0.1 Lightning/1.8
Accept: text/xml
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive
Accept-Charset: utf-8,*;q=0.1
If-Match: "121964d8ec2b1c47789cccb18a74bfa2"
Authorization: Basic ZnMjaGxpY7g6YUh1JHY5OH0=
Pragma: no-cache
Cache-Control: no-cache

HTTP/1.1 204 No Content
Date: Tue, 23 Oct 2012 13:09:57 GMT
Server: Apache/2.2.16 (Debian)
X-Powered-By: PHP/5.3.3-7+squeeze14
DAV: 1, 2, 3, access-control, calendar-access, calendar-schedule
DAV: extended-mkcol, bind, addressbook, calendar-auto-schedule, calendar-proxy
X-DAViCal-Version: DAViCal/1.1.1; DB/1.2.11
Vary: Accept-Encoding
Content-Length: 0
Keep-Alive: timeout=15, max=100
Connection: Keep-Alive
Content-Type: text/plain; charset="utf-8"

To me, this looks like lightning telling davical to delete the entire collection, and davical duly complies. This is a lightning bug that causes the loss of precious user data.
Comment 3 fschlich 2012-10-24 02:16:42 PDT
Created attachment 674598 [details] [diff] [review]
strip double slashes from aUri.path so that we calculate a correct uriPathComponentLength

When a user has entered a calendar url containing a double slash such as http://simbabwe.devel:8081//davical/caldav.php/fschlich/calendar/, function caldav_addTargetCalendarItem gets called with parameters aUri.path which does contain the double slash, and path which does *not* contain the double path. As a consequence, uriPathComponentLength is computed too big and locationPath is empty / this.mItemInfoCache[item.id].locationPath is set to an empty value. When calling function caldav_doDeleteItemOrUseCache on this item, eventUri.spec points to the entire calendar instead of just the one item, and hence the entire collection is lost.

This patch strips double slashes from aUri.path when calculating uriPathComponentLength, fixing the issue in calDavCalendar.js. I wonder if the amount of slashes in parameters 1 and 3 of caldav_addTargetCalendarItem differ on purpose, though...
Comment 4 fschlich 2012-11-02 08:25:40 PDT
while at it, a check should be added to doDeleteItemOrUseCache that errors out in case an eventUri is to be created from an empty this.mItemInfoCache[aItem.id].locationPath
Comment 5 fschlich 2012-11-13 02:07:58 PST
Created attachment 680984 [details] [diff] [review]
updated patch, including a check in doDeleteItem to prevent deletion of the entire collection

I have now found the time to code and test the additional check. I don't get the error message in the error popup details, though, it is instead printed to the shell from which I started thunderbird...
Comment 6 fschlich 2012-11-13 02:14:14 PST
justification for severity: can cause substantial data loss (accidental deletion of entire calendar collection)
Comment 7 Philipp Kewisch [:Fallen] 2012-11-29 09:16:09 PST
Comment on attachment 680984 [details] [diff] [review]
updated patch, including a check in doDeleteItem to prevent deletion of the entire collection

This looks like a good idea to me. Have you tested this patch with other servers? If we can get some broader testing soon, then we could possibly commit this patch also for Lightning 1.9.1.
Comment 8 Philipp Kewisch [:Fallen] 2012-11-29 09:25:57 PST
Pushed to comm-central changeset efd79c122d2f
Comment 9 Philipp Kewisch [:Fallen] 2012-11-29 09:26:16 PST
Backported to releases/comm-aurora changeset d2b1ce68bb9c
Comment 10 Philipp Kewisch [:Fallen] 2012-11-29 09:26:36 PST
Backported to releases/comm-beta changeset d05aaa70da3c
Comment 11 Matthew Mecca [:mmecca] 2013-02-07 20:05:33 PST
Comment on attachment 680984 [details] [diff] [review]
updated patch, including a check in doDeleteItem to prevent deletion of the entire collection

Has this been sufficiently tested to land in 1.9.1?
Comment 12 Philipp Kewisch [:Fallen] 2013-02-08 07:07:38 PST
Comment on attachment 680984 [details] [diff] [review]
updated patch, including a check in doDeleteItem to prevent deletion of the entire collection

I think this is safe for 1.9.1, I can't say much for testing, but the code looks clean.
Comment 13 Matthew Mecca [:mmecca] 2013-02-08 12:14:13 PST
comm-esr17 - https://hg.mozilla.org/releases/comm-esr17/rev/e31303ce7098

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