Issue 1527 - Properly escape generated CSS selectors

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -24,9 +24,24 @@
 var clickHideFiltersDialog = null;
 var lastRightClickEvent = null;
 
+var ctrlChar = /[\x00-\x1F\x7F]/;
+
+function escapeChar(chr)
+{
+  if (ctrlChar.test(chr) || /\d/.test(chr))
+    return "\\" + chr.charCodeAt(0).toString(16) + " ";
+
+  return "\\" + chr;
+}
+
 function quote(value)
 {
-  return '"' + value.replace(/(["\\])/g, "\\$1") + '"';
+  return '"' + value.replace(new RegExp('["\\\\]|' + ctrlChar.source, "g"), escapeChar) + '"';

[Wladimir Palant, 2014/11/06 21:26:06]

Generating regular expressions dynamically for no good reason whatsoever is a
clear step back as far as readability and maintainability goes :-(

[Sebastian Noack, 2014/11/10 12:13:18]

The idea was not to hard-code the char codes for control characters twice. But
the way I previously reused the regex lead to buggy behavior, since RegExp
objects using the global flag, persist the postion of the last match and resume
at that position when calling .test() the next time. But frankly, I also dislike
this construct, and removed it now.

+}
+
+function escapeCSS(s)
+{
+  return s.replace(/^\d|^-(?![^\d\-])|[^\w\-\u0080-\uFFFF]/g, escapeChar);

[Wladimir Palant, 2014/11/06 21:26:06]

Thinking more about this, I'm definitely opposed to a special case for dashes
not followed by a digit or dash here.

1) This special case is irrelevant on real websites.
2) It makes the rules more complicated and much harder to verify (as opposed to:
"always escape dashes if they are the first character").
3) Mozilla implements this logic differently which shows that this rule isn't
canonical.
4) This makes the otherwise trivial regular expression much harder to
understand.

[Sebastian Noack, 2014/11/10 12:13:18]

It's probably not worth to argue here. But I still think that the rule is easier
to read when the leading dash isn't escaped, and therefore should only be
escaped when necessary, and that a look-ahead doesn't make the regex much harder
to understand.

Instead escaping the leading dash, you can also escape the digit or dash in the
second position. That is what Gecko's CSS.escape() function does, when the
string starts with a dash followed by a digit. However, Gecko doesn't escape
sequences of leading dashes, since those need no escaping there.

 }
 
 function supportsShadowRoot(element)
@@ -376,10 +391,6 @@
   else if (elt.src)
     url = elt.src;
 
-  // Construct filters. The popup will retrieve these.
-  // Only one ID
-  var elementId = elt.id ? elt.id.split(' ').join('') : null;
-
   clickHideFilters = new Array();
   selectorList = new Array();
 
@@ -389,15 +400,15 @@
     selectorList.push(selector);
   };
 
-  if (elementId)
-    addSelector("#" + elementId);
+  if (elt.id)
+    addSelector("#" + escapeCSS(elt.id));
 
   if (elt.classList.length > 0)
   {
     var selector = "";
 
     for (var i = 0; i < elt.classList.length; i++)
-      selector += "." + elt.classList[i].replace(/([^\w-])/g, "\\$1");
+      selector += "." + escapeCSS(elt.classList[i]);
 
     addSelector(selector);
   }
@@ -405,7 +416,7 @@
   if (url)
   {
     var src = elt.getAttribute("src");
-    var selector = src && elt.localName + '[src=' + quote(src) + ']';
+    var selector = src && escapeCSS(elt.localName) + '[src=' + quote(src) + ']';
 
     if (/^https?:/i.test(url))
     {
@@ -427,7 +438,7 @@
   {
     var style = elt.getAttribute("style");
     if (style)
-      addSelector(elt.localName + '[style=' + quote(style) + ']');
+      addSelector(escapeCSS(elt.localName) + '[style=' + quote(style) + ']');
   }
 
   // Show popup