Change cookie policy to relax the mailnews-specific APP_TYPE_MAIL requirement

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Networking: Cookies
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({dev-doc-complete, relnote})

unspecified
mozilla1.9.3a1
dev-doc-complete, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 .4-fixed)

Details

(Whiteboard: [tb3needed][no l10n impact])

Attachments

(1 attachment, 1 obsolete attachment)

In the current cookie policy [1] we have a mailnews specific check (ifdefed) in nsCookiePermission::CanAccess that will deny cookies when:

a) The originating URI is a mailnews url.
b) The load context is within a doc shell that is of type APP_TYPE_MAIL.

For Thunderbird (and SeaMonkey, but not as much) this is a real issue for extensions, especially now we have the possibility to load remote content in tabs, or for example, enabling logging into bugzilla within Thunderbird and hence rss feeds could then be read properly (i.e. include the secure bugs that you have to log in to be able to read).

The ideal solution to this bug, would be to fix the core issue (e.g. bug 375442) - allowing extensions/non-Firefox applications to alter the cookie policy in potentially a similar way to how the content policy works.

However, that sort of change can now only happen in 1.9.2 and we'd like to open up the cookie policy for TB 3. As this code is ifdef mailnews, it won't affect Firefox, so we can probably still do it in 1.9.1 if we agree a solution.


My proposal is that we simply drop the APP_TYPE_MAIL check and rely on the URI scheme. Here's the reasoning and analysis of effects:

- Dropping APP_TYPE_MAIL would allow any non mailnews url sourced item to have cookies allowed (subject to the remaining core cookie policy checks) in mail type windows. Good for browser elements containing remote content e.g. extensions.

- Remote content in emails would have cookies enabled. I think it is not a significant issue to allow remote content with cookies in emails because 1) the content policy denies remote content in emails by default, the user has to specially enable them. 2) if the user is that worried about cookies they can alter the preferences to deny them.

- Viewing RSS feed items as web pages would allow cookies (good for viewing bugzilla items).

I have a patch in progress for this that I'll hopefully attach later today.
Whiteboard: [tb3needs]
Created attachment 386846 [details] [diff] [review]
The fix

As mentioned previously this patch only affects TB & SM (apart from some header include removal that FF doesn't need).

Implemented as I suggested in comment 0. Requesting reviews from module owner and TB & SM devs.

I'm looking to get this in for Thunderbird before our beta 4.
Attachment #386846 - Flags: superreview?(dmose)
Attachment #386846 - Flags: review?(neil)
Attachment #386846 - Flags: review?(dwitte)

Updated

8 years ago
Attachment #386846 - Flags: review?(neil) → review+

Comment 2

8 years ago
Comment on attachment 386846 [details] [diff] [review]
The fix

Great! Now I can re-disable cookies in mail ;-)

Updated

8 years ago
Keywords: dev-doc-needed
Whiteboard: [tb3needs] → [tb3needs][needs review dwitte,dmose]
Blocks: 477120

Updated

8 years ago
Attachment #386846 - Flags: review?(dwitte) → review+

Comment 3

8 years ago
Comment on attachment 386846 [details] [diff] [review]
The fix

sure! r=me
Whiteboard: [tb3needs][needs review dwitte,dmose] → [tb3needs][needs review dmose]

Updated

8 years ago
Whiteboard: [tb3needs][needs review dmose] → [tb3needs][needs review dmose][no l10n impact]
 
> - Remote content in emails would have cookies enabled. I think it is not a
> significant issue to allow remote content with cookies in emails because 1) the
> content policy denies remote content in emails by default, the user has to
> specially enable them. 2) if the user is that worried about cookies they can
> alter the preferences to deny them.

Which preference do you mean? 

As written, this patch leaves the tree in a state where there's a preference named network.cookie.disableCookieForMailNews that defaults to "true", but actually the actual effect of it would be better characterized as "sometimes".  I think this setting people who see that preference up to do something they don't intend.

Updated

8 years ago
Attachment #386846 - Flags: superreview?(dmose) → superreview-
(In reply to comment #4)
> > - Remote content in emails would have cookies enabled. I think it is not a
> > significant issue to allow remote content with cookies in emails because 1) the
> > content policy denies remote content in emails by default, the user has to
> > specially enable them. 2) if the user is that worried about cookies they can
> > alter the preferences to deny them.
> 
> Which preference do you mean?

They could use

pref("network.cookie.cookieBehavior", 0); // 0-Accept, 1-dontAcceptForeign, 2-dontUse

and/or

