crass icon indicating copy to clipboard operation
crass copied to clipboard

Upgrade dependencies to fix security issues

Open prantlf opened this issue 5 years ago • 4 comments

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.

prantlf avatar Jun 21 '19 13:06 prantlf

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 avatar Jun 21 '19 16:06 mattbasta

I'll try to comb through this PR soon!

mattbasta avatar Jun 21 '19 16:06 mattbasta

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 [email protected] 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.

prantlf avatar Jun 21 '19 17:06 prantlf

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

  • Bumping the version to 1.0.0; it feels like I'd might as well do a major version bump if I'm going to change the API
  • Drop node <10
  • Since node <10 is gone, I dropped babel/browserify
  • Swapped mocha out for jest, which I much prefer these days anyway

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.

mattbasta avatar Jul 10 '19 15:07 mattbasta