Issue 6091 - Add checks for overly generic antiadblock filters

lib/antiadblockInit.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-/* global publicSuffixes  */

[kzar, 2018/05/22 13:24:02]

Nit: Seems like we generally write "globals" instead of "global" in our code so
far.

[Jon Sonesen, 2018/05/22 21:25:18]

Done.

+ /* globals publicSuffixes */

[Thomas Greiner, 2018/05/30 10:18:03]

Detail: This whitespace appears to have been added by accident.

[kzar, 2018/05/30 10:57:48]

The newline? If so it seems consistent with how we did it elsewhere.

(More or less anyway. Apparently it varies a little if we write "globals" or
"global", if we put that above or below "use strict"; and if we add the newline
or not.)

[Thomas Greiner, 2018/05/30 12:30:09]

No, the whitespace character to the left of the comment.

[kzar, 2018/05/30 12:59:02]

Oh yea, I see what you mean now.

[Jon Sonesen, 2018/05/30 15:46:54]

Acknowledged.

 
 "use strict";
 
 const {Prefs} = require("prefs");
 const {ActiveFilter} = require("filterClasses");
 const {FilterStorage} = require("filterStorage");
 const {FilterNotifier} = require("filterNotifier");
 const {Subscription} = require("subscriptionClasses");
 const {Notification} = require("notification");
 
@@ skipping 17 matching lines @@
   function addAntiAdblockNotification(subscription)
   {
     let urlFilters = [];
     for (let filter of subscription.filters)
     {
       if (filter instanceof ActiveFilter && filter.domains)
       {
         for (let [domain, included] of filter.domains)
         {
           let urlFilter = "||" + domain + "^$document";
           if (domain && included && urlFilters.indexOf(urlFilter) == -1)

[kzar, 2018/05/22 13:24:02]

Seems kinda inefficient how we're doing urlFilters.indexOf each time. Perhaps we
should make urlFilters a Set, but then convert it to an Array when assigning it
to notification.urlFilters?

[Jon Sonesen, 2018/05/22 18:26:11]

I agree here, and was thinking about this. Was not sure if it counted as an
unrelated change though. @Thomas whats your opinion here?

[Thomas Greiner, 2018/05/22 18:43:29]

I don't know whether it'd be faster and, if so, by how much but it should be
easy to find out using jsPerf or other means.

The results may also depend on the amount of domains we have to process so note
that the antiadblockfilters.txt list currently contains less than 400 filters
with less than 300 domains.

           {
-            if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1)

[Jon Sonesen, 2018/05/18 20:16:18]

Is hardly an exhaustive list of TLDs but there are also quite a few so not sure
how many we should cover.

[Thomas Greiner, 2018/05/22 12:43:10]

All public suffixes (including TLDs) are listed in
https://publicsuffix.org/list/public_suffix_list.dat and
https://publicsuffix.org/list/effective_tld_names.dat but, for some reason, it
seems like "com" and others are not included in the lib/publicSuffixList.js file
we generate when building the extension (see
https://hg.adblockplus.org/buildtools/file/168dac24ad9e/publicSuffixListUpdat...).

Instead, we're filtering out lines which don't contain a dot and consider any
domain that doesn't contain a dot as a TLD (see
https://hg.adblockplus.org/adblockpluschrome/file/939bf6cdd435/lib/tldjs.js#l30).
Be careful, however, when checking for that because of FQDNs which end with a
dot but it looks like we're not checking for those there either.

[kzar, 2018/05/22 13:24:02]

Yes, I expected we would first check that the domain had at least one dot before
checking it wasn't present in the public suffix list. We definitely don't these
hard coded domains here.

I didn't consider the case of a trailing dot however. I did a test to see if
they can even show up and found they can. So Thomas is right, be careful to
handle those. (I found we don't handle trailing dots consistently, we strip them
for blocking filters but not element hiding filters, so I've opened #6690 to
discuss that.)

[Jon Sonesen, 2018/05/22 18:26:11]

Okay, thanks for that. Was not sure how we handled generic filters in other
places and this was just a "works for now" implementation as I wasn't 100% sure
how to proceed. Thank you guys for clearing it up.

+            if (/\.[^$]/.test(domain) && !publicSuffixes.hasOwnProperty(domain))

[kzar, 2018/05/30 10:57:48]

Please combine the two if statements. Also please do the publicSuffixes lookup
before the regexp test, I think it's cheaper. Also what about a case like
"co.uk."?

[Thomas Greiner, 2018/05/30 12:30:09]

Good call. This regex will indeed fail for multi-part TLDs.

Here are two suggestions I could think of for tackling this:
- /\..*[^.]$/.test(domain)
- !domain.endsWith(".") && domain.includes(".")

[kzar, 2018/05/30 12:59:02]

Well my concern was that "co.uk" is in the publicSuffixes Object but not
"co.uk.". But perhaps the regex was wrong too!
Perhaps you'd be better using .indexOf and then checking the index isn't either
-1 nor the length of the array-1?

Either way please check your code against these Jon:

Yes:

"foo.com", "foo.com.", "foo.co.uk", "foo.co.uk.", "foo.no-ip.co.uk",
"foo.no-ip.co.uk."

No:

"com", "com.", "co.uk", "co.uk.", "foo.no-ip.co.uk", "foo.no-ip.co.uk."

In fact this would probably be a good place to add some unit tests.

[Jon Sonesen, 2018/05/30 15:46:54]

Unit tests it is then!

             {
-              if (!(publicSuffixes.hasOwnProperty(domain)))

[Thomas Greiner, 2018/05/22 12:43:10]

Ideally, this logic would be part of `Filter.prototype.isGeneric()` because I'd
consider filters which apply to a large subset of domains (e.g. all "com"
domains) also as generic.

@Dave What's your take on that?

[kzar, 2018/05/22 13:24:02]

Well you're right that a filter for the domain "com" is a very generic filter
and therefore what you suggest makes sense. Unfortunately in the context of the
core code a filter being generic or specific means something else, which means
we can't do it in practice (without overhauling getSelectorsForDomain and other
core code).

[Thomas Greiner, 2018/05/22 17:48:59]

That's quite a pity. Nevermind then.

[Jon Sonesen, 2018/05/22 18:26:11]

Acknowledged.

-                urlFilters.push(urlFilter);
+              urlFilters.push(urlFilter);
             }
           }
         }
       }
     }
     notification.urlFilters = urlFilters;
     Notification.addNotification(notification);
     Notification.addQuestionListener(notification.id, notificationListener);
   }
 
@@ skipping 18 matching lines @@
     if (url in FilterStorage.knownSubscriptions && subscription.disabled)
       addAntiAdblockNotification(subscription);
     else
       removeAntiAdblockNotification();
   }
 
   FilterNotifier.on("subscription.updated", onSubscriptionChange);
   FilterNotifier.on("subscription.removed", onSubscriptionChange);
   FilterNotifier.on("subscription.disabled", onSubscriptionChange);
 };