Opened 5 months ago

Closed 4 months ago

Last modified 7 weeks ago

#6797 closed change (fixed)

Require active domains in snippet filters

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

https://codereview.adblockplus.org/29849575/

Description (last modified by mjethani)

Background

Similar to element hiding emulation filters, snippet filters should also be required to have active domains. This is for security and performance reasons.

What to change

Add a check in ContentFilter.fromText: If there are no active domains in the snippet filter, return an InvalidFilter instance with the appropriate reason.

Integration notes

The change for this adds a new "reason" for the invalid filter called filter_snippet_nodomain. It needs a change in the UI. The text should be similar to the text for filter_elemhideemulation_nodomain.

Hints for testers

Test the following types of generic filters:

  1. Given three snippet filters, #$#log foo-1, example.com#$#log foo-2, and ~www.example.com#$#log foo-3, only foo-2 should be printed in the DevTools console when visiting example.com.
  1. Given three element hiding emulation filters, #?#.foo-1, example.com#?#.foo-2, and ~www.example.com#?#.foo-3, only example.com#?#.foo-2 should be applied on example.com (see the Adblock Plus DevTools panel for this).
  1. Given three element hiding filters, ##.foo-1, example.com##.foo-2, and ~www.example.com##.foo-3, all three filters should be applied on example.com.

Change History (13)

comment:1 Changed 5 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 4 months ago by mjethani

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

comment:3 Changed 4 months ago by mjethani

  • Owner set to mjethani

comment:4 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 4 months ago by greiner

  • Cc wspee greiner added

Thank you very much for the heads-up about the new strings.

Unfortunately, they're still located in adblockpluschrome. Therefore it'd be great if we could get Platform's approval on #6633 so that we can take over management of those strings from them. After that has happened I can go on and create a UI ticket to introduce those new strings.

comment:6 follow-up: Changed 4 months ago by kzar

  • Cc sebastian added

Well, this new string isn't located anywhere yet :p. Couldn't we just add it to adblockplusui, then migrate the other similar ones over in the future? (Yea, it would be kind of inconsistent.)

Anyway, I'll mention this to Sebastian, see if we can agree on something re #6633.

comment:7 in reply to: ↑ 6 Changed 4 months ago by greiner

Replying to kzar:

Well, this new string isn't located anywhere yet :p. Couldn't we just add it to adblockplusui, then migrate the other similar ones over in the future? (Yea, it would be kind of inconsistent.)

That should also work so, if necessary, we can do that for now.

comment:8 Changed 4 months ago by abpbot

A commit referencing this issue has landed:
Issue 6797 - Require active domains in snippet filters

comment:9 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:10 Changed 4 months ago by mjethani

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

comment:11 Changed 4 months ago by sebastian

Fine, let's move all strings to adblockplusui.

comment:12 Changed 4 months ago by greiner

  • Blocking 6846 added

comment:13 Changed 7 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described. Snippets and emulation filters require an active domain.

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.