[$csp2 adblockpluscore] Issue 6329 - Add the CSP filter type

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
@@ skipping 80 matching lines @@
 /**
  * Regular expression that RegExp filters specified as RegExps should match
  * @type {RegExp}
  */
 Filter.regexpRegExp = /^(@@)?\/.*\/(?:\$~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,\s]+)?)*)?$/;
 /**
  * Regular expression that options on a RegExp filter should match
  * @type {RegExp}
  */
 Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$/;
+/**
+ * Regular expression that matches an invalid Content Security Policy
+ * @type {RegExp}
+ */
+Filter.invalidCSPRegExp = /(;|^) ?(base-uri|referrer|report-to|report-uri|upgrade-insecure-requests)\b/i;
 
 /**
  * Creates a filter of correct type from its text representation - does the
  * basic parsing and calls the right constructor then.
  *
  * @param {string} text   as in Filter()
  * @return {Filter}
  */
 Filter.fromText = function(text)
 {
@@ skipping 54 matching lines @@
  * Removes unnecessary whitespaces from filter text, will only return null if
  * the input parameter is null.
  * @param {string} text
  * @return {string}
  */
 Filter.normalize = function(text)
 {
   if (!text)
     return text;
 
-  // Remove line breaks and such
-  text = text.replace(/[^\S ]/g, "");
+  // Remove line breaks, tabs etc
+  text = text.replace(/[^\S ]+/g, "");
 
   // Don't remove spaces inside comments
   if (/^ *!/.test(text))
     return text.trim();
 
   // Special treatment for element hiding filters, right side is allowed to
   // contain spaces
   if (Filter.elemhideRegExp.test(text))
   {
     let [, domain, separator, selector] = /^(.*?)(#@?#?)(.*)$/.exec(text);
-    return domain.replace(/ /g, "") + separator + selector.trim();

[Manish Jethani, 2018/03/14 15:27:30]

By the way, I tried to find out if / +/g is any better than / /g.

Here's a test I copied and pasted into the Chrome and Firefox consoles:

(function()
{
  let data = new Array(1000000);
  for (let i = 0; i < data.length; i++)
  {
    data[i] = Math.random().toString(36).slice(2).split("").join(
      new Array(Math.ceil(Math.random() * 10)).fill(" ").join("")
    );
  }

  console.log(data.slice(0, 10));

  let startTime = Date.now();
  for (let i = 0; i < data.length; i++)
    data[i] = data[i].replace(/ /g, "");

  console.log("time:", Date.now() - startTime);

  console.log(data.slice(0, 10));
})();

So this prints "time: 2128" or something in that order (slightly faster on
Firefox) for me. If I replace the regular expression with / +/g, it prints
"time: 812" or something in that order (again slightly faster on Firefox). I
think it cuts the execution time in half at least.

I repeated the test several times of course to account for the VM optimizing
that function and variance in general.

So we should go ahead and use / +/g in my opinion.

Note that we can also do the same for /[^\S ]/g above since that too is just a
single character match.

[kzar, 2018/03/14 17:28:59]

Done.

-  }
-
-  // For most regexp filters we strip all whitespace.
-  let strippedText = text.replace(/ /g, "");
-  if (!/csp=/i.test(strippedText))

[Manish Jethani, 2018/03/14 18:10:50]

So how about adding another check here:

  if (strippedText.indexOf("$") == -1 || !/csp=/i.test(strippedText))
    return strippedText;

Because EasyList starts off with a ton of filters with no "$"

Even better:

  let dollarIndex = strippedText.lastIndexOf("$");
  if (dollarIndex == -1 || !/csp=/i.test(strippedText.substring(dollarIndex +
1)))
    return strippedText;

We could probably reuse dollarIndex below (see my other comments).

Also I would change /csp=/i to /\bcsp=/i

[kzar, 2018/03/14 19:48:24]

Seems you're right, of 65k filters EasyList has 47k with no options part. Done.

+    return domain.replace(/ +/g, "") + separator + selector.trim();
+  }
+
+  // For most regexp filters we strip all spaces, but $csp filter options
+  // are allowed to contain single (non trailing) spaces.
+  let strippedText = text.replace(/ +/g, "");
+  if (!strippedText.includes("$") || !/\bcsp=/i.test(strippedText))
     return strippedText;
 
   let optionsMatch = Filter.optionsRegExp.exec(strippedText);
   if (!optionsMatch)
     return strippedText;
 
-  // But since the values of $csp filter options are allowed to contain single
-  // (non trailing) spaces we have to be more careful if they might be present.
+  // For $csp filters we must first separate out the options part of the
+  // text, being careful to preserve its spaces.
   let beforeOptions = strippedText.substring(0, optionsMatch.index);
-  let optionsText = text;
-  for (let i = beforeOptions.split("$").length; i > 0; i--)

[Manish Jethani, 2018/03/14 18:10:50]

Could you explain this part? I'm not sure what this is supposed to do. If the
idea is to get the part after the last "$" in the original text, we could just
use String.lastIndexOf here.

  let optionsText = text.substring(text.lastIndexOf("$") + 1);

For what it's worth neither the original code nor my suggestion works with the
following case:

  /foo$/$ csp = script-src  http://example.com/?$1=1&$2=2&$3=3

"$" is a valid character in the query string part of the URL:

https://stackoverflow.com/questions/1547899/which-characters-make-a-url-invalid

Thankfully there's no case for having a query string or indeed even the path
part of the URL in the $csp filter. Not yet anyway, but things may change in the
future with the CSP standard. Further the filter author can always encode the
"$" (recommended anyway) so it should be OK.

[kzar, 2018/03/14 19:48:24]

This code uses the index of the options part of the stripped filter text to
figure out what the index of the options part is of the original filter text. It
does that by counting the number of '$'s before the options start in the
stripped string and skipping past that many in the non stripped string.
I've added a test case for that and it worked OK, have I missed something?

-    optionsText = optionsText.substr(optionsText.indexOf("$") + 1);
-
-  let options = [];

[Manish Jethani, 2018/03/14 18:10:50]

Why create a separate array for the stripped version? We could just do this:

  let options = optionsText.split(",");
  for (let i = 0; i < options.length; i++)
    options[i] = options[i].replace(...);

[kzar, 2018/03/14 19:48:24]

Done.

-  for (let option of optionsText.split(","))
-  {
-    let cspMatch = /^( *c *s *p *=)([^,]+)/i.exec(option);

[Manish Jethani, 2018/03/14 18:10:50]

The regular expression here could be /^( *c *s *p *=)(.+)/i because we already
know that there are no commas there.

And why do we need the capturing groups?

  let cspMatch = /^ *c *s *p *=/.exec(option);
  if (cspMatch)
  {
    options.push(
      cspMatch[0].replace(/ /g, "") +
      option.substring(cspMatch[0].length).trim().replace(/ +/g, " ")
    );
  }

I have a feeling that this might actually be faster, though I haven't tested
this.

[kzar, 2018/03/14 19:48:24]

Done.

+  let strippedDollarIndex = -1;
+  let dollarIndex = -1;
+  do
+  {
+    strippedDollarIndex = beforeOptions.indexOf("$", strippedDollarIndex + 1);
+    dollarIndex = text.indexOf("$", dollarIndex + 1);
+  }
+  while (strippedDollarIndex != -1);
+  let optionsText = text.substr(dollarIndex + 1);
+
+  // Then we can normalize spaces in the options part safely
+  let options = optionsText.split(",");
+  for (let i = 0; i < options.length; i++)
+  {
+    let option = options[i];
+    let cspMatch = /^ *c *s *p *=/i.exec(option);
     if (cspMatch)
     {
-      options.push(
-        cspMatch[1].replace(/ /g, "") + cspMatch[2].trim().replace(/ +/g, " ")
-      );
+      options[i] = cspMatch[0].replace(/ +/g, "") +
+                   option.substr(cspMatch[0].length).trim().replace(/ +/g, " ");
     }
     else
-      options.push(option.replace(/ /g, ""));
+      options[i] = option.replace(/ +/g, "");
   }
 
   return beforeOptions + "$" + options.join();
 };
 
 /**
  * @see filterToRegExp
  */
 Filter.toRegExp = filterToRegExp;
 
@@ skipping 546 matching lines @@
         option = option.substr(0, separatorIndex);
       }
       option = option.replace(/-/, "_").toUpperCase();
       if (option in RegExpFilter.typeMap)
       {
         if (contentType == null)
           contentType = 0;
         contentType |= RegExpFilter.typeMap[option];
 
         if (option == "CSP" && typeof value != "undefined")
-        {
-          if (csp)
-            csp.push(value);
-          else
-            csp = [value];
-        }
+          csp = value;
       }
       else if (option[0] == "~" && option.substr(1) in RegExpFilter.typeMap)
       {
         if (contentType == null)
           ({contentType} = RegExpFilter.prototype);
         contentType &= ~RegExpFilter.typeMap[option.substr(1)];
       }
       else if (option == "MATCH_CASE")
         matchCase = true;
       else if (option == "~MATCH_CASE")
@@ skipping 12 matching lines @@
         sitekeys = value.toUpperCase();
       else
         return new InvalidFilter(origText, "filter_unknown_option");
     }
   }
 
   try
   {
     if (blocking)
     {
-      if (csp)
-      {
-        csp = csp.join("; ").toLowerCase();
-
-        // Prevent filters from injecting report-uri or report-to directives
-        // since they are a privacy concern. Regexp based upon reBadCSP[1].
-        // [1] - https://github.com/gorhill/uBlock/blob/67e06f53b4d73df6179f6d320553a55da4ead40e/src/js/static-net-filtering.js#L1362
-        if (/(;|^)\s*report-(to|uri)\b/.test(csp))
-          return new InvalidFilter(origText, "filter_invalid_csp");
-      }
+      if (csp && Filter.invalidCSPRegExp.test(csp))
+        return new InvalidFilter(origText, "filter_invalid_csp");
 
       return new BlockingFilter(origText, text, contentType, matchCase, domains,
                                 thirdParty, sitekeys, collapse, csp);
     }
     return new WhitelistFilter(origText, text, contentType, matchCase, domains,
                                thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, "filter_invalid_regexp");
@@ skipping 238 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });