Opened on 02/09/2019 at 12:39:40 AM

Closed on 02/14/2019 at 12:57:23 PM

Last modified on 07/25/2019 at 12:56:37 PM

#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: Ross Verified working: yes
Review URL(s):

Description (last modified by mjethani)


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[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.

Attachments (0)

Change History (17)

comment:1 Changed on 02/09/2019 at 12:39:53 AM by hfiguiere

  • Owner set to hfiguiere

comment:2 Changed on 02/09/2019 at 12:43:06 AM by hfiguiere

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

comment:3 Changed on 02/11/2019 at 12:55:03 AM 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 on 02/11/2019 at 03:44:14 AM 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 on 02/11/2019 at 05:30:00 PM 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 on 02/12/2019 at 12:40:51 PM 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 on 02/12/2019 at 11:52:15 PM by hfiguiere


comment:8 Changed on 02/13/2019 at 04:36:25 PM by hfiguiere

  • Review URL(s) modified (diff)

comment:9 Changed on 02/13/2019 at 06:04:43 PM by abpbot

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

comment:10 Changed on 02/14/2019 at 02:59:41 AM by mjethani

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

comment:11 Changed on 02/14/2019 at 03:02:38 AM by mjethani

  • Description modified (diff)

comment:12 Changed on 02/14/2019 at 03:03:09 AM by mjethani

  • Review URL(s) modified (diff)

comment:13 Changed on 02/14/2019 at 03:04:12 AM by mjethani

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

comment:14 Changed on 02/14/2019 at 04:01:49 AM by mjethani

Filed #7284.

comment:15 Changed on 02/14/2019 at 12:57:03 PM by hfiguiere

  • Review URL(s) modified (diff)

comment:16 Changed on 02/14/2019 at 12:57:23 PM by hfiguiere

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

comment:17 Changed on 07/25/2019 at 12:56:37 PM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

Microsoft Edge 44.17763.1.0 / Windows 10 1809

Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from hfiguiere.
Note: See TracTickets for help on using tickets.