Issue 6538, 6781 - Implement script compilation for snippets

lib/snippets.js

Index: lib/snippets.js
===================================================================
--- a/lib/snippets.js
+++ b/lib/snippets.js
@@ -148,8 +148,42 @@
       }
     }
   }
 
   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;