simularium / simularium-website

Front end website for the Simularium project, includes the Simularium viewer
https://simularium.allencell.org
Apache License 2.0
6 stars 3 forks source link

Fix npm dependency security vulnerabilities #191

Closed schoinh closed 3 years ago

schoinh commented 3 years ago

What needs to happen?

Currently there are 13 vulnerabilities (3 low, 8 moderate, 2 high) in our npm dependencies. We should try to fix at least the high severity vulnerabilities.

image

Why should we do this?

So that we're not susceptible to a ReDoS attack? What are the chances we'd be impacted by something like that though?

When does this need to get done?

I'm not sure

schoinh commented 3 years ago

dat.gui

@toloudis Looks like there is no fix for the dat.gui vulnerability. If we want to keep using it for simularium-viewer, can we move dat.gui to devDependencies in simularium-viewer? That way at least it won't show up as a vulnerability in simularium-website? I'm not even sure if that's actually helpful or not security-wise. But I know we don't need dat.gui in simularium-website.

glob-parent

We removed copy-webpack-plugin because it wasn't being used, but webpack-dev-server still uses the vulnerable version of glob-parent. The good news is it looks like they're fixing it for their upcoming release, so we just need to upgrade to v4 when it gets released.

minimist

The minimist vulnerability has been removed since we stopped using mocha.

toloudis commented 3 years ago

Notes on dat.gui: this lib is being used to put up a quick set of sliders for tweaking internal render parameters on the viewer. This gui is never shown in production code - it could be factored out (as a devDependency like you suggest) but then it would never be available when the viewer is embedded into simularium-website. If we are fine with that, I can make it happen. An alternative is to look for another thing to replace it. The only reason to have it available in simularium-website would be to tweak these same rendering parameters in the context of the full site, or maybe with trajectories that aren't as readily available in the viewer testbed.

schoinh commented 3 years ago

My thought process was, since we're not using dat.gui in the simularium-website context currently and have not done so recently (as far as I know), we can stick it under devDependencies in the viewer, and can move it to dependencies if we ever need to. But I don't feel too strongly about this issue. Like I mentioned above, I'm iffy on how seriously I should take these warnings! I just get annoyed seeing them every time I npm i 🤦🏻

meganrm commented 3 years ago

I agree this should be a dev dependency. We can test it out, but I'm pretty sure even if it is a dev dependency you can install it yourself in development when using similuarium-website and the app will find it in the node_modules. It's just that it won't be installed by default.

toloudis commented 3 years ago

Yeah - I don't like seeing big red flags either, and I am all for making the package smaller. Also, dat.gui is kind of old and dead. It hasn't seen any kind of updates in a long while. It just happens to be super easy and convenient (and still kind of widely used) but there are other things like it that are newer and similarly simple. I guess my instinct would be to move to something like tweakpane which looks like a very similar api and is both prettier and more actively developed.

If I were going to make it a devDependency, I'd have the test app import it and either pass it in to the main code as a generic object or else just have it blatantly write to internal variables from the outside.

schoinh commented 3 years ago

If tweakpane or any other newer/better library can replace dat.gui without too much trouble it seems like that would be ideal!

schoinh commented 3 years ago

Nothing that can be done currently, so closing!