Opened 9 days ago

Closed 3 days ago

#7268 closed defect (fixed)

ContainsSelector element hide emulation doesn't work with {} in regexp

Reported by: hfiguiere Assignee: hfiguiere
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
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 (16)

comment:1 Changed 9 days ago by hfiguiere

  • Owner set to hfiguiere

comment:2 Changed 9 days ago by hfiguiere

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

comment:3 Changed 7 days 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 7 days 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 6 days 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 5 days 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 5 days ago by hfiguiere

Done

comment:8 Changed 4 days ago by hfiguiere

  • Review URL(s) modified (diff)

comment:9 Changed 4 days ago by abpbot

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

comment:10 Changed 4 days ago by mjethani

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

comment:11 Changed 4 days ago by mjethani

  • Description modified (diff)

comment:12 Changed 4 days ago by mjethani

  • Review URL(s) modified (diff)

comment:13 Changed 4 days ago by mjethani

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

comment:14 Changed 4 days ago by mjethani

Filed #7284.

comment:15 Changed 3 days ago by hfiguiere

  • Review URL(s) modified (diff)

comment:16 Changed 3 days ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.