Issue 6091 - Add checks for overly generic antiadblock filters

lib/antiadblockInit.js

Index: lib/antiadblockInit.js
===================================================================
--- a/lib/antiadblockInit.js
+++ b/lib/antiadblockInit.js
@@ -10,16 +10,18 @@
  * 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 */

[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");
@@ -47,17 +49,22 @@
     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 (/\.[^$]/.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);
   }