Opened 3 months ago

Closed 3 months ago

Last modified 7 weeks ago

#6931 closed defect (fixed)

localhost isn't considered a domain by contentfilter validating code

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords:
Cc: mjethani, sebastian, Ross, hfiguiere, jsonesen Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29876561/

Description (last modified by mjethani)

Environment

adblockpluschrome 07878a4, Chrome 68

How to reproduce

  1. Open the Adblock Plus options page.
  2. Select the Advanced tab.
  3. Add the custom filter localhost#?#div:-abp-properties(width: 213px)

Observed behaviour

The filter isn't added, the "No active domain specified for extended element hiding filter" error message is displayed.

Expected behaviour

The filter is added, no error message is displayed.

Notes

  • This matters since it causes some of the testpages.adblockplus.org test cases to fail when running the website locally. See #6769.
  • The troublesome regexp in adblockpluscore/lib/filterClasses.js is if (!/,[^~][^,.]*\.[^,]/.test("," + domains)).

Hints for testers

First, be aware of #6939.

After this change, a filter like localhost#$#log foo (snippet filter) should be valid and should work on the domain localhost. If there's a subdomain of localhost, like www.localhost, that needs to be excluded, it should be possible to specify it as localhost,~www.localhost#$#log foo. It should also be possible to combine localhost with some other domain containing a dot, like localhost,example.com#$#log foo. As usual the order in which the domains (both included and excluded) are specified shouldn't matter. For example, localhost,~www.localhost#$#log foo and ~www.localhost,localhost#$#log foo should both work identically.

It should not be possible to specify any other dotless domain with a filter like com#$#log foo (but #6939 is an exception to this).

Change History (16)

comment:1 follow-up: Changed 3 months ago by mjethani

Has ABP ever considered localhost as a valid domain? I'm just asking, I don't have enough background.

comment:2 Changed 3 months ago by sebastian

Why wouldn't it? Adblock Plus doesn't really validate the domain names but just compares what is given in filters to what the browser provided. The exception are extended element hiding filter, which require one dot at least, since they are supposed to be limited to a specific website and if filter list authors would specify com#?#..., that filter would apply to all domains ending on .com.

However, localhost is special and not allowing it complicates testing emulated element hiding filters.

comment:3 follow-up: Changed 3 months ago by mjethani

Well, this could be addressed for now by changing localhost to localhost.localdomain in the tests, but this doesn't address the problem of domain names like google or bing.

Also the regular expression used to verify that a content filter has an active domain name strikes me as odd. We could perhaps change that check to /,[^~,]/.test("," + domains) or /(^|,)[^~,]/.test(domains).

comment:4 Changed 3 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:5 Changed 3 months ago by mjethani

  • Cc hfiguiere jsonesen added

comment:6 in reply to: ↑ 3 Changed 3 months ago by kzar

Replying to mjethani:

Well, this could be addressed for now by changing localhost to localhost.localdomain in the tests...

I thought that, but unfortunately we'd also need to pass the --address argument to the CMS' test_server.py, since the filters are going to be written based upon the site_url Jinja2 variable, which would still be localhost by default.

comment:7 in reply to: ↑ 1 Changed 3 months ago by kzar

Replying to mjethani:

Has ABP ever considered localhost as a valid domain?

FWIW the other test cases such as for blocking filters pass already.

comment:8 Changed 3 months ago by mjethani

  • Owner set to mjethani

comment:9 Changed 3 months ago by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:10 Changed 3 months ago by sebastian

FWIW, requiring a dot in the domain part in order to force the filter to be site-specific was a hack to start with (it doesn't guard against co.uk for example). Perhaps, a better approach would be to disable the recursive semantics of domain matching for element hiding emulation filters, so that element hiding emulation filters with no dot in the domain part are always considered valid, but com#?#... wouldn't match example.com. What do you think?

comment:11 Changed 3 months ago by mjethani

If that's a concern then I'd much rather just address the issue here and whitelist localhost by itself (anything else without a dot still stays invalid).

comment:12 Changed 3 months ago by sebastian

Fair enough.

comment:13 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 6931 - Allow content filters for localhost

comment:14 Changed 3 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:16 Changed 7 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Looks to be working as described.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

Note: See TracTickets for help on using tickets.