1478427 - Apply the default cookie restrictions to trackers as well as non-visited third-parties

Status: RESOLVED WONTFIX

(Firefox :: Protections UI, enhancement, P3)

Comment 1

[Tanvi Vyas[:tanvi]]

There should be a way to apply both:
1) default cookie restrictions experiment - block third party cookies from trackers. And
2) block third party cookies from sites I've never visited.

First we would do 1, and then fallback to the from visited rule for everything that isn't categorized as a tracker.

The SSO provider activation should still work as part of 1).  For 2), it shouldn't technically be an issue since you have probably visited the SSO site before trying to login with it.  But if you haven't, when you window.open it, it becomes visited.  We should ensure there is a test case for that:
* Test: First party foo.com, third party login.com.  Third party is not a tracker.  Third party is not visited.  foo.com does a window.open to login.com.  login.com third party cookies should now be accessible on foo.com.

Comment 2

[(no longer active)]

It's worth explaining something about the behavior of the existing "Accept third party cookies only from visited websites" feature.  That pref *only* impacts the acceptance of cookies, and nothing else.  For all other storage backends, when that pref is active, Gecko blocks third-party storage, no matter whether the website is visited or not:

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/dom/base/nsContentUtils.cpp#9071
https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/dom/base/nsContentUtils.cpp#8820

Also, in the case of cookies, what gets checked in practice is whether the host has already set a cookie in the cookie database, not whether the site has been visited. (i.e., the Places database is never examined).

The description in comment 1 doesn't match at all with how BEHAVIOR_LIMIT_FOREIGN is implemented, so I don't think that's how we should implement the new cookie behavior proposed in this bug.  Here is how I think this should work, and how I was planning to implement it:

* For non-cookie storage backends, do exactly as we do when the privacy.restrict3rdpartystorage.enabled pref is turned on.  No visited checks of any sorts happen for non-cookie backends.
* For the cookie storage backend, if the target host is tracking, use the rules from the implementation of the current privacy.restrict3rdpartystorage.enabled pref.  Or, if the target host has already set one cookie, accept the cookie.  Else, reject the cookie.
* Extend the application of our heuristic and granting first-party storage access based on that, or the storage access API, only based on whether the target context is third-party, as opposed to whether it's a *tracking* third-party context.  This means our heuristic *and* the storage access API will work with all third-party contexts when this pref is turned on, which makes sense, as storage in *all* third-party contexts will be blocked, as I explained above.
* The heuristic will at no point check or care about the visited-ness of a website, that isn't really needed for anything here.

