Allow 3rd party cookies for session only

RESOLVED FIXED

Status

()

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

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [evang-wanted])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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.
Whiteboard: [evang-wanted]
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 565965
(Assignee)

Updated

7 years ago
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
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.
Attachment #446308 - Flags: review?(sdwilsh)
(Assignee)

Comment 5

7 years ago
Created attachment 446569 [details] [diff] [review]
patch v1.1

Fixed missing conditional.
Attachment #446308 - Attachment is obsolete: true
Attachment #446569 - Flags: review?(sdwilsh)
Attachment #446308 - Flags: review?(sdwilsh)
(Assignee)

Comment 6

7 years ago
Comment on attachment 446569 [details] [diff] [review]
patch v1.1

Nevermind, this fails mochi.
Attachment #446569 - Flags: review?(sdwilsh)
(Assignee)

Comment 7

7 years ago
Created attachment 446781 [details] [diff] [review]
patch v1.2

Now with mochi unfail.
Attachment #446569 - Attachment is obsolete: true
Attachment #446781 - Flags: review?(sdwilsh)
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.
(Assignee)

Comment 9

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

Updated

7 years ago
Attachment #446781 - Flags: review?(sdwilsh) → review+
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.
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Comment 12

7 years ago
Landed.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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:
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.
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.
(Assignee)

Updated

7 years ago
Blocks: 570630

Comment 16

7 years ago
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

7 years ago
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
(Assignee)

Comment 18

7 years ago
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.
Keywords: dev-doc-needed
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?
(Assignee)

Comment 20

7 years ago
I wouldn't document anything yet. This isn't really exposed to anyone who doesn't go hunting for hidden prefs.
I'm removing the doc needed flag from this; any follow-up bug that will impact docs should get it added.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.