1345158 - Implement browser.privacy.trackingProtection API

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Bob Silverberg [:bsilverberg]]

This is a new setting for the privacy API which does not mirror an existing setting in Chrome's implementation. [1] 

There are two prefs in which we are interested:

privacy.trackingprotection.enabled (globally)
privacy.trackingprotection.pbmode.enabled (only in Private Browsing)

I have a few questions about the implementation:

1. Should this be put into an existing namespace, such as privacy.services, or should it be given a new namespace that indicates it is a setting only relevant for Firefox?

2. How should the two above prefs be settable? Do we need two separate settings for trackingprotection, or just one that accepts multiple values. For example, we might have one setting which accepts values that could mean:
1 - disabled globally
2 - enabled in private browsing only
3 - enabled globally

Does the above make sense, or should it work differently from that?

[1] https://developer.chrome.com/extensions/privacy

Comment 1

[François Marier [:francois]]

(In reply to Bob Silverberg [:bsilverberg] from comment #0)
> 1. Should this be put into an existing namespace, such as privacy.services,
> or should it be given a new namespace that indicates it is a setting only
> relevant for Firefox?

Another option to consider would be a new privacy.tracking.* namespace. This would allow us to expose other anti-tracking settings that are already in about:preferences (e.g. privacy.donottrackheader.enabled) or that we are testing/considering adding to about:preferences (e.g. privacy.resistFingerprinting).

I think there are a few addons already (NoScript? PrivacyBadger?) that toggle the Do Not Track pref.

> 2. How should the two above prefs be settable? Do we need two separate
> settings for trackingprotection, or just one that accepts multiple values.
> For example, we might have one setting which accepts values that could mean:
> 1 - disabled globally
> 2 - enabled in private browsing only
> 3 - enabled globally

The single setting with the above three values makes sense and matches the about:preferences UI in Nightly. The default is 2.

Comment 2

[Bob Silverberg [:bsilverberg]]

Thanks François, I like the suggestion of the privacy.tracking namespace. I'm going to go that route.

Comment 3

[Bob Silverberg [:bsilverberg]]

François, I have one more question about the relationship between privacy.trackingprotection.enabled and privacy.trackingprotection.pbmode.enabled.

If privacy.trackingprotection.enabled = true and privacy.trackingprotection.pbmode.enabled = false, does that still mean that tracking protection is enabled globally (i.e., the former overrides the latter)? Or does that mean that tracking protection is only enabled for non-private windows, and if so, wouldn't that need to be a fourth option above?

Comment 4

[François Marier [:francois]]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> If privacy.trackingprotection.enabled = true and
> privacy.trackingprotection.pbmode.enabled = false, does that still mean that
> tracking protection is enabled globally (i.e., the former overrides the
> latter)? Or does that mean that tracking protection is only enabled for
> non-private windows, and if so, wouldn't that need to be a fourth option
> above?

The global switch (privacy.trackingprotection.enabled) takes precendence over the private browsing one. If TP is enabled globally, then we ignore the private browsing pref. There's no way to have TP enabled only in normal windows and not in private browsing windows.

Comment 5

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1345158 - Implement browser.privacy.trackingProtection API

Review commit: https://reviewboard.mozilla.org/r/118212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118212/

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8845424 [details]
Bug 1345158 - Implement browser.privacy.trackingProtection API

https://reviewboard.mozilla.org/r/118212/#review121130

Without having looked closely at the code, I have two questions about the design:
1. Why have the private browsing setting embedded in the value when we have SettingScope (http://searchfox.org/mozilla-central/rev/b035220d0f939559f4f8cf7b9079bc12f6adfc35/toolkit/components/extensions/schemas/types.json#11)
2. Maybe this is a question for francois, does disabling tracking protection stop us from downloading ths list of trackers?  If so, I think this really belongs in privacy.services

Comment 7

[François Marier [:francois]]

(In reply to Andrew Swan [:aswan] from comment #6)
> 2. Maybe this is a question for francois, does disabling tracking protection
> stop us from downloading ths list of trackers?  If so, I think this really
> belongs in privacy.services

We also download the list of trackers if privacy.trackingprotection.annotate_channels is enabled. If both are disabled, then the list is not downloaded.

Comment 8

[Andrew Swan [:aswan]]

Hm, well browser.privacy.services.* is primarily about features that cause the browser to access external services.  tracking protection is one of those but the actual features is also obviously closely related to privacy, so I'm not sure what the right place is...

Comment 9

[François Marier [:francois]]

Comment on attachment 8845424 [details]
Bug 1345158 - Implement browser.privacy.trackingProtection API

https://reviewboard.mozilla.org/r/118212/#review124984

I can't speak for the correctness of the Web Extension code, but as far as I can tell, you are using the prefs correctly and the API makes sense to me.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8845424 [details]
Bug 1345158 - Implement browser.privacy.trackingProtection API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118212/diff/1-2/

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8845424 [details]
Bug 1345158 - Implement browser.privacy.trackingProtection API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118212/diff/2-3/

Comment 12

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #6)
> Comment on attachment 8845424 [details]
> Bug 1345158 - Implement browser.privacy.trackingProtection API
> 
> https://reviewboard.mozilla.org/r/118212/#review121130
> 
> Without having looked closely at the code, I have two questions about the
> design:
> 1. Why have the private browsing setting embedded in the value when we have
> SettingScope
> (http://searchfox.org/mozilla-central/rev/
> b035220d0f939559f4f8cf7b9079bc12f6adfc35/toolkit/components/extensions/
> schemas/types.json#11)

We could try to use SettingScope for this, but I am concerned that we'd only be supporting part of it. Maybe that's okay, but I am concerned it would just cause confusion for developers.

We don't currently support SettingScope for any of the privacy settings, so using it for just this one may cause confusion. Also, we could map what's currently being called "enabled_globally" to "regular", and we could map what's currently being called "enabled_private_browsing_only" to "incognito_persistent", but there would be inconsistencies with how SettingScope is meant to work.

For example, in Chrome, you can use SettingScope to set different values for regular and incognito_persistent scopes. For example you could set regular to enabled and incognito_persistent to disabled, but that wouldn't be possible in our scenario, because if "privacy.trackingprotection.enabled" is set to true, the value of "privacy.trackingprotection.pbmode.enabled" is ignored. I.e., you cannot enable tracking protection for just regular windows and not private windows.

For the same reason the SettingScope of "regular_only" wouldn't be possible. So we'd be sort of supporting SettingScope, partially, but, as I think I have illustrated, it would be confusing. It seems to me that it's much simpler to provide the three options as specific settings for browser.privacy.tracking.trackingProtectionEnabled. (Note that I changed it from browser.privacy.tracking.trackingProtection to browser.privacy.tracking.trackingProtectionEnabled, which I think makes more sense as a setting name).

> 2. Maybe this is a question for francois, does disabling tracking protection
> stop us from downloading ths list of trackers?  If so, I think this really
> belongs in privacy.services

I gather from the conversation above, and François' final comment, that I should go ahead with the privacy.tracking namespace that he suggested.

Comment 13

[Bob Silverberg [:bsilverberg]]

Mark has started working on a more extensive tracking protection API and has some ideas for naming that I think may be better than what I've proposed, so let's just put this on the back burner for now. If Mark ends up implementing it instead he can just take this patch and use most of the code therein.

Comment 14

[Mike Kaply [:mkaply]]

When you say "back burner", do you mean not making Firefox 57?

Comment 15

[Bob Silverberg [:bsilverberg]]

(In reply to Mike Kaply [:mkaply] from comment #14)
> When you say "back burner", do you mean not making Firefox 57?

No, that's not what I meant. I just meant I won't work on it anymore for now, and Mark can take over. I also removed aswan's r?

This is a pretty simple change, and most of the code has been written, so I see no reason why it cannot be implemented prior to 57. At this point I think that's somewhat dependent on Mark's availability to work on it among his other priorities.

Comment 17

[Jonathan Kingston [:jkt] he/him]

Is this dependent on the other experiment? As this will block the current behaviour of the lightbeam extension launching pre/on 57.

Comment 18

[Bob Silverberg [:bsilverberg]]

(In reply to Jonathan Kingston [:jkt] from comment #17)
> Is this dependent on the other experiment? As this will block the current
> behaviour of the lightbeam extension launching pre/on 57.

Do you mean https://github.com/mstriemer/tracking-protection-experiment? I see no reason why this has to depend on that. The two are unrelated except for the fact that they both involve tracking protection. Mark, do you have plans to implement this privacy setting independent of the work being done on the experiment?

Comment 19

[Jonathan Kingston [:jkt] he/him]

Yeah that experiment. Worst case scenario we can remove the functionality within the extension for the time being.

Comment 20

[Mark Striemer [:mstriemer]]

You're looking for bug 1342265, right?

The plan for the experiment was to collect feedback for now and likely look at implementing something post 57. I've poked around in the C++ code that handles tracking protection but haven't written any C++ since university so I'm not confident I'd be able to sneak a patch in exposing this in short order.

If you just need to know if something is considered a tracker, you could duplicate that logic. The experiment has an extension included with how the code would look without the experiment [1]. It isn't a whole lot more code but it did show that the experiment has a moderate performance benefit even without hooking into C++ code.

You'd include the Disconnect block list and Mozilla's public suffix list then you can check if a request is third party (public suffix list is used for this) and in the Disconnect block list. This is essentially what Tracking Protection does. It isn't perfect but should allow you to keep the functionality until we can expose this through webRequest.

Would that work in the interim?

[1] https://github.com/mstriemer/tracking-protection-experiment/blob/master/webRequest-extension/background.js

Comment 21

[Jonathan Kingston [:jkt] he/him]

That might be useful for one of the stretch goals to show a difference in the graph pre and post tracking protection. We could also follow your example like Blok does to actually block the requests too, however I think I would rather remove the toggle for tracking protection than ship all this code again in this extension.

I might be able to get time next quarter if you are still busy for just this API to flip the preference which doesn't seem like it interferers with the design of bug 1342265.

Comment 22

[Mark Striemer [:mstriemer]]

Ah, I think I misunderstood. I didn't know Lightbeam had a toggle for tracking protection. I thought you were looking for the full tracking protection/webRequest updates.

I'm currently looking at how we manage extensions taking control of preferences that user's have access to modifying. I'll make a note to get some UX input into how this will look and we should be fairly well setup to add support for this for 57.

Comment 23

[Jonathan Kingston [:jkt] he/him]

After you spoke to me about this code today I actually understand the patch. Is there anything else blocking this landing as it looks good to me. andym asked to ni you for this as it is used in Lightbeam, it's not 100% a blocker to launch however it is a current feature.

Comment 24

[Andy McKay]

I would suggest, that we file a follow on bug for Mark, exposing this in about:preferences and seperate the API from the UI.

Comment 25

[Bob Silverberg [:bsilverberg]]

Sorry, it seems like we keep pushing this bug back and forth. Mark, did you plan on implementing just the tracking protection toggle as per the attached patch? If this is important, and you don't think you'll get to it soon, I can pick my patch back up and land it as is.

Comment 26

[Mark Striemer [:mstriemer]]

This is next up on my list of things to do.

Comment 27

[Mark Striemer [:mstriemer]]

Attachment: Bug 1345158 - Implement privacy.websites.trackingProtectionMode

Review commit: https://reviewboard.mozilla.org/r/166750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166750/

Comment 28

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8895535 [details]
Bug 1345158 - Implement privacy.websites.trackingProtectionMode

https://reviewboard.mozilla.org/r/166750/#review172690

Looks good Mark. Just a couple of minor comments about the json schema.

::: toolkit/components/extensions/schemas/privacy.json:68
(Diff revision 1)
>      "namespace": "privacy.websites",
>      "description": "Use the <code>browser.privacy</code> API to control usage of the features in the browser that can affect a user's privacy.",
>      "permissions": ["privacy"],
> +    "types": [
> +      {
> +        "id": "trackingProtectionModes",

I think the id name is generally a singular noun as opposed to a plural noun.

::: toolkit/components/extensions/schemas/privacy.json:95
(Diff revision 1)
>          "description": "<strong>Available on Windows and ChromeOS only</strong>: If enabled, the browser provides a unique ID to plugins in order to run protected content. The value of this preference is of type boolean, and the default value is <code>true</code>.",
>          "unsupported": true
> +      },
> +      "trackingProtectionMode": {
> +        "$ref": "types.Setting",
> +        "description": "Allow users to specify the mode for tracking protection. This setting's value is of type trackingProtectionEnabledMode, defaulting to <code>private_browsing_only</code>."

The type name referred to in the description is not the same as the id used above in the json schema.

Comment 29

[Mark Striemer [:mstriemer]]

Comment on attachment 8895535 [details]
Bug 1345158 - Implement privacy.websites.trackingProtectionMode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166750/diff/1-2/

Comment 30

[Mark Striemer [:mstriemer]]

Andrew, I got Bob to take a first look at this but it's just a modified version of his patch so if you could give it a once over that would be good.

Comment 31

[Andrew Swan [:aswan]]

Comment on attachment 8895535 [details]
Bug 1345158 - Implement privacy.websites.trackingProtectionMode

https://reviewboard.mozilla.org/r/166750/#review173304

::: toolkit/components/extensions/ext-privacy.js:232
(Diff revision 2)
>                return Preferences.get("network.http.sendRefererHeader") !== 0;
>              }),
> +          trackingProtectionMode: getPrivacyAPI(extension,
> +            "websites.trackingProtectionMode",
> +            () => {
> +              if (Preferences.get("privacy.trackingprotection.enabled")) {

Hrm why are we using Preferences.jsm here?  We should be using Services.prefs instead.  But its already used all over the rest of this file so we can leave that for a separate task I guess...

::: toolkit/components/extensions/schemas/privacy.json:68
(Diff revision 2)
>      "namespace": "privacy.websites",
>      "description": "Use the <code>browser.privacy</code> API to control usage of the features in the browser that can affect a user's privacy.",
>      "permissions": ["privacy"],
> +    "types": [
> +      {
> +        "id": "trackingProtectionModeOption",

By convention type names start with a capital letter.

Comment 32

[Mark Striemer [:mstriemer]]

Comment on attachment 8895535 [details]
Bug 1345158 - Implement privacy.websites.trackingProtectionMode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166750/diff/2-3/

Comment 33

[Mark Striemer [:mstriemer]]

Comment on attachment 8895535 [details]
Bug 1345158 - Implement privacy.websites.trackingProtectionMode

https://reviewboard.mozilla.org/r/166750/#review173304

> Hrm why are we using Preferences.jsm here?  We should be using Services.prefs instead.  But its already used all over the rest of this file so we can leave that for a separate task I guess...

I filed bug 1390611 for this.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/610617beb53b
Implement privacy.websites.trackingProtectionMode r=aswan,bsilverberg

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/610617beb53b

Comment 36

[Will Bamberg [:wbamberg]]

Documented at: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/websites

Please let me know if this covers it!

Comment 37

[Bob Silverberg [:bsilverberg]]

(In reply to Will Bamberg [:wbamberg] from comment #36)
> Documented at:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/
> websites
> 
> Please let me know if this covers it!

Looks great, thanks Will.

Comment 38

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!