Opened 9 months ago

Last modified 4 months ago

#7268 closed defect

ContainsSelector element hide emulation doesn't work with {} in regexp — at Version 15

Reported by: hfiguiere Assignee: hfiguiere
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/30006562/

Description (last modified by mjethani)

Environment

ABP 3.4.3
Any supported browser
Any supported OS
No filter lists

How to reproduce

  1. Create an advanced selector filter with :-abp-contains() and a regexp that contain a quantifier {n}. Something like
    example.com#?#div:-abp-contains([aAbBdD]{9,})
    
  2. Create an HTML document <!DOCTYPE html><html><body><div>Badbadbad</div><div>Goodgoodgood</div></body></html>

Observed behaviour

The filter doesn't hide any elements.

Expected behaviour

The filter should hide the element containing the text Badbadbad.

Additional notes

ContainsSelector element hide emulation doesn't work with {} in regexp.
This is because in ElemHideBase.selector() we escape
{ and } to prevent CSS injection but the ContainsSelector constructor doesn't put them back.

Change History (15)

comment:1 Changed 9 months ago by hfiguiere

  • Owner set to hfiguiere

comment:2 Changed 9 months ago by hfiguiere

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

comment:3 Changed 9 months ago by mjethani

I think it's time to address the issue raised in #6726. We should move the escaping from the ElemHideBase class to the ElemHide module, since we are now generating the style sheet itself over there. It should be part of the createStyleSheet function. The selector property should contain the original raw selector.

The rules for CSS escaping are simple and can be written in less than 20 lines of JavaScript.

comment:4 Changed 9 months ago by mjethani

Let's start by adding a new escapedSelector property getter to replace the current selector property. The selector property should be used everywhere except in contexts where the caller needs to inject the selector into a style sheet.

comment:5 Changed 9 months ago by hfiguiere

We can definitely do a more complex fix that will work. But at the moment this is a situation that :-abp-properties() handle but not :-abp-contains().

comment:6 Changed 9 months ago by mjethani

What I meant was:

  1. We change the selector getter in lib/filterClasses.js to return the unescaped value of this.body
  2. In createStyleSheet in lib/elemHide.js, we escape the selector before writing it to the style sheet
  3. Remove the unescaping from lib/content/elemHideEmulation.js for PropsSelector

What do you think?

comment:7 Changed 9 months ago by hfiguiere

Done

comment:8 Changed 9 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:9 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 7268 - Unescape { and } for :-abp-contains()

comment:10 Changed 9 months ago by mjethani

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:11 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 9 months ago by mjethani

  • Review URL(s) modified (diff)

comment:13 Changed 9 months ago by mjethani

Let's close this, I'll file a new issue for the refactoring.

comment:14 Changed 9 months ago by mjethani

Filed #7284.

comment:15 Changed 9 months ago by hfiguiere

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.