Closed Bug 1505931 Opened Last year Closed 11 months ago

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

Categories

(Firefox :: Protections UI, defect)

65 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- unaffected
firefox65 + verified

People

(Reporter: tanvi, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [privacy65][triage])

Attachments

(3 files, 2 obsolete files)

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.
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?
Flags: needinfo?(francois)
Ehsan is right. That assumption no longer holds post-bug 1461515.
Flags: needinfo?(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.
(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.
Blocks: 1461515
Keywords: regression
Summary: Private Browsing Mode doesn't honor the list in the urlclassifier.trackingAnnotationTable → Tracking Protection enabled causes us to not honor the list in the urlclassifier.trackingAnnotationTable pref
As this is tracking everything current & upcoming, is someone working on this?
Flags: needinfo?(jhofmann)
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.)
We'll triage this in our team meeting next week when Francois is back.
Flags: needinfo?(jhofmann)
Wontfix for 63 as we have no plans for another dot release at the moment and 64 ships in 4 weeks.
We have decided to stick to the basic list until 65 or 66 so this bug doesn't need to be fixed until then.
Removing tracking for 63 and 64 per data in comment #9
Attached patch annotation.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
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
Attachment #9027987 - Flags: feedback+
Attachment #9027987 - Attachment is obsolete: true
Attached patch part 3 - tests (obsolete) — Splinter Review
Attachment #9028590 - Flags: review+
Attachment #9028592 - Flags: review+
Attachment #9028593 - Flags: review+
Attached patch part 3 -Splinter Review
Attachment #9028593 - Attachment is obsolete: true
Keywords: checkin-needed
: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"
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Pushed by amarchesini@mozilla.com:
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
Verified on Windows 10, Ubuntu 18, Mac OS, on Nightly 65.0a1 (2018-12-03).
Status: RESOLVED → VERIFIED
Flags: needinfo?(amarchesini)
Regressions: 1566825
No longer regressions: 1566825
You need to log in before you can comment on or make changes to this bug.