Opened on 12/01/2016 at 06:40:35 PM
Closed on 12/13/2016 at 04:35:40 PM
Last modified on 05/22/2017 at 12:14:24 PM
#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): | |||
Description (last modified by fhd)
Environment
ABP for Firefox 2.8.2
How to reproduce
- Go to filter preferences.
- 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.
Attachments (0)
Change History (8)
comment:2 Changed on 12/01/2016 at 07:01:33 PM by fhd
comment:3 Changed on 12/01/2016 at 07:06:41 PM by trev
- Cc trev added; trev@adblockplus.org 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 on 12/02/2016 at 11:45:52 PM 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 on 12/08/2016 at 10:14:49 AM by fhd
- Cc scuturic amrmak arthur added
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:6 Changed on 12/13/2016 at 04:34:50 PM by abpbot
A commit referencing this issue has landed:
Issue 4684 - Allow { and } in attribute and property selectors
comment:7 Changed on 12/13/2016 at 04:35:40 PM by fhd
- Resolution set to fixed
- Status changed from reviewing to closed
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.