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/>.
  */
 
+/* globals publicSuffixes  */
+
 "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");
 
 exports.initAntiAdblockNotification = function initAntiAdblockNotification()
@@ skipping 17 matching lines @@
   {
     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)
-            urlFilters.push(urlFilter);
+          {
+            if (domain.indexOf(".") != -1 || domain.endsWith("."))

[Thomas Greiner, 2018/05/23 13:45:54]

Effectively, this logic leads to the following results:

  example.com  true or false  == true
  example.com. true or true   == true
  com          false or false == false
  com.         false or true  == true

However, we wanted to treat "com" and "com." the same here so shouldn't it
instead be `domain.indexOf(".") != -1 || !domain.endsWith(".")`?

[Thomas Greiner, 2018/05/23 13:45:54]

Suggestion: Double negation usually makes code quite hard to read (i.e. `!= -1`
here means "does not not exist") so rewriting it to the following could help
making it a bit easier to understand:

  let firstDot = domain.indexOf(".");
  if (firstDot == -1 || firstDot == domain.length - 1)
    continue;

Alternatively, you could use the regular expression `/\.[^$]/` to check whether
the domain contains a dot that's not at the end of the string.

[kzar, 2018/05/23 14:41:45]

Also please can you use `.includes(".")` instead of `.indexOf(".") != -1` ?

[Jon Sonesen, 2018/05/23 23:12:51]

For sure

[Jon Sonesen, 2018/05/23 23:12:51]

Acknowledged.

[Jon Sonesen, 2018/05/23 23:12:52]

Hey,

My bad I think I misunderstood the last comments reading comprehension faaiil.
Thanks, will do.

[Jon Sonesen, 2018/05/24 00:00:09]

Actually, on second thought would it be better to use `.startsWith(".")` here?

[kzar, 2018/05/24 11:28:57]

I'm not sure I understand the question. Why do you consider using startsWith?

+            {
+              if (!(publicSuffixes.hasOwnProperty(domain)))
+                urlFilters.push(urlFilter);
+            }
+          }
         }
       }
     }
     notification.urlFilters = urlFilters;
     Notification.addNotification(notification);
     Notification.addQuestionListener(notification.id, notificationListener);
   }
 
   function removeAntiAdblockNotification()
   {
@@ skipping 16 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);
 };