Issue 4450 - Prevent ElemHide filters with empty domains

Issue 4450 - Prevent ElemHide filters with empty domains

[kzar, 2016-10-05 14:30:44]

Patch Set 1

So far I only check that ElemHide filters don't have empty domains. I realised
that `Filter.optionsRegExp` doesn't match option strings like
"$domains=foo.com,," so to fix the logic there we would have to make the regexp
more lenient and then do another check for empty domains...

Frankly I'm no longer sure if any of this is worth the trouble, but I figured
I'd upload it and see what you thought. (I wouldn't mind to simple ignore the
edge case of element hiding filters with empty domains while keeping the simpler
getSelectorsForDomain logic.)

[Wladimir Palant, 2016-10-06 08:06:15]

> I realised
> that `Filter.optionsRegExp` doesn't match option strings like
> "$domains=foo.com,," so to fix the logic there we would have to make the
regexp
> more lenient and then do another check for empty domains...

Or maybe keep in mind that RegExpFilter has a different separator character?
It's $domains=foo.com|| - I'm fairly sure that checking for correctness there
isn't worth it. The only reason we do it in ElemHideBase here is simplifying the
logic elsewhere.

https://codereview.adblockplus.org/29356018/diff/29356019/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29356018/diff/29356019/lib/filterClasses.j...
lib/filterClasses.js:949: if (domain && emptyDomains(domain,
ElemHideBase.prototype.domainSeparator))
Why have this function rather than /(^|,)~?(,|$)/.test(domain)? Yes, you would
duplicate the separator character but the result would be considerably faster.

https://codereview.adblockplus.org/29356018/diff/29356019/lib/filterClasses.j...
lib/filterClasses.js:950: return new InvalidFilter(text, "filter_empty_domain");
filter_invalid_domain? We allow empty domains, but here the domain is invalid.

https://codereview.adblockplus.org/29356018/diff/29356019/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29356018/diff/29356019/test/filterClasses....
test/filterClasses.js:374: ",foo.com##selector", "foo.com,~##selector"
What about empty domains in the middle? foo.com,,bar.com##selector and
foo.com,~,bar.com##selector

[kzar, 2016-10-06 08:53:56]

Patch Set 2 : Addressed feedback

> Or maybe keep in mind that RegExpFilter has a different separator character?

Ah, whoops yea you're right.

https://codereview.adblockplus.org/29356018/diff/29356019/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29356018/diff/29356019/lib/filterClasses.j...
lib/filterClasses.js:949: if (domain && emptyDomains(domain,
ElemHideBase.prototype.domainSeparator))
On 2016/10/06 08:06:14, Wladimir Palant wrote:
> Why have this function rather than /(^|,)~?(,|$)/.test(domain)? Yes, you would
> duplicate the separator character but the result would be considerably faster.

Done.

https://codereview.adblockplus.org/29356018/diff/29356019/lib/filterClasses.j...
lib/filterClasses.js:950: return new InvalidFilter(text, "filter_empty_domain");
On 2016/10/06 08:06:14, Wladimir Palant wrote:
> filter_invalid_domain? We allow empty domains, but here the domain is invalid.

Done.

https://codereview.adblockplus.org/29356018/diff/29356019/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29356018/diff/29356019/test/filterClasses....
test/filterClasses.js:374: ",foo.com##selector", "foo.com,~##selector"
On 2016/10/06 08:06:14, Wladimir Palant wrote:
> What about empty domains in the middle? foo.com,,bar.com##selector and
> foo.com,~,bar.com##selector

Done.

[Wladimir Palant, 2016-10-06 09:46:29]

LGTM