Issue 705 - Generate element hiding instead request blocking filters for non-HTTP URLs

Issue 705 - Generate element hiding instead request blocking filters for non-HTTP URLs

[Thomas Greiner, 2014-10-10 12:04:44]

Quite a bit has changed in this file since this review was created. Could you
please upload a patch with the rebased changes?

[Sebastian Noack, 2014-10-23 11:16:59]

On 2014/10/10 12:04:44, Thomas Greiner wrote:
> Quite a bit has changed in this file since this review was created. Could you
> please upload a patch with the rebased changes?

Done, but note that this patch is still on top of
http://codereview.adblockplus.org/4829486721794048/

[Thomas Greiner, 2014-10-24 15:43:29]

On 2014/10/23 11:16:59, Sebastian Noack wrote:
> On 2014/10/10 12:04:44, Thomas Greiner wrote:
> > Quite a bit has changed in this file since this review was created. Could
you
> > please upload a patch with the rebased changes?
> 
> Done, but note that this patch is still on top of
> http://codereview.adblockplus.org/4829486721794048/

Thanks. LGTM

[Wladimir Palant, 2014-10-24 17:56:55]

http://codereview.adblockplus.org/5945877571043328/diff/5649050225344512/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5945877571043328/diff/5649050225344512/incl...
include.postload.js:388: addSelector(element.localName + "[" + attribute + '="'
+ value.replace(/"/g, '\\"') + '"]');
This isn't a sufficient escaping approach. First of all, the attribute name
should be escaped as well. Here the corresponding code from Element Hiding
Helper:

function escapeChar(dummy, match)
{
  return "\\" + match.charCodeAt(0).toString(16) + " ";
}

function escapeName(name)
{
  return name.replace(/([^\w\-])/g, "\\$1")
             .replace(/\\([\{\}])/g, escapeChar);
}

The rules for escaping values are even more complicated, e.g. one has to make
sure to escape backslashes. Again, code from Element Hiding Helper:

var escapedValue = attr.selected.replace(/(["\\])/g, '\\$1')
                                .replace(/([\{\}])/g, escapeChar)
                                .replace(/([^\S ])/g, escapeChar);

Frankly, I no longer remember why all of this was necessary. We had lots of
issues with this code and had to adjust it multiple times - we would need to
look at the change history if we want to know the details.

Not that any other selectors the existing code proposes are properly escaped...

[Sebastian Noack, 2014-10-30 16:36:24]

http://codereview.adblockplus.org/5945877571043328/diff/5649050225344512/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5945877571043328/diff/5649050225344512/incl...
include.postload.js:388: addSelector(element.localName + "[" + attribute + '="'
+ value.replace(/"/g, '\\"') + '"]');
On 2014/10/24 17:56:55, Wladimir Palant wrote:
> This isn't a sufficient escaping approach. First of all, the attribute name
> should be escaped as well.

The attribute name is hard-coded in the calling code and needs no escaping. I
restructured the code to make it more obvious.

> The rules for escaping values are even more complicated, e.g. one has to make
> sure to escape backslashes. Again, code from Element Hiding Helper:
> 
> var escapedValue = attr.selected.replace(/(["\\])/g, '\\$1')
>                                 .replace(/([\{\}])/g, escapeChar)
>                                 .replace(/([^\S ])/g, escapeChar);
>
> Frankly, I no longer remember why all of this was necessary.

Backslashes are escaped now. However, I tested it and there is no need to escape
curly brackets and any kind of whitespaces on Chrome and Safari.

[Wladimir Palant, 2014-10-30 22:36:28]

http://codereview.adblockplus.org/5945877571043328/diff/5657382461898752/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5945877571043328/diff/5657382461898752/incl...
include.postload.js:412: var selector = src && elt.localName + '[src=' +
quote(src) + ']';
elt.localName still needs escaping, DOM methods allow creating elements with any
tag name. And that's an unquoted identifier so the complicated escaping approach
is actually required...

Same thing with the style-based selector below.

[Sebastian Noack, 2014-10-31 13:52:49]

http://codereview.adblockplus.org/5945877571043328/diff/5657382461898752/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5945877571043328/diff/5657382461898752/incl...
include.postload.js:412: var selector = src && elt.localName + '[src=' +
quote(src) + ']';
On 2014/10/30 22:36:28, Wladimir Palant wrote:
> elt.localName still needs escaping, DOM methods allow creating elements with
any
> tag name.

The syntax for valid tag names is very restrictive. It seems that none of the
valid characters needs escaping in CSS. And (at least on Chrome and Safari)
calling document.createElement() with an invalid name raises an exception. Tags
with invalid names given in the HTML source are parsed as  text nodes.

[Wladimir Palant, 2014-11-03 18:14:27]

LGTM

http://codereview.adblockplus.org/5945877571043328/diff/5657382461898752/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5945877571043328/diff/5657382461898752/incl...
include.postload.js:412: var selector = src && elt.localName + '[src=' +
quote(src) + ']';
On 2014/10/31 13:52:49, Sebastian Noack wrote:
> The syntax for valid tag names is very restrictive. It seems that none of the
> valid characters needs escaping in CSS. And (at least on Chrome and Safari)
> calling document.createElement() with an invalid name raises an exception.
Tags
> with invalid names given in the HTML source are parsed as  text nodes.

document.createElement("foo:bar") might still be problematic but not addressing
this here is fine with me.