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,28 @@
 var clickHideFiltersDialog = null;
 var lastRightClickEvent = null;
 
+function escapeChar(chr, pos, s) {
+    var code = chr.charCodeAt(0);
+
+    // non-ascii, needs no escaping
+    if (code >= 128)

[Sebastian Noack, 2014/11/05 18:09:04]

I just realized that I can easily check in the regex for non-ascii in order to
get rid of that check.

+      return chr;
+
+    // control characters and numbers, must be escaped based on their code point
+    if ((code >= 0 && code <= 31) || (code >= 48 && code <= 57) || code == 127)

[Wladimir Palant, 2014/11/05 15:14:41]

How about:

  if (code <= 0x1F || /\d/.test(chr) || code == 0x7F)

Why I prefer this:

1. The character code cannot be negative, and there is nothing around that this
check should be consistent with.
2. Hexadecimal makes the connection to regular expressions below easier to
understand.
3. Digits are better described with a regexp rather than ASCII codes.

[Sebastian Noack, 2014/11/05 18:09:04]

I already considered using that regex to check for digits. But I find it
slightly inconsistent. But never mind.

+      return "\\" + code.toString(16) + (pos + 1 < s.length ? " " : "");

[Wladimir Palant, 2014/11/05 15:14:41]

Why create a special case for end of string? That extra space certainly won't
hurt.

[Sebastian Noack, 2014/11/05 18:09:04]

I find the extra space in the end confusing. It can be easily misread at the end
of a quoted string, and it looks ugly when there are duplicated spaces between
two tokens. But given this is a corner case, I guess we can remove that check to
simplify the code.

+
+    // others, can be escaped by prepending a backslash
+    return "\\" + chr;
+}
+
 function quote(value)
 {
-  return '"' + value.replace(/(["\\])/g, "\\$1") + '"';
+  return '"' + value.replace(/["\\\r\n\f\0]/g, escapeChar) + '"';

[Wladimir Palant, 2014/11/05 15:14:41]

How about generally escaping all non-printable characters?

  return '"' + value.replace(/[\0x00-\0x1F"\\], escapeChar/) + '"';

This removes a bunch of assumptions, and non-printable characters are generally
uncommon enough that the overescaping here shouldn't matter.

[Sebastian Noack, 2014/11/05 18:09:04]

Yeah, un-escaped control characters make quoted strings harder to read anyway. I
have restructured the code, to have the code points for the control characters
only given once.

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

[Wladimir Palant, 2014/11/05 15:14:41]

How about always escaping leading dashes? These are uncommon anyway and the rule
will become much simpler:

  // Alphanumerical characters, dash and underscore generally don't need to be
escaped, first character can only be a letter however.
  return s.replace(/^[\d-]|[^\w-]/, escapeChar);

[Sebastian Noack, 2014/11/05 18:09:04]

I'd prefer to keep the logic for escaping dashes. Dashes in IDs and classes are
pretty common, and occasionally they occur in the first character. And not
over-escaping those, doesn't add any complexity to the JS code, but just a
simple branch to the regex, which is justified IMO.

[Wladimir Palant, 2014/11/05 19:56:52]

I'll leave the decision on whether that branch in the regexp can be described as
simple up to Dave.

As to IDs starting with a dash - I don't buy into this being anywhere near
common. At least EasyList and EasyList Germany only have filters referring to
#\2d for one website. And it is really the entire ID in that case, so escaping
is unavoidable. Not sure whether that site indeed had some element with id="-"
at some point, it doesn't seem to have it now.

 }
 
 function supportsShadowRoot(element)
@@ -376,10 +395,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 +404,15 @@
     selectorList.push(selector);
   };
 
-  if (elementId)
-    addSelector("#" + elementId);
+  if (elt.id)
+    addSelector("#" + escapeToken(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 += "." + escapeToken(elt.classList[i]);
 
     addSelector(selector);
   }
@@ -405,7 +420,7 @@
   if (url)
   {
     var src = elt.getAttribute("src");
-    var selector = src && elt.localName + '[src=' + quote(src) + ']';
+    var selector = src && escapeToken(elt.localName) + '[src=' + quote(src) + ']';
 
     if (/^https?:/i.test(url))
     {
@@ -427,7 +442,7 @@
   {
     var style = elt.getAttribute("style");
     if (style)
-      addSelector(elt.localName + '[style=' + quote(style) + ']');
+      addSelector(escapeToken(elt.localName) + '[style=' + quote(style) + ']');
   }
 
   // Show popup