Last Comment Bug 565475 - Allow 3rd party cookies for session only
: Allow 3rd party cookies for session only
Status: RESOLVED FIXED
[evang-wanted]
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: dwitte@gmail.com
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: doublekey 570630
  Show dependency treegraph
 
Reported: 2010-05-12 15:12 PDT by dwitte@gmail.com
Modified: 2010-12-08 06:47 PST (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (21.54 KB, patch)
2010-05-19 14:26 PDT, dwitte@gmail.com
no flags Details | Diff | Splinter Review
patch v1.1 (21.56 KB, patch)
2010-05-20 14:07 PDT, dwitte@gmail.com
no flags Details | Diff | Splinter Review
patch v1.2 (21.60 KB, patch)
2010-05-21 12:53 PDT, dwitte@gmail.com
sdwilsh: review+
Details | Diff | Splinter Review
tests (11.08 KB, patch)
2010-05-27 12:57 PDT, dwitte@gmail.com
no flags Details | Diff | Splinter Review

Description dwitte@gmail.com 2010-05-12 15:12:09 PDT
Like the title says: make 3rd party cookies persistent only for the session. By default.

The 'accept third party cookies' option would change meaning to be this new definition; so unchecking it would disable all third party cookies, like before.

We should implement this as a tristate pref: 0 = enable persistent third party cookies; 1 = for session only; 2 = disable entirely.
Comment 1 dwitte@gmail.com 2010-05-12 15:32:06 PDT
Also need to check what Safari does here. We might be able to disable setting of 3rd party cookies by default, though I can't really see that flying. Safari might have some tweaks I don't know about.
Comment 2 Daniel Veditz [:dveditz] 2010-05-17 19:42:29 PDT
Would state 0 mean "mingle first and third party cookies" as we currently do, or would it create a new state where 3rd party cookies are segregated by top-level site and simply made permanent?

If you add a new pref you'd have to also add migration code to convert people who currently disable 3rd party cookies to the new pref with the "2" state. We don't want privacy-conscious users accusing us of doing a Facebook.
Comment 3 dwitte@gmail.com 2010-05-18 12:56:40 PDT
For this bug, it would mean 'do what we currently do'. Changing the keying can happen in bug 565965. Migration code is a given.

How to implement the '1' state is a bit trickier. We have two choices:

a) Have a totally separate bucket for 3rd party cookies, where they are stored/read from. (So this is sorta changing how cookies are keyed, by having general third-party-ness part of the key.)

b) Have them mix in with 1st party cookies, but just degrade their lifetime to session when they're set.

I'm gonna do b), since it avoids writing something that we're just gonna rework in bug 565965.
Comment 4 dwitte@gmail.com 2010-05-19 14:26:46 PDT
Created attachment 446308 [details] [diff] [review]
patch v1

Downgrades third party cookies to session, by default.

I went with not changing existing pref names or values, since dealing with pref migration is a pain. Which means no changes at all are required to the pref panel -- we just sneakily use the new session-only default.
Comment 5 dwitte@gmail.com 2010-05-20 14:07:49 PDT
Created attachment 446569 [details] [diff] [review]
patch v1.1

Fixed missing conditional.
Comment 6 dwitte@gmail.com 2010-05-21 11:27:38 PDT
Comment on attachment 446569 [details] [diff] [review]
patch v1.1

Nevermind, this fails mochi.
Comment 7 dwitte@gmail.com 2010-05-21 12:53:33 PDT
Created attachment 446781 [details] [diff] [review]
patch v1.2

Now with mochi unfail.
Comment 8 Daniel Veditz [:dveditz] 2010-05-21 13:59:47 PDT
What about cookies that are used as both 1st and 3rd party cookies? This looks like it will convert them to session, so next time you restart Firefox GMail/Yahoo/whoever no longer knows who you are.

I guess it only converts when cookies are set, so maybe in those more legitimate cases the cookies won't actually be converted.
Comment 9 dwitte@gmail.com 2010-05-21 14:02:03 PDT
Yeah. That's the downside of not having a separate bucket (yet). If you login as a first party, then later as a third, they get dumped.

Not sure how big of a problem this'll be. In any case, we should follow up fast with bug 565965.
Comment 10 Shawn Wilsher :sdwilsh 2010-05-24 16:00:21 PDT
Comment on attachment 446781 [details] [diff] [review]
patch v1.2

>+++ b/browser/app/profile/firefox.js
>+pref("network.cookie.thirdparty.sessionOnly", true);
You don't need this since you have this in all.js, right? (This really applies to all the cookie prefs I think).

You should also be able to test this with a chrome mochitest and shutting down and then restarting the cookie service and making sure the cookie no longer exists.  May even be able to do this with xpcshell.

r=sdwilsh with a test.
Comment 11 dwitte@gmail.com 2010-05-27 12:57:45 PDT
Created attachment 447810 [details] [diff] [review]
tests

Adds infrastructure for (faking of) reloading a profile from xpcshell, and adds tests for 1) general cookie persistence; 2) third party cookies in general (specifically, testing the nsIHttpChannelInternal.forceAllowThirdPartyCookies flag); 3) third party cookie persistence (this bug).

Will land all this together.
Comment 12 dwitte@gmail.com 2010-05-28 14:57:46 PDT
Landed.
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2010-05-28 15:53:51 PDT
For posterity... when this landed the first run had the following failure: moz2-linux-slave20
TEST-UNEXPECTED-FAIL | /builds/slave/mozilla-central-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_thirdparty_session.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/mozilla-central-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_thirdparty_session.js | 0 == 4 - See following stack:

The next run had two failures:
s: moz2-linux-slave19
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_persistence.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_persistence.js | 0 == 4 - See following stack:
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_thirdparty_session.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_thirdparty_session.js | 0 == 4 - See following stack:

and

s: moz2-linux-slave13
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_thirdparty_session.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-central-linux-debug-unittest-xpcshell/build/xpcshell/tests/test_cookies/unit/test_cookies_thirdparty_session.js | 0 == 4 - See following stack:
Comment 14 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-06-04 19:34:48 PDT
This is a pretty big change in how our app behaves with respect to various embedded-content scenarios.  Was the rationale for it discussed somewhere in more detail than the simple assertion in comment 0 that we should do this?  I certainly missed it, but I would love to catch up.  If there wasn't a discussion of it, I'm concerned that we pushed not only the new capability but also the new default without one.  Interaction of auth, user state and such are pretty important for a lot of existing sites and behaviours, and we shouldn't be making changes to them without a fair bit of discussion, IMO.
Comment 15 Asbjørn Riis-Knudsen 2010-06-07 09:05:03 PDT
Why was this changed with no indication in the UI? This breaks many things. I noticed that LastPass now no longer remembers login state across sessions and I get logged out of Facebook even though I have it set to stay logged in. At the very least make it an option in the UI and not something to go hunting for in about:config. But, really, this needs to be backed out. Why should this even be changed? I don't see a reason.
Comment 16 Brendan Butterworth 2010-06-09 12:52:27 PDT
Restating to confirm understanding.  The states 0/1/2 seem to map to the following, existing preferences (tools-options-privacy):

State 0 - Accept third-party cookies / Keep until they expire
State 1 - Accept third-party cookies / Keep until I close Firefox
State 2 - Do not accept third-party cookies

This bug seems to indicate two changes:

Change 1 - Make State 1 the default state (current default is State 0)
Change 2 - Change the treatment of 3rd party "session" cookies.  

I'm not exactly clear on Change 2, but I'd expect that there are three ways of implementing this:

Method 1 - Last state.  If the cookie was last used as a 3rd party cookie, it's considered a 3rd party cookie.  If it was last used as a 1st party cookie, it's considered a 1st party cookie.  
Method 2 - 1st Party Promotion.  Any 3rd party cookie ever used as a 1st party cookie is promoted to being a 1st party cookie.  
Method 3 - 3rd Party Demotion.  Any 1st party cookie ever used as a 3rd party cookie is demoted to being a 3rd party cookie.  

Use would have to be evaluated as to whether it's "sent" or "set".  I expect that Method 3 is in use.

Care to elucidate all readers on the accuracy of my understanding?
Comment 17 Jian Shen 2010-06-09 16:31:23 PDT
I'd also love to get confirmation on the new behavior as Brendan as stated.

I've also commented on how Bug 565965 has adverse effects on our services. https://bugzilla.mozilla.org/show_bug.cgi?id=565965#c15
Comment 18 dwitte@gmail.com 2010-06-10 14:57:07 PDT
This has been preffed off by default (bug 570630) pending more study, and ultimately, a solution that has been well tested and understood and can be introduced in a less piecemeal way.

To be clear, and to answer comment 16 -- the "Keep until" dropdown is unrelated to third party cookies. (This could be better indicated in the UI!)

What the new pref, "network.cookie.thirdparty.sessionOnly", does is restrict third party cookies to the session only. Of course, this only has any effect if third party cookies are enabled to begin with ("Accept third party cookies").

If you want to test this on your own, you may flip the pref in about:config -- but be warned, this is not representative of whatever solution we end up implementing, so test at your own risk.
Comment 19 Eric Shepherd [:sheppy] 2010-08-13 11:02:50 PDT
I'm unclear on what needs documenting here. After reading the comments, it sounds like the standard behavior has returned to what it was originally. Is all I need to document here the new network.cookie.thirdparty.sessionOnly pref?
Comment 20 dwitte@gmail.com 2010-08-13 11:04:59 PDT
I wouldn't document anything yet. This isn't really exposed to anyone who doesn't go hunting for hidden prefs.
Comment 21 Eric Shepherd [:sheppy] 2010-08-13 11:39:45 PDT
I'm removing the doc needed flag from this; any follow-up bug that will impact docs should get it added.

Note You need to log in before you can comment on or make changes to this bug.