1486375 - Remove eval from Redux.jsm

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P2)

Description

[Vinothkumar Nagasayanan [:vinoth]]

Usage of eval function along with system principal is being removed from everywhere as part of Bug 1473549. Here in redux.jsm, eval should be removed and new way should be used to access the global 'this' object.

Comment 1

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug 1486375 - usage of eval removed from redux.jsm

Comment 2

[Johann Hofmann [:johannh]]

Considering that the library has a try..catch around their use of Function and eval to check for CSP restrictions, do we even need to make changes to it?

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/browser/components/newtab/vendor/Redux.jsm#695

Note that I think Redux.jsm is a vendored form of the Redux library (for React stuff) used by Activity Stream, so there's no point in landing fixes in m-c, since they will be overwritten by

a) new versions of the library
b) the fact that AS people do chunked multi-patch imports into m-c https://bugzilla.mozilla.org/show_bug.cgi?id=1446053

It's probably best if you reach out by creating an issue here: https://github.com/mozilla/activity-stream

(In general I think it's preferable to open these bugs in the relevant components that utilize the code we want to remove rather than in Core::DOM:Security)

Comment 3

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Considering that the library has a try..catch around their use of Function
> and eval to check for CSP restrictions, do we even need to make changes to
> it?
> 
> https://searchfox.org/mozilla-central/rev/
> e126996d9b0a3d7653de205898517f4f5b632e7f/browser/components/newtab/vendor/
> Redux.jsm#695
> 
> Note that I think Redux.jsm is a vendored form of the Redux library (for
> React stuff) used by Activity Stream, so there's no point in landing fixes
> in m-c, since they will be overwritten by
> 
> a) new versions of the library
> b) the fact that AS people do chunked multi-patch imports into m-c
> https://bugzilla.mozilla.org/show_bug.cgi?id=1446053
> 
> It's probably best if you reach out by creating an issue here:
> https://github.com/mozilla/activity-stream
> 
> (In general I think it's preferable to open these bugs in the relevant
> components that utilize the code we want to remove rather than in
> Core::DOM:Security)

Thanks for the info. I got it. Will open an issue in https://github.com/mozilla/activity-stream.

Comment 4

[Vinothkumar Nagasayanan [:vinoth]]

Hey Ed lee,

In https://github.com/mozilla/activity-stream/blob/master/vendor/Redux.jsm#L695 , Eval and new Function is used to get the global this object and it is executed with system principal. Hence this should be removed as part of Bug 1473549.

Since redux.jsm is vendor specific and used by activity stream please let me know, if you know someone from the activity-stream team to make these changes.

Thanks.

Comment 5

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Considering that the library has a try..catch around their use of Function
> and eval to check for CSP restrictions, do we even need to make changes to
> it?

CSP used in about:newtab restricts eval functions but still assertion is triggered. I am investigating on why this is triggered.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Hey Chris, we would like to forbid eval() in system privileged contexts and land an assertion that we can not introduce it. We identified that there is one usage within ActivityStream, in particular in the File Redux.jsm (for details see comment 2).

Can you route this ni? request to the right people so that we can potentially remove the eval() from that file and replace with something else? Thanks!

Comment 7

[Ed Lee :Mardak]

k88hudson originally added Redux.jsm and can probably answer the eval question. We've had custom modifications to Redux.jsm with bug 1399997, which seems to want to do the same thing as this bug ? Although the fix there was:

```diff
 this.EXPORTED_SYMBOLS = ["redux"];
+
+// Defining these prevents redux from using indirect eval or `new
+// Function()` to get its global object.
+const self = this;
+this.Object = Object;
+
 this.redux =
```

Alternatively to getting thisObject in attachment 9004150 [details], why not just

```
/* 6 */
/***/ (function(module, exports) {

module.exports = this;
```
?

Comment 8

[Kate Hudson :k88hudson]

I imported this code directly from Redux (except for the EXPORTED_SYMBOLS bit), it seems to be the result of a combination of Redux and webpack. They've since upgraded their version of webpack so many it's not a problem anymore? 

I would rather try to upgrade this / repack it to fix the issue so that we don't run into it again when upgrading, rather than edit the output.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kate Hudson :k88hudson from comment #8)
> I imported this code directly from Redux (except for the EXPORTED_SYMBOLS
> bit), it seems to be the result of a combination of Redux and webpack.
> They've since upgraded their version of webpack so many it's not a problem
> anymore? 
> 
> I would rather try to upgrade this / repack it to fix the issue so that we
> don't run into it again when upgrading, rather than edit the output.

Hey Kate, thanks for your response. I am not entirely sure what this means in terms of moving forward. Are you saying we should try to upgrade redux and hope that the eval() code disappeared? If so, where and how would we do that?

Comment 10

[Ed Lee :Mardak]

The latest version of Redux 4.0.1 seems to still have `  root = Function('return this')();`

https://unpkg.com/redux@4.0.1/dist/redux.js

Although 3.6.0 which is what's in Redux.jsm doesn't seem to have the function wrapper for module/global detection perhaps added by webpack?

https://unpkg.com/redux@3.6.0/dist/redux.js

k88hudson, were you suggesting with newer Redux and newer webpack, there could be an optimization step that removes the Function/eval?

Comment 11

[Kate Hudson :k88hudson]

Indeed, looks like their distribution still has the reference to Function. I was hoping maybe that would be removed, but oh well.

Let's do the upgrade and just remove it manually.

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/509c8be33765b867061a8fded7561541897a276e
Fix Bug 1486375 - Upgrade redux and react redux manually removing Function (#4530)

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kate Hudson :k88hudson from comment #11)
> Let's do the upgrade and just remove it manually.

Thanks Kate!

Comment 14

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/49d47a692ca4

Comment 15

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 9004150 [details]
Bug 1486375 - usage of eval removed from redux.jsm

Changes are made in redux.jsm. Hence marking this as obsolete.