Opened 13 months ago

Closed 12 months ago

Last modified 7 months ago

#4684 closed defect (fixed)

Element hiding filters containing { or } in the selector part do not work

Reported by: fhd Assignee: fhd
Priority: P2 Milestone:
Module: Core Keywords:
Cc: trev, scuturic, amrmak, arthur Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29367031/

Description (last modified by fhd)

Environment

ABP for Firefox 2.8.2

How to reproduce

  1. Go to filter preferences.
  2. Add a custom element hiding filter containing { or }, e.g.: example.com##[-abp-properties="/margin: [0-9]{2}px/"]

Observed behaviour

All spaces are being removed from the filter (as a side effect of it not being detected as an element hiding filter) and it doesn't actually work (since it fails the validity check).

Expected behaviour

The filter shows up in the list just as it was typed in, and it actually works.

Change History (8)

comment:1 Changed 13 months ago by fhd

  • Description modified (diff)

comment:2 Changed 13 months ago by fhd

The source of this issue is basically elemhideRegExp which doesn't match the filter above, because of it specifically not allowing { or } in the selector part: ([^{}].*). Would be easy enough to fix by not disallowing these characters but I wonder why we exclude those specifically.

I dug back into March 2006, and this got introduced along with non-simplified element hiding filters: https://hg.adblockplus.org/adblockplus/rev/f2171d939a2d41f461f8eff4b46696d745107efd

So I have no idea why we exclude those specifically, we don't worry about the validity of selectors otherwise.

comment:3 Changed 13 months ago by trev

  • Cc trev added; trev@… removed

We exclude those specifically because not disallowing them might lead to rule injection - consider ##foo{color:red}. Normally, { and } in selectors are supposed to be escaped (\00007b and \00007d should work). So question is whether we want to support escape sequences within CSS property filters or solve it by different means.

comment:4 Changed 13 months ago by fhd

  • Owner set to fhd

My vote goes to "other means" - regexp matching is tricky enough, we should probably not make it even trickier. Generally speaking, we should allow arbitrary content in the value part of both attribute and property selectors, but still disallow it everywhere else. I'll look into that.

comment:5 Changed 13 months ago by fhd

  • Cc scuturic amrmak arthur added
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:6 Changed 12 months ago by abpbot

A commit referencing this issue has landed:
Issue 4684 - Allow { and } in attribute and property selectors

comment:7 Changed 12 months ago by fhd

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

comment:8 Changed 7 months ago by Ross

  • Verified working set

Tested this change as part of #4658. Using {} in filters now works as expected.

ABP 2.8.2.4246-beta
Firefox 51 / 53 / Beta (54.0b2)
Windows 7

Note: See TracTickets for help on using tickets.