Issue 1527 - Properly escape generated CSS selectors

include.postload.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 // Click-to-hide stuff
 var clickHide_activated = false;
 var clickHide_filters = null;
 var currentElement = null;
 var clickHideFilters = null;
 var highlightedElementsSelector = null;
 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 escapeChar(chr)
+{
+  var code = chr.charCodeAt(0);
+
+  if (code <= 0x1F || code == 0x7F || /\d/.test(chr))
+    return "\\" + code.toString(16) + " ";
+
+  return "\\" + chr;
 }
 
 function quote(value)
 {
-  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.

+  return '"' + value.replace(/["\\\x00-\x1F\x7F]/g, escapeChar) + '"';
+}
+
+function escapeCSS(s)
+{
+  return s.replace(/^[\d\-]|[^\w\-\u0080-\uFFFF]/g, escapeChar);
 }
 
 function supportsShadowRoot(element)
 {
   if (!("createShadowRoot" in element))
     return false;
 
   // There are some elements (e.g. <textarea>), which don't
   // support author created shadow roots and throw an exception.
   var clone = element.cloneNode(false);
@@ skipping 339 matching lines @@
   clickHideFilters = new Array();
   selectorList = new Array();
 
   var addSelector = function(selector)
   {
     clickHideFilters.push(document.domain + "##" + selector);
     selectorList.push(selector);
   };
 
   if (elt.id)
-    addSelector("#" + escapeToken(elt.id));
+    addSelector("#" + escapeCSS(elt.id));
 
   if (elt.classList.length > 0)
   {
     var selector = "";
 
     for (var i = 0; i < elt.classList.length; i++)
-      selector += "." + escapeToken(elt.classList[i]);
+      selector += "." + escapeCSS(elt.classList[i]);
 
     addSelector(selector);
   }
 
   if (url)
   {
     var src = elt.getAttribute("src");
-    var selector = src && escapeToken(elt.localName) + '[src=' + quote(src) + ']';
+    var selector = src && escapeCSS(elt.localName) + '[src=' + quote(src) + ']';
 
     if (/^https?:/i.test(url))
     {
       clickHideFilters.push(url.replace(/^[\w\-]+:\/+(?:www\.)?/, "||"));
 
       if (selector)
         selectorList.push(selector);
     }
     else if (selector)
       addSelector(selector);
   }
 
   // restore the original style, before generating the fallback filter that
   // will include the style, and to prevent highlightElements from saving those
   unhighlightElement(currentElement);
 
   // as last resort, create a filter based on inline styles
   if (clickHideFilters.length == 0)
   {
     var style = elt.getAttribute("style");
     if (style)
-      addSelector(escapeToken(elt.localName) + '[style=' + quote(style) + ']');
+      addSelector(escapeCSS(elt.localName) + '[style=' + quote(style) + ']');
   }
 
   // Show popup
   clickHide_showDialog(e.clientX, e.clientY, clickHideFilters);
 
   // Highlight the elements specified by selector in yellow
   highlightElements(selectorList.join(","));
   // Now, actually highlight the element the user clicked on in red
   highlightElement(currentElement, "#fd1708", "#f6a1b5");
 
@@ skipping 218 matching lines @@
         break;
       default:
         sendResponse({});
         break;
     }
   });
 
   if (window == window.top)
     ext.backgroundPage.sendMessage({type: "report-html-page"});
 }