1710241 - Remove privacy.storagePrincipal.enabledForTrackers pref

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, task, P3)

Description

[Rob Wu [:robwu]]

The privacy.storagePrincipal.enabledForTrackers pref defaults to false and is used at https://searchfox.org/mozilla-central/rev/2f98b2305c5b4b180159b7d54cf298f50960b7d2/toolkit/components/antitracking/StorageAccess.cpp#364-366,381-383
This preference is not documented at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Privacy/State_Partitioning#disable_dynamic_state_partitioning

bug 1547813 introduced BEHAVIOR_REJECT_TRACKER_AND_PARTITION_FOREIGN (5), which seemingly removes the need for BEHAVIOR_REJECT_TRACKER (4) + privacy.storagePrincipal.enabledForTrackers.

To simplify code (and to match the behavior of the public documentation), I suggest to remove this privacy.storagePrincipal.enabledForTrackers pref. Users won't notice any difference since the pref is false by default. Even if they did use the pref, then they do have an alternative (cookieBehavior=5).

Comment 1

[Rob Wu [:robwu]]

Johann, I asked Arthur about whether this pref can be removed to simplify the implementation of bug 1669716. Apparently the pref is needed in the future. Could you share more details so I can determine how I should account for this pref in my work on bug 1669716?

Comment 2

[Johann Hofmann [:johannh]]

Hi Rob, sorry, my original thinking in our meeting was that we could use this pref to opt trackers into partitioning when using cookieBehavior 5 (BEHAVIOR_REJECT_TRACKER_AND_PARTITION_FOREIGN). That currently won't work the way it's implemented but it would probably be a small change. The alternative would be to use an entirely new cookieBehavior, which doesn't really make sense from a user's perspective. The combination cookieBehavior 4 + privacy.storagePrincipal.enabledForTrackers however will likely never be used.

Since there are no immediate plans to make use of the pref for CB 5 you may remove it, but we will end up wanting to support either such a pref or a new cookieBehavior in the future. How does the pref currently impact your work on bug 1669716, would it be different if it also applied to CB 5 and would things be easier if we had a different cookieBehavior instead?

Comment 3

[Rob Wu [:robwu]]

In bug 1669716 I'm trying to abstract the behavior of the internal cookie API implementation, and need to map from one extension-provided value to the internal origin attributes and back. The extension-provided value is passed via the firstPartyDomain in the extension API (the name came from FPI for which the feature was designed). To support dFPI, the extension API will take a URL and transform it into the (scheme,domain,port) tuple that is used internally as the value for partitionKey (based on prefs). This is basically the TL;DR of https://bugzilla.mozilla.org/show_bug.cgi?id=1669716#c10.

How does the pref currently impact your work on bug 1669716, would it be different if it also applied to CB 5 and would things be easier if we had a different cookieBehavior instead?

The reason for reusing the external extension-facing firstPartyDomain concept in the extension API as an abstraction over FPI+dFPI is simplicity. At a conceptual level they're both used to partition storage (and they're mutually exclusive). With the API, extensions can remove or edit cookies, but with an increasing number of prefs/options to control the partitioning logic, it becomes increasingly difficult to document/explain to extension developers how they should choose the value for the firstPartyDomain value when creating cookies.

If you have any ideas about how to make it easier for extension developers to choose the right firstPartyDomain/partitionKey, please let me know.

Comment 4

[Emma Zühlcke [:emz]]

Removing the pref itself and the production code that relies on it is relatively straightforward. However, we have a lot of test cases which depend on the pref. It's part of the test harness here: https://searchfox.org/mozilla-central/rev/3a8091d1c29473a0839ad7a5810028f41363fe2e/toolkit/components/antitracking/test/browser/storageprincipal_head.js#29

Tim, it looks like the test helper StoragePrincipalHelper exlusively tests BEHAVIOR_REJECT_TRACKER + privacy.storagePrincipal.enabledForTrackers. Do you think we can remove the whole helper and therefore all the test cases it enables?

Comment 5

[Emma Zühlcke [:emz]]

Attachment: Bug 1710241 - Remove privacy.storagePrincipal.enabledForTrackers pref. r=timhuang!

Comment 6

[Tim Huang[:timhuang]]

I think 'privacy.storagePrincipal.enabledForTrackers' is a by-product on the ways we moved to BEHAVIOR_REJECT_TRACKER_AND_PARTITION_FOREIGN from BEHAVIOR_REJECT_TRACKER. From the testing standpoint, DynamicFPIHelper has cover the testing of storage principal. So, it should be fine to remove test cases for StoragePrincipalHelper.

However, I am not sure if we want to provide partitioned Storage for trackers in the future. This is a product question. If we won't, we can definitely remove the pref and the tests. Otherwise, we should keep them.

Johann, do you think it is possible that we enable partitioned Storage for trackers in the future?

Comment 7

[Johann Hofmann [:johannh]]

Yes, I think we'd definitely want to move to a consistent treatment of all origins, tracker or not, eventually. And we will likely want to have a pref for that. However, looking at the code, this pref was only in effect for cookieBehavior 4. So I would be fine with either solution:

• Rewrite this code to apply to CB 5, or
• Remove the pref and add something similar back in later, when we want to ship tracker partitioning with CB 5

Does that answer your question?

Comment 8

[Emma Zühlcke [:emz]]

In that case I think we can remove the pref + test. It's not a big architectural change so we can always bring it back if needed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97ad1ef597a9
Remove privacy.storagePrincipal.enabledForTrackers pref. r=timhuang

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/97ad1ef597a9