770691 - Update nsCookiePermission to enable 'per site' 3rd party cookie blocking

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement)

Description

[David Dahl :ddahl]

Just one of several bugs to enable per site 3rd party cookie blocking.

We will need to add nsICookiePermission::ALLOW_THIRD_PARTY, DENY_THIRD_PARTY as well as update nsCookiePermission::CanSetCookie, CanAccess to check first for specific site 3rd party cookie block or allow rules.

Comment 1

[David Dahl :ddahl]

See: https://wiki.mozilla.org/Privacy/Features/Per-Site_Third-Party_Cookie_Setting

Comment 2

[David Dahl :ddahl]

We may also have to update nsCookieService::CheckPrefs where it calls CanAccess to pass aChannel instead of nsnull

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

- Added ALLOW_FIRST_PARTY_ONLY to nsCookiePermission.idl
- Removed RequireThirdPartyCheck from nsCookieService, since we always require it now
- Added isThirdParty checks where required in nsCookiePermission::CanAccess
- updated test_cookies_thirdparty.js

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Tests pass locally, still waiting on try. I should modify test_cookies_thirdparty_session as well.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

I see I left some debug log statements in, sorry about that.

Comment 6

[Jason Duell]

Comment on attachment 650338 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

Review of attachment 650338 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good to me--I mainly looked at the necko parts, but the basic idea seems fine, with one question mark about how it'll work in B2G (see comment below).

I'm in the middle of ripping nsCookieService into little bits to get it working with B2G in bug 756648.  Please don't land this before that lands.

::: extensions/cookie/nsCookiePermission.cpp
@@ +6,5 @@
>  
>  
> +#ifdef MOZ_LOGGING
> +// this next define has to appear before the include of prlog.h
> +#define FORCE_PR_LOG // Allow logging in the release build

Note this this defines FORCE_PR_LOG to "// Allow logging..."  It'll work, but it's unintuitive.

Probably better to leave rest of line empty :)

::: netwerk/cookie/nsCookieService.cpp
@@ +3069,4 @@
>      nsCookieAccess access;
>      // Not passing an nsIChannel here is probably OK; our implementation
>      // doesn't do anything with it anyway.
>      rv = mPermissionService->CanAccess(aHostURI, nullptr, &access);

I suspect that we'll need to start passing in a channel (or nsILoadContext, or perhaps just an AppId/InBrowserElement tuple: API is not clear yet) here to make this work with B2G, unless we want to allow users to do per-site 3rd party blocking only on a phone-wide basis (i.e blocking a site blocks it for all apps).  It's probably nice to offer phone-wide blocking, but we may also want per-app settings.  Find out whether we're planning to make the permission manager app-savvy.

::: netwerk/cookie/nsICookiePermission.idl
@@ +21,5 @@
>     */
>    const nsCookieAccess ACCESS_DEFAULT = 0;
> +  const nsCookieAccess ACCESS_ALLOW = 1;
> +  const nsCookieAccess ACCESS_DENY = 2;
> +  const nsCookieAccess ACCESS_ALLOW_FIRST_PARTY_ONLY = 3;

Keep constants indented to same level?  It looks prettier.

Comment 7

[Jason Duell]

Hmm, I just noticed bug 565965 (for which someone's just updated the patch).  Is it still relevant?   Have we run the policy for this bug by dwitte (cookie module owner) to see whether he likes this policy more than that one (note: I haven't looked at it closely).  dwitte is best reached by personal email, BTW.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

bug 565965 works differently (I think it has different use cases) and can work in parallel, or even in cooperation with this feature.  Double-keying changes the client-side behavior of cookies, so it could introduce some unexpected behavior to web sites.  This just turns on/off cookies and has potential to be less breaky.

I'll ping dwitte for feedback, but he's really bogged down in other things and I've been trying to avoid bothering him too much.

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Jason,

Thanks for the review! Who would know if there are plans to make the permission manager app-savvy?

Thanks,
Monica

Comment 10

[Jason Duell]

Monica: hop onto #b2g on IRC and ask cjones, mounir, and/or fabrice--they should know, or know who knows...

Comment 11

[David Dahl :ddahl]

Comment on attachment 650338 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

sid and others in neckoland are much better reviewers for the code in cookie/

Comment 12

[David Dahl :ddahl]

(In reply to Monica Chew from comment #9)
> Hi Jason,
> 
> Thanks for the review! Who would know if there are plans to make the
> permission manager app-savvy?
> 
> Thanks,
> Monica

That's funny - we are doing that right now...

see bug 770731 and related bugs

Comment 13

[Jason Duell]

this page provides some background for the security model we need to follow here:

  https://wiki.mozilla.org/Apps/SecurityDetails#Sandboxed_Permissions
  
So yes, it looks like for B2G we'll need a way to identify which app is being used, so we get the right permissions for it.

Comment 14

[Jason Duell]

re: comment 12 and 13: so the cookie service mPermissionService is not a nsIPermissionManager.idl, it's a nsICookiePermission.  So work on the former is not relevant. 

I suspect we shouldn't let making nsICookiePermission know about appIds block this bug.  If/when B2G supports per site 3rd party blocking, it will initially be a global setting that affects all apps and browsers on the phone.  If we need to fix that, we can fix it later.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 650338 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

Review of attachment 650338 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.  One question lingering in my head (that may not be necessary for this bug, maybe a follow-up): do we want to support "first-party only session" as an option?  about:permissions lets you say "Allow", "Block" and "Session", which makes me think "First party Session-Only" is useful -- but maybe not.

I can't r+ this since I'm not a peer, but this looks good so f=me.  Mike: since mmc is hacking an idl, can you sr this?

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

Hmm, if we go this route it may make sense to keep SESSION_ONLY as a separate permission to avoid state blowup -- unless ALLOW_FIRST_PARTY_SESSION_ONLY is the last last cookie permission, ever :) Based on recent conversations I have a feeling that ALLOW_SAME_CATEGORY_ONLY (for 3rd party cookies between domains that are the same type of site) might also be useful.

Of course orthogonalizing SESSION_ONLY would mean that things like allowing 3rd party session only cookies would be possible, which probably doesn't make sense.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 650338 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

Review of attachment 650338 [details] [diff] [review]:
-----------------------------------------------------------------

SR me, and the cookie stuff looks fine with comments addressed.

