Opened 3 months ago

Closed 7 weeks ago

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

https://codereview.adblockplus.org/30002601/

Description (last modified by hfiguiere)

Background

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 (example.com#@#foo-snippet 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 example.com#?#div:-abp-contains([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.: example.com#?#:-abp-properties(/margin: [0-9]{2}px/)
    3. The filter should work and hide the element containing Goodgoodgood

Change History (13)

comment:1 Changed 3 months ago by mjethani

  • Cc hfiguiere added

comment:2 Changed 3 months ago by hfiguiere

  • Owner set to hfiguiere

comment:3 Changed 3 months ago by hfiguiere

  • Status changed from new to reviewing

comment:4 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 3 months ago by hfiguiere

  • Description modified (diff)

comment:6 Changed 3 months ago by abpbot

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

comment:7 Changed 3 months ago by hfiguiere

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

comment:8 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:9 Changed 3 months ago by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening, this needs hints for testers.

comment:10 Changed 3 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:11 Changed 3 months ago by hfiguiere

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

comment:12 Changed 2 months ago 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 7 weeks ago 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.

Note: See TracTickets for help on using tickets.