Closed Bug 1482950 Opened 2 years ago Closed 1 year ago

Third-party checks for tracking annotations are too strict

Categories

(Firefox :: Protections UI, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: francois, Assigned: francois)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files)

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.
Blocks: 1476715
Assignee: nobody → francois
Status: NEW → ASSIGNED
See Also: → 1480923
Blocks: 1484315
Depends on: 1484493
This refactoring is now unnecessary since the third-party check
is getting removed.
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
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 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)
Attachment #9002258 - Flags: review?(xeonchen)
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
Attachment #9002259 - Flags: review?(ehsan)
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)
Attachment #9002260 - Flags: review?(dlee)
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
Attachment #9002261 - Flags: review?(honzab.moz)
Attachment #9002261 - Flags: review?(ehsan)
Attachment #9002261 - Flags: review?(dlee)
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 on attachment 9002258 [details]
Bug 1482950 - Backed out changeset 32d94a3cc7af. r=xeonchen!

Gary Chen [:xeonchen] has approved the revision.
Attachment #9002258 - Flags: review+
Attachment #9002258 - Flags: review?(xeonchen) → review+
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.
Attachment #9002260 - Flags: review+
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.
Attachment #9002261 - Flags: review+
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.
Attachment #9002261 - Flags: review+
Comment on attachment 9002259 [details]
Bug 1482950 - Improve tracking annotation test. r=ehsan!

:Ehsan Akhgari has approved the revision.
Attachment #9002259 - Flags: review+
Comment on attachment 9002261 [details]
Bug 1482950 - Use the correct 3rdparty check in tracking annotations. r=dimi!,ehsan!

:Ehsan Akhgari has approved the revision.
Attachment #9002261 - Flags: review+
Comment on attachment 9002259 [details]
Bug 1482950 - Improve tracking annotation test. r=ehsan!

Looks like Phabricator flags aren't synched properly... :-(
Attachment #9002259 - Flags: review?(ehsan)
Attachment #9002261 - Flags: review?(ehsan)
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe91a0b8bb4c
Backed out changeset 32d94a3cc7af. r=xeonchen!
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e95533b40e1
Improve tracking annotation test. r=Ehsan!
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd35c530c6e7
Fix eslint errors in the tracking annotations test. r=dimi!
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/251360ecbedf
Use the correct 3rdparty check in tracking annotations. r=dimi,Ehsan,mayhemer!,ehsan!
Attachment #9002261 - Flags: review?(honzab.moz)
Attachment #9002261 - Flags: review?(dlee)
Attachment #9002260 - Flags: review?(dlee)
You need to log in before you can comment on or make changes to this bug.