Opened on 02/14/2019 at 04:01:14 AM

Closed on 04/02/2019 at 07:22:44 AM

Last modified on 07/25/2019 at 01:22:37 PM

#7284 closed change (fixed)

Move basic CSS escaping to ElemHide module

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

Description (last modified by hfiguiere)


Some basic CSS escaping was implemented in #4684 to prevent rule injection. When the code was refactored to make snippet filters also belong in the family of content filters (#6781), there was an idea to move the CSS escaping to lib/elemHide.js so that the snippet filters could use the same exception syntax ( arg1 arg2), but this was shot down. Now since all of the style sheet creation is centralized in lib/elemHide.js via the createStyleSheet() function (#6957, #6999), it makes sense to approach this issue again.

Ideally the CSS escaping should:

  1. Implement the specification
  2. Only escape for style sheet creation (with possible caching of the escaped value)
  3. Perform well

What to change

  • in filterClasses.js, change the selector() getter for ElemHideBase.prototype to return just this.body (no escaping)
  • in content/elemHidingEmulation.js, in ContainsSelector() and PropsSelector() remove the unescaping.
  • in elemHide.js, add a function escapeSelector()
  • in the same file, change createRule() to call escapeSelector() where appropriate.
  • in test/browser/elemHideEmulation.js, remove testPropertySelectorWithEscapedBrace and exports.testPropertySelectorWithImproperlyEscapedBrace that are now irrelevant.
  • in test/elemHide.js, in testCreateStyleSheet() add a test for the escaping. Also fix the initialisation of selectors

Hints for testers

  • Same as issue #7268:
    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>
    3. The filter should hide the element containing the text Badbadbad.
  • Also issue #4684 but with the new syntax:
    1. Create an HTML document <!DOCTYPE html><html><body><1iv>Badbadbad</div><div style="margin: 10px;">Goodgoodgood</div></body></html>
    2. Add a custom element hiding filter containing { or }, e.g.: [0-9]{2}px/)
    3. The filter should work and hide the element containing Goodgoodgood

Attachments (0)

Change History (14)

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

  • Cc hfiguiere added

comment:2 Changed on 02/14/2019 at 12:57:47 PM by hfiguiere

  • Owner set to hfiguiere

comment:3 Changed on 02/14/2019 at 12:57:54 PM by hfiguiere

  • Status changed from new to reviewing

comment:4 Changed on 02/14/2019 at 01:15:27 PM by mjethani

  • Description modified (diff)

comment:5 Changed on 02/15/2019 at 02:47:13 PM by hfiguiere

  • Description modified (diff)

comment:6 Changed on 02/19/2019 at 01:02:48 PM by abpbot

A commit referencing this issue has landed:
Issue 7284 - Move CSS escaping to createStyleSheet

comment:7 Changed on 02/19/2019 at 01:04:05 PM by hfiguiere

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

comment:8 Changed on 02/19/2019 at 06:15:26 PM by mjethani

  • Description modified (diff)

comment:9 Changed on 02/19/2019 at 06:15:58 PM by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening, this needs hints for testers.

comment:10 Changed on 02/19/2019 at 06:16:25 PM by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:11 Changed on 02/21/2019 at 08:24:21 PM by hfiguiere

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reopened to closed

comment:12 Changed on 03/06/2019 at 11:23:52 AM by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm reopening this.

I think we should consider implementing proper CSS escaping here. For this we need to evaluate its impact on performance. I can take this up.

comment:13 Changed on 04/02/2019 at 07:22:44 AM by mjethani

  • Resolution set to fixed
  • Status changed from reopened to closed
  • Summary changed from Implement CSS escaping in ElemHide module to Move basic CSS escaping to ElemHide module

Since this is already in a dependency update, I am redefining the scope of this change and closing it. We should still implement proper CSS escaping at some point but for this we can file a separate issue.

comment:14 Changed on 07/25/2019 at 01:22: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.