1505931 - Tracking Protection enabled causes us to not honor the list in the urlclassifier.trackingAnnotationTable pref

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect)

Description

[Tanvi Vyas[:tanvi]]

If you set urlclassifier.trackingAnnotationTable to the strict lit with value test-track-simple,base-track-digest256,content-track-digest256, then we should block third party cookies from parties that match the strict Tracking Protection list from Disconnect (the basic list plus the content category).

We do this in normal mode, but in private mode we don't block tracking cookies associated with the strict list when tracking protection is on the basic list.

Steps to reproduce:
1) Start with fresh profile using firefox 65 (Tracking Protection should be on in Private Windows only, and tracking cookies should be blocked).

2) Visit https://senglehardt.com/test/identity_providers/google.html in normal mode.  Observe the shield in the url bar and observer that tracking cookies are blocked.

3) Visit https://senglehardt.com/test/identity_providers/google.html in private mode.  

Expected results: Observer the shield in the url bar and observer that tracking cookies are blocked.

Actual results: Observer that there is no shield in the url bar.  Clicking on the lock shows you that no blockable content is detected.

Comment 1

[(no longer active)]

I think this is caused by this code: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/netwerk/base/nsChannelClassifier.cpp#439

When TP is enabled, we don't attempt to annotate channels separately.  Francois, is my understanding correct?

Comment 2

[François Marier [:francois]]

Ehsan is right. That assumption no longer holds post-bug 1461515.

Comment 3

[François Marier [:francois]]

I suspect this has nothing to do with Private Browsing and everything to do with the fact that TP is enabled, regardless of the mode.

Comment 4

[(no longer active)]

(In reply to François Marier [:francois] from comment #3)
> I suspect this has nothing to do with Private Browsing and everything to do
> with the fact that TP is enabled, regardless of the mode.

Yes, that's true.

Comment 5

[David Durst [:ddurst]]

As this is tracking everything current & upcoming, is someone working on this?

Comment 6

[(no longer active)]

I think Francois would be a good person to either pick it up or answer this question.  (He is on PTO this week so can't be needinfo'ed.)

Comment 7

[Johann Hofmann [:johannh]]

We'll triage this in our team meeting next week when Francois is back.

Comment 8

[Pascal Chevrel:pascalc]

Wontfix for 63 as we have no plans for another dot release at the moment and 64 ships in 4 weeks.

Comment 9

[François Marier [:francois]]

We have decided to stick to the basic list until 65 or 66 so this bug doesn't need to be fixed until then.

Comment 10

[Pascal Chevrel:pascalc]

Removing tracking for 63 and 64 per data in comment #9

Comment 11

[Andrea Marchesini [:baku]]

Attachment: annotation.patch

Comment 12

[François Marier [:francois]]

Comment on attachment 9027987 [details] [diff] [review]
annotation.patch

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

That's exactly how I would start. There might be other places where we assume that TP makes annotations unnecessary, but you'll probably need to find them in testing.

Specifically, I'd look for various combinations like:

- match on the TP and annotation blacklists (but not the whitelists)
- match on the TP blacklist but not the annotation list (not whitelisted)
- match on the annotation list only (not whitelisted)
- match on both blacklists but only whitelisted for TP
- match on both blacklists and only whitelisted for annotations
- match on both blacklists and whitelisted on both

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - annotation vs TP

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - pref check

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 3 - tests

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 3 -

Comment 17

[Cristian Brindusan [:cbrindusan]]

:baku, I've received hunk failed when applying this patch, can you please take a look?

"applying annotation.patch
patching file netwerk/base/nsChannelClassifier.cpp
Hunk #1 succeeded at 434 with fuzz 2 (offset 36 lines).
applying annotation_fix.patch
patching file toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
Hunk #1 FAILED at 1812
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/url-classifier/nsUrlClassifierDBService.cpp.rej"

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94594ae2fc1
Channel-Classifier should not skip annotation when TP is enabled, r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5c160d056e
URL-Classifier should consider all the prefs, r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/c466f72acdec
Tests for tracking annotation lists vs tracking protection lists, r=francois

Comment 19

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f94594ae2fc1
https://hg.mozilla.org/mozilla-central/rev/fe5c160d056e
https://hg.mozilla.org/mozilla-central/rev/c466f72acdec

Comment 20

[David Olah]

Verified on Windows 10, Ubuntu 18, Mac OS, on Nightly 65.0a1 (2018-12-03).