Noissue - Do not add subdomain wildcard if subdomain excluded

lib/abp2blocklist.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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
@@ skipping 272 matching lines @@
 
   if (included.length > 0)
   {
     trigger["if-domain"] = [];
 
     for (let name of included)
     {
       // If this is a blocking filter or an element hiding filter, add the
       // subdomain wildcard only if no subdomains have been excluded.
       let notSubdomains = null;
       if ((filter instanceof filterClasses.BlockingFilter ||

[Sebastian Noack, 2017/05/21 20:56:13]

Ẁhy do we have to explicitly check for BlockingFilter/ElemHideFilter here?

[Manish Jethani, 2017/05/21 21:49:05]

Because what is desired in the case of whitelisting filters is the exact
opposite. For example, "@@foo$domain=bar.com|~evil.bar.com" should still
generate "if-domain='*bar.com'". i.e. we want to err on the side of not blocking
rather than blocking.

Correct me if I'm wrong.

[Sebastian Noack, 2017/05/22 08:06:11]

That might be a good point. It seems to be better to let some ads through
occasionally than potentially blocking too much breaking some website. Perhaps
the comment here should explain this though.

            filter instanceof filterClasses.ElemHideFilter) &&
           (notSubdomains = findSubdomainsInList(name, excluded)).length > 0)
       {
         trigger["if-domain"].push(name);
 
         // Add the "www" prefix but only if it hasn't been excluded.
-        if (!notSubdomains.some(name => name == "www"))

[Sebastian Noack, 2017/05/21 20:56:13]

Is this check actually correct? What if the domain is foo.www.example.com?

[Manish Jethani, 2017/05/21 21:49:05]

Can you give a more complete example of where this might fail?

[Sebastian Noack, 2017/05/22 08:06:11]

So in case of a filter like this:

$domain=example.com|~foo.www.example.com

as far as I understand we wouldn't add www.example.com, though doing so wouldn't
hurt a we don't add a wildcard, right?

Or asked differently, any reason to not just use `notSubdomains[0] != "www"`?

[Manish Jethani, 2017/05/22 10:37:39]

In this case the code would be comparing with "foo.www" rather than just "www",
so it would go ahead and add the "www." prefix.

[Sebastian Noack, 2017/05/22 11:08:59]

Alright. But I think it still can be slightly simplified by using includes()
instead of some().

[Manish Jethani, 2017/05/22 16:07:08]

Done.

+        if (!notSubdomains.includes("www"))
           trigger["if-domain"].push("www." + name);
       }
       else
       {
         trigger["if-domain"].push("*" + name);
       }
     }
   }
   else if (excluded.length > 0)
   {
@@ skipping 71 matching lines @@
   {
     newSelector.push(selector.substring(i, pos.start));
     newSelector.push('[id=', selector.substring(pos.start + 1, pos.end), ']');
     i = pos.end;
   }
   newSelector.push(selector.substring(i));
 
   return newSelector.join("");
 }
 
+function addCSSRules(rules, selectors, matchDomain)
+{
+  while (selectors.length)
+  {
+    let selector = selectors.splice(0, selectorLimit).join(", ");
+
+    // As of Safari 9.0 element IDs are matched as lowercase. We work around
+    // this by converting to the attribute format [id="elementID"]
+    selector = convertIDSelectorsToAttributeSelectors(selector);
+
+    rules.push({
+      trigger: {"url-filter": matchDomain,
+                "url-filter-is-case-sensitive": true},
+      action: {type: "css-display-none",
+               selector: selector}
+    });
+  }
+}
+
 let ContentBlockerList =
 /**
  * Create a new Adblock Plus filter to content blocker list converter
  *
  * @constructor
  */
 exports.ContentBlockerList = function ()
 {
   this.requestFilters = [];
   this.requestExceptions = [];
   this.elemhideFilters = [];
   this.elemhideExceptions =  [];
+  this.generichideExceptions = [];
   this.elemhideSelectorExceptions = new Map();
 };
 
 /**
  * Add Adblock Plus filter to be converted
  *
  * @param {Filter} filter Filter to convert
  */
 ContentBlockerList.prototype.addFilter = function(filter)
 {
   if (filter.sitekeys)
     return;
   if (filter instanceof filterClasses.RegExpFilter &&
       filter.regexpSource == null)
     return;
 
   if (filter instanceof filterClasses.BlockingFilter)
     this.requestFilters.push(filter);
 
   if (filter instanceof filterClasses.WhitelistFilter)
   {
     if (filter.contentType & (typeMap.DOCUMENT | whitelistableRequestTypes))
       this.requestExceptions.push(filter);
 
-      if (filter.contentType & typeMap.ELEMHIDE)
-        this.elemhideExceptions.push(filter);
+    if (filter.contentType & typeMap.ELEMHIDE)
+      this.elemhideExceptions.push(filter);
+    else if (filter.contentType & typeMap.GENERICHIDE)
+      this.generichideExceptions.push(filter);
   }
 
   if (filter instanceof filterClasses.ElemHideFilter)
     this.elemhideFilters.push(filter);
 
   if (filter instanceof filterClasses.ElemHideException)
   {
     let domains = this.elemhideSelectorExceptions[filter.selector];
     if (!domains)
       domains = this.elemhideSelectorExceptions[filter.selector] = [];
 
     parseDomains(filter.domains, domains, []);
   }
 };
 
 /**
  * Generate content blocker list for all filters that were added
  *
  * @returns   {Filter}   filter    Filter to convert
  */
 ContentBlockerList.prototype.generateRules = function(filter)
 {
   let rules = [];
 
+  let genericSelectors = [];
   let groupedElemhideFilters = new Map();
+
   for (let filter of this.elemhideFilters)
   {
     let result = convertElemHideFilter(filter, this.elemhideSelectorExceptions);
     if (!result)
       continue;
 
     if (result.matchDomains.length == 0)
-      result.matchDomains = ["^https?://"];
-
-    for (let matchDomain of result.matchDomains)
-    {
-      let group = groupedElemhideFilters.get(matchDomain) || [];
-      group.push(result.selector);
-      groupedElemhideFilters.set(matchDomain, group);
-    }
-  }
+    {
+      genericSelectors.push(result.selector);
+    }
+    else
+    {
+      for (let matchDomain of result.matchDomains)
+      {
+        let group = groupedElemhideFilters.get(matchDomain) || [];
+        group.push(result.selector);
+        groupedElemhideFilters.set(matchDomain, group);
+      }
+    }
+  }
+
+  addCSSRules(rules, genericSelectors, "^https?://");
+
+  // Right after the generic element hiding filters, add the exceptions that
+  // should apply only to those filters.
+  for (let filter of this.generichideExceptions)
+    convertFilterAddRules(rules, filter, "ignore-previous-rules", false);
 
   groupedElemhideFilters.forEach((selectors, matchDomain) =>
   {
-    while (selectors.length)
-    {
-      let selector = selectors.splice(0, selectorLimit).join(", ");
-
-      // As of Safari 9.0 element IDs are matched as lowercase. We work around
-      // this by converting to the attribute format [id="elementID"]
-      selector = convertIDSelectorsToAttributeSelectors(selector);
-
-      rules.push({
-        trigger: {"url-filter": matchDomain,
-                  "url-filter-is-case-sensitive": true},
-        action: {type: "css-display-none",
-                 selector: selector}
-      });
-    }
+    addCSSRules(rules, selectors, matchDomain);
   });
 
   for (let filter of this.elemhideExceptions)
     convertFilterAddRules(rules, filter, "ignore-previous-rules", false);
   for (let filter of this.requestFilters)
     convertFilterAddRules(rules, filter, "block", true);
   for (let filter of this.requestExceptions)
     convertFilterAddRules(rules, filter, "ignore-previous-rules", true);
 
   return rules.filter(rule => !hasNonASCI(rule));
 };