1258033 - Fix the DNT loophole for tracking protection

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

If a site gets removed from the Disconnect list of trackers because they honor the DNT signal, TP stops blocking it, but it doesn't guarantee that tracking will be disabled since it relies on the user enabling DNT separately (as pointed out in bug 1225143).

We intend to move these de-listed sites to a separate list of DNT-enabled trackers that will not be blocked but will receive an automatic DNT signal when TP is on.

Comment 1

[François Marier [:francois]]

Selectively sending the DNT header to sites on a DNT-compliant list wasn't going to work with the DOM API for DNT, so we decided to instead send all sites a DNT header when the user is in TP mode.

Comment 2

[François Marier [:francois]]

There are two places where we send the DNT signal:

- the DoNotTrack header: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/netwerk/protocol/http/nsHttpHandler.cpp#492-498
- the DoNotTrack DOM API: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/dom/base/Navigator.cpp#760-770

They currently just look at privacy.donottrackheader.enabled and that needs
to be replaced with something like:

    enableDNT = getbool(privacy.donottrackheader.enabled) || TrackingProtectionEnabled()

where TrackingProtectionEnabled() essentially does this check:

    if (!Preferences::GetBool("privacy.trackingprotection.enabled", false) &&
        (!Preferences::GetBool("privacy.trackingprotection.pbmode.enabled",
                               false) || !NS_UsePrivateBrowsing(aChannel))) {
      return false;
    } else {
      return true;
    }

See: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/netwerk/base/nsChannelClassifier.cpp#69-73

Ideally, instead of duplicating that check, we should refactor it into a
separate function and then call it from both places. Maybe that function
should live in the URI Classifier or in the HttpChannel itself, I'm not
sure.

The four cases we need to test are:

- DNT turned on,  TP turned off               -> DNT signal sent both in private browsing and normal mode
- DNT turned off, TP turned on globally       -> DNT signal sent both in private browsing and normal mode
- DNT turned off, TP in Private Browsing only -> DNT signal sent in private browsing mode only
- DNT turned off, TP turned off               -> DNT signal is never sent

It would be good to test that the "DNT" request header is the right one and
that the "navigator.DoNotTrack()" JavaScript API:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack

returns the right value too.

Comment 3

[Dimi Lee [:dimi]]

Attachment: WIP patch

Comment 4

[Dimi Lee [:dimi]]

Attachment: P1. Support IsTrackingProtectionEnabled util function

Comment 5

[Dimi Lee [:dimi]]

Attachment: P2. Set DNT header based on preference and private browsing mode

Hi Francois,
The reason i move set DNT header from |Init| to |begineConnect| is because now we will set 'DNT' based on private browsing mode and in the |Init| it is too early to check if this channel is in private browsing mode.

But I am not really sure where is the best place to put the header check, i guess we will need a necko peer to review this also.

Comment 6

[Dimi Lee [:dimi]]

Attachment: P3. Return navigator.doNotTrack based on preference and private browsing mode

Comment 7

[Dimi Lee [:dimi]]

Attachment: P4. Testcase

The testcase covers suggestions by francois in comment2

Comment 8

[François Marier [:francois]]

Comment on attachment 8756683 [details] [diff] [review]
P2. Set DNT header based on preference and private browsing mode

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

