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  */
+ /* 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 16 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 domainIsTLD = !domain.includes(".") || domain.endsWith(".");

[kzar, 2018/05/25 13:51:56]

This logic doesn't look right to me, what about the example of
"dave.example.com.", that's definitely not a TLD.

[kzar, 2018/05/25 13:51:57]

This variable seems kind of pointless, when we only use it once and immediately
after we declare it.

[Jon Sonesen, 2018/05/25 21:04:39]

Hm, but there is the case where `com.` and `com` should be treated equally,
meaning it is considered a TLD. However in all other cases it isn't considered a
TLD? I am confused about what you guys want here. If it doesn't have a dot it's
generic but if it ends with a dot it isn't and yet "com" and "com." are treated
the same? This is seems contradictory  can you please clarify?

-          if (domainIsTLD || publicSuffixes.hasOwnProperty(domain))

[kzar, 2018/05/25 13:51:57]

If the domain ends in "." it won't be found in publicSuffixes, for example
"googlecode.com.".

[Jon Sonesen, 2018/05/25 21:04:39]

Acknowledged.

-            continue;
           let urlFilter = "||" + domain + "^$document";
           if (domain && included && urlFilters.indexOf(urlFilter) == -1)

[kzar, 2018/05/25 13:51:57]

It seems a shame we're doing these cheaper checks `domain && included` after
some more expensive ones like `domain.endsWith(".")`.

[Jon Sonesen, 2018/05/25 21:04:39]

Acknowledged.

           {
-            urlFilters.push(urlFilter);
+            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!

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