pref("network.cookie.lifetimePolicy", 0); // accept normally, 1-askBeforeAccepting, 2-acceptForSession,3-acceptForNDays

> As written, this patch leaves the tree in a state where there's a preference
> named network.cookie.disableCookieForMailNews that defaults to "true", but
> actually the actual effect of it would be better characterized as "sometimes". 
> I think this setting people who see that preference up to do something they
> don't intend.

With this patch, that pref would be "disable cookies on mailnews protocols" (i.e. not remove content, just on imap:// etc).

afaik cookies don't really exist on those protocols anyway, so maybe we should now just remove that pref.
Created attachment 396978 [details] [diff] [review]
Also remove disableCookieForMailNews pref

Given I knew it wouldn't take long, here's a patch with network.cookie.disableCookieForMailNews removed based on my previous comment.
Attachment #396978 - Flags: superreview?(dmose)

Updated

8 years ago
Attachment #386846 - Attachment is obsolete: true
Comment on attachment 396978 [details] [diff] [review]
Also remove disableCookieForMailNews pref

Since JavaScript is now always turned off in messages, cookies are, at best, minimally usable in messages.  

The one slightly odd case that I can think of is this:

* a web site shown by the RSS code or by an extension sets a cookie
* the user subsequently receives an email which they agree to trust remote content from (or is from a sender whom they've previously agreed to trust all messages from).
* remote content in the email will send the cookie from the previous browsing session back to the server, allowing correlation previously unavailable between web pages

I think we should at least discuss this in the Tb3 sec review meeting (which I hope to plan/propose tomorrow), as well as considering making third-party cookies off by default in Thunderbird (which is different from Firefox).

All that said, this feels to me like a reasonable change to make.  Please update <https://developer.mozilla.org/en/Thunderbird/Cookies_In_Thunderbird> when you makes this change.  Adding the relnote keyword to this bug, as I think the relnotes want to link to an updated version of that page.
Attachment #396978 - Flags: superreview?(dmose) → superreview+

Updated

8 years ago
Keywords: relnote
Whiteboard: [tb3needs][needs review dmose][no l10n impact] → [tb3needs][no l10n impact]
(In reply to comment #7)
> * remote content in the email will send the cookie from the previous browsing
> session back to the server, allowing correlation previously unavailable between
> web pages

That should have read "between web page views and message views."
Checked in on trunk: http://hg.mozilla.org/mozilla-central/rev/7f1305cd07de

I'll let this bake for a day before requesting approvals.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [tb3needs][no l10n impact] → [tb3needs][no l10n impact][baking on trunk]
Target Milestone: --- → mozilla1.9.3a1
Attachment #396978 - Flags: approval1.9.2?
Attachment #396978 - Flags: approval1.9.1.4?
Comment on attachment 396978 [details] [diff] [review]
Also remove disableCookieForMailNews pref

Requesting permission to land this on the 1.9.1 and 1.9.2 branches.

The main part of these changes are ifdef MOZ_MAIL_NEWS and hence won't affect Firefox - there are two header include removals that aren't ifdef MOZ_MAIL_NEWS as well as a pref removal, hence requesting approvals.

Risk to Firefox will be minimal.

This is a very much needed bug to allow Thunderbird to have a sane cookie policy and allow extensions to work better and do new things within Thunderbird.
Whiteboard: [tb3needs][no l10n impact][baking on trunk] → [tb3needs][no l10n impact][awaiting branch approval]
Comment on attachment 396978 [details] [diff] [review]
Also remove disableCookieForMailNews pref

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #396978 - Flags: approval1.9.1.4? → approval1.9.1.4+
Fixed on 1.9.1.4: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a6df0cf1f8f3
status1.9.1: --- → .4-fixed
Whiteboard: [tb3needs][no l10n impact][awaiting branch approval] → [tb3needs][no l10n impact][fixed 191, awaiting 192 approval]
Whiteboard: [tb3needs][no l10n impact][fixed 191, awaiting 192 approval] → [tb3needed][no l10n impact][fixed 191, awaiting 192 approval]
Attachment #396978 - Flags: approval1.9.2? → approval1.9.2+
Fixed on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/00e303b6dad5
status1.9.2: --- → beta1-fixed
Whiteboard: [tb3needed][no l10n impact][fixed 191, awaiting 192 approval] → [tb3needed][no l10n impact]
Is there a mechanism to manually test this fix for verification?
This appears to be documented here:

https://developer.mozilla.org/en/Thunderbird/Cookies_In_Thunderbird
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.