Redirects cause dialog asking user if he wants to post his data again

RESOLVED FIXED in 3.9

Status

Calendar
Provider: CalDAV
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Lightning 1.7

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Using a kerio caldav server, a redirect warning shows up on every add/modify/delete etc when using a "wrong" url:

https://iop.test.kerio.com//full-calendars/iop.test.kerio.com/user1/b07a9efb-0a96-4512-b23c-10b04724a96e

Note the two // before full-calendars. This causes kerio to do a redirect, probably to https://iop.test.kerio.com/full-calendars/iop.test.kerio.com/user1/b07a9efb-0a96-4512-b23c-10b04724a96e

Lightning shows a dialog box asking if the user wants to post his data again.

This should be fixed client side to not show that dialog, or even update the URL the calendar is saved with.
(Assignee)

Updated

4 years ago
Whiteboard: [CalDAV server: Kerio][calconnect25-newissues] → [CalDAV server: Kerio][calconnect31]
(Assignee)

Comment 1

3 years ago
Created attachment 8497831 [details] [diff] [review]
Fix - v1

This patch takes care by changing the URL of the calendar during the initial propfind. I decided not to add UI for this since its really a technical detail and the browser also doesn't ask the user when doing a redirect, but there could of course be potential security issues here. I think its fine, if you have a moment please think about it and tell me if you agree.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #8497831 - Flags: review?(matthew.mecca)
(Assignee)

Updated

3 years ago
Whiteboard: [CalDAV server: Kerio][calconnect31] → [CalDAV server: Kerio][calconnect31-haspatch]
(Assignee)

Updated

3 years ago
Duplicate of this bug: 809454
Comment on attachment 8497831 [details] [diff] [review]
Fix - v1

Review of attachment 8497831 [details] [diff] [review]:
-----------------------------------------------------------------

Untested but r=mmecca codewise.

The browser doesn't notify on a redirect, but it also doesn't modify a stored url like a bookmark. Maybe a one-time notification before permanently updating the calendar location would be appropriate? I'm not sure how much of a security risk there really is though.
Attachment #8497831 - Flags: review?(matthew.mecca) → review+
Is this patch ready for checked in?
Flags: needinfo?(philipp)
(Assignee)

Comment 5

3 years ago
Comment on attachment 8497831 [details] [diff] [review]
Fix - v1

I think I'll go with the one-time notification Matt suggested before pushing this one. Keeping needinfo so I don't forget.
Attachment #8497831 - Flags: review+
(Assignee)

Comment 6

3 years ago
Created attachment 8525995 [details] [diff] [review]
Fix - v2

Ok here is a patch that opens a dialog. I decided to make the "No" option disable the calendar, otherwise there might be too many dialogs. On the other hand, if someone is at an airport that decides to do 301 redirects, he will have to disable his calendars to not change the URL. What do you think?
Attachment #8497831 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8525995 - Flags: review?(matthew.mecca)
(In reply to Philipp Kewisch [:Fallen] from comment #6)
> Created attachment 8525995 [details] [diff] [review]
> Fix - v2
> 
> Ok here is a patch that opens a dialog. I decided to make the "No" option
> disable the calendar, otherwise there might be too many dialogs. On the
> other hand, if someone is at an airport that decides to do 301 redirects, he
> will have to disable his calendars to not change the URL. What do you think?

Does that actually happen? I wouldn't think that a 301 response by a public router would be a common case, so I don't think disabling the calendar here would be a big problem.
Attachment #8525995 - Flags: review?(matthew.mecca) → review+
(Assignee)

Comment 8

3 years ago
I'm not really sure, but given most servers are https and there should be a security error first, I also think its ok. Lets stay with the patch as is.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/be0bbb3b6d7c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.9
You need to log in before you can comment on or make changes to this bug.