Issue 6538, 6781 - Implement script compilation for snippets

lib/snippets.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
  * 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/>.
  */
 
 "use strict";
 
 /**
  * @fileOverview Snippets implementation.
  */
 
-const {ElemHide} = require("./elemHide");
 const {Filter} = require("./filterClasses");
 
 let filters = new Set();
 
 /**
  * Container for snippet filters
  * @class
  */
 let Snippets = {
   /**
@@ skipping 18 matching lines @@
    * @param {SnippetFilter} filter
    */
   remove(filter)
   {
     filters.delete(filter.text);
   },
 
   /**
    * Returns a list of all scripts active on a particular domain
    * @param {string} domain
-   * @return {SnippetFilter[]}
+   * @return {string[]}
    */
   getScriptsForDomain(domain)
   {
     let result = [];
     for (let text of filters)
     {
       let filter = Filter.fromText(text);
-      if (filter.isActiveOnDomain(domain) &&
-          !ElemHide.getException(filter, domain))
-      {
-        result.push(filter);
-      }
+      if (filter.isActiveOnDomain(domain))
+        result.push(filter.script);
     }
     return result;
-  },
-
-  /**
-   * Parses a script and returns a list of all its commands and their arguments
-   * @param {string} script
-   * @return {Array.<string[]>}
-   */
-  parseScript(script)
-  {
-    let tree = [];
-
-    let escape = false;
-    let literal = false;
-
-    let call = [];
-    let argument = "";
-
-    for (let character of [...script.trim(), ";"])
-    {
-      if (escape)
-      {
-        escape = false;
-
-        argument += character;
-      }
-      else if (character == "\\")
-      {
-        escape = true;
-      }
-      else if (character == "'")
-      {
-        literal = !literal;
-      }
-      else if (literal || character != ";" && !/\s/u.test(character))
-      {
-        argument += character;
-      }
-      else
-      {
-        if (argument)
-        {
-          call.push(argument);
-          argument = "";
-        }
-
-        if (character == ";" && call.length > 0)
-        {
-          tree.push(call);
-          call = [];
-        }
-      }
-    }
-
-    return tree;
-  },
-
-  /**
-   * Compiles a script against a given list of libraries into executable code
-   * @param {string} script
-   * @param {string[]} libraries
-   * @return {string}
-   */
-  compileScript(script, libraries)
-  {
-    return `
-      "use strict";
-      {
-        const libraries = ${JSON.stringify(libraries)};
-
-        const script = ${JSON.stringify(Snippets.parseScript(script))};
-
-        let imports = Object.create(null);

[Manish Jethani, 2018/04/26 13:46:00]

There's an imports object and it gets populated by the "library". The library is
just a JavaScript module like the following:

  // my-lib.js
  exports.foo = function()
  {
    console.log("foo");
  };

Where does my-lib.js get the exports object? We pass our "imports" object to it
as an argument, which becomes its exports object. This way we load multiple
libraries and all of them populate the same exports object.

The reason for supporting multiple libraries is that we want to have an initial
local library (bundled with the extension) and then have a remote library loaded
and updated at regular intervals, which overrides the local library. Also at
some point we might want to split up our snippets into multiple libraries for
organization purposes (and to make the downloads smaller so you only download
what you need).

-        for (let library of libraries)
-          new Function("exports", library)(imports);
-
-        for (let [name, ...args] of script)
-        {
-          if (Object.prototype.hasOwnProperty.call(imports, name))
-          {
-            let value = imports[name];

[Manish Jethani, 2018/04/26 13:46:00]

Note that the function must exist directly on the imports object, hence no
chance of calling window.eval and things like that.

-            if (typeof value == "function")
-              value(...args);

[Manish Jethani, 2018/04/26 13:46:00]

Also note that the function is called without a "this". It still has access to
its file scope.

  //my-lib.js
  let foo = 0;
  exports.setFoo = function(value)
  {
    foo = value;
  }

Calling setFoo here without a "this" will still set the value of foo. This is
explicitly tested in the unit tests.

Each script execution happens with a new instance of the library so the value of
foo won't be retained between script executions. This is also explicitly tested
in the unit tests.

Basically we want things to be as stateless as possible; script execution on one
site should not have any effect on other sites where the same script executes.
State can be shared within the same script (e.g. "setFoo 123; checkFoo 123") but
this is only incidental and not by design. We won't have any global variables in
our snippets library so there will be nothing to share in practice.

-          }
-        }
-      }
-    `;
   }
 };
 
 exports.Snippets = Snippets;
+
+/**
+ * Parses a script and returns a list of all its commands and their arguments
+ * @param {string} script
+ * @return {Array.<string[]>}
+ */
+function parseScript(script)
+{
+  const singleCharacterEscapes = new Map([
+    ["n", "\n"], ["r", "\r"], ["t", "\t"]
+  ]);
+
+  let tree = [];
+
+  let escape = false;
+  let literal = false;
+
+  let unicodeEscape = null;
+
+  let call = [];
+  let argument = "";
+
+  for (let character of [...script.trim(), ";"])
+  {
+    if (unicodeEscape != null)
+    {
+      unicodeEscape += character;
+
+      if (unicodeEscape.length == 4)
+      {
+        let codePoint = parseInt(unicodeEscape, 16);
+        if (!isNaN(codePoint))
+          argument += String.fromCodePoint(codePoint);
+
+        unicodeEscape = null;
+      }
+    }
+    else if (escape)
+    {
+      escape = false;
+
+      if (character == "u")
+        unicodeEscape = "";
+      else
+        argument += singleCharacterEscapes.get(character) || character;
+    }
+    else if (character == "\\")
+    {
+      escape = true;
+    }
+    else if (character == "'")
+    {
+      literal = !literal;
+    }
+    else if (literal || character != ";" && !/\s/u.test(character))
+    {
+      argument += character;
+    }
+    else
+    {
+      if (argument)
+      {
+        call.push(argument);
+        argument = "";
+      }
+
+      if (character == ";" && call.length > 0)
+      {
+        tree.push(call);
+        call = [];
+      }
+    }
+  }
+
+  return tree;
+}
+
+exports.parseScript = parseScript;
+
+/**
+ * Compiles a script against a given list of libraries into executable code
+ * @param {string} script
+ * @param {string[]} libraries
+ * @return {string}
+ */
+function compileScript(script, libraries)
+{
+  return `
+    "use strict";
+    {
+      const libraries = ${JSON.stringify(libraries)};
+
+      const script = ${JSON.stringify(parseScript(script))};
+
+      let imports = Object.create(null);
+      for (let library of libraries)
+        new Function("exports", library)(imports);

[kzar, 2018/07/10 14:35:15]

I wonder why we take include all libraries, instead of the ones that are being
used?

[kzar, 2018/07/10 14:35:15]

If I got it right, `compileScript` has a string representation of the library
function, which we then inject into the generated code as a string using
JSON.stringify, before evaling that string using the Function constructor.
Couldn't we instead omit the JSON.stringify step, so that the library function
was injected into the code as a function?

[Manish Jethani, 2018/07/12 09:48:31]

The idea is that there will be a local library, which is shipped with the
extension, and then there will be a remote version downloaded periodically. The
remote version will contain overrides of functions in the local library. If
there is no override, the local version of the function will be used.

Anyway right now there is only one library.

[Manish Jethani, 2018/07/12 09:48:32]

libraries is an array of strings. The only way to safely encode a string into
another string is using JSON.stringify. This function returns a string; it needs
to include the original library code, but the code must be a string literal
which we pass to the Function constructor.

As for why we need the Function constructor:

 1.  It is safer, because the library code runs in global context and therefore
cannot affect the surrounding variables
 2.  It's a neat way for the library to export functions into our imports object
here

[kzar, 2018/07/12 10:21:43]

I still don't see the point of stringifying it, just to eval it again. Unless
I'm mistaken we're doing something like this (contrived and untested, but you
get the idea):

let libraries = ["console.log("example");"];
eval(`
  let codeString = ${JSON.stringify(libraries)}[0];
  eval(codeString);
`);

But I figure we could juts do something like this:

let libraries = ["console.log("example");"];
eval(libraries[0]);

[Manish Jethani, 2018/07/12 11:05:29]

The JSON.stringify is what converts your first snippet into your second snippet.
The return value of this function is exactly what you are suggesting we should
do, but to do that we need to insert the library code into the string as a
literal. i.e. we are not actually calling eval, we are constructing a string
that contains code that calls eval. The eval call is elsewhere, not in this
function.

Also an important point regarding eval: eval would execute the code in the local
scope (so it could mess with local variables), which is what we don't want,
that's why we use the Function constructor.

[kzar, 2018/07/12 11:19:55]

It's really not, but nevermind.
Yes, I just used `eval` in my example for brevity, I don't have a strong opinion
to use `eval` vs the `Function` constructor.

[Manish Jethani, 2018/07/12 11:35:16]

OK, perhaps I am misunderstanding. If you can rewrite the function to not use
JSON.stringify, it would help me understand what you mean.
Acknowledged.

[Sebastian Noack, 2018/07/16 19:14:53]

So we are generating code that generates code just for the matter that we can
use the Function constructor? Are there any (significant) benefits over inlining
the library code here? I could imagine that this would be somewhat faster, and
might make things slightly easier to debug.

[Manish Jethani, 2018/07/17 19:57:15]

The library looks like this:

https://github.com/adblockplus/adblockpluscore/blob/master/lib/content/snippe...

It's a JavaScript module that exports functions.

I suppose we could write something like this:

  return `
    let exports = Object.create(null);

    ${libraries.map(library => `(function(){${library}})()`).join(";")};

    // use exports
  `;

But this is both uglier and more prone to error, the latter because the library
would be able to access and modify state in the surrounding context.

+
+      for (let [name, ...args] of script)
+      {
+        if (Object.prototype.hasOwnProperty.call(imports, name))

[kzar, 2018/07/10 14:35:15]

Why not `name in imports`, you created the `imports` Object with no prototype.

[Manish Jethani, 2018/07/12 09:48:31]

It's just safer this way. Right now it's a null prototype, maybe later somebody
modifies the code so it's a regular object or an instance of some class. I'm
trying to avoid accidental security issues as much as possible. There's no harm
in doing it this way, on the other hand.

[kzar, 2018/07/12 10:21:43]

Alright, I disagree but won't insist.

[Manish Jethani, 2018/07/12 11:05:29]

Acknowledged.

+        {
+          let value = imports[name];
+          if (typeof value == "function")

[kzar, 2018/07/10 14:35:15]

Could this check ever fail?

[Manish Jethani, 2018/07/12 09:48:31]

Yes, a library could export a "constant" for whatever misguided reason.

  exports.MAX_INTERVAL = 100;

So it's worth having the check.

Basically this function places as little trust in the library code as it must.
If a library is malicious it can still do anything, but at least we can try to
avoid accidental failures.

[kzar, 2018/07/12 10:21:43]

Acknowledged.

+            value(...args);
+        }
+      }
+    }
+  `;
+}
+
+exports.compileScript = compileScript;