Issue 6819 - Shadow globals in snippets

Issue 6819 - Shadow globals in snippets

[Manish Jethani, 2018-07-31 14:17:08]

Patch Set 1

I found a way to achieve what I have described in #6819. Please take a look.

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js
File lib/snippets.js (right):

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:170: for (let name of typeof window != "undefined" ?
Object.keys(window) : [])
Shadow globals on the window object (browser).

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:173: for (let name of typeof global != "undefined" ?
Object.keys(global) : [])
Shadow globals on the global object (Node.js, for testing).

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:178: globals.browser = {
Expose only certain extension APIs.

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:187: globals.document = new Proxy(document, {
Make document object available with restricted access.

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:187: globals.document = new Proxy(document, {
Use a Proxy object here so it is transparent to the calling code (e.g. `document
instanceof HTMLDocument` still works).

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:190: if (property == "defaultView")
Prevent access to the window object.

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:195: return value.bind(target);
Bind the function to the target so its `this` is the target.

https://codereview.adblockplus.org/29843566/diff/29843567/lib/snippets.js#new...
lib/snippets.js:209: let func = new Function("exports", ...Object.keys(globals),
library);
Pass the globals as arguments to the snippet library.

https://codereview.adblockplus.org/29843566/diff/29843567/test/snippets.js
File test/snippets.js (right):

https://codereview.adblockplus.org/29843566/diff/29843567/test/snippets.js#ne...
test/snippets.js:197: let globals = {};
This is copied and pasted from lib/snippets.js (it's getting a bit ridiculous
but I'd still keep it this way for now).

https://codereview.adblockplus.org/29843566/diff/29843567/test/snippets.js#ne...
test/snippets.js:279: // Test sandboxing.
This is where we test the sandboxing. The process object should be set to null
in the snippet so it cannot do anything evil.

[hub, 2018-08-02 17:12:04]

I'm not familiar with Proxy. I don't see anything wrong with this change though.

[Manish Jethani, 2018-08-15 08:40:34]

On 2018/08/02 17:12:04, hub wrote:
> I'm not familiar with Proxy. I don't see anything wrong with this change
though.

A Proxy is basically a transparent wrapper around an object (property access) or
a function (calls). Here we use a Proxy so that `document instanceof
HTMLDocument` still evaluates to true, which would not be the case if we used a
plain object. On the other hand, we don't have to use a Proxy for the browser
object because the client doesn't make any such assumptions about the type of
the object.

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...

[hub, 2018-08-16 01:30:43]

On 2018/08/15 08:40:34, Manish Jethani wrote:
> On 2018/08/02 17:12:04, hub wrote:
> > I'm not familiar with Proxy. I don't see anything wrong with this change
> though.
> 
> A Proxy is basically 

I know. My point is that I'm not confident enough about it. So if Dave can
review as well, I'd feel better.

[Manish Jethani, 2018-08-16 04:56:33]

On 2018/08/16 01:30:43, hub wrote:
> On 2018/08/15 08:40:34, Manish Jethani wrote:
> > On 2018/08/02 17:12:04, hub wrote:
> > > I'm not familiar with Proxy. I don't see anything wrong with this change
> > though.
> > 
> > A Proxy is basically 
> 
> I know. My point is that I'm not confident enough about it. So if Dave can
> review as well, I'd feel better.

Makes sense, thanks.