|
|
Created:
June 24, 2014, 2:54 p.m. by Sebastian Noack Modified:
Nov. 4, 2014, 8 a.m. Visibility:
Public. |
DescriptionIssue 705 - Generate element hiding instead request blocking filters for non-HTTP URLs
Patch Set 1 #Patch Set 2 : Rebased #
Total comments: 2
Patch Set 3 : Rebased #
Total comments: 3
MessagesTotal messages: 9
Quite a bit has changed in this file since this review was created. Could you please upload a patch with the rebased changes?
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/
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
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...
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.
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.
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.
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. |