Issue 4327 - Prevent filters with no hostname from blocking documents

Issue 4327 - Prevent filters with no hostname from blocking documents

[Manish Jethani, 2017-06-16 16:10:04]

Patch Set 1

This is essentially the same as Dave's 29349989 rebased with an additional
change, I wasn't sure I could update that one.

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

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.j...
lib/abp2blocklist.js:284: // requests.
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?

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.j...
lib/abp2blocklist.js:327: filter.contentType & typeMap.SUBDOCUMENT &&
parsed.hostname)
Could you explain why we're checking for a hostname here now? I don't understand
so far.

[kzar, 2017-07-07 12:58:43]

(Moving Sebastian to Cc.)

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

Patch Set 2: Add comments

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.j...
lib/abp2blocklist.js:284: // requests.
On 2017/07/07 12:58:03, kzar wrote:
> 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?

Done.

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.j...
lib/abp2blocklist.js:327: filter.contentType & typeMap.SUBDOCUMENT &&
parsed.hostname)
On 2017/07/07 12:58:03, kzar wrote:
> Could you explain why we're checking for a hostname here now? I don't
understand
> so far.

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]

LGTM, go ahead and push this when you're ready, I don't think it needs Sebastian
to take a look.

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29467585/diff/29467586/lib/abp2blocklist.j...
lib/abp2blocklist.js:327: filter.contentType & typeMap.SUBDOCUMENT &&
parsed.hostname)
On 2017/07/08 11:29:50, Manish Jethani wrote:
> On 2017/07/07 12:58:03, kzar wrote:
> > Could you explain why we're checking for a hostname here now? I don't
> understand
> > so far.
> 
> 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.

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.