Issue 6931 - Allow content filters for localhost

[Manish Jethani, 2018-09-06 10:28:19]

Patch Set 1

[Manish Jethani, 2018-09-07 02:03:06]

Patch Set 2: Limit scope to localhost

See discussion:

https://issues.adblockplus.org/ticket/6931#comment:10

Let's limit the scope of this particular change to fixing the actual issue. We
can think about whether to support dotless domains separately.

[Sebastian Noack, 2018-09-07 03:21:21]

This would be good enough to handle the very specific scenario where the domain
is just "localhost" but what about "localhost,some.other.domain"?

[Manish Jethani, 2018-09-07 03:25:34]

On 2018/09/07 03:21:21, Sebastian Noack wrote:
> This would be good enough to handle the very specific scenario where the
domain
> is just "localhost" but what about "localhost,some.other.domain"?

Do we really care about that? As far as I understand, the `site_url` Jinja2
variable is set to "localhost" by default, so that's specifically what we're
trying to address here. In the long run we should probably find a way to allow
domains like https://bing/ somehow in an efficient way so we don't have to do
this special check.

Dave, what do you think?

[Sebastian Noack, 2018-09-07 15:52:35]

On 2018/09/07 03:25:34, Manish Jethani wrote:
> On 2018/09/07 03:21:21, Sebastian Noack wrote:
> > This would be good enough to handle the very specific scenario where the
> domain
> > is just "localhost" but what about "localhost,some.other.domain"?
> 
> Do we really care about that? As far as I understand, the `site_url` Jinja2
> variable is set to "localhost" by default, so that's specifically what we're
> trying to address here. In the long run we should probably find a way to allow
> domains like https://bing/ somehow in an efficient way so we don't have to do
> this special check.
> 
> Dave, what do you think?

Well, if we consider this specifically a hack to test the test pages locally,
the current implemention would suffice. But the behavior seems inconsistent and
unexpected to me. Dave is on vacation, BTW.

[Manish Jethani, 2018-09-10 09:25:40]

On 2018/09/07 15:52:35, Sebastian Noack wrote:
> On 2018/09/07 03:25:34, Manish Jethani wrote:
> > On 2018/09/07 03:21:21, Sebastian Noack wrote:
> > > This would be good enough to handle the very specific scenario where the
> > domain
> > > is just "localhost" but what about "localhost,some.other.domain"?
> > 
> > Do we really care about that? As far as I understand, the `site_url` Jinja2
> > variable is set to "localhost" by default, so that's specifically what we're
> > trying to address here. In the long run we should probably find a way to
allow
> > domains like https://bing/ somehow in an efficient way so we don't have to
do
> > this special check.
> > 
> > Dave, what do you think?
> 
> Well, if we consider this specifically a hack to test the test pages locally,
> the current implemention would suffice. But the behavior seems inconsistent
and
> unexpected to me. Dave is on vacation, BTW.

Well, "localhost,some.other.domain" would be covered by the regular expression,
so the only issue here is "~www.localhost,localhost"

This however shows that the regular expression is inadequate: for example, you
can write a filter like `com,example.com#$#bad-snippet` and it'll still apply on
all .com domains, if I understand correctly. We should file a separate issue for
addressing these inconsistencies.

[Manish Jethani, 2018-09-10 09:41:02]

Patch Set 3: Cover ~www.localhost,localhost case

[Manish Jethani, 2018-09-10 12:59:53]

On 2018/09/10 09:25:40, Manish Jethani wrote:
> This however shows that the regular expression is inadequate: for example, you
> can write a filter like `com,example.com#$#bad-snippet` and it'll still apply
on
> all .com domains, if I understand correctly. We should file a separate issue
for
> addressing these inconsistencies.

Filed: https://issues.adblockplus.org/ticket/6939

[Sebastian Noack, 2018-09-10 17:44:37]

https://codereview.adblockplus.org/29876561/diff/29877555/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29876561/diff/29877555/lib/filterClasses.j...
lib/filterClasses.js:1135: /,localhost,/.test("," + domains + ",")))
It seems we don't need a regular expression here, but could use .includes().

[Manish Jethani, 2018-09-10 17:57:33]

Patch Set 4: Use includes

https://codereview.adblockplus.org/29876561/diff/29877555/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29876561/diff/29877555/lib/filterClasses.j...
lib/filterClasses.js:1135: /,localhost,/.test("," + domains + ",")))
On 2018/09/10 17:44:37, Sebastian Noack wrote:
> It seems we don't need a regular expression here, but could use .includes().

Good point, done.

[Sebastian Noack, 2018-09-10 17:58:38]

LGTM