Opened 8 months ago

Last modified 2 months ago

#6939 new defect

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:
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)


ABP 3.3.1 on Chrome, macOS 10.12.6

How to reproduce

  1. Add the filter com,$#log 'Hello, .com!'
  2. Load in a new tab
  3. Open the DevTools console for the tab
  4. Load 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 Even better, the filter should be considered invalid.

Additional notes

The no-TLD restriction doesn't work for suffixes like, for example, nor does it work for filters like 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 (7)

comment:1 Changed 8 months ago by mjethani

  • Cc hfiguiere sebastian added

comment:2 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 8 months ago by mjethani

  • Priority changed from Unknown to P3
  • Ready set

comment:4 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 6 months ago by mjethani

  • Blocked By 7121 added

This could be fixed after #7121.

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

comment:6 Changed 6 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 2 months ago by mjethani

  • Owner set to mjethani
Note: See TracTickets for help on using tickets.