1496680 - Usage of Function('return this') in Debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, task, P5)

Description

[Vinothkumar Nagasayanan [:vinoth]]

Usage of new Function() along with system principal is being removed from everywhere as part of Bug 1473549.

Files redux.js, react-redux.js, lodash.js, vendors.js, search-worker.js and parser-worker.js use new Function() to get the 'this' object. This should be replaced with secure alternatives.

Comment 1

[Vinothkumar Nagasayanan [:vinoth]]

Hi Mike,

These files use this new Function(), 
https://dxr.mozilla.org/mozilla-central/search?q=var+root+%3D+freeGlobal+%7C%7C+freeSelf+%7C%7C+Function(%27return+this%27)()%3B+ext%3Ajs&redirect=false

We have to replace it with secure alternatives. Seems like you made changes to these files in Bug 1441147. Please let us know the possibilities of changing these vendor codes and is it possible to directly make changes to all these files redux.js, react-redux.js, lodash.js, vendors.js, search-worker.js and parser-worker.js?

Please let me know if you have any idea on these or please direct us to the right person in the Devtools team.

Thanks.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Sounds like you need to speak with Jason Laster.

Comment 3

[Logan Smyth [:loganfsmyth]]

Jason is out on PTO for the next 3 weeks FYI. I'm still new to this codebase, so I can't 100% confidently say much, but I can try to keep this conversation moving.

That code is pulled in from Lodash: https://github.com/lodash/lodash/blob/6018350ac10d5ce6a5b7db625140b82aeab804df/.internal/root.js#L7. That's a very common way to get a reference to a global, since there is no standardized way to do that yet, though hopefully one will exist soon.

Is this logic actually triggering runtime errors when restrictions on `Function()` are put in place? I'm surprised because I would assume that the `freeSelf` check would shortcircuit any calls to `Function()` in that context.

Comment 4

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Logan Smyth [:loganfsmyth] from comment #3)
> 
> Is this logic actually triggering runtime errors when restrictions on
> `Function()` are put in place? I'm surprised because I would assume that the
> `freeSelf` check would shortcircuit any calls to `Function()` in that
> context.

Yes this logic is triggering assertion errors for the following files,

[1] - lodash.js
https://dxr.mozilla.org/mozilla-central/rev/b0b856065d5b7ad2996f707e6e797d0d72afd803/devtools/client/shared/vendor/lodash.js#423

[2] - react-redux.js
https://dxr.mozilla.org/mozilla-central/rev/b0b856065d5b7ad2996f707e6e797d0d72afd803/devtools/client/shared/vendor/react-redux.js#1536

[3] - redux.js
https://dxr.mozilla.org/mozilla-central/rev/b0b856065d5b7ad2996f707e6e797d0d72afd803/devtools/client/shared/vendor/redux.js#18

Few test files that doesnt use 'freeSelf' check but use Function('return this')() are,

1) toolkit/components/extensions/test/mochitest/test_ext_background_page.html
2) dom/security/test/general/browser_test_FTP_console_warning.js
3) browser/components/extensions/test/browser/browser_ext_addon_debugging_netmonitor.js

A similar Bug 1486375 was using eval to get the 'this', but it has been replaced now.
Please kindly let me know if you have alternatives to fix these files.

Thanks.

Comment 5

[Jason Laster [:jlast]]

So it looks like react-redux and redux pull in Lodash's root module [1], which is why the three examples are equivalent.

As a general rule of thumb, I would prefer not to modify 3rd-party code. Would it be possible to follow up with Lodash folks and see what they think?


[1] https://github.com/lodash/lodash/blob/4ea8c2ec249be046a0f4ae32539d652194caf74f/.internal/root.js#L7

Comment 6

[Jason Laster [:jlast]]

I filed an issue in lodash to see what others think (https://github.com/lodash/lodash/issues/4066). Lets see if we can find some good alternatives!

Comment 7

[Jason Laster [:jlast]]

Vinothkumar, I understand that eval's in general could be dangerous, but is there anything particularly dangerous about `Function("return this")`? 

I'm asking because if this case is safe, perhaps we can whitelist it given it is used by Lodash, which is a top 5 used library on npm.

Comment 8

[Logan Smyth [:loganfsmyth]]

Can we create a `self` into the global manually in the contexts where it isn't already available? It seems like, as long as we create it before `lodash` loads, it would short-circuit before calling `Function('return this')` and I assume avoid the issue. e.g.

> if (typeof self === "undefined") {
>   self = function(){ return this; }();
> }

In the long run we'll standardize on 'globalThis' and this will resolve itself, so hopefully this would only be needed until then.

Comment 9

[Jason Laster [:jlast]]

Routing @jdalton's question from github

> How do you import scripts into the privileged environment and what can be used as the global object reference?

Comment 10

[Jason Laster [:jlast]]

Is this still relevant?

Comment 11

[Jonas Allmann [:jallmann]]

This bug has become obsolete. All issues were resolved in different bugs under the meta Bug 1473549.