Opened 14 months ago

Closed 2 months ago

#6939 closed defect (rejected)

No-TLD restriction for content filters does not work if there's at least one non-TLD

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
Cc: hfiguiere, sebastian Blocked By: #7121
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by mjethani)

Environment

ABP 3.3.1 on Chrome, macOS 10.12.6

How to reproduce

  1. Add the filter com,example.com#$#log 'Hello, .com!'
  2. Load example.com in a new tab
  3. Open the DevTools console for the tab
  4. Load eyeo.com in a new tab
  5. Open the DevTools console for the tab

Observed behaviour

In both steps 3 and 5, you see "Hello, .com!" printed to the console

Expected behaviour

Since com isn't a valid domain as per the restrictions (which may be changed as part of resolving this issue, by the way), it should consistently not be allowed in element hiding emulation and snippet filters. At the very least the filter should not be applied on unrelated domains like eyeo.com. Even better, the filter should be considered invalid.

Additional notes

The no-TLD restriction doesn't work for suffixes like .co.in, for example, nor does it work for filters like com,example.com#$#bad-snippet as this issues shows.

There are a couple of ways to address this:

  1. Add an additional check for TLDs in ContentFilter.fromText; this additional check would reject any filters containing a TLD, like the one in this issue (unless it's localhost, see #6931); unfortunately this would also likely slow down the loading of the filters.
  2. Don't check for TLDs in ContentFilter.fromText, but instead, do one of the following: (1) do the check at the time of looking up the filters in Snippets.getFiltersForDomain, etc.; (2) check for well-known public suffixes instead, also in Snippets.getFiltersForDomain, etc; (3) don't check anything anywhere.

Also see ticket:6773#comment:13

Change History (10)

comment:1 Changed 14 months ago by mjethani

  • Cc hfiguiere sebastian added

comment:2 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 14 months ago by mjethani

  • Priority changed from Unknown to P3
  • Ready set

comment:4 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 12 months ago by mjethani

  • Blocked By 7121 added

This could be fixed after #7121.

Last edited 12 months ago by mjethani (previous) (diff)

comment:6 Changed 12 months ago by mjethani

By the way, I also had an alternative implementation of extracting the base domain based on the public suffix list.

comment:7 Changed 8 months ago by mjethani

  • Owner set to mjethani

comment:8 Changed 4 months ago by mjethani

We have moved the public suffix list into Core (#7121). But we can't check for *.com because the generated publicSuffixList.json does not include com (even though it is part of Mozilla's public suffix list).

I shall think about this, but any ideas are welcome.

Last edited 4 months ago by mjethani (previous) (diff)

comment:9 Changed 4 months ago by sebastian

If there is no entry in publicSuffixList.json, the last part of the domain (or the whole domain if it doesn't include a dot) is considered the TLD (or public suffix).

comment:10 Changed 2 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.