Looks fine to me, but we should ask a necko peer to review this one. I don't know the implications of moving the place where we check the pref and set the header.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7548,5 @@
> +nsHttpChannel::SetDoNotTrack()
> +{
> +  /**
> +   * 'DoNotTrack' header should be added if 'privacy.donottrackheader.enabled'
> +   * is true or tracking protection is enabled.

Let's reference this bug in the comment by adding "(see bug 1258033)" at the end of this comment.

Comment 9

[François Marier [:francois]]

Comment on attachment 8756684 [details] [diff] [review]
P3. Return navigator.doNotTrack based on preference and private browsing mode

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

::: dom/base/Navigator.cpp
@@ +777,5 @@
> +  nsCOMPtr<nsILoadContext> loadContext;
> +  win->GetInterface(NS_GET_IID(nsILoadContext), getter_AddRefs(loadContext));
> +
> +  bool isPrivateBrowsing = false;
> +  loadContext->GetUsePrivateBrowsing(&isPrivateBrowsing);

I wonder if we couldn't put the IsTrackingProtectionEnabled util function in the load context or the channel instead?

It would be good to avoid duplicating the pref checking if we can because there might be other ways for tracking protection to be enabled in the future.

This (if it's possible) would be quite nice:

  bool isTrackingProtectionEnable = false
  loadContext->GetTrackingProtectionEnabled(&isTrackingProtectionEnabled)

Comment 10

[François Marier [:francois]]

Comment on attachment 8756685 [details] [diff] [review]
P4. Testcase

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

Very thorough test, thanks!

Only a small issue around pref cleanup at the end of the test.

::: toolkit/components/url-classifier/tests/mochitest/test_donottrack.html
@@ +73,5 @@
> +const contentPage =
> +  "http://mochi.test:8888/tests/toolkit/components/url-classifier/tests/mochitest/dnt.html";
> +
> +function executeTest(test) {
> +  SpecialPowers.setBoolPref(DNT_PREF, test.setting.dntPref);

Maybe use SpecialPowers.pushPrefEnv() and popPrefEnv() instead?

We should clean up the prefs at the end to avoid interfering with any other tests.

Comment 11

[François Marier [:francois]]

Comment on attachment 8756682 [details] [diff] [review]
P1. Support IsTrackingProtectionEnabled util function

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

This looks good, but it may need to move somewhere else if you find a way to address comment 9.

::: netwerk/base/nsChannelClassifier.cpp
@@ +587,5 @@
> +// static
> +bool
> +nsChannelClassifier::IsTrackingProtectionEnabled(nsIChannel *aChannel)
> +{
> +  if (!Preferences::GetBool("privacy.trackingprotection.enabled", false) &&

nit: it might be a good time to rewrite this check so that it's easier to read

For example:

if (privacy.trackingprotection.enabled) {
  return true
}
if (privacy.trackingprotection.pbmode && NS_UsePrivateBrowsing(channel)) {
  return true
}
return false

Comment 12

[Dimi Lee [:dimi]]

Attachment: P1. Add IsTrackingProtectionOn for nsILoadContext

Comment 13

[Dimi Lee [:dimi]]

hi Olli,
Could you help review if it is ok to add new API for nsILoadContext ? The purpose for this API is to know if tracking protection is on for the load context based on preference and private browsing mode.
Thanks!

Comment 14

[Dimi Lee [:dimi]]

Attachment: P2. Set DNT header based on preference and tracking protection

Hi Patrick,
The patch is to add DNT header based on preference and tracking protection mode.
Comment 5 explains why i change the place to set DNT header.

Another question is that i keep |mDoNotTrackEnabled| for telemetry[1]
But should i also update how we record the Telemetry::DNT_USAGE since now if DNT header should be sent is changed.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#2144

Comment 15

[Dimi Lee [:dimi]]

Attachment: P3. navigator.doNotTrack should be determinded by preference and tracking protection

Comment 16

[Dimi Lee [:dimi]]

Attachment: P4. Use LoadContext API for channel classifier

Comment 17

[Dimi Lee [:dimi]]

Attachment: P5. Testcase

Address Comment 10

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757183 [details] [diff] [review]
P1. Add IsTrackingProtectionOn for nsILoadContext


>+
>+  /**
>+   * Returns true if tracking protection is enabeld for the load context.
enabled

>+   */
>+  [noscript] boolean IsTrackingProtectionOn();
Why noscript? A tad odd in an interface which isn't marked to be builtinclass

>+
>+%{C++
>+  /**
>+   * De-XPCOMed getter to make call-sites cleaner.
>+   */
>+  bool UseTrackingProtection() {
>+    bool usingTP;
>+    IsTrackingProtectionOn(&usingTP);
>+    return usingTP;
>+  }
>+%}
Ahaa, [infallible] readonly attribute wouldn't work since the interface isn't builtinclass.

Comment 19

[François Marier [:francois]]

Comment on attachment 8757190 [details] [diff] [review]
P3. navigator.doNotTrack should be determinded by preference and tracking protection

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

I would suggest getting a DOM peer to review this one too once you've added the null check.

::: dom/base/Navigator.cpp
@@ +757,5 @@
>  Navigator::GetDoNotTrack(nsAString &aResult)
>  {
> +  aResult.AssignLiteral("unspecified");
> +
> +  nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow);

I would add a null check like NS_ENSURE_TRUE(win, NS_OK);

Comment 20

[François Marier [:francois]]

Comment on attachment 8757192 [details] [diff] [review]
P4. Use LoadContext API for channel classifier

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

Looks good to me, perhaps roll it into the other necko patch so that mcmanus looks at both of them at once?

Comment 21

[François Marier [:francois]]

Comment on attachment 8757194 [details] [diff] [review]
P5. Testcase

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

Looks great!

Comment 22

[Dimi Lee [:dimi]]

Attachment: P2. Set DNT header based on preference and tracking protection v2

Merge P2 and P4 to review based on francois's suggestion.
Purpose of this patch is describe in Comment 5 and Comment 14

Comment 23

[Dimi Lee [:dimi]]

Attachment: P3. navigator.doNotTrack should be determinded by preference and tracking protection v2

Hi Olli,
Could you also help review this patch ? thanks!

Comment 24

[François Marier [:francois]]

Comment on attachment 8757192 [details] [diff] [review]
P4. Use LoadContext API for channel classifier

These got merged (see comment 22).

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757537 [details] [diff] [review]
P3. navigator.doNotTrack should be determinded by preference and tracking protection v2

Could we figure out a way to not lose the boolvarcache part.
Maybe add AddBoolVarCache to nsContentUtils and use the same value in dochshell and in this code.
This is not hot code or anything, but one never knows what some silly DOM benchmarks do. And there isn't any reason to not use varcache.
And besides, that reduces code duplication when the reading of the pref is handled in one place.

I know, this is über-nit.

Comment 26

[Dimi Lee [:dimi]]

(In reply to Olli Pettay [:smaug] from comment #18)
> >+   */
> >+  [noscript] boolean IsTrackingProtectionOn();
> Why noscript? A tad odd in an interface which isn't marked to be builtinclass

Yes, you are right, i shouldn't add noscript, will remove it in next patch

Comment 27

[Dimi Lee [:dimi]]

(In reply to Olli Pettay [:smaug] from comment #25)
> Could we figure out a way to not lose the boolvarcache part.
> Maybe add AddBoolVarCache to nsContentUtils and use the same value in
> dochshell and in this code.

The docshell did not use the "privacy.donottrackheader.enabled", but in both navigator and nsHttpChannel in patch P2 will use this preference.
I will add boolvarcache in nsContentUtils, thanks for you suggestion!

Comment 28

[Dimi Lee [:dimi]]

Attachment: P1. Add IsTrackingProtectionOn for nsILoadContext v2

Address Comment 18

Comment 29

[Dimi Lee [:dimi]]

Attachment: P3. navigator.doNotTrack should be determinded by preference and tracking protection v3

The |nsContentUtils::DoNotTrackEnabled| should be also used by |nsHttpChannel| in patch Part2. I will add it after the patch is reviewed.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757772 [details] [diff] [review]
P3. navigator.doNotTrack should be determinded by preference and tracking protection v3

> Navigator::GetDoNotTrack(nsAString &aResult)
> {
>-  if (sDoNotTrackEnabled) {
>+  aResult.AssignLiteral("unspecified");
>+
>+  nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow);
>+  NS_ENSURE_TRUE(win, NS_OK);
So this is a change to the behavior, returning unspecified for disconnected Navigator.

>+
>+  if ((loadContext && loadContext->UseTrackingProtection()) ||
>+      nsContentUtils::DoNotTrackEnabled()) {
>     aResult.AssignLiteral("1");
>-  } else {
>-    aResult.AssignLiteral("unspecified");
>   }

Perhaps you could write this:

bool doNotTrack = nsContentUtils::DoNotTrackEnabled();
if (!doNotTrack) {
  nsCOMPtr<nsILoadContext> loadContext = do_GetInterface(mWindow);
  doNotTrack = loadContext && loadContext->UseTrackingProtection();
}

if (doNotTrack) {
  aResult.AssignLiteral("1");
} else {
  aResult.AssignLiteral("unspecified");
}

return NS_OK;


>+nsContentUtils::DoNotTrackEnabled()
>+{
Given that we're planning to use this in Necko, want to add 
MOZ_ASSERT(NS_IsMainThread());


with that all, r+

Comment 31

[Dimi Lee [:dimi]]

Attachment: Missing serialized load context(This should be merged to P1)

Hi Olli,
Sorry i missed the serialized load context part for review, could you help review again ? thanks!

One thing I am not sure is in |TabParent::GetLoadContext|[1], in this patch I didn't pass "tracking protection on/off" to LoadContext constructor, should I pass it ? If yes, do you have any suggestion what is a better way to do it. Because I am not sure if check private browsing mode and preference here is a good idea.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#2776

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #31)
> Because I am not sure if check private browsing mode and
> preference here is a good idea.
Why not?
That is what I'd expect. Then we'd get consistent handling with docshell.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8758648 [details] [diff] [review]
Missing serialized load context(This should be merged to P1)

So based on that I assume a new patch. Or if you think we for some reason should use the pb flag and/or pref in loadcontext, please explain why not.

Comment 34

[Dimi Lee [:dimi]]

(In reply to Olli Pettay [:smaug] from comment #32)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #31)
> > Because I am not sure if check private browsing mode and
> > preference here is a good idea.
> Why not?
> That is what I'd expect. Then we'd get consistent handling with docshell.

Since IsTrackingProtectionOn is based on pref and pb flag, and LoadContext already has pb flag(mUsePrivateBrowsing). I am thinking if we should implement the check directly in the LoadContext::IsTrackingProtectionOn instead of passing it in TabParent.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

oh you mean that. Yes I was thinking too that it should be implemented in LoadContext::IsTrackingProtectionOn based on the existing pb flag and pref.

Comment 36

[Dimi Lee [:dimi]]

(In reply to Olli Pettay [:smaug] from comment #35)
> oh you mean that. Yes I was thinking too that it should be implemented in
> LoadContext::IsTrackingProtectionOn based on the existing pb flag and pref.

Sorry I didn't explain it clearly in previous comment.

In the TabParent case, it looks like a good idea implement it in the LoadContext::IsTrackingProtectionOn itself. But LoadContext is also used in ipc[1], in this case, it seems LoadContext::IsTrackingProtectionOn should represent the value passed in SerializedLoadContext, not pref + pb flag. I am not really familiar with loadContext, so not sure what is the best way here.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#200

Comment 37

[Dimi Lee [:dimi]]

Attachment: Implement IsTrackingProtection in LoadContext(This should be merged to P1)

Ask for review again based on Olli's suggestion in Comment 35.

Comment 38

[Dimi Lee [:dimi]]

Attachment: P1. Add IsTrackingProtectionOn for nsILoadContext. (r+ by smaug)

Merge P1 and "Implement IsTrackingProtection in LoadContext" patch

Comment 39

[Dimi Lee [:dimi]]

Attachment: P2. navigator.doNotTrack should be determinded by preference and tracking protection. (r+ by smaug)

This is original P3, but according to Comment 25 and Comment 27. Add |nsContentUtils::DoNotTrackEnabled| to this patch and it will also be used by necko in P2.
So reverse the order of p2 and p3.

Comment 40

[Dimi Lee [:dimi]]

Attachment: P3. Set DNT header based on preference and tracking protection (r+ by mcmanus)

Move this to P3

Comment 41

[Dimi Lee [:dimi]]

Attachment: P4. Testcase for DNT. (r+ by francois)

Comment 42

[Dimi Lee [:dimi]]

Attachment: P4. Testcase for DNT. (r+ by francois)

fix a error that testcase may timeout

Comment 43

[Dimi Lee [:dimi]]

try :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f218a8f65693

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e4a2e5f30099
Part 1: Add IsTrackingProtectionOn for nsILoadContext. r=smaug
https://hg.mozilla.org/integration/fx-team/rev/9df3db7f35d8
Part 2: navigator.doNotTrack should be determinded by preference and tracking protection. r=smaug
https://hg.mozilla.org/integration/fx-team/rev/1311e22e3ab4
Part 3: Set DNT header based on preference and tracking protection. r=mcmanus
https://hg.mozilla.org/integration/fx-team/rev/001698fcd787
Part 4: Testcase for DNT. r=francois

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4a2e5f30099
https://hg.mozilla.org/mozilla-central/rev/9df3db7f35d8
https://hg.mozilla.org/mozilla-central/rev/1311e22e3ab4
https://hg.mozilla.org/mozilla-central/rev/001698fcd787

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4a2e5f30099
https://hg.mozilla.org/mozilla-central/rev/9df3db7f35d8
https://hg.mozilla.org/mozilla-central/rev/1311e22e3ab4
https://hg.mozilla.org/mozilla-central/rev/001698fcd787

Comment 47

[Matt Wobensmith [:mwobensmith][:matt:]]

Tested using steps in comment 2. I looked at the various combinations of tracking protection, private browsing and DNT. I also verified the value of navigator.doNotTrack under the same conditions.

All seem to behave as expected. We are sending the DNT header in the cases we want to, and not sending it otherwise. In addition, the return value of that JS API reflects the same.

Verified on Nightly, 2016-06-08.

Comment 48

[Stas Sergeev]

And now how to disable DNT when TP
is enabled? Is there some way remained
at least via about:config?
The aforementioned loop-hole does not
exist if I set TP to always on (instead
of selected by the Disconnect list) and
DNT to always off. And there is no longer
a possibility to get such config?

Comment 49

[François Marier [:francois]]

(In reply to Stas Sergeev from comment #48)
> And now how to disable DNT when TP
> is enabled? Is there some way remained
> at least via about:config?

DNT cannot be disabled if TP is enabled. This is by design. See the "user story" field at the top of this bug.

Comment 50

[Stas Sergeev]

(In reply to François Marier [:francois] from comment #49)

> DNT cannot be disabled if TP is enabled. This is by design. See the "user
> story" field at the top of this bug.

And how about seeing my msg instead?
---
The aforementioned loop-hole does not
exist if I set TP to always on (instead
of selected by the Disconnect list) and
DNT to always off.
---

Not sure if I can explain this even clearer.
As always, useful config was removed for
no good reason.

Comment 51

[François Marier [:francois]]

(In reply to Stas Sergeev from comment #50)
> The aforementioned loop-hole does not
> exist if I set TP to always on (instead
> of selected by the Disconnect list) and
> DNT to always off.

I don't understand the configuration you're describing. Tracking protection relies on the list of trackers curated by Disconnect. That's how it determines whether or not a third-party resource will be blocked.

Comment 52

[Stas Sergeev]

(In reply to François Marier [:francois] from comment #51)
> I don't understand the configuration you're describing. Tracking protection
> relies on the list of trackers curated by Disconnect. That's how it
> determines whether or not a third-party resource will be blocked.

OK, let me explain the setup I look for.
https://disconnect.me/trackerprotection
---
Tracking is the collection of data regarding a particular user's activity across multiple websites or applications that aren’t owned by the data collector, and the retention, use or sharing of that data.
---

What I want is firefox to disallow the
aforementioned sharing of any data between
sites. For example the use of non-owned cookies.
In this case even if the tracker is removed
from Disconnect list, it will not be able to
track. So I can keep DNT turned off and there
is no aforementioned loop-hole.
Is such setup possible?

Comment 53

[François Marier [:francois]]

> ---
> Tracking is the collection of data regarding a particular user's activity across multiple websites
> or applications that aren’t owned by the data collector, and the retention, use or sharing of that data.
> ---
> What I want is firefox to disallow the aforementioned sharing of any data between sites.

That high-level goal is more-or-less what we are trying to do with our anti-tracking effort: https://blog.mozilla.org/futurereleases/2018/08/30/changing-our-approach-to-anti-tracking/

The hope is that once that work is complete, you'll be able to prevent tracking without turning ON tracking protection or DNT.

Comment 54

[Stas Sergeev]

> The hope is that once that work is complete, you'll be able to prevent tracking without turning ON tracking protection

I was assuming this is (or should be)
a part of the tracking protection...
Thanks for the clear answer!
Waiting for ff-65 then.