::: extensions/cookie/nsCookiePermission.cpp
@@ +188,5 @@
>    // finally, check with permission manager...
>    rv = mPermMgr->TestPermission(aURI, kPermissionType, (PRUint32 *) aResult);
>    if (NS_SUCCEEDED(rv)) {
>      switch (*aResult) {
>      // if we have one of the publicly-available values, just return it

We can just add ACCESS_SESSION here and remove the special case, the comment/reasoning is legacy cruft (see the comments on the IDL bits)

::: netwerk/cookie/nsCookieService.cpp
@@ +3081,1 @@
>        case nsICookiePermission::ACCESS_ALLOW:

As I mentioned earlier, if we remove the special-casing and add ACCESS_SESSION here, this feels cleaner.

@@ +3081,5 @@
>        case nsICookiePermission::ACCESS_ALLOW:
>          return STATUS_ACCEPTED;
> +      case nsICookiePermission::ACCESS_ALLOW_FIRST_PARTY_ONLY:
> +        if (aIsForeign)
> +          return STATUS_REJECTED;

blank line here please (similar bits throughout, a blank line after a return is prevailing style, please follow it for readability)

@@ +3101,5 @@
> +    if (mCookieBehavior == BEHAVIOR_REJECTFOREIGN) {
> +      COOKIE_LOGFAILURE(aCookieHeader ? SET_COOKIE : GET_COOKIE, aHostURI, aCookieHeader,
> +                        "context is third party");
> +      return STATUS_REJECTED;
> +      }

uber-nit: misalignment.  I started looking for the other brace... it's been a long day, but still. :)

::: netwerk/cookie/nsICookiePermission.idl
@@ +21,5 @@
>     */
>    const nsCookieAccess ACCESS_DEFAULT = 0;
> +  const nsCookieAccess ACCESS_ALLOW = 1;
> +  const nsCookieAccess ACCESS_DENY = 2;
> +  const nsCookieAccess ACCESS_ALLOW_FIRST_PARTY_ONLY = 3;

Kinda picky, but can we stick this with ACCESS_SESSION (and change the number higher) since we intentionally left that gap to allow for nsIPermissionManager values to be added without colliding.  (Since they're _really_ similar...)

(You just made me remember 2004. That stung a little.)

@@ +28,5 @@
>     * additional values for nsCookieAccess, which are not directly used by
>     * any methods on this interface, but are nevertheless convenient to define
>     * here. these may be relocated somewhere else if we ever consider freezing
>     * this interface.
>     */

This comment is actually now false, since CanSetCookie directly uses it, please replace with a "values that don't match nsIPermissionManager" type comment?

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: addressed jduell's and mconnor's comments, except for one

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

From a discussion with mconnor and geekboy on #security, this patch deliberately does not support ACCESS_ALLOW_FIRST_PARTY_SESSION_ONLY. ACCESS_SESSION was introduced in 2004 long before any of the clear private data features were added. There are currently too many ways to clear cookies at the end of a session already. ACCESS_ALLOW_FIRST_PARTY_SESSION_ONLY should only be introduced if there is a significant need, and then only after revisiting the cookie preferences UI.
 
(In reply to Mike Connor [:mconnor] from comment #17)
> Comment on attachment 650338 [details] [diff] [review]
> changes to nsCookiePermission and nsCookieService to support per-site 3rd
> party cookies
> 
> Review of attachment 650338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> SR me, and the cookie stuff looks fine with comments addressed.
> 
> ::: extensions/cookie/nsCookiePermission.cpp
> @@ +188,5 @@
> >    // finally, check with permission manager...
> >    rv = mPermMgr->TestPermission(aURI, kPermissionType, (PRUint32 *) aResult);
> >    if (NS_SUCCEEDED(rv)) {
> >      switch (*aResult) {
> >      // if we have one of the publicly-available values, just return it
> 
> We can just add ACCESS_SESSION here and remove the special case, the
> comment/reasoning is legacy cruft (see the comments on the IDL bits)

Done.
 
> ::: netwerk/cookie/nsCookieService.cpp
> @@ +3081,1 @@
> >        case nsICookiePermission::ACCESS_ALLOW:
> 
> As I mentioned earlier, if we remove the special-casing and add
> ACCESS_SESSION here, this feels cleaner.

ACCESS_SESSION is never returned by nsCookiePermission::CanAccess. The allowable values are ACCESS_ALLOW, ACCESS_DENY and ACCESS_FIRST_PARTY_ONLY, all with different logic, so I left this. 

> @@ +3081,5 @@
> >        case nsICookiePermission::ACCESS_ALLOW:
> >          return STATUS_ACCEPTED;
> > +      case nsICookiePermission::ACCESS_ALLOW_FIRST_PARTY_ONLY:
> > +        if (aIsForeign)
> > +          return STATUS_REJECTED;
> 
> blank line here please (similar bits throughout, a blank line after a return
> is prevailing style, please follow it for readability)
> 
> @@ +3101,5 @@
> > +    if (mCookieBehavior == BEHAVIOR_REJECTFOREIGN) {
> > +      COOKIE_LOGFAILURE(aCookieHeader ? SET_COOKIE : GET_COOKIE, aHostURI, aCookieHeader,
> > +                        "context is third party");
> > +      return STATUS_REJECTED;
> > +      }
> 
> uber-nit: misalignment.  I started looking for the other brace... it's been
> a long day, but still. :)

Oops, done :)
 
> ::: netwerk/cookie/nsICookiePermission.idl
> @@ +21,5 @@
> >     */
> >    const nsCookieAccess ACCESS_DEFAULT = 0;
> > +  const nsCookieAccess ACCESS_ALLOW = 1;
> > +  const nsCookieAccess ACCESS_DENY = 2;
> > +  const nsCookieAccess ACCESS_ALLOW_FIRST_PARTY_ONLY = 3;
> 
> Kinda picky, but can we stick this with ACCESS_SESSION (and change the
> number higher) since we intentionally left that gap to allow for
> nsIPermissionManager values to be added without colliding.  (Since they're
> _really_ similar...)

Moved to number 9 below ACCESS_SESSION.

> @@ +28,5 @@
> >     * additional values for nsCookieAccess, which are not directly used by
> >     * any methods on this interface, but are nevertheless convenient to define
> >     * here. these may be relocated somewhere else if we ever consider freezing
> >     * this interface.
> >     */
> 
> This comment is actually now false, since CanSetCookie directly uses it,
> please replace with a "values that don't match nsIPermissionManager" type
> comment?

Fixed.

jduell, could you please re-review this? Sid tells me I need an r+ in addition to sr+.

Thank you,
Monica

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies, updates to test_cookies_thirdparty.js and bugfix to test_cookies_thirdparty_session.js

Same attachment as before, corrected to the right type, sorry for the noise.

Comment 21

[Jason Duell]

Comment on attachment 658661 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies, updates to test_cookies_thirdparty.js and bugfix to test_cookies_thirdparty_session.js

I can't +r since I'm not a cookie peer, but mconnor already +sr'd this in comment 17.  Emailed him and biesi to see if between them we can get +r/+sr.

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Re-pinged mconnor and mbiesi to get the right combination of +r/+sr. Try results came in a while back and are green.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 658661 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies, updates to test_cookies_thirdparty.js and bugfix to test_cookies_thirdparty_session.js

Review of attachment 658661 [details] [diff] [review]:
-----------------------------------------------------------------

sr=biesi

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 658661 [details] [diff] [review]
changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies, updates to test_cookies_thirdparty.js and bugfix to test_cookies_thirdparty_session.js

I should have just marked this r/sr, since I really did a full review on it anyway. :)

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Christian and Mike, you are awesome! Thanks so much for all your help. I will get Sid to check this in.

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: refreshed patch, changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

Comment 27

[[:mmc] Monica Chew (no longer reading bugmail)]

Try is green.

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

Checked in.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3192c8f271

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661362 [details] [diff] [review]
refreshed patch, changes to nsCookiePermission and nsCookieService to support per-site 3rd party cookies

re-marking the reviews (r=mconnor sr=jduell,biesi,mconnor)

Try pushlog:
https://tbpl.mozilla.org/?tree=Try&rev=2d42b1576b16

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd3192c8f271

Comment 31

[Gervase Markham [:gerv]]

Hi Monica,

It's great that this has been done :-) Bugs like bug 535336 have been open for some time now, requesting that extensions have the ability to hook into cookie setting and getting, so different people can experiment with different cookie acceptance policies and see what does, and does not, break too much of the web. Are you changes here going to help people do that, or is that situation unimproved?

It would have been awesome if this feature could have been architected so that the cookie acceptance policy was pluggable...

Gerv

Comment 32

[Sid Stamm [:geekboy or :sstamm]]

Extensions will be able to set these permissions to perhaps block third party cookies for a list of sites the user doesn't trust.  I don't think this helps with the need for hooking into set/get policies, but it is a tool add-ons can use.

Comment 33

[Mike Connor [:mconnor]]

It's also probably fairly trivial to write an add-on to extend the existing permissions UIs.  I think it _may_ be worth exposing this capability in the prefwindow permissions list, if only to prevent any setting of this permission from breaking/confusing that UI.

Comment 34

[[:mmc] Monica Chew (no longer reading bugmail)]

(already replied off bug, but adding here too)
These changes don't affect the pluggability of cookie policies, but that's a great idea.

I was also planning on exposing the new enum in about:permissions (see bug 770705).

Comment 35

[Ian Nartowicz]

Did this fix a different bug from the one that was raised?  As I understand the description in comment 1, we need a setting to allow third party cookies to be set by any domain on particular pages: ALLOW_THIRD_PARTY.  And possibly to block them despite being globally allowed: DENY_THIRD_PARTY.  This is useful for support of things like syndicated logins.  The design wiki describes this case explicitly:

3rd Party Cookie Global DENY & foo.com 3rd party cookie ALLOW

    A page at bar.com gets/sets baz.com 3rd party cookie: DENY
    A page at foo.com gets/sets baz.com 3rd party cookie: ALLOW 

The patch here implements something completely different (ALLOW_FIRST_PARTY_ONLY), the ability for foo.com to be allowed to set cookies despite any global settings, but only on foo.com pages, not on baz.com or bar.com pages.

Comment 36

[Sid Stamm [:geekboy or :sstamm]]

Ian: perhaps that feature page could be clearer and more consistent.  If you look at section 2 of the feature page (linked in comment 1) it describes two examples of "I trust site x.com to set cookies regardless of what site I'm visiting".  

So the original point of this bug was to be able to specify which hosts can set third party cookies across the web, not to specify on which sites *all* hosts' cookies should be allowed.

More explicitly, for a situation where I'm on site A and it embeds content from B (A=>B): we want to enable cookies for individual host B regardless of the value of A.  (Not enable cookies for all values of B when visiting one specific A).

I think perhaps that section you reference is misleading and should be removed from the document.  I'll fix that.

Comment 37

[Ian Nartowicz]

Thanks for clearing that up, Sid.  Shame really.  This bug provides an interesting feature, but what I described is what a lot of people really need.  Just thinking about it for a few minutes suggests many complications from creating permissions that apply to the domain of the page rather than to the domain of the cookie, so I guess we'll have to wait a bit longer for that.

Comment 39

[Ian Nartowicz]

This breaks the existing cookie exception dialog.  Setting an exception of the new type causes the dialog to be empty and a console exception.  I raised bug 841212.