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 19 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)
           {
-            if (domain.match(/\.[^$]/) && !publicSuffixes.hasOwnProperty(domain))

[Thomas Greiner, 2018/05/28 13:09:37]

Detail: We're only interested in whether the string matches so please use
`/\.[^$]/.test(domain)` instead.

See also
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[Jon Sonesen, 2018/05/29 20:31:43]

Acknowledged.

+            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);
 };