Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record third-party pings (navigator.sendBeacon) as tracking #2024

Closed
wants to merge 15 commits into from
Closed

Record third-party pings (navigator.sendBeacon) as tracking #2024

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1ef8380
Record third-party pings as tracking.
ghostwords May 15, 2018
449e2ca
Update ping/beacon listener for Firefox.
ghostwords May 16, 2018
b248e5d
Add test for beacon detection.
ghostwords May 17, 2018
b4a55be
Merge branch 'master'
ghostwords Sep 17, 2018
390895c
Remove unnecessary check.
ghostwords Sep 17, 2018
49524a8
Add TODO.
ghostwords Sep 17, 2018
c653a9f
Move pingAccounting definition within file.
ghostwords Sep 17, 2018
899b632
Fix beacon detection test.
ghostwords Sep 17, 2018
91eeb6c
Update listener comments.
ghostwords Sep 17, 2018
cad6368
Merge branch 'master'
ghostwords Oct 16, 2018
3914269
Merge branch 'master'.
ghostwords Dec 21, 2018
ef45715
Merge branch 'master'.
ghostwords Jul 3, 2019
6a65aba
Merge branch 'master'.
ghostwords Aug 20, 2019
803a8f8
Move beacon test from GitCDN to EFF's CDN.
ghostwords Aug 20, 2019
61e3fd2
Take MDFP into account for beacon tracking.
ghostwords Aug 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/js/heuristicblocking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ HeuristicBlocker.prototype = {
let tabOrigin = tabOrigins[details.tabId];

// ignore first-party requests
// TODO fix to use MDFP
if (!tabOrigin || origin == tabOrigin) {
return {};
}
Expand Down Expand Up @@ -157,6 +158,31 @@ HeuristicBlocker.prototype = {
);
},

/**
* Record third-party pings as tracking.
*/
pingAccounting: function (details) {
const fqdn = (new URI(details.url)).host,
origin = window.getBaseDomain(fqdn),
tab_origin = tabOrigins[details.tabId];
Copy link
Member Author

@ghostwords ghostwords May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably susceptible to navigation-related misattribution (tabOrigins has the new site's URL instead of the site that originated the ping; similar to #1997), but this may be OK for now (problem exists elsewhere, no need to block on the fix if the new feature works well enough).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an issue for the ping attribute on hyperlinks. For example, visit google.com in chrome, make a search, and click on any outbound link. For me, most of the time clicking on foo.com adds that domain as an entry for google.com in the snitch_map. Since the ping request fires off simultaneously with the user navigating away from Google, this bug is much more likely to come up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but is that a blocking issue, practically speaking? Privacy Badger is very likely to learn to block google.com anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in general the navigation bug means we are likely to record first-party pings as third-party tracking. Or third-party pings as third-party on the wrong party.

Copy link
Contributor

@bcyphers bcyphers May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not a big deal for google in particular, but my concern is that it will happen to any site that uses the ping attr on links. Maybe that's not a bad thing? If we decide that we want to consider ping on outgoing links to be a tracking action, then this could be a feature, not a bug.

The wrong-third-party scenario would end up messing up the snitch_map for a tracking domain, but not the tracking domain itself, right?

Copy link
Member Author

@ghostwords ghostwords May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this navigation-related attribution bug doesn't affect the tracking domain, just where it was seen. We could attribute the tracking domain to the wrong site domain, the one the user just navigated to. #1997 is waiting on information from Chromium devs at the moment.


// ignore first-party requests
// TODO fix to use MDFP
ghostwords marked this conversation as resolved.
Show resolved Hide resolved
if (!tab_origin || origin == tab_origin) {
return {};
}

// abort if we already made a decision for this FQDN
const action = this.storage.getAction(fqdn);
if (action != constants.NO_TRACKING && action != constants.ALLOW) {
return {};
}

this._recordPrevalence(fqdn, origin, tab_origin);

return {};
},

/**
* Record HTTP request prevalence. Block a tracker if seen on more
* than constants.TRACKING_THRESHOLD pages
Expand Down Expand Up @@ -510,7 +536,20 @@ function hasCookieTracking(details, origin) {
return false;
}

function pingListener(details) {
if (badger) {
return badger.heuristicBlocking.pingAccounting(details);
} else {
return {};
}
}

function startListeners() {
chrome.webRequest.onBeforeRequest.addListener(
pingListener,
{ types: ["ping"], urls: ["<all_urls>"] }
);

/**
* Adds heuristicBlockingAccounting as listened to onBeforeSendHeaders request
*/
Expand Down