1480923 - Remove the checks for 3rd-party storage restrictions from the channel classifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

Once bug 1476715 has landed, all of the users of tracking annotations will be responsible for checking whether loads are first-party or third-party.

We should simplify the logic in the channel classifier by removing all of the checks for StaticPrefs::privacy_restrict3rdpartystorage_enabled() or AntiTrackingCommon::ShouldActivate().

This will mean that the 3rd-party storage restrictions, like network.http.tailing.enabled and privacy.trackingprotection.lower_network_priority will be gated by privacy.trackingprotection.annotate_channels.

Comment 1

[(no longer active)]

Can you please explain more about what the existing checks need to be replaced with?

Simply removing the checks doesn't work, because of the logic in nsChannelClassifier::ShouldEnableTrackingProtectionInternal().  That logic currently ensures that we never annotate first-party channels under any circumstances, which is the reason why we need those pref checks.

Comment 2

[François Marier [:francois]]

(In reply to :Ehsan Akhgari from comment #1)
> Can you please explain more about what the existing checks need to be
> replaced with?
> 
> Simply removing the checks doesn't work, because of the logic in
> nsChannelClassifier::ShouldEnableTrackingProtectionInternal().  That logic
> currently ensures that we never annotate first-party channels under any
> circumstances, which is the reason why we need those pref checks.

Now that bug 1476715 has landed, we can remove the centralized third-party check from ShouldEnableTrackingProtection() when aAnnotationsOnly==true since that third-party check has been distributed across all of the current users.

This means that the default cookie restrictions annotations are no longer special.

Comment 3

[(no longer active)]

Attachment: Part 1: Remove the centralized third-party checks from nsChannelClassifier::ShouldEnableTrackingProtectionInternal

Comment 4

[(no longer active)]

Attachment: Part 2: Remove the checks for the restrict3rdpartystorage pref from the channel classifier and only rely on the annotate_channels pref

Comment 5

[François Marier [:francois]]

Comment on attachment 8999245 [details] [diff] [review]
Part 1: Remove the centralized third-party checks from nsChannelClassifier::ShouldEnableTrackingProtectionInternal

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

Bug 1476715 only added third-party checks to the consumers of tracking annotations. So if we take the check out entirely, we break tracking _protection_.

Something else I just realized about https://hg.mozilla.org/mozilla-central/rev/cb8a7a9f4a73: we're only checking for third-party _channels_, not third-party windows. Therefore we've probably re-introduced bug 1108017. Sadly we never landed a test to go with Monica's patch so that's probably why we didn't notice.

Comment 6

[(no longer active)]

(In reply to François Marier [:francois] from comment #5)
> Comment on attachment 8999245 [details] [diff] [review]
> Part 1: Remove the centralized third-party checks from
> nsChannelClassifier::ShouldEnableTrackingProtectionInternal
> 
> Review of attachment 8999245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bug 1476715 only added third-party checks to the consumers of tracking
> annotations. So if we take the check out entirely, we break tracking
> _protection_.

I see.

> Something else I just realized about
> https://hg.mozilla.org/mozilla-central/rev/cb8a7a9f4a73: we're only checking
> for third-party _channels_, not third-party windows. Therefore we've
> probably re-introduced bug 1108017. Sadly we never landed a test to go with
> Monica's patch so that's probably why we didn't notice.

Can you please file a separate regression bug for that?  That should probably be tracked for the 63 release as a regression...

Comment 7

[(no longer active)]

Attachment: Part 1: Remove the centralized third-party checks from nsChannelClassifier::ShouldEnableTrackingProtectionInternal in annotation-only mode

Comment 8

[François Marier [:francois]]

(In reply to :Ehsan Akhgari from comment #6)
> Can you please file a separate regression bug for that?  That should
> probably be tracked for the 63 release as a regression...

I just filed bug 1482950 for it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdc01374747
Part 1: Remove the centralized third-party checks from nsChannelClassifier::ShouldEnableTrackingProtectionInternal in annotation-only mode; r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/174bb3e3138c
Part 2: Remove the checks for the restrict3rdpartystorage pref from the channel classifier and only rely on the annotate_channels pref; r=francois

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ecdc01374747
https://hg.mozilla.org/mozilla-central/rev/174bb3e3138c