Issue 6538, 6781 - Implement support for snippet filters

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 66 matching lines @@
   }
 };
 
 /**
  * Cache for known filters, maps string representation to filter objects.
  * @type {Map.<string,Filter>}
  */
 Filter.knownFilters = new Map();
 
 /**
- * Regular expression that element hiding filters should match
+ * Regular expression that content filters should match
  * @type {RegExp}
  */
-Filter.elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;
+Filter.contentRegExp = /^([^/*|@"!]*?)#([@?$])?#(.+)$/;
 /**
  * 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)
 {
   let filter = Filter.knownFilters.get(text);
   if (filter)
     return filter;
 
-  let match = (text.includes("#") ? Filter.elemhideRegExp.exec(text) : null);
+  let match = text.includes("#") ? Filter.contentRegExp.exec(text) : null;
   if (match)
   {
     let propsMatch;
     if (!match[2] &&
         (propsMatch = /\[-abp-properties=(["'])([^"']+)\1\]/.exec(match[3])))
     {
       // This is legacy CSS properties syntax, convert to current syntax
       let prefix = match[3].substr(0, propsMatch.index);
       let expression = propsMatch[2];
       let suffix = match[3].substr(propsMatch.index + propsMatch[0].length);
       return Filter.fromText(`${match[1]}#?#` +
           `${prefix}:-abp-properties(${expression})${suffix}`);
     }
 
-    filter = ElemHideBase.fromText(
-      text, match[1], match[2], match[3]
-    );
+    filter = ContentFilter.fromText(text, match[1], match[2], match[3]);
   }
   else if (text[0] == "!")
     filter = new CommentFilter(text);
   else
     filter = RegExpFilter.fromText(text);
 
   Filter.knownFilters.set(filter.text, filter);
   return filter;
 };
 
@@ skipping 29 matching lines @@
   if (!text)
     return text;
 
   // 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))
+  // Special treatment for content filters, right side is allowed to contain
+  // spaces
+  if (Filter.contentRegExp.test(text))
   {
-    let [, domains, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text);
-    return domains.replace(/ +/g, "") + separator + selector.trim();
+    let [, domains, separator, script] = /^(.*?)(#[@?$]?#?)(.*)$/.exec(text);
+    return domains.replace(/ +/g, "") + separator + script.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)
@@ skipping 785 matching lines @@
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 }
 exports.WhitelistFilter = WhitelistFilter;
 
 WhitelistFilter.prototype = extend(RegExpFilter, {
   type: "whitelist"
 });
 
 /**
- * Base class for element hiding filters
+ * Base class for content filters
  * @param {string} text see {@link Filter Filter()}
  * @param {string} [domains] Host names or domains the filter should be
  *                           restricted to
- * @param {string} selector   CSS selector for the HTML elements that should be
- *                            hidden
+ * @param {string} script    Script that should be executed
  * @constructor
  * @augments ActiveFilter
  */
-function ElemHideBase(text, domains, selector)
+function ContentFilter(text, domains, script)
 {
   ActiveFilter.call(this, text, domains || null);
 
-  // Braces are being escaped to prevent CSS rule injection.
-  this.selector = selector.replace("{", "\\7B ").replace("}", "\\7D ");
+  this.script = script;
 }
-exports.ElemHideBase = ElemHideBase;
+exports.ContentFilter = ContentFilter;
 
-ElemHideBase.prototype = extend(ActiveFilter, {
+ContentFilter.prototype = extend(ActiveFilter, {
   /**
    * @see ActiveFilter.domainSeparator
    */
   domainSeparator: ",",
 
   /**
-   * CSS selector for the HTML elements that should be hidden
+   * Script that should be executed

[kzar, 2018/07/10 12:53:22]

This comment seems misleading, I think it should be clearer that the value also
might be a CSS selector.

[Manish Jethani, 2018/07/10 18:29:20]

This is kinda overridden in the subclass as a new property called "selector"
(what it was called previously). The way I think of it, ContentFilter is generic
and it always runs a "script" on the document; ElemHideBase is a special case
where the "script" is a CSS selector.

[kzar, 2018/07/10 18:53:59]

To me a CSS selector is not a script (nor code), which is why I think this name
is misleading. Also I don't think of element hiding filters to be a special case
of content filters, like you mention they're by far the most common.

    * @type {string}
    */
-  selector: null
+  script: null

[kzar, 2018/07/10 12:53:22]

"script" seems a misleading name, how about "body"? (We should also update some
of the comments and parameter names to match.)

[Manish Jethani, 2018/07/10 18:29:20]

This was originally called "code", then I renamed it to "script". "script" seems
OK to me, but I don't really care too much what it's called. What do you think,
"code" or "body"?

[kzar, 2018/07/10 18:53:59]

I prefer "body" over "script" and "code".

[Manish Jethani, 2018/07/11 13:04:25]

Done.

 });
 
 /**
- * Creates an element hiding filter from a pre-parsed text representation
+ * Creates a content filter from a pre-parsed text representation
  *
  * @param {string} text         same as in Filter()
  * @param {string} [domains]
  *   domains part of the text representation
  * @param {string} [type]
-*    rule type, either empty or @ (exception) or ? (emulation rule)
- * @param {string} selector     raw CSS selector
+ *   rule type, either empty or @ (exception) or ? (emulation rule) or

[kzar, 2018/07/10 12:53:22]

Please could you improve this part of the comment? How about this?

The rule type, either:
 - "" for an element hiding filter
 - "@" for an exception filter
 - "?" for an element hiding emulation filter
 - "$" for a snippet filter

I don't care too much about the formatting anyway, so long as it's a bit easier
to grok.

[Manish Jethani, 2018/07/11 13:04:24]

Done.

[Manish Jethani, 2018/07/11 13:16:31]

By the way, in case you're wondering if it shouldn't be called "content
exception" rather than "element hiding exception", I have thought about this. In
the initial version these exceptions won't apply to snippet filters. When we get
around to it finally, the class should also be renamed to ContentException and
should be moved one level up to extend directly from ContentFilter. But I'm not
fully sure about this. Maybe snippet filters should have their own exceptions
like "#$@#"? Something to think about.

[kzar, 2018/07/11 16:33:12]

Yea, this is something I asked you about on one of the other reviews. I
disagree, I think #@# exceptions should apply to snippet filters too. Unless I'm
mistaken "example.com#@#.some_class" isn't going to matter to a snippet filter,
but it would be logical to expect "example.com#@#foo arg" to work.

Sebastian/Sergz what do you guys think?

[Manish Jethani, 2018/07/11 16:42:46]

Yeah, I too think that those exceptions could just apply to snippet filters as
well. We will have to update the code and the documentation, that's all.

One challenge with the exceptions was that the selector property was being
escaped. This is mostly resolved now, because we have a separate
code/script/body property (unescaped) in the superclass, which we should use for
matching. The selector property should be used only to get the actual (escaped)
CSS selector to inject.

[Sebastian Noack, 2018/07/11 16:47:30]

So far for misusing element-hiding-like syntax for snippets. Just another
example why this analogy lacks (and causes confusion). But well, assuming it's
now established that we go with this obscure syntax, I agree with Manish here
that #@# exceptions rules probably shouldn't apply to snippet filters.

The problem is "x y" could either be a CSS selector targeting an element with
the name "y" inside another element with the name "x", or it could mean snippet
"x" with argument "y".

[Manish Jethani, 2018/07/11 16:55:05]

I'm not sure I agree with that, but since we are undecided, and we don't need
exceptions to work in the initial version, we can postpone the decision.
I highly doubt that this will be a problem in practice, but we could also just
have a naming convention for snippet functions so they never clash with CSS
selectors. For example simply following the practice of surrounding all snippet
function names with single quotes, in the snippet filter itself (which is
supported by the syntax), would be enough.

[Manish Jethani, 2018/07/11 17:12:10]

To give some background on this, the initial version is an "alpha" or even
"pre-alpha" if you will. It will only be used by our own filter list authors (we
will discard snippet filters in any other filter lists [1]). This means the
syntax can be changed, everything can be changed easily. We should really stop
worrying about this right now.

When we have developed the feature fully and we know that it is giving results
(which we can only know if we try it out in production), then we can flip the
switch and make it available to all filter list authors. Until then, we have
time to figure out the details.

[1]: https://codereview.adblockplus.org/29824589/

[kzar, 2018/07/11 17:17:51]

OK, seems the vote is #@# exception filters shouldn't apply to snippet filters
for now. Please just add a note about this in the issue to explain the
situation, and ideally link to an issue about adding the #@$# or similiar
syntax.

[kzar, 2018/07/11 17:17:51]

Well I was already in the process of replying... I can only do a review so fast.

+ *   $ (snippet)
+ * @param {string} script
+ *   script part of the text representation, either a CSS selector or a snippet
+ *   script
  * @return {ElemHideFilter|ElemHideException|
- *          ElemHideEmulationFilter|InvalidFilter}
+ *          ElemHideEmulationFilter|SnippetFilter|InvalidFilter}
  */
-ElemHideBase.fromText = function(text, domains, type, selector)
+ContentFilter.fromText = function(text, domains, type, script)
 {
-  // We don't allow ElemHide filters which have any empty domains.
-  // Note: The ElemHide.prototype.domainSeparator is duplicated here, if that
-  // changes this must be changed too.
+  // We don't allow content filters which have any empty domains.
+  // Note: The ContentFilter.prototype.domainSeparator is duplicated here, if
+  // that changes this must be changed too.
   if (domains && /(^|,)~?(,|$)/.test(domains))
     return new InvalidFilter(text, "filter_invalid_domain");
 
   if (type == "@")
-    return new ElemHideException(text, domains, selector);
+    return new ElemHideException(text, domains, script);
+
+  if (type == "$")
+    return new SnippetFilter(text, domains, script);
 
   if (type == "?")
   {
     // Element hiding emulation filters are inefficient so we need to make sure
     // that they're only applied if they specify active domains
     if (!/,[^~][^,.]*\.[^,]/.test("," + domains))
       return new InvalidFilter(text, "filter_elemhideemulation_nodomain");
 
-    return new ElemHideEmulationFilter(text, domains, selector);
+    return new ElemHideEmulationFilter(text, domains, script);
   }
 
-  return new ElemHideFilter(text, domains, selector);
+  return new ElemHideFilter(text, domains, script);
 };
 
 /**
+ * Base class for element hiding filters
+ * @param {string} text see {@link Filter Filter()}
+ * @param {string} [domains] see {@link ContentFilter ContentFilter()}
+ * @param {string} selector  CSS selector for the HTML elements that should be
+ *                           hidden
+ * @constructor
+ * @augments ContentFilter
+ */
+function ElemHideBase(text, domains, selector)
+{
+  ContentFilter.call(this, text, domains, selector);
+}
+exports.ElemHideBase = ElemHideBase;
+
+ElemHideBase.prototype = extend(ContentFilter, {
+  /**
+   * CSS selector for the HTML elements that should be hidden
+   * @type {string}
+   */
+  get selector()
+  {
+    // Braces are being escaped to prevent CSS rule injection.
+    return this.script.replace("{", "\\7B ").replace("}", "\\7D ");
+  }
+});
+
+/**
  * Class for element hiding filters
  * @param {string} text see {@link Filter Filter()}
  * @param {string} [domains]  see {@link ElemHideBase ElemHideBase()}
  * @param {string} selector see {@link ElemHideBase ElemHideBase()}
  * @constructor
  * @augments ElemHideBase
  */
 function ElemHideFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
@@ skipping 32 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });
+
+/**
+ * Class for snippet filters
+ * @param {string} text see Filter()
+ * @param {string} [domains] see ContentFilter()
+ * @param {string} script    Script that should be executed
+ * @constructor
+ * @augments ContentFilter
+ */
+function SnippetFilter(text, domains, script)
+{
+  ContentFilter.call(this, text, domains, script);
+}
+exports.SnippetFilter = SnippetFilter;
+
+SnippetFilter.prototype = extend(ContentFilter, {
+  type: "snippet"
+});