Closed Bug 492279 Opened 11 years ago Closed 10 years ago

Sort out cookie handling within Thunderbird

Categories

(Thunderbird :: Security, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [no l10n impact])

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #374578 +++

I'm spinning the cookie part of bug 374578 out to this bug. Thunderbird has several problems when it comes to cookies, and these hurt extensions as well as some RSS feeds that require logins.

Basic issues:

1) Thunderbird blocks anything non-RSS.
2) Thunderbird ignores the general cookie preferences
3) We don't pass the cookie tests because of 1).

The issues with the current cookie system are:

- Thunderbird overrides the core cookie policy because:
-- the core cookie code cannot be extended via components in the same way as the content policy can - this looks like bug 375442.
- Its not clear if the core (ifdefed) cookie policy will do the right thing for cookies in RSS feeds in mailnews.
This feed might do for a test case:
http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml

At least, it seems to be asking for a cookie

"In order to access our Web site, your Web browser must accept cookies from NYTimes.com."
Assignee: nobody → bugzilla
Flags: blocking-thunderbird3+
(In reply to comment #1)
> This feed might do for a test case:
> http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml
> 
> At least, it seems to be asking for a cookie

Thanks Joe, that's an excellent example - you don't need to log in, but you do need cookies to view the page!
Attached patch Revert TB's cookie policy (obsolete) — Splinter Review
This patch reverts TB's specific cookie policy override.
Depends on: 493546
Attached patch Revert TB's cookie policy v1 (obsolete) — Splinter Review
This patch drops TB's specific cookie policy in favour of the one in core.

Currently Thunderbird disables cookies for any content except:

- chrome based loads
- Loading of RSS message summary lists (not viewing of RSS messages).

In windows of any non-APP_TYPE_MAIL, this patch will*:

- Enables cookies per Firefox rules and default prefs (i.e. proper session expiry, 'ask for what to do' dialog enabled with this patch and the patch on bug 493546).
- Will still disable if someone tries to set cookies on an imap, news, snews, mailbox url.

In windows that are APP_TYPE_MAIL, this patch will*:

- change nothing, except to allow disabling of the mailnews cookie check completely (see below).

* In both of the cases above, setting network.cookie.disableCookieForMailNews (listed in about:config) to false will disable the mail specific cookie setting.

This patch also:

- Re-enables the cookie tests for Thunderbird that we had previously disabled due to our content policy.

Although this patch doesn't allow cookies in APP_TYPE_MAIL it is heading in the right direction, and I'm going to be filing a bug about fixing the core issue (which affects SeaMonkey as well) which is mailnews specific code in mozilla/extensions/cookie.

This patch doesn't address any UI for remove cookies etc - I'm planning on doing that within this bug starting with a proposal a bit later.

Dan if you want someone else to review as well, please feel free to redirect/request additional reviews.
Attachment #378018 - Attachment is obsolete: true
Attachment #378094 - Flags: superreview?(dmose)
Attachment #378094 - Flags: review?(dmose)
Updating depends/blocker lists to reflect reality and setting initial target of b3 as we have some patches and it'd be good to get them to extensions for testing - although we could slip this if wanted.
Blocks: 431125, 477120
No longer depends on: 375442, 431125, 477120
Target Milestone: --- → Thunderbird 3.0b3
Whiteboard: [has patch to paritally open cookies][needs review dmose]
Updated to fix bitrot.
Attachment #378094 - Attachment is obsolete: true
Attachment #382275 - Flags: superreview?(dmose)
Attachment #382275 - Flags: review?(dmose)
Attachment #378094 - Flags: superreview?(dmose)
Attachment #378094 - Flags: review?(dmose)
Whiteboard: [has patch to paritally open cookies][needs review dmose] → [has patch to partially open cookies][needs review dmose]
Blocks: 497564
(In reply to comment #4)
> In windows that are APP_TYPE_MAIL, this patch will*:
> 
> - change nothing, except to allow disabling of the mailnews cookie check
> completely (see below).
> 

Which means that cookies-in-RSS will break as soon as this patch lands, correct?

> Although this patch doesn't allow cookies in APP_TYPE_MAIL it is heading in the
> right direction, and I'm going to be filing a bug about fixing the core issue
> (which affects SeaMonkey as well) which is mailnews specific code in
> mozilla/extensions/cookie.

If you haven't already done this, please do so now, and mark it as blocking both Thunderbird 3 and this bug.
Or is it that cookies-in-RSS are already broken today?  Reading through nsCookiePermission::CanAccess makes me think that isMail would get set to true today, causing that method to bail with ACCESS_DENY.
Attachment #382275 - Flags: superreview?(dmose) → superreview?(bienvenu)
Comment on attachment 382275 [details] [diff] [review]
[checked in] Revert TB's cookie policy v2

Since this is security sensitive code, I think it makes sense to get two sets of eyes on it.
Whiteboard: [has patch to partially open cookies][needs review dmose] → [has patch to partially open cookies][needs input Standard8, review dmose, bienvenu]
Comment on attachment 382275 [details] [diff] [review]
[checked in] Revert TB's cookie policy v2

as I understand it, cookies in rss messages have never worked...but if this is a step along the way of making them work, then yea!
Attachment #382275 - Flags: superreview?(bienvenu) → superreview+
FYI in SeaMonkey you can already work around the core issue by setting the network.cookie.disableCookieForMailNews pref to false.
(In reply to comment #8)
> Or is it that cookies-in-RSS are already broken today?  Reading through
> nsCookiePermission::CanAccess makes me think that isMail would get set to true
> today, causing that method to bail with ACCESS_DENY.

If you subscribe http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml as an RSS feed in current Thunderbird trunk, with or without the patch, viewing one of its items as web page will say that you need to log in (for some weird reason it wants cookies otherwise it won't let you see the page).

If you subscribe the same into Thunderbird 2, then you get to see the page.

I think this difference in behaviour could be a regression from removing the extra generated rss iframe which means we no longer can detect it is an RSS feed item (although I haven't done any checking to verify that).

Also, in Thunderbird trunk:

- If you have an RSS feed that requires logging in to view some items (e.g. bugzilla) it seems to work that you can find out the item is there in the rss list with or without the patch, as long as you have a valid login cookie (e.g. copy cookie files). This is because loading the feed in the background skips the APP_TYPE_MAIL check because we're not loading it into the UI.

- Using Thunderbrowse I can log into bugzilla and navigate using its standalone window. Without the patch it doesn't retain the log in, with the patch it does.
Blocks: 501910
Depends on: 501925
(In reply to comment #7)
> (In reply to comment #4)
> > Although this patch doesn't allow cookies in APP_TYPE_MAIL it is heading in the
> > right direction, and I'm going to be filing a bug about fixing the core issue
> > (which affects SeaMonkey as well) which is mailnews specific code in
> > mozilla/extensions/cookie.
> 
> If you haven't already done this, please do so now, and mark it as blocking
> both Thunderbird 3 and this bug.

Bug 501925 raised for the core fix. Bug 501910 raised for Thunderbird UI.
Whiteboard: [has patch to partially open cookies][needs input Standard8, review dmose, bienvenu] → [has patch to partially open cookies][needs review dmose, bienvenu]
Whiteboard: [has patch to partially open cookies][needs review dmose, bienvenu] → [has patch to partially open cookies][needs review dmose]
OK, thanks.  Since this bug has security implications (i.e. it's changing the way our policy code works), I'm uncomfortable landing this without more assurance that we're making the changes we think we're making and that this won't regress in the future.  As a result, I'd like to ask for some mozmill tests before approving.  I apologize for not thinking of this earlier.
Whiteboard: [has patch to partially open cookies][needs review dmose] → [needs new patch with mozmill tests]
Attachment #382275 - Flags: review?(dmose) → review-
Comment on attachment 382275 [details] [diff] [review]
[checked in] Revert TB's cookie policy v2

Mark pointed out that there are actually mailnews-specific unit tests in the core cookie tests, of which I was unaware.  He also convinced me that actual functional tests can wait until bug 501925, which we need for Tb3 anyway.
Attachment #382275 - Flags: review- → review+
Whiteboard: [needs new patch with mozmill tests] → [needs landing]
Comment on attachment 382275 [details] [diff] [review]
[checked in] Revert TB's cookie policy v2

Checked in: http://hg.mozilla.org/comm-central/rev/13ff76efb6f5
Attachment #382275 - Attachment description: Revert TB's cookie policy v2 → [checked in] Revert TB's cookie policy v2
Now that patch is landed, we are in the situation that:

- Non-APP_TYPE_MAIL windows have cookies enabled. For TB on its own this basically equates to nothing, with Thunderbrowse opening new links in a window should enable cookies.
- TB will obey cookie prefs, although the prompt to ask will be broken until bug 493546 is landed on branch.
- Windows which are APP_TYPE_MAIL will not allow cookies unless network.cookie.disableCookieForMailNews is toggled in which case cookies would be allowed for mailnews protocols as well. Bug 501925 is looking at making the default case more flexible.
- We need to write mozmill tests to check cookie functionality is working as we expect (core tests check cookies denial on mailnews protocols).
Whiteboard: [needs landing] → [Non-mail windows fixed][needs tests][needs core bug 493546 and bug 501925]
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
So far so good
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1pre) Gecko/20090710 Shredder/3.0b3pre ID:20090710082549
Can read feed requiring cookies
(In reply to comment #17)
> - TB will obey cookie prefs, although the prompt to ask will be broken until
> bug 493546 is landed on branch.

Bug 493546 is landed on branch, therefore the cookie prompts should be working (if the prefs are set to require them).
Whiteboard: [Non-mail windows fixed][needs tests][needs core bug 493546 and bug 501925] → [Non-mail windows fixed][needs tests][needs core bug 501925]
I've set up a document on what Thunderbird now supports at https://developer.mozilla.org/en/Thunderbird/Cookies_In_Thunderbird
Attached patch Basic test (obsolete) — Splinter Review
This is a basic test which currently checks that cookies are not enabled in remote content (via contentTabs) in Thunderbird as is the current situation.

When bug 501925 lands we can tweak the test to check cookies are enabled.

I plan to add checks for remote content in emails, however this is a starter so we have at least one check.
Attachment #390293 - Flags: review?(dmose)
(In reply to comment #21)
> Created an attachment (id=390293) [details]
> Basic test

I forgot to say, the mozmilltests.list part of the patch will only apply if the patches from bug 500142 have also been applied.
Minor change to fix a couple of comments (supply bug numbers). Still need to add tests for remote content in messages.
Attachment #390293 - Attachment is obsolete: true
Attachment #393155 - Flags: review?(dmose)
Attachment #390293 - Flags: review?(dmose)
(In reply to comment #23)
> Created an attachment (id=393155) [details]
> Basic test v2
> 
> Minor change to fix a couple of comments (supply bug numbers). Still need to
> add tests for remote content in messages.

Now I think about this a bit more, I'm not sure it is possible to easily check for cookies in remote content in messages - javascript isn't enabled, and the fake http server doesn't implement server side cookies.

So I think we need to rely on the core cookie tests for the protocol enabling/disabling, and the mozmill tests we have here for cookies in remote content that isn't in messages.
Whiteboard: [Non-mail windows fixed][needs tests][needs core bug 501925] → [Non-mail windows fixed][tests need review][needs core bug 501925]
Whiteboard: [Non-mail windows fixed][tests need review][needs core bug 501925] → [Non-mail windows fixed][tests need review][needs core bug 501925][no l10n impact]
Attachment #393155 - Flags: review?(dmose) → review+
Comment on attachment 393155 [details] [diff] [review]
[checked in] Basic test v2

The HTML files would be easier to read if the tags were indented in the usual way.

>diff --git a/mail/test/mozmill/cookies/html/cookietest1.html b/mail/test/mozmill/cookies/html/cookietest1.html
>new file mode 100644
>--- /dev/null
>+++ b/mail/test/mozmill/cookies/html/cookietest1.html
>@@ -0,0 +1,15 @@
>+<html>
>+<head>
>+<title>Cookie Test</title> 
>+<script type="text/javascript">
>+<!--
>+document.cookie = "name=CookieTest";
>+// -->

Is the HTML comment glop actually required by Gecko these days?  It hurts legibility.

>+/**
>+ * Test deleting junk messages with no messages marked as junk.
>+ */
>+function test_load_cookie_page() {
>+  newTab = mc.tabmail.openTab("contentTab",
>+                              {contentPage: url + "cookietest1.html"});
>+
>+  if (!newTab)
>+    throw new Error("Expected new tab info to be returned from openTab");
>+
>+  // XXX When bug 508999 is fixed, remove the sleep and use the waitForEval
>+  // instead.
>+  // controller.waitForEval("subject.busy == false", 1000, 100, newTab);
>+  controller.sleep(1000);

Maybe use 5000 there and in the other sleep, just to give us extra padding for avoiding random Tinderbox oranges on heavily loaded machines?

>+function test_load_cookie_result_page() {

This test seems to be assuming that test_ functions are run in order of declaration.  Is that actually guaranteed?
(In reply to comment #24)

> Now I think about this a bit more, I'm not sure it is possible to easily check
> for cookies in remote content in messages - javascript isn't enabled, and the
> fake http server doesn't implement server side cookies.

True.  We could check for it in RSS "messages" now, though that doesn't seem critically important.

For other messages, how about filing a bug on the fake http server and either leaving this one with in-testsuite? and depending on that one, or filing a new bug for that test.
Whiteboard: [Non-mail windows fixed][tests need review][needs core bug 501925][no l10n impact] → [Non-mail windows fixed][needs core bug 501925][no l10n impact]
The core bug has now been checked in on trunk and will be in today's nightlies. I'll leave this bug open until we get it on branch as well.

Likewise I'll leave checking in the unit tests until we get the core patch on branch to avoid branch/trunk failures as we haven't branched yet.
Whiteboard: [Non-mail windows fixed][needs core bug 501925][no l10n impact] → [fixed on trunk][needs core bug 501925 on branch][no l10n impact]
This is now fixed on 1.9.1 and trunk. Bug 501925 will continue the tracking into 1.9.2. Therefore marking as fixed.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed on trunk][needs core bug 501925 on branch][no l10n impact] → [no l10n impact]
(In reply to comment #25)
> (From update of attachment 393155 [details] [diff] [review])
> The HTML files would be easier to read if the tags were indented in the usual
> way.

Fixed

> Is the HTML comment glop actually required by Gecko these days?  It hurts
> legibility.

Removed

> >+  // XXX When bug 508999 is fixed, remove the sleep and use the waitForEval
> >+  // instead.
> >+  // controller.waitForEval("subject.busy == false", 1000, 100, newTab);
> >+  controller.sleep(1000);
> 
> Maybe use 5000 there and in the other sleep, just to give us extra padding for
> avoiding random Tinderbox oranges on heavily loaded machines?

I'm actually using 400 on another content tab test and I've not seen any failures for that. I don't want to increase it because it will extend the length of time to run the complete tests for developers. As everything is local to the machine, loading a page shouldn't take long anyway.

> >+function test_load_cookie_result_page() {
> 
> This test seems to be assuming that test_ functions are run in order of
> declaration.  Is that actually guaranteed?

It isn't explicitly written down afaict, but https://developer.mozilla.org/en/Mozmill_Tests/Tutorial%3a_Introduction_to_Mozmill implies they are. If they aren't then I expect a lot of our tests will break.
Comment on attachment 393155 [details] [diff] [review]
[checked in] Basic test v2

Checked in: http://hg.mozilla.org/comm-central/rev/a3886c649402
Attachment #393155 - Attachment description: Basic test v2 → [checked in] Basic test v2
Setting in-testsuite? as I still need to file the bugs for comment 26.
Flags: in-testsuite?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.