1482950 - Third-party checks for tracking annotations are too strict

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[François Marier [:francois]]

In bug 1476715, we added third-party checks to all of the users of tracking annotations, but we're now only checking for third-party _channels_, not third-party windows.

Therefore we've probably re-introduced bug 1108017.

This really needs a test otherwise we're likely to reintroduce this bug again in the future.

Comment 1

[François Marier [:francois]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb393ff4d294d8e171c29103d1b7b86a9702baa0

Comment 2

[François Marier [:francois]]

Attachment: Bug 1482950 - Backed out changeset 32d94a3cc7af. r=xeonchen!

This refactoring is now unnecessary since the third-party check
is getting removed.

Comment 3

[François Marier [:francois]]

Attachment: Bug 1482950 - Improve tracking annotation test. r=ehsan!

This expands the test to cover the case where annotations are enabled
by priority lowering isn't.

A new parameter is introduced to makeConnection in order to specify
the topWindowURI. This will be useful in future tests, but it also
highlights the fact that we do set this in all of the existing
tests.

Finally, I also added a number of comments and explicit parameter
setting in order to make the test more readily understandable.

Depends on D3720

Comment 4

[François Marier [:francois]]

Attachment: Bug 1482950 - Fix eslint errors in the tracking annotations test. r=dimi!

Depends on D3721

Comment 5

[François Marier [:francois]]

Attachment: Bug 1482950 - Use the correct 3rdparty check in tracking annotations. r=dimi!,ehsan!

The mIsTrackingResource flag on nsIHttpChannel was split into two separate
flags depending on whether or not the resource is third-party. The correct
flag will be set by the channel classifier. Similarly, a new function was
introduced, GetIsThirdPartyTrackingResource(), for those consumers (like TP)
who only care about third-party trackers.

The existing function, GetIsTracking(), will continue to look at both
first-party and third-party trackers (the behavior since first party
tracking was added to annotations in bug 1476324).

The OverrideTrackingResource() function now allows nsHTMLDocument to
override both mIsFirstPartyTrackingResource and
mIsThirdPartyTrackingResource, but since this function is a little dangerous
and only has a single user, I added an assert to make future callers think
twice about using it to opt out of tracking annotations.

Currently, only the default storage restrictions need to look at first-party
trackers so every other consumer has been moved to
mIsThirdPartyTrackingResource or GetIsThirdPartyTrackingResource().

This effectively reverts the third-party checks added in bug 1476715 and
replaces them with the more complicated check that was added in bug 1108017.
It follows the approach that Ehsan initially suggested in bug 1476715. It
also reverts the changes in the expected values of the tracking annotation
test since these were, in hindsight, a warning about this regression.

Depends on D3722

Comment 6

[François Marier [:francois]]

Comment on attachment 9002258 [details]
Bug 1482950 - Backed out changeset 32d94a3cc7af. r=xeonchen!

r? Gary for the FastBlock parts (straight revert of his previous patch that's no longer needed)

Comment 7

[François Marier [:francois]]

Comment on attachment 9002259 [details]
Bug 1482950 - Improve tracking annotation test. r=ehsan!

r? Ehsan for improvements to the tracking annotation test you created last year

Comment 8

[François Marier [:francois]]

Comment on attachment 9002260 [details]
Bug 1482950 - Fix eslint errors in the tracking annotations test. r=dimi!

r? dimi for pretty straighforward fixes to make the JS linter happy (no actual changes)

Comment 9

[François Marier [:francois]]

Comment on attachment 9002261 [details]
Bug 1482950 - Use the correct 3rdparty check in tracking annotations. r=dimi!,ehsan!

r? Ehsan for the changes relevant to the default storage restrictions
r? Dimi for the URL classifier logic
r? Honza for the necko changes

Comment 10

[François Marier [:francois]]

Better tests are planned in bug 1484493, but I have manually verified this using this new test page I created: https://itisatracker.org/bug1108017.html

Comment 11

[Gary Chen [:xeonchen]]

Comment on attachment 9002258 [details]
Bug 1482950 - Backed out changeset 32d94a3cc7af. r=xeonchen!

Gary Chen [:xeonchen] has approved the revision.

Comment 12

[Dimi Lee [:dimi][:dlee]]

Comment on attachment 9002260 [details]
Bug 1482950 - Fix eslint errors in the tracking annotations test. r=dimi!

Dimi Lee[:dimi][:dlee] has approved the revision.

Comment 13

[Dimi Lee [:dimi][:dlee]]

Comment on attachment 9002261 [details]
Bug 1482950 - Use the correct 3rdparty check in tracking annotations. r=dimi!,ehsan!

Dimi Lee[:dimi][:dlee] has approved the revision.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 9002261 [details]
Bug 1482950 - Use the correct 3rdparty check in tracking annotations. r=dimi!,ehsan!

Honza Bambas (:mayhemer) has approved the revision.

Comment 15

[(no longer active)]

Comment on attachment 9002259 [details]
Bug 1482950 - Improve tracking annotation test. r=ehsan!

:Ehsan Akhgari has approved the revision.

Comment 16

[(no longer active)]

Comment on attachment 9002261 [details]
Bug 1482950 - Use the correct 3rdparty check in tracking annotations. r=dimi!,ehsan!

:Ehsan Akhgari has approved the revision.

Comment 17

[(no longer active)]

Comment on attachment 9002259 [details]
Bug 1482950 - Improve tracking annotation test. r=ehsan!

Looks like Phabricator flags aren't synched properly... :-(

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe91a0b8bb4c
Backed out changeset 32d94a3cc7af. r=xeonchen!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e95533b40e1
Improve tracking annotation test. r=Ehsan!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd35c530c6e7
Fix eslint errors in the tracking annotations test. r=dimi!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/251360ecbedf
Use the correct 3rdparty check in tracking annotations. r=dimi,Ehsan,mayhemer!,ehsan!

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fe91a0b8bb4c
https://hg.mozilla.org/mozilla-central/rev/0e95533b40e1
https://hg.mozilla.org/mozilla-central/rev/bd35c530c6e7
https://hg.mozilla.org/mozilla-central/rev/251360ecbedf