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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: francois, Assigned: Ehsan)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Flags: needinfo?(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.
Flags: needinfo?(francois)
Assignee: nobody → ehsan
Attachment #8999246 - Flags: review?(francois) → review+
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.
Attachment #8999245 - Flags: review?(francois) → review-
(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...
Attachment #8999245 - Attachment is obsolete: true
(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.
Attachment #8999306 - Flags: review?(francois) → review+
See Also: → 1482950
Pushed by eakhgari@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ecdc01374747
https://hg.mozilla.org/mozilla-central/rev/174bb3e3138c
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.