(In reply to Tanvi Vyas[:tanvi] from comment #1)
> * Test: First party foo.com, third party login.com.  Third party is not a
> tracker.  Third party is not visited.  foo.com does a window.open to
> login.com.  login.com third party cookies should now be accessible on
> foo.com.

This will work with the proposed design I explained above.

Comment 3

[Tanvi Vyas[:tanvi]]

baku, are you taking this or is Ehsan to complete when he is back?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - BEHAVIOR_LIMIT_FOREIGN_AND_REJECT_TRACKER

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - UI

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 9

[(no longer active)]

Comment on attachment 9010331 [details] [diff] [review]
part 1 - BEHAVIOR_LIMIT_FOREIGN_AND_REJECT_TRACKER

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

::: browser/components/preferences/in-content/privacy.js
@@ +797,5 @@
>          break;
> +      case Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN_AND_REJECT_TRACKER:
> +        blockCookiesCtrl.value = "disallow";
> +        blockCookiesMenu.value = "trackers";
> +        break;

This looks like part of your UI patch too...

::: browser/components/preferences/in-content/tests/browser_contentblocking.js
@@ +599,5 @@
>      [Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN, 0],
>      [Ci.nsICookieService.BEHAVIOR_REJECT, 1],
>      [Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN, 2],
>      [Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER, 0],
> +    [Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER_AND_LIMIT_FOREIGN, 0],

You should not need this line in this patch, maybe this should be added to the UI patch.  Or, this is a sign that something is broken if the test fails after applying this patch without this line.

::: netwerk/base/nsILoadInfo.idl
@@ +1070,5 @@
> +  /**
> +    * True if the current loading has cookies involved. This is used to know
> +    * if this channel has been already visited.
> +    */
> +  [infallible] attribute boolean hasCookies;

This is such a misleading name!  Please call this:

  hasSetCookiesAsFirstPartyBefore;

::: netwerk/cookie/CookieServiceChild.cpp
@@ -302,5 @@
>    int32_t val;
>    if (NS_SUCCEEDED(aPrefBranch->GetIntPref(kPrefCookieBehavior, &val)))
>      mCookieBehavior =
>        val >= nsICookieService::BEHAVIOR_ACCEPT &&
> -      val <= nsICookieService::BEHAVIOR_LIMIT_FOREIGN

oops!

Can you file this part as a separate bug?  We should maybe uplift this to beta...

::: netwerk/cookie/nsCookieService.cpp
@@ +4363,5 @@
> +    // cookies are allowed if have have some existing cookie (website visited)
> +    if (aCookieBehavior == nsICookieService::BEHAVIOR_REJECT_TRACKER_AND_LIMIT_FOREIGN) {
> +      if (aIsTrackingResource && !aFirstPartyStorageAccessGranted) {
> +        COOKIE_LOGFAILURE(aCookieHeader ? SET_COOKIE : GET_COOKIE, aHostURI, aCookieHeader, "cookies are disabled in trackers");
> +        *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_TRACKER;

This is wrong.  You need to define a new rejection reason (STATE_COOKIES_BLOCKED_TRACKER_AND_LIMIT_FOREIGN) and emit that here.

The reason is that we use these notifications to inform the user about what category of our blockers got activated in the control centre.  With what you have now, on the UI side we will be unable to distinguish the case where cookies are blocked by the pref 4 or 5 unless we look at the pref, which will at least be racy and unreliable.

@@ +4367,5 @@
> +        *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_TRACKER;
> +        return STATUS_REJECTED;
> +      }
> +
> +      if (!aIsTrackingResource && aNumOfCookies == 0) {

I think these conditions are probably a bit wrong.

If we have a tracker, we need to make sure aFirstPartyStorageAccessGranted can override the blocking.  So, the first condition looks OK.  The second one is suspect though.  What if you have a top-level document which is on the content blocking allow list (which will cause aFirstPartyStorageAccessGranted to be true) and aNumOfCookies is 0 and your channel is a non-tracking one?  It looks like here you would reject the cookies but I think the right behavior would be to accept.

IOW, I think this block should be rewritten as:

  if (aCookieBehavior == nsICookieService::BEHAVIOR_REJECT_TRACKER_AND_LIMIT_FOREIGN &&
      !aFirstPartyStorageAccessGranted) {
    if (aIsTrackingResource) {
      ...
    }
    if (aNumOfCookies == 0) {
      ...
    }
  }

What do you think?

@@ +4369,5 @@
> +      }
> +
> +      if (!aIsTrackingResource && aNumOfCookies == 0) {
> +        COOKIE_LOGFAILURE(aCookieHeader ? SET_COOKIE : GET_COOKIE, aHostURI, aCookieHeader, "context is unvisited third party");
> +        *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_FOREIGN;

Ditto.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3481,5 @@
>      if (cookie.IsEmpty()) {
>        cookie = mUserSetCookieHeader;
>      }
> +    else {
> +      MOZ_ALWAYS_SUCCEEDS(mLoadInfo->SetHasCookies(true));

This is completely wrong.  :-)  I think you'll realize why once you renamed the loadinfo member...

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +556,1 @@
>      LOG(("Nothing more to do due to the behavior code %d", int(behavior)));

Nit: the behavior is BEHAVIOR_REJECT_FOREIGN now, so you can just log it directly.

@@ +582,5 @@
> +      return true;
> +    }
> +
> +    // Now, we have to also honour the Content Blocking pref.
> +    if (!StaticPrefs::browser_contentblocking_enabled()) {

Tanvi has been very strongly opposed to this, we need to convince her first before making this change.  And once we do, the right way to make this change around here: <https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/toolkit/components/antitracking/AntiTrackingCommon.cpp#541> by removing the if and/or changing its condition.

@@ +588,5 @@
> +           "by pretending our window isn't a third-party window"));
> +      return true;
> +    }
> +
> +    if (CheckContentBlockingAllowList(aWindow)) {

Tanvi has also been very strongly opposed to this as well, see above.

@@ +594,5 @@
> +      return true;
> +    }
> +
> +    LOG(("Nothing more to do due to the behavior code %d", int(behavior)));
> +    *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_FOREIGN;

I think this would probably deserve a new rejection code too (and we should send it from the cookie manager as well.)

@@ +619,5 @@
> +        LOG(("Allowing access to visited third-party window"));
> +        return true;
> +      }
> +
> +      LOG(("Nothing more to do due to the behavior code %d", int(behavior)));

What about the permission check?!

I think you should remove these three lines and fall into a similar code path as BEHAVIOR_REJECT_TRACKER after checking visitedness.

@@ +809,1 @@
>      LOG(("Nothing more to do due to the behavior code %d", int(behavior)));

See the above nit.

@@ +817,5 @@
> +      return true;
> +    }
> +
> +    // Now, we have to also honour the Content Blocking pref.
> +    if (!StaticPrefs::browser_contentblocking_enabled()) {

See above.

@@ +823,5 @@
> +           "by pretending our window isn't a third-party window"));
> +      return true;
> +    }
> +
> +    if (CheckContentBlockingAllowList(aChannel)) {

See above.

@@ +829,5 @@
> +      return true;
> +    }
> +
> +    LOG(("Nothing more to do due to the behavior code %d", int(behavior)));
> +    *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_FOREIGN;

See above.

@@ +874,5 @@
> +        LOG(("Allowing access to visited third-party window"));
> +        return true;
> +      }
> +
> +      LOG(("Nothing more to do due to the behavior code %d", int(behavior)));

See above.

Comment 10

[(no longer active)]

Comment on attachment 9010332 [details] [diff] [review]
part 2 - UI

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

The changes here look fine, but I think Johann is a better review than I am.  (You should merge a couple of hunks from the first patch into this before asking for review)

Were you also planning to work on the control centre UI?

::: browser/locales/en-US/browser/preferences/preferences.ftl
@@ +767,5 @@
>      .label = Third-party trackers (recommended)
>  sitedata-block-trackers-option =
>      .label = Third-party trackers
> +sitedata-block-trackers-and-unvisited-option =
> +    .label = Third-party trackers and Cookies from unvisited websites

I asked Tanvi to dig up the latest copy of the strings we have from Michelle for these.

Note that you need a review from a l10n peer like :flod.

Comment 11

[(no longer active)]

Comment on attachment 9010335 [details] [diff] [review]
part 3 - tests

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

This is really nice, thanks!

One case that I think this doesn't cover though is third-parties that aren't trackers.  Were you planning to add tests for those cases as well?

::: toolkit/components/antitracking/test/browser/head.js
@@ +280,5 @@
> +          blockingByContentBlocking: true,
> +          allowList: true,
> +          callback: callbackNonTracking,
> +          extraPrefs: [["urlclassifier.trackingAnnotationWhitelistTable.testEntries",
> +                        "tracking.example.org,tracking.example.com"]],

Out of curiosity, what makes this necessary?

Comment 12

[Andrea Marchesini [:baku]]

> One case that I think this doesn't cover though is third-parties that aren't
> trackers.  Were you planning to add tests for those cases as well?
> ...
> > +          extraPrefs: [["urlclassifier.trackingAnnotationWhitelistTable.testEntries",
> > +                        "tracking.example.org,tracking.example.com"]],
> Out of curiosity, what makes this necessary?

This is what the test does :)
I whitelist the tracking URLs. These URLs will not be annotated as trackers, and we test the unvisited part of this new cookie policy.

Comment 13

[Andrea Marchesini [:baku]]

> Can you file this part as a separate bug?  We should maybe uplift this to
> beta...

Bug 1492769.

> This is wrong.  You need to define a new rejection reason
> (STATE_COOKIES_BLOCKED_TRACKER_AND_LIMIT_FOREIGN) and emit that here.
> 
> The reason is that we use these notifications to inform the user about what
> category of our blockers got activated in the control centre.  With what you
> have now, on the UI side we will be unable to distinguish the case where
> cookies are blocked by the pref 4 or 5 unless we look at the pref, which
> will at least be racy and unreliable.

But in this way it's not possible to distinguish the blocking of trackers and the blocking of unvisited websites. I guess we should check what's the UI needs before introducing a new block value.

> > +    // Now, we have to also honour the Content Blocking pref.
> > +    if (!StaticPrefs::browser_contentblocking_enabled()) {
> 
> Tanvi has been very strongly opposed to this, we need to convince her first
> before making this change.  And once we do, the right way to make this
> change around here:
> <https://searchfox.org/mozilla-central/rev/
> a0333927deabfe980094a14d0549b589f34cbe49/toolkit/components/antitracking/
> AntiTrackingCommon.cpp#541> by removing the if and/or changing its condition.

Well, it's more consistent with the previous code. I'll ping her to know why she is against this check.

> > +      LOG(("Nothing more to do due to the behavior code %d", int(behavior)));
> 
> What about the permission check?!

This is done at the beginning of the method, right?

Comment 14

[Andrea Marchesini [:baku]]

> > +    * True if the current loading has cookies involved. This is used to know
> > +    * if this channel has been already visited.
> > +    */
> > +  [infallible] attribute boolean hasCookies;
> 
> This is such a misleading name!  Please call this:
> 
>   hasSetCookiesAsFirstPartyBefore;

This is a wrong name as well. 'Visited' doesn't mean 'as first party'. In fact, if I change the cookie policy, it could happen that a 3rd party website already has cookies and it must be considered as visited. Plus, cookies do not have any knowledge of the context (first party or 3rd party).

I would use: "hasSetCookiesBefore".

Comment 15

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #13)
> > Can you file this part as a separate bug?  We should maybe uplift this to
> > beta...
> 
> Bug 1492769.

Thanks!  :-)

> > This is wrong.  You need to define a new rejection reason
> > (STATE_COOKIES_BLOCKED_TRACKER_AND_LIMIT_FOREIGN) and emit that here.
> > 
> > The reason is that we use these notifications to inform the user about what
> > category of our blockers got activated in the control centre.  With what you
> > have now, on the UI side we will be unable to distinguish the case where
> > cookies are blocked by the pref 4 or 5 unless we look at the pref, which
> > will at least be racy and unreliable.
> 
> But in this way it's not possible to distinguish the blocking of trackers
> and the blocking of unvisited websites. I guess we should check what's the
> UI needs before introducing a new block value.

Well, in my experience we have to be prepared for *everything* since the folks in charge of the UI change their opinions every day.  ;-)  So realistically if we just implement the thing that is needed today we will probably have to modify it tomorrow...

Perhaps the best thing to do is to add block values that tell us we blocked what kind of cookie/storage access based on this policy, e.g. we blocked a tracker because or this policy, or, we blocked an unvisited foreign party because of this policy.  That's the most complete information we could expose and we can build what we want on the UI side on top of it.  What do you think?

> > > +    // Now, we have to also honour the Content Blocking pref.
> > > +    if (!StaticPrefs::browser_contentblocking_enabled()) {
> > 
> > Tanvi has been very strongly opposed to this, we need to convince her first
> > before making this change.  And once we do, the right way to make this
> > change around here:
> > <https://searchfox.org/mozilla-central/rev/
> > a0333927deabfe980094a14d0549b589f34cbe49/toolkit/components/antitracking/
> > AntiTrackingCommon.cpp#541> by removing the if and/or changing its condition.
> 
> Well, it's more consistent with the previous code. I'll ping her to know why
> she is against this check.

Good luck!  :-)  I have had that conversation a bunch of times.

Note that if you convinced her, we would need to change a whole list of things, and that should definitely happen in a follow-up bug.  That impacts both Gecko side and UI side.  If you got a yes, please file a new bug and I'll write a long description of what needs to change...

> > > +      LOG(("Nothing more to do due to the behavior code %d", int(behavior)));
> > 
> > What about the permission check?!
> 
> This is done at the beginning of the method, right?

The cookie permission check, yes.  I'm talking about 3rdPartyStorage check.

Comment 16

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #14)
> > > +    * True if the current loading has cookies involved. This is used to know
> > > +    * if this channel has been already visited.
> > > +    */
> > > +  [infallible] attribute boolean hasCookies;
> > 
> > This is such a misleading name!  Please call this:
> > 
> >   hasSetCookiesAsFirstPartyBefore;
> 
> This is a wrong name as well. 'Visited' doesn't mean 'as first party'. In
> fact, if I change the cookie policy, it could happen that a 3rd party
> website already has cookies and it must be considered as visited. Plus,
> cookies do not have any knowledge of the context (first party or 3rd party).
> 
> I would use: "hasSetCookiesBefore".

We talked about this on IRC.

We decided to add a ternary isForeign field to the cookies DB to be able to precisely identify first-party cookies for this purpose.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 9010332 [details] [diff] [review]
part 2 - UI

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

I just looked at the code and didn't try to run it. As Ehsan mentioned, there are still some frontend parts in patch 1, it would be nice to get them into this one.

- Maybe I'm missing something but don't you need to update this function as well?: https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/browser/components/preferences/in-content/privacy.js#1293

- Please also add the new pref value to this test: https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/browser/components/preferences/in-content/tests/browser_contentblocking.js#553

Thanks!

::: browser/locales/en-US/browser/preferences/preferences.ftl
@@ +767,5 @@
>      .label = Third-party trackers (recommended)
>  sitedata-block-trackers-option =
>      .label = Third-party trackers
> +sitedata-block-trackers-and-unvisited-option =
> +    .label = Third-party trackers and Cookies from unvisited websites

This might not be the final copy, but note that "Cookies" probably shouldn't be capitalized here.

Comment 18

[Tanvi Vyas[:tanvi]]

The latest copy I have from Michelle is here:
https://docs.google.com/presentation/d/1P5J29D02y6SMV52bB6D-LjlEcZuavgbwX3_y8cAHLv4/edit#slide=id.g3de60d0219_0_12

Michelle, this is in regards to a new cookie setting that combines from visited and known trackers.  The content for the Cookies and Site Data section reads:
"Third-party trackers and cookies from unvisited websites"

The content in the Content Blocking section reads:
"Trackers and cookies from unvisited sites"

Michelle, please confirm this is still good.

(Note, that I'm not sure if this bug is just updating the Cookies and Site data section, or also updating Content Blocking.)

Comment 19

[Tanvi Vyas[:tanvi]]

Also needinfo'ing Besty.

(In reply to Tanvi Vyas[:tanvi] from comment #18)
> The latest copy I have from Michelle is here:
> https://docs.google.com/presentation/d/1P5J29D02y6SMV52bB6D-
> LjlEcZuavgbwX3_y8cAHLv4/edit#slide=id.g3de60d0219_0_12
> 
> Michelle, this is in regards to a new cookie setting that combines from
> visited and known trackers.  The content for the Cookies and Site Data
> section reads:
> "Third-party trackers and cookies from unvisited websites"
> 
> The content in the Content Blocking section reads:
> "Trackers and cookies from unvisited sites"
> 
> Michelle, please confirm this is still good.
> 
> (Note, that I'm not sure if this bug is just updating the Cookies and Site
> data section, or also updating Content Blocking.)

Comment 20

[(no longer active)]

This UI is going to change in 65, I really don't think it is worth trying to land a UI for this feature right now, much less so figuring out the right strings for it.

Andrea, my suggestion is to remove the UI changes for now and punt on that part before the 65 changes to the Prefs/Control Centre UI are in.

Comment 22

[Johann Hofmann [:johannh]]

We're not doing this anymore.