mattbasta / crass

A CSS minifier and utility library for JavaScript
http://mattbasta.com/crass
MIT License
102 stars 6 forks source link

Upgrade dependencies to fix security issues #70

Open prantlf opened 5 years ago

prantlf commented 5 years ago

I upgraded all dependencies in package.json to their current versions.

The new version of SVGO returns Promises. I'll try to modify crass to do it too. It will be a bigger change and a breaking one.

                       === npm audit security report ===
┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ js-yaml                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.13.0                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ crass                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ crass > svgo > js-yaml                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/788                             │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Code Injection                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ js-yaml                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.13.1                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ crass                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ crass > svgo > js-yaml                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/813                             │
└───────────────┴──────────────────────────────────────────────────────────────┘

found 2 vulnerabilities (1 moderate, 1 high) in 2481 scanned packages
  2 vulnerabilities require manual review. See the full report for details.
mattbasta commented 5 years ago

Thanks for putting this together. It's been on my list for a while, but fairly low priority because of the low risk of exploit (I certainly hope nobody is using crass in a server-side runtime environment!).

I would like to eventually move to an async API. But doing that safely is tricky (lots of remembering to await). I think that change would need to be preceded by a port to TS, which is what almost all of my code is these days anyway.

mattbasta commented 5 years ago

I'll try to comb through this PR soon!

prantlf commented 5 years ago

Yes, it's not going to be trivial. I'm afraid, that I posted this PR too early with too little work... Having the optimize method synchronous is very convenient for easier coding. It's also an interface used by all nodes. The broad usage of this method makes the change bigger, than I initially thought.

The only method, which really needs to be asynchronous is optimizeDataURI because of the usage of SVGO. I was playing with the idea of returning the promise only from there and when any of the optimize methods is called, deciding on what to do by testing the result with instanceof Promise. I'm not sure, if it'd make the work simpler.

And you're right, the risk is low. It's just that npm starting to provoke me by that audit report :-)

I tried a "hotfix" by forking the last synchronous svgo@0.7.2 to @prantlf/svgo and depending on it. It's a zero-effort change in crass to get rid of the security warning, but depending on an old package wasn't not the final solution. If you're interested, I could open a PR with that as a temporary "silencing" the npm audit.

mattbasta commented 5 years ago

I took a bit of time to do some work this morning. Namely, I've done the following:

To get svgo, the plan is to make pretty, optimize, and all the associated helper functions (that have side effects or use async code) async. Which actually doesn't seem so bad; most of it is going to be some find-and-replace and slapping await in a bunch of spots. I'll see about doing that soon.