[$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
+  // Remove line breaks, tabs etc
   text = text.replace(/[^\S ]+/g, "");
 
   // Don't remove spaces inside comments
   if (/^ *!/.test(text))

[Sebastian Noack, 2018/03/14 20:59:20]

It seems previously, comments were allowed do be preceded by any kind of withe
space (\s), now you are explicitly checking for the space character. Why?

[Manish Jethani, 2018/03/15 08:09:59]

In the preceding code we're stripping out all non-space whitespace characters
(e.g. "\n") using /[^\S ]/ so we end up with only spaces and non-whitespace
characters. Now we only need to strip out spaces.

[kzar, 2018/03/15 10:26:24]

(See this comment as well
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.j...
)

     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();

[Sebastian Noack, 2018/03/14 20:59:20]

Similarly, here.

[kzar, 2018/03/15 10:26:25]

See our responses above.

   }
 
-  // For most regexp filters we strip all whitespace.
+  // 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("$") || !/csp=/i.test(strippedText))

[Manish Jethani, 2018/03/15 08:23:17]

Can we make this /\bcsp=/i instead?

[kzar, 2018/03/15 10:26:24]

Done.

+  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--)
-    optionsText = optionsText.substr(optionsText.indexOf("$") + 1);

[Sebastian Noack, 2018/03/14 20:59:20]

It seems the same can be achieved with less code and without creating an array:

  for (let i; (i = optionsText.indexOf("$")) != -1;)
    optionsText = optionsText.substr(i + 1);

[Manish Jethani, 2018/03/15 08:09:59]

This only looks for the last "$" in the string, which can be achieved using
String.lastIndexOf, but as Dave points out it's not correct. There could be
legit "$" within the options text as well. This is not actually true right now,
but may be in the future; however, since we already don't support commas in the
value of the $csp option, we should also not support "$" in the value. If the
filter author needs to use a "$" they can encode it as "%24" [1]. Am I right?

I can't think of an actual reason why we'd want to support "$" in the options
part of the filter. The most complex option is in fact $csp, but even here
there's no instance of needing to support "$" in the value [2]

[1]:
https://stackoverflow.com/questions/1547899/which-characters-make-a-url-invalid
[2]:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Po...

[Manish Jethani, 2018/03/15 09:11:06]

Actually I still don't get why we have to do this. We should run the options
regular expression on the unstripped text.

  if (!text.includes("$") || !/\bc *s *p *=/i.test(text))
    return text.replace(/ +/g, "");

  let optionsMatch = Filter.optionsRegExp.exec(text);
  if (!optionsMatch)
    return text.replace(/ +/g, "");

  ...

  return beforeOptions.replace(/ +/g, "") + "$" + options.join();

optionsMatch already points to the correct location, plus we have skipped one
unnecessary cycle of stripping spaces out of the options text.

[kzar, 2018/03/15 10:26:24]

Then the options regexp might not match depending on the whitespace. Yes we
could change the optionsRegexp to add an optional whitespace inbetween every
character, but I don't want to do that since it's already huge and also it's
used later in the common case for parsing regexp filters.

[kzar, 2018/03/15 10:26:24]

I can't tell if you're trolling or not at this point. If we don't care about
slightly changing how existing filters are parsed why didn't we go with the
original suggestion of doing something like `text.trim().replace(/ +/, " ")`?

[Manish Jethani, 2018/03/15 10:42:42]

Sorry, then I must have misunderstood. Are "$" supported in option values right
now? If they are not already supported, why do we care about supporting them
now? We already know that the value of the $csp option doesn't need to have "$"
in it.

[Manish Jethani, 2018/03/15 10:42:42]

Sorry, yeah, I see what you mean. It wouldn't work.

[Manish Jethani, 2018/03/15 11:25:47]

OK, I think I understand what you mean.

  normalize("foo$domain=bar.com$")

This currently works, and it will continue to work because there's no "csp=" in
there.

  normalize("foo$csp=script-src 'self',domain=bar.com$")

This currently "works" (the $csp option isn't normalized correctly), and it'll
break if we only take the last "$" to be the starting point of the options text.
Well, so it's a "breaking" change, I agree, but what does it actually break?
There are no actual filters that would break because of this. It's only a
breaking change in theory. We can "officially" not support "$" in option values
from now on, and nobody would be affected by it. So if we are to be practical
I'd say let's just take the index of the last "$". If in the future there's a
case where an option value needs to have an actual "$", then we can switch to
more complicated code.

Let me know what you think.

[kzar, 2018/03/15 11:38:50]

Well the part of the filter following a "$" is considered to be the option
string if it matches the options regexp, otherwise it's not and that part is
considered to be a part of the filter's regexp. Take this example:

  somead$domain=valid.com|$invalid.com

So far it blocks requests matching /somead/ for the domain valid.com. With your
suggestion it would instead block requests matching
/somead\$domain=valid\.com\|\$invalid\.com/ for all domains.

FWIW I agree that this probably doesn't matter much in practice, but I have had
questions about these types of edge cases from users (vai Mapx) before. Take
this filter

  /$file/analytics.

IIRC they were confused why it worked, but one like /$file didn't. Well that's
since "$file" is considered a valid options string and not used for the regexp
where as $file/analytics" isn't.

[kzar, 2018/03/15 11:43:52]

Sure or we could "officially" not support filters with random spaces inside just
the same. So we just do `.trim().replace(/ /+, " ")`, if the user gets lucky
their filter still works, but if the space was in an option value name (or
similar) things break. I think it's too late to talk about being practical in
this codereview to be honest.
Well that's speculation, hopefully few users would be anyway. But that also
applies to my above point.

[Manish Jethani, 2018/03/15 12:00:11]

But the two are not comparable, are they? If we don't support spaces, we break
filters that used to work; if we don't support "$" in a filter that _also_
contains "csp=" we break nothing.

A filter can contain spaces because the person who wrote it thought they could
include a space after the "$" or after each ",", I mean how do they know better?
That's not the case with "$", there's no reason to include a "$" in an option
value (can you think of any?). You would have to very deliberately add a "$" for
who-knows-why. (Note: a domain name can't contain "$")
I'm not aware of the history of this normalize function, but I wouldn't expect
option names or value to contain spaces in practice. I suspect the code strips
out _all_ spaces instead of just spaces between option names and value is
because it's easier and faster to do text.replace(/ /g, "") than to split the
string up and remove spaces around each name and each value. If that's the case,
by all means let's just not care about spaces within option names and non-CSP
option values.

It's totally legit though to write a filter like "foo $ domain = bar.com " and
expect it to work.

Anyway, so if you feel strongly about this then I don't mind reverting to the
original text.replace(/ +/g, " ") though you'd be in a better position to say if
it would break anything (literally we'd be changing the output of the function
for potentially lots of actual filters out there).

[Manish Jethani, 2018/03/15 12:14:21]

To sum this up, the normalize function looks good to me as it is, though we
could optimize further, but that's your call.

I'm looking at the other parts of this patch now.

[kzar, 2018/03/15 13:13:33]

Thanks OK. (If you can see how to optimise the common code path further I'm
interested, but not for the csp code path unless it doesn't hurt readability
further.)

[Manish Jethani, 2018/03/15 13:47:33]

Acknowledged.

-
+  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[i] = option.substring(0, cspMatch[0].length).replace(/ +/g, "") +

[Manish Jethani, 2018/03/15 08:23:17]

Well option.substring(0, cspMatch[0].length) is the same as cspMatch[0] so can
we skip the unnecessary substring here? We already have the string we need.

[kzar, 2018/03/15 10:26:24]

Done.

+      options[i] = cspMatch[0].replace(/ +/g, "") +
                    option.substr(cspMatch[0].length).trim().replace(/ +/g, " ");

[Manish Jethani, 2018/03/15 08:23:17]

Any reason why this uses substr instead of substring? We need to pick a favorite
and stick with it unless necessary otherwise, and I think substring is the best
choice. substr wasn't even standardized until much later.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[kzar, 2018/03/15 10:26:25]

substr and substring are the same unless the second parameter is passed, since
the second parameter wasn't passed here I went with the shorter option, which
seems consistent with the rest of this file.

I'm more worried about this use of substr[1] since it passes the second
parameter as an index, it just happens to work since the offset is always 0. But
you already disagreed that I should change that...

[1] -
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...

     }
     else
       options[i] = option.replace(/ +/g, "");
   }
 
   return beforeOptions + "$" + options.join();
 };
 
 /**
  * @see filterToRegExp
@@ skipping 549 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"
 });