Add migration to fix yellowlisted yet blocked domains #1508

ghostwords · 2017-07-17T20:48:21Z

Fixes #1466, fixes #1469, fixes #1494, fixes #1495.

Follows up on #1482.

ghostwords · 2017-07-17T20:54:36Z

To add to #1474 (comment), yellowlist domain removal logic marked subdomains (camo.githubusercontent.com) of each removed domain (githubusercontent.com) as blocked if they were in action_map (regardless of their heuristicAction), while yellowlist domain addition logic marked blocked domains (githubusercontent.com) and their base domains as "cookieblocked", but didn't do anything about the subdomains.

This PR should fix the affected subdomains by looking for blocked, yellowlisted domains and then fixing their status to one of "cookieblock", "allow" or "", depending on snitch_map contents.

cowlicks

Looks good, I was concerned about performance, but I tested it out and it seemed fine.

cowlicks · 2017-07-17T22:14:26Z

I restarted the failing travis job. I'll merge if it passes.

ghostwords · 2017-07-17T22:28:57Z

Note that this is still not a perfect reversal of the incident:

Previously non-tracking domains ~~(not their basedomains, which this PR addresses)~~ on the yellowlist are now yellowlisted. (Not too worried about this one.)
Current yellowlist domain removal logic blocked domains if they ended with the removed domain. Which is different from being the removed domain's subdomain ...

ghostwords · 2017-07-18T04:11:06Z

Looking through error reports, it looks like we do have some substring-blocked domains, for example, jse.trivago.com (because of go.com), onesignal.com (because of al.com), appleid.cdn-apple.com (because of apple.com). I'll update the migration to cover all of these too.

ghostwords · 2017-07-18T15:49:22Z

I expanded the scope of the migration to try to undo everything we might have blocked by mistake when we blanked out people's yellowlists.

The updated migration goes through every blocked domain and checks if the domain (jse.trivago.com) ends with any yellowlisted domains (go.com). If it does, we check snitch_map to see if we have no entries (heuristicAction becomes ""), one or two entries (heuristicAction becomes "allow"), or three or more entries.

If we have three or more entries in snitch_map, we call blacklistOrigin, which takes care of updating the domain and its base domain either to "block" or "cookieblock".

If there are bugs in this logic, I expect they would be in the direction of unblocking too much, which should be OK since Badger will relearn to block as necessary.

The updated migration checks every blocked domain to see if it ends with any yellowlisted domains. This mirrors current yellowlist domain removal logic, which checks if any known domains end with the removed-from-yellowlist domain.

cowlicks · 2017-07-18T17:39:39Z

Question: What is the policy for subdomains of domains on the CBL? What is the policy of parent domains on the CBL? What is the policy when a domain on the CBL is also in the PSL?

I think the answer to this should be documented somewhere.

Reviewing now.

ghostwords · 2017-07-18T19:05:43Z

When we block a domain (the correct way, via blacklistOrigin) now, we "cookieblock" the domain (instead of blocking) when the domain, or any of its subdomains down to the base domain are on the yellowlist. We also "cookieblock" those subdomains.

When we add a domain to the yellowlist and its base domain is already blocked or cookieblocked, we cookieblock that domain right away.

So, subdomains of yellowlisted domains should end up getting "cookieblocked".

ghostwords · 2017-07-18T19:10:30Z

The PSL is used when we get the base domain. The base domain of ajax.googleapis.com is ajax.googleapis.com, not googleapis.com, because googleapis.com is on the PSL.

ghostwords · 2017-07-18T19:46:43Z

However, blacklistOrigin calls utils.explodeSubdomains with true for the second argument,
which avoids using the PSL, which means blocking fonts.googleapis.com ends up cookieblocking the domain even though its base domain (fonts.googleapis.com) is not on the yellowlist.

ghostwords · 2017-07-18T19:50:15Z

Some bits use the PSL, some do not; the various bookkeeping structures can drift apart; importing data loses snitch_map state for non-""/"allow" heuristicAction domains ...

Just recheck all blocked domains, don't even look at the yellowlist.

cowlicks · 2017-07-18T23:11:42Z

I think there is a problem.

If we have a site on the cookieblock list like consent.google.com, with 3 snitch map entries. Then we also have a site like google.com. Suppose they are both blocked. When we run this migration, google.com may either be blocked, or unblocked, depending on the order.

If google.com is run first, then it will get blocked later when consent.google.com is put through blacklistOrigin. If it is run 2nd, it will be unblocked.

Is this correct? If so which way should it be?

ghostwords · 2017-07-19T16:34:22Z

~~We don't actually have google.com in the yellowlist; we do have www.google.com, but that would not run into this, right.~~ Oh, I think I see, this happens when the base domain is not on the yellowlist.
Have you been able to reproduce this potential issue in your testing?
If this is indeed an issue (still need to figure out if it is), is the fallout of this edge case that a base domain may stay incorrectly blocked? Meaning we still fix all these other domains except for this one edge case.
How many domains on the yellowlist might be affected by this?

ghostwords · 2017-07-19T17:49:41Z

Wouldn't setupSubdomainsForCookieblock take care of this? The function loops over every yellowlisted domain (cse.google.com, etc.), gets its base domain (google.com) and compares it to the passed-in base domain (google.com). If they are equal, the base domain gets cookieblocked.

cowlicks · 2017-07-19T17:53:45Z

@ghostwords yeah you are correct.

It seems like there would still be an issue, should it be cookieblocked or noaction/allow?

ghostwords · 2017-07-19T17:57:15Z

As I wrote above, previously non-tracking, yellowlist-associated domains are now yellowlisted for affected Badgers. I think this is OK, something that we can deal with later, if it actually becomes an issue.

cowlicks · 2017-07-19T18:01:50Z

@ghostwords with the current PR as-is, we don't know if the domain would be set to noaction/allow or cookieblock. It depends on the order that these get run in.

ghostwords · 2017-07-19T18:03:25Z

Both are acceptable, although have you verified this actually happens? See my comments above please.

cowlicks · 2017-07-19T18:20:13Z

I realize that both are better than incorrectly blocked. However I'd like it to be at least deterministic. Can we just decide on one to be correct and go with that?

I'm working on verifying this issue.

cowlicks · 2017-07-19T21:58:21Z

@ghostwords do you still plan on adding tests?

ghostwords · 2017-07-19T22:11:37Z

I did, here: #1515 (comment)

Add migration to fix yellowlisted, blocked domains

9d4d846

ghostwords assigned cowlicks and unassigned cowlicks Jul 17, 2017

ghostwords requested a review from cowlicks July 17, 2017 20:49

cowlicks approved these changes Jul 17, 2017

View reviewed changes

andresbase added this to the W28-29 milestone Jul 18, 2017

ghostwords requested a review from cowlicks July 18, 2017 15:41

Expand migration to cover more domains.

37abdf8

The updated migration checks every blocked domain to see if it ends with any yellowlisted domains. This mirrors current yellowlist domain removal logic, which checks if any known domains end with the removed-from-yellowlist domain.

ghostwords force-pushed the fix-blocked-domains-on-yellowlist branch from a3e00c1 to 37abdf8 Compare July 18, 2017 16:07

Expand migration to cover even more domains.

2eed4cc

Just recheck all blocked domains, don't even look at the yellowlist.

ghostwords mentioned this pull request Jul 19, 2017

Document scoping rules taking action on a domain #1515

Closed

cowlicks merged commit b66356f into master Jul 19, 2017

ghostwords deleted the fix-blocked-domains-on-yellowlist branch July 19, 2017 23:13

ghostwords mentioned this pull request Aug 22, 2017

Add solvemedia.com to yellowlist #1554

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migration to fix yellowlisted yet blocked domains #1508

Add migration to fix yellowlisted yet blocked domains #1508

ghostwords commented Jul 17, 2017 •

edited

Loading

ghostwords commented Jul 17, 2017

cowlicks left a comment

cowlicks commented Jul 17, 2017

ghostwords commented Jul 17, 2017 •

edited

Loading

ghostwords commented Jul 18, 2017 •

edited

Loading

ghostwords commented Jul 18, 2017

cowlicks commented Jul 18, 2017

ghostwords commented Jul 18, 2017

ghostwords commented Jul 18, 2017

ghostwords commented Jul 18, 2017 •

edited

Loading

ghostwords commented Jul 18, 2017

cowlicks commented Jul 18, 2017

ghostwords commented Jul 19, 2017 •

edited

Loading

ghostwords commented Jul 19, 2017

cowlicks commented Jul 19, 2017 •

edited

Loading

ghostwords commented Jul 19, 2017

cowlicks commented Jul 19, 2017

ghostwords commented Jul 19, 2017

cowlicks commented Jul 19, 2017

cowlicks commented Jul 19, 2017

ghostwords commented Jul 19, 2017

Add migration to fix yellowlisted yet blocked domains #1508

Add migration to fix yellowlisted yet blocked domains #1508

Conversation

ghostwords commented Jul 17, 2017 • edited Loading

ghostwords commented Jul 17, 2017

cowlicks left a comment

Choose a reason for hiding this comment

cowlicks commented Jul 17, 2017

ghostwords commented Jul 17, 2017 • edited Loading

ghostwords commented Jul 18, 2017 • edited Loading

ghostwords commented Jul 18, 2017

cowlicks commented Jul 18, 2017

ghostwords commented Jul 18, 2017

ghostwords commented Jul 18, 2017

ghostwords commented Jul 18, 2017 • edited Loading

ghostwords commented Jul 18, 2017

cowlicks commented Jul 18, 2017

ghostwords commented Jul 19, 2017 • edited Loading

ghostwords commented Jul 19, 2017

cowlicks commented Jul 19, 2017 • edited Loading

ghostwords commented Jul 19, 2017

cowlicks commented Jul 19, 2017

ghostwords commented Jul 19, 2017

cowlicks commented Jul 19, 2017

cowlicks commented Jul 19, 2017

ghostwords commented Jul 19, 2017

ghostwords commented Jul 17, 2017 •

edited

Loading

ghostwords commented Jul 17, 2017 •

edited

Loading

ghostwords commented Jul 18, 2017 •

edited

Loading

ghostwords commented Jul 18, 2017 •

edited

Loading

ghostwords commented Jul 19, 2017 •

edited

Loading

cowlicks commented Jul 19, 2017 •

edited

Loading