334423 - calDAV provider leaks auth info into X-MOZ-LOCATIONURI

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Bruno Browning]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060313 Fedora/1.5.0.1-9 Firefox/1.5.0.1 pango-text
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060417 Mozilla Sunbird/0.3a1+

When a calDAV calendar is defined in the form http(s)://user:pass@host.... calendar inserts the URI, including the auth info, into an X-MOZ-LOCATIONURI property in events created in the calendar. 

Reproducible: Always

Steps to Reproduce:
1. Create CalDAV calendar using a spec with embedded auth info
2. Create an event in that calendar
3. Examine X-MOZ-LOCATIONURI property of event

Actual Results:  
X-MOZ-LOCATIONURI shows username, password to the calendar

Expected Results:  
Calendar should not expose user authentication info

Comment 1

[Bruno Browning]

Attachment: change X-MOZ-LOCATIONURI to X-MOZ-LOCATIONPATH

patch replaces X-MOZ-LOCATIONURI with X-MOZ-LOCATIONPATH. X-MOZ-LOCATIONPATH should be the portion of the item URI not already specified by the calendar .spec. This patch just uses item.id + ".ics" for new items but is marginally smarter in reportInternal.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 218761 [details] [diff] [review]
change X-MOZ-LOCATIONURI to X-MOZ-LOCATIONPATH

Looks good; thanks for reporting this and posting a patch so quickly!  I've got a couple of really minor nits, which I'll fix before checking in:

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v
>retrieving revision 1.55
>diff -p -u -r1.55 calDavCalendar.js
>--- calendar/providers/caldav/calDavCalendar.js	15 Apr 2006 13:31:18 -0000	1.55
>+++ calendar/providers/caldav/calDavCalendar.js	18 Apr 2006 02:28:12 -0000
>@@ -265,7 +265,7 @@ calDavCalendar.prototype = {
>   
>         aItem.calendar = this;
>         aItem.generation = 1;
>-        aItem.setProperty("X-MOZ-LOCATIONURI", itemUri.spec);
>+        aItem.setProperty("X-MOZ-LOCATIONPATH", aItem.id + ".ics");

Since |aItem.id + ".ics"| is already done earlier in the function, I'll hoist that definition out to a single "const" statement.

>@@ -557,8 +557,9 @@ calDavCalendar.prototype = {
>                 item.calendar = thisCalendar;
> 
>                 // save the location name in case we need to modify
>-                item.setProperty("X-MOZ-LOCATIONURI", aResource.spec);
>-                debug("X-MOZ-LOCATIONURI = " + aResource.spec + "\n");
>+                var localPath = aResource.path.substr(thisCalendar.mCalendarUri.path.length);

I think it makes a bit more sense to call this variable "locationPath", since it's not really "local" per se.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Landed; thanks for the patch!

Comment 4

[Bruno Browning]

reopening because previous patch breaks on oracle calDAV server, which urlencodes the path in question

Comment 5

[Bruno Browning]

Attachment: urldecode path

this patch (against current calendar) sets LOCATIONPATH correctly on both current version of cosmo and the oracle calDAV server.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 219698 [details] [diff] [review]
urldecode path

Thanks for the patch; r=dmose.  I've landed the patch, but I'm leaving the bug open, as I think this needs one more tweak: we have no guarantee that the user created this calendar with an unescaped URL.  So I think we need to create a guaranteed-unescaped version in the setter that we keep around and then use to do this comparison.

Comment 7

[Bruno Browning]

Attachment: urldecoded mLocationPath

Comment 8

[Simon Paquet [:sipaq]]

Comment on attachment 248801 [details] [diff] [review]
urldecoded mLocationPath

This does not need a 2nd review anymore, since the former module owner (dmose) already gave his review.

Comment 9

[Matthew (lilmatt) Willis]

Comment on attachment 248801 [details] [diff] [review]
urldecoded mLocationPath

Actually, this is a separate patch that _does_ need review.

r=lilmatt with the }, line indented properly

Comment 10

[Matthew (lilmatt) Willis]

Patch (with spacing fixed) checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED