501925 - Change cookie policy to relax the mailnews-specific APP_TYPE_MAIL requirement

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: 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.

Comment 2

[[e-mail]]

Comment on attachment 386846 [details] [diff] [review]
The fix

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

Comment 3

[[e-mail]]

Comment on attachment 386846 [details] [diff] [review]
The fix

sure! r=me

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

 
> - 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.

Comment 5

[Mark Banner (:standard8)]

(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.

Comment 6

[Mark Banner (:standard8)]

Attachment: 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.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

(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."

Comment 9

[Mark Banner (:standard8)]

Checked in on trunk: http://hg.mozilla.org/mozilla-central/rev/7f1305cd07de

I'll let this bake for a day before requesting approvals.

Comment 10

[Mark Banner (:standard8)]

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.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 396978 [details] [diff] [review]
Also remove disableCookieForMailNews pref

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 12

[Mark Banner (:standard8)]

Fixed on 1.9.1.4: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a6df0cf1f8f3

Comment 13

[Mark Banner (:standard8)]

Fixed on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/00e303b6dad5

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Is there a mechanism to manually test this fix for verification?

Comment 15

[Eric Shepherd [:sheppy]]

This appears to be documented here:

https://developer.mozilla.org/en/Thunderbird/Cookies_In_Thunderbird