Upgrading dependencies to use static-eval 2 in cwise

[archmoj]

This PR resolves https://www.npmjs.com/advisories/548.
It is also tested on a Plotly PR in different modules namely ndarray-fill.

[etpinard]

@archmoj could you patch the .travis.yml to show

node_js:
  - 6
  - 8
  - 10

i.e. run the tests in node 6, 8 and 10. This PR will of course be part of the major version bump.

[archmoj]

@etpinard Thanks for the instruction. The first attempt was not successful. But when I added acorn to dependencies it passed those tests. Now I would try to see if we may add it to devDependencies and pass the tests.

[archmoj]

@etpinard OK. Acorn could be located in dev-dependencies.

[etpinard]

Thanks @archmoj !

Let's wait a couple days to see if other cwise maintainers have an opinion on this patch, and/or other things they would like to make part of cwise@2.0.0.

[archmoj]

@etpinard Would you please merge this and possibly publish new version?

[etpinard]

I'll wait until Wednesday at the latest. Last Thursday and Friday were holidays in the US.

[etpinard]

@archmoj turns out I have write rights on Github, but I don't have publish rights on npm. So even if we merge this, we won't be able to publish the changes.

cc @rreusser and @mikolalysenko

[rreusser]

Although the tests passes and the transform code itself works, it looks like this static-module does not function correctly, resulting in about an extra 120kb of esprima parser code in each bundle.

The problem appears to be present in version 3.0.0 of static-module (not sure about 2.*). It looks like maybe it doesn't deal well with bare module functions (e.g. require('cwise')(...)) as opposed to functions which are properties on the module (e.g. require('fs').readFileSync(...)). That's a bit of a guess/inference, but it does seem like there's a problem in this area.

Testing strategy:

First of all, remember to rm -rf node_modules and reinstall if you switch branches!

Within this repo, I created a file browserifytest/index.js:

// browserifytest/index.js:
var cwise = require('cwise');
var addeq = cwise({args: ["array", "array"], body: function(a, b) { a += b }}) 

I added a browserifytest/package.json with symlinked cwise:

// browserifytest/package.json:
{
  "dependencies": {
    "cwise": "../"
  }
}

This way, cwise gets resolved to the cloned repo, as required in order to actually use static-module. (I had to mkdir -p browserifytest/node_modules and symlink ln -s browserifytest/node_modules/cwise ./ since npm install in this directory was being a little weird).

Finally, I created a script to browserify it:

// browserifytest/browserify.js
require('browserify')()
  .add('./index.js')
  .transform('../cwise.js')
  .bundle()
  .pipe(process.stdout);

Then you can run:

$ node browserify.js |grep "cwise/lib/wrapper"

If this line is present, it's been static-module-transformed correctly. The master branch correctly transforms the code and removes esprima, while the archmoj:upgrade-dep does not.

I wish I had a cleaner test case, but these things are just a bit awkward to test since require('./') is completely different from require('cwise') as far as static-module is concerned.

It'd be great to fix the security issue, but I think we probably need to figure out what's going on here first.

[etpinard]

Thanks very much @rreusser for looking into this!

[adarob]

How about just upgrading to 2..?

[rreusser]

@adarob Version 3.0 is current, but it was version 2.0 which broke compatibility. I'm sure a workaround can be constructed, but the behavior in question is pretty much at the center of what cwise does so that it doesn't seem trivial and would mean thinking carefully about what cwise is and how it works. I do try to keep the wheels turning here, but it's not currently something I have the ability to work on. See: browserify/static-module#48 (comment)

[Domino987]

Any news on this?

[mhnsn]

Seriously. We depend on plotly for a product, and can't afford arbitrary code execution vulnerabilities, since we're opening up our API.

[etpinard]

@mhnsn some info on the topic that you might find relevant:

• static-eval only gets invoked when bundling plotly.js
• every plotly.js partial bundle has a corresponding npm "dist" package with no dependency (and no vulnerability 😏 ) e.g plotly.js-dist, plotly.js-basic-dist

[cjwainwright]

@etpinard The “dist” packages do not include minified plotly code that can be included by script tag. Is there anything other than the full plotly.js package that does? Which would allow bypassing this vulnerability without adding a new build step.

[etpinard]

The “dist” packages do not include minified plotly code that can be included by script tag.

You're right, we don't include the minified version, still the files in each of the "dist" packages are bundled up and effectively bypass the build step. We don't include minified bundles in the "dist" packages to keep the amount of bytes to download to a minimum.

s there anything other than the full plotly.js package that does?

If you want a minified file w/o having to minify it yourself, you can use our CDN bundles ending with min.js e.g. https://cdn.plot.ly/plotly-basic-latest.min.js

[cjwainwright]

@etpinard Thanks for explaining. Our use case is to have npm pull in third party libraries, and just copy the relevant files to our dist folder. For libraries like jQuery etc the minified file is present so we can just copy directly without any further processing. Using direct from the CDN is not feasible in our case.

I had already gone ahead and investigated if it was possible to add the minified files to the packages, I have a pull request (on my own fork) if you're interested in taking a look: cjwainwright/plotly.js#1 But I understand if this is a deliberate choice to exclude the minified files then we will have to find a different way.

[kibertoad]

@etpinard Is anything left before this can be merged?

[nicolaskruchten]

Hi folks,

Is there any path towards getting the security issue sorted out in this repo? If not, we will likely have to inline the entire dependency path to this module into the plotly.js tree to bypass it ourselves, and thus in effect do a fork of a bunch of stuff :(

[nicolaskruchten]

OK so @archmoj has submitted browserify/static-eval#31, which if merged would allow us to resolve browserify/static-module#48 such that this current issue can be resolved, such that https://www.npmjs.com/advisories/758 would no longer apply to Plotly.js, resolving plotly/plotly.js#4796 and clearing up plotly/plotly.py#2386 and plotly/plotly.py#2385 and plotly/jupyterlab-chart-editor#47

[nicolaskruchten]

Update: we've resolved the downstream issues by having plotly.js no longer depend on cwise.

[archmoj]

Update: we've resolved the downstream issues by having plotly.js no longer depend on cwise.

For more info: https://github.com/plotly/plotly.js/releases/tag/v1.54.4