Issue 4327 - Prevent filters with no hostname from blocking documents

lib/abp2blocklist.js

Index: lib/abp2blocklist.js
===================================================================
--- a/lib/abp2blocklist.js
+++ b/lib/abp2blocklist.js
@@ -271,20 +271,29 @@
 
   parseDomains(filter.domains, included, excluded);
 
   if (exceptionDomains)
     excluded = excluded.concat(exceptionDomains);
 
   if (withResourceTypes)
   {
-    trigger["resource-type"] = getResourceTypes(filter);
+    let resourceTypes = getResourceTypes(filter);
 
-    if (trigger["resource-type"].length == 0)
+    // Content blocker rules can't differentiate between sub-document requests
+    // (iframes) and top-level document requests. To avoid too many false
+    // positives, we prevent rules with no hostname part from blocking document
+    // requests.

[kzar, 2017/07/07 12:58:03]

Maybe add a note that when Safari X is the lowest version we support we could
remove this hack by making use of unless-top-url and if-top-url?

[Manish Jethani, 2017/07/08 11:29:50]

Done.

+    if (filter instanceof filterClasses.BlockingFilter && !parsed.hostname)
+      resourceTypes = resourceTypes.filter(type => type != "document");
+
+    if (resourceTypes.length == 0)
       return;
+
+    trigger["resource-type"] = resourceTypes;
   }
 
   if (filter.thirdParty != null)
     trigger["load-type"] = [filter.thirdParty ? "third-party" : "first-party"];
 
   if (included.length > 0)
   {
     trigger["if-domain"] = [];
@@ -310,17 +319,17 @@
       }
     }
   }
   else if (excluded.length > 0)
   {
     trigger["unless-domain"] = excluded.map(name => "*" + name);
   }
   else if (filter instanceof filterClasses.BlockingFilter &&
-           filter.contentType & typeMap.SUBDOCUMENT)
+           filter.contentType & typeMap.SUBDOCUMENT && parsed.hostname)

[kzar, 2017/07/07 12:58:03]

Could you explain why we're checking for a hostname here now? I don't understand
so far.

[Manish Jethani, 2017/07/08 11:29:50]

There are two parts to this change.

 1.  For filters with no hostname part, remove the "document" type entirely
 2.  For filters with a hostname part that have a "document" type, add an
unless-top-url exception

The check is for the second part. We could just blindly add the exception, but
it would then end up unblocking too much.

For example, this filter: "foo$subdocument,image"

Gives us this rule:

  {
    "trigger": {
      "url-filter": "^https?://.*foo",
      "resource-type": [
        "image"
      ]
    },
    "action": {
      "type": "block"
    }
  }

We've already stripped out the "document" type. Now if we add the exception,
it'll end up unblocking top-level image URLs containing the string "foo". I
don't know if it hurts too much, but that was not the intention here.

This filter on the other hand: "||foo.com$subdocument,image"

Gives us this rule:

  {
    "trigger": {
      "url-filter": "^https?://([^/]+\\.)?foo\\.com",
      "url-filter-is-case-sensitive": true,
      "resource-type": [
        "image",
        "document"
      ],
      "unless-top-url": [
        "^https?://([^/]+\\.)?foo\\.com"
      ],
      "top-url-filter-is-case-sensitive": true
    },
    "action": {
      "type": "block"
    }
  }

We leave the "document" type there, but we unblock if it's the top-level URL.
This will also unblock top-level images from *.foo.com, but that's more
acceptable.

Ideally we would split this one into two rules, but that's a bit overkill given
we're trying to minimize the number of rules.

[kzar, 2017/07/10 13:01:14]

Thanks for the explanation, sounds good to me. I like how we are still reducing
false-positives for users on older versions of Safari, whilst making use of the
new feature for users who are using newer versions too.

   {
     trigger["unless-top-url"] = [trigger["url-filter"]];
     if (trigger["url-filter-is-case-sensitive"])
       trigger["top-url-filter-is-case-sensitive"] = true;
   }
 
   rules.push({trigger: trigger, action: {type: action}});
 }