Malformed URL can cause Lightning to delete complete calendar file (CalDAV)

RESOLVED FIXED in 1.9.1

Status

Calendar
Provider: CalDAV
P1
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: thorsten_h@ymail.com, Assigned: fschlich)

Tracking

Lightning 1.8
1.9.1

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
OS: All → Windows 7
Hardware: All → x86_64
(Assignee)

Comment 1

5 years ago
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"
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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...
Attachment #674598 - Flags: review?(mozilla)
Assignee: nobody → fschlich
Attachment #674598 - Flags: review?(mozilla) → review?(philipp)
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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...
Attachment #674598 - Attachment is obsolete: true
Attachment #674598 - Flags: review?(philipp)
Attachment #680984 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Severity: normal → critical
OS: Windows 7 → All
Priority: -- → P1
Hardware: x86_64 → All
Version: Lightning 1.4 → Lightning 1.8
(Assignee)

Comment 6

5 years ago
justification for severity: can cause substantial data loss (accidental deletion of entire calendar collection)
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.
Attachment #680984 - Flags: review?(philipp)
Attachment #680984 - Flags: review+
Attachment #680984 - Flags: approval-calendar-beta+
Attachment #680984 - Flags: approval-calendar-aurora+
Pushed to comm-central changeset efd79c122d2f
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2
Backported to releases/comm-aurora changeset d2b1ce68bb9c
Target Milestone: 2.2 → 2.1
Backported to releases/comm-beta changeset d05aaa70da3c
Target Milestone: 2.1 → 2.0
Whiteboard: [wanted-1.9.x]
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?
Attachment #680984 - Flags: approval-calendar-release?(philipp)
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.
Attachment #680984 - Flags: approval-calendar-release?(philipp) → approval-calendar-release+
comm-esr17 - https://hg.mozilla.org/releases/comm-esr17/rev/e31303ce7098
Target Milestone: 2.0 → 1.9.1
Whiteboard: [wanted-1.9.x]
You need to log in before you can comment on or make changes to this bug.