Noissue - Do not add subdomain wildcard if subdomain excluded

lib/abp2blocklist.js

Index: lib/abp2blocklist.js
===================================================================
--- a/lib/abp2blocklist.js
+++ b/lib/abp2blocklist.js
@@ -59,16 +59,30 @@
   return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
 }
 
 function matchDomain(domain)
 {
   return "^https?://([^/:]*\\.)?" + escapeRegExp(domain).toLowerCase() + "[/:]";
 }
 
+function findSubdomainsInList(domain, list)
+{
+  let subdomains = [];
+  let suffixLength = domain.length + 1;
+
+  for (let name of list)
+  {
+    if (name.length > suffixLength && name.slice(-suffixLength) == "." + domain)
+      subdomains.push(name.slice(0, -suffixLength));
+  }
+
+  return subdomains;
+}
+
 function convertElemHideFilter(filter, elemhideSelectorExceptions)
 {
   let included = [];
   let excluded = [];
   let rules = [];
 
   parseDomains(filter.domains, included, excluded);
 
@@ -263,19 +277,44 @@
     if (trigger["resource-type"].length == 0)
       return;
   }
 
   if (filter.thirdParty != null)
     trigger["load-type"] = [filter.thirdParty ? "third-party" : "first-party"];
 
   if (included.length > 0)
-    trigger["if-domain"] = included.map(name => "*" + name);
+  {
+    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.

+          trigger["if-domain"].push("www." + name);
+      }
+      else
+      {
+        trigger["if-domain"].push("*" + name);
+      }
+    }
+  }
   else if (excluded.length > 0)
+  {
     trigger["unless-domain"] = excluded.map(name => "*" + name);
+  }
 
   rules.push({trigger: trigger, action: {type: action}});
 }
 
 function hasNonASCI(obj)
 {
   if (typeof obj == "string")
   {