Issue 4684 - Allow { and } in attribute and property selectors

Issue 4684 - Allow { and } in attribute and property selectors

[Felix Dahlke, 2016-12-08 10:13:36]

I feel mildly bad about making that regexp more complex than it was. OTOH, this
is a pretty unobtrusive change that does the trick. I've added some tests for
CSS rule injection just to make sure we always cover all the edge cases here.
There might be ways I haven't thought about yet, however.

[Wladimir Palant, 2016-12-08 12:36:19]

We shouldn't risk introducing security issues here. I think the secure approach
would be accepting { and } in the regexp but escaping them as "\7B " and "\7D "
respectively in the selector member (note that the space is important).
cssProperties.js can then unescape the selector part that isn't going to be fed
to the CSS engine.

https://codereview.adblockplus.org/29367031/diff/29367032/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29367031/diff/29367032/lib/filterClasses.j...
lib/filterClasses.js:88: Filter.elemhideRegExp =
/^([^\/\*\|\@"!]*?)#(\@)?(?:([\w\-]+|\*)((?:\([\w\-]+(?:[$^*]?=[^\(\)"]*)?\))*)|#((?:[^{}]|(?:\[[\w-]+[~|]?=(['"])[^'"]*?\6\]))+))$/;
This won't consider escaped quotation marks properly.

[Felix Dahlke, 2016-12-09 14:09:06]

Not sure I like that approach better, dealing with braces could get pretty
complex in the element hiding emulation content script, and I'm not sure we want
to increase the complexity in that thing. OTOH, we err on the side of security
which is probably sensible, and we even simplify elemhideRegExp in the process a
bit.

So here's a new patch set with that approach.

[Wladimir Palant, 2016-12-13 14:32:16]

https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elem...
chrome/content/elemHideEmulation.js:141: .replace("\\x7B", "{").replace("\\x7D",
"}");
This will incorrectly replace \x7B1234 which is a Unicode symbol very different
from a bracket.

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.j...
lib/filterClasses.js:831: this.selector = selector.replace("{",
"\\x7B").replace("}", "\\x7D");
I was probably not clear enough but the space at the end is important: the
replacement string should be "\x7B " (yes, in CSS that space is really part of
the escape sequence). As you have it right now the selector foo="{1234" will be
converted into foo="\x7B1234" which will expect a single Unicode character
inside the attribute.

[Felix Dahlke, 2016-12-13 16:01:47]

Add a space after the escape sequence for { and }

[Felix Dahlke, 2016-12-13 16:04:48]

New patch set is up.

https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29367031/diff/29367165/chrome/content/elem...
chrome/content/elemHideEmulation.js:141: .replace("\\x7B", "{").replace("\\x7D",
"}");
On 2016/12/13 14:32:16, Wladimir Palant wrote:
> This will incorrectly replace \x7B1234 which is a Unicode symbol very
different
> from a bracket.

Done.

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.j...
lib/filterClasses.js:831: this.selector = selector.replace("{",
"\\x7B").replace("}", "\\x7D");
On 2016/12/13 14:32:16, Wladimir Palant wrote:
> I was probably not clear enough but the space at the end is important: the
> replacement string should be "\x7B " (yes, in CSS that space is really part of
> the escape sequence). As you have it right now the selector foo="{1234" will
be
> converted into foo="\x7B1234" which will expect a single Unicode character
> inside the attribute.

Done. I gotta admit this is feeling hacky. But I think I've fixed it as
suggested now.

[Wladimir Palant, 2016-12-13 16:29:13]

LGTM

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.j...
lib/filterClasses.js:831: this.selector = selector.replace("{",
"\\x7B").replace("}", "\\x7D");
On 2016/12/13 16:04:48, Felix Dahlke wrote:
> Done. I gotta admit this is feeling hacky.

This is exactly how EHH is doing it - took a while to arrive at a reliable
solution.