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)
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:
- Implement the specification
- Only escape for style sheet creation (with possible caching of the escaped value)
- 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:
- 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,})
- Create an HTML document <!DOCTYPE html><html><body><div>Badbadbad</div><div>Goodgoodgood</div></body></html>
- The filter should hide the element containing the text Badbadbad.
- Also issue #4684 but with the new syntax:
- Create an HTML document <!DOCTYPE html><html><body><1iv>Badbadbad</div><div style="margin: 10px;">Goodgoodgood</div></body></html>
- Add a custom element hiding filter containing { or }, e.g.: example.com#?#:-abp-properties(/margin: [0-9]{2}px/)
- 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:6 Changed on 02/19/2019 at 01:02:48 PM by abpbot
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: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.
ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809
ABP 3.5.2.2340
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
A commit referencing this issue has landed:
Issue 7284 - Move CSS escaping to createStyleSheet