Closed Bug 334423 Opened 14 years ago Closed 13 years ago

calDAV provider leaks auth info into X-MOZ-LOCATIONURI

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: browning, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

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
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.
Attachment #218761 - Flags: first-review?(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.
Attachment #218761 - Flags: first-review?(dmose) → first-review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Landed; thanks for the patch!
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
reopening because previous patch breaks on oracle calDAV server, which urlencodes the path in question
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch urldecode pathSplinter Review
this patch (against current calendar) sets LOCATIONPATH correctly on both current version of cosmo and the oracle calDAV server.
Attachment #218761 - Attachment is obsolete: true
Attachment #219698 - Flags: first-review?(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.
Attachment #219698 - Flags: first-review?(dmose) → first-review+
Attachment #219698 - Attachment is patch: true
Attachment #219698 - Attachment mime type: text/x-patch → text/plain
Attachment #248801 - Flags: second-review?(dmose)
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.
Attachment #248801 - Flags: second-review?(dmose)
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
Attachment #248801 - Flags: first-review+
Patch (with spacing fixed